Mempool cleanups #12225

1. 
   sdaftuar commented at 4:13 pm on January 19, 2018: member

   Following up on #12127 and #12118, this cleans up a comment that was left incorrect in txmempool.h, and addresses a couple of the observations @TheBlueMatt made about an unnecessary use of queryHashes() and a small information leak when prioritizing transactions.

   Left undone is nuking queryHashes altogether; that would require changing the behavior of the getrawmempool rpc call, which I think I might be in favor of doing, but wanted to save for its own PR.

2. Correct mempool mapTx comment 09754063e0
3. fanquake added the label Mempool on Jan 19, 2018
4. 
   meshcollider commented at 4:23 am on January 20, 2018: contributor

   utACK https://github.com/bitcoin/bitcoin/commit/16a8389bb14a892c204efe0d39580b4c1458e098
5. in src/policy/fees.cpp:986 in 16a8389bb1 outdated

   983@@ -984,10 +984,9 @@ bool CBlockPolicyEstimator::Read(CAutoFile& filein)
   984 void CBlockPolicyEstimator::FlushUnconfirmed(CTxMemPool& pool) {
   985     int64_t startclear = GetTimeMicros();
   986     std::vector<uint256> txids;
   

   ---

   ---

   

   promag commented at 3:58 pm on January 20, 2018:

   Remove?
6. in src/policy/fees.cpp:988 in 16a8389bb1 outdated

   983@@ -984,10 +984,9 @@ bool CBlockPolicyEstimator::Read(CAutoFile& filein)
   984 void CBlockPolicyEstimator::FlushUnconfirmed(CTxMemPool& pool) {
   985     int64_t startclear = GetTimeMicros();
   986     std::vector<uint256> txids;
   987-    pool.queryHashes(txids);
   988-    LOCK(cs_feeEstimator);
   989-    for (auto& txid : txids) {
   990-        removeTx(txid, false);
   991+    LOCK2(pool.cs, cs_feeEstimator);
   992+    for (auto mi=pool.mapTx.begin(); mi != pool.mapTx.end(); ++mi) {
   

   ---

   ---

   

   promag commented at 4:00 pm on January 20, 2018:

   Nit, can early exit loop if mapMemPoolTxs is empty.
7. in src/policy/fees.cpp:992 in 16a8389bb1 outdated

   991+    LOCK2(pool.cs, cs_feeEstimator);
   992+    for (auto mi=pool.mapTx.begin(); mi != pool.mapTx.end(); ++mi) {
   993+        removeTx(mi->GetTx().GetHash(), false);
   994     }
   995     int64_t endclear = GetTimeMicros();
   996     LogPrint(BCLog::ESTIMATEFEE, "Recorded %u unconfirmed txs from mempool in %gs\n",txids.size(), (endclear - startclear)*0.000001);
   

   ---

   ---

   

   promag commented at 4:03 pm on January 20, 2018:

   txids is empty, update logging.
8. 
   promag commented at 4:03 pm on January 20, 2018: member

   What is usually bigger?

   1. CTxMemPool::mapTx
   2. CBlockPolicyEstimator::mapMemPoolTxs

   My suggestion is to loop the shorter map.

9. 
   sdaftuar commented at 5:03 pm on January 22, 2018: member

   @promag After further investigation, it looks like FlushUnconfirmed() doesn’t need the mempool at all, so I re-wrote it a bit.
10. in src/policy/fees.cpp:991 in bd5260956b outdated

    991-        removeTx(txid, false);
    992+    size_t num_entries = mapMemPoolTxs.size();
    993+    // Remove every entry in mapMemPoolTxs
    994+    while (!mapMemPoolTxs.empty()) {
    995+        auto mi = mapMemPoolTxs.begin();
    996+        removeTx(mi->first, false); // this calls erase() on mapMemPoolTxs
    

    ---

    ---

    

    promag commented at 8:56 pm on January 22, 2018:

    IMO decreasing one by one is avoidable when the end result is to be empty. Prefer something like?

    0LOCK(cs_feeEstimator);
    1size_t num_entries = mapMemPoolTxs.size();
    2for (const auto& it = mapMemPoolTxs.begin(); it != mapMemPoolTxs.end(); ++it) {
    3    const auto& stat = it->second;
    4    feeStats->removeTx(stat.blockHeight, nBestSeenHeight, stat.bucketIndex, false);
    5    shortStats->removeTx(stat.blockHeight, nBestSeenHeight, stat.bucketIndex, false);
    6    longStats->removeTx(stat.blockHeight, nBestSeenHeight, stat.bucketIndex, false);
    7}
    8mapMemPoolTxs.clear();
    

    ---

    

    sdaftuar commented at 5:58 pm on January 23, 2018:

    I didn’t want to duplicate the code in removeTx() or figure out how to refactor it to be more efficient for this purpose – since FlushUnconfirmed() is only called in shutdown, I don’t think the performance savings here from more code changes is worth the review/maintenance burden.

    If you’d rather see a bigger refactor here to clean things up, I think I’d prefer to just remove this commit and someone can take it on in a new PR…

    ---

    

    promag commented at 11:52 pm on January 23, 2018:

    I think it’s fine as it is. I’ll take note to measure an alternative implementation.
11. Speca92 approved
12. fee estimator: avoid sorting mempool on shutdown e868b22917
13. Avoid leaking prioritization information when relaying transactions 669c9433cf
14. in src/txmempool.h:244 in bd5260956b outdated

    241@@ -242,14 +242,17 @@ class CompareTxMemPoolEntryByDescendantScore
    242 /** \class CompareTxMemPoolEntryByScore
    243  *
    244  *  Sort by score of entry ((fee+delta)/size) in descending order
    

    ---

    ---

    

    promag commented at 11:56 pm on January 23, 2018:

    Remove +delta?

    ---

    

    sdaftuar commented at 11:03 pm on January 25, 2018:

    Fixed!
15. sdaftuar force-pushed on Jan 25, 2018
16. 
    sdaftuar commented at 11:24 pm on January 25, 2018: member

    Squashed 12225.1 -> 669c943
17. 
    promag commented at 2:48 pm on February 3, 2018: member

    utACK 669c943.
18. 
    laanwj commented at 9:10 pm on February 8, 2018: member

    utACK 669c943
19. laanwj merged this on Feb 8, 2018
20. laanwj closed this on Feb 8, 2018

    ---

21. laanwj referenced this in commit 67447ba060 on Feb 8, 2018
22. Mengerian referenced this in commit e535b8caa9 on Aug 6, 2019
23. PastaPastaPasta referenced this in commit 361cd424b7 on Jul 17, 2020
24. PastaPastaPasta referenced this in commit c0a094ebe3 on Jul 17, 2020
25. PastaPastaPasta referenced this in commit ed5f046a99 on Jul 17, 2020
26. DrahtBot locked this on Sep 8, 2021

Contributors
sdaftuar meshcollider promag Speca92 laanwj

Labels
Mempool