Repository: bitcoin/bitcoin (language: cpp)

Issue:
wallet: wallet_migration.py fails on sqlite-only build
Steps to reproduce:

* sqlite-only build (the default)
* `./bld-cmake/test/functional/wallet_migration.py`

```
2024-12-07T04:56:18.691000Z TestFramework (INFO): PRNG seed is: 7532643259753153678
2024-12-07T04:56:18.691000Z TestFramework (INFO): Initializing test directory /tmp/test_runner_₿_🏃_20241207_042808/wallet_migration_2
2024-12-07T04:56:20.630000Z TestFramework (INFO): Test migration of a basic keys only wallet without balance
2024-12-07T04:56:20.915000Z TestFramework (INFO): Test migration of a basic keys only wallet with a balance
2024-12-07T04:56:26.448000Z TestFramework (INFO): Test backup file can be successfully restored
2024-12-07T04:56:27.024000Z TestFramework (INFO): Test migration of a wallet with balance received on the seed
2024-12-07T04:56:28.270000Z TestFramework (INFO): Test "nothing to migrate" when the user tries to migrate a loaded wallet with no legacy data
2024-12-07T04:56:28.271000Z TestFramework (INFO): Test "nothing to migrate" when the user tries to migrate an unloaded wallet with no legacy data
2024-12-07T04:56:28.273000Z TestFramework (INFO): Test migration of a wallet with all keys for a multisig
2024-12-07T04:56:28.473000Z TestFramework (INFO): Test migration of a wallet that has some keys in a multisig
2024-12-07T04:56:28.798000Z TestFramework (INFO): Test migration of a wallet with watchonly imports
2024-12-07T04:56:29.107000Z TestFramework (INFO): Test migration of a pure watchonly wallet
2024-12-07T04:56:29.217000Z TestFramework (INFO): Test migration of a pure watchonly wallet with pubkeys in keypool
2024-12-07T04:56:29.326000Z TestFramework (INFO): Test migration of a wallet using old pk() coinbases
2024-12-07T04:56:29.529000Z TestFramework (INFO): Test migration of an encrypted wallet
2024-12-07T04:56:31.123000Z TestFramework (INFO): Check migratewallet errors for nonexistent wallets
2024-12-07T04:56:31.124000Z TestFramework (INFO): Test migration of a wallet that isn't loaded, specified by path
2024-12-07T04:56:31.300000Z TestFramework (INFO): Test migration of the wallet named as the empty string
2024-12-07T04:56:31.450000Z TestFramework (INFO): Test migration of a wallet that is not in a wallet directory
2024-12-07T04:56:31.584000Z TestFramework (INFO): Test migration of address book data
2024-12-07T04:56:35.062000Z TestFramework (INFO): Test migration of watch-only raw p2sh script
2024-12-07T04:56:35.346000Z TestFramework (INFO): Test migration when wallet contains conflicting transactions
2024-12-07T04:56:35.609000Z TestFramework (INFO): Test migration when wallet contains a hybrid pubkey
2024-12-07T04:56:35.829000Z TestFramework (INFO): Test that a failed migration is cleaned up
2024-12-07T04:56:35.968000Z TestFramework (ERROR): Assertion failed
Traceback (most recent call last):
  File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 160, in try_rpc
    fun(*args, **kwds)
  File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/coverage.py", line 50, in __call__
    return_val = self.auth_service_proxy_instance.__call__(*args, **kwargs)
                 ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/authproxy.py", line 146, in __call__
    raise JSONRPCException(response['error'], status)
test_framework.authproxy.JSONRPCException: Wallet file verification failed. Failed to open database path '/tmp/test_runner_₿_🏃_20241207_042808/wallet_migration_2/node0/regtest/wallets/failed'. Build does not support Berkeley DB database format.
Unable to restore backup of wallet. (-4)
During handling of the above exception, another exception occurred:
Traceback (most recent call last):
  File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/test_framework.py", line 135, in main
    self.run_test()
  File "/tmp/cirrus-ci-build/bitcoin-core/bld-cov/test/functional/wallet_migration.py", line 1031, in run_test
    self.test_failed_migration_cleanup()
  File "/tmp/cirrus-ci-build/bitcoin-core/bld-cov/test/functional/wallet_migration.py", line 895, in test_failed_migration_cleanup
    assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, "failed")
  File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 151, in assert_raises_rpc_error
    assert try_rpc(code, message, fun, *args, **kwds), "No exception raised"
           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "/tmp/cirrus-ci-build/bitcoin-core/test/functional/test_framework/util.py", line 166, in try_rpc
    raise AssertionError(
AssertionError: Expected substring not found in error message:
substring: 'Failed to create database'
error message: 'Wallet file verification failed. Failed to open database path '/tmp/test_runner_₿_🏃_20241207_042808/wallet_migration_2/node0/regtest/wallets/failed'. Build does not support Berkeley DB database format.
Unable to restore backup of wallet.'.


Discussion / hints:
The error occurs during the final `RestoreWallet()` call, post migration error. We attempt to reload the BDB wallet in rw mode even though support for it is unavailable. Moving the process to open the wallet in read-only mode fixes the issue but it might unchain other problems; e.g. leave the read-only bdb wallet loaded, and "usable", in the system.
will cook up something to fix it.
I'm surprised that previous revisions of #28710 or #31250 don't seem to run into this, given that BDB is completely removed there. I suppose it's possible that it also fixed the problem in doing the removal.
> I'm surprised that previous revisions of #28710 or #31250 don't seem to run into this, given that BDB is completely removed there. I suppose it's possible that it also fixed the problem in doing the removal.

Just looking at it (after pushing the PR.. sorry). You fixed it in #31250, in 9beab081e977e86a462854035ad31a118a8b13ae. It works fine because the legacy wallet can no longer be loaded there, but a direct cherry-pick into master would prematurely block the reload while it's possible when bdb is still enabled. I initially started coding the fix the same way you did but decided to keep the reload enabled until we fully disable bdb in #31250. Yet, I'm not totally convinced about the approach atm, happy to receive some feedback.
> You fixed it in #31250, in [9beab08](https://github.com/bitcoin/bitcoin/commit/9beab081e977e86a462854035ad31a118a8b13ae)

Ah yes!

> but a direct cherry-pick into master would prematurely block the reload while it's possible when bdb is still enabled.

Yes, but that seems trivially fixable? We already know whether the wallet being migrated was loaded, so we could just pass `was_loaded` to `RestoreWallet` to load the previously loaded wallet.
> Yes, but that seems trivially fixable? We already know whether the wallet being migrated was loaded, so we could just pass was_loaded to RestoreWallet to load the previously loaded wallet.

Ha, yeah true. I was trying to avoid the `RestoreWallet()` double nullptr return value meaning (nullptr for errors and nullptr when `was_loaded=true`) but.. that is fine because we have the `error` field to compare results anyway.
Just updated the PR with few other improvements. Thanks for the feedback!

Fix patch:
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 0961aebc7c993..5c597e5a26d9f 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -490,7 +490,9 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
     return wallet;
 }
 
-std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings)
+// Re-creates wallet from the backup file by renaming and moving it into the wallet's directory.
+// If 'load_after_restore=true', the wallet object will be fully initialized and appended to the context.
+std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings, bool load_after_restore)
 {
     DatabaseOptions options;
     ReadDatabaseArgs(*context.args, options);
@@ -515,13 +517,17 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b
 
         fs::copy_file(backup_file, wallet_file, fs::copy_options::none);
 
-        wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
+        if (load_after_restore) {
+            wallet = LoadWallet(context, wallet_name, load_on_start, options, status, error, warnings);
+        }
     } catch (const std::exception& e) {
         assert(!wallet);
         if (!error.empty()) error += Untranslated("\n");
         error += Untranslated(strprintf("Unexpected exception: %s", e.what()));
     }
-    if (!wallet) {
+
+    // Remove created wallet path only when loading fails
+    if (load_after_restore && !wallet) {
         fs::remove_all(wallet_path);
     }
 
@@ -4523,7 +4529,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
     }
     if (!success) {
         // Migration failed, cleanup
-        // Copy the backup to the actual wallet dir
+        // Before deleting the wallet's directory, copy the backup file to the top-level wallets dir
         fs::path temp_backup_location = fsbridge::AbsPathJoin(GetWalletDir(), backup_filename);
         fs::copy_file(backup_path, temp_backup_location, fs::copy_options::none);
 
@@ -4560,17 +4566,24 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(const std::string& walle
         }
 
         // Restore the backup
-        DatabaseStatus status;
-        std::vector<bilingual_str> warnings;
-        if (!RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, error, warnings)) {
-            error += _("\nUnable to restore backup of wallet.");
+        // Convert the backup file to the wallet db file by renaming it and moving it into the wallet's directory.
+        // Reload it into memory if the wallet was previously loaded.
+        bilingual_str restore_error;
+        const auto& ptr_wallet = RestoreWallet(context, temp_backup_location, wallet_name, /*load_on_start=*/std::nullopt, status, restore_error, warnings, /*load_after_restore=*/was_loaded);
+        if (!restore_error.empty()) {
+            error += restore_error + _("\nUnable to restore backup of wallet.");
             return util::Error{error};
         }
 
-        // Move the backup to the wallet dir
+        // The wallet directory has been restored, but just in case, copy the previously created backup to the wallet dir
         fs::copy_file(temp_backup_location, backup_path, fs::copy_options::none);
         fs::remove(temp_backup_location);
 
+        // Verify that there is no dangling wallet: when the wallet wasn't loaded before, expect null.
+        // This check is performed after restoration to avoid an early error before saving the backup.
+        bool wallet_reloaded = ptr_wallet != nullptr;
+        assert(was_loaded == wallet_reloaded);
+
         return util::Error{error};
     }
     return res;
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index d869f031bbf7b..c6a45e9a1482f 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -95,7 +95,7 @@ std::shared_ptr<CWallet> GetDefaultWallet(WalletContext& context, size_t& count)
 std::shared_ptr<CWallet> GetWallet(WalletContext& context, const std::string& name);
 std::shared_ptr<CWallet> LoadWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
 std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string& name, std::optional<bool> load_on_start, DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
-std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings);
+std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& backup_file, const std::string& wallet_name, std::optional<bool> load_on_start, DatabaseStatus& status, bilingual_str& error, std::vector<bilingual_str>& warnings, bool load_after_restore = true);
 std::unique_ptr<interfaces::Handler> HandleLoadWallet(WalletContext& context, LoadWalletFn load_wallet);
 void NotifyWalletLoaded(WalletContext& context, const std::shared_ptr<CWallet>& wallet);
 std::unique_ptr<WalletDatabase> MakeWalletDatabase(const std::string& name, const DatabaseOptions& options, DatabaseStatus& status, bilingual_str& error);


Test patch:
diff --git a/test/functional/wallet_migration.py b/test/functional/wallet_migration.py
index 5abb43e3b97e4..da816f9a8453d 100755
--- a/test/functional/wallet_migration.py
+++ b/test/functional/wallet_migration.py
@@ -894,9 +894,7 @@ def test_failed_migration_cleanup(self):
         shutil.copytree(self.old_node.wallets_path / "failed", self.master_node.wallets_path / "failed")
         assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, "failed")
 
-        assert "failed" in self.master_node.listwallets()
-        assert "failed_watchonly" not in self.master_node.listwallets()
-        assert "failed_solvables" not in self.master_node.listwallets()
+        assert all(wallet not in self.master_node.listwallets() for wallet in ["failed", "failed_watchonly", "failed_solvables"])
 
         assert not (self.master_node.wallets_path / "failed_watchonly").exists()
         # Since the file in failed_solvables is one that we put there, migration shouldn't touch it
@@ -910,6 +908,22 @@ def test_failed_migration_cleanup(self):
             _, _, magic = struct.unpack("QII", data)
             assert_equal(magic, BTREE_MAGIC)
 
+        ####################################################
+        # Perform the same test with a loaded legacy wallet.
+        # The wallet should remain loaded after the failure.
+        #
+        # This applies only when BDB is enabled, as the user
+        # cannot interact with the legacy wallet database
+        # without BDB support.
+        if self.is_bdb_compiled() is not None:
+            # Advance time to generate a different backup name
+            self.master_node.setmocktime(self.master_node.getblockheader(self.master_node.getbestblockhash())['time'] + 100)
+            assert "failed" not in self.master_node.listwallets()
+            self.master_node.loadwallet("failed")
+            assert_raises_rpc_error(-4, "Failed to create database", self.master_node.migratewallet, "failed")
+            wallets = self.master_node.listwallets()
+            assert "failed" in wallets and all(wallet not in wallets for wallet in ["failed_watchonly", "failed_solvables"])
+
     def test_blank(self):
         self.log.info("Test that a blank wallet is migrated")
         wallet = self.create_legacy_wallet("blank", blank=True)