fuzz: wallet: add target for spkm migration by brunoerg · Pull Request #29694 · bitcoin/bitcoin

brunoerg · 2024-03-21T17:35:09Z

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 · 2024-03-21T17:35:11Z

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:

#28710 (Remove the legacy wallet and BDB dependency by achow101)

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.

brunoerg · 2024-05-27T17:57:39Z

cc: @achow101

src/wallet/test/fuzz/scriptpubkeyman.cpp

brunoerg · 2024-10-23T09:32:41Z

Inputs for this are available at: brunoerg/qa-assets#2

maflcko · 2024-10-23T17:28:43Z

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

src/wallet/test/fuzz/scriptpubkeyman.cpp

brunoerg · 2024-11-11T16:57:52Z

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

furszy

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

DrahtBot · 2024-11-26T19:49:01Z

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

Hints

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 · 2024-11-26T20:48:04Z

Force-pushed addressing #29694 (comment)

brunoerg · 2024-11-26T20:48:31Z

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 · 2025-01-27T19:02:05Z

Rebased

DrahtBot · 2025-02-03T16:12:27Z

https://cirrus-ci.com/task/5046981191532544?logs=ci#L1799

[22:03:02.859]  test  2025-01-27T22:03:01.984000Z TestFramework (ERROR): Assertion failed 
[22:03:02.859]                                    Traceback (most recent call last):
[22:03:02.859]                                      File "/ci_container_base/test/functional/test_framework/test_framework.py", line 135, in main
[22:03:02.859]                                        self.run_test()
[22:03:02.859]                                      File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_migration.py", line 1098, in run_test
[22:03:02.859]                                        self.test_failed_migration_cleanup()
[22:03:02.859]                                      File "/ci_container_base/ci/scratch/build-x86_64-pc-linux-gnu/test/functional/wallet_migration.py", line 899, in test_failed_migration_cleanup
[22:03:02.859]                                        assert all(wallet not in self.master_node.listwallets() for wallet in ["failed", "failed_watchonly", "failed_solvables"])
[22:03:02.859]                                    AssertionError

brunoerg · 2025-02-05T16:58:24Z

Just fixed CI, ready for review!

pablomartin4btc

You are skipping the backup restore in commit: "wallet: skip backup restoration when using in-memory dbs"; shouldn't the backup itself be skipped too when in_memory?

pablomartin4btc · 2025-04-25T08:22:44Z

src/wallet/wallet.cpp

I think if you add the condition there, there are other stuff that's missing and I think they need to be done which are:

the for after // Unload the wallets;

the lines after // Verify that there is no dangling wallet

Why? the for after // Unload the wallets are basically playing with created_wallets which is used only if the db is not in memory. Same for the lines after // Verify that there is no dangling wallet which needs ptr_wallet.

the for after // Unload the wallets...

There's the removal of the wallet from the context (if (!RemoveWallet(context, w, /*load_on_start=*/false)) {) using created_wallets - that was done when the wallet was loaded (in LoadWalletInternal-> AddWallet(context, wallet); ).

the lines after // Verify that there is no dangling wallet which needs ptr_wallet.

At the end of the condition is returning an error (return util::Error{error};)

Be aware that if you remove the commit wallet: skip backup restoration when using in-memory dbs altogether you need to add a conditional to avoid the restore section.

At some point I think we need to refactor the wallet and extract the wallet migration behaviour into a new/ few object/ s.

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

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

DrahtBot · 2025-05-07T14:33:31Z

🐙 This pull request conflicts with the target branch and needs rebase.

brunoerg · 2025-05-08T20:30:51Z

I'm reworking this target, will draft it.

brunoerg · 2025-05-27T11:34:25Z

Closing in favor of #32624.

779e782 fuzz: wallet: add target for `MigrateToDescriptor` (brunoerg) Pull request description: 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. ACKs for top commit: frankomosh: Code Review ACK 779e782 marcofleon: reACK 779e782 Tree-SHA512: 08ef5166602c21658765bc063c5421e81055d094d346c4e2a28215209c6b7768b99a424f3ba47cf718dc8d827a588da22394ba23402a40a71a976d80d65e6c2e

DrahtBot added the Tests label Mar 21, 2024

brunoerg mentioned this pull request Mar 25, 2024

Proposed Timeline for Legacy Wallet and BDB removal #20160

Closed

48 tasks

brunoerg mentioned this pull request Apr 17, 2024

Wallet fuzzing tracking issue #29901

Open

19 tasks

brunoerg force-pushed the 2024-03-fuzz-spkm-migration branch from 114066e to d0c3ca9 Compare April 19, 2024 08:49

furszy reviewed Jun 16, 2024

View reviewed changes

src/wallet/test/fuzz/scriptpubkeyman.cpp Outdated Show resolved Hide resolved

Mazzika1 approved these changes Jun 16, 2024

View reviewed changes

brunoerg marked this pull request as draft June 25, 2024 20:37

brunoerg force-pushed the 2024-03-fuzz-spkm-migration branch from d0c3ca9 to 2e7ac47 Compare September 9, 2024 19:02

DrahtBot added the CI failed label Sep 10, 2024

brunoerg marked this pull request as ready for review September 10, 2024 11:28

DrahtBot removed the CI failed label Sep 11, 2024

achow101 self-requested a review October 15, 2024 15:53

achow101 reviewed Nov 4, 2024

View reviewed changes

brunoerg force-pushed the 2024-03-fuzz-spkm-migration branch from 2e7ac47 to 77c31c8 Compare November 11, 2024 16:56

brunoerg force-pushed the 2024-03-fuzz-spkm-migration branch from 77c31c8 to ed1a16b Compare November 26, 2024 19:33

furszy reviewed Nov 26, 2024

View reviewed changes

brunoerg force-pushed the 2024-03-fuzz-spkm-migration branch from ed1a16b to cdc9458 Compare November 26, 2024 19:48

DrahtBot added the CI failed label Nov 26, 2024

DrahtBot removed the CI failed label Nov 26, 2024

brunoerg marked this pull request as draft November 27, 2024 13:13

brunoerg force-pushed the 2024-03-fuzz-spkm-migration branch 2 times, most recently from 1421097 to 9750b3f Compare December 2, 2024 12:27

DrahtBot added the Needs rebase label Jan 25, 2025

brunoerg force-pushed the 2024-03-fuzz-spkm-migration branch from 5e2e916 to 457f1d4 Compare January 27, 2025 18:53

DrahtBot added CI failed and removed Needs rebase labels Jan 27, 2025

brunoerg force-pushed the 2024-03-fuzz-spkm-migration branch 2 times, most recently from a7749a9 to cbffd66 Compare January 27, 2025 21:47

brunoerg marked this pull request as draft February 3, 2025 16:41

brunoerg force-pushed the 2024-03-fuzz-spkm-migration branch from cbffd66 to 0c0e79d Compare February 5, 2025 13:09

DrahtBot removed the CI failed label Feb 5, 2025

brunoerg marked this pull request as ready for review February 5, 2025 16:58

DrahtBot mentioned this pull request Apr 19, 2025

bench: close wallets after migration #32309

Merged

DrahtBot added the Needs rebase label Apr 22, 2025

pablomartin4btc reviewed Apr 25, 2025

View reviewed changes

brunoerg added 4 commits April 28, 2025 11:29

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

ba8d151

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

72a424e

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

wallet: skip backup restoration when using in-memory dbs

6f0e025

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

fuzz: wallet: add target for spkm migration

c2bbe12

brunoerg force-pushed the 2024-03-fuzz-spkm-migration branch from 0c0e79d to c2bbe12 Compare April 28, 2025 16:49

DrahtBot removed the Needs rebase label Apr 28, 2025

DrahtBot added the Needs rebase label May 7, 2025

brunoerg marked this pull request as draft May 8, 2025 20:30

brunoerg mentioned this pull request May 27, 2025

fuzz: wallet: add target for MigrateToDescriptor #32624

Merged

brunoerg closed this May 27, 2025

Conversation

brunoerg commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Mar 21, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

brunoerg commented May 27, 2024

Uh oh!

Uh oh!

brunoerg commented Oct 23, 2024

Uh oh!

maflcko commented Oct 23, 2024

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

brunoerg commented Nov 11, 2024

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Nov 26, 2024

Uh oh!

brunoerg commented Nov 26, 2024

Uh oh!

brunoerg commented Nov 26, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

brunoerg commented Jan 27, 2025

Uh oh!

DrahtBot commented Feb 3, 2025

Uh oh!

brunoerg commented Feb 5, 2025

Uh oh!

pablomartin4btc left a comment

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc Apr 25, 2025

Choose a reason for hiding this comment

Uh oh!

brunoerg May 2, 2025

Choose a reason for hiding this comment

Uh oh!

pablomartin4btc May 5, 2025

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented May 7, 2025

Uh oh!

brunoerg commented May 8, 2025

Uh oh!

brunoerg commented May 27, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

brunoerg commented Mar 21, 2024 •

edited

Loading

DrahtBot commented Mar 21, 2024 •

edited

Loading

brunoerg commented Nov 26, 2024 •

edited

Loading