fuzz: enhance wallet_fees by mocking mempool stuff #33210

brunoerg · 2025-08-18T12:25:00Z

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

DrahtBot · 2025-08-18T12:25:05Z

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)
#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.

src/test/fuzz/util/mempool.cpp

maflcko · 2025-08-29T07:28:08Z

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

lgtm ACK 84927d3 📠

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 84927d37745920412b270ad3502308f0ad8cb7ca 📠
LwOaL7YJaoPKVQe54/zAKC/RoOtidVJw2FAu4exHHtZwutOT6fuCdQ/LpiDi5sjAw5vCNVpiU2x2NhnRjQ4DAA==

maflcko

.

src/test/fuzz/util/mempool.cpp

src/wallet/test/fuzz/fees.cpp

brunoerg · 2025-08-29T21:37:32Z

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:

// Obey mempool min fee when using smart fee estimation
CFeeRate min_mempool_feerate = wallet.chain().mempoolMinFee();
if (feerate_needed < min_mempool_feerate) {
    feerate_needed = min_mempool_feerate;
    if (feeCalc) feeCalc->reason = FeeReason::MEMPOOL_MIN;
}

maflcko

re-ACK dd4ff4d 🔟

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK dd4ff4d1ddd15213dd21413a39cff049a21b0fab 🔟
kpezRcZswgT6wmKg9dzzknOYCcDJipWwOc5EPajG49a+DaKzcdOBuYXqbjZzvTDIOYZQpxWu1jWA+pVlQV+4Dg==

src/wallet/test/fuzz/fees.cpp

src/test/util/txmempool.h

src/wallet/test/fuzz/fees.cpp

brunoerg · 2025-09-01T12:39:24Z

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

DrahtBot · 2025-09-01T12:41:12Z

🚧 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.}

Hints

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 · 2025-09-01T13:05:40Z

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

maflcko · 2025-09-01T14:47:59Z

re-ACK 5ded99a 🎯

Show signature

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: re-ACK 5ded99a7f007b142f6b0ec89e0c71ef281b42684 🎯
UQRB3KB6eMI44rzTEUX/7KKms2q1AD4HkP+qr/eelKu1N4MWq7+SHjJ454uspf+i6DcaxbyPbDiQkl3ECel4Cw==

ismaelsadeeq

Concept ACK

fuzz: set mempool options in wallet_fees

19273d0

DrahtBot added the Tests label Aug 18, 2025

maflcko reviewed Aug 18, 2025

View reviewed changes

src/test/fuzz/util/mempool.cpp Outdated Show resolved Hide resolved

brunoerg force-pushed the 2025-08-fuzz-improve-wallet-fees branch from c4eee44 to 84927d3 Compare August 18, 2025 18:23

brunoerg mentioned this pull request Jul 17, 2025

Wallet fuzzing tracking issue #29901

Open

13 tasks

brunoerg requested a review from maflcko August 20, 2025 13:31

maflcko reviewed Aug 29, 2025

View reviewed changes

fees: make estimateSmartFee/HighestTargetTracked virtual for mocking

f591c3b

brunoerg force-pushed the 2025-08-fuzz-improve-wallet-fees branch from 84927d3 to dd4ff4d Compare August 29, 2025 15:35

DrahtBot mentioned this pull request Aug 30, 2025

Introduce SockMan ("lite"): low-level socket handling for HTTP #32747

Open

maflcko reviewed Sep 1, 2025

View reviewed changes

src/wallet/test/fuzz/fees.cpp Outdated Show resolved Hide resolved

src/test/util/txmempool.h Outdated Show resolved Hide resolved

src/wallet/test/fuzz/fees.cpp Outdated Show resolved Hide resolved

brunoerg added 2 commits September 1, 2025 09:21

fuzz: mock CBlockPolicyEstimator in wallet_fuzz

ff10a37

fuzz: create FeeEstimatorTestingSetup to set fee_estimator

adf67eb

brunoerg force-pushed the 2025-08-fuzz-improve-wallet-fees branch from dd4ff4d to 3312763 Compare September 1, 2025 12:38

DrahtBot added the CI failed label Sep 1, 2025

brunoerg added 2 commits September 1, 2025 11:44

test: move MockMempoolMinFee to util/txmempool

c9a7a19

fuzz: MockMempoolMinFee in wallet_fees

5ded99a

brunoerg force-pushed the 2025-08-fuzz-improve-wallet-fees branch from 3312763 to 5ded99a Compare September 1, 2025 14:44

ismaelsadeeq reviewed Sep 1, 2025

View reviewed changes

DrahtBot removed the CI failed label Sep 1, 2025

DrahtBot mentioned this pull request Sep 3, 2025

test: add end-to-end tests for CConnman and PeerManager #26812

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

fuzz: enhance wallet_fees by mocking mempool stuff #33210

fuzz: enhance wallet_fees by mocking mempool stuff #33210

brunoerg commented Aug 18, 2025

Uh oh!

DrahtBot commented Aug 18, 2025 •

edited

Loading

Uh oh!

Uh oh!

maflcko commented Aug 29, 2025

Uh oh!

maflcko left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Aug 29, 2025

Uh oh!

maflcko left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Sep 1, 2025

Uh oh!

DrahtBot commented Sep 1, 2025

Uh oh!

brunoerg commented Sep 1, 2025

Uh oh!

maflcko commented Sep 1, 2025

Uh oh!

ismaelsadeeq left a comment

Uh oh!

Uh oh!

fuzz: enhance wallet_fees by mocking mempool stuff #33210

Are you sure you want to change the base?

fuzz: enhance wallet_fees by mocking mempool stuff #33210

Conversation

brunoerg commented Aug 18, 2025

Uh oh!

DrahtBot commented Aug 18, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

Uh oh!

maflcko commented Aug 29, 2025

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Aug 29, 2025

Uh oh!

maflcko left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Sep 1, 2025

Uh oh!

DrahtBot commented Sep 1, 2025

Uh oh!

brunoerg commented Sep 1, 2025

Uh oh!

maflcko commented Sep 1, 2025

Uh oh!

ismaelsadeeq left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Aug 18, 2025 •

edited

Loading