Repository: bitcoin/bitcoin (language: cpp)

Issue:
bench: `MempoolCheck` actually runs with `check_ratio = 0`
For `MempoolCheck`, it seems that the intention was to have a mempool with `check_ratio = 1` (see `-checkmempool=1`):

https://github.com/bitcoin/bitcoin/blob/e66630cc87c017f40ec29f6c1edf2ed5a286e49d/src/bench/mempool_stress.cpp#L102-L116

However in the subsequent line in the above snippet, a `CTxMemPool` gets constructed with no arguments, which means that `check_ratio` will default to 0:

https://github.com/bitcoin/bitcoin/blob/e66630cc87c017f40ec29f6c1edf2ed5a286e49d/src/txmempool.h#L571

Manually specifying `check_ratio` to 1 (according to the original intention) and running the `MempoolCheck` benchmark results in an assertion error:

```
diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp
index afa4618e1b..32cdb70539 100644
--- a/src/bench/mempool_stress.cpp
+++ b/src/bench/mempool_stress.cpp
@@ -105,7 +105,7 @@ static void MempoolCheck(benchmark::Bench& bench)
     const int childTxs = bench.complexityN() > 1 ? static_cast<int>(bench.complexityN()) : 2000;
     const std::vector<CTransactionRef> ordered_coins = CreateOrderedCoins(det_rand, childTxs, /* min_ancestors */ 5);
     const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(CBaseChainParams::MAIN, {"-checkmempool=1"});
-    CTxMemPool pool;
+    CTxMemPool pool{nullptr, 1};
     LOCK2(cs_main, pool.cs);
     const CCoinsViewCache& coins_tip = testing_setup.get()->m_node.chainman->ActiveChainstate().CoinsTip();
     for (auto& tx : ordered_coins) AddTx(tx, pool);
```

```sh
$ ./src/bench/bench_bitcoin -filter='MempoolCheck'
...
bench_bitcoin: txmempool.cpp:759: void CTxMemPool::check(const CCoinsViewCache &, int64_t) const: Assertion `mempoolDuplicate.HaveCoin(txin.prevout)' failed.
```

Aside from this problem, we should also probably just re-use the `CTxMemPool` in `TestingSetup`.

Ping @glozow
bench: `MempoolCheck` actually runs with `check_ratio = 0`
For `MempoolCheck`, it seems that the intention was to have a mempool with `check_ratio = 1` (see `-checkmempool=1`):

https://github.com/bitcoin/bitcoin/blob/e66630cc87c017f40ec29f6c1edf2ed5a286e49d/src/bench/mempool_stress.cpp#L102-L116

However in the subsequent line in the above snippet, a `CTxMemPool` gets constructed with no arguments, which means that `check_ratio` will default to 0:

https://github.com/bitcoin/bitcoin/blob/e66630cc87c017f40ec29f6c1edf2ed5a286e49d/src/txmempool.h#L571

Manually specifying `check_ratio` to 1 (according to the original intention) and running the `MempoolCheck` benchmark results in an assertion error:

```
diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp
index afa4618e1b..32cdb70539 100644
--- a/src/bench/mempool_stress.cpp
+++ b/src/bench/mempool_stress.cpp
@@ -105,7 +105,7 @@ static void MempoolCheck(benchmark::Bench& bench)
     const int childTxs = bench.complexityN() > 1 ? static_cast<int>(bench.complexityN()) : 2000;
     const std::vector<CTransactionRef> ordered_coins = CreateOrderedCoins(det_rand, childTxs, /* min_ancestors */ 5);
     const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(CBaseChainParams::MAIN, {"-checkmempool=1"});
-    CTxMemPool pool;
+    CTxMemPool pool{nullptr, 1};
     LOCK2(cs_main, pool.cs);
     const CCoinsViewCache& coins_tip = testing_setup.get()->m_node.chainman->ActiveChainstate().CoinsTip();
     for (auto& tx : ordered_coins) AddTx(tx, pool);
```

```sh
$ ./src/bench/bench_bitcoin -filter='MempoolCheck'
...
bench_bitcoin: txmempool.cpp:759: void CTxMemPool::check(const CCoinsViewCache &, int64_t) const: Assertion `mempoolDuplicate.HaveCoin(txin.prevout)' failed.
```

Aside from this problem, we should also probably just re-use the `CTxMemPool` in `TestingSetup`.

Ping @glozow


Discussion / hints:
Wow, good catch! No wonder it runs so fast, we're benching a no-op... :facepalm:

> Aside from this problem, we should also probably just re-use the CTxMemPool in TestingSetup.

Agreed.

> Manually specifying check_ratio to 1 (according to the original intention) and running the MempoolCheck benchmark results in an assertion error:

This is because `CreateOrderedCoins` references UTXOs which don't actually exist on chain to construct transactions. My bad for not realizing that. The solution is to either use a `TestChain100Setup` instead of `TestingSetup` so we have some coins to work with, or create a fake `CCoinsCache` with the UTXOs needed in `CreateOrderedCoins`.
It seems like you care about `min_ancestors` here, so perhaps `TestChain100Setup` wouldn't work well?
I don't think `min_ancestors` is an issue, but using `TestChain100Setup` slows it down massively. I guess the solution is to make a fake `CCoinsCache`, but at that point we're just testing that we can use a coins cache properly. Leaning towards just deleting the test. We call `check()` all the time in functional tests anyway...
Slows it down because there are too many coins?
I think because it has to initialize TestChain100Setup (which involves mining 100 blocks and some other stuff)
What do you mean with "slows it down massively" and why is that important? It's only done once during the setup part, whereas the actual benchmark just calls `check()` in a loop and should be unaffected by this.
Omg, thanks for keeping me honest @dongcarl @mzumsande. You're right that it makes no sense, not sure why my "massive slowdown" happened yesterday but I tried again from scratch today and it seems to work.

I've opened #24927 to fix this in the original way proposed: "use a TestChain100Setup instead of TestingSetup so we have some coins to work with," adding a `PopulateMempool()` util function to randomly add a bunch of transactions to the mempool.
Wow, good catch! No wonder it runs so fast, we're benching a no-op... :facepalm:

> Aside from this problem, we should also probably just re-use the CTxMemPool in TestingSetup.

Agreed.

> Manually specifying check_ratio to 1 (according to the original intention) and running the MempoolCheck benchmark results in an assertion error:

This is because `CreateOrderedCoins` references UTXOs which don't actually exist on chain to construct transactions. My bad for not realizing that. The solution is to either use a `TestChain100Setup` instead of `TestingSetup` so we have some coins to work with, or create a fake `CCoinsCache` with the UTXOs needed in `CreateOrderedCoins`.
It seems like you care about `min_ancestors` here, so perhaps `TestChain100Setup` wouldn't work well?
I don't think `min_ancestors` is an issue, but using `TestChain100Setup` slows it down massively. I guess the solution is to make a fake `CCoinsCache`, but at that point we're just testing that we can use a coins cache properly. Leaning towards just deleting the test. We call `check()` all the time in functional tests anyway...
Slows it down because there are too many coins?
I think because it has to initialize TestChain100Setup (which involves mining 100 blocks and some other stuff)
What do you mean with "slows it down massively" and why is that important? It's only done once during the setup part, whereas the actual benchmark just calls `check()` in a loop and should be unaffected by this.
Omg, thanks for keeping me honest @dongcarl @mzumsande. You're right that it makes no sense, not sure why my "massive slowdown" happened yesterday but I tried again from scratch today and it seems to work.

I've opened #24927 to fix this in the original way proposed: "use a TestChain100Setup instead of TestingSetup so we have some coins to work with," adding a `PopulateMempool()` util function to randomly add a bunch of transactions to the mempool.

Fix patch:
diff --git a/src/bench/mempool_stress.cpp b/src/bench/mempool_stress.cpp
index a58658c4f1286..725a6f8f5b37c 100644
--- a/src/bench/mempool_stress.cpp
+++ b/src/bench/mempool_stress.cpp
@@ -88,7 +88,7 @@ static void ComplexMemPool(benchmark::Bench& bench)
     }
     std::vector<CTransactionRef> ordered_coins = CreateOrderedCoins(det_rand, childTxs, /*min_ancestors=*/1);
     const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(CBaseChainParams::MAIN);
-    CTxMemPool pool;
+    CTxMemPool& pool = *testing_setup.get()->m_node.mempool;
     LOCK2(cs_main, pool.cs);
     bench.run([&]() NO_THREAD_SAFETY_ANALYSIS {
         for (auto& tx : ordered_coins) {
@@ -102,16 +102,15 @@ static void ComplexMemPool(benchmark::Bench& bench)
 static void MempoolCheck(benchmark::Bench& bench)
 {
     FastRandomContext det_rand{true};
-    const int childTxs = bench.complexityN() > 1 ? static_cast<int>(bench.complexityN()) : 2000;
-    const std::vector<CTransactionRef> ordered_coins = CreateOrderedCoins(det_rand, childTxs, /*min_ancestors=*/5);
-    const auto testing_setup = MakeNoLogFileContext<const TestingSetup>(CBaseChainParams::MAIN, {"-checkmempool=1"});
-    CTxMemPool pool;
+    auto testing_setup = MakeNoLogFileContext<TestChain100Setup>(CBaseChainParams::REGTEST, {"-checkmempool=1"});
+    CTxMemPool& pool = *testing_setup.get()->m_node.mempool;
     LOCK2(cs_main, pool.cs);
+    testing_setup->PopulateMempool(det_rand, 400, true);
     const CCoinsViewCache& coins_tip = testing_setup.get()->m_node.chainman->ActiveChainstate().CoinsTip();
-    for (auto& tx : ordered_coins) AddTx(tx, pool);
 
     bench.run([&]() NO_THREAD_SAFETY_ANALYSIS {
-        pool.check(coins_tip, /*spendheight=*/2);
+        // Bump up the spendheight so we don't hit premature coinbase spend errors.
+        pool.check(coins_tip, /*spendheight=*/300);
     });
 }
 


Test patch:
diff --git a/src/test/txvalidationcache_tests.cpp b/src/test/txvalidationcache_tests.cpp
index d41b54af200a5..dd4bc5af75fbd 100644
--- a/src/test/txvalidationcache_tests.cpp
+++ b/src/test/txvalidationcache_tests.cpp
@@ -15,7 +15,7 @@
 
 struct Dersig100Setup : public TestChain100Setup {
     Dersig100Setup()
-        : TestChain100Setup{{"-testactivationheight=dersig@102"}} {}
+        : TestChain100Setup{CBaseChainParams::REGTEST, {"-testactivationheight=dersig@102"}} {}
 };
 
 bool CheckInputScripts(const CTransaction& tx, TxValidationState& state,
diff --git a/src/test/util/setup_common.cpp b/src/test/util/setup_common.cpp
index b7566bd1fab9d..01f41ccf82006 100644
--- a/src/test/util/setup_common.cpp
+++ b/src/test/util/setup_common.cpp
@@ -43,6 +43,7 @@
 #include <validationinterface.h>
 #include <walletinitinterface.h>
 
+#include <algorithm>
 #include <functional>
 #include <stdexcept>
 
@@ -161,7 +162,7 @@ ChainTestingSetup::ChainTestingSetup(const std::string& chainName, const std::ve
     GetMainSignals().RegisterBackgroundSignalScheduler(*m_node.scheduler);
 
     m_node.fee_estimator = std::make_unique<CBlockPolicyEstimator>();
-    m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), 1);
+    m_node.mempool = std::make_unique<CTxMemPool>(m_node.fee_estimator.get(), m_node.args->GetIntArg("-checkmempool", 1));
 
     m_cache_sizes = CalculateCacheSizes(m_args);
 
@@ -242,8 +243,8 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
     }
 }
 
-TestChain100Setup::TestChain100Setup(const std::vector<const char*>& extra_args)
-    : TestingSetup{CBaseChainParams::REGTEST, extra_args}
+TestChain100Setup::TestChain100Setup(const std::string& chain_name, const std::vector<const char*>& extra_args)
+    : TestingSetup{chain_name, extra_args}
 {
     SetMockTime(1598887952);
     constexpr std::array<unsigned char, 32> vchKey = {
@@ -357,6 +358,52 @@ CMutableTransaction TestChain100Setup::CreateValidMempoolTransaction(CTransactio
     return mempool_txn;
 }
 
+std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContext& det_rand, size_t num_transactions, bool submit)
+{
+    std::vector<CTransactionRef> mempool_transactions;
+    std::deque<std::pair<COutPoint, CAmount>> unspent_prevouts;
+    std::transform(m_coinbase_txns.begin(), m_coinbase_txns.end(), std::back_inserter(unspent_prevouts),
+        [](const auto& tx){ return std::make_pair(COutPoint(tx->GetHash(), 0), tx->vout[0].nValue); });
+    while (num_transactions > 0 && !unspent_prevouts.empty()) {
+        // The number of inputs and outputs are random, between 1 and 24.
+        CMutableTransaction mtx = CMutableTransaction();
+        const size_t num_inputs = det_rand.randrange(24) + 1;
+        CAmount total_in{0};
+        for (size_t n{0}; n < num_inputs; ++n) {
+            if (unspent_prevouts.empty()) break;
+            const auto& [prevout, amount] = unspent_prevouts.front();
+            mtx.vin.push_back(CTxIn(prevout, CScript()));
+            total_in += amount;
+            unspent_prevouts.pop_front();
+        }
+        const size_t num_outputs = det_rand.randrange(24) + 1;
+        // Approximately 1000sat "fee," equal output amounts.
+        const CAmount amount_per_output = (total_in - 1000) / num_outputs;
+        for (size_t n{0}; n < num_outputs; ++n) {
+            CScript spk = CScript() << CScriptNum(num_transactions + n);
+            mtx.vout.push_back(CTxOut(amount_per_output, spk));
+        }
+        CTransactionRef ptx = MakeTransactionRef(mtx);
+        mempool_transactions.push_back(ptx);
+        if (amount_per_output > 2000) {
+            // If the value is high enough to fund another transaction + fees, keep track of it so
+            // it can be used to build a more complex transaction graph. Insert randomly into
+            // unspent_prevouts for extra randomness in the resulting structures.
+            for (size_t n{0}; n < num_outputs; ++n) {
+                unspent_prevouts.push_back(std::make_pair(COutPoint(ptx->GetHash(), n), amount_per_output));
+                std::swap(unspent_prevouts.back(), unspent_prevouts[det_rand.randrange(unspent_prevouts.size())]);
+            }
+        }
+        if (submit) {
+            LOCK2(m_node.mempool->cs, cs_main);
+            LockPoints lp;
+            m_node.mempool->addUnchecked(CTxMemPoolEntry(ptx, 1000, 0, 1, false, 4, lp));
+        }
+        --num_transactions;
+    }
+    return mempool_transactions;
+}
+
 CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(const CMutableTransaction& tx) const
 {
     return FromTx(MakeTransactionRef(tx));
diff --git a/src/test/util/setup_common.h b/src/test/util/setup_common.h
index a1b7525cf454f..4270eb18c6134 100644
--- a/src/test/util/setup_common.h
+++ b/src/test/util/setup_common.h
@@ -122,7 +122,8 @@ class CScript;
  * Testing fixture that pre-creates a 100-block REGTEST-mode block chain
  */
 struct TestChain100Setup : public TestingSetup {
-    TestChain100Setup(const std::vector<const char*>& extra_args = {});
+    TestChain100Setup(const std::string& chain_name = CBaseChainParams::REGTEST,
+                      const std::vector<const char*>& extra_args = {});
 
     /**
      * Create a new block with just given transactions, coinbase paying to
@@ -164,6 +165,19 @@ struct TestChain100Setup : public TestingSetup {
                                                       CAmount output_amount = CAmount(1 * COIN),
                                                       bool submit = true);
 
+    /** Create transactions spending from m_coinbase_txns. These transactions will only spend coins
+     * that exist in the current chain, but may be premature coinbase spends, have missing
+     * signatures, or violate some other consensus rules. They should only be used for testing
+     * mempool consistency. All transactions will have some random number of inputs and outputs
+     * (between 1 and 24). Transactions may or may not be dependent upon each other; if dependencies
+     * exit, every parent will always be somewhere in the list before the child so each transaction
+     * can be submitted in the same order they appear in the list.
+     * @param[in]   submit      When true, submit transactions to the mempool.
+     *                          When false, return them but don't submit them.
+     * @returns A vector of transactions that can be submitted to the mempool.
+     */
+    std::vector<CTransactionRef> PopulateMempool(FastRandomContext& det_rand, size_t num_transactions, bool submit);
+
     std::vector<CTransactionRef> m_coinbase_txns; // For convenience, coinbase transactions
     CKey coinbaseKey; // private/public key needed to spend coinbase transactions
 };