Merge 78567b052d into merged_master (Bitcoin PR bitcoin/bitcoin#30697)

src/interfaces/chain.h

@@ -98,6 +98,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.
@@ -346,9 +357,16 @@ public:
     //! 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

src/node/interfaces.cpp

@@ -815,14 +815,32 @@ public:
         });
         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) {
+            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) {
-            if (value.isNull()) {
-                settings.rw_settings.erase(name);
-            } else {
-                settings.rw_settings[name] = value;
-            }
+            settings.rw_settings.erase(name);
         });
         return !write || args().WriteSettingsFile();
     }

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);
         }
     }
 

src/wallet/test/wallet_tests.cpp

@@ -330,6 +330,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.
 //

src/wallet/wallet.cpp

@@ -101,25 +101,30 @@ namespace wallet {
 isminetype InputIsMine(const CWallet& wallet, const COutPoint& txin); // ELEMENTS: forward declaration to remove circular dep on receive.h
 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,