Wallet fuzzing tracking issue

[brunoerg]

The wallet has poor fuzz coverage. Hopefully, some work is being done to improve it. The goal of this issue is to actively track current work and work that needs to be done to improve fuzz coverage for the wallet.

Open PRs / Ready to review

• fuzz: add more coverage for ScriptPubKeyMan #30134
• fuzz: wallet: add target for CreateTransaction #29936
• fuzz: wallet: add target for spkm migration #29694
• fuzz: wallet, add target for Spend #28236
• fuzz: wallet, add target for Crypter #28074

Current wallet targets

We currently have 6 specific targets for wallet, which cover:

• Fees (wallet/fees.cpp)
• Coin selection
• ScriptPubKeyManager (descriptor)
• Coin control (CCoinControl)
• Notifications
• ISO8601 parser

Nice to have

• Feebumper target
• More coverage for CWallet stuff
• Wallet transaction target
• Wallet receive target
• CoinsResult target

Won't/Can't cover

Legacy wallet removal

The goal is to remove legacy wallets and migrate them to descriptor ones. There is an open issue with a proposed timeline for Legacy Wallet and BDB removal (#20160). For this reason, we do not aim to increase fuzz coverage for legacy stuff. See that the scriptpubkeyman target only uses descriptor ones.

External signer

I do believe we can't have fuzz coverage for external signer code.

Performance and determinism

Unfortunately, some aspects of the wallet may affect the fuzzing performance. E.g.:

• SetupDescriptorScriptPubKeyMans (it might also be non-deterministic due to key generation)
• Wallet encryption
• Wallet migration (there is a PR improving it - wallet: optimize migration process, batch db transactions #28574)

---

Any ideas about it or PRs to add let me know.

[maflcko]

Coverage is nice, but if the fuzz test never finds any real user-facing issues, it will be useless. I don't think any wallet fuzz target found a real issue yet? Obviously coverage is the first step toward adding regression fuzz tests, so here are some ideas:

• fuzz: Add regression test for wallet crash #23444 tried to add a regression fuzz test, but it was picked up as part of fuzz: Faster wallet_notifications target #28933, and I couldn't reproduce the regression crash by re-introducing it.
• RPC: Fix fund transaction crash when at 0-value, 0-fee #27271 would be another idea to check
• Something else?

SetupDescriptorScriptPubKeyMans (it might also be non-deterministic due to key generation)

I wrote the ImportDescriptors helper, which should be deterministic.

Edit: Also links to the current coverage:

• https://maflcko.github.io/b-c-cov/fuzz.coverage/src/wallet/index-sort-l.html
• https://storage.googleapis.com/oss-fuzz-coverage/bitcoin-core/reports/20240416/linux/src/bitcoin-core/src/wallet/report.html

[brunoerg]

Coverage is nice, but if the fuzz test never finds any real user-facing issues, it will be useless. I don't think any wallet fuzz target found a real issue yet?

I think we found a real issue with the coin selection target after the rework. But yes, it would be the only one.

Coverage is nice, but if the fuzz test never finds any real user-facing issues, it will be useless. I don't think any wallet fuzz target found a real issue yet? Obviously coverage is the first step toward adding regression fuzz tests, so here are some ideas:

• fuzz: Add regression test for wallet crash #23444 tried to add a regression fuzz test, but it was picked up as part of fuzz: Faster wallet_notifications target #28933, and I couldn't reproduce the regression crash by re-introducing it.
• RPC: Fix fund transaction crash when at 0-value, 0-fee #27271 would be another idea to check
• Something else?

I'm working on a target to cover and reproduce #27271.

[brunoerg]

Just added #29936 to the list. It's a regression to #27271.

[brunoerg]

Added #30134 to the list for review. It adds more coverage for ScriptPubKeyMan.