fuzz: several improvements to scriptpubkeyman harness #34969

brunoerg commented at 7:10 PM on March 31, 2026: contributor

The scriptpubkeyman harness is slow - I'm getting under 40 exec/s on my personal server. This PR solves the following issues that may be affecting the performance:

There are redundant IsMine calls
ConsumeCoins always create up to 10'000 coins which may cause slowness.
There are some expensive ops in a loop - e.g. One of the slow units I got on my server is about a repeated sequence of GetNewDestination calls (see flamegraph below).
We're always calling GetDescriptorString unconditionally. It looks like an inofensive function but this is extremely costly due to calls to ToNormalizedString/ToPrivateString functions that touch key operations.
Not related to slowness, but there is a bug on harness because we are aborting the execution when we cannot update the wallet descriptor, we can just skip and continue.

spkm-updated

DrahtBot added the label Fuzzing on Mar 31, 2026

DrahtBot commented at 7:11 PM on March 31, 2026: 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/34969.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	pablomartin4btc

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

No conflicts as of last run.

andrewtoth commented at 9:04 PM on March 31, 2026: contributor

Getting decent throughput when running on my laptop:

$ FUZZ=scriptpubkeyman ./build_fuzz_nosan/bin/fuzz ../qa-assets/fuzz_corpora/scriptpubkeyman/
...
[#3326974](/bitcoin-bitcoin/3326974/)	REDUCE cov: 14175 ft: 84867 corp: 5289/37Mb lim: 998620 exec/s: 1319 rss: 262Mb L: 15776/882699 MS: 1 EraseBytes-
[#3327186](/bitcoin-bitcoin/3327186/)	REDUCE cov: 14175 ft: 84867 corp: 5289/37Mb lim: 998620 exec/s: 1319 rss: 262Mb L: 961/882699 MS: 2 ChangeBit-EraseBytes-
[#3328687](/bitcoin-bitcoin/3328687/)	REDUCE cov: 14175 ft: 84867 corp: 5289/37Mb lim: 998620 exec/s: 1319 rss: 262Mb L: 1249/882699 MS: 1 EraseBytes-
...

How is your server configured? Do you see a big speedup after applying this patch?

brunoerg commented at 1:11 AM on April 1, 2026: contributor

Getting decent throughput when running on my laptop:

What are the specs? How did you build it? Building with --preset=libfuzzer on my Ubuntu machine (AMD Ryzen 9 7900 / 32GB RAM), I get around 20 exec/s. With this PR, on same machine, I could reach similar coverage from master with an avg of 150 exec/s.

andrewtoth commented at 3:07 AM on April 1, 2026: contributor

I'm on a i9-14900 with 96 GB RAM.

I was running with only fuzzer sanitizer. When --preset=libfuzzer that uses undefined,address,fuzzer. With those I get a lot slower. It started at at 39 but after a few minutes was up to 128 exec/s. I checked out this PR and then it started at 128 and after a few minutes was up to 385 exec/s. Not bad!

Running with only fuzzer sanitizer on this branch however didn't seem to speed up it was around 1100 exec/s.

brunoerg commented at 12:13 PM on April 1, 2026: contributor

I'm on a i9-14900 with 96 GB RAM.

I was running with only fuzzer sanitizer. When --preset=libfuzzer that uses undefined,address,fuzzer. With those I get a lot slower. It started at at 39 but after a few minutes was up to 128 exec/s. I checked out this PR and then it started at 128 and after a few minutes was up to 385 exec/s. Not bad!

Running with only fuzzer sanitizer on this branch however didn't seem to speed up it was around 1100 exec/s.

Ah, got it. This is expected, because ASan instruments most memory accesses in instrumented code and tracks heap allocations, while UBSan adds runtime checks for selected undefined behaviors. So operations like GetScriptPubKeys(), ConsumeCoins(), and some string operations, such as GetDescriptorString() can become more expensive under these sanitizers.

in src/wallet/test/fuzz/scriptpubkeyman.cpp:131 in 1d826e5b8c outdated

 126 | -                }
 127 | -            },
 128 |              [&] {
 129 |                  auto spks{spk_manager->GetScriptPubKeys()};
 130 |                  for (const CScript& spk : spks) {
 131 | -                    assert(spk_manager->IsMine(spk));

maflcko commented at 10:53 AM on April 10, 2026:

1d826e5b8c49f8ebb8acd84c50861ea65c4129f8: Can you explain this a bit better? Sure, it may be internally covered in MarkUnusedAddresses, but the goal of fuzz tests is not only to cover code, but also to assert on results or assumptions

brunoerg commented at 1:48 PM on April 13, 2026:

I've changed this commit to say that it's removing excessive IsMine calls (e.g. calling it for every single spk from GetScriptPubKeys. In a later commit, I add the assert back but in a smoothly way.

pablomartin4btc commented at 1:28 AM on August 11, 2026:

Following up on @maflcko's concern about assertions: the assert is restored in the last commit but only for one randomly picked script from GetScriptPubKeys(). The original code asserted IsMine for every script in the set — a stronger invariant that would catch a bug where some (but not all) returned scripts fail IsMine.

Also, the inverse direction check from the old code is gone entirely: if (IsMine(random_script)) → assert(GetScriptPubKeys().contains(script)). That covered the other direction of the invariant — a bug where IsMine claims ownership of a script that isn't listed in GetScriptPubKeys().

Since spks is already computed once outside the loop, the full assertion can be kept cheaply:

for (const CScript& spk : spks) {
    assert(spk_manager->IsMine(spk));
}

The expensive part was calling GetScriptPubKeys() repeatedly in the loop — iterating over the already-computed spks is negligible.

brunoerg commented at 1:04 PM on August 19, 2026:

Yes, I agree. Just changed it to full assertion since it's cheap.

brunoerg force-pushed on Apr 13, 2026

DrahtBot added the label Needs rebase on Apr 29, 2026

brunoerg force-pushed on Apr 30, 2026

DrahtBot removed the label Needs rebase on Apr 30, 2026

DrahtBot added the label CI failed on Apr 30, 2026

DrahtBot commented at 7:58 PM on April 30, 2026: contributor

🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/25183113515/job/73833297823</sub> <sub>LLM reason (✨ experimental): CI failed because the IWYU include/format check for src/util/feefrac.h generated a failure (^^^ ⚠️ Failure generated from IWYU).</sub>

<details><summary>Hints</summary>

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.

</details>

brunoerg force-pushed on Apr 30, 2026

DrahtBot removed the label CI failed on Apr 30, 2026

DrahtBot added the label Needs rebase on May 5, 2026

brunoerg force-pushed on May 5, 2026

brunoerg commented at 2:42 PM on May 5, 2026: contributor

Rebased

DrahtBot removed the label Needs rebase on May 5, 2026

sedited requested review from marcofleon on May 26, 2026

ekzyis commented at 6:25 PM on June 13, 2026: contributor

Building with --preset=libfuzzer on my Ubuntu machine (AMD Ryzen 9 7900 / 32GB RAM), I get around 20 exec/s

I get around 2000 exec/s on master (4c99ed1076) and 40786bb2f8 with --preset=libfuzzer.

I also fuzzed the scriptpubkeyman target for 10 minutes on both commits, and got a similar amount of runs:

$ tail -1 fuzz-scriptpubkeyman-4c99ed1076.txt
Done 1191929 runs in 601 second(s)
$ tail -1 fuzz-scriptpubkeyman-40786bb2f8.txt
Done 1195257 runs in 601 second(s)

Intel(R) Xeon(R) CPU E3-1275 v6 @ 4.20GHz with 64GB RAM

I thought maybe the latest changes to the harness since your comments changed performance:

$ git log --oneline --since=2026-04-01 master -- src/wallet/test/fuzz/scriptpubkeyman.cpp
c6f225c757c Merge bitcoin/bitcoin#28333: wallet: Construct ScriptPubKeyMans with all data rather than loaded progressively
c568624ff29 psbt: Return std::optional from PrecomputePSBTData
c01c7f068c5 psbt: Remove default constructor
6538f691357 fuzz: Skip adding descriptor to wallet if it cannot be expanded
0301c758ea0 wallet migration, fuzz: Migrate hd seed once
dc4a5d1270f refactor: use PSBTFillOptions for filling and signing

I checked out c6f225c757c and c6f225c757c^. Same performance with around 2000 exec/s:

$ tail -1 fuzz-scriptpubkeyman-5486ef8cc2.txt
Done 1198053 runs in 601 second(s)
$ tail -1 fuzz-scriptpubkeyman-c6f225c757.txt
Done 1124156 runs in 601 second(s)

So not sure where the low exec/s on your machines are coming from.

brunoerg commented at 11:55 AM on June 16, 2026: contributor

I get around 2000 exec/s on master (https://github.com/bitcoin/bitcoin/commit/4c99ed10766c7fac1353d1402d1b99de4c656c08) and https://github.com/bitcoin/bitcoin/commit/40786bb2f8c01fe80f96b9e1dce610d043b800f2 with --preset=libfuzzer.

Running from our corpus?

ekzyis commented at 4:15 PM on June 26, 2026: contributor

Running from our corpus?

Oh, no. With the scriptpubkeyman corpus from qa-assets, I get about 200–300 exec/s on the same machine. Thanks, this clears up my confusion! Sorry for the noise

DrahtBot added the label Needs rebase on Jul 14, 2026

brunoerg force-pushed on Jul 15, 2026

brunoerg commented at 4:10 PM on July 15, 2026: contributor

Rebased

DrahtBot removed the label Needs rebase on Jul 15, 2026

in src/wallet/test/fuzz/scriptpubkeyman.cpp:181 in 08499b1dcb outdated

 177 | +    }
 178 | +
 179 | +    if (fuzzed_data_provider.ConsumeBool()) {
 180 | +        if (auto opt_tx_to{ConsumeDeserializable<CMutableTransaction>(fuzzed_data_provider, TX_WITH_WITNESS)}) {
 181 | +            CMutableTransaction tx_to;
 182 | +            tx_to = *opt_tx_to;

pablomartin4btc commented at 5:08 PM on August 10, 2026:

minor nit: redundant default construction + assignment...

            auto tx_to = {*opt_tx_to};

brunoerg commented at 1:05 PM on August 19, 2026:

will leave as-is for now.

pablomartin4btc commented at 1:45 AM on August 11, 2026: member

Concept ACK

A real improvement — the early return bug fix and moving expensive ops (GetDescriptorString, GetNewDestination) out of the hot path are the right calls.

Left a comment inline on the assertion coverage (not blocking).

fuzz: spkm: remove excessive IsMine calls

Avoid calling IsMine for every single spkm from
GetScriptPubKeys(). Also, expands `MarkUnusedAddresses`
which covers it. In a later commit, we introduce the
assertion back in a better way.

84a334ceda

fuzz: add a num_coins parameter to ConsumeCoins

This new parameter is used to control the max number of coins
this function might create. It allows us to create less than
10'000 coins and then reduce the cost.

13aba9b969

fuzz: spkm: reduce number of created coins 5aca41ac46

fuzz: spkm: move expensive ops out of the loop ae9779616d

fuzz: spkm: do not call GetDescriptorString unconditionally

GetDescriptorString calls ToNormalizedString or ToPrivateString,
both of which rebuild the full descriptor string. This is expensive.

5d67ca2c31

fuzz: spkm: fix early return on updating the wallet descriptor 23ed87cab4

brunoerg force-pushed on Aug 19, 2026

fuzz: spkm: assert the IsMine/GetScriptPubKeys invariant

Assert both directions of the invariant, using the `spks` set that is
now computed only once:

- every script from GetScriptPubKeys() is IsMine;
- every IsMine script is in GetScriptPubKeys().

The latter is asserted before MarkUnusedAddresses(), which may TopUp()
and add scripts that are not part of the already computed `spks` set.

0b0df80ba0

brunoerg force-pushed on Aug 19, 2026

DrahtBot added the label CI failed on Aug 19, 2026

DrahtBot removed the label CI failed on Aug 19, 2026