Merge bitcoin/bitcoin#26747: wallet: fix confusing error / GUI crash on cross-chain legacy wallet restore

21ad4e26ec test: add coverage for cross-chain wallet restore (Sebastian Falbesoner)
8c7222bda3 wallet: fix GUI crash on cross-chain legacy wallet restore (Sebastian Falbesoner)

Pull request description:

  Restoring a wallet backup from another chain should result in a dedicated error message (we have _"Wallet files should not be reused across chains. Restart bitcoind with -walletcrosschain to override."_ for that). Unfortunately this is currently not the case for legacy wallet restores, as in the course of cleaning up the newly created wallet directory a `filesystem_error` exception is thrown due to the directory not being empty; the wallet database did indeed load successfully (otherwise we wouldn't know that the chain doesn't match) and hence BDB-related files and directories are already created in the wallet directory.

  For bitcoind, this leads to a very confusing error message:
  ```
  $ ./src/bitcoin-cli restorewallet test123 ~/.bitcoin/regtest/wallets/regtest_wallet/wallet.dat
  error code: -1
  error message: filesystem error: in remove: Directory not empty ["/home/thestack/.bitcoin/wallets/test123"]
  ```

  Even worse, the GUI crashes in such a scenario:
  ```
  libc++abi: terminating with uncaught exception of type std::__1::__fs::filesystem::filesystem_error: filesystem error: in remove: Directory not empty ["/home/thestack/.bitcoin/wallets/foobar"]
  Abort trap (core dumped)
  ```

  Fix this by simply deleting the whole folder via `fs::remove_all`. With this, the expected error message appears both for the `restorewallet` RPC call and in the GUI (as a message-box):

  ```
  $ ./src/bitcoin-cli restorewallet test123 ~/.bitcoin/regtest/wallets/regtest_wallet/wallet.dat
  error code: -4
  error message:
  Wallet loading failed. Wallet files should not be reused across chains. Restart bitcoind with -walletcrosschain to override.
  ```

ACKs for top commit:
  achow101:
    ACK 21ad4e26ec
  aureleoules:
    ACK 21ad4e26ec
  furszy:
    utACK 21ad4e26

Tree-SHA512: 313f6494c2fbe823bff9b975cb2d9410bb518977a1e59a5159ee9836bc012947fa50b56be0e41b1a2f50d9c0c7f4fddfdf4fbe479d8a59a6ee44bb389c804abc

src/wallet/wallet.cpp

@@ -472,8 +472,7 @@ std::shared_ptr<CWallet> RestoreWallet(WalletContext& context, const fs::path& b
         error += strprintf(Untranslated("Unexpected exception: %s"), e.what());
     }
     if (!wallet) {
-        fs::remove(wallet_file);
+        fs::remove_all(wallet_path);
-        fs::remove(wallet_path);
     }
 
     return wallet;

test/functional/wallet_crosschain.py

@@ -36,20 +36,28 @@ class WalletCrossChain(BitcoinTestFramework):
         self.log.info("Creating wallets")
 
         node0_wallet = os.path.join(self.nodes[0].datadir, 'node0_wallet')
+        node0_wallet_backup = os.path.join(self.nodes[0].datadir, 'node0_wallet.bak')
         self.nodes[0].createwallet(node0_wallet)
+        self.nodes[0].backupwallet(node0_wallet_backup)
         self.nodes[0].unloadwallet(node0_wallet)
         node1_wallet = os.path.join(self.nodes[1].datadir, 'node1_wallet')
+        node1_wallet_backup = os.path.join(self.nodes[0].datadir, 'node1_wallet.bak')
         self.nodes[1].createwallet(node1_wallet)
+        self.nodes[1].backupwallet(node1_wallet_backup)
         self.nodes[1].unloadwallet(node1_wallet)
 
-        self.log.info("Loading wallets into nodes with a different genesis blocks")
+        self.log.info("Loading/restoring wallets into nodes with a different genesis block")
 
         if self.options.descriptors:
             assert_raises_rpc_error(-18, 'Wallet file verification failed.', self.nodes[0].loadwallet, node1_wallet)
             assert_raises_rpc_error(-18, 'Wallet file verification failed.', self.nodes[1].loadwallet, node0_wallet)
+            assert_raises_rpc_error(-18, 'Wallet file verification failed.', self.nodes[0].restorewallet, 'w', node1_wallet_backup)
+            assert_raises_rpc_error(-18, 'Wallet file verification failed.', self.nodes[1].restorewallet, 'w', node0_wallet_backup)
         else:
             assert_raises_rpc_error(-4, 'Wallet files should not be reused across chains.', self.nodes[0].loadwallet, node1_wallet)
             assert_raises_rpc_error(-4, 'Wallet files should not be reused across chains.', self.nodes[1].loadwallet, node0_wallet)
+            assert_raises_rpc_error(-4, 'Wallet files should not be reused across chains.', self.nodes[0].restorewallet, 'w', node1_wallet_backup)
+            assert_raises_rpc_error(-4, 'Wallet files should not be reused across chains.', self.nodes[1].restorewallet, 'w', node0_wallet_backup)
 
         if not self.options.descriptors:
             self.log.info("Override cross-chain wallet load protection")