Merge bitcoin/bitcoin#30828: interfaces: #30697 follow ups

84663291275248fd52da644b0c2566bbf9cc780b chain: simplify `deleteRwSettings` code and improve it's doc (ismaelsadeeq)
f8d91f49c7091102138fcb123850399e8fadfbc7 chain: dont check for null settings value in `overwriteRwSetting` (ismaelsadeeq)
df601993f2d7454f081316d6a8ddefbcefa49b3d chain: ensure `updateRwSetting` doesn't update to a null settings (ismaelsadeeq)
c8e2eeeffb494d99d95c1c45efeda5a98dce94cd chain: uniformly use `SettingsAction` enum in settings methods (ismaelsadeeq)
1e9e735670f029c975d3b7486a54e5bb67135df7 chain: move new settings safely in `overwriteRwSetting` (ismaelsadeeq)
1c409004c80bc5f2314e20cc922076e22931cf73 test: remove wallet context from `write_wallet_settings_concurrently` (ismaelsadeeq)

Pull request description:

  This PR addresses the remaining review comments from #30697

  1. Disallowed overwriting settings values with a `null` value.
  2. Uniformly used the `SettingsAction` enum in all settings methods instead of a boolean parameter.
  3. Updated `overwriteRwSetting` to receive the `common::SettingsValue` parameter by value, enabling it to be moved safely.
  4. Removed wallet context from the `write_wallet_settings_concurrently` unit test, as it is not needed.

ACKs for top commit:
  achow101:
    ACK 84663291275248fd52da644b0c2566bbf9cc780b
  ryanofsky:
    Code review ACK 84663291275248fd52da644b0c2566bbf9cc780b. Looks good, thanks for taking suggestions and applying them to the right commits. Only changes since last review were documentation improvements and simplifying delete method.
  furszy:
    Code review ACK 84663291275248fd52da644b0c2566bbf9cc780b

Tree-SHA512: baf2f59ed5aac4a4bda0c84fb6554a466a40d1f7b52b61dc2ff293d83ae60e82b925b7003237b633fecb65eba3a4c108e69166046895d1295809fbe0de67b052

2 files changed, 8 insertions, 9 deletions

diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
index 81f5e30082..2b5c021cda 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), interfaces::SettingsAction::SKIP_WRITE);
         }
     }
 
diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
index 5a520cbfe9..b5de4b4b3d 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<std::thread> 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);
 }