Merge bitcoin#12225: Mempool cleanups

669c943 Avoid leaking prioritization information when relaying transactions (Suhas Daftuar)
e868b22 fee estimator: avoid sorting mempool on shutdown (Suhas Daftuar)
0975406 Correct mempool mapTx comment (Suhas Daftuar)

Pull request description:

  Following up on bitcoin#12127 and bitcoin#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.

Tree-SHA512: c97d10b96dcd6520459287a4a2eda92774173757695100fcfe61e526aef86f394507c331d17f9e0c14b496c33ec46198a0f165a847762ca50f7c6780b993f162

src/init.cpp

@@ -213,7 +213,7 @@ void Shutdown()
 
     if (fFeeEstimatesInitialized)
     {
-        ::feeEstimator.FlushUnconfirmed(::mempool);
+        ::feeEstimator.FlushUnconfirmed();
         fs::path est_path = GetDataDir() / FEE_ESTIMATES_FILENAME;
         CAutoFile est_fileout(fsbridge::fopen(est_path, "wb"), SER_DISK, CLIENT_VERSION);
         if (!est_fileout.IsNull())

src/policy/fees.cpp

@@ -981,16 +981,17 @@ bool CBlockPolicyEstimator::Read(CAutoFile& filein)
     return true;
 }
 
-void CBlockPolicyEstimator::FlushUnconfirmed(CTxMemPool& pool) {
+void CBlockPolicyEstimator::FlushUnconfirmed() {
     int64_t startclear = GetTimeMicros();
-    std::vector<uint256> txids;
-    pool.queryHashes(txids);
     LOCK(cs_feeEstimator);
-    for (auto& txid : txids) {
-        removeTx(txid, false);
+    size_t num_entries = mapMemPoolTxs.size();
+    // Remove every entry in mapMemPoolTxs
+    while (!mapMemPoolTxs.empty()) {
+        auto mi = mapMemPoolTxs.begin();
+        removeTx(mi->first, false); // this calls erase() on mapMemPoolTxs
     }
     int64_t endclear = GetTimeMicros();
-    LogPrint(BCLog::ESTIMATEFEE, "Recorded %u unconfirmed txs from mempool in %gs\n",txids.size(), (endclear - startclear)*0.000001);
+    LogPrint(BCLog::ESTIMATEFEE, "Recorded %u unconfirmed txs from mempool in %gs\n", num_entries, (endclear - startclear)*0.000001);
 }
 
 FeeFilterRounder::FeeFilterRounder(const CFeeRate& minIncrementalFee)

src/policy/fees.h

@@ -223,7 +223,7 @@ class CBlockPolicyEstimator
     bool Read(CAutoFile& filein);
 
     /** Empty mempool transactions on shutdown to record failure to confirm for txs still in mempool */
-    void FlushUnconfirmed(CTxMemPool& pool);
+    void FlushUnconfirmed();
 
     /** Calculation of highest target that estimates are tracked for */
     unsigned int HighestTargetTracked(FeeEstimateHorizon horizon) const;

src/txmempool.h

@@ -241,15 +241,18 @@ class CompareTxMemPoolEntryByDescendantScore
 
 /** \class CompareTxMemPoolEntryByScore
  *
- *  Sort by score of entry ((fee+delta)/size) in descending order
+ *  Sort by feerate of entry (fee/size) in descending order
+ *  This is only used for transaction relay, so we use GetFee()
+ *  instead of GetModifiedFee() to avoid leaking prioritization
+ *  information via the sort order.
  */
 class CompareTxMemPoolEntryByScore
 {
 public:
     bool operator()(const CTxMemPoolEntry& a, const CTxMemPoolEntry& b) const
     {
-        double f1 = (double)a.GetModifiedFee() * b.GetTxSize();
-        double f2 = (double)b.GetModifiedFee() * a.GetTxSize();
+        double f1 = (double)a.GetFee() * b.GetTxSize();
+        double f2 = (double)b.GetFee() * a.GetTxSize();
         if (f1 == f2) {
             return b.GetTx().GetHash() < a.GetTx().GetHash();
         }
@@ -379,8 +382,9 @@ class SaltedTxidHasher
  *
  * mapTx is a boost::multi_index that sorts the mempool on 4 criteria:
  * - transaction hash
- * - feerate [we use max(feerate of tx, feerate of tx with all descendants)]
+ * - descendant feerate [we use max(feerate of tx, feerate of tx with all descendants)]
  * - time in mempool
+ * - ancestor feerate [we use min(feerate of tx, feerate of tx with all unconfirmed ancestors)]
  *
  * Note: the term "descendant" refers to in-mempool transactions that depend on
  * this one, while "ancestor" refers to in-mempool transactions that a given