Merge bitcoin/bitcoin#32984: wallet: Set migrated wallet name only on success

060695c test: Failed load after migrate should restore backup (MarcoFalke)
8a4cfdd wallet: Set migrated wallet name only on success (Ava Chow)

Pull request description:

  After a wallet is migrated and we are trying to load it, if it could not be loaded, don't try to set the wallet name. Otherwise we have a segfault.

  This can be tested by migrated a legacy wallet from another network (e.g. trying to migrate a testnet wallet on mainnet). The fixed behavior is return an error and restore the backup.

ACKs for top commit:
  davidgumberg:
    ACK 060695c
  furszy:
    ACK 060695c
  rkrux:
    ACK 060695c
  w0xlt:
    reACK bitcoin/bitcoin@060695c
  pablomartin4btc:
    ACK 060695c

Tree-SHA512: f4289e0b3dedef0a3d734c18604f2fd0df0dc65e9641bc342cfa45088d2540a3f6631bbea8bdd394b2631fa83b38e8ac37c83cfc4b53b19dcbd0b820a9beb6e4

Author: fanquake

src/wallet/wallet.cpp

@@ -4306,7 +4306,7 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
                 success = (wallet != nullptr);
 
                 // When no wallet is set, set the main wallet.
-                if (!res.wallet) {
+                if (success && !res.wallet) {
                     res.wallet_name = wallet->GetName();
                     res.wallet = std::move(wallet);
                 }

test/functional/wallet_migration.py

@@ -65,6 +65,12 @@ def assert_is_sqlite(self, wallet_name):
             assert_equal(file_magic, b'SQLite format 3\x00')
         assert_equal(self.master_node.get_wallet_rpc(wallet_name).getwalletinfo()["format"], "sqlite")
 
+    def assert_is_bdb(self, wallet_name):
+        with open(self.master_node.wallets_path / wallet_name / self.wallet_data_filename, "rb") as f:
+            data = f.read(16)
+            _, _, magic = struct.unpack("QII", data)
+            assert_equal(magic, BTREE_MAGIC)
+
     def create_legacy_wallet(self, wallet_name, **kwargs):
         self.old_node.createwallet(wallet_name=wallet_name, descriptors=False, **kwargs)
         wallet = self.old_node.get_wallet_rpc(wallet_name)
@@ -916,10 +922,7 @@ def test_failed_migration_cleanup(self):
         assert_equal(original_shasum, new_shasum)
 
         # Check the wallet we tried to migrate is still BDB
-        with open(self.master_node.wallets_path / "failed" / "wallet.dat", "rb") as f:
-            data = f.read(16)
-            _, _, magic = struct.unpack("QII", data)
-            assert_equal(magic, BTREE_MAGIC)
+        self.assert_is_bdb("failed")
 
     def test_blank(self):
         self.log.info("Test that a blank wallet is migrated")
@@ -1372,6 +1375,27 @@ def test_solvable_no_privs(self):
         assert_equal(addr_info["solvable"], True)
         assert "hex" in addr_info
 
+    def test_loading_failure_after_migration(self):
+        self.log.info("Test that a failed loading of the wallet at the end of migration restores the backup")
+        self.stop_node(self.old_node.index)
+        self.old_node.chain = "signet"
+        self.old_node.replace_in_config([("regtest=", "signet="), ("[regtest]", "[signet]")])
+        # Disable network sync and prevent disk space warning on small (tmp)fs
+        self.start_node(self.old_node.index, extra_args=self.old_node.extra_args + ["-maxconnections=0", "-prune=550"])
+
+        wallet_name = "failed_load_after_migrate"
+        self.create_legacy_wallet(wallet_name)
+        assert_raises_rpc_error(-4, "Wallet loading failed. Wallet files should not be reused across chains.", lambda: self.migrate_and_get_rpc(wallet_name))
+
+        # Check the wallet we tried to migrate is still BDB
+        self.assert_is_bdb(wallet_name)
+
+        self.stop_node(self.old_node.index)
+        self.old_node.chain = "regtest"
+        self.old_node.replace_in_config([("signet=", "regtest="), ("[signet]", "[regtest]")])
+        self.start_node(self.old_node.index)
+        self.connect_nodes(1, 0)
+
     def run_test(self):
         self.master_node = self.nodes[0]
         self.old_node = self.nodes[1]
@@ -1404,6 +1428,7 @@ def run_test(self):
         self.test_miniscript()
         self.test_taproot()
         self.test_solvable_no_privs()
+        self.test_loading_failure_after_migration()
 
 if __name__ == '__main__':
     WalletMigrationTest(__file__).main()