fuzz: wallet: add target for MigrateToDescriptor #32624

pull brunoerg wants to merge 1 commits into bitcoin:master from brunoerg:2025-05-fuzz-spkm-migration changing 1 files +145 −1
  1. brunoerg commented at 11:34 am on May 27, 2025: contributor

    This PR adds fuzz coverage for the scriptpubkeyman migration (MigrateToDescriptor). Note that it’s a test for the migration of the scriptpubkey manager, not for the whole migration process as tried in #29694, because:

    1. The wallet migration deals with DBs which is expensive for fuzzing (was getting around 3 exec/s);
    2. Mocking would require lots of refactors.

    This target loads keys, HDChain (even inactive ones), watch only and might add tons of different scripts, then calls MigrateToDescriptor. It does not play with encrypted stuff because it would make the target super slow. Also, after the migration there are some assertions that would work as a regression test for #31452, for example.

  2. DrahtBot commented at 11:34 am on May 27, 2025: 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/32624.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    ACK marcofleon
    Approach ACK w0xlt
    Stale ACK maflcko

    If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

  3. DrahtBot added the label Tests on May 27, 2025
  4. DrahtBot added the label CI failed on May 27, 2025
  5. in src/wallet/test/fuzz/scriptpubkeyman.cpp:59 in 08b574dde2 outdated 
    51@@ -50,6 +52,13 @@ void initialize_spkm()
52     MOCKED_DESC_CONVERTER.Init();
53 }
54 
55+void initialize_spkm_migration()
56+{
57+    static const auto testing_setup{MakeNoLogFileContext<const TestingSetup>()};
58+    g_setup = testing_setup.get();
59+    SelectParams(ChainType::MAIN);
    maflcko commented at 12:41 pm on May 27, 2025:
    why is this needed? Isn’t the chain already MAIN?
    brunoerg commented at 12:56 pm on May 27, 2025:
    Just removed it. Redudant.
  6. brunoerg force-pushed on May 27, 2025
  7. brunoerg commented at 12:56 pm on May 27, 2025: contributor
    08b574dde2c5f13a4c9dadcb9d8bded158c59623..5f2fb4dd333f8d1db27c78414afc84454e6ee4a5: Address #32624 (review)
  8. DrahtBot removed the label CI failed on May 27, 2025
  9. w0xlt commented at 10:26 pm on May 27, 2025: contributor
    Approach ACK
  10. brunoerg requested review from maflcko on Jun 5, 2025
  11. brunoerg force-pushed on Jun 16, 2025
  12. in src/wallet/test/fuzz/scriptpubkeyman.cpp:225 in 73439a78f3 outdated 
    220+    const auto& node{g_setup->m_node};
221+    Chainstate& chainstate{node.chainman->ActiveChainstate()};
222+    WalletContext context;
223+    auto& args{g_setup->m_args};
224+    context.args = const_cast<ArgsManager*>(&args);
225+    context.chain = g_setup->m_node.chain.get();
    maflcko commented at 1:42 pm on June 17, 2025:
    unused?
    brunoerg commented at 4:13 pm on June 17, 2025:
    Yes, thanks. It was a leftover from a previous approach, just removed it.
  13. brunoerg force-pushed on Jun 17, 2025
  14. maflcko commented at 8:44 pm on June 17, 2025: member
    lgtm ACK 6d29c9a6d1b4239a0844791d4a53599ae3a79cd0
  15. DrahtBot requested review from w0xlt on Jun 17, 2025
  16. achow101 removed review request from w0xlt on Oct 22, 2025
  17. achow101 requested review from marcofleon on Oct 22, 2025
  18. achow101 requested review from achow101 on Oct 22, 2025
  19. in src/wallet/test/fuzz/scriptpubkeyman.cpp:345 in 6d29c9a6d1 outdated 
    331+    auto result{legacy_data.MigrateToDescriptor()};
332+    size_t added_size{keys.size() + static_cast<size_t>(add_hd_chain) + static_cast<size_t>(add_inactive_hd_chain)};
333+    if (added_script > 0) {
334+        assert(result->desc_spkms.size() >= added_size);
335+    } else {
336+        assert(result->desc_spkms.size() == added_size);
    marcofleon commented at 4:21 pm on October 27, 2025: 
     0fuzz: ../../../../src/wallet/test/fuzz/scriptpubkeyman.cpp:335: void wallet::(anonymous namespace)::spkm_migration_fuzz_target(FuzzBufferType): Assertion `result->desc_spkms.size() == added_size' failed.
 1==1736884== ERROR: libFuzzer: deadly signal
 2    [#0](/bitcoin-bitcoin/0/) 0x5650eb34b2a4 in __sanitizer_print_stack_trace (/root/bitcoin/fuzzbuild/bin/fuzz+0x9fd2a4) (BuildId: 714ae02d0bf0aee30b5aead68737ef9e6f49c96a)
 3    [#1](/bitcoin-bitcoin/1/) 0x5650eb3211d8 in fuzzer::PrintStackTrace() (/root/bitcoin/fuzzbuild/bin/fuzz+0x9d31d8) (BuildId: 714ae02d0bf0aee30b5aead68737ef9e6f49c96a)
 4    [#2](/bitcoin-bitcoin/2/) 0x5650eb3075f3 in fuzzer::Fuzzer::CrashCallback() (/root/bitcoin/fuzzbuild/bin/fuzz+0x9b95f3) (BuildId: 714ae02d0bf0aee30b5aead68737ef9e6f49c96a)
 5    [#3](/bitcoin-bitcoin/3/) 0x7f07fa81d04f  (/lib/x86_64-linux-gnu/libc.so.6+0x3c04f) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
 6    [#4](/bitcoin-bitcoin/4/) 0x7f07fa86be2b  (/lib/x86_64-linux-gnu/libc.so.6+0x8ae2b) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
 7    [#5](/bitcoin-bitcoin/5/) 0x7f07fa81cfb1 in raise (/lib/x86_64-linux-gnu/libc.so.6+0x3bfb1) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
 8    [#6](/bitcoin-bitcoin/6/) 0x7f07fa807471 in abort (/lib/x86_64-linux-gnu/libc.so.6+0x26471) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
 9    [#7](/bitcoin-bitcoin/7/) 0x7f07fa807394  (/lib/x86_64-linux-gnu/libc.so.6+0x26394) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
10    [#8](/bitcoin-bitcoin/8/) 0x7f07fa815eb1 in __assert_fail (/lib/x86_64-linux-gnu/libc.so.6+0x34eb1) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
11    [#9](/bitcoin-bitcoin/9/) 0x5650eb6cd1c5 in wallet::(anonymous namespace)::spkm_migration_fuzz_target(std::span<unsigned char const, 18446744073709551615ul>) /root/bitcoin/fuzzbuild/src/test/fuzz/../../../../src/wallet/test/fuzz/scriptpubkeyman.cpp:335:9
12    [#10](/bitcoin-bitcoin/10/) 0x5650eb6df861 in std::function<void (std::span<unsigned char const, 18446744073709551615ul>)>::operator()(std::span<unsigned char const, 18446744073709551615ul>) const /usr/bin/../lib/gcc/x86_64-linux-gnu/12/../../../../include/c++/12/bits/std_function.h:591:9
13    [#11](/bitcoin-bitcoin/11/) 0x5650eb6df861 in test_one_input(std::span<unsigned char const, 18446744073709551615ul>) /root/bitcoin/fuzzbuild/src/test/fuzz/util/../../../../../src/test/fuzz/fuzz.cpp:88:5
14    [#12](/bitcoin-bitcoin/12/) 0x5650eb6df861 in LLVMFuzzerTestOneInput /root/bitcoin/fuzzbuild/src/test/fuzz/util/../../../../../src/test/fuzz/fuzz.cpp:216:5
15    [#13](/bitcoin-bitcoin/13/) 0x5650eb308a00 in fuzzer::Fuzzer::ExecuteCallback(unsigned char const*, unsigned long) (/root/bitcoin/fuzzbuild/bin/fuzz+0x9baa00) (BuildId: 714ae02d0bf0aee30b5aead68737ef9e6f49c96a)
16    [#14](/bitcoin-bitcoin/14/) 0x5650eb308135 in fuzzer::Fuzzer::RunOne(unsigned char const*, unsigned long, bool, fuzzer::InputInfo*, bool, bool*) (/root/bitcoin/fuzzbuild/bin/fuzz+0x9ba135) (BuildId: 714ae02d0bf0aee30b5aead68737ef9e6f49c96a)
17    [#15](/bitcoin-bitcoin/15/) 0x5650eb3098c5 in fuzzer::Fuzzer::MutateAndTestOne() (/root/bitcoin/fuzzbuild/bin/fuzz+0x9bb8c5) (BuildId: 714ae02d0bf0aee30b5aead68737ef9e6f49c96a)
18    [#16](/bitcoin-bitcoin/16/) 0x5650eb30a4c5 in fuzzer::Fuzzer::Loop(std::vector<fuzzer::SizedFile, std::allocator<fuzzer::SizedFile>>&) (/root/bitcoin/fuzzbuild/bin/fuzz+0x9bc4c5) (BuildId: 714ae02d0bf0aee30b5aead68737ef9e6f49c96a)
19    [#17](/bitcoin-bitcoin/17/) 0x5650eb2f86ef in fuzzer::FuzzerDriver(int*, char***, int (*)(unsigned char const*, unsigned long)) (/root/bitcoin/fuzzbuild/bin/fuzz+0x9aa6ef) (BuildId: 714ae02d0bf0aee30b5aead68737ef9e6f49c96a)
20    [#18](/bitcoin-bitcoin/18/) 0x5650eb321aa2 in main (/root/bitcoin/fuzzbuild/bin/fuzz+0x9d3aa2) (BuildId: 714ae02d0bf0aee30b5aead68737ef9e6f49c96a)
21    [#19](/bitcoin-bitcoin/19/) 0x7f07fa808249  (/lib/x86_64-linux-gnu/libc.so.6+0x27249) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
22    [#20](/bitcoin-bitcoin/20/) 0x7f07fa808304 in __libc_start_main (/lib/x86_64-linux-gnu/libc.so.6+0x27304) (BuildId: 58254ca972028402bc40624f81388d85ec95f70d)
23    [#21](/bitcoin-bitcoin/21/) 0x5650eb2edae0 in _start (/root/bitcoin/fuzzbuild/bin/fuzz+0x99fae0) (BuildId: 714ae02d0bf0aee30b5aead68737ef9e6f49c96a)
24
25NOTE: libFuzzer has rudimentary signal handlers.
26      Combine libFuzzer with AddressSanitizer or similar for better crash reports.
27SUMMARY: libFuzzer: deadly signal
28MS: 1 InsertByte-; base unit: 2068efd5f9f44e2724a628cd896cc4911f0f66a7
290xff,0x0,0xfd,0xfb,0x0,0x0,0x0,0xd7,0x52,0x87,0x87,0x87,0x87,0x87,0x87,0x87,0x87,0x87,0x40,0x87,0x87,0x87,0x87,0x87,0x87,0x87,0x87,0x87,0x87,0x0,0x0,0x0,0x0,0x0,0x6f,0x0,0x0,0x13,0x87,0x87,0x87,0x87,0x28,
30\377\000\375\373\000\000\000\327R\207\207\207\207\207\207\207\207\207@\207\207\207\207\207\207\207\207\207\207\000\000\000\000\000o\000\000\023\207\207\207\207(
31artifact_prefix='./'; Test unit written to ./crash-e661f705f50b682bb8c4b81ce95935565c579253
32Base64: /wD9+wAAANdSh4eHh4eHh4eHQIeHh4eHh4eHh4cAAAAAAG8AABOHh4eHKA==

    Something being miscounted for added_size I would assume?

    brunoerg commented at 5:16 pm on October 27, 2025:

    Interesting how I didn’t get it but I think this is related to #32624 (review). The check was wrong so it was playing with not-fully-valid keys.

    edit: there is one more reason I’m checking out.

  20. in src/wallet/test/fuzz/scriptpubkeyman.cpp:264 in 6d29c9a6d1 
    259+        legacy_data.AddInactiveHDChain(hd_chain);
260+    }
261+
262+    bool watch_only = false;
263+    const auto pub_key = ConsumeDeserializable<CPubKey>(fuzzed_data_provider);
264+    if (!pub_key && !pub_key->IsFullyValid()) return;
    marcofleon commented at 4:26 pm on October 27, 2025:
    Shouldn’t this be ||? Wouldn’t want to dereference if pub_key is null.
    brunoerg commented at 5:05 pm on October 27, 2025:
    Yes, nice find!
  21. DrahtBot requested review from w0xlt on Oct 27, 2025
  22. maflcko closed this on Oct 27, 2025
  23. maflcko reopened this on Oct 27, 2025
  24. brunoerg force-pushed on Oct 27, 2025
  25. brunoerg commented at 5:16 pm on October 27, 2025: contributor
    Force-pushed addressing #32624 (review). Thanks, @marcofleon.
  26. brunoerg force-pushed on Oct 27, 2025
  27. brunoerg commented at 6:06 pm on October 27, 2025: contributor
    There was one more case that I forgot to count in. We should consider the split chain support when verifying the result (desc_spkms). Force-pushed fixing it (6ced804 to 6436ad7).
  28. fanquake added the label Fuzzing on Oct 30, 2025
  29. in src/wallet/test/fuzz/scriptpubkeyman.cpp:331 in 6436ad7f2f 
    326+            }
327+        );
328+    }
329+
330+    auto result{legacy_data.MigrateToDescriptor()};
331+    auto hd_chain_version{legacy_data.GetHDChain().nVersion};
    marcofleon commented at 4:38 am on November 13, 2025:

    Adding this and modifying added_chains based on it does fix the crash I was seeing before. But shouldn’t we save this variable before doing the migration? To make the assertion below actually verify the correctness.

    I haven’t looked at the migration code in depth, so this might be fine. Just figured if we’re checking added_size against the result then it’s better if it’s based on the pre-migration state.

    brunoerg commented at 2:31 pm on November 13, 2025:
    Nice catch, we can simply check hd_chain.nVersion instead of having auto hd_chain_version{legacy_data.GetHDChain().nVersion};. We could also add an assert to ensure these two values are the same.
  30. in src/wallet/test/fuzz/scriptpubkeyman.cpp:335 in 6436ad7f2f outdated 
    325+                }
326+            }
327+        );
328+    }
329+
330+    auto result{legacy_data.MigrateToDescriptor()};
    marcofleon commented at 4:43 am on November 13, 2025:
    No null check after this? Doesn’t seem to ever return null based on the coverage report, but could still be worth having to avoid potential undefined behavior.
    brunoerg commented at 2:34 pm on November 13, 2025:
    No null check because we always expect it to succeed given how the harness is built, but will add a check just to avoid UB.
  31. in src/wallet/test/fuzz/scriptpubkeyman.cpp:264 in 6436ad7f2f 
    259+    }
260+
261+    bool watch_only = false;
262+    const auto pub_key = ConsumeDeserializable<CPubKey>(fuzzed_data_provider);
263+    if (!pub_key || !pub_key->IsFullyValid()) return;
264+    if (legacy_data.LoadWatchOnly(GetScriptForDestination(WitnessV0KeyHash{*pub_key}))) watch_only = true;
    marcofleon commented at 4:50 am on November 13, 2025:
    Is it worth testing other script types here as well? Not super familiar with the wallet code, so disregard if irrelevant.
    brunoerg commented at 7:28 pm on November 13, 2025:
    I can also add PKHash, but for watch only loading I don’t think worth adding more types. Wouldn’t reflect its usage in practice.
  32. marcofleon commented at 4:52 am on November 13, 2025: contributor

    Left some questions below.

    Also, coverage report after about a day of fuzzing.

  33. brunoerg force-pushed on Nov 13, 2025
  34. brunoerg commented at 7:46 pm on November 13, 2025: contributor

    Force-pushed 6436ad7 to 6227441:

  35. DrahtBot added the label CI failed on Nov 13, 2025
  36. fuzz: wallet: add target for `MigrateToDescriptor` 51917d94e6
  37. brunoerg force-pushed on Nov 13, 2025
  38. DrahtBot removed the label CI failed on Nov 13, 2025
  39. marcofleon commented at 2:17 pm on November 17, 2025: contributor
    Nice, lgtm ACK 51917d94e657024cc0af8aa748a9d5d39a617e85
  40. DrahtBot requested review from maflcko on Nov 17, 2025


brunoerg DrahtBot maflcko w0xlt marcofleon


achow101 maflcko w0xlt

Labels
Tests Fuzzing

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2025-11-20 15:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me