fuzz: enhance wallet_fees by mocking mempool stuff

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.

fuzz: set mempool options in wallet_fees 19273d0705

DrahtBot added the label Tests on Aug 18, 2025

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, 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)
#26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)

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.

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.

brunoerg force-pushed on Aug 18, 2025

brunoerg requested review from maflcko on Aug 20, 2025

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.

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.

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.

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

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.

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==

maflcko commented at 7:28 am on August 29, 2025: member

.

fees: make estimateSmartFee/HighestTargetTracked virtual for mocking f591c3beca

brunoerg force-pushed on Aug 29, 2025

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}

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.

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.

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.

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==

fuzz: mock CBlockPolicyEstimator in wallet_fuzz ff10a37e99

fuzz: create FeeEstimatorTestingSetup to set fee_estimator adf67eb21b

brunoerg force-pushed on Sep 1, 2025

brunoerg commented at 12:39 pm on September 1, 2025: contributor

Force-pushed addressing #33210 (review), #33210 (review) and #33210 (review).

DrahtBot added the label CI failed on Sep 1, 2025

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.

brunoerg commented at 1:05 pm on September 1, 2025: contributor

Oops, forgot to fix the unit test, doing this.

test: move MockMempoolMinFee to util/txmempool c9a7a198d9

fuzz: MockMempoolMinFee in wallet_fees 5ded99a7f0

brunoerg force-pushed on Sep 1, 2025

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==

ismaelsadeeq commented at 2:52 pm on September 1, 2025: member

Concept ACK

DrahtBot removed the label CI failed on Sep 1, 2025

in src/wallet/test/fuzz/fees.cpp:19 in ff10a37e99 outdated

15@@ -16,6 +16,25 @@ namespace wallet {
16 namespace {
17 TestingSetup* g_setup;
18 
19+class FuzzedBlockPolicyEstimator : public CBlockPolicyEstimator

ismaelsadeeq commented at 11:10 am on September 15, 2025:

In “fuzz: mock CBlockPolicyEstimator in wallet_fuzz” ff10a37e99271125a9ece92bae571f7b78fb9e22

It would be nice if FuzzedBlockPolicyEstimator were renamed to MockedBlockPolicyEstimator. It could then be moved to src/test/util/ so that it can be reused by other components that might need a fee rate estimate as well.

brunoerg commented at 6:27 pm on September 15, 2025:

The idea of FuzzedBlockPolicyEstimator is to have a way of using the buffer to determine the returned value of some functions. Mocked (e.g. in unit tests) is more common when we leave the values fixed.

in src/wallet/test/fuzz/fees.cpp:30 in adf67eb21b outdated

26+    }
27+
28+    void SetFeeEstimator(std::unique_ptr<CBlockPolicyEstimator> fee_estimator)
29+    {
30+        m_node.fee_estimator = std::move(fee_estimator);
31+    }

ismaelsadeeq commented at 11:28 am on September 15, 2025:

In “fuzz: create FeeEstimatorTestingSetup to set fee_estimator” adf67eb21baf39a222b65480e45ae76f093e8f66

nit: It will be nice if we don’t have to set the fee estimator and just initialize it in the FeeEstimatorTestingSetup constructor, but we have to find a way to pass the fuzz data provider as well.

ismaelsadeeq approved

ismaelsadeeq commented at 11:33 am on September 15, 2025: member

Code review ACK 5ded99a7f007b142f6b0ec89e0c71ef281b42684 with a comment and nit happy to reACK when you amend my suggestion

DrahtBot requested review from ismaelsadeeq on Sep 15, 2025

brunoerg commented at 12:42 pm on September 23, 2025: contributor

rfm?

achow101 merged this on Oct 24, 2025

achow101 closed this on Oct 24, 2025

TheCharlatan referenced this in commit f50d90fcbb on Oct 29, 2025

fanquake added the label Fuzzing on Oct 30, 2025

fuzz: enhance wallet_fees by mocking mempool stuff #33210

Code Coverage & Benchmarks

Reviews

Conflicts