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

achow101 commented at 9:32 pm on November 13, 2023: member

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 commented at 9:32 pm on November 13, 2023: contributor

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.

DrahtBot added the label Wallet on Nov 13, 2023

DrahtBot added the label CI failed on Nov 13, 2023

achow101 commented at 0:29 am on November 14, 2023: member

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 commented at 6:29 am on November 14, 2023: member

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

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

Sjors commented at 6:57 am on November 14, 2023: member

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 5800c558eb5efb4839ed00d6967e43306d68e1c3, 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:

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

Once I import an address it does migrate.

Sjors commented at 7:24 am on November 14, 2023: member

Other than that a47f220933f202dd1f972d30f6cb31b09ca0a225 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 f06b5c02e9a1f1e5a26662fc8bbea5ed11388a03 also catches it if you revert the commit.

Regarding 65b2da99fe18768d1bd4cd67522f7031f16a3420 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 412c34b161ca901be4bf4de18f5a6afd9b987c57 also catches the issue if you revert 4dab813a9e69e4bb15cc1c4195358d127691cc89, so that’s good.

fanquake commented at 3:51 pm on November 14, 2023: member

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

achow101 commented at 4:54 pm on November 14, 2023: member

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

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

furszy commented at 3:42 pm on November 15, 2023: member

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 (review).

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 commented at 9:03 pm on November 30, 2023: member

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 force-pushed on Nov 30, 2023

achow101 commented at 10:13 pm on November 30, 2023: member

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

furszy commented at 11:54 pm on November 30, 2023: member

ha, made the same change two hours ago. https://github.com/furszy/bitcoin-core/commit/193e0c9c7ffe34ab2e8a4f5b95f66ecc4bac2791. Will review.

DrahtBot removed the label CI failed on Dec 1, 2023

in src/wallet/wallet.cpp:3950 in 3807f195b0 outdated

3966-                    continue;
3967                 }
3968+                continue;
3969             }
3970+        }
3971+        if (!IsMine(*wtx->tx) && !IsFromMe(*wtx->tx)) {

furszy commented at 12:58 pm on December 2, 2023:

tiny nit: Could save the “is mine” result at the top and use it in both if/else paths.

 0diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 1--- a/src/wallet/wallet.cpp	(revision ee14243fa526cfaa0ba538286588bb27e13d8c3c)
 2+++ b/src/wallet/wallet.cpp	(revision e13fcf39a74a45696d1536017b13e5dd393a8ae3)
 3@@ -3922,31 +3922,32 @@
 4         }
 5     }
 6     for (const auto& [_pos, wtx] : wtxOrdered) {
 7-        if (!IsMine(*wtx->tx) && !IsFromMe(*wtx->tx)) {
 8-            // Check it is the watchonly wallet's
 9-            // solvable_wallet doesn't need to be checked because transactions for those scripts weren't being watched for
10-            if (data.watchonly_wallet) {
11-                LOCK(data.watchonly_wallet->cs_wallet);
12-                if (data.watchonly_wallet->IsMine(*wtx->tx) || data.watchonly_wallet->IsFromMe(*wtx->tx)) {
13-                    // Add to watchonly wallet
14-                    const uint256& hash = wtx->GetHash();
15-                    const CWalletTx& to_copy_wtx = *wtx;
16-                    if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) {
17-                        if (!new_tx) return false;
18-                        ins_wtx.SetTx(to_copy_wtx.tx);
19-                        ins_wtx.CopyFrom(to_copy_wtx);
20-                        return true;
21-                    })) {
22-                        error = strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex());
23-                        return false;
24-                    }
25-                    watchonly_batch->WriteTx(data.watchonly_wallet->mapWallet.at(hash));
26-                    // Mark as to remove from this wallet
27-                    txids_to_delete.push_back(hash);
28-                    continue;
29-                }
30-            }
31-            // Both not ours and not in the watchonly wallet
32+        bool is_mine = IsMine(*wtx->tx) || IsFromMe(*wtx->tx);
33+        // Check it is the watchonly wallet's
34+        // solvable_wallet doesn't need to be checked because transactions for those scripts weren't being watched for
35+        if (data.watchonly_wallet) {
36+            LOCK(data.watchonly_wallet->cs_wallet);
37+            if (data.watchonly_wallet->IsMine(*wtx->tx) || data.watchonly_wallet->IsFromMe(*wtx->tx)) {
38+                // Add to watchonly wallet
39+                const uint256& hash = wtx->GetHash();
40+                const CWalletTx& to_copy_wtx = *wtx;
41+                if (!data.watchonly_wallet->LoadToWallet(hash, [&](CWalletTx& ins_wtx, bool new_tx) EXCLUSIVE_LOCKS_REQUIRED(data.watchonly_wallet->cs_wallet) {
42+                    if (!new_tx) return false;
43+                    ins_wtx.SetTx(to_copy_wtx.tx);
44+                    ins_wtx.CopyFrom(to_copy_wtx);
45+                    return true;
46+                })) {
47+                    error = strprintf(_("Error: Could not add watchonly tx %s to watchonly wallet"), wtx->GetHash().GetHex());
48+                    return false;
49+                }
50+                watchonly_batch->WriteTx(data.watchonly_wallet->mapWallet.at(hash));
51+                // Mark as to remove from this wallet
52+                if (!is_mine) txids_to_delete.push_back(hash);
53+                continue;
54+            }
55+        }
56+        // Both not ours and not in the watchonly wallet
57+        if (!is_mine) {
58             error = strprintf(_("Error: Transaction %s in wallet cannot be identified to belong to migrated wallets"), wtx->GetHash().GetHex());
59             return false;
60         }

achow101 commented at 9:41 pm on January 2, 2024:

Done as suggested

furszy commented at 12:59 pm on December 2, 2023: member

Code review ACK 0cf1ae5a

achow101 force-pushed on Jan 2, 2024

furszy approved

furszy commented at 2:45 pm on January 3, 2024: member

reACK b093e5e

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

achow101 requested review from Sjors on Jan 11, 2024

DrahtBot added the label CI failed on Jan 16, 2024

ryanofsky referenced this in commit 5a1473e2c0 on Jan 31, 2024

in src/wallet/wallet.cpp:4246 in d8002ed333 outdated

4211@@ -4210,6 +4212,21 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4212         return util::Error{Untranslated("Wallet file verification failed.") + Untranslated(" ") + error};
4213     }
4214 
4215+    // In case of reloading failure after migration completes, we need to remember the wallet dirs to remove
4216+    // Set is used as it may be populated with the same wallet directory paths multiple times,
4217+    // both before and after reloading. This ensures the set is complete even if one of the wallets
4218+    // fails to reload.
4219+    std::set<fs::path> wallet_dirs;
4220+    // Helper to reload as normal for some of our exit scenarios

ryanofsky commented at 10:14 pm on January 31, 2024:

In commit “wallet: Reload the wallet if migration exited early” (d8002ed333e13e54726bcd922303cb1e42544b44)

I don’t think it is good to move reload_wallet here before BackupWallet is called. It seems like it would make it easy to add an innocent call to reload_wallet(local_wallet) that looks like it is just reloading the wallet, but doesn’t return right away, so the wallet get added to wallet_dirs before it is backed up and possibly deleted as part of the fs::remove_all loop.

Most of the code in reload_wallet is also unnecessary for handling early returns, in addition to being potentially unsafe. I think the simplest way to handle early returns would just be to add a new simpler lambda, or maybe even inline the LoadWallet call like

0if (was_loaded) {
1    local_wallet = LoadWallet(context, local_wallet->GetName(), /*load_on_start=*/std::nullopt, options, status, error, warnings);
2}

achow101 commented at 7:16 pm on February 1, 2024:

I’ve removed the wallet_dirs.insert from reload_wallet and moved that to above the three reload_wallet calls that actually need it. The lambda is still useful since we have to unload local_wallet every time.

DrahtBot added the label Needs rebase on Jan 31, 2024

in src/wallet/wallet.cpp:4219 in d8002ed333 outdated

4190@@ -4191,11 +4191,13 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
4191     std::vector<bilingual_str> warnings;
4192 
4193     // If the wallet is still loaded, unload it so that nothing else tries to use it while we're changing it
4194+    bool was_loaded = false;

ryanofsky commented at 10:33 pm on January 31, 2024:

In commit “wallet: Reload the wallet if migration exited early” (d8002ed333e13e54726bcd922303cb1e42544b44)

I was confused by “As these checks do not change anything in the wallet, such unloaded wallets should be reloaded prior to exiting” in the commit description, since the wallets would need to be reloaded whether or not checks changed anything.

I think the commit message can just say “Such unloaded wallets should be reloaded prior to exiting” without the “As these checks” part

achow101 commented at 7:16 pm on February 1, 2024:

Updated the commit message.

ryanofsky commented at 10:40 pm on January 31, 2024: contributor

Code review ACK b093e5ea29efa44626db8aeff05ad2ee3296e707, 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).

ryanofsky approved

wallet: Write bestblock to watchonly and solvable wallets

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.

9332c7edda

wallet: Reload the wallet if migration exited early

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.

78ba0e6748

test: Make sure that migration test does not rescan on reloading

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.

71cb28ea8c

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

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.

c62a8d03a8

test: Test migration of tx with both spendable and watchonly 4da76ca247

achow101 force-pushed on Feb 1, 2024

DrahtBot removed the label Needs rebase on Feb 1, 2024

DrahtBot removed the label CI failed on Feb 1, 2024

in src/wallet/wallet.cpp:4310 in 78ba0e6748 outdated

4313-            to_reload.reset();
4314-            to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings);
4315-            return to_reload != nullptr;
4316-        };
4317         // Reload the main wallet
4318+        wallet_dirs.insert(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path());

ryanofsky commented at 9:30 pm on February 2, 2024:

In commit “wallet: Reload the wallet if migration exited early” (78ba0e6748d2a519a96c41dea851e7c43b82f251)

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 approved

ryanofsky commented at 9:43 pm on February 2, 2024: contributor

Code review ACK 4da76ca24725eb9ba8122317e04a6e1ee14ac846. 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.

DrahtBot requested review from furszy on Feb 2, 2024

ryanofsky commented at 9:46 pm on February 2, 2024: contributor

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

in src/wallet/wallet.cpp:4257 in 78ba0e6748 outdated

4252     // Before anything else, check if there is something to migrate.
4253     if (local_wallet->IsWalletFlagSet(WALLET_FLAG_DESCRIPTORS)) {
4254+        if (was_loaded) {
4255+            reload_wallet(local_wallet);
4256+        }
4257         return util::Error{_("Error: This wallet is already a descriptor wallet")};

furszy commented at 11:51 pm on February 2, 2024:

in 78ba0e6748d:

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)

furszy commented at 11:51 pm on February 2, 2024: member

Code review ACK 4da76ca2

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

ryanofsky assigned ryanofsky on Feb 3, 2024

ryanofsky merged this on Feb 3, 2024

ryanofsky closed this on Feb 3, 2024

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

Code Coverage

Reviews

Conflicts