Merge bitcoin/bitcoin#31122: cluster mempool: Implement changeset int…

…erface for mempool

5736d1d tracing: pass if replaced by tx/pkg to tracepoint (0xb10c)
a4ec07f doc: add comments for CTxMemPool::ChangeSet (Suhas Daftuar)
83f814b Remove m_all_conflicts from SubPackageState (Suhas Daftuar)
d3c8e7d Ensure that we don't add duplicate transactions in rbf fuzz tests (Suhas Daftuar)
d7dc9fd Move CalculateChunksForRBF() to the mempool changeset (Suhas Daftuar)
284a1d3 Move prioritisation into changeset (Suhas Daftuar)
446b08b Don't distinguish between direct conflicts and all conflicts when doing cluster-size-2-rbf checks (Suhas Daftuar)
b530410 Duplicate transactions are not permitted within a changeset (Suhas Daftuar)
b447416 Public mempool removal methods Assume() no changeset is outstanding (Suhas Daftuar)
2b30f4d Make RemoveStaged() private (Suhas Daftuar)
1882919 Enforce that there is only one changeset at a time (Suhas Daftuar)
7fb62f7 Apply mempool changeset transactions directly into the mempool (Suhas Daftuar)
34b6c58 Clean up FinalizeSubpackage to avoid workspace-specific information (Suhas Daftuar)
57983b8 Move LimitMempoolSize to take place outside FinalizeSubpackage (Suhas Daftuar)
01e145b Move changeset from workspace to subpackage (Suhas Daftuar)
802214c Introduce mempool changesets (Suhas Daftuar)
87d92fa test: Add unit test coverage of package rbf + prioritisetransaction (Suhas Daftuar)
15d982f Add package hash to package-rbf log message (Suhas Daftuar)

Pull request description:

  part of cluster mempool: #30289

  It became clear while working on cluster mempool that it would be helpful for transaction validation if we could consider a full set of proposed changes to the mempool -- consisting of a set of transactions to add, and a set of transactions (ie conflicts) to simultaneously remove -- and perform calculations on what the mempool would look like if the proposed changes were to be applied.  Two specific examples of where we'd like to do this:

  - Determining if ancestor/descendant/TRUC limits would be violated (in the future, cluster limits) if either a single transaction or a package of transactions were to be accepted
  - Determining if an RBF would make the mempool "better", however that idea is defined, both in the single transaction and package of transaction cases

  In preparation for cluster mempool, I have pulled this reworking of the mempool interface out of #28676 so it can be reviewed on its own.  I have not re-implemented ancestor/descendant limits to be run through the changeset, since with cluster mempool those limits will be going away, so this seems like wasted effort.  However, I have rebased #28676 on top of this branch so reviewers can see what the new mempool interface could look like in the cluster mempool setting.

  There are some minor behavior changes here, which I believe are inconsequential:
  - In the package validation setting, transactions would be added to the mempool before the `ConsensusScriptChecks()` are run. In theory, `ConsensusScriptChecks()` should always pass if the `PolicyScriptChecks()` have passed and it's just a belt-and-suspenders for us, but if somehow they were to diverge then there could be some small behavior change from adding transactions and then removing them, versus never adding them at all.
  - The error reporting on `CheckConflictTopology()` has slightly changed due to no longer distinguishing between direct conflicts and indirect conflicts. I believe this should be entirely inconsequential because there shouldn't be a logical difference between those two ideas from the perspective of this function, but I did have to update some error strings in some tests.
  - Because, in a package setting, RBFs now happen as part of the entire package being accepted, the logging has changed slightly because we do not know which transaction specifically evicted a given removed transaction.
    -  Specifically, the "package hash" is now used to reference the set of transactions that are being accepted, rather than any single txid. The log message relating to package RBF that happen in the `TXPACKAGES` category has been updated as well to include the package hash, so that it's possible to see which specific set of transactions are being referenced by that package hash.
    - Relatedly, the tracepoint logging in the package rbf case has been updated as well to reference the package hash, rather than a transaction hash.

ACKs for top commit:
  naumenkogs:
    ACK bitcoin/bitcoin@5736d1d
  instagibbs:
    ACK 5736d1d
  ismaelsadeeq:
    reACK 5736d1d
  glozow:
    ACK 5736d1d

Tree-SHA512: 21810872e082920d337c89ac406085aa71c5f8e5151ab07aedf41e6601f60a909b22fbf462ef3b735d5d5881e9b76142c53957158e674dd5dfe6f6aabbdf630b

doc/tracing.md

@@ -245,14 +245,15 @@ Arguments passed:
 2. Replaced transaction virtual size as `int32`
 3. Replaced transaction fee as `int64`
 4. Replaced transaction mempool entry time (epoch) as `uint64`
-5. Replacement transaction ID (hash) as `pointer to unsigned chars` (i.e. 32 bytes in little-endian)
+5. Replacement transaction ID or package hash as `pointer to unsigned chars` (i.e. 32 bytes in little-endian)
 6. Replacement transaction virtual size as `int32`
 7. Replacement transaction fee as `int64`
+8. `bool` indicating if the argument 5. is a transaction ID or package hash (true if it's a transaction ID)
 
-Note: In cases where a single replacement transaction replaces multiple
+Note: In cases where a replacement transaction or package replaces multiple
 existing transactions in the mempool, the tracepoint is called once for each
-replaced transaction, with data of the replacement transaction being the same
-in each call.
+replaced transaction, with data of the replacement transaction or package
+being the same in each call.
 
 #### Tracepoint `mempool:rejected`
 

src/bench/mempool_ephemeral_spends.cpp

@@ -11,6 +11,7 @@
 #include <script/script.h>
 #include <sync.h>
 #include <test/util/setup_common.h>
+#include <test/util/txmempool.h>
 #include <txmempool.h>
 #include <util/check.h>
 
@@ -28,7 +29,7 @@ static void AddTx(const CTransactionRef& tx, CTxMemPool& pool) EXCLUSIVE_LOCKS_R
     unsigned int sigOpCost{4};
     uint64_t fee{0};
     LockPoints lp;
-    pool.addUnchecked(CTxMemPoolEntry(
+    AddToMempool(pool, CTxMemPoolEntry(
         tx, fee, nTime, nHeight, sequence,
         spendsCoinbase, sigOpCost, lp));
 }

src/bench/mempool_eviction.cpp

@@ -10,6 +10,7 @@
 #include <script/script.h>
 #include <sync.h>
 #include <test/util/setup_common.h>
+#include <test/util/txmempool.h>
 #include <txmempool.h>
 #include <util/check.h>
 
@@ -26,7 +27,7 @@ static void AddTx(const CTransactionRef& tx, const CAmount& nFee, CTxMemPool& po
     bool spendsCoinbase = false;
     unsigned int sigOpCost = 4;
     LockPoints lp;
-    pool.addUnchecked(CTxMemPoolEntry(
+    AddToMempool(pool, CTxMemPoolEntry(
         tx, nFee, nTime, nHeight, sequence,
         spendsCoinbase, sigOpCost, lp));
 }

src/bench/mempool_stress.cpp

@@ -10,6 +10,7 @@
 #include <script/script.h>
 #include <sync.h>
 #include <test/util/setup_common.h>
+#include <test/util/txmempool.h>
 #include <txmempool.h>
 #include <validation.h>
 
@@ -28,7 +29,7 @@ static void AddTx(const CTransactionRef& tx, CTxMemPool& pool) EXCLUSIVE_LOCKS_R
     bool spendsCoinbase = false;
     unsigned int sigOpCost = 4;
     LockPoints lp;
-    pool.addUnchecked(CTxMemPoolEntry(tx, 1000, nTime, nHeight, sequence, spendsCoinbase, sigOpCost, lp));
+    AddToMempool(pool, CTxMemPoolEntry(tx, 1000, nTime, nHeight, sequence, spendsCoinbase, sigOpCost, lp));
 }
 
 struct Available {

src/bench/rpc_mempool.cpp

@@ -10,6 +10,7 @@
 #include <script/script.h>
 #include <sync.h>
 #include <test/util/setup_common.h>
+#include <test/util/txmempool.h>
 #include <txmempool.h>
 #include <univalue.h>
 #include <util/check.h>
@@ -21,7 +22,7 @@
 static void AddTx(const CTransactionRef& tx, const CAmount& fee, CTxMemPool& pool) EXCLUSIVE_LOCKS_REQUIRED(cs_main, pool.cs)
 {
     LockPoints lp;
-    pool.addUnchecked(CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/4, lp));
+    AddToMempool(pool, CTxMemPoolEntry(tx, fee, /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0, /*spends_coinbase=*/false, /*sigops_cost=*/4, lp));
 }
 
 static void RpcMempool(benchmark::Bench& bench)

src/policy/rbf.cpp

@@ -184,14 +184,10 @@ std::optional<std::string> PaysForRBF(CAmount original_fees,
     return std::nullopt;
 }
 
-std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(CTxMemPool& pool,
-                                                const CTxMemPool::setEntries& direct_conflicts,
-                                                const CTxMemPool::setEntries& all_conflicts,
-                                                CAmount replacement_fees,
-                                                int64_t replacement_vsize)
+std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(CTxMemPool::ChangeSet& changeset)
 {
     // Require that the replacement strictly improves the mempool's feerate diagram.
-    const auto chunk_results{pool.CalculateChunksForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)};
+    const auto chunk_results{changeset.CalculateChunksForRBF()};
 
     if (!chunk_results.has_value()) {
         return std::make_pair(DiagramCheckError::UNCALCULABLE, util::ErrorString(chunk_results).original);

src/policy/rbf.h

@@ -117,19 +117,9 @@ std::optional<std::string> PaysForRBF(CAmount original_fees,
 
 /**
  * The replacement transaction must improve the feerate diagram of the mempool.
- * @param[in]   pool                The mempool.
- * @param[in]   direct_conflicts    Set of in-mempool txids corresponding to the direct conflicts i.e.
- *                                  input double-spends with the proposed transaction
- * @param[in]   all_conflicts       Set of mempool entries corresponding to all transactions to be evicted
- * @param[in]   replacement_fees    Fees of proposed replacement package
- * @param[in]   replacement_vsize   Size of proposed replacement package
+ * @param[in]   changeset           The changeset containing proposed additions/removals
  * @returns error type and string if mempool diagram doesn't improve, otherwise std::nullopt.
  */
-std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(CTxMemPool& pool,
-                                                const CTxMemPool::setEntries& direct_conflicts,
-                                                const CTxMemPool::setEntries& all_conflicts,
-                                                CAmount replacement_fees,
-                                                int64_t replacement_vsize)
-                                                EXCLUSIVE_LOCKS_REQUIRED(pool.cs);
+std::optional<std::pair<DiagramCheckError, std::string>> ImprovesFeerateDiagram(CTxMemPool::ChangeSet& changeset);
 
 #endif // BITCOIN_POLICY_RBF_H

src/test/blockencodings_tests.cpp

@@ -67,7 +67,7 @@ BOOST_AUTO_TEST_CASE(SimpleRoundTripTest)
     CBlock block(BuildBlockTestCase(rand_ctx));
 
     LOCK2(cs_main, pool.cs);
-    pool.addUnchecked(entry.FromTx(block.vtx[2]));
+    AddToMempool(pool, entry.FromTx(block.vtx[2]));
     BOOST_CHECK_EQUAL(pool.get(block.vtx[2]->GetHash()).use_count(), SHARED_TX_OFFSET + 0);
 
     // Do a simple ShortTxIDs RT
@@ -151,7 +151,7 @@ BOOST_AUTO_TEST_CASE(NonCoinbasePreforwardRTTest)
     CBlock block(BuildBlockTestCase(rand_ctx));
 
     LOCK2(cs_main, pool.cs);
-    pool.addUnchecked(entry.FromTx(block.vtx[2]));
+    AddToMempool(pool, entry.FromTx(block.vtx[2]));
     BOOST_CHECK_EQUAL(pool.get(block.vtx[2]->GetHash()).use_count(), SHARED_TX_OFFSET + 0);
 
     uint256 txhash;
@@ -222,7 +222,7 @@ BOOST_AUTO_TEST_CASE(SufficientPreforwardRTTest)
     CBlock block(BuildBlockTestCase(rand_ctx));
 
     LOCK2(cs_main, pool.cs);
-    pool.addUnchecked(entry.FromTx(block.vtx[1]));
+    AddToMempool(pool, entry.FromTx(block.vtx[1]));
     BOOST_CHECK_EQUAL(pool.get(block.vtx[1]->GetHash()).use_count(), SHARED_TX_OFFSET + 0);
 
     uint256 txhash;
@@ -322,7 +322,7 @@ BOOST_AUTO_TEST_CASE(ReceiveWithExtraTransactions) {
     extra_txn.resize(10);
 
     LOCK2(cs_main, pool.cs);
-    pool.addUnchecked(entry.FromTx(block.vtx[2]));
+    AddToMempool(pool, entry.FromTx(block.vtx[2]));
     BOOST_CHECK_EQUAL(pool.get(block.vtx[2]->GetHash()).use_count(), SHARED_TX_OFFSET + 0);
     // Ensure the non_block_tx is actually not in the block
     for (const auto &block_tx : block.vtx) {

src/test/fuzz/mini_miner.cpp

@@ -59,7 +59,7 @@ FUZZ_TARGET(mini_miner, .init = initialize_miner)
         TestMemPoolEntryHelper entry;
         const CAmount fee{ConsumeMoney(fuzzed_data_provider, /*max=*/MAX_MONEY/100000)};
         assert(MoneyRange(fee));
-        pool.addUnchecked(entry.Fee(fee).FromTx(tx));
+        AddToMempool(pool, entry.Fee(fee).FromTx(tx));
 
         // All outputs are available to spend
         for (uint32_t n{0}; n < num_outputs; ++n) {
@@ -154,7 +154,7 @@ FUZZ_TARGET(mini_miner_selection, .init = initialize_miner)
         TestMemPoolEntryHelper entry;
         const CAmount fee{ConsumeMoney(fuzzed_data_provider, /*max=*/MAX_MONEY/100000)};
         assert(MoneyRange(fee));
-        pool.addUnchecked(entry.Fee(fee).FromTx(tx));
+        AddToMempool(pool, entry.Fee(fee).FromTx(tx));
         transactions.push_back(tx);
     }
     std::vector<COutPoint> outpoints;

src/test/fuzz/partially_downloaded_block.cpp

@@ -78,7 +78,7 @@ FUZZ_TARGET(partially_downloaded_block, .init = initialize_pdb)
 
         if (add_to_mempool && !pool.exists(GenTxid::Txid(tx->GetHash()))) {
             LOCK2(cs_main, pool.cs);
-            pool.addUnchecked(ConsumeTxMemPoolEntry(fuzzed_data_provider, *tx));
+            AddToMempool(pool, ConsumeTxMemPoolEntry(fuzzed_data_provider, *tx));
             available.insert(i);
         }
     }

src/test/fuzz/rbf.cpp

@@ -73,12 +73,16 @@ FUZZ_TARGET(rbf, .init = initialize_rbf)
             mtx->vin[0].prevout = COutPoint{another_tx.GetHash(), 0};
         }
         LOCK2(cs_main, pool.cs);
-        pool.addUnchecked(ConsumeTxMemPoolEntry(fuzzed_data_provider, another_tx));
+        if (!pool.GetIter(another_tx.GetHash())) {
+            AddToMempool(pool, ConsumeTxMemPoolEntry(fuzzed_data_provider, another_tx));
+        }
     }
     const CTransaction tx{*mtx};
     if (fuzzed_data_provider.ConsumeBool()) {
         LOCK2(cs_main, pool.cs);
-        pool.addUnchecked(ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));
+        if (!pool.GetIter(tx.GetHash())) {
+            AddToMempool(pool, ConsumeTxMemPoolEntry(fuzzed_data_provider, tx));
+        }
     }
     {
         LOCK(pool.cs);
@@ -104,10 +108,18 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
     std::vector<CTransaction> mempool_txs;
     size_t iter{0};
 
-    int32_t replacement_vsize = fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(1, 1000000);
-
     // Keep track of the total vsize of CTxMemPoolEntry's being added to the mempool to avoid overflow
     // Add replacement_vsize since this is added to new diagram during RBF check
+    std::optional<CMutableTransaction> replacement_tx = ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider, TX_WITH_WITNESS);
+    if (!replacement_tx) {
+        return;
+    }
+    assert(iter <= g_outpoints.size());
+    replacement_tx->vin.resize(1);
+    replacement_tx->vin[0].prevout = g_outpoints[iter++];
+    CTransaction replacement_tx_final{*replacement_tx};
+    auto replacement_entry = ConsumeTxMemPoolEntry(fuzzed_data_provider, replacement_tx_final);
+    int32_t replacement_vsize = replacement_entry.GetTxSize();
     int64_t running_vsize_total{replacement_vsize};
 
     LOCK2(cs_main, pool.cs);
@@ -129,7 +141,8 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
             mempool_txs.pop_back();
             break;
         }
-        pool.addUnchecked(parent_entry);
+        assert(!pool.GetIter(parent_entry.GetTx().GetHash()));
+        AddToMempool(pool, parent_entry);
         if (fuzzed_data_provider.ConsumeBool()) {
             child.vin[0].prevout = COutPoint{mempool_txs.back().GetHash(), 0};
         }
@@ -141,7 +154,9 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
             mempool_txs.pop_back();
             break;
         }
-        pool.addUnchecked(child_entry);
+        if (!pool.GetIter(child_entry.GetTx().GetHash())) {
+            AddToMempool(pool, child_entry);
+        }
 
         if (fuzzed_data_provider.ConsumeBool()) {
             pool.PrioritiseTransaction(mempool_txs.back().GetHash().ToUint256(), fuzzed_data_provider.ConsumeIntegralInRange<int32_t>(-100000, 100000));
@@ -162,9 +177,17 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
         pool.CalculateDescendants(txiter, all_conflicts);
     }
 
-    // Calculate the chunks for a replacement.
     CAmount replacement_fees = ConsumeMoney(fuzzed_data_provider);
-    auto calc_results{pool.CalculateChunksForRBF(replacement_fees, replacement_vsize, direct_conflicts, all_conflicts)};
+    auto changeset = pool.GetChangeSet();
+    for (auto& txiter : all_conflicts) {
+        changeset->StageRemoval(txiter);
+    }
+    changeset->StageAddition(replacement_entry.GetSharedTx(), replacement_fees,
+            replacement_entry.GetTime().count(), replacement_entry.GetHeight(),
+            replacement_entry.GetSequence(), replacement_entry.GetSpendsCoinbase(),
+            replacement_entry.GetSigOpCost(), replacement_entry.GetLockPoints());
+    // Calculate the chunks for a replacement.
+    auto calc_results{changeset->CalculateChunksForRBF()};
 
     if (calc_results.has_value()) {
         // Sanity checks on the chunks.
@@ -192,7 +215,7 @@ FUZZ_TARGET(package_rbf, .init = initialize_package_rbf)
     }
 
     // If internals report error, wrapper should too
-    auto err_tuple{ImprovesFeerateDiagram(pool, direct_conflicts, all_conflicts, replacement_fees, replacement_vsize)};
+    auto err_tuple{ImprovesFeerateDiagram(*changeset)};
     if (!calc_results.has_value()) {
          assert(err_tuple.value().first == DiagramCheckError::UNCALCULABLE);
     } else {