diff options
author | MarcoFalke <falke.marco@gmail.com> | 2020-06-29 11:14:17 -0400 |
---|---|---|
committer | MarcoFalke <falke.marco@gmail.com> | 2020-06-29 11:14:26 -0400 |
commit | 5c3c7cc50c5f70bbd2946c65c6fe0d50b380a076 (patch) | |
tree | 0903e4d30e5a33853bc13b0065e80f9ee3031400 | |
parent | dbadf746e26840b68cb3b01a690aa46256ae2191 (diff) | |
parent | 9b009fae6e2eb0ab2ee7ce7882c3556a9ac363a7 (diff) | |
download | bitcoin-5c3c7cc50c5f70bbd2946c65c6fe0d50b380a076.tar.xz |
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
-rw-r--r-- | src/wallet/wallet.cpp | 20 | ||||
-rwxr-xr-x | test/functional/wallet_multiwallet.py | 29 |
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) |