Problem: The mempool memory estimate omits the txid set used to retry the initial broadcast of locally submitted transactions, so getmempoolinfo and -maxmempool undercount retained memory.
Fix: Count this set when calculating mempool memory usage.
Track a mempool transaction before and after adding its txid to the unbroadcast set, which currently retains a node without changing reported memory usage.
Include `m_unbroadcast_txids` in `DynamicMemoryUsage()` to count the set nodes retained for locally submitted transactions awaiting their first successful broadcast, with at most one entry per mempool transaction.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35923.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline and AI policy for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | jeanpablojp, bartoli, musaHaruna, instagibbs, ismaelsadeeq, achow101 |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
ACK db21e03c9d50386072a1123568f917ba65dcf348
I have tested the code. m_unbroadcast_txids is a std::set<Txid> that memusage.h handles, 80 bytes per txid on my build, and it was not counted elsewhere in DynamicMemoryUsage(). Dropping the term makes the added test fail (760 <= 760), so it covers the change. Built the merge with master (05a7c47): unit suite, mempool_unbroadcast.py and mempool_persist.py pass.
ACK db21e03c9d50386072a1123568f917ba65dcf348
Cherry-picked both commits on master. Builds on VS2026. the test seems valid and runs fine. I re-checked CTxMempool members, and now it really seems there are no 'big' members left to account for
Tested ACK db21e03
Compared getmempoolinfo before and after the change by submitting three transactions to a regtest node, then requesting each individually from a test peer using GETDATA.
Without the change, usage reported 3320 bytes and remained unchanged as unbroadcastcount decreased. With the change, it initially reported 3560 bytes and decreased by 80 bytes per transaction served, reaching 3320 after all three were served. Transaction count and virtual size stayed unchanged in both runs. These are estimated memory figures on my machine.
Mempool unit tests and mempool_unbroadcast.py, mempool_persist.py, and mempool_limit.py passed.
One non-blocking follow-up: MempoolInfoToJSON() uses GetUnbroadcastTxs().size(), which copies the entire set under the mempool lock just to count it. A count accessor could avoid that copy. This predates the PR and can be handled separately. I can put together a small follow-up PR if that sounds useful.
536 | @@ -537,7 +537,7 @@ BOOST_AUTO_TEST_CASE(MempoolUnbroadcastMemoryUsage) 537 | 538 | auto usage_before{pool.DynamicMemoryUsage()}; 539 | pool.AddUnbroadcastTx(txid); 540 | - BOOST_CHECK_EQUAL(pool.DynamicMemoryUsage(), usage_before); // TODO: Include the retained set node in reported usage 541 | + BOOST_CHECK_GT(pool.DynamicMemoryUsage(), usage_before);
nit: bot suggests tight assertion
BOOST_CHECK_EQUAL(new, before + memusage::MallocUsage(sizeof(memusage::stl_tree_node<Txid>)))
This was deliberate to make the characterization test assertions minimal (otherwise the assertion itself would change in the before/after scenario so we wouldn't be testing the same thing), but if reviewers would prefer that, I can extend it.
ACK db21e03c9d50386072a1123568f917ba65dcf348
risk is ~0 without this fix, but this also seems correct
ACK db21e03c9d50386072a1123568f917ba65dcf348
Straightforward fix.
MempoolInfoToJSON() uses GetUnbroadcastTxs().size(), which copies the entire set under the mempool lock just to count it.
Hah, good find, seems return-value elision does not eliminate it, please open a new PR for this and see if we have other related similar cases in that area. If you can, a small temporary benchmark could be added as a manual reproducer to prove that the compiler doesn't actually eliminate the copy.
ACK db21e03c9d50386072a1123568f917ba65dcf348