fuzz: wallet: add target for spkm migration

brunoerg commented at 5:35 pm on March 21, 2024: contributor

This PR adds a fuzz target for ScriptPubKeyMan migration. It creates a LegacyDataSPKM which can have some keys/scripts/etc, and then migrate it to descriptor. I tried to keep it as compatible as possible with future legacy wallet removal.

DrahtBot commented at 5:35 pm on March 21, 2024: 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/29694.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

DrahtBot added the label Tests on Mar 21, 2024

brunoerg force-pushed on Apr 19, 2024

brunoerg commented at 5:57 pm on May 27, 2024: contributor

cc: @achow101

in src/wallet/test/fuzz/scriptpubkeyman.cpp:228 in d0c3ca9269 outdated

223+        keys.push_back(key);
224+    }
225+
226+    if (keys.empty()) return;
227+
228+    auto& legacy_spkm{*wallet.GetOrCreateLegacyScriptPubKeyMan()};

furszy commented at 10:35 pm on June 16, 2024:

The legacy spkm class will be deleted soon in #28710. You might need to add a mechanism to mock the bdb reader class just so you can feed the migration process with a hand-crafted LegacyDataSPKM. This will let you avoid crafting the bdb file manually when bdb is not compiled anymore.

brunoerg commented at 8:36 pm on June 25, 2024:

You might need to add a mechanism to mock the bdb reader class just so you can feed the migration process with a hand-crafted LegacyDataSPKM. This will let you avoid crafting the bdb file manually when bdb is not compiled anymore.

You’re right, I’ll move this PR to draft to work on it.

brunoerg commented at 11:33 am on September 10, 2024:

Just changed it to use LegacyDataSPKM. Note that this target is about the spkm migration only, not the whole process.

Mazzika1 approved

brunoerg marked this as a draft on Jun 25, 2024

brunoerg force-pushed on Sep 9, 2024

DrahtBot added the label CI failed on Sep 10, 2024

brunoerg marked this as ready for review on Sep 10, 2024

DrahtBot removed the label CI failed on Sep 11, 2024

achow101 requested review from achow101 on Oct 15, 2024

brunoerg commented at 9:32 am on October 23, 2024: contributor

Inputs for this are available at: https://github.com/brunoerg/qa-assets/pull/2

maflcko commented at 5:28 pm on October 23, 2024: member

Coverage for this target: https://drahtbot.space/host_reports/DrahtBot/reports/coverage_fuzz/monotree/2e7ac47bbc28c2cb/300ddaa72f7e4b9b/fuzz.coverage/index.html

in src/wallet/test/fuzz/scriptpubkeyman.cpp:248 in 2e7ac47bbc outdated

243+                if (legacy_data.HaveKey(keys[key_index].GetPubKey().GetID())) return;
244+                if (legacy_data.LoadKey(keys[key_index], keys[key_index].GetPubKey())) {
245+                    load_key_count++;
246+                    if (fuzzed_data_provider.ConsumeBool()) {
247+                        CHDChain hd_chain;
248+                        hd_chain.nVersion = CHDChain::VERSION_HD_CHAIN_SPLIT;

achow101 commented at 9:35 pm on November 4, 2024:

I think it would also be good to cover hd chain before hd chain split.

brunoerg commented at 2:43 pm on November 5, 2024:

Will do it.

in src/wallet/test/fuzz/scriptpubkeyman.cpp:289 in 2e7ac47bbc outdated

284+    }
285+
286+    std::optional<MigrationData> res{legacy_data.MigrateToDescriptor()};
287+    if (res.has_value()) {
288+        assert(static_cast<int>(res->desc_spkms.size()) >= load_key_count);
289+        if (!res->solvable_descs.empty()) assert(script_count > 0);

achow101 commented at 9:36 pm on November 4, 2024:

Would it be possible for script_count to really be watchonly script count and just have it increment only when a watchonly script is added?

brunoerg commented at 4:40 pm on November 5, 2024:

I think so, it just needs to check if the script was built with a loaded key.

brunoerg commented at 2:09 pm on November 6, 2024:

In fact, I tried to track watchonly and spendable scripts, but I don’t think it’s simple since we load keys, hd chain, diverse scripts, etc. I ended up writing similar or copying a lot of things from wallet/scriptpubkey.cpp which I don’t think would be a valuable approach here.

in src/wallet/test/fuzz/scriptpubkeyman.cpp:288 in 2e7ac47bbc outdated

283+        );
284+    }
285+
286+    std::optional<MigrationData> res{legacy_data.MigrateToDescriptor()};
287+    if (res.has_value()) {
288+        assert(static_cast<int>(res->desc_spkms.size()) >= load_key_count);

achow101 commented at 9:37 pm on November 4, 2024:

If watchonly script count can be tracked, then this check should be able to be an exact check as desc_spkms should be load_key_count plus the number of spendable scripts.

in src/wallet/test/fuzz/scriptpubkeyman.cpp:265 in 2e7ac47bbc outdated

263+            },
264+            [&] {
265+                CScript script{ConsumeScript(fuzzed_data_provider)};
266+                const auto key_index{fuzzed_data_provider.ConsumeIntegralInRange<int>(0, keys.size() - 1)};
267+                LIMITED_WHILE(fuzzed_data_provider.ConsumeBool(), 2) {
268+                    CallOneOf(

achow101 commented at 9:38 pm on November 4, 2024:

It would be nice to have multisig scripts here as the handling of ismine for that is quite complicated and having fuzz testing of it would be great.

brunoerg commented at 4:17 pm on November 5, 2024:

Sure, I will do it.

fuzz: wallet: add target for spkm migration 77c31c840d

brunoerg force-pushed on Nov 11, 2024

brunoerg commented at 4:57 pm on November 11, 2024: contributor

Force-pushed addressing (multisig and hd chain cover):

fuzz: wallet: add target for spkm migration #29694

Code Coverage & Benchmarks

Reviews