From 1c409004c80bc5f2314e20cc922076e22931cf73 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Thu, 5 Sep 2024 20:32:20 +0100 Subject: [PATCH 1/6] test: remove wallet context from `write_wallet_settings_concurrently` --- src/wallet/test/wallet_tests.cpp | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp index 5a520cbfe93..b5de4b4b3d3 100644 --- a/src/wallet/test/wallet_tests.cpp +++ b/src/wallet/test/wallet_tests.cpp @@ -334,12 +334,11 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup) // 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(); + auto 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()); + BOOST_REQUIRE(chain->getRwSetting("wallet").isNull()); const auto& check_concurrent_wallet = [&](const auto& settings_function, int num_expected_wallets) { std::vector threads; @@ -347,19 +346,19 @@ BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup) 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"); + auto wallets = 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))); + check_concurrent_wallet([&chain](int i) { + Assert(AddWalletSetting(*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))); + check_concurrent_wallet([&chain](int i) { + Assert(RemoveWalletSetting(*chain, strprintf("wallet_%d", i))); }, /*num_expected_wallets=*/0); } From 1e9e735670f029c975d3b7486a54e5bb67135df7 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Thu, 5 Sep 2024 20:40:01 +0100 Subject: [PATCH 2/6] chain: move new settings safely in `overwriteRwSetting` --- src/interfaces/chain.h | 2 +- src/node/interfaces.cpp | 2 +- src/wallet/load.cpp | 2 +- 3 files changed, 3 insertions(+), 3 deletions(-) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index be596b17657..8aac48446aa 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -361,7 +361,7 @@ public: virtual bool updateRwSetting(const std::string& name, const SettingsUpdate& update_function) = 0; //! Replace a setting in /settings.json with a new value. - virtual bool overwriteRwSetting(const std::string& name, common::SettingsValue& value, bool write = true) = 0; + virtual bool overwriteRwSetting(const std::string& name, common::SettingsValue value, bool write = true) = 0; //! Delete a given setting in /settings.json. virtual bool deleteRwSettings(const std::string& name, bool write = true) = 0; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 54b986c926a..44a2cb93f20 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -828,7 +828,7 @@ public: // 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 + 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) { diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index 129b5c7c2a0..1149ce0a476 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.overwriteRwSetting("wallet", wallets, /*write=*/false); + chain.overwriteRwSetting("wallet", std::move(wallets), /*write=*/false); } } From c8e2eeeffb494d99d95c1c45efeda5a98dce94cd Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Thu, 5 Sep 2024 21:25:06 +0100 Subject: [PATCH 3/6] chain: uniformly use `SettingsAction` enum in settings methods --- src/interfaces/chain.h | 4 ++-- src/node/interfaces.cpp | 12 ++++++------ src/wallet/load.cpp | 2 +- 3 files changed, 9 insertions(+), 9 deletions(-) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 8aac48446aa..15ec220c2af 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -361,10 +361,10 @@ public: virtual bool updateRwSetting(const std::string& name, const SettingsUpdate& update_function) = 0; //! Replace a setting in /settings.json with a new value. - virtual bool overwriteRwSetting(const std::string& name, common::SettingsValue value, bool write = true) = 0; + virtual bool overwriteRwSetting(const std::string& name, common::SettingsValue value, SettingsAction action = SettingsAction::WRITE) = 0; //! Delete a given setting in /settings.json. - virtual bool deleteRwSettings(const std::string& name, bool write = true) = 0; + virtual bool deleteRwSettings(const std::string& name, SettingsAction action = SettingsAction::WRITE) = 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 44a2cb93f20..97d674ee06a 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -826,22 +826,22 @@ public: }); if (!action) return false; // Now dump value to disk if requested - return *action == interfaces::SettingsAction::SKIP_WRITE || args().WriteSettingsFile(); + return *action != interfaces::SettingsAction::WRITE || args().WriteSettingsFile(); } - bool overwriteRwSetting(const std::string& name, common::SettingsValue value, bool write) override + bool overwriteRwSetting(const std::string& name, common::SettingsValue value, interfaces::SettingsAction action) override { - if (value.isNull()) return deleteRwSettings(name, write); + if (value.isNull()) return deleteRwSettings(name, action); return updateRwSetting(name, [&](common::SettingsValue& settings) { settings = std::move(value); - return write ? interfaces::SettingsAction::WRITE : interfaces::SettingsAction::SKIP_WRITE; + return action; }); } - bool deleteRwSettings(const std::string& name, bool write) override + bool deleteRwSettings(const std::string& name, interfaces::SettingsAction action) override { args().LockSettings([&](common::Settings& settings) { settings.rw_settings.erase(name); }); - return !write || args().WriteSettingsFile(); + return action != interfaces::SettingsAction::WRITE || args().WriteSettingsFile(); } void requestMempoolTransactions(Notifications& notifications) override { diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp index 1149ce0a476..6285cb4db82 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.overwriteRwSetting("wallet", std::move(wallets), /*write=*/false); + chain.overwriteRwSetting("wallet", std::move(wallets), interfaces::SettingsAction::SKIP_WRITE); } } From df601993f2d7454f081316d6a8ddefbcefa49b3d Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Sun, 8 Sep 2024 21:06:33 +0100 Subject: [PATCH 4/6] chain: ensure `updateRwSetting` doesn't update to a null settings Co-authored-by: Ryan Ofsky --- src/interfaces/chain.h | 2 ++ src/node/interfaces.cpp | 12 ++++++++---- 2 files changed, 10 insertions(+), 4 deletions(-) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 15ec220c2af..6194d920fdf 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -356,6 +356,8 @@ public: virtual common::SettingsValue getRwSetting(const std::string& name) = 0; //! Updates a setting in /settings.json. + //! Null can be passed to erase the setting. There is intentionally no + //! support for writing null values to 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; diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 97d674ee06a..8e98434cb5a 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -819,10 +819,14 @@ public: { std::optional 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 (auto* value = common::FindKey(settings.rw_settings, name)) { + action = update_settings_func(*value); + if (value->isNull()) settings.rw_settings.erase(name); + } else { + UniValue new_value; + action = update_settings_func(new_value); + if (!new_value.isNull()) settings.rw_settings[name] = std::move(new_value); + } }); if (!action) return false; // Now dump value to disk if requested From f8d91f49c7091102138fcb123850399e8fadfbc7 Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Sun, 8 Sep 2024 20:26:49 +0100 Subject: [PATCH 5/6] chain: dont check for null settings value in `overwriteRwSetting` - Just call updateRwSetting it will erase the settings when the new value is null. --- src/interfaces/chain.h | 3 +++ src/node/interfaces.cpp | 1 - 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index 6194d920fdf..e81d1888ac4 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -363,6 +363,9 @@ public: virtual bool updateRwSetting(const std::string& name, const SettingsUpdate& update_function) = 0; //! Replace a setting in /settings.json with a new value. + //! Null can be passed to erase the setting. + //! This method provides a simpler alternative to updateRwSetting when + //! atomically reading and updating the setting is not required. virtual bool overwriteRwSetting(const std::string& name, common::SettingsValue value, SettingsAction action = SettingsAction::WRITE) = 0; //! Delete a given setting in /settings.json. diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index 8e98434cb5a..f6d85f2b72a 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -834,7 +834,6 @@ public: } bool overwriteRwSetting(const std::string& name, common::SettingsValue value, interfaces::SettingsAction action) override { - if (value.isNull()) return deleteRwSettings(name, action); return updateRwSetting(name, [&](common::SettingsValue& settings) { settings = std::move(value); return action; From 84663291275248fd52da644b0c2566bbf9cc780b Mon Sep 17 00:00:00 2001 From: ismaelsadeeq Date: Wed, 11 Sep 2024 16:57:28 +0100 Subject: [PATCH 6/6] chain: simplify `deleteRwSettings` code and improve it's doc Co-authored-by: Ryan Ofsky --- src/interfaces/chain.h | 2 ++ src/node/interfaces.cpp | 5 +---- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h index e81d1888ac4..7bc2d11b607 100644 --- a/src/interfaces/chain.h +++ b/src/interfaces/chain.h @@ -369,6 +369,8 @@ public: virtual bool overwriteRwSetting(const std::string& name, common::SettingsValue value, SettingsAction action = SettingsAction::WRITE) = 0; //! Delete a given setting in /settings.json. + //! This method provides a simpler alternative to overwriteRwSetting when + //! erasing a setting, for ease of use and readability. virtual bool deleteRwSettings(const std::string& name, SettingsAction action = SettingsAction::WRITE) = 0; //! Synchronously send transactionAddedToMempool notifications about all diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp index f6d85f2b72a..31536e15d0d 100644 --- a/src/node/interfaces.cpp +++ b/src/node/interfaces.cpp @@ -841,10 +841,7 @@ public: } bool deleteRwSettings(const std::string& name, interfaces::SettingsAction action) override { - args().LockSettings([&](common::Settings& settings) { - settings.rw_settings.erase(name); - }); - return action != interfaces::SettingsAction::WRITE || args().WriteSettingsFile(); + return overwriteRwSetting(name, {}, action); } void requestMempoolTransactions(Notifications& notifications) override {