Merge bitcoin/bitcoin#33210: fuzz: enhance wallet_fees by mocking mempool stuff

5ded99a7f0 fuzz: MockMempoolMinFee in wallet_fees (brunoerg)
c9a7a198d9 test: move MockMempoolMinFee to util/txmempool (brunoerg)
adf67eb21b fuzz: create FeeEstimatorTestingSetup to set fee_estimator (brunoerg)
ff10a37e99 fuzz: mock CBlockPolicyEstimator in wallet_fuzz (brunoerg)
f591c3beca fees: make estimateSmartFee/HighestTargetTracked virtual for mocking (brunoerg)
19273d0705 fuzz: set mempool options in wallet_fees (brunoerg)

Pull request description:

  Some functions in `wallet/fees.cpp` (fuzzed by the wallet_fees target) depends on some mempool stuff - e.g. relay current min fee, smart fee and max blocks estimation, relay dust fee and other ones. For better fuzzing of it, it would be great to have these values/interactions. That said, this PR enhances the `wallet_fees` target by:

  - Setting mempool options - `min_relay_feerate`,  `dust_relay_feerate` and `incremental_relay_feerate` - when creating the `CTxMemPool`.
  - Creates a `ConsumeMempoolMinFee` function which is used to have a mempool min fee (similar approach from `MockMempoolMinFee` from unit test).
  - Mock `CBlockPolicyEstimator` - estimateSmartFee/HighestTagretTracket functions, especifically. It's better to mock it then trying to interact to CBlockPolicyEstimator in order to have some effective values due to performance.

  Note that I created `FeeEstimatorTestingSetup` because we cannot set `m_node.fee_estimator` in `ChainTestingSetup` since fae8c73d9e.

ACKs for top commit:
  maflcko:
    re-ACK 5ded99a7f0 🎯
  ismaelsadeeq:
    Code review ACK 5ded99a7f0

Tree-SHA512: 13d2af042098afd237ef349437021ea841069d93d4c3e3a32e1b562c027d00c727f375426709d34421092993398caf7ba8ff19077982cb6f470f8938a44e7754

src/policy/fees.h

@@ -224,7 +224,7 @@ public:
      *  the closest target where one can be given.  'conservative' estimates are
      *  valid over longer time horizons also.
      */
-    CFeeRate estimateSmartFee(int confTarget, FeeCalculation *feeCalc, bool conservative) const
+    virtual CFeeRate estimateSmartFee(int confTarget, FeeCalculation *feeCalc, bool conservative) const
         EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator);
 
     /** Return a specific fee estimate calculation with a given success
@@ -248,7 +248,7 @@ public:
         EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator);
 
     /** Calculation of highest target that estimates are tracked for */
-    unsigned int HighestTargetTracked(FeeEstimateHorizon horizon) const
+    virtual unsigned int HighestTargetTracked(FeeEstimateHorizon horizon) const
         EXCLUSIVE_LOCKS_REQUIRED(!m_cs_fee_estimator);
 
     /** Drop still unconfirmed transactions and record current estimations, if the fee estimation file is present. */

src/test/txpackage_tests.cpp

@@ -357,7 +357,7 @@ BOOST_AUTO_TEST_CASE(package_submission_tests)
 {
     // Mine blocks to mature coinbases.
     mineBlocks(3);
-    MockMempoolMinFee(CFeeRate(5000));
+    MockMempoolMinFee(CFeeRate(5000), *m_node.mempool);
     LOCK(cs_main);
     unsigned int expected_pool_size = m_node.mempool->size();
     CKey parent_key = GenerateRandomKey();
@@ -634,7 +634,7 @@ BOOST_AUTO_TEST_CASE(package_witness_swap_tests)
 {
     // Mine blocks to mature coinbases.
     mineBlocks(5);
-    MockMempoolMinFee(CFeeRate(5000));
+    MockMempoolMinFee(CFeeRate(5000), *m_node.mempool);
     LOCK(cs_main);
 
     // Transactions with a same-txid-different-witness transaction in the mempool should be ignored,
@@ -867,7 +867,7 @@ BOOST_AUTO_TEST_CASE(package_witness_swap_tests)
 BOOST_AUTO_TEST_CASE(package_cpfp_tests)
 {
     mineBlocks(5);
-    MockMempoolMinFee(CFeeRate(5000));
+    MockMempoolMinFee(CFeeRate(5000), *m_node.mempool);
     LOCK(::cs_main);
     size_t expected_pool_size = m_node.mempool->size();
     CKey child_key = GenerateRandomKey();

src/test/util/setup_common.cpp

@@ -582,40 +582,6 @@ std::vector<CTransactionRef> TestChain100Setup::PopulateMempool(FastRandomContex
     return mempool_transactions;
 }
 
-void TestChain100Setup::MockMempoolMinFee(const CFeeRate& target_feerate)
-{
-    LOCK2(cs_main, m_node.mempool->cs);
-    // Transactions in the mempool will affect the new minimum feerate.
-    assert(m_node.mempool->size() == 0);
-    // The target feerate cannot be too low...
-    // ...otherwise the transaction's feerate will need to be negative.
-    assert(target_feerate > m_node.mempool->m_opts.incremental_relay_feerate);
-    // ...otherwise this is not meaningful. The feerate policy uses the maximum of both feerates.
-    assert(target_feerate > m_node.mempool->m_opts.min_relay_feerate);
-
-    // Manually create an invalid transaction. Manually set the fee in the CTxMemPoolEntry to
-    // achieve the exact target feerate.
-    CMutableTransaction mtx = CMutableTransaction();
-    mtx.vin.emplace_back(COutPoint{Txid::FromUint256(m_rng.rand256()), 0});
-    mtx.vout.emplace_back(1 * COIN, GetScriptForDestination(WitnessV0ScriptHash(CScript() << OP_TRUE)));
-    // Set a large size so that the fee evaluated at target_feerate (which is usually in sats/kvB) is an integer.
-    // Otherwise, GetMinFee() may end up slightly different from target_feerate.
-    BulkTransaction(mtx, 4000);
-    const auto tx{MakeTransactionRef(mtx)};
-    LockPoints lp;
-    // The new mempool min feerate is equal to the removed package's feerate + incremental feerate.
-    const auto tx_fee = target_feerate.GetFee(GetVirtualTransactionSize(*tx)) -
-        m_node.mempool->m_opts.incremental_relay_feerate.GetFee(GetVirtualTransactionSize(*tx));
-    {
-        auto changeset = m_node.mempool->GetChangeSet();
-        changeset->StageAddition(tx, /*fee=*/tx_fee,
-                /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0,
-                /*spends_coinbase=*/true, /*sigops_cost=*/1, lp);
-        changeset->Apply();
-    }
-    m_node.mempool->TrimToSize(0);
-    assert(m_node.mempool->GetMinFee() == target_feerate);
-}
 /**
  * @returns a real block (0000000000013b8ab2cd513b0261a14096412195a72a0c4827d229dcc7e0f7af)
  *      with 9 txs.

src/test/util/setup_common.h

@@ -238,17 +238,6 @@ struct TestChain100Setup : public TestingSetup {
      */
     std::vector<CTransactionRef> PopulateMempool(FastRandomContext& det_rand, size_t num_transactions, bool submit);
 
-    /** Mock the mempool minimum feerate by adding a transaction and calling TrimToSize(0),
-     * simulating the mempool "reaching capacity" and evicting by descendant feerate.  Note that
-     * this clears the mempool, and the new minimum feerate will depend on the maximum feerate of
-     * transactions removed, so this must be called while the mempool is empty.
-     *
-     * @param target_feerate    The new mempool minimum feerate after this function returns.
-     *                          Must be above max(incremental feerate, min relay feerate),
-     *                          or 1sat/vB with default settings.
-     */
-    void MockMempoolMinFee(const CFeeRate& target_feerate);
-
     std::vector<CTransactionRef> m_coinbase_txns; // For convenience, coinbase transactions
     CKey coinbaseKey; // private/public key needed to spend coinbase transactions
 };

src/test/util/txmempool.cpp

@@ -10,6 +10,7 @@
 #include <policy/rbf.h>
 #include <policy/truc_policy.h>
 #include <txmempool.h>
+#include <test/util/transaction_utils.h>
 #include <util/check.h>
 #include <util/time.h>
 #include <util/translation.h>
@@ -218,3 +219,38 @@ void AddToMempool(CTxMemPool& tx_pool, const CTxMemPoolEntry& entry)
             entry.GetSpendsCoinbase(), entry.GetSigOpCost(), entry.GetLockPoints());
     changeset->Apply();
 }
+
+void MockMempoolMinFee(const CFeeRate& target_feerate, CTxMemPool& mempool)
+{
+    LOCK2(cs_main, mempool.cs);
+    // Transactions in the mempool will affect the new minimum feerate.
+    assert(mempool.size() == 0);
+    // The target feerate cannot be too low...
+    // ...otherwise the transaction's feerate will need to be negative.
+    assert(target_feerate > mempool.m_opts.incremental_relay_feerate);
+    // ...otherwise this is not meaningful. The feerate policy uses the maximum of both feerates.
+    assert(target_feerate > mempool.m_opts.min_relay_feerate);
+
+    // Manually create an invalid transaction. Manually set the fee in the CTxMemPoolEntry to
+    // achieve the exact target feerate.
+    CMutableTransaction mtx{};
+    mtx.vin.emplace_back(COutPoint{Txid::FromUint256(uint256{123}), 0});
+    mtx.vout.emplace_back(1 * COIN, GetScriptForDestination(WitnessV0ScriptHash(CScript() << OP_TRUE)));
+    // Set a large size so that the fee evaluated at target_feerate (which is usually in sats/kvB) is an integer.
+    // Otherwise, GetMinFee() may end up slightly different from target_feerate.
+    BulkTransaction(mtx, 4000);
+    const auto tx{MakeTransactionRef(mtx)};
+    LockPoints lp;
+    // The new mempool min feerate is equal to the removed package's feerate + incremental feerate.
+    const auto tx_fee = target_feerate.GetFee(GetVirtualTransactionSize(*tx)) -
+        mempool.m_opts.incremental_relay_feerate.GetFee(GetVirtualTransactionSize(*tx));
+    {
+        auto changeset = mempool.GetChangeSet();
+        changeset->StageAddition(tx, /*fee=*/tx_fee,
+                /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0,
+                /*spends_coinbase=*/true, /*sigops_cost=*/1, lp);
+        changeset->Apply();
+    }
+    mempool.TrimToSize(0);
+    assert(mempool.GetMinFee() == target_feerate);
+}

src/test/util/txmempool.h

@@ -67,4 +67,17 @@ void CheckMempoolTRUCInvariants(const CTxMemPool& tx_pool);
  *  and applying it. */
 void AddToMempool(CTxMemPool& tx_pool, const CTxMemPoolEntry& entry);
 
+/** Mock the mempool minimum feerate by adding a transaction and calling TrimToSize(0),
+ * simulating the mempool "reaching capacity" and evicting by descendant feerate.  Note that
+ * this clears the mempool, and the new minimum feerate will depend on the maximum feerate of
+ * transactions removed, so this must be called while the mempool is empty.
+ *
+ * @param target_feerate    The new mempool minimum feerate after this function returns.
+ *                          Must be above max(incremental feerate, min relay feerate),
+ *                          or 1sat/vB with default settings.
+ * @param mempool           The mempool to mock the minimum feerate for. Must be empty
+ *                          when called.
+ */
+void MockMempoolMinFee(const CFeeRate& target_feerate, CTxMemPool& mempool);
+
 #endif // BITCOIN_TEST_UTIL_TXMEMPOOL_H

src/wallet/test/fuzz/fees.cpp

@@ -6,6 +6,7 @@
 #include <test/fuzz/fuzz.h>
 #include <test/fuzz/util.h>
 #include <test/util/setup_common.h>
+#include <test/util/txmempool.h>
 #include <validation.h>
 #include <wallet/coincontrol.h>
 #include <wallet/fees.h>
@@ -14,11 +15,46 @@
 
 namespace wallet {
 namespace {
-const TestingSetup* g_setup;
+
+struct FeeEstimatorTestingSetup : public TestingSetup {
+    FeeEstimatorTestingSetup(const ChainType chain_type, TestOpts opts) : TestingSetup{chain_type, opts}
+    {
+    }
+
+    ~FeeEstimatorTestingSetup() {
+        m_node.fee_estimator.reset();
+    }
+
+    void SetFeeEstimator(std::unique_ptr<CBlockPolicyEstimator> fee_estimator)
+    {
+        m_node.fee_estimator = std::move(fee_estimator);
+    }
+};
+
+FeeEstimatorTestingSetup* g_setup;
+
+class FuzzedBlockPolicyEstimator : public CBlockPolicyEstimator
+{
+    FuzzedDataProvider& fuzzed_data_provider;
+
+public:
+    FuzzedBlockPolicyEstimator(FuzzedDataProvider& provider)
+        : CBlockPolicyEstimator(fs::path{}, false), fuzzed_data_provider(provider) {}
+
+    CFeeRate estimateSmartFee(int confTarget, FeeCalculation* feeCalc, bool conservative) const override
+    {
+        return CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/1'000'000)};
+    }
+
+    unsigned int HighestTargetTracked(FeeEstimateHorizon horizon) const override
+    {
+        return fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(1, 1000);
+    }
+};
 
 void initialize_setup()
 {
-    static const auto testing_setup = MakeNoLogFileContext<const TestingSetup>();
+    static const auto testing_setup = MakeNoLogFileContext<FeeEstimatorTestingSetup>();
     g_setup = testing_setup.get();
 }
 
@@ -27,8 +63,23 @@ FUZZ_TARGET(wallet_fees, .init = initialize_setup)
     SeedRandomStateForTest(SeedRand::ZEROS);
     FuzzedDataProvider fuzzed_data_provider{buffer.data(), buffer.size()};
     SetMockTime(ConsumeTime(fuzzed_data_provider));
-    const auto& node{g_setup->m_node};
+    auto& node{g_setup->m_node};
     Chainstate* chainstate = &node.chainman->ActiveChainstate();
+
+    bilingual_str error;
+    CTxMemPool::Options mempool_opts{
+        .incremental_relay_feerate = CFeeRate{ConsumeMoney(fuzzed_data_provider, 1'000'000)},
+        .min_relay_feerate = CFeeRate{ConsumeMoney(fuzzed_data_provider, 1'000'000)},
+        .dust_relay_feerate = CFeeRate{ConsumeMoney(fuzzed_data_provider, 1'000'000)}
+    };
+    node.mempool = std::make_unique<CTxMemPool>(mempool_opts, error);
+    std::unique_ptr<CBlockPolicyEstimator> fee_estimator = std::make_unique<FuzzedBlockPolicyEstimator>(fuzzed_data_provider);
+    g_setup->SetFeeEstimator(std::move(fee_estimator));
+    auto target_feerate{CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/1'000'000)}};
+    if (target_feerate > node.mempool->m_opts.incremental_relay_feerate &&
+        target_feerate > node.mempool->m_opts.min_relay_feerate) {
+        MockMempoolMinFee(target_feerate, *node.mempool);
+    }
     std::unique_ptr<CWallet> wallet_ptr{std::make_unique<CWallet>(node.chain.get(), "", CreateMockableWalletDatabase())};
     CWallet& wallet{*wallet_ptr};
     {