#36068 fuzz: reuse one fuzzed wallet across inputs
| Reviewability | Reviews | Agreement | Size | ||||
|---|---|---|---|---|---|---|---|
| #36068 fuzz: reuse one fuzzed wallet across inputs | Ready | 2 | Strong | 19 + 124 tests | |||
|
|
|
|
| |||
Reviewability: ReadyStalePaused Agreement: StrongPositivePositive w/ caveatsNeutralMildDisputedBlockedCrickets Reviews: current code-review ACKs, then (+stale ACKs) and -NACKs; greener = more ACKs. Size: lines added or modified outside tests, then in tests; greener = smaller. | |||||||
Goal
Reuses a single FuzzedWallet instance across iterations in the wallet fuzz harnesses instead of rebuilding it per input. A new test-only method CWallet::ClearInMemoryTxStateForTest resets transaction and coin-spend maps in memory without reimporting keys or touching descriptor state. FuzzedWallet now precomputes a destination pool, clears wallet transaction state per run, and periodically rebuilds the wallet after every 1000 iterations.
Problem: Constructing FuzzedWallet for every fuzz iteration imports eight descriptors and incurs BIP32 derivations and multiple database transactions before reaching the target code, severely limiting fuzz execution throughput.
Evidence of importance:
- Author reports a ~6x speedup on an Ubuntu machine without sanitizers (PR description).
Scope: Adds a test-only method (ClearInMemoryTxStateForTest) directly to CWallet in production source files to allow harnesses to reset private in-memory transaction state.
Risk of a mistake: low (wallet, tests). ClearInMemoryTxStateForTest is test-only, so an error can only cause false positives or masked issues in fuzz testing rather than affecting production nodes.
Changed since the previous assessment: Added a periodic rebuild every 1000 iterations to bound wallet state growth and an RAII drift check asserting bounded descriptor range movement during test execution.
Reviewability
Ready
Nothing known that would invalidate a review now.
Waiting on (from the thread read):
- reviewer: review from marcofleon and Crypt-iQ 2026-10-21
Agreement
Strong
substantive support, no open objection (jeanpablojp, frankomosh, frankomosh)
Thread notes: Added fanquake to participants as neutral after pinging additional reviewers. PR is waiting on those reviewers.
Objections, suggestions, and questions:
| Reviewer | Type | Harm, request, or question | Status | Must be | Author replied | Quote |
|---|---|---|---|---|---|---|
| jeanpablojp (contributor) | objection, correctness | descriptor next_index drift causes successive iterations to derive different change addresses and invalidates drift assertion comment assumptions | fixed · fix pushed · addressed by force-push | answered | yes | 2026-08-29: 'The descriptor's `next_index` isn't reset here, so the same input builds a different transaction depending on how many ran before it.' Status from: 2026-09-07: brunoerg: 'Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/36068#discussion_r3887572966' |
| jeanpablojp (contributor) | objection, safety | clearing mapWallet before wtxOrdered and m_txos leaves latent dangling references | answered · author gave rationale and reviewer did not push back | answered | yes | 2026-08-29: 'mapWallet.clear() runs before wtxOrdered and m_txos, and both point into it. Nothing dereferences in between so it's latent' Status from: 2026-09-07: brunoerg: 'No reason, will leave as is but can think about it in case I have to touch it again. Thanks.' |
| frankomosh (contributor) | objection, correctness | checking state at the start of an input causes leaks to be reported on the subsequent input, which does not reproduce in isolation | fixed · fix pushed · addressed by force-push | answered | yes | 2026-09-22: 'when one input leaks, the crash is reported on the next input, which then does not reproduce on its own' Status from: 2026-09-22: brunoerg: 'Force-pushed addressing https://github.com/bitcoin/bitcoin/pull/36068#discussion_r4070883464' |
| frankomosh (contributor) | suggestion, correctness | assert inserted output counts in the fuzzed wallet to catch future regressions | answered · author deferred to a follow-up PR | answered | yes | 2026-09-24: 'Asserting inserted output counts here would help catch any future regression in this harness' Status from: 2026-10-19: brunoerg: 'This is a good one, but I'll leave this for a follow-up PR' |
| frankomosh (contributor) | suggestion, correctness | assert that CreateTransaction result fee is non-negative | answered · author deferred to a follow-up PR | answered | yes | 2026-09-24: 'Also, I think we could do an assert check on the result here ?..had also suggested this elswhere' Status from: 2026-10-19: brunoerg: 'Same: https://github.com/bitcoin/bitcoin/pull/36068#discussion_r4316065579' |
Support:
- jeanpablojp (contributor) (Approach ACK, 2026-08-29) [tested]: Ran the qa-assets corpus on master and on this head, the speedup reproduces, around 4x here.
- frankomosh (contributor) (Concept ACK, 2026-09-22): improving performance
- frankomosh (contributor) (Tested ACK, 2026-09-24) [tested]: This improves performance of this harness.
- maflcko (member) (ACK, 2026-10-20): stability impact is acceptable; lgtm [not substantive]
Participants:
- jeanpablojp (contributor, objection, 3 statements 2026-08-29 to 2026-08-29): Approach ACK with ~4x speedup benchmark; raised concerns on next_index drift and mapWallet clear order
- frankomosh (contributor, objection, 5 statements 2026-09-22 to 2026-09-24): Tested ACK (previously Concept ACK); raised concern on leak check timing and suggested additional assertions in spend harness
- sedited (member, neutral, 1 statements 2026-10-19 to 2026-10-19): asked whether the author intends to take frankomosh's suggestion nits
- maflcko (member, support, 2 statements 2026-10-20 to 2026-10-20): review ACK, noting that potential stability degradation is acceptable given existing baseline
- fanquake (member, neutral, 1 statements 2026-10-21 to 2026-10-21): pinged additional reviewers
Files
141 lines under test/bench/ci.
- src/test/fuzz/util/wallet.h +105/-12
- src/wallet/test/fuzz/spend.cpp +19/-5
- src/wallet/wallet.cpp +11/-0
- src/wallet/wallet.h +8/-0
Card
The short text the ranking pass compares across PRs.
This pull request optimizes wallet fuzzing by caching a single FuzzedWallet across inputs rather than reconstructing it every iteration. It adds a test-only CWallet::ClearInMemoryTxStateForTest method to wipe in-memory transaction and spend maps, while FuzzedWallet pools precomputed destinations and rebuilds every 1000 iterations. Rebuilding the wallet per input was paying descriptor imports and BIP32 derivation costs on every test run. The author benchmarks a roughly 6x speedup without sanitizers.
Data
thread read through 2026-10-21 (22 statements), head 12d9ed130f
labels: Fuzzing
Categories
Test infrastructure
P3 · speedup
Significantly accelerates wallet fuzzing throughput by 4x to 6x by reusing a single FuzzedWallet instance instead of reimporting descriptors on each iteration. The PR description reports a "~6x speedup on an Ubuntu machine without sanitizers," which was confirmed by reviewer jeanpablojp who noted "the speedup reproduces, around 4x here."
Membership: Carries the Fuzzing label and updates fuzz infrastructure in src/test/fuzz/util/wallet.h and the spend fuzz harness.
Factors: security/stability 1, bug 0, performance 1, user value 0, leverage 1