tx_pool: fix write abort and improve locking for relayable txs
What the developer wrote
Authored by jeffro256
tx_pool: fix write abort and improve locking for relayable txs
What changed, and why it matters
This patch tightens up how Monero's transaction pool records when to next re-broadcast relayable transactions. Previously, a database transaction was held open while iterating over all candidate transactions; now the lock is taken only when there are actual timestamp updates to write, and each update is wrapped in its own try/catch so one failing update does not abort the whole batch. The commit title says it fixes a 'write abort' and improves locking. The change reduces the chance that a transient database error or a single bad metadata update rolls back every scheduled relay update, which could have caused transactions to be re-relayed too soon or too late and potentially leak timing information about Dandelion++ routing.
Treat as a hardening/reliability fix rather than an active critical vulnerability. Review whether swallowed exceptions should be rate-limited or surfaced to metrics, and verify that partial commit semantics do not leave the txpool in an inconsistent state if later updates in the same batch fail after earlier ones committed. No urgent advisory appears warranted from the diff alone.
Security signals we found
transaction-pool locking change
database write abort handling
Dandelion++ relay timing metadata update
narrowing of critical section
exception swallowing on per-update basis
Evidence from the diff
In tx_pool::get_relayable_transactions(), the old code created a LockedTXN around the entire for_all_txpool_txes() scan and the subsequent change_timestamps loop. The new code removes that outer DB lock, collects metadata changes in memory, and only opens a LockedTXN if change_timestamps is non-empty. Each m_blockchain.update_txpool_tx() call is now inside a try/catch that logs and continues on exception, and the DB transaction is committed only if at least one update succeeded. This narrows the critical section, prevents a single failing update from aborting the whole write batch, and avoids holding the DB transaction while iterating the txpool under m_transactions_lock and m_blockchain locks.
Changed components
src/cryptonote_core/tx_pool.cpptransaction pool relay schedulerDandelion++ routing metadataInspect captured patch +25 / −10
diff --git a/src/cryptonote_core/tx_pool.cpp b/src/cryptonote_core/tx_pool.cpp
index 4bc723a..1402ed4 100644
--- a/src/cryptonote_core/tx_pool.cpp
+++ b/src/cryptonote_core/tx_pool.cpp
@@ -810,7 +810,6 @@ namespace cryptonote
CRITICAL_REGION_LOCAL(m_transactions_lock);
CRITICAL_REGION_LOCAL1(m_blockchain);
- LockedTXN lock(m_blockchain.get_db());
txs.reserve(m_blockchain.get_txpool_tx_count());
m_blockchain.for_all_txpool_txes([this, now, &txs, &change_timestamps, &next_check](const crypto::hash &txid, const txpool_tx_meta_t &meta, const cryptonote::blobdata_ref *){
// 0 fee transactions are never relayed
@@ -859,16 +858,32 @@ namespace cryptonote
return true;
}, false, relay_category::relayable);
- for (auto& elem : change_timestamps)
+ if (!change_timestamps.empty())
{
- /* These transactions are still in forward or stem state, so the field
- represents the next time a relay should be attempted. Will be
- overwritten when the state is upgraded to stem, fluff or block. This
- function is only called every ~2 minutes, so this resetting should be
- unnecessary, but is primarily a precaution against potential changes
- to the callback routines. */
- elem.second.last_relayed_time = now + get_relay_delay(elem.second.last_relayed_time, elem.second.receive_time);
- m_blockchain.update_txpool_tx(elem.first, elem.second);
+ LockedTXN db_lock(m_blockchain.get_db());
+ bool made_an_update = false;
+ for (auto& elem : change_timestamps)
+ {
+ /* These transactions are still in forward or stem state, so the field
+ represents the next time a relay should be attempted. Will be
+ overwritten when the state is upgraded to stem, fluff or block. This
+ function is only called every ~2 minutes, so this resetting should be
+ unnecessary, but is primarily a precaution against potential changes
+ to the callback routines. */
+ elem.second.last_relayed_time = now + get_relay_delay(elem.second.last_relayed_time, elem.second.receive_time);
+ try
+ {
+ m_blockchain.update_txpool_tx(elem.first, elem.second);
+ made_an_update = true;
+ }
+ catch (...)
+ {
+ MDEBUG("Got an exception while updating txpool meta for relayable tx " << elem.first << ", ignoring...");
+ continue;
+ }
+ }
+ if (made_an_update)
+ db_lock.commit();
}
m_next_check = time_t(next_check);
Why this scored 41/100
Community notes
Notes can correct, qualify, or add evidence to the AI analysis. Every note shown here has been validated by a human moderator.
The AI analysis stands alone for now. Submit a note if you can add evidence or important context.