fuzz: enhance wallet_fees by mocking mempool stuff #33210

pull brunoerg wants to merge 6 commits into bitcoin:master from brunoerg:2025-08-fuzz-improve-wallet-fees changing 7 files +108 −53
  1. brunoerg commented at 12:25 pm on August 18, 2025: contributor

    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 fae8c73d9e4eba4603447bb52b6e3e760fbf15f8.

  2. fuzz: set mempool options in wallet_fees 19273d0705
  3. DrahtBot added the label Tests on Aug 18, 2025
  4. DrahtBot commented at 12:25 pm on August 18, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33210.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK maflcko
    Concept ACK ismaelsadeeq

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32747 (Introduce SockMan (“lite”): low-level socket handling for HTTP by pinheadmz)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

  5. in src/test/fuzz/util/mempool.cpp:37 in c4eee44d3a outdated 
    28@@ -29,3 +29,21 @@ CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider,
29     const unsigned int sig_op_cost = fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, MAX_BLOCK_SIGOPS_COST);
30     return CTxMemPoolEntry{MakeTransactionRef(tx), fee, time, entry_height, entry_sequence, spends_coinbase, sig_op_cost, {}};
31 }
32+
33+void ConsumeMempoolMinFee(FuzzedDataProvider& fuzzed_data_provider, CTxMemPool& mempool) noexcept
34+{
35+    LOCK2(cs_main, mempool.cs);
36+    CMutableTransaction mtx = CMutableTransaction();
37+    mtx.vin.emplace_back(COutPoint{Txid::FromUint256(ConsumeUInt256(fuzzed_data_provider)), 0});
    maflcko commented at 1:29 pm on August 18, 2025:

    Is the 256-byte value used at all? If not, seems easier to just hardcode a value (like 0 or 1 or 2), or use a deterministic random comtext?

    Also, it may be good to document what the goal of this function is. Maybe with an Assert(mempool.GetMinFee()==min_fee) or similar?

    brunoerg commented at 6:07 pm on August 18, 2025:

    Is the 256-byte value used at all? If not, seems easier to just hardcode a value (like 0 or 1 or 2), or use a deterministic random comtext?

    Good catch. I don’t think it’s used at all, we can simply use a hardcoded value instead of spending part of the buffer with it.

    brunoerg commented at 6:23 pm on August 18, 2025:
    Done.
  6. brunoerg force-pushed on Aug 18, 2025
  7. brunoerg requested review from maflcko on Aug 20, 2025
  8. in src/test/fuzz/util/mempool.cpp:36 in 0fe7c25a7a outdated 
    28@@ -29,3 +29,24 @@ CTxMemPoolEntry ConsumeTxMemPoolEntry(FuzzedDataProvider& fuzzed_data_provider,
29     const unsigned int sig_op_cost = fuzzed_data_provider.ConsumeIntegralInRange<unsigned int>(0, MAX_BLOCK_SIGOPS_COST);
30     return CTxMemPoolEntry{MakeTransactionRef(tx), fee, time, entry_height, entry_sequence, spends_coinbase, sig_op_cost, {}};
31 }
32+
33+void ConsumeMempoolMinFee(FuzzedDataProvider& fuzzed_data_provider, CTxMemPool& mempool) noexcept
34+{
35+    LOCK2(cs_main, mempool.cs);
36+    CMutableTransaction mtx = CMutableTransaction();
    maflcko commented at 6:52 am on August 29, 2025:
    nit in 0fe7c25a7a6086cabdf96c6ebd0e0de9b6e4176d: just CMutableTransaction mtx{}; is enough, no need to list the type twice.
    brunoerg commented at 3:41 pm on August 29, 2025:
    Done.
  9. in src/test/fuzz/util/mempool.cpp:51 in 0fe7c25a7a outdated 
    46+                /*time=*/0, /*entry_height=*/1, /*entry_sequence=*/0,
47+                /*spends_coinbase=*/true, /*sigops_cost=*/1, lp);
48+        changeset->Apply();
49+    }
50+    mempool.TrimToSize(0);
51+    assert(mempool.GetMinFee() > CFeeRate{0});
    maflcko commented at 7:01 am on August 29, 2025:
    nit in https://github.com/bitcoin/bitcoin/commit/0fe7c25a7a6086cabdf96c6ebd0e0de9b6e4176d: Why only assert >0 and not >fee, or even ==fee+min_inc_fee?
    maflcko commented at 7:27 am on August 29, 2025:
    or maybe just move MockMempoolMinFee to the src/test/util/mempool utils and use it? (Skip if the target fee is less than inc relay fee or min relay fee)?
    brunoerg commented at 3:41 pm on August 29, 2025:
    I moved MockMempoolMinFee to mempool utils to use it here and in the unit tests. I had to add one more parameter which is the mempool to be mocked.
  10. in src/test/fuzz/util/mempool.cpp:38 in 0fe7c25a7a outdated 
    33+void ConsumeMempoolMinFee(FuzzedDataProvider& fuzzed_data_provider, CTxMemPool& mempool) noexcept
34+{
35+    LOCK2(cs_main, mempool.cs);
36+    CMutableTransaction mtx = CMutableTransaction();
37+    mtx.vin.emplace_back(COutPoint{Txid::FromUint256(uint256{1}), 0});
38+    mtx.vout.emplace_back(1 * COIN, GetScriptForDestination(WitnessV0ScriptHash(CScript() << OP_TRUE)));
    maflcko commented at 7:02 am on August 29, 2025:
    nit in https://github.com/bitcoin/bitcoin/commit/0fe7c25a7a6086cabdf96c6ebd0e0de9b6e4176d: Can just be a default-constructed CScript?
    brunoerg commented at 3:42 pm on August 29, 2025:
    I think so, I can change it if I have to touch it again or in a follow-up.
  11. in src/wallet/test/fuzz/fees.cpp:20 in 79b28c0740 outdated 
    16@@ -17,6 +17,21 @@ namespace wallet {
17 namespace {
18 TestingSetup* g_setup;
19 
20+class FuzzedCBlockPolicyEstimator : public CBlockPolicyEstimator {
    maflcko commented at 7:15 am on August 29, 2025:
    nit in 79b28c0740c57aac5b91fd3aff20bc46b9229376: Could drop the C for new code? FuzzedBlockPolicyEstimator.
    maflcko commented at 7:16 am on August 29, 2025:
    also clang-format for new code?
    brunoerg commented at 3:40 pm on August 29, 2025:
    Done
  12. in src/wallet/test/fuzz/fees.cpp:42 in 79b28c0740 outdated 
    16@@ -17,6 +17,21 @@ namespace wallet {
17 namespace {
18 TestingSetup* g_setup;
19 
20+class FuzzedCBlockPolicyEstimator : public CBlockPolicyEstimator {
21+    FuzzedDataProvider& fuzzed_data_provider;
22+public:
23+    FuzzedCBlockPolicyEstimator(FuzzedDataProvider& provider)
24+        : CBlockPolicyEstimator(fs::path{}, false), fuzzed_data_provider(provider) {}
    maflcko commented at 7:18 am on August 29, 2025:
    nit in https://github.com/bitcoin/bitcoin/commit/79b28c0740c57aac5b91fd3aff20bc46b9229376: Seems to pass right now, even though it runs into fs path errors. Seems fine for now, but in the future there could be a pure abstract base class below CBlockPolicyEstimator (used by wallet_fees) and CBlockPolicyEstimator as well as FuzzedBlockPolicyEstimator could derive from it (final). But this can be done in a follow-up, if there is need to.
    brunoerg commented at 3:42 pm on August 29, 2025:
    Make sense, will leave it for a possible follow-up.
  13. maflcko commented at 7:28 am on August 29, 2025: member

    Are there any stats on increased coverage, or is this just checking more values in the same paths?

    lgtm ACK 84927d37745920412b270ad3502308f0ad8cb7ca 📠

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: lgtm ACK 84927d37745920412b270ad3502308f0ad8cb7ca 📠
3LwOaL7YJaoPKVQe54/zAKC/RoOtidVJw2FAu4exHHtZwutOT6fuCdQ/LpiDi5sjAw5vCNVpiU2x2NhnRjQ4DAA==
  14. maflcko commented at 7:28 am on August 29, 2025: member
    .
  15. fees: make estimateSmartFee/HighestTargetTracked virtual for mocking f591c3beca
  16. brunoerg force-pushed on Aug 29, 2025
  17. brunoerg commented at 9:37 pm on August 29, 2025: contributor

    Are there any stats on increased coverage, or is this just checking more values in the same paths?

    I’m going to share a coverage report but yes, it increases the coverage and reaches 100% of wallet/fees.cpp. One of the motivations for this PR is that I noticed that the following code was uncovered due to mempool min fee being always zero:

    0// Obey mempool min fee when using smart fee estimation
1CFeeRate min_mempool_feerate = wallet.chain().mempoolMinFee();
2if (feerate_needed < min_mempool_feerate) {
3    feerate_needed = min_mempool_feerate;
4    if (feeCalc) feeCalc->reason = FeeReason::MEMPOOL_MIN;
5}
  18. in src/wallet/test/fuzz/fees.cpp:56 in 5c195cee21 outdated 
    52@@ -34,9 +53,10 @@ FUZZ_TARGET(wallet_fees, .init = initialize_setup)
53     CTxMemPool::Options mempool_opts{
54         .incremental_relay_feerate = CFeeRate{ConsumeMoney(fuzzed_data_provider, 1'000'000)},
55         .min_relay_feerate = CFeeRate{ConsumeMoney(fuzzed_data_provider, 1'000'000)},
56-        .dust_relay_feerate = CFeeRate{ConsumeMoney(fuzzed_data_provider, 1'000'000)}
57-    };
58+        .dust_relay_feerate = CFeeRate{ConsumeMoney(fuzzed_data_provider, 1'000'000)}};
    maflcko commented at 7:41 am on September 1, 2025:
    nit in 5c195cee21e821ba85f4e0064be6d82037bdf73e: Unrelated change? Either revert, or add a trailing comma for the last field?
    brunoerg commented at 12:38 pm on September 1, 2025:
    Done, just reverted it.
  19. in src/test/util/txmempool.h:81 in 7d9443e504 outdated 
    76+ *                          Must be above max(incremental feerate, min relay feerate),
77+ *                          or 1sat/vB with default settings.
78+ * @param mempool           The mempool to mock the minimum feerate for. Must be empty
79+ *                          when called.
80+ */
81+void MockMempoolMinFee(const CFeeRate& target_feerate, const std::unique_ptr<CTxMemPool>& mempool);
    maflcko commented at 7:48 am on September 1, 2025:
    nit in 7d9443e504f65d861c76550ad927796e083d20dd: When passing a mutable reference, you can just use Type& ref. const std::unique_ptr<Type>& ref is similar, but more verbose and confusing.
    brunoerg commented at 12:38 pm on September 1, 2025:
    Done.
  20. in src/wallet/test/fuzz/fees.cpp:79 in dd4ff4d1dd outdated 
    73@@ -73,6 +74,9 @@ FUZZ_TARGET(wallet_fees, .init = initialize_setup)
74     node.mempool = std::make_unique<CTxMemPool>(mempool_opts, error);
75     std::unique_ptr<CBlockPolicyEstimator> fee_estimator = std::make_unique<FuzzedBlockPolicyEstimator>(fuzzed_data_provider);
76     g_setup->SetFeeEstimator(std::move(fee_estimator));
77+    auto target_feerate{CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/1'000'000)}};
78+    if (target_feerate > node.mempool->m_opts.incremental_relay_feerate &&
79+        target_feerate > node.mempool->m_opts.min_relay_feerate) MockMempoolMinFee(target_feerate, node.mempool);
    maflcko commented at 7:50 am on September 1, 2025:
    nit in dd4ff4d1ddd15213dd21413a39cff049a21b0fab: Could insert {} around the if body for multi-line if?
    brunoerg commented at 12:38 pm on September 1, 2025:
    Done.
  21. maflcko commented at 7:50 am on September 1, 2025: member

    re-ACK dd4ff4d1ddd15213dd21413a39cff049a21b0fab 🔟

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK dd4ff4d1ddd15213dd21413a39cff049a21b0fab 🔟
3kpezRcZswgT6wmKg9dzzknOYCcDJipWwOc5EPajG49a+DaKzcdOBuYXqbjZzvTDIOYZQpxWu1jWA+pVlQV+4Dg==
  22. fuzz: mock CBlockPolicyEstimator in wallet_fuzz ff10a37e99
  23. fuzz: create FeeEstimatorTestingSetup to set fee_estimator adf67eb21b
  24. brunoerg force-pushed on Sep 1, 2025
  25. brunoerg commented at 12:39 pm on September 1, 2025: contributor
    Force-pushed addressing #33210 (review), #33210 (review) and #33210 (review).
  26. DrahtBot added the label CI failed on Sep 1, 2025
  27. DrahtBot commented at 12:41 pm on September 1, 2025: contributor

    🚧 At least one of the CI tasks failed. Task macOS-cross, gui, no tests: https://github.com/bitcoin/bitcoin/runs/49328200705 LLM reason (✨ experimental): Compile error in test_bitcoin: MockMempoolMinFee calls pass a unique_ptr where a CTxMemPool& is required, causing no viable function match and failing the build.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  28. brunoerg commented at 1:05 pm on September 1, 2025: contributor
    Oops, forgot to fix the unit test, doing this.
  29. test: move MockMempoolMinFee to util/txmempool c9a7a198d9
  30. fuzz: MockMempoolMinFee in wallet_fees 5ded99a7f0
  31. brunoerg force-pushed on Sep 1, 2025
  32. maflcko commented at 2:47 pm on September 1, 2025: member

    re-ACK 5ded99a7f007b142f6b0ec89e0c71ef281b42684 🎯

    Signature:

    0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: re-ACK 5ded99a7f007b142f6b0ec89e0c71ef281b42684 🎯
3UQRB3KB6eMI44rzTEUX/7KKms2q1AD4HkP+qr/eelKu1N4MWq7+SHjJ454uspf+i6DcaxbyPbDiQkl3ECel4Cw==
  33. ismaelsadeeq commented at 2:52 pm on September 1, 2025: member
    Concept ACK
  34. DrahtBot removed the label CI failed on Sep 1, 2025


brunoerg DrahtBot maflcko ismaelsadeeq

Labels
Tests

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-09-02 06:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me