Repository: bitcoin/bitcoin (language: cpp)

Issue:
wallet: setting changes are subject to race conditions
### Is there an existing issue for this?

- [X] 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_


Discussion / hints:
@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`.

<details>  
<summary>See the diff</summary>

```diff
diff --git a/test/functional/wallet_startup.py b/test/functional/wallet_startup.py
index 6feb00af8e1..2fed858c098 100755
--- a/test/functional/wallet_startup.py
+++ b/test/functional/wallet_startup.py
@@ -6,6 +6,7 @@

 Verify that a bitcoind node can maintain a list of wallets loading on startup
 """
+from threading import Thread
 from test_framework.test_framework import BitcoinTestFramework
 from test_framework.util import (
     assert_equal,
@@ -56,6 +57,31 @@ class WalletStartupTest(BitcoinTestFramework):
         self.nodes[0].loadwallet(filename='')
         self.restart_node(0)
         assert_equal(set(self.nodes[0].listwallets()), set(('w2', 'w3')))
+        self.nodes[0].unloadwallet(wallet_name='w2', load_on_startup=False)
+        self.nodes[0].unloadwallet(wallet_name='w3', load_on_startup=False)
+        self.test_simultaneous_wallet()
+
+    def test_simultaneous_wallet(self):
+        self.log.info("Testing simultaneous creation of wallets with load_on_startup=true")
+        create_wallet_func = lambda node, wallet_name: node.createwallet(wallet_name=wallet_name, load_on_startup=True)
+        # Create threads for wallet creation
+        threads = []
+        wallet_names = []
+        for i in range(1, 10):
+            wallet_name = f"wallet{i}"
+            wallet_names.append(wallet_name)
+            t = Thread(target=create_wallet_func, args=(self.nodes[0].cli, wallet_name), name=f"Thread{i}")
+            t.start()
+            threads.append(t)
+        for t in threads:
+            t.join()
+
+        # Restart the node to check if the wallets load on startup
+        self.restart_node(0)
+        # Ensure all wallets have the option
+        for wallet in wallet_names:
+            wallet in self.nodes[0].listwallets()
+

 if __name__ == '__main__':
     WalletStartupTest(__file__).main()

```
</details>
After running the script the race condition you described was not happening.

---
`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 
@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:
```
BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurretly, TestingSetup)
{
    interfaces::Chain& chain = *m_node.chain;
    std::vector<std::thread> threads;
    const int NUM_WALLETS = 5;
    for (int i=0; i<NUM_WALLETS; i++) {
        threads.emplace_back([&] {
            BOOST_CHECK_MESSAGE(AddWalletSetting(chain, strprintf("wallet_%d", i)), strprintf("write wallet_%d failed", i));
        });
    }
    for (auto& t : threads) t.join();

    auto wallets = chain.getRwSetting("wallet");
    BOOST_CHECK_EQUAL(wallets.getValues().size(), NUM_WALLETS);
}
```

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.

```diff
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 44ffddb1687..8cdf2276ff5 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -329,6 +329,23 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
     }
 }
 
+BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurretly, TestingSetup)
+{
+    WalletContext context;
+    context.chain = m_node.chain.get();
+    std::vector<std::thread> threads;
+    const int NUM_WALLETS = 5;
+    for (int i=0; i<NUM_WALLETS; i++) {
+        threads.emplace_back([i, &context] {
+            BOOST_CHECK_MESSAGE(AddWalletSetting(context, strprintf("wallet_%d", i)), strprintf("write wallet_%d failed", i));
+        });
+    }
+    for (auto& t : threads) t.join();
+
+    auto wallets = context.chain->getRwSetting("wallet");
+    BOOST_CHECK_EQUAL(wallets.getValues().size(), NUM_WALLETS);
+}
+
 // Check that GetImmatureCredit() returns a newly calculated value instead of
 // the cached value after a MarkDirty() call.
 //
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 5584b43520a..b6e714decb0 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -91,38 +91,40 @@ using util::ToString;
 
 namespace wallet {
 
-bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name)
+bool AddWalletSetting(WalletContext& context, const std::string& wallet_name)
 {
-    common::SettingsValue setting_value = chain.getRwSetting("wallet");
+    LOCK(context.wallets_mutex);
+    common::SettingsValue setting_value = context.chain->getRwSetting("wallet");
     if (!setting_value.isArray()) setting_value.setArray();
     for (const common::SettingsValue& value : setting_value.getValues()) {
         if (value.isStr() && value.get_str() == wallet_name) return true;
     }
     setting_value.push_back(wallet_name);
-    return chain.updateRwSetting("wallet", setting_value);
+    return context.chain->updateRwSetting("wallet", setting_value);
 }
 
-bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_name)
+bool RemoveWalletSetting(WalletContext& context, const std::string& wallet_name)
 {
-    common::SettingsValue setting_value = chain.getRwSetting("wallet");
+    LOCK(context.wallets_mutex);
+    common::SettingsValue setting_value = context.chain->getRwSetting("wallet");
     if (!setting_value.isArray()) return true;
     common::SettingsValue new_value(common::SettingsValue::VARR);
     for (const common::SettingsValue& value : setting_value.getValues()) {
         if (!value.isStr() || value.get_str() != wallet_name) new_value.push_back(value);
     }
     if (new_value.size() == setting_value.size()) return true;
-    return chain.updateRwSetting("wallet", new_value);
+    return context.chain->updateRwSetting("wallet", new_value);
 }
 
-static void UpdateWalletSetting(interfaces::Chain& chain,
+static void UpdateWalletSetting(WalletContext& context,
                                 const std::string& wallet_name,
                                 std::optional<bool> load_on_startup,
                                 std::vector<bilingual_str>& warnings)
 {
     if (!load_on_startup) return;
-    if (load_on_startup.value() && !AddWalletSetting(chain, wallet_name)) {
+    if (load_on_startup.value() && !AddWalletSetting(context, wallet_name)) {
         warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may not be loaded next node startup."));
-    } else if (!load_on_startup.value() && !RemoveWalletSetting(chain, wallet_name)) {
+    } else if (!load_on_startup.value() && !RemoveWalletSetting(context, wallet_name)) {
         warnings.emplace_back(Untranslated("Wallet load on startup setting could not be updated, so wallet may still be loaded next node startup."));
     }
 }
@@ -157,7 +159,6 @@ bool RemoveWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet
 {
     assert(wallet);
 
-    interfaces::Chain& chain = wallet->chain();
     std::string name = wallet->GetName();
 
     // Unregister with the validation interface which also drops shared pointers.
@@ -172,7 +173,7 @@ bool RemoveWallet(WalletContext& context, const std::shared_ptr<CWallet>& wallet
     wallet->NotifyUnload();
 
     // Write the wallet setting
-    UpdateWalletSetting(chain, name, load_on_start, warnings);
+    UpdateWalletSetting(context, name, load_on_start, warnings);
 
     return true;
 }
@@ -293,7 +294,7 @@ std::shared_ptr<CWallet> LoadWalletInternal(WalletContext& context, const std::s
         wallet->postInitProcess();
 
         // Write the wallet setting
-        UpdateWalletSetting(*context.chain, name, load_on_start, warnings);
+        UpdateWalletSetting(context, name, load_on_start, warnings);
 
         return wallet;
     } catch (const std::runtime_error& e) {
@@ -474,7 +475,7 @@ std::shared_ptr<CWallet> CreateWallet(WalletContext& context, const std::string&
     wallet->postInitProcess();
 
     // Write the wallet settings
-    UpdateWalletSetting(*context.chain, name, load_on_start, warnings);
+    UpdateWalletSetting(context, name, load_on_start, warnings);
 
     // Legacy wallets are being deprecated, warn if a newly created wallet is legacy
     if (!(wallet_creation_flags & WALLET_FLAG_DESCRIPTORS)) {
@@ -4324,7 +4325,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
             }
 
             // Add the wallet to settings
-            UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/true, warnings);
+            UpdateWalletSetting(context, wallet_name, /*load_on_startup=*/true, warnings);
         }
         if (data->solvable_descs.size() > 0) {
             wallet.WalletLogPrintf("Making a new watchonly wallet containing the unwatched solvable scripts\n");
@@ -4361,7 +4362,7 @@ bool DoMigration(CWallet& wallet, WalletContext& context, bilingual_str& error,
             }
 
             // Add the wallet to settings
-            UpdateWalletSetting(*context.chain, wallet_name, /*load_on_startup=*/true, warnings);
+            UpdateWalletSetting(context, wallet_name, /*load_on_startup=*/true, warnings);
         }
     }
 
diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
index 3ea1cf48b22..ad2e2817c46 100644
--- a/src/wallet/wallet.h
+++ b/src/wallet/wallet.h
@@ -1112,10 +1112,10 @@ public:
 };
 
 //! Add wallet name to persistent configuration so it will be loaded on startup.
-bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name);
+bool AddWalletSetting(WalletContext& context, const std::string& wallet_name);
 
 //! Remove wallet name from persistent configuration so it will not be loaded on startup.
-bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_name);
+bool RemoveWalletSetting(WalletContext& context, const std::string& wallet_name);
 
 struct MigrationResult {
     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.

Fix patch:
diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
index af45f81f95e45..be596b17657cb 100644
--- a/src/interfaces/chain.h
+++ b/src/interfaces/chain.h
@@ -96,6 +96,17 @@ struct BlockInfo {
     BlockInfo(const uint256& hash LIFETIMEBOUND) : hash(hash) {}
 };
 
+//! The action to be taken after updating a settings value.
+//! WRITE indicates that the updated value must be written to disk,
+//! while SKIP_WRITE indicates that the change will be kept in memory-only
+//! without persisting it.
+enum class SettingsAction {
+    WRITE,
+    SKIP_WRITE
+};
+
+using SettingsUpdate = std::function<std::optional<interfaces::SettingsAction>(common::SettingsValue&)>;
+
 //! Interface giving clients (wallet processes, maybe other analysis tools in
 //! the future) ability to access to the chain state, receive notifications,
 //! estimate fees, and submit transactions.
@@ -344,9 +355,16 @@ class Chain
     //! Return <datadir>/settings.json setting value.
     virtual common::SettingsValue getRwSetting(const std::string& name) = 0;
 
-    //! Write a setting to <datadir>/settings.json. Optionally just update the
-    //! setting in memory and do not write the file.
-    virtual bool updateRwSetting(const std::string& name, const common::SettingsValue& value, bool write=true) = 0;
+    //! Updates a setting in <datadir>/settings.json.
+    //! Depending on the action returned by the update function, this will either
+    //! update the setting in memory or write the updated settings to disk.
+    virtual bool updateRwSetting(const std::string& name, const SettingsUpdate& update_function) = 0;
+
+    //! Replace a setting in <datadir>/settings.json with a new value.
+    virtual bool overwriteRwSetting(const std::string& name, common::SettingsValue& value, bool write = true) = 0;
+
+    //! Delete a given setting in <datadir>/settings.json.
+    virtual bool deleteRwSettings(const std::string& name, bool write = true) = 0;
 
     //! Synchronously send transactionAddedToMempool notifications about all
     //! current mempool transactions to the specified handler and return after
diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
index 9fe08eb3dd3d4..54b986c926afd 100644
--- a/src/node/interfaces.cpp
+++ b/src/node/interfaces.cpp
@@ -814,14 +814,32 @@ class ChainImpl : public Chain
         });
         return result;
     }
-    bool updateRwSetting(const std::string& name, const common::SettingsValue& value, bool write) override
+    bool updateRwSetting(const std::string& name,
+                         const interfaces::SettingsUpdate& update_settings_func) override
     {
+        std::optional<interfaces::SettingsAction> action;
         args().LockSettings([&](common::Settings& settings) {
-            if (value.isNull()) {
-                settings.rw_settings.erase(name);
-            } else {
-                settings.rw_settings[name] = value;
-            }
+            auto* ptr_value = common::FindKey(settings.rw_settings, name);
+            // Create value if it doesn't exist
+            auto& value = ptr_value ? *ptr_value : settings.rw_settings[name];
+            action = update_settings_func(value);
+        });
+        if (!action) return false;
+        // Now dump value to disk if requested
+        return *action == interfaces::SettingsAction::SKIP_WRITE || args().WriteSettingsFile();
+    }
+    bool overwriteRwSetting(const std::string& name, common::SettingsValue& value, bool write) override
+    {
+        if (value.isNull()) return deleteRwSettings(name, write);
+        return updateRwSetting(name, [&](common::SettingsValue& settings) {
+            settings = std::move(value);
+            return write ? interfaces::SettingsAction::WRITE : interfaces::SettingsAction::SKIP_WRITE;
+        });
+    }
+    bool deleteRwSettings(const std::string& name, bool write) override
+    {
+        args().LockSettings([&](common::Settings& settings) {
+            settings.rw_settings.erase(name);
         });
         return !write || args().WriteSettingsFile();
     }
diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
index e26347d437af3..129b5c7c2a0a7 100644
--- a/src/wallet/load.cpp
+++ b/src/wallet/load.cpp
@@ -69,7 +69,7 @@ bool VerifyWallets(WalletContext& context)
             // Pass write=false because no need to write file and probably
             // better not to. If unnamed wallet needs to be added next startup
             // and the setting is empty, this code will just run again.
-            chain.updateRwSetting("wallet", wallets, /* write= */ false);
+            chain.overwriteRwSetting("wallet", wallets, /*write=*/false);
         }
     }
 
diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp
index 5584b43520aa6..e2d7429f94dbc 100644
--- a/src/wallet/wallet.cpp
+++ b/src/wallet/wallet.cpp
@@ -93,25 +93,30 @@ namespace wallet {
 
 bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name)
 {
-    common::SettingsValue setting_value = chain.getRwSetting("wallet");
-    if (!setting_value.isArray()) setting_value.setArray();
-    for (const common::SettingsValue& value : setting_value.getValues()) {
-        if (value.isStr() && value.get_str() == wallet_name) return true;
-    }
-    setting_value.push_back(wallet_name);
-    return chain.updateRwSetting("wallet", setting_value);
+    const auto update_function = [&wallet_name](common::SettingsValue& setting_value) {
+        if (!setting_value.isArray()) setting_value.setArray();
+        for (const auto& value : setting_value.getValues()) {
+            if (value.isStr() && value.get_str() == wallet_name) return interfaces::SettingsAction::SKIP_WRITE;
+        }
+        setting_value.push_back(wallet_name);
+        return interfaces::SettingsAction::WRITE;
+    };
+    return chain.updateRwSetting("wallet", update_function);
 }
 
 bool RemoveWalletSetting(interfaces::Chain& chain, const std::string& wallet_name)
 {
-    common::SettingsValue setting_value = chain.getRwSetting("wallet");
-    if (!setting_value.isArray()) return true;
-    common::SettingsValue new_value(common::SettingsValue::VARR);
-    for (const common::SettingsValue& value : setting_value.getValues()) {
-        if (!value.isStr() || value.get_str() != wallet_name) new_value.push_back(value);
-    }
-    if (new_value.size() == setting_value.size()) return true;
-    return chain.updateRwSetting("wallet", new_value);
+    const auto update_function = [&wallet_name](common::SettingsValue& setting_value) {
+        if (!setting_value.isArray()) return interfaces::SettingsAction::SKIP_WRITE;
+        common::SettingsValue new_value(common::SettingsValue::VARR);
+        for (const auto& value : setting_value.getValues()) {
+            if (!value.isStr() || value.get_str() != wallet_name) new_value.push_back(value);
+        }
+        if (new_value.size() == setting_value.size()) return interfaces::SettingsAction::SKIP_WRITE;
+        setting_value = std::move(new_value);
+        return interfaces::SettingsAction::WRITE;
+    };
+    return chain.updateRwSetting("wallet", update_function);
 }
 
 static void UpdateWalletSetting(interfaces::Chain& chain,


Test patch:
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 44ffddb168701..12d5a3b3eba0f 100644
--- a/src/wallet/test/wallet_tests.cpp
+++ b/src/wallet/test/wallet_tests.cpp
@@ -329,6 +329,40 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
     }
 }
 
+// This test verifies that wallet settings can be added and removed
+// concurrently, ensuring no race conditions occur during either process.
+BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
+{
+    WalletContext context;
+    context.chain = m_node.chain.get();
+    const auto NUM_WALLETS{5};
+
+    // Since we're counting the number of wallets, ensure we start without any.
+    BOOST_REQUIRE(context.chain->getRwSetting("wallet").isNull());
+
+    const auto& check_concurrent_wallet = [&](const auto& settings_function, int num_expected_wallets) {
+        std::vector<std::thread> threads;
+        threads.reserve(NUM_WALLETS);
+        for (auto i{0}; i < NUM_WALLETS; ++i) threads.emplace_back(settings_function, i);
+        for (auto& t : threads) t.join();
+
+        auto wallets = context.chain->getRwSetting("wallet");
+        BOOST_CHECK_EQUAL(wallets.getValues().size(), num_expected_wallets);
+    };
+
+    // Add NUM_WALLETS wallets concurrently, ensure we end up with NUM_WALLETS stored.
+    check_concurrent_wallet([&context](int i) {
+        Assert(AddWalletSetting(*context.chain, strprintf("wallet_%d", i)));
+    },
+                            /*num_expected_wallets=*/NUM_WALLETS);
+
+    // Remove NUM_WALLETS wallets concurrently, ensure we end up with 0 wallets.
+    check_concurrent_wallet([&context](int i) {
+        Assert(RemoveWalletSetting(*context.chain, strprintf("wallet_%d", i)));
+    },
+                            /*num_expected_wallets=*/0);
+}
+
 // Check that GetImmatureCredit() returns a newly calculated value instead of
 // the cached value after a MarkDirty() call.
 //