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.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#31250 (wallet: Disable creating and loading legacy wallets by achow101)
#31241 (wallet: remove BDB dependency from wallet migration benchmark by furszy)
#28710 (Remove the legacy wallet and BDB dependency by achow101)
#25722 (refactor: Use util::Result class for wallet loading by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

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.

nevermind! will address it next push.

brunoerg commented at 8:47 pm on November 26, 2024:

Done.

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.

brunoerg commented at 8:47 pm on November 26, 2024:

Done.

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.

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):

brunoerg force-pushed on Nov 26, 2024

furszy commented at 7:40 pm on November 26, 2024: member

Would be good to ensure that #31374 can be replicated here.

brunoerg force-pushed on Nov 26, 2024

DrahtBot added the label CI failed on Nov 26, 2024

DrahtBot commented at 7:49 pm on November 26, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33561912794

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.

DrahtBot removed the label CI failed on Nov 26, 2024

brunoerg commented at 8:48 pm on November 26, 2024: contributor

Force-pushed addressing #29694 (review)

brunoerg commented at 8:48 pm on November 26, 2024: contributor

Would be good to ensure that #31374 can be replicated here.

Since this target is only for spkm migration, I don’t think it can be replicated here.

Edit: I can add ApplyMigrationData here and reproduce the crash from #31374. I’m going to move this to draft while working on it.

brunoerg marked this as a draft on Nov 27, 2024

brunoerg force-pushed on Dec 2, 2024

DrahtBot added the label CI failed on Dec 2, 2024

DrahtBot commented at 12:27 pm on December 2, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/33784139981

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.

brunoerg force-pushed on Dec 2, 2024

brunoerg marked this as ready for review on Dec 2, 2024

brunoerg commented at 3:42 pm on December 2, 2024: contributor

Force-pushed changing the fuzz target to address the ApplyMigrationData function. Now, after migrating the spk to descriptor, it creates some transactions and apply the migrated data.

Note that the CI failure is expected (see #31374):

0[09:11:48.112] Run spkm_migration with args ['/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/src/test/fuzz/fuzz', '-max_total_time=60']INFO: Running with entropic power schedule (0xFF, 100).
1[09:11:48.112] INFO: Seed: 2346481497
2[09:11:48.112] INFO: Loaded 1 modules   (629856 inline 8-bit counters): 629856 [0x55a29859f118, 0x55a298638d78), 
3[09:11:48.112] INFO: Loaded 1 PC tables (629856 PCs): 629856 [0x55a298638d78,0x55a298fd5378), 
4[09:11:48.112] INFO: -max_len is not provided; libFuzzer will not generate inputs larger than 4096 bytes
5[09:11:48.112] wallet/wallet.cpp:4088 ApplyMigrationData: Assertion `!m_cached_spks.empty()' failed.

in src/wallet/test/fuzz/scriptpubkeyman.cpp:331 in 514aff2f4b outdated

326+
327+    WalletBatch batch{wallet.GetDatabase()};
328+    LOCK(wallet.cs_wallet);
329+    CBlockLocator loc{chainstate.m_chain.GetLocator()};
330+    assert(batch.WriteBestBlock(loc));
331+    (void)wallet.ApplyMigrationData(batch, *res);

furszy commented at 6:55 pm on December 5, 2024:

Cannot call ApplyMigrationData without executing the entire DoMigration step. That’s just incorrect. The code skips the creation of the watch-only and solvable wallets from the MigrationData result, which causes a crash inside ApplyMigrationData because it is trying to access the watch-only wallet.

brunoerg commented at 9:23 pm on December 5, 2024:

Yes, just realized it. I’m going to change it, but afaik DoMigration can’t be called directly here.

brunoerg commented at 8:54 pm on December 9, 2024:

Done

brunoerg force-pushed on Dec 9, 2024

brunoerg commented at 8:56 pm on December 9, 2024: contributor

Force-pushed:

Now the fuzz target runs the entire migration process (MigrateLegacyToDescriptor). Also, I added a new commit that allows to migrate in-memory wallets to speed up the target and avoid timeouts.

brunoerg force-pushed on Dec 9, 2024

brunoerg marked this as a draft on Dec 9, 2024

brunoerg force-pushed on Dec 10, 2024

DrahtBot removed the label CI failed on Dec 10, 2024

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

brunoerg force-pushed on Dec 11, 2024

DrahtBot added the label CI failed on Dec 11, 2024

brunoerg force-pushed on Dec 11, 2024

wallet: add option to create in-memory SQLite DB in the migration c492bb1a29

wallet: sqlite: there is no need to have exclusive locking when mocking

For in-memory SQLite databases, exclusive locking is generally not
necessary because in-memory dbs are private to the connection that
created them.

38277a9f3d

wallet: skip backup restoration when using in-memory dbs

By using in-memory databases, we do not have dbs file to delete
and restore.

d13c542440

fuzz: wallet: add target for spkm migration 94c3bf1e02

brunoerg force-pushed on Dec 11, 2024

DrahtBot removed the label CI failed on Dec 11, 2024

fuzz: wallet: add target for spkm migration #29694

Code Coverage & Benchmarks

Reviews

Conflicts