Merge #19300: wallet: Handle concurrent wallet loading

9b009fae6e2eb0ab2ee7ce7882c3556a9ac363a7 qa: Test concurrent wallet loading (João Barbosa)
b9971ae5853c1d62e09d976a8705f4f731290d85 wallet: Handle concurrent wallet loading (João Barbosa)

Pull request description:

  This PR handles concurrent wallet loading.

  This can be tested by running in parallel the following script a couple of times:
  ```sh
  for i in {1..10}
  do
    src/bitcoin-cli -regtest loadwallet foo
    src/bitcoin-cli -regtest unloadwallet foo
  done
  ```

  Eventually the error occurs:
  ```
  error code: -4
  error message:
  Wallet already being loading.
  ```

  For reference, loading and already loaded wallet gives:
  ```
  error code: -4
  error message:
  Wallet file verification failed. Error loading wallet w1. Duplicate -wallet filename specified.
  ```

  Fixes #19232.

ACKs for top commit:
  MarcoFalke:
    Concept ACK 9b009fae6e2eb0ab2ee7ce7882c3556a9ac363a7 I have not reviewed the code
  hebasto:
    ACK 9b009fae6e2eb0ab2ee7ce7882c3556a9ac363a7, tested on Linux Mint 20 (x86_64):
  ryanofsky:
    Code review good-but-not-ideal ACK 9b009fae6e2eb0ab2ee7ce7882c3556a9ac363a7

Tree-SHA512: 0ccd77b03c0926e4c4e51efb31e193b93cb4b9ffe8bac6bb018f7344c55dfd939b873b8cf5e657dca73e6202eb75aa672de2acb787cc133184b0b3b51e47b972

2 files changed, 47 insertions, 2 deletions

diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 57eec9baf9..fbb1a58ea1 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -99,9 +99,11 @@ std::unique_ptr<interfaces::Handler> HandleLoadWallet(LoadWalletFn load_wallet)
     return interfaces::MakeHandler([it] { LOCK(cs_wallets); g_load_wallet_fns.erase(it); });
 }
 
+static Mutex g_loading_wallet_mutex;
 static Mutex g_wallet_release_mutex;
 static std::condition_variable g_wallet_release_cv;
-static std::set<std::string> g_unloading_wallet_set;
+static std::set<std::string> g_loading_wallet_set GUARDED_BY(g_loading_wallet_mutex);
+static std::set<std::string> g_unloading_wallet_set GUARDED_BY(g_wallet_release_mutex);
 
 // Custom deleter for shared_ptr<CWallet>.
 static void ReleaseWallet(CWallet* wallet)
@@ -145,7 +147,8 @@ void UnloadWallet(std::shared_ptr<CWallet>&& wallet)
     }
 }
 
-std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector<bilingual_str>& warnings)
+namespace {
+std::shared_ptr<CWallet> LoadWalletInternal(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector<bilingual_str>& warnings)
 {
     try {
         if (!CWallet::Verify(chain, location, error, warnings)) {
@@ -166,6 +169,19 @@ std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocati
         return nullptr;
     }
 }
+} // namespace
+
+std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const WalletLocation& location, bilingual_str& error, std::vector<bilingual_str>& warnings)
+{
+    auto result = WITH_LOCK(g_loading_wallet_mutex, return g_loading_wallet_set.insert(location.GetName()));
+    if (!result.second) {
+        error = Untranslated("Wallet already being loading.");
+        return nullptr;
+    }
+    auto wallet = LoadWalletInternal(chain, location, error, warnings);
+    WITH_LOCK(g_loading_wallet_mutex, g_loading_wallet_set.erase(result.first));
+    return wallet;
+}
 
 std::shared_ptr<CWallet> LoadWallet(interfaces::Chain& chain, const std::string& name, bilingual_str& error, std::vector<bilingual_str>& warnings)
 {
diff --git a/test/functional/wallet_multiwallet.py b/test/functional/wallet_multiwallet.py
index ff9ff34185..88beef1034 100755
--- a/test/functional/wallet_multiwallet.py
+++ b/test/functional/wallet_multiwallet.py
@@ -7,19 +7,36 @@
 Verify that a bitcoind node can load multiple wallet files
 """
 from decimal import Decimal
+from threading import Thread
 import os
 import shutil
 import time
 
+from test_framework.authproxy import JSONRPCException
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.test_node import ErrorMatch
 from test_framework.util import (
     assert_equal,
     assert_raises_rpc_error,
+    get_rpc_proxy,
 )
 
 FEATURE_LATEST = 169900
 
+got_loading_error = False
+def test_load_unload(node, name):
+    global got_loading_error
+    for i in range(10):
+        if got_loading_error:
+            return
+        try:
+            node.loadwallet(name)
+            node.unloadwallet(name)
+        except JSONRPCException as e:
+            if e.error['code'] == -4 and 'Wallet already being loading' in e.error['message']:
+                got_loading_error = True
+                return
+
 
 class MultiWalletTest(BitcoinTestFramework):
     def set_test_params(self):
@@ -212,6 +229,18 @@ class MultiWalletTest(BitcoinTestFramework):
         w2 = node.get_wallet_rpc(wallet_names[1])
         w2.getwalletinfo()
 
+        self.log.info("Concurrent wallet loading")
+        threads = []
+        for _ in range(3):
+            n = node.cli if self.options.usecli else get_rpc_proxy(node.url, 1, timeout=600, coveragedir=node.coverage_dir)
+            t = Thread(target=test_load_unload, args=(n, wallet_names[2], ))
+            t.start()
+            threads.append(t)
+        for t in threads:
+            t.join()
+        global got_loading_error
+        assert_equal(got_loading_error, True)
+
         self.log.info("Load remaining wallets")
         for wallet_name in wallet_names[2:]:
             loadwallet_name = self.nodes[0].loadwallet(wallet_name)