wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs #28868

achow101 · 2023-11-13T21:32:17Z

A transaction does not necessarily have to belong to either the migrated wallet (with the private keys) and the watchonly wallet (with watchonly things), it could have multiple outputs with each isminetype. So we should be putting such transactions in one or the other wallet, but rather putting it in both.

I've added a test for this behavior, however the test also revealed a few other issues. Notably, it revealed that migratewallet would have the watchonly wallet rescan from genesis when it is reloaded at the end of migration. This could be a cause for migration appearing to be very slow. This is resolved by first writing best block records to the watchonly and solvable wallets, as well as updating the test to make sure that rescans don't happen.

The change to avoid rescans also found an issue where some of our early exits would result in unloading the wallet even though nothing happened. So there is also a commit to reload the wallet for such early exits.

DrahtBot · 2023-11-13T21:32:20Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, furszy

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#28710 (Remove the legacy wallet and BDB dependency by achow101)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet's own transaction outputs in memory by achow101)
#26596 (wallet: Migrate legacy wallets to descriptor wallets without requiring BDB 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.

achow101 · 2023-11-14T00:29:35Z

It seems like there's a random failure happening where something in BDB is free()ing an invalid pointer which causes bitcoind to abort. This appears to only be happening with the change the reloads the wallet for the early exits. I don't really understand why that is even happening, and it doesn't happen consistently enough to reliably debug.

Sjors · 2023-11-14T06:29:07Z

I also get a test failure at 65b2da9 about half the time. With the first commit a47f220 this test doesn't fail (at least not the first ten times I tried).

228/283 - wallet_migration.py failed, Duration: 2 s

stdout:
2023-11-14T06:24:21.154000Z TestFramework (INFO): PRNG seed is: 2952483070848634871
...
2023-11-14T06:24:22.297000Z TestFramework (INFO): Test migration of an encrypted wallet
2023-11-14T06:24:22.892000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
...
  File "/home/sjors/dev/bitcoin/test/functional/wallet_migration.py", line 452, in test_encrypted
    assert_raises_rpc_error(-4, "The passphrase contains a null character", wallet.migratewallet, None, "pass\0with\0null")
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/util.py", line 130, in assert_raises_rpc_error
    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
  File "/home/sjors/dev/bitcoin/test/functional/test_framework/util.py", line 150, in try_rpc
    raise AssertionError("Unexpected exception raised: " + type(e).__name__)
AssertionError: Unexpected exception raised: RemoteDisconnected
2023-11-14T06:24:22.944000Z TestFramework (INFO): Stopping nodes
[node 0] Cleaning up leftover process

Sjors · 2023-11-14T06:57:07Z

While trying to test the first commit, I ran into something odd. It seems that an empty watch-only wallet legacy is treated as a descriptor wallet by migratewallet.

Checkout master at 5800c55, compile with BDB
src/bitcoind -deprecatedrpc=create_bdb
src/bitcoin-cli createwallet TestMigrate true true "" false false (this warns that legacy wallets are deprecated)
Restart the node (not sure if necessary) and load the wallet again, double check with getwalletinfo that it's a legacy wallet
src/bitcoin-cli -rpcwallet=TestMigrate migratewallet

That results in:

error code: -4
error message:
Error: This wallet is already a descriptor wallet

Once I import an address it does migrate.

Sjors · 2023-11-14T07:24:06Z

Other than that a47f220 works. Without it, when migrating a watch-only wallet it rescans from the wallet birth height. With it, it does not rescan. The test in f06b5c0 also catches it if you revert the commit.

Regarding 65b2da9 this gets a bit unwieldy because there are many places the migration can fail. Maybe just let the caller MigrateLegacyToDescriptor attempt a reload if migration failed.

The test in 412c34b also catches the issue if you revert 4dab813, so that's good.

fanquake · 2023-11-14T15:51:27Z

Is (some part of) this meant for backport to 26.x?

achow101 · 2023-11-14T16:54:32Z

Is (some part of) this meant for backport to 26.x?

With the current test failure that is hard to debug, no.

furszy

Nice findings 👌🏼.

I haven't gone deeper over the BDB freeing issue within this context yet. But, can report that in the branch where I changed the underlying db migration mechanism (link), I haven't encountered the issue. I just pulled and ran all this PR new tests there and they all pass successfully in a loop.

One of the ideas of this work is to fix failures like 65b2da9 and others in a safer manner. But can find the detailed reasoning behind it here #28609 (comment).

Note: the all-in-one commit in my branch is currently quite messy. The priority was to validate the feasibility of the approach first. Ensuring all tests pass before making any code cleanup and atomic commits reorg.

Will try to submit a PR with the changes next week.

achow101 · 2023-11-30T21:03:19Z

While trying to test the first commit, I ran into something odd. It seems that an empty watch-only wallet legacy is treated as a descriptor wallet by migratewallet.

This is a separate issue in master. I've opened #28976 to address it.

achow101 · 2023-11-30T22:13:10Z

Figured out the random failure, should be all working now.

furszy

ha, made the same change two hours ago. furszy@193e0c9. Will review.

furszy

Code review ACK 0cf1ae5

src/wallet/wallet.cpp

furszy

reACK b093e5e

Only cleaned the IsMine && IsFromMe code dup from my last review.

c11c404 tests: Test migration of blank wallets (Andrew Chow) 563b2a6 wallet: Better error message when missing LegacySPKM during migration (Andrew Chow) b1d2c77 wallet: Check for descriptors flag before migration (Andrew Chow) 8c127ff wallet: Skip key and script migration for blank wallets (Andrew Chow) Pull request description: Blank wallets (wallets without any keys are scripts) are being detected as already being descriptor wallets even though they are not. This is because the check for whether a wallet is already a descriptor wallet uses the presence of a `LegacyScriptPubKeyMan` which is only setup when keys or scripts are found. This PR resolves this issue by checking for the descriptor wallet flag instead and subsequently skipping the keys and scripts part of migration for blank wallets. Fixes the issue mentioned in #28868 (comment) ACKs for top commit: furszy: reACK c11c404. CI failure is unrelated. ryanofsky: Code review ACK c11c404 Tree-SHA512: 2466fdf1542eb8489c841253191f85dc88365493f0bb3395b67dee3e43709a9993c68b9d7623657b54b779adbe68fc81962d60efef4802c5d461f154167af7f4

ryanofsky

Code review ACK b093e5e, but only lightly reviewed the tests.

This PR has a merge conflict currently so needs to be updated. Also I think the place where reload_wallet lamba is moved is a little unsafe so would suggest changing that (see below).

src/wallet/wallet.cpp

When migrating, we should also be writing the bestblock record to the watchonly and solvable wallets to avoid rescanning on the reload as that can be slow.

Migration will unload loaded wallets prior to beginning. It will then perform some checks which may exit early. Such unloaded wallets should be reloaded prior to exiting.

We want to make sure that all of the transactions are being copied to the watchonly and solvable wallets as expected. The automatic rescanning behavior can cause us to pass a test by finding the transaction on loading rather than having it be copied as expected.

It is possible for a transaction that has an output that belongs to the mgirated wallet, and another output that belongs to the watchonly wallet. Such transaction should appear in both wallets during migration.

ryanofsky

Code review ACK 4da76ca. This looks great. The code is actually cleaner than before, two bugs are fixed, and the test checking for rescanning is pretty clever and broadens test coverage.

I left a review comment about improving derivation of wallet paths in the MigrateLegacyToDescriptor function, but that isn't really about this PR and would be better addressed in a separate followup.

ryanofsky · 2024-02-02T21:30:17Z

src/wallet/wallet.cpp

        // Reload the main wallet
+        wallet_dirs.insert(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path());


In commit "wallet: Reload the wallet if migration exited early" (78ba0e6)

Not related to this PR, but I think the fs::PathFromString(wallet->GetDatabase().Filename()).parent_path() expression repeated throughout this function is unnecessarily verbose and also potentially dangerous if it is used in another context, or if the surrounding context changes.

It happens to be safe currently, because we know this code is only called after MigrateToSQLite is called, so the database FileName() method will always return the path to a file in a wallet subdirectory. But in other contexts wallet->GetDatabase().Filename() could return the path to a file in a top level wallets directory, or even a top level data directory, so calling fs::remove_all(fs::PathFromString(wallet->GetDatabase().Filename()).parent_path()) could wipe out a lot of other data not part of the wallet we are trying to delete.

To fix this as a start, I think we should be deleting the original wallet path
AbsPathJoin(GetWalletDir(), fs::PathFromString(wallet->GetName())), not the parent of the path of the database inside the wallet. It would also be good to move away from using fs::remove_all which could delete a lot of files unintentially, and instead write a wallet delete function that only deletes files we know about.

ryanofsky · 2024-02-02T21:46:18Z

If @furszy wants to re-ack, or if someone else adds a review, I think this could be merged

furszy

Code review ACK 4da76ca

Also verified that the tests fail when bug fix commits are reverted.

furszy · 2024-02-02T23:51:10Z

src/wallet/wallet.cpp

    if (local_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
+        if (was_loaded) {
+            reload_wallet(local_wallet);
+        }
        return util::Error{_("Error: This wallet is already a descriptor wallet")};


in 78ba0e6:

I know that this reload_wallet() call shouldn't fail, but it would be good to log the error if it happens (the error string will be set) and inform the user in the error message that they need to reload the wallet manually.

(same for the others)

When migrating, we should also be writing the bestblock record to the watchonly and solvable wallets to avoid rescanning on the reload as that can be slow. Github-Pull: bitcoin#28868 Rebased-From: 9332c7e

Migration will unload loaded wallets prior to beginning. It will then perform some checks which may exit early. Such unloaded wallets should be reloaded prior to exiting. Github-Pull: bitcoin#28868 Rebased-From: 78ba0e6

We want to make sure that all of the transactions are being copied to the watchonly and solvable wallets as expected. The automatic rescanning behavior can cause us to pass a test by finding the transaction on loading rather than having it be copied as expected. Github-Pull: bitcoin#28868 Rebased-From: 71cb28e

It is possible for a transaction that has an output that belongs to the mgirated wallet, and another output that belongs to the watchonly wallet. Such transaction should appear in both wallets during migration. Github-Pull: bitcoin#28868 Rebased-From: c62a8d0

Github-Pull: bitcoin#28868 Rebased-From: 4da76ca

DrahtBot added the Wallet label Nov 13, 2023

DrahtBot mentioned this pull request Nov 13, 2023

Remove the legacy wallet and BDB dependency #28710

Merged

DrahtBot added the CI failed label Nov 13, 2023

This was referenced Nov 14, 2023

wallet: Keep track of the wallet's own transaction outputs in memory #27286

Merged

wallet: Migrate legacy wallets to descriptor wallets without requiring BDB #26596

Merged

DrahtBot mentioned this pull request Nov 14, 2023

wallet: Track no-longer-spendable TXOs separately #27865

Open

furszy reviewed Nov 15, 2023

View reviewed changes

achow101 mentioned this pull request Nov 30, 2023

wallet: Fix migration of blank wallets #28976

Merged

achow101 force-pushed the test-migration-watchonly-spendable branch from 412c34b to 0cf1ae5 Compare November 30, 2023 22:13

furszy reviewed Nov 30, 2023

View reviewed changes

DrahtBot removed the CI failed label Dec 1, 2023

furszy reviewed Dec 2, 2023

View reviewed changes

src/wallet/wallet.cpp Outdated Show resolved Hide resolved

DrahtBot mentioned this pull request Dec 7, 2023

rpc: add path to gethdkey #22341

Closed

achow101 force-pushed the test-migration-watchonly-spendable branch from 0cf1ae5 to b093e5e Compare January 2, 2024 21:41

furszy approved these changes Jan 3, 2024

View reviewed changes

achow101 requested a review from Sjors January 11, 2024 20:43

DrahtBot added the CI failed label Jan 16, 2024

furszy mentioned this pull request Jan 31, 2024

rpc: Drop migratewallet experimental warning #28037

Merged

DrahtBot added the Needs rebase label Jan 31, 2024

ryanofsky reviewed Jan 31, 2024

View reviewed changes

src/wallet/wallet.cpp Show resolved Hide resolved

src/wallet/wallet.cpp Show resolved Hide resolved

ryanofsky approved these changes Jan 31, 2024

View reviewed changes

achow101 added 5 commits February 1, 2024 13:43

wallet: Write bestblock to watchonly and solvable wallets

9332c7e

When migrating, we should also be writing the bestblock record to the watchonly and solvable wallets to avoid rescanning on the reload as that can be slow.

wallet: Reload the wallet if migration exited early

78ba0e6

Migration will unload loaded wallets prior to beginning. It will then perform some checks which may exit early. Such unloaded wallets should be reloaded prior to exiting.

wallet: Keep txs that belong to both watchonly and migrated wallets

c62a8d0

It is possible for a transaction that has an output that belongs to the mgirated wallet, and another output that belongs to the watchonly wallet. Such transaction should appear in both wallets during migration.

test: Test migration of tx with both spendable and watchonly

4da76ca

achow101 force-pushed the test-migration-watchonly-spendable branch from b093e5e to 4da76ca Compare February 1, 2024 19:15

DrahtBot removed Needs rebase CI failed labels Feb 1, 2024

ryanofsky mentioned this pull request Feb 2, 2024

wallet: Set descriptors flag after migrating blank wallets #29367

Merged

ryanofsky approved these changes Feb 2, 2024

View reviewed changes

DrahtBot requested a review from furszy February 2, 2024 21:43

furszy reviewed Feb 2, 2024

View reviewed changes

ryanofsky self-assigned this Feb 3, 2024

ryanofsky merged commit a115856 into bitcoin:master Feb 3, 2024

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Apr 24, 2024

test: Test migration of tx with both spendable and watchonly

0f12bff

Github-Pull: bitcoin#28868 Rebased-From: 4da76ca

bitcoin locked and limited conversation to collaborators Feb 2, 2025

		// Reload the main wallet
		wallet_dirs.insert(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path());

wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs #28868

wallet: Fix migration of wallets with txs that have both spendable and watchonly outputs #28868

Uh oh!

Conversation

achow101 commented Nov 13, 2023

Uh oh!

DrahtBot commented Nov 13, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage

Reviews

Conflicts

Uh oh!

achow101 commented Nov 14, 2023

Uh oh!

Sjors commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Sjors commented Nov 14, 2023

Uh oh!

Sjors commented Nov 14, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Nov 14, 2023

Uh oh!

achow101 commented Nov 14, 2023

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

achow101 commented Nov 30, 2023

Uh oh!

achow101 commented Nov 30, 2023

Uh oh!

furszy left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ryanofsky left a comment

Choose a reason for hiding this comment

Uh oh!

ryanofsky Feb 2, 2024

Choose a reason for hiding this comment

Uh oh!

ryanofsky commented Feb 2, 2024

Uh oh!

furszy left a comment

Choose a reason for hiding this comment

Uh oh!

furszy Feb 2, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

DrahtBot commented Nov 13, 2023 •

edited

Loading

Sjors commented Nov 14, 2023 •

edited

Loading

Sjors commented Nov 14, 2023 •

edited

Loading

furszy left a comment •

edited

Loading

furszy left a comment •

edited

Loading

furszy Feb 2, 2024 •

edited

Loading