wallet: Migrate wallets that are not in a wallet dir #26740

achow101 commented at 10:55 PM on December 21, 2022: member

This PR fixes an assertion error that is hit during the setup of the new database during migration of a wallet that was not contained in a wallet dir. Also added a test for this case as well as one for migrating the default wallet.

DrahtBot commented at 10:55 PM on December 21, 2022: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	ryanofsky, furszy
Stale ACK	aureleoules

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:

#26836 (wallet: finish addressbook encapsulation and simplify addressbook migration by furszy)

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 Dec 21, 2022

aureleoules approved

aureleoules commented at 3:15 PM on January 2, 2023: member

ACK 17ffb123a14f9f8af3c0425563f4169d403d6207 - LGTM

I cherry-picked the test on master and it indeed crashes.

furszy approved

furszy commented at 2:09 AM on January 18, 2023: member

Code review ACK 17ffb123

The assertion error happens when you have the default legacy wallet created (no dir, standalone file), and try to migrate another wallet file that is placed in the same base directory.

As we are using the directory path inside the migration process to create the new database instead of the file path, the code auto-completes the path with the wallet.dat file name; which clashes with the default legacy wallet, which already exist, returning nullptr from the db creation process, crashing at the assertion point.

If there would be no default legacy wallet, the process should work "fine". The only problematic would be the migrated wallet file that would be named "wallet.dat" instead of using the wallet's real filename.

DrahtBot added the label Needs rebase on Feb 22, 2023

achow101 force-pushed on Mar 1, 2023

DrahtBot removed the label Needs rebase on Mar 1, 2023

furszy approved

furszy commented at 11:33 PM on March 2, 2023: member

rebase ACK 52634dba

DrahtBot requested review from aureleoules on Mar 2, 2023

DrahtBot added the label CI failed on May 2, 2023

DrahtBot removed the label CI failed on May 2, 2023

in src/wallet/wallet.cpp:3850 in 52634dba1c outdated

3846 | @@ -3847,6 +3847,11 @@ bool CWallet::MigrateToSQLite(bilingual_str& error)
3847 |      m_database->Close();
3848 |      fs::remove(db_path);
3849 |  
3850 | +    if (db_path.filename() == GetName()) {

Sjors commented at 3:24 PM on June 5, 2023:

I'm a bit confused what this comparison is doing.

furszy commented at 3:29 PM on June 5, 2023:

If you load a wallet from an external path (not the wallet directory), the wallet name is set to the external path.

Sjors commented at 3:42 PM on June 5, 2023:

So this applies to wallets outside the datadir, but not to the original default wallet sitting in the root of the data dir (instead of /wallets)?

furszy commented at 3:52 PM on June 5, 2023:

Check #26740#pullrequestreview-1252729613.

IIRC (because passed some time since I tested this), you can have several wallets inside the datadir root, and trying to migrate them cause the error when the old wallet.dat is there, because the process is not using the wallet name, it's using the db path and appending the "wallet.dat" name at the end.

achow101 commented at 11:38 AM on June 8, 2023:

For wallets that are not in a wallet dir, the db's filename will be the name of the wallet itself.

ryanofsky commented at 1:29 PM on June 9, 2023:

In commit "wallet: Be able to migrate wallets that are not in a wallet dir" (e22a3a471bf6696cb9ac96f3f348030cb07aa8b2)

This logic seems unnecessarily confusing. I think there is no benefit to writing:

fs::path db_dir = db_path.parent_path();
if (db_path.filename() == GetName()) {
    db_dir = db_path
}

Instead of simply:

const fs::path db_dir = GetName();

It also looks like db_dir variable is used only once, so you could probably get rid of it and just write GetName() directly below.

achow101 commented at 5:10 PM on June 12, 2023:

It can't use GetName() directly since that is just the name of the wallet, not the path to where we want it to be stored as db_dir is in this case. With external wallets, this would be the path to the wallet, but with wallet files in the base walletdir, GetName() would be the just the name of the file, not the path to it.

For example, a typical path for a wallet is ~/.bitcoin/wallets/mywallet/wallet.dat. However we allow wallet files in the base walletdir, so ~/.bitcoin/wallets/mywallet is also allowed. In both cases, the name of the wallet is mywallet, even though the paths to the database are different. Since MakeDatabase takes the path to the directory that will contain the wallet.dat file, we need to get the path to the parent dir for the first case. For the second case, we want to migrate into the subdir structure, so we need to use the same path as the file itself. This code detects the latter case by checking whether the filename (the last component of the path) matches the name of the wallet.

ryanofsky commented at 6:25 PM on June 12, 2023:

Sorry, I overlooked that in my suggestion. But I think this function can do the same thing as MakeWalletDatabase, VerifyWallets, RestoreWallet, MigrateLegacyToDescriptor, and ExecuteWalletToolFunc and derive the wallet path from the wallet name in the normal way. The following change seems to work:

--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -3843,21 +3843,18 @@ bool CWallet::MigrateToSQLite(bilingual_str& error)
 
     // Close this database and delete the file
     fs::path db_path = fs::PathFromString(m_database->Filename());
-    fs::path db_dir = db_path.parent_path();
     m_database->Close();
     fs::remove(db_path);
 
-    if (db_path.filename() == GetName()) {
-        // For wallet files that aren't in a wallet dir, we will want to make a wallet dir that has the same name
-        db_dir = db_path;
-    }
+    // Turn wallet name into an absolute path that can be passed to passed to MakeDatabase.
+    const fs::path wallet_path = fsbridge::AbsPathJoin(GetWalletDir(), fs::PathFromString(m_name));
 
     // Make new DB
     DatabaseOptions opts;
     opts.require_create = true;
     opts.require_format = DatabaseFormat::SQLITE;
     DatabaseStatus db_status;
-    std::unique_ptr<WalletDatabase> new_db = MakeDatabase(db_dir, opts, db_status, error);
+    std::unique_ptr<WalletDatabase> new_db = MakeDatabase(wallet_path, opts, db_status, error);
     assert(new_db); // This is to prevent doing anything further with this wallet. The original file was deleted, but a backup exists.
     m_database.reset();
     m_database = std::move(new_db);

achow101 commented at 7:13 PM on June 12, 2023:

Done as suggested

in test/functional/wallet_migration.py:475 in 52634dba1c outdated

 470 | @@ -470,6 +471,40 @@ def test_unloaded_by_path(self):
 471 |  
 472 |          assert_equal(bals, wallet.getbalances())
 473 |  
 474 | +    def test_default_wallet(self):
 475 | +        self.log.info("Test migration of the default wallet")

ryanofsky commented at 1:39 PM on June 9, 2023:

In commit "test: Add test for migrating default wallet and plain file wallet" (52634dba1cba8928b8767d05c8e30b5fd3da45e4)

Maybe say "legacy top level wallet" instead of "default wallet". Default wallet isn't really a concept that exists after #15454

achow101 commented at 5:10 PM on June 12, 2023:

Will do if I need to push again.

ryanofsky approved

ryanofsky commented at 1:40 PM on June 9, 2023: contributor

Code review ACK 52634dba1cba8928b8767d05c8e30b5fd3da45e4, but it would be nice if fix could be simplified as suggested.

bitcoin deleted a comment on Jun 9, 2023

achow101 force-pushed on Jun 12, 2023

wallet: Generated migrated wallet's path from walletdir and name

Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>

bdbe3fd76b

test: Add test for migrating default wallet and plain file wallet a1e653828b

achow101 force-pushed on Jun 12, 2023

ryanofsky approved

ryanofsky commented at 7:31 PM on June 12, 2023: contributor

Code review ACK a1e653828bc59351b2a0dd5a70f519e6b61199bc

Just applied suggested change since last review

DrahtBot requested review from furszy on Jun 12, 2023

DrahtBot added the label CI failed on Jun 12, 2023

furszy approved

furszy commented at 12:55 AM on June 14, 2023: member

ACK a1e65382

ryanofsky merged this on Jun 20, 2023

ryanofsky closed this on Jun 20, 2023

sidhujag referenced this in commit 33c2474268 on Jun 21, 2023

bitcoin locked this on Jun 19, 2024