Wallet fuzzing tracking issue #29901

1. 
   brunoerg commented at 4:15 pm on April 17, 2024: contributor

   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

   • #29694
   • #29936
   • #30134
   • #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
   • Wallet RPC (https://github.com/bitcoin/bitcoin/issues/30458 - good first issue)

   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 (https://github.com/bitcoin/bitcoin/issues/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 - #28574)

   ---

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

2. 
   maflcko commented at 5:03 pm on April 17, 2024: member

   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:

   • #23444 tried to add a regression fuzz test, but it was picked up as part of #28933, and I couldn’t reproduce the regression crash by re-introducing it.
   • #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
3. laanwj added the label Wallet on Apr 17, 2024
4. laanwj added the label Tests on Apr 17, 2024
5. 
   brunoerg commented at 12:37 pm on April 18, 2024: contributor

   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.

6. 
   brunoerg commented at 7:44 pm on April 22, 2024: contributor

   Just added #29936 to the list. It’s a regression to #27271.
7. 
   brunoerg commented at 8:48 pm on May 17, 2024: contributor

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

   edit: merged

8. 
   kevkevinpal commented at 1:08 pm on July 23, 2024: contributor

   I started to take a look at the Wallet RPC issue, I’m still fairly new to fuzz testing so it might take longer but I am working off of this branch if you would like to track my progress.
9. 
   kevkevinpal commented at 3:45 pm on July 30, 2024: contributor

   I was able to get the coverage to

   0Lines: 32%
   1Functions 50%
   

   I think I just need to run the fuzzer for longer to get better coverage but I can do more testing and also cleanup my branch. If you would like the qa-assets for wallet-rpc please let me know.

   

10. 
    murchandamus commented at 8:10 pm on July 30, 2024: contributor

    Given that the first segment of your seeds need to correspond to an RPC command, a dictionary might help with bootstrapping. I’m also not particularly knowledgeable about building fuzz tests, but it seems to me that it might be better to build a separate harness for each RPC?
11. 
    maflcko commented at 8:16 pm on July 30, 2024: member

    Most fuzz engines will create the dictionary themselves, by extracting the string literals. If not, -use_value_profile=1 may be useful to “recover” the RPC name.

    If you look at the coverage line-by-line, you will be able to see where the fuzz engine returns early. The right fix will then usually be obvious.

12. 
    brunoerg commented at 4:55 pm on November 11, 2024: contributor

    Removed #30570 since it’s closed.
13. 
    brunoerg commented at 5:57 pm on December 18, 2024: contributor

    Not sure about performance issues, but a fuzz target to cover #31474 would be great?

Contributors
brunoerg maflcko kevkevinpal murchandamus

Labels
Wallet Tests