Merge bitcoin/bitcoin#26347: wallet: ensure the wallet is unlocked when needed for rescanning

6a5b348f2e526f048d0b448b01f6c4ab608569af test: test rescanning encrypted wallets (ishaanam)
493b813e171a389a8b6750b4f2e42e8363a0267e wallet: ensure that the passphrase is not deleted from memory when being used to rescan (ishaanam)
66a86ebabb26a055ca92af846bfa39dbd2f9f722 wallet: keep track of when the passphrase is needed when rescanning (ishaanam)

Pull request description:

  Wallet passphrases are needed to top up the keypool of encrypted wallets
  during a rescan. The following RPCs need the passphrase when rescanning:
      - `importdescriptors`
      - `rescanblockchain`

  The following RPCs use the information about whether or not the
  passphrase is being used to ensure that full rescans are able to
  take place (meaning the following RPCs should not be able to run
  if a rescan requiring the wallet to be unlocked  is taking place):
      - `walletlock`
      - `encryptwallet`
      - `walletpassphrasechange`

  `m_relock_mutex` is also introduced so that the passphrase is not
  deleted from memory when the timeout provided in
  `walletpassphrase` is up and the wallet is still rescanning.
  Fixes #25702, #11249

  Thanks to achow101 for coming up with the idea of using a new mutex to solve this issue and for answering related questions.

ACKs for top commit:
  achow101:
    ACK 6a5b348f2e526f048d0b448b01f6c4ab608569af
  hernanmarino:
    ACK 6a5b348f2e526f048d0b448b01f6c4ab608569af
  furszy:
    Tested ACK 6a5b348f

Tree-SHA512: 0b6db692714f6f94594fa47249f5ee24f85713bfa70ac295a7e84b9ca6c07dda65df7b47781a2dc73e5b603a8725343a2f864428ae20d3e126c5b4802abc4ab5

5 files changed, 39 insertions, 13 deletions

diff --git a/src/wallet/rpc/backup.cpp b/src/wallet/rpc/backup.cpp
index e3701fe312..744537cfbd 100644
--- a/src/wallet/rpc/backup.cpp
+++ b/src/wallet/rpc/backup.cpp
@@ -1650,10 +1650,14 @@ RPCHelpMan importdescriptors()
     }
 
     WalletRescanReserver reserver(*pwallet);
-    if (!reserver.reserve()) {
+    if (!reserver.reserve(/*with_passphrase=*/true)) {
         throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
     }
 
+    // Ensure that the wallet is not locked for the remainder of this RPC, as
+    // the passphrase is used to top up the keypool.
+    LOCK(pwallet->m_relock_mutex);
+
     const UniValue& requests = main_request.params[0];
     const int64_t minimum_timestamp = 1;
     int64_t now = 0;
diff --git a/src/wallet/rpc/encrypt.cpp b/src/wallet/rpc/encrypt.cpp
index fcf25e01d6..38960bda7b 100644
--- a/src/wallet/rpc/encrypt.cpp
+++ b/src/wallet/rpc/encrypt.cpp
@@ -90,7 +90,7 @@ RPCHelpMan walletpassphrase()
     std::weak_ptr<CWallet> weak_wallet = wallet;
     pwallet->chain().rpcRunLater(strprintf("lockwallet(%s)", pwallet->GetName()), [weak_wallet, relock_time] {
         if (auto shared_wallet = weak_wallet.lock()) {
-            LOCK(shared_wallet->cs_wallet);
+            LOCK2(shared_wallet->m_relock_mutex, shared_wallet->cs_wallet);
             // Skip if this is not the most recent rpcRunLater callback.
             if (shared_wallet->nRelockTime != relock_time) return;
             shared_wallet->Lock();
@@ -122,12 +122,16 @@ RPCHelpMan walletpassphrasechange()
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
     if (!pwallet) return UniValue::VNULL;
 
-    LOCK(pwallet->cs_wallet);
-
     if (!pwallet->IsCrypted()) {
         throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an unencrypted wallet, but walletpassphrasechange was called.");
     }
 
+    if (pwallet->IsScanningWithPassphrase()) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "Error: the wallet is currently being used to rescan the blockchain for related transactions. Please call `abortrescan` before changing the passphrase.");
+    }
+
+    LOCK2(pwallet->m_relock_mutex, pwallet->cs_wallet);
+
     // TODO: get rid of these .c_str() calls by implementing SecureString::operator=(std::string)
     // Alternately, find a way to make request.params[0] mlock()'d to begin with.
     SecureString strOldWalletPass;
@@ -175,12 +179,16 @@ RPCHelpMan walletlock()
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
     if (!pwallet) return UniValue::VNULL;
 
-    LOCK(pwallet->cs_wallet);
-
     if (!pwallet->IsCrypted()) {
         throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an unencrypted wallet, but walletlock was called.");
     }
 
+    if (pwallet->IsScanningWithPassphrase()) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "Error: the wallet is currently being used to rescan the blockchain for related transactions. Please call `abortrescan` before locking the wallet.");
+    }
+
+    LOCK2(pwallet->m_relock_mutex, pwallet->cs_wallet);
+
     pwallet->Lock();
     pwallet->nRelockTime = 0;
 
@@ -219,8 +227,6 @@ RPCHelpMan encryptwallet()
     std::shared_ptr<CWallet> const pwallet = GetWalletForJSONRPCRequest(request);
     if (!pwallet) return UniValue::VNULL;
 
-    LOCK(pwallet->cs_wallet);
-
     if (pwallet->IsWalletFlagSet(WALLET_FLAG_DISABLE_PRIVATE_KEYS)) {
         throw JSONRPCError(RPC_WALLET_ENCRYPTION_FAILED, "Error: wallet does not contain private keys, nothing to encrypt.");
     }
@@ -229,6 +235,12 @@ RPCHelpMan encryptwallet()
         throw JSONRPCError(RPC_WALLET_WRONG_ENC_STATE, "Error: running with an encrypted wallet, but encryptwallet was called.");
     }
 
+    if (pwallet->IsScanningWithPassphrase()) {
+        throw JSONRPCError(RPC_WALLET_ERROR, "Error: the wallet is currently being used to rescan the blockchain for related transactions. Please call `abortrescan` before encrypting the wallet.");
+    }
+
+    LOCK2(pwallet->m_relock_mutex, pwallet->cs_wallet);
+
     // TODO: get rid of this .c_str() by implementing SecureString::operator=(std::string)
     // Alternately, find a way to make request.params[0] mlock()'d to begin with.
     SecureString strWalletPass;
diff --git a/src/wallet/rpc/transactions.cpp b/src/wallet/rpc/transactions.cpp
index e590aa1f08..3bfe296d90 100644
--- a/src/wallet/rpc/transactions.cpp
+++ b/src/wallet/rpc/transactions.cpp
@@ -872,15 +872,18 @@ RPCHelpMan rescanblockchain()
     wallet.BlockUntilSyncedToCurrentChain();
 
     WalletRescanReserver reserver(*pwallet);
-    if (!reserver.reserve()) {
+    if (!reserver.reserve(/*with_passphrase=*/true)) {
         throw JSONRPCError(RPC_WALLET_ERROR, "Wallet is currently rescanning. Abort existing rescan or wait.");
     }
 
     int start_height = 0;
     std::optional<int> stop_height;
     uint256 start_block;
+
+    LOCK(pwallet->m_relock_mutex);
     {
         LOCK(pwallet->cs_wallet);
+        EnsureWalletIsUnlocked(*pwallet);
         int tip_height = pwallet->GetLastBlockHeight();
 
         if (!request.params[0].isNull()) {
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index eed40f9462..f2c0fdcb3d 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -552,7 +552,7 @@ bool CWallet::ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase,
     bool fWasLocked = IsLocked();
 
     {
-        LOCK(cs_wallet);
+        LOCK2(m_relock_mutex, cs_wallet);
         Lock();
 
         CCrypter crypter;
@@ -787,7 +787,7 @@ bool CWallet::EncryptWallet(const SecureString& strWalletPassphrase)
         return false;
 
     {
-        LOCK(cs_wallet);
+        LOCK2(m_relock_mutex, cs_wallet);
         mapMasterKeys[++nMasterKeyMaxID] = kMasterKey;
         WalletBatch* encrypted_batch = new WalletBatch(GetDatabase());
         if (!encrypted_batch->TxnBegin()) {
@@ -3412,7 +3412,7 @@ bool CWallet::Lock()
         return false;
 
     {
-        LOCK(cs_wallet);
+        LOCK2(m_relock_mutex, cs_wallet);
         if (!vMasterKey.empty()) {
             memory_cleanse(vMasterKey.data(), vMasterKey.size() * sizeof(decltype(vMasterKey)::value_type));
             vMasterKey.clear();
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 6ec95220d6..5dd694e114 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -243,6 +243,7 @@ private:
     std::atomic<bool> fAbortRescan{false};
     std::atomic<bool> fScanningWallet{false}; // controlled by WalletRescanReserver
     std::atomic<bool> m_attaching_chain{false};
+    std::atomic<bool> m_scanning_with_passphrase{false};
     std::atomic<int64_t> m_scanning_start{0};
     std::atomic<double> m_scanning_progress{0};
     friend class WalletRescanReserver;
@@ -463,6 +464,7 @@ public:
     void AbortRescan() { fAbortRescan = true; }
     bool IsAbortingRescan() const { return fAbortRescan; }
     bool IsScanning() const { return fScanningWallet; }
+    bool IsScanningWithPassphrase() const { return m_scanning_with_passphrase; }
     int64_t ScanningDuration() const { return fScanningWallet ? GetTimeMillis() - m_scanning_start : 0; }
     double ScanningProgress() const { return fScanningWallet ? (double) m_scanning_progress : 0; }
 
@@ -482,6 +484,9 @@ public:
 
     // Used to prevent concurrent calls to walletpassphrase RPC.
     Mutex m_unlock_mutex;
+    // Used to prevent deleting the passphrase from memory when it is still in use.
+    RecursiveMutex m_relock_mutex;
+
     bool Unlock(const SecureString& strWalletPassphrase, bool accept_no_keys = false);
     bool ChangeWalletPassphrase(const SecureString& strOldWalletPassphrase, const SecureString& strNewWalletPassphrase);
     bool EncryptWallet(const SecureString& strWalletPassphrase);
@@ -962,12 +967,13 @@ private:
 public:
     explicit WalletRescanReserver(CWallet& w) : m_wallet(w) {}
 
-    bool reserve()
+    bool reserve(bool with_passphrase = false)
     {
         assert(!m_could_reserve);
         if (m_wallet.fScanningWallet.exchange(true)) {
             return false;
         }
+        m_wallet.m_scanning_with_passphrase.exchange(with_passphrase);
         m_wallet.m_scanning_start = GetTimeMillis();
         m_wallet.m_scanning_progress = 0;
         m_could_reserve = true;
@@ -987,6 +993,7 @@ public:
     {
         if (m_could_reserve) {
             m_wallet.fScanningWallet = false;
+            m_wallet.m_scanning_with_passphrase = false;
         }
     }
 };