Merge bitcoin/bitcoin#26594: wallet: Avoid a segfault in migratewallet failure cleanup

5e65a21 wallet: Explicitly say migratewallet on encrypted wallets is unsupported (Andrew Chow)
88afc73 tests: Test for migrating encrypted wallets (Andrew Chow)
86ef7b3 wallet: Avoid null pointer deref when cleaning up migratewallet (Andrew Chow)

Pull request description:

  When `migratewallet` fails, we do an automatic cleanup in order to reset everything so that the user does not experience any interruptions. However, this apparently has a segfault in it, caused by the the pointers to the watchonly and solvables wallets being nullptr. If those wallets are not created (either not needed, or failed early on), we will accidentally attempt to dereference these nullptrs, which causes a segfault.

  This failure can be easily reached by trying to migrate an encrypted wallet. Currently, we can't migrate encrypted wallets because of how we unload wallets before migrating, and therefore forget the encryption key if the wallet was unlocked. So any encrypted wallets will fail, entering the cleanup, and because watchonly and solvables wallets don't exist yet, the segfault is reached.

  This PR fixes this by not putting those nullptrs in a place that we will end up dereferencing them later. It also adds a test that uses the encrypted wallet issue.

ACKs for top commit:
  S3RK:
    reACK 5e65a21
  stickies-v:
    ACK [5e65a21](bitcoin/bitcoin@5e65a21)
  furszy:
    diff ACK 5e65a21

Tree-SHA512: f75643797220d4232ad3ab8cb4b46d0f3667f00486e910ca748c9b6d174d446968f1ec4dd7f907da1be9566088849da7edcd8cd8f12de671c3241b513deb8e80

Author: fanquake

src/wallet/rpc/wallet.cpp

@@ -730,7 +730,9 @@ static RPCHelpMan migratewallet()
             std::shared_ptr<CWallet> wallet = GetWalletForJSONRPCRequest(request);
             if (!wallet) return NullUniValue;
 
-            EnsureWalletIsUnlocked(*wallet);
+            if (wallet->IsCrypted()) {
+                throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: migratewallet on encrypted wallets is currently unsupported.");
+            }
 
             WalletContext& context = EnsureWalletContext(request.context);
 

src/wallet/wallet.cpp

@@ -4182,8 +4182,8 @@ util::Result<MigrationResult> MigrateLegacyToDescriptor(std::shared_ptr<CWallet>
 
         // Make list of wallets to cleanup
         std::vector<std::shared_ptr<CWallet>> created_wallets;
-        created_wallets.push_back(std::move(res.watchonly_wallet));
-        created_wallets.push_back(std::move(res.solvables_wallet));
+        if (res.watchonly_wallet) created_wallets.push_back(std::move(res.watchonly_wallet));
+        if (res.solvables_wallet) created_wallets.push_back(std::move(res.solvables_wallet));
 
         // Get the directories to remove after unloading
         for (std::shared_ptr<CWallet>& w : created_wallets) {

test/functional/wallet_migration.py

@@ -396,6 +396,15 @@ def test_pk_coinbases(self):
 
         assert_equal(bals, wallet.getbalances())
 
+    def test_encrypted(self):
+        self.log.info("Test migration of an encrypted wallet")
+        wallet = self.create_legacy_wallet("encrypted")
+
+        wallet.encryptwallet("pass")
+
+        assert_raises_rpc_error(-15, "Error: migratewallet on encrypted wallets is currently unsupported.", wallet.migratewallet)
+        # TODO: Fix migratewallet so that we can actually migrate encrypted wallets
+
     def run_test(self):
         self.generate(self.nodes[0], 101)
 
@@ -405,6 +414,7 @@ def run_test(self):
         self.test_other_watchonly()
         self.test_no_privkeys()
         self.test_pk_coinbases()
+        self.test_encrypted()
 
 if __name__ == '__main__':
     WalletMigrationTest().main()