wallet: setting changes are subject to race conditions

wydengyre commented at 6:25 pm on August 9, 2024: none

Is there an existing issue for this?

I have searched the existing issues

Current behaviour

Creating two wallets with load_on_startup=true simultaneously will result in only one of the wallets being added to the startup file.

Expected behaviour

Both wallets should be added.

Steps to reproduce

I don’t have an easy script to reproduce this, but the issue is quite visible in the code: https://github.com/bitcoin/bitcoin/blob/389cf32aca0be6c4b01151c92cc3be79c120f197/src/wallet/wallet.cpp#L94

The setting gets read from a backing JSON file, manipulated in memory, and then saved to the file, without any lock being taken out.

It’s likely possible to demonstrate this with a quick BASH or python script that fires up bitcoind and creates two wallets simultaneously.

Relevant log output

No response

How did you obtain Bitcoin Core

Pre-built binaries

What version of Bitcoin Core are you using?

27.1

Operating system and version

MacOS Sonoma 14.6.1

Machine specifications

No response

maflcko added the label Bug on Aug 9, 2024

maflcko added the label Wallet on Aug 9, 2024

maflcko added the label RPC/REST/ZMQ on Aug 9, 2024

maflcko added this to the milestone 28.0 on Aug 10, 2024

ismaelsadeeq commented at 11:53 am on August 15, 2024: member

@wydengyre
Thanks for reporting the issue.

It’s likely possible to demonstrate this with a quick BASH or Python script that starts bitcoind and creates two wallets simultaneously.

I tried to recreate this in the functional test by spawning threads and calling createwallet simultaneously with load_on_startup=True.

 0diff --git a/test/functional/wallet_startup.py b/test/functional/wallet_startup.py
 1index 6feb00af8e1..2fed858c098 100755
 2--- a/test/functional/wallet_startup.py
 3+++ b/test/functional/wallet_startup.py
 4@@ -6,6 +6,7 @@
 5
 6 Verify that a bitcoind node can maintain a list of wallets loading on startup
 7 """
 8+from threading import Thread
 9 from test_framework.test_framework import BitcoinTestFramework
10 from test_framework.util import (
11     assert_equal,
12@@ -56,6 +57,31 @@ class WalletStartupTest(BitcoinTestFramework):
13         self.nodes[0].loadwallet(filename='')
14         self.restart_node(0)
15         assert_equal(set(self.nodes[0].listwallets()), set(('w2', 'w3')))
16+        self.nodes[0].unloadwallet(wallet_name='w2', load_on_startup=False)
17+        self.nodes[0].unloadwallet(wallet_name='w3', load_on_startup=False)
18+        self.test_simultaneous_wallet()
19+
20+    def test_simultaneous_wallet(self):
21+        self.log.info("Testing simultaneous creation of wallets with load_on_startup=true")
22+        create_wallet_func = lambda node, wallet_name: node.createwallet(wallet_name=wallet_name, load_on_startup=True)
23+        # Create threads for wallet creation
24+        threads = []
25+        wallet_names = []
26+        for i in range(1, 10):
27+            wallet_name = f"wallet{i}"
28+            wallet_names.append(wallet_name)
29+            t = Thread(target=create_wallet_func, args=(self.nodes[0].cli, wallet_name), name=f"Thread{i}")
30+            t.start()
31+            threads.append(t)
32+        for t in threads:
33+            t.join()
34+
35+        # Restart the node to check if the wallets load on startup
36+        self.restart_node(0)
37+        # Ensure all wallets have the option
38+        for wallet in wallet_names:
39+            wallet in self.nodes[0].listwallets()
40+
41
42 if __name__ == '__main__':
43     WalletStartupTest(__file__).main()

AddWalletSetting is reading the current settings adding the new setting and then updating the settings by invoking updateRwSetting with the new settings. IIUC updateRwSettings lock a mutex which will disallow the race condition you are describing

https://github.com/bitcoin/bitcoin/blob/2f7d9aec4d049701fccfc029f44934d187467432/src/node/interfaces.cpp#L154-L164

Am I missing something here? cc @furszy

furszy commented at 3:23 pm on August 16, 2024: member

@ismaelsadeeq, the race is one level above. AddWalletSetting obtains a copy of the ‘wallet’ settings json array, modifies it without any lock, and then calls updateRwSetting, which entirely replaces the existing value. If two threads access AddWalletSetting simultaneously, they both obtain the same ‘wallet’ settings array, append their new wallet, and store it. Only the data from the thread that executes the write operation last survives. The same issue occurs on RemoveWalletSetting. The guard should likely be implemented at a higher level, such as in UpdateWalletSetting.

The race can be replicated running the following unit test:

 0BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurretly, TestingSetup)
 1{
 2    interfaces::Chain& chain = *m_node.chain;
 3    std::vector<std::thread> threads;
 4    const int NUM_WALLETS = 5;
 5    for (int i=0; i<NUM_WALLETS; i++) {
 6        threads.emplace_back([&] {
 7            BOOST_CHECK_MESSAGE(AddWalletSetting(chain, strprintf("wallet_%d", i)), strprintf("write wallet_%d failed", i));
 8        });
 9    }
10    for (auto& t : threads) t.join();
11
12    auto wallets = chain.getRwSetting("wallet");
13    BOOST_CHECK_EQUAL(wallets.getValues().size(), NUM_WALLETS);
14}

ismaelsadeeq commented at 8:48 am on August 19, 2024: member

Yes, I encountered the race condition after running the test with the diff you provided.

I believe I fixed the race condition by locking the wallet context mutex in both AddWalletSetting and RemoveWalletSetting. However, the test will still fail because the loop variable i is captured by reference. This means that all threads share the same reference to i, leading to situations where multiple threads might update the same wallet name. As a result, the size of the settings can become non-deterministic.

I updated the test to instead capture by value.

  0diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
  1index 44ffddb1687..8cdf2276ff5 100644
  2--- a/src/wallet/test/wallet_tests.cpp
  3+++ b/src/wallet/test/wallet_tests.cpp
  4@@ -329,6 +329,23 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
  5     }
  6 }
  7 
  8+BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurretly, TestingSetup)
  9+{
 10+    WalletContext context;
 11+    context.chain = m_node.chain.get();
 12+    std::vector<std::thread> threads;
 13+    const int NUM_WALLETS = 5;
 14+    for (int i=0; i<NUM_WALLETS; i++) {
 15+        threads.emplace_back([i, &context] {
 16+            BOOST_CHECK_MESSAGE(AddWalletSetting(context, strprintf("wallet_%d", i)), strprintf("write wallet_%d failed", i));
 17+        });
 18+    }
 19+    for (auto& t : threads) t.join();
 20+
 21+    auto wallets = context.chain->getRwSetting("wallet");
 22+    BOOST_CHECK_EQUAL(wallets.getValues().size(), NUM_WALLETS);
 23+}
 24+
 25 // Check that GetImmatureCredit() returns a newly calculated value instead of
 26 // the cached value after a MarkDirty() call.
 27 //
 28diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
 29index 5584b43520a..b6e714decb0 100644
 30--- a/src/wallet/wallet.cpp
 31+++ b/src/wallet/wallet.cpp
 32@@ -91,38 +91,40 @@ using util::ToString;
 33 
 34 namespace wallet {
 35 
 36-bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name)
 37+bool AddWalletSetting(WalletContext& context, const std::string& wallet_name)
 38 {
 39-    common::SettingsValue setting_value = chain.getRwSetting("wallet");
 40+    LOCK(context.wallets_mutex);
 41+    common::SettingsValue setting_value = context.chain->getRwSetting("wallet");
 42     if (!setting_value.isArray()) setting_value.setArray();
 43     for (const common::SettingsValue& value : setting_value.getValues()) {
 44         if (value.isStr() && value.get_str() == wallet_name) return true;
 45     }
 46     setting_value.push_back(wallet_name);
 47-    return chain.updateRwSetting("wallet", setting_value);
 48+    return context.chain->updateRwSetting("wallet", setting_value);
 49 }
 50 
 51-bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_name)
 52+bool RemoveWalletSetting(WalletContext& context, const std::string& wallet_name)
 53 {
 54-    common::SettingsValue setting_value = chain.getRwSetting("wallet");
 55+    LOCK(context.wallets_mutex);
 56+    common::SettingsValue setting_value = context.chain->getRwSetting("wallet");
 57     if (!setting_value.isArray()) return true;
 58     common::SettingsValue new_value(common::SettingsValue::VARR);
 59     for (const common::SettingsValue& value : setting_value.getValues()) {
 60         if (!value.isStr() || value.get_str() != wallet_name) new_value.push_back(value);
 61     }
 62     if (new_value.size() == setting_value.size()) return true;
 63-    return chain.updateRwSetting("wallet", new_value);
 64+    return context.chain->updateRwSetting("wallet", new_value);
 65 }
 66 
 67-static void UpdateWalletSetting(interfaces::Chain& chain,
 68+static void UpdateWalletSetting(WalletContext& context,
 69                                 const std::string& wallet_name,
 70                                 std::optional<bool> load_on_startup,
 71                                 std::vector<bilingual_str>& warnings)
 72 {
 73     if (!load_on_startup) return;
 74-    if (load_on_startup.value() && !AddWalletSetting(chain, wallet_name)) {
 75+    if (load_on_startup.value() && !AddWalletSetting(context, wallet_name)) {
 76         warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup."));
 77-    } else if (!load_on_startup.value() && !RemoveWalletSetting(chain, wallet_name)) {
 78+    } else if (!load_on_startup.value() && !RemoveWalletSetting(context, wallet_name)) {
 79         warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may still be loaded next node startup."));
 80     }
 81 }
 82@@ -157,7 +159,6 @@ bool RemoveWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet
 83 {
 84     assert(wallet);
 85 
 86-    interfaces::Chain& chain = wallet->chain();
 87     std::string name = wallet->GetName();
 88 
 89     // Unregister with the validation interface which also drops shared pointers.
 90@@ -172,7 +173,7 @@ bool RemoveWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet
 91     wallet->NotifyUnload();
 92 
 93     // Write the wallet setting
 94-    UpdateWalletSetting(chain, name, load_on_start, warnings);
 95+    UpdateWalletSetting(context, name, load_on_start, warnings);
 96 
 97     return true;
 98 }
 99@@ -293,7 +294,7 @@ std::shared_ptr<CWallet> LoadWalletInternal(WalletContext& context, const std::s
100         wallet->postInitProcess();
101 
102         // Write the wallet setting
103-        UpdateWalletSetting(*context.chain, name, load_on_start, warnings);
104+        UpdateWalletSetting(context, name, load_on_start, warnings);
105 
106         return wallet;
107     } catch (const std::runtime_error& e) {
108@@ -474,7 +475,7 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
109     wallet->postInitProcess();
110 
111     // Write the wallet settings
112-    UpdateWalletSetting(*context.chain, name, load_on_start, warnings);
113+    UpdateWalletSetting(context, name, load_on_start, warnings);
114 
115     // Legacy wallets are being deprecated, warn if a newly created wallet is legacy
116     if (!(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) {
117@@ -4324,7 +4325,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
118             }
119 
120             // Add the wallet to settings
121-            UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/true, warnings);
122+            UpdateWalletSetting(context, wallet_name, /*load_on_startup=*/true, warnings);
123         }
124         if (data->solvable_descs.size() > 0) {
125             wallet.WalletLogPrintf("Making a new watchonly wallet containing the unwatched solvable scripts\n");
126@@ -4361,7 +4362,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
127             }
128 
129             // Add the wallet to settings
130-            UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/true, warnings);
131+            UpdateWalletSetting(context, wallet_name, /*load_on_startup=*/true, warnings);
132         }
133     }
134 
135diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
136index 3ea1cf48b22..ad2e2817c46 100644
137--- a/src/wallet/wallet.h
138+++ b/src/wallet/wallet.h
139@@ -1112,10 +1112,10 @@ public:
140 };
141 
142 //! Add wallet name to persistent configuration so it will be loaded on startup.
143-bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name);
144+bool AddWalletSetting(WalletContext& context, const std::string& wallet_name);
145 
146 //! Remove wallet name from persistent configuration so it will not be loaded on startup.
147-bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_name);
148+bool RemoveWalletSetting(WalletContext& context, const std::string& wallet_name);
149 
150 struct MigrationResult {
151     std::string wallet_name;

I believe this is the right fix and that locking the wallet mutex there is safe. If this does not introduce any issues, I will create a PR and include @furszy as a co-author since he provided the test.

achow101 closed this on Aug 27, 2024

hebasto referenced this in commit 78567b052d on Aug 27, 2024

wallet: setting changes are subject to race conditions #30620