wallet: Reload watchonly and solvables wallets after migration #28609

achow101 · 2023-10-06T22:37:30Z

Some incomplete/incorrect state as a result of migration can be mitigated/cleaned up by simply restarting the migrated wallets. We already do this for a wallet when it is migrated, but we do not for the new watchonly and solvables wallets that may be created. This PR introduces this behavior, in addition to creating those wallets initially without an attached chain.

While implementing this, I noticed that not all CWalletTx metadata was being copied over to the watchonly wallet and so some data, such as time received, was being lost. This PR fixes this as a side effect of not having a chain attached to the watchonly wallet. A test has also been added.

DrahtBot · 2023-10-06T22:37:32Z

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	ishaanam, 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:

#28610 (wallet: Migrate entire address book entries to watchonly and solvables too by achow101)
#28546 (bugfix: watchonly wallets created after migration have incorrect height values by ryanofsky)
#28264 (test: refactor: support sending funds with outpoint result by theStack)
#27865 (wallet: Track no-longer-spendable TXOs separately 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.

src/wallet/wallet.cpp

src/wallet/transaction.cpp

src/wallet/wallet.cpp

furszy · 2023-10-11T19:39:45Z

src/wallet/transaction.cpp

+    nTimeReceived = _tx.nTimeReceived;
+    nTimeSmart = _tx.nTimeSmart;
+    fFromMe = _tx.fFromMe;
+    nOrderPos = _tx.nOrderPos;


In 4ccec8b:

Doesn't make much sense to copy nOrderPos. The field is strictly connected to the parent wallet. Depending on what the wallet is watching, the tx order position could be higher or lower.

I'd prefer to keep this function generic so it should copy everything even if we will overwrite stuff in actual usage.

furszy · 2023-10-11T19:54:03Z

src/wallet/wallet.cpp

        assert(local_wallet.use_count() == 1);
+        wallet_dirs.insert(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path());


In 6eb3df7:

Why this line?. Isn't the local wallet datadir added at line 4282?

In case the wallet fails to reload, we can still cleanup at the end.

furszy · 2023-10-11T20:00:16Z

src/wallet/wallet.cpp

+    if (success && res.watchonly_wallet) {
+        assert(res.watchonly_wallet.use_count() == 1);
+        std::string watchonly_wallet_name = res.watchonly_wallet->GetName();
+        wallet_dirs.insert(fs::PathFromString(res.watchonly_wallet->GetDatabase().Filename()).parent_path());


In 6eb3df7:

Why this line?. The watch-only wallet datadir is added at line 4295.
Same for the solvable one.

If the wallet fails to reload, we can still know to clean it up.

DrahtBot · 2023-10-17T07:48:08Z

test/functional/wallet_migration.py:863:15: E275 missing whitespace after keyword
test/functional/wallet_migration.py:864:15: E275 missing whitespace after keyword
test/functional/wallet_migration.py:865:15: E275 missing whitespace after keyword
test/functional/wallet_migration.py:867:15: E275 missing whitespace after keyword
test/functional/wallet_migration.py:869:15: E275 missing whitespace after keyword

furszy

Code review ACK 014231c

Feel free to ignore the nits.

src/wallet/wallet.cpp

test/functional/wallet_migration.py

furszy

diff ACK e2dfe76

ryanofsky

Code review ACK e2dfe76. This really seems like 3 unrelated changes smashed into one PR, but all the changes look good.

As far as I understand there is no test coverage to verify the fix in the third commit b57218e, but with the asserts in #28546 added, existing tests should fail. I'm happy to rebase #28546 on top of this after it is merged, though to avoid making this PR bigger.

src/wallet/transaction.cpp

src/wallet/wallet.cpp

ryanofsky · 2023-10-19T20:14:54Z

src/wallet/wallet.cpp

+            std::string name = to_reload->GetName();
+            wallet_dirs.insert(fs::PathFromString(to_reload->GetDatabase().Filename()).parent_path());
+            to_reload.reset();
+            to_reload = LoadWallet(context, name, /*load_on_start=*/std::nullopt, options, status, error, warnings);


In commit "wallet: Reload watchonly and solvables wallets after migration" (b57218e)

Just a thought, not for this PR, but maybe it would be good to load the new wallet on startup if the previous wallet loaded on startup.

Also it would be good to improve error reporting here. It seems like errors/warnings just get discarded which might make problems hard to debug. Though I guess we don't expect failure loading a newly creating wallet.

src/wallet/wallet.cpp

ryanofsky · 2023-10-19T20:46:49Z

src/wallet/wallet.cpp

        }

        // Delete the wallet directories
-        for (fs::path& dir : wallet_dirs) {
+        for (const fs::path& dir : wallet_dirs) {
            fs::remove_all(dir);


In commit "wallet: Reload watchonly and solvables wallets after migration" (b57218e)

Not directly related to this PR, but this remove_all seems not great because if there were any non-wallet files in the wallet directory they will be recursively removed. I think it would be preferable to only delete files that we know about and leave files that we do not recognize alone.

This would also avoid the need to copy the backup file from the wallet directory to the wallet parent directory, and then copy it back in again when the wallet directory is recreated.

A lot of this logic seems pretty fragile and possible to simplify (Maybe furszy already has a PR to do it?)

Not in a PR, but I do have something related in progress. Still, we should move forward with your suggestion; it is much more scoped than what I'm doing.

I'm rewriting the process, so the bdb->sqlite migration, the removal of "no longer ours" database records, and the backup creation steps are not needed.
The goal is to leave the legacy wallet untouched for the entire migration process. Then, only if migration succeeds, execute the path rename. So, in case the user faces a crash or abortion during the process, the legacy wallet will open as if nothing has happened. Which is what the process lacks today; If it crashes right now, the user has to manually do the cleanup and previous wallet recovery.

This would indirectly solve the issue you mentioned because, in case of a failure, the process would only need to delete the newly created wallet directories and nothing else.

When moving a tx to the watchonly wallet during migration, make sure that all of the CWalletTx data follows it.

When migrating, create the watchonly and solvables wallets without a context. Then unload and reload them after migration completes, as we do for the actual wallet. There is also additional handling for a failed reload.

achow101 · 2023-10-19T22:14:40Z

This really seems like 3 unrelated changes smashed into one PR

Two of the changes are actually required for the third.

The primary change here is to create the watchonly and solvables wallets without a chain context. This necessitates the second change of reloading at the end of migration. This also required changes to the cleanup, which prompted me to write a test for that.

It then turned out that AddToWallet requires a chain, and without one it segfaults. So this required changing the copying to use LoadToWallet. This has the side effect of copying the metadata, and when I realized that, I also wrote a test for it.

ishaanam

light code review ACK 4814e40

This looks good so far, might review d616d30 a bit more later.

ryanofsky

Code review ACK 4814e40. Just implemented the suggested orderpos, copyfrom, and path set comments since last review

furszy

ACK 4814e40

Open topic #28609 (comment).

src/wallet/transaction.cpp

furszy · 2023-10-21T14:34:03Z

src/wallet/wallet.cpp

        }

        // Delete the wallet directories
-        for (fs::path& dir : wallet_dirs) {
+        for (const fs::path& dir : wallet_dirs) {
            fs::remove_all(dir);


Not in a PR, but I do have something related in progress. Still, we should move forward with your suggestion; it is much more scoped than what I'm doing.

I'm rewriting the process, so the bdb->sqlite migration, the removal of "no longer ours" database records, and the backup creation steps are not needed.
The goal is to leave the legacy wallet untouched for the entire migration process. Then, only if migration succeeds, execute the path rename. So, in case the user faces a crash or abortion during the process, the legacy wallet will open as if nothing has happened. Which is what the process lacks today; If it crashes right now, the user has to manually do the cleanup and previous wallet recovery.

This would indirectly solve the issue you mentioned because, in case of a failure, the process would only need to delete the newly created wallet directories and nothing else.

S3RK · 2023-10-23T08:09:43Z

src/wallet/transaction.h

    // Disable copying of CWalletTx objects to prevent bugs where instances get
    // copied in and out of the mapWallet map, and fields are updated in the
    // wrong copy.
-    CWalletTx(CWalletTx const &) = delete;
-    void operator=(CWalletTx const &x) = delete;
+    CWalletTx(const CWalletTx&) = default;


Should the comment above be updated?

I think it's still valid when these are private.

…ith asserts, comments, and refactoring f06016d wallet: Add asserts to detect unset transaction height values (Ryan Ofsky) 262a78b wallet, refactor: Add CWalletTx::updateState function (Ryan Ofsky) Pull request description: Originally, this PR fixed a wallet migration bug that could cause the watchonly wallet created by legacy wallet migration to have incorrect transaction height values. A different fix for the bug was implemented in #28609, but that PR did not add any test coverage that would have caught the bug, and didn't include other changes from this PR intended to prevent problems from invalid transaction heights. This PR adds new asserts to catch invalid transaction heights, which would trigger test failures without bugfix in #28609. This PR also refactors code and adds comments to clarify assumptions and make it less likely a bug from invalid transaction height values would be introduced. ACKs for top commit: achow101: ACK f06016d Sjors: utACK f06016d furszy: Code review ACK f06016d Tree-SHA512: 82657c403724d60354f7676b53bcfcc95bdc5864e051a2eb8bfad09d8ad35615393b2d6b432b46f908def9be37bebded3a55ec9ae19e19371d35897fe842c92e

…fter migration 4814e40 test: Check tx metadata is migrated to watchonly (Andrew Chow) d616d30 wallet: Reload watchonly and solvables wallets after migration (Andrew Chow) 118f2d7 wallet: Copy all tx metadata to watchonly wallet (Andrew Chow) 9af87cf test: Check that a failed wallet migration is cleaned up (Andrew Chow) Pull request description: Some incomplete/incorrect state as a result of migration can be mitigated/cleaned up by simply restarting the migrated wallets. We already do this for a wallet when it is migrated, but we do not for the new watchonly and solvables wallets that may be created. This PR introduces this behavior, in addition to creating those wallets initially without an attached chain. While implementing this, I noticed that not all `CWalletTx` metadata was being copied over to the watchonly wallet and so some data, such as time received, was being lost. This PR fixes this as a side effect of not having a chain attached to the watchonly wallet. A test has also been added. ACKs for top commit: ishaanam: light code review ACK 4814e40 ryanofsky: Code review ACK 4814e40. Just implemented the suggested orderpos, copyfrom, and path set comments since last review furszy: ACK 4814e40 Tree-SHA512: 0b992430df9f452cb252c2212df8e876613f43564fcd1dc00c6c31fa497adb84dfff6b5ef597590f9b288c5f64cb455f108fcc9b6c9d1fe9eb2c39e7f2c12a89

DrahtBot added the Wallet label Oct 6, 2023

furszy reviewed Oct 9, 2023

View reviewed changes

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

furszy reviewed Oct 9, 2023

View reviewed changes

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

furszy reviewed Oct 9, 2023

View reviewed changes

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

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

achow101 force-pushed the reload-all-migrated branch from 1fe21d2 to af91cfa Compare October 10, 2023 18:52

DrahtBot mentioned this pull request Oct 10, 2023

refactor: [tidy] modernize-use-emplace #28583

Merged

DrahtBot added the Needs rebase label Oct 11, 2023

achow101 added this to the 26.0 milestone Oct 11, 2023

achow101 force-pushed the reload-all-migrated branch from af91cfa to deb0b31 Compare October 11, 2023 15:42

DrahtBot removed the Needs rebase label Oct 11, 2023

furszy reviewed Oct 11, 2023

View reviewed changes

furszy mentioned this pull request Oct 13, 2023

rpc: Drop migratewallet experimental warning #28037

Merged

DrahtBot added the Needs rebase label Oct 16, 2023

achow101 force-pushed the reload-all-migrated branch from deb0b31 to 014231c Compare October 16, 2023 19:22

DrahtBot added CI failed and removed Needs rebase labels Oct 16, 2023

DrahtBot mentioned this pull request Oct 16, 2023

wallet: Migrate entire address book entries to watchonly and solvables too #28610

Merged

furszy reviewed Oct 17, 2023

View reviewed changes

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

test/functional/wallet_migration.py Show resolved Hide resolved

achow101 force-pushed the reload-all-migrated branch from 014231c to 13dd5e5 Compare October 17, 2023 17:32

test: Check that a failed wallet migration is cleaned up

9af87cf

achow101 force-pushed the reload-all-migrated branch from 13dd5e5 to e2dfe76 Compare October 17, 2023 18:57

DrahtBot removed the CI failed label Oct 17, 2023

furszy reviewed Oct 17, 2023

View reviewed changes

DrahtBot mentioned this pull request Oct 17, 2023

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

Draft

maflcko requested review from Sjors and ryanofsky October 19, 2023 08:26

ryanofsky approved these changes Oct 19, 2023

View reviewed changes

achow101 force-pushed the reload-all-migrated branch 2 times, most recently from b927000 to 7d6d6d6 Compare October 19, 2023 22:05

achow101 added 3 commits October 19, 2023 18:06

wallet: Copy all tx metadata to watchonly wallet

118f2d7

When moving a tx to the watchonly wallet during migration, make sure that all of the CWalletTx data follows it.

wallet: Reload watchonly and solvables wallets after migration

d616d30

When migrating, create the watchonly and solvables wallets without a context. Then unload and reload them after migration completes, as we do for the actual wallet. There is also additional handling for a failed reload.

test: Check tx metadata is migrated to watchonly

4814e40

achow101 force-pushed the reload-all-migrated branch from 7d6d6d6 to 4814e40 Compare October 19, 2023 22:06

achow101 mentioned this pull request Oct 20, 2023

Proposed Timeline for Legacy Wallet and BDB removal #20160

Open

40 tasks

ishaanam reviewed Oct 20, 2023

View reviewed changes

DrahtBot requested review from ryanofsky and furszy October 20, 2023 20:45

ryanofsky approved these changes Oct 20, 2023

View reviewed changes

furszy reviewed Oct 21, 2023

View reviewed changes

S3RK reviewed Oct 23, 2023

View reviewed changes

bitcoin deleted a comment Oct 23, 2023

ryanofsky merged commit d724bb5 into bitcoin:master Oct 23, 2023
16 checks passed

furszy mentioned this pull request Nov 15, 2023

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

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet: Reload watchonly and solvables wallets after migration #28609

wallet: Reload watchonly and solvables wallets after migration #28609

achow101 commented Oct 6, 2023

DrahtBot commented Oct 6, 2023 •

edited

furszy Oct 11, 2023

achow101 Oct 16, 2023

furszy Oct 11, 2023

achow101 Oct 16, 2023

furszy Oct 11, 2023

achow101 Oct 16, 2023

DrahtBot commented Oct 17, 2023

furszy left a comment

furszy left a comment

ryanofsky left a comment

ryanofsky Oct 19, 2023

ryanofsky Oct 19, 2023

furszy Oct 21, 2023

achow101 commented Oct 19, 2023

ishaanam left a comment

ryanofsky left a comment

furszy left a comment •

edited

furszy Oct 21, 2023

S3RK Oct 23, 2023

achow101 Oct 23, 2023

		assert(local_wallet.use_count() == 1);
		wallet_dirs.insert(fs::PathFromString(local_wallet->GetDatabase().Filename()).parent_path());

wallet: Reload watchonly and solvables wallets after migration #28609

wallet: Reload watchonly and solvables wallets after migration #28609

Conversation

achow101 commented Oct 6, 2023

DrahtBot commented Oct 6, 2023 • edited

Code Coverage

Reviews

Conflicts

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented Oct 17, 2023

furszy left a comment

Choose a reason for hiding this comment

furszy left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

achow101 commented Oct 19, 2023

ishaanam left a comment

Choose a reason for hiding this comment

ryanofsky left a comment

Choose a reason for hiding this comment

furszy left a comment • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

DrahtBot commented Oct 6, 2023 •

edited

furszy left a comment •

edited