Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface

ismaelsadeeq commented at 2:56 pm on August 22, 2024: member

This PR fixes #30620.

As outlined in the issue, creating two wallets with load_on_startup=true simultaneously results in only one wallet being added to the startup file.

The current issue arises because the wallet settings update process involves:

Obtaining the settings value while acquiring the settings lock.
Modifying the settings value.
Overwriting the settings value while acquiring the settings lock again.

This sequence is not thread-safe. Different threads could modify the same base value simultaneously, overwriting data from other workers without realizing it.

The PR attempts to fix this by modifying the chain interface’s updateRwSetting method to accept a function that will be called with the settings reference. This function will either update or delete the setting and return an enum indicating whether the settings need to be overwritten in this or not.

Additionally, this PR introduces two new methods to the chain interface:

overwriteRwSetting: This method replaces the setting with a new value. Used in VerifyWallets
deleteRwSettings: This method completely erases a specified setting. This method is currently used only in overwriteRwSetting.

These changes ensure that updates are race-free across all clients.

DrahtBot commented at 2:56 pm on August 22, 2024: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	achow101, furszy
Stale ACK	stickies-v

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)
#27286 (wallet: Keep track of the wallet’s own transaction outputs in memory by achow101)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

ismaelsadeeq renamed this:
~~Wallet, Bugfix: Lock Wallet Context `cs` Before Adding/Removing Settings~~
Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings
on Aug 22, 2024

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

maflcko added the label Wallet on Aug 22, 2024

maflcko commented at 3:08 pm on August 22, 2024: member

(Assigned 28.0, but this is not a regression, so not a blocker for rc1. I think it can be backported to rc2, just in case it needs more time.)

in src/wallet/test/wallet_tests.cpp:341 in 135eb1d5b1 outdated

334+BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
335+{
336+    WalletContext context;
337+    context.chain = m_node.chain.get();
338+    const int NUM_WALLETS = 5;
339+    std::vector<std::thread> threads;

stickies-v commented at 3:20 pm on August 22, 2024:

For stability, would ensure we start without any wallets (e.g. in case any defaults get added, will make it more visible why this test fails. Alternatively, could capture the count of wallets at the beginning of the test and use that to compare with later on, to make the test resilient to unrelated changes, but I think that might be a bit overkill.

0    std::vector<std::thread> threads;
1    // Since we're counting the number of wallets, ensure we start without any.
2    BOOST_REQUIRE(context.chain->getRwSetting("wallet").isNull());

stickies-v commented at 3:48 pm on August 22, 2024:

0    std::vector<std::thread> threads;
1    threads.reserve(NUM_WALLETS);

ismaelsadeeq commented at 8:13 pm on August 22, 2024:

done

ismaelsadeeq commented at 8:27 pm on August 22, 2024:

Done.

in src/wallet/test/wallet_tests.cpp:347 in 135eb1d5b1 outdated

335+{
336+    WalletContext context;
337+    context.chain = m_node.chain.get();
338+    const int NUM_WALLETS = 5;
339+    std::vector<std::thread> threads;
340+    {

stickies-v commented at 3:21 pm on August 22, 2024:

nit: brief docstrings to at-a-glance help see how the test is structured:

 0diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
 1index ec9792f7a9..d434c1c207 100644
 2--- a/src/wallet/test/wallet_tests.cpp
 3+++ b/src/wallet/test/wallet_tests.cpp
 4@@ -337,6 +337,7 @@ BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
 5     context.chain = m_node.chain.get();
 6     const int NUM_WALLETS = 5;
 7     std::vector<std::thread> threads;
 8+    // Add NUM_WALLETS wallets concurrently, ensure we end up with NUM_WALLETS.
 9     {
10         for (int i{0}; i < NUM_WALLETS; i++) {
11             threads.emplace_back([i, &context] {
12@@ -349,6 +350,7 @@ BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
13         BOOST_CHECK_EQUAL(wallets.getValues().size(), NUM_WALLETS);
14     }
15     threads.clear();
16+    // Remove NUM_WALLETS wallets concurrently, ensure we end up with 0 wallets.
17     {
18         for (int i{0}; i < NUM_WALLETS; i++) {
19             threads.emplace_back([i, &context] {

ismaelsadeeq commented at 8:21 pm on August 22, 2024:

Added

in src/wallet/wallet.h:1115 in 135eb1d5b1 outdated

1111@@ -1112,10 +1112,10 @@ class WalletRescanReserver
1112 };
1113 
1114 //! Add wallet name to persistent configuration so it will be loaded on startup.
1115-bool AddWalletSetting(interfaces::Chain& chain, const std::string& wallet_name);
1116+bool AddWalletSetting(WalletContext& context, const std::string& wallet_name);

stickies-v commented at 3:26 pm on August 22, 2024:

Would be good to guard against lock contention (+ for RemoveWalletSetting):

 0diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h
 1index ad2e2817c4..fd61b39c47 100644
 2--- a/src/wallet/wallet.h
 3+++ b/src/wallet/wallet.h
 4@@ -19,6 +19,7 @@
 5 #include <script/script.h>
 6 #include <support/allocators/secure.h>
 7 #include <sync.h>
 8+#include <threadsafety.h>
 9 #include <tinyformat.h>
10 #include <uint256.h>
11 #include <util/fs.h>
12@@ -28,6 +29,7 @@
13 #include <util/time.h>
14 #include <util/ui_change_type.h>
15 #include <wallet/crypter.h>
16+#include <wallet/context.h>
17 #include <wallet/db.h>
18 #include <wallet/scriptpubkeyman.h>
19 #include <wallet/transaction.h>
20@@ -1112,10 +1114,10 @@ public:
21 };
22 
23 //! Add wallet name to persistent configuration so it will be loaded on startup.
24-bool AddWalletSetting(WalletContext& context, const std::string& wallet_name);
25+bool AddWalletSetting(WalletContext& context, const std::string& wallet_name) EXCLUSIVE_LOCKS_REQUIRED(!context.wallets_mutex);
26 
27 //! Remove wallet name from persistent configuration so it will not be loaded on startup.
28-bool RemoveWalletSetting(WalletContext& context, const std::string& wallet_name);
29+bool RemoveWalletSetting(WalletContext& context, const std::string& wallet_name) EXCLUSIVE_LOCKS_REQUIRED(!context.wallets_mutex);
30 
31 struct MigrationResult {
32     std::string wallet_name;

ismaelsadeeq commented at 8:20 pm on August 22, 2024:

Done.

in src/wallet/test/wallet_tests.cpp:359 in 135eb1d5b1 outdated

354+            threads.emplace_back([i, &context] {
355+                BOOST_CHECK_MESSAGE(RemoveWalletSetting(context, strprintf("wallet_%d", i)), strprintf("write wallet_%d failed", i));
356+            });
357+        }
358+        for (auto& t : threads)
359+            t.join();

stickies-v commented at 3:33 pm on August 22, 2024:

style nit: should either use braces or multi-line, or move to one-line approach

0        for (auto& t : threads) t.join();

ismaelsadeeq commented at 8:28 pm on August 22, 2024:

This is what was written initially. Clang format changed it to this format, which I thought was the recommended approach: https://github.com/bitcoin/bitcoin/tree/master/contrib/devtools#clang-format-diffpy

I’ve now switched to your suggestion.

ismaelsadeeq marked this as a draft on Aug 22, 2024

ismaelsadeeq commented at 3:40 pm on August 22, 2024: member

In draft now before addressing TSAN failure https://github.com/bitcoin/bitcoin/pull/30697/checks?check_run_id=29119417676

in src/wallet/test/wallet_tests.cpp:343 in 135eb1d5b1 outdated

338+    const int NUM_WALLETS = 5;
339+    std::vector<std::thread> threads;
340+    {
341+        for (int i{0}; i < NUM_WALLETS; i++) {
342+            threads.emplace_back([i, &context] {
343+                BOOST_CHECK_MESSAGE(AddWalletSetting(context, strprintf("wallet_%d", i)), strprintf("write wallet_%d failed", i));

maflcko commented at 3:45 pm on August 22, 2024:

I haven’t looked at the CI failure in detail, but IIRC, BOOST_CHECK macros are not allowed to be used in threads, because they are not thread-safe.

Does the race go away if you use Assert instead? Alternatively, you could try to collect the results and BOOST_CHECK them after the threads have joined

ismaelsadeeq commented at 3:51 pm on August 22, 2024:

I suppose I should update to check for the boolean returned value of true not a string message.

I will update to just Assert the returned value is true.

I will test your suggestion, thanks

stickies-v commented at 3:54 pm on August 22, 2024:

I think TSAN is failing because of these BOOST_CHECK_MESSAGE statements. Since you’re checking the count a bit further down already anyway, I think this is fine to remove. Exactly which thread failed doesn’t really add information anyway, I think?

 0diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
 1index ec9792f7a9..651c6efdf2 100644
 2--- a/src/wallet/test/wallet_tests.cpp
 3+++ b/src/wallet/test/wallet_tests.cpp
 4@@ -340,7 +340,7 @@ BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
 5     {
 6         for (int i{0}; i < NUM_WALLETS; i++) {
 7             threads.emplace_back([i, &context] {
 8-                BOOST_CHECK_MESSAGE(AddWalletSetting(context, strprintf("wallet_%d", i)), strprintf("write wallet_%d failed", i));
 9+                AddWalletSetting(context, strprintf("wallet_%d", i));
10             });
11         }
12         for (auto& t : threads)
13@@ -352,7 +352,7 @@ BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
14     {
15         for (int i{0}; i < NUM_WALLETS; i++) {
16             threads.emplace_back([i, &context] {
17-                BOOST_CHECK_MESSAGE(RemoveWalletSetting(context, strprintf("wallet_%d", i)), strprintf("write wallet_%d failed", i));
18+                RemoveWalletSetting(context, strprintf("wallet_%d", i));
19             });
20         }
21         for (auto& t : threads)

stickies-v commented at 3:56 pm on August 22, 2024:

As per #30697 (review), I’m not sure capturing the offending thread index adds value, since it won’t be deterministic anyway? And we’re already testing that all wallets were added/removed successfully? Also no objection to adding an Assert, though.

ismaelsadeeq commented at 8:17 pm on August 22, 2024:

Done thanks.

ismaelsadeeq commented at 8:29 pm on August 22, 2024:

This is fixed.

stickies-v commented at 3:54 pm on August 22, 2024: contributor

Approach ACK

ismaelsadeeq commented at 4:19 pm on August 22, 2024: member

Thanks @stickies-v for review, I’ve accepted all your suggestion and updated the branch. I will force push after verifying TSan C.I passes locally

DrahtBot added the label CI failed on Aug 22, 2024

DrahtBot commented at 4:29 pm on August 22, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29119417676

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

ismaelsadeeq force-pushed on Aug 22, 2024

DrahtBot removed the label CI failed on Aug 22, 2024

ismaelsadeeq marked this as ready for review on Aug 22, 2024

in src/wallet/test/wallet_tests.cpp:363 in 2cba0bed5b outdated

357+
358+    // Remove NUM_WALLETS wallets concurrently, ensure we end up with 0 wallets.
359+    {
360+        for (int i{0}; i < NUM_WALLETS; i++) {
361+            threads.emplace_back([i, &context] {
362+                Assert(RemoveWalletSetting(context, strprintf("wallet_%d", i)));

stickies-v commented at 12:10 pm on August 23, 2024:

nit: requires #include <util/check.h>

ismaelsadeeq commented at 12:34 pm on August 23, 2024:

Added

in src/wallet/test/wallet_tests.cpp:10 in 2cba0bed5b outdated

 6@@ -7,6 +7,7 @@
 7 #include <future>
 8 #include <memory>
 9 #include <stdint.h>
10+#include <thread>

stickies-v commented at 12:12 pm on August 23, 2024:

nit: while touching adjacent line, could help a neighbour out

0#include <cstdint>
1#include <future>
2#include <memory>
3#include <thread>

ismaelsadeeq commented at 12:34 pm on August 23, 2024:

Done.

in src/wallet/test/wallet_tests.cpp:334 in 2cba0bed5b outdated

329@@ -329,6 +330,44 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
330     }
331 }
332 
333+// This test verifies that wallet settings can be added and removed
334+// Concurrently, ensuring no race conditions occur during either process.

stickies-v commented at 12:14 pm on August 23, 2024:

0// concurrently, ensuring no race conditions occur during either process.

ismaelsadeeq commented at 12:34 pm on August 23, 2024:

Taken

stickies-v approved

stickies-v commented at 12:22 pm on August 23, 2024: contributor

ACK 2cba0bed5be8a25e1635b7cde0a98285f64b5469, left non-blocking nits but happy to quickly re-ACK.

Would suggest updating the commit message to describe the change a bit better, add bugfix to the title, and remove the double [] and : notation:

wallet: bugfix: lock wallet context before adding/removing wallet settings

When performing certain wallet operations simultaneously, a race condition can be triggered. For example, simultaneously loading two wallets can lead to only one wallet being added to the startup settings. Fix this by adding locks, and add test coverage.

ismaelsadeeq force-pushed on Aug 23, 2024

ismaelsadeeq commented at 12:36 pm on August 23, 2024: member

Would suggest updating the commit message to describe the change a bit better, add bugfix to the title, and remove the double [] and : notation:

Done, thank you.

stickies-v commented at 1:01 pm on August 23, 2024: contributor

re-ACK 245dfa20bd37f78783adbd03897b7f144d71a7b8

furszy commented at 4:09 pm on August 24, 2024: member

I don’t think using the context’s wallet mutex is the best approach for this issue. The problem lies in how we update settings, which is not atomic. Today, the race is in the wallets flow, but tomorrow it could easily affect another settings option.

Updates are executed in three separate steps:

Obtain settings value while acquiring the settings lock.
Modify settings value.
Overwrite settings value while acquiring the settings lock.

This process is not thread-safe. Different threads could modify the same base value simultaneously, overwriting data from other workers without realizing it.

Given this context, I’ve implemented a different solution that makes the underlying settings update operation atomic: https://github.com/furszy/bitcoin-core/commit/de05ba1144bd6332d1d903d07e49463268412dc0. This commit also shortens the test code.

ismaelsadeeq commented at 8:35 pm on August 24, 2024: member

Today, the race is in the wallets flow, but tomorrow it could easily affect another settings option.

I agree with this point. All clients that update settings will be victim to the same bug.

Given this context, I’ve implemented a different solution that makes the underlying settings update operation atomic: furszy@de05ba1. This commit also shortens the test code.

This seems like a great idea and an improvement to this PR. Clients will no longer need to lock a mutex before updating settings. I will take a look at furszy@de05ba1. Thank you for your feedback @furszy

furszy commented at 11:35 pm on August 25, 2024: member

Take this one: https://github.com/furszy/bitcoin-core/commit/b017949d83c1c47e6aea708ac89f0d81cead1064. Have removed the OptionModel changes because those require further changes to the Node interface and implementation.

ismaelsadeeq force-pushed on Aug 26, 2024

ismaelsadeeq renamed this:
~~Wallet, Bugfix: Lock Wallet Context mutex Before Adding/Removing Settings~~
Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface
on Aug 26, 2024

ismaelsadeeq commented at 11:16 am on August 26, 2024: member

This force-push diff is a complete change in approach from the previous one, which was simpler and more straightforward 245dfa20bd37f78783adbd03897b7f144d71a7b8 (i.e., locking the mutex before starting the settings update sequence). The commit was reviewed and acked by @stickies-v thanks

The new approach was written by @furszy which modifies the chain interface’s updateRwSetting method to receive an update function that is executed while the settings lock is held. The update function will returns an enum indicating whether the update was successful and if we should overwrite the settings.

I also think this approach is better and more sustainable, allowing other clients to avoid implementing their own thread control mechanisms for updating or deleting settings from the chain interface.

The changes in this diff include:

Adding a SettingsAction enum, which indicates whether we want to write to the settings after the update.
Introducing a SettingsUpdate type alias for the update function pointer.
Updating updateRwSetting to receive a SettingsUpdate function pointer.
- overwriteRwSetting: Replaces the setting with a new value.
- deleteRwSettings: Completely erases a specified setting. This method is currently used only in overwriteRwSetting.
Updating AddWalletSetting and RemoveWalletSetting to define and pass the update function to updateRwSetting.

The tests were also refactored to be a bit dry. @furszy, I made some changes to your commit, including adjusting variable initializations to use auto-deduced types, adding type aliases for the function pointers, and making variable names more descriptive.

DrahtBot added the label CI failed on Aug 26, 2024

DrahtBot commented at 12:28 pm on August 26, 2024: contributor

🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/29248667821

Make sure to run all tests locally, according to the documentation.

The failure may happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

wallet: bugfix: ensure atomicity in settings updates

- Settings updates were not thread-safe, as they were executed in
  three separate steps:

  1) Obtain settings value while acquiring the settings lock.
  2) Modify settings value.
  3) Overwrite settings value while acquiring the settings lock.

  This approach allowed concurrent threads to modify the same base value
  simultaneously, leading to data loss. When this occurred, the final
  settings state would only reflect the changes from the last thread
  that completed the operation, overwriting updates from other threads.

  Fix this by making the settings update operation atomic.

- Add test coverage for this behavior.

Co-authored-by: furszy <matiasfurszyfer@protonmail.com>

1b41d45d46

ismaelsadeeq force-pushed on Aug 26, 2024

DrahtBot removed the label CI failed on Aug 26, 2024

achow101 commented at 6:50 pm on August 26, 2024: member

ACK 1b41d45d462d856a9d0b44ae0039bbb2cd78407c

DrahtBot requested review from stickies-v on Aug 26, 2024

in src/interfaces/chain.h:108 in 1b41d45d46

103+enum class SettingsAction {
104+    WRITE,
105+    SKIP_WRITE
106+};
107+
108+using SettingsUpdate = std::function<std::optional<interfaces::SettingsAction>(common::SettingsValue&)>;

furszy commented at 7:57 pm on August 26, 2024:

I don’t think this alias is needed. It isn’t really a complex function and forces devs to scroll / search it. It can just be in another line in the function declaration.

ismaelsadeeq commented at 8:00 am on August 27, 2024:

I added it just to make the updateRwSetting method easier to read. I will update when their is need to retouch.

furszy commented at 8:00 pm on August 26, 2024: member

self-code-ACK https://github.com/bitcoin/bitcoin/commit/1b41d45d462d856a9d0b44ae0039bbb2cd78407c

luke-jr commented at 0:42 am on August 27, 2024: member

Any reason not to just lock the mutex across the three steps?

ismaelsadeeq commented at 8:02 am on August 27, 2024: member

Any reason not to just lock the mutex across the three steps?

Yes the reasons were highlighted in #30697#pullrequestreview-2258765600 and #30697#pullrequestreview-2260433636 comments

achow101 merged this on Aug 27, 2024

achow101 closed this on Aug 27, 2024

ismaelsadeeq deleted the branch on Aug 27, 2024

stickies-v commented at 6:25 pm on August 27, 2024: contributor

I wish this hadn’t been merged so quickly as I was still working on reviewing this, but I understand getting this in before 28.x branch off is helpful. I’m not a big fan of extending the Chain interface with non-chainstate related functions, that’s a wrong direction imo. I think rebasing this PR on something like https://github.com/stickies-v/bitcoin/commit/53acb88072ce4450d4cc3f16c40b05d8382e7287 would have been a more elegant base.

Also note that this PR did not update Node::updateRwSetting, which now behaves more like Chain::overwriteRwSetting, and is theoretically vulnerable to the same race conditions as the ones addressed in this PR. Since this is currently only used by the GUI, triggering a race condition there seems less feasible though.

ryanofsky commented at 6:47 pm on August 27, 2024: contributor

I’m not a big fan of extending the Chain interface with non-chainstate related functions, that’s a wrong direction imo.

It would be be nice to split up the Chain interface or just rename it to reflect way it is used, but it probably makes sense for wallet to use some interface class to read and write settings instead of using ArgsManager directly as in https://github.com/stickies-v/bitcoin/commit/53acb88072ce4450d4cc3f16c40b05d8382e7287. Wallet code originally did used ArgsManager directly until #22217, which changed it to use an interface class so wallet and node code could run in different processes and both could change settings without getting out of sync or corrupting the file. The wallet could be changed to use an independent settings file instead, and this would probably be a nice long term result. But defining interface classes for wallet code to go through instead of calling node functions and accessing node state directly has been an easy way to keep wallet code separate from node code without having to change wallet behavior.

furszy commented at 8:58 pm on August 27, 2024: member

Also note that this PR did not update Node::updateRwSetting, which now behaves more like Chain::overwriteRwSetting, and is theoretically vulnerable to the same race conditions as the ones addressed in this PR. Since this is currently only used by the GUI, triggering a race condition there seems less feasible though.

Yeah. I didn’t touch it to avoid expanding this PR beyond what was necessary to fix the race. The GUI settings change events are executed sequentially on the main thread, so they aren’t susceptible to this issue - at least not at this point in time -. We should definitely unify both updateRwSetting in a follow-up.

in src/node/interfaces.cpp:835 in 1b41d45d46

836+    }
837+    bool overwriteRwSetting(const std::string& name, common::SettingsValue& value, bool write) override
838+    {
839+        if (value.isNull()) return deleteRwSettings(name, write);
840+        return updateRwSetting(name, [&](common::SettingsValue& settings) {
841+            settings = std::move(value);

stickies-v commented at 11:52 am on August 28, 2024:

Moving an lvalue ref here isn’t safe, I think? Not an issue with the current callsites, but function signature should either take an rvalue ref or make a copy here. E.g. impl with rvalue ref (which is sufficient for now, but may require an overload in future):

 0diff --git a/src/interfaces/chain.h b/src/interfaces/chain.h
 1index be596b1765..20a801fa94 100644
 2--- a/src/interfaces/chain.h
 3+++ b/src/interfaces/chain.h
 4@@ -16,6 +16,7 @@
 5 #include <stddef.h>
 6 #include <stdint.h>
 7 #include <string>
 8+#include <utility>
 9 #include <vector>
10 
11 class ArgsManager;
12@@ -361,7 +362,7 @@ public:
13     virtual bool updateRwSetting(const std::string& name, const SettingsUpdate& update_function) = 0;
14 
15     //! Replace a setting in <datadir>/settings.json with a new value.
16-    virtual bool overwriteRwSetting(const std::string& name, common::SettingsValue& value, bool write = true) = 0;
17+    virtual bool overwriteRwSetting(const std::string& name, common::SettingsValue&& value, bool write = true) = 0;
18 
19     //! Delete a given setting in <datadir>/settings.json.
20     virtual bool deleteRwSettings(const std::string& name, bool write = true) = 0;
21diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
22index 54b986c926..0a89b5a3e4 100644
23--- a/src/node/interfaces.cpp
24+++ b/src/node/interfaces.cpp
25@@ -828,7 +828,7 @@ public:
26         // Now dump value to disk if requested
27         return *action == interfaces::SettingsAction::SKIP_WRITE || args().WriteSettingsFile();
28     }
29-    bool overwriteRwSetting(const std::string& name, common::SettingsValue& value, bool write) override
30+    bool overwriteRwSetting(const std::string& name, common::SettingsValue&& value, bool write) override
31     {
32         if (value.isNull()) return deleteRwSettings(name, write);
33         return updateRwSetting(name, [&](common::SettingsValue& settings) {
34diff --git a/src/wallet/load.cpp b/src/wallet/load.cpp
35index 129b5c7c2a..1149ce0a47 100644
36--- a/src/wallet/load.cpp
37+++ b/src/wallet/load.cpp
38@@ -69,7 +69,7 @@ bool VerifyWallets(WalletContext& context)
39             // Pass write=false because no need to write file and probably
40             // better not to. If unnamed wallet needs to be added next startup
41             // and the setting is empty, this code will just run again.
42-            chain.overwriteRwSetting("wallet", wallets, /*write=*/false);
43+            chain.overwriteRwSetting("wallet", std::move(wallets), /*write=*/false);
44         }
45     }
46

ryanofsky commented at 6:38 pm on September 5, 2024:

IMO it’s not great that this new API allows setting null values when previous API did not. Allowing null values to be set could encourage setting null values, which could encourage using null values, and treating them differently than missing values, which is probably not a good outcome. (Or even if it is good, is something that should be thought about more clearly, not just introduced in a bugfix.)

ryanofsky commented at 6:41 pm on September 5, 2024:

Agree rvalue reference would be an improvement, less for the reason that it would make the function safer to use, and more for the reason that it would make the function more convenient to use, because lvalue references cannot bind to temporary or literal values.

But even better than an rvalue reference would just be a plain value like common::SettingsValue value because this would allow the caller to decide whether they want to move or copy, instead of forcing the caller to always move.

ismaelsadeeq commented at 10:41 pm on September 5, 2024:

Taken your suggestion @ryanofsky this is fixed in #30828

ismaelsadeeq commented at 10:43 pm on September 5, 2024:

Agreed, I see no reason for allowing a null value. Removed in #30828

in src/node/interfaces.cpp:842 in 1b41d45d46

843+        });
844+    }
845+    bool deleteRwSettings(const std::string& name, bool write) override
846+    {
847+        args().LockSettings([&](common::Settings& settings) {
848+            settings.rw_settings.erase(name);

stickies-v commented at 12:34 pm on August 28, 2024:

Note: behaviour already existed prior to this PR, but this line (and thus also overwriteRwSetting) can throw if name is not an existing key, and would benefit from being documented in both function’s docstring I think.

ryanofsky commented at 6:32 pm on September 5, 2024:

Note: behaviour already existed prior to this PR, but this line (and thus also overwriteRwSetting) can throw if name is not an existing key, and would benefit from being documented in both function’s docstring I think.

map::erase shouldn’t actually throw anything, API was always intended to erase the setting if it exists otherwise do nothing

ismaelsadeeq commented at 10:42 pm on September 5, 2024:

I’ve tested this locally by passing an unknown settings name this does not throw.

in src/node/interfaces.cpp:824 in 1b41d45d46

825-            } else {
826-                settings.rw_settings[name] = value;
827-            }
828+            auto* ptr_value = common::FindKey(settings.rw_settings, name);
829+            // Create value if it doesn't exist
830+            auto& value = ptr_value ? *ptr_value : settings.rw_settings[name];

stickies-v commented at 12:38 pm on August 28, 2024:

I think this should be simplified to:

 0diff --git a/src/node/interfaces.cpp b/src/node/interfaces.cpp
 1index 54b986c926..b1699bc7ae 100644
 2--- a/src/node/interfaces.cpp
 3+++ b/src/node/interfaces.cpp
 4@@ -819,10 +819,8 @@ public:
 5     {
 6         std::optional<interfaces::SettingsAction> action;
 7         args().LockSettings([&](common::Settings& settings) {
 8-            auto* ptr_value = common::FindKey(settings.rw_settings, name);
 9-            // Create value if it doesn't exist
10-            auto& value = ptr_value ? *ptr_value : settings.rw_settings[name];
11-            action = update_settings_func(value);
12+            auto& existing_value{settings.rw_settings[name]};
13+            action = update_settings_func(existing_value);
14         });
15         if (!action) return false;
16         // Now dump value to disk if requested

ryanofsky commented at 6:53 pm on September 5, 2024:

Agree this would be an improvement. But I think it would be better to change behavior and avoid writing null settings (for reasons in other comment). Would probably write as:

0if (auto* value = common::FindKey(settings.rw_settings, name)) {
1    action = update_settings_func(*value);
2    if (value->isNull()) settings.rw_settings.erase(name);
3} else {
4    UniValue value;
5    action = update_settings_func(*value);
6    if (!value.isNull()) settings.rw_settings[name] = std::move(value);
7}

ismaelsadeeq commented at 10:42 pm on September 5, 2024:

Taken in #30828

in src/wallet/test/wallet_tests.cpp:336 in 1b41d45d46

328@@ -329,6 +329,40 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
329     }
330 }
331 
332+// This test verifies that wallet settings can be added and removed
333+// concurrently, ensuring no race conditions occur during either process.
334+BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
335+{
336+    WalletContext context;

stickies-v commented at 12:48 pm on August 28, 2024:

nit: there’s no need for a WalletContext here:

 0diff --git a/src/wallet/test/wallet_tests.cpp b/src/wallet/test/wallet_tests.cpp
 1index 12d5a3b3eb..7597765b24 100644
 2--- a/src/wallet/test/wallet_tests.cpp
 3+++ b/src/wallet/test/wallet_tests.cpp
 4@@ -333,12 +333,11 @@ BOOST_FIXTURE_TEST_CASE(importwallet_rescan, TestChain100Setup)
 5 // concurrently, ensuring no race conditions occur during either process.
 6 BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
 7 {
 8-    WalletContext context;
 9-    context.chain = m_node.chain.get();
10+    auto chain{m_node.chain.get()};
11     const auto NUM_WALLETS{5};
12 
13     // Since we're counting the number of wallets, ensure we start without any.
14-    BOOST_REQUIRE(context.chain->getRwSetting("wallet").isNull());
15+    BOOST_REQUIRE(chain->getRwSetting("wallet").isNull());
16 
17     const auto& check_concurrent_wallet = [&](const auto& settings_function, int num_expected_wallets) {
18         std::vector<std::thread> threads;
19@@ -346,19 +345,19 @@ BOOST_FIXTURE_TEST_CASE(write_wallet_settings_concurrently, TestingSetup)
20         for (auto i{0}; i < NUM_WALLETS; ++i) threads.emplace_back(settings_function, i);
21         for (auto& t : threads) t.join();
22 
23-        auto wallets = context.chain->getRwSetting("wallet");
24+        auto wallets = chain->getRwSetting("wallet");
25         BOOST_CHECK_EQUAL(wallets.getValues().size(), num_expected_wallets);
26     };
27 
28     // Add NUM_WALLETS wallets concurrently, ensure we end up with NUM_WALLETS stored.
29-    check_concurrent_wallet([&context](int i) {
30-        Assert(AddWalletSetting(*context.chain, strprintf("wallet_%d", i)));
31+    check_concurrent_wallet([chain](int i) {
32+        Assert(AddWalletSetting(*chain, strprintf("wallet_%d", i)));
33     },
34                             /*num_expected_wallets=*/NUM_WALLETS);
35 
36     // Remove NUM_WALLETS wallets concurrently, ensure we end up with 0 wallets.
37-    check_concurrent_wallet([&context](int i) {
38-        Assert(RemoveWalletSetting(*context.chain, strprintf("wallet_%d", i)));
39+    check_concurrent_wallet([chain](int i) {
40+        Assert(RemoveWalletSetting(*chain, strprintf("wallet_%d", i)));
41     },
42                             /*num_expected_wallets=*/0);
43 }

ismaelsadeeq commented at 10:42 pm on September 5, 2024:

Removed in #30828

stickies-v commented at 12:59 pm on August 28, 2024: contributor

post-merge ACK 1b41d45d462d856a9d0b44ae0039bbb2cd78407c

I think some follow-up improvements should be made as per my review comments, but this PR addresses the race condition well.

Wallet code originally did used ArgsManager directly until #22217, which changed it to use an interface class so wallet and node code could run in different processes and both could change settings without getting out of sync or corrupting the file.

Thanks for the context, I was wondering why things are the way they are at the moment. It makes sense in a multi-process context, but yeah, I don’t think these functions should be in Chain, but it may currently be the least bad option. The developer notes also state we shouldn’t be implementing any new functionality in interface methods, which we’re violating here:

Interface method definitions should wrap existing functionality instead of implementing new functionality. Any substantial new node or wallet functionality should be implemented in src/node/ or src/wallet/ and just exposed in src/interfaces/ instead of being implemented there, so it can be more modular and accessible to unit tests.

The wallet could be changed to use an independent settings file instead, and this would probably be a nice long term result.

This was my initial thought too, but too big of a change for a bugfix PR indeed.

stickies-v commented at 1:03 pm on August 28, 2024: contributor

Wallet code originally did used ArgsManager directly until #22217, which changed it to use an interface class so wallet and node code could run in different processes and both could change settings without getting out of sync or corrupting the file.

Actually, doesn’t the fact that the Rw methods on both Node and Chain interfaces directly use ArgsManager violate that principle too? Edit: looks like you’ve already addressed that in #22217 (comment) - although I don’t understand why it works that way, but that’s beyond the scope of this PR.

ryanofsky commented at 2:10 pm on August 28, 2024: contributor

Actually, doesn’t the fact that the Rw methods on both Node and Chain interfaces directly use ArgsManager violate that principle too?

This is ok because of the way multiprocess code works. When node and wallet code are running in the same process, and the wallet calls a virtual interfaces::Chain method, this just invokes the node::ChainImpl implementation of that method in src/node/interfaces.cpp. But when node and wallet code are running in different processes, and wallet code calls an interfaces::Chain method, it calls an IPC implementation of that method that forwards the method call and arguments to the node process, and then invokes the node::ChainImpl implemention of the method in the node process. So all settings reads and writes happen through the node process ArgsManager object and there is not more than one process accessing the settings file.

The developer notes also state we shouldn’t be implementing any new functionality in interface methods, which we’re violating here

I think this is a borderline case. Ideally wrapper methods in src/node/interfaces.cpp and src/wallet/interfaces.cpp are just a few lines long and consist of glue code that doesn’t implement any real functionality. I think that is basically the case with the methods in this PR, though though they do implement some options and features, and maybe those could be moved from ChainImpl to ArgsManager so the ArgsManager interface is more complete and glue code is simpler.

stickies-v commented at 2:18 pm on August 28, 2024: contributor

This is ok because of the way multiprocess code works … there is not more than one process accessing the settings file.

Thank you, that explanation actually clears up a lot, appreciate the thoughtful response, as always.

and maybe those could be moved from ChainImpl to ArgsManager so the ArgsManager interface is more complete and glue code is simpler.

and it would also allow code re-use across both interfaces.

ryanofsky commented at 4:25 pm on August 28, 2024: contributor

Thanks, I think a natural cleanup or followup to this PR could move the interfaces::Node settings methods (isSettingIgnored, getPersistentSetting, updateRwSetting, forceSetting, and resetSettings) and the interfaces::Chain settings methods (getSetting, getSettingList, getRwSetting, updateRwSetting, overwriteRwSetting, deleteRwSettings) into a new interfaces::Settings class and add a new interfaces::Init method returning it.

In the longer run, I think wallet processes should not need to access the settings interface directly, either because they store their own JSON settings detached from node settings, or because the setting that tracks which wallets to load at startup is managed by gui or node processes instead of wallet processes (i.e. the list of wallets to load would be passed to the WalletLoaderImpl::load() method, instead of it reading settings and deciding which wallets to load itself).

in src/node/interfaces.cpp:831 in 1b41d45d46

832+        });
833+        if (!action) return false;
834+        // Now dump value to disk if requested
835+        return *action == interfaces::SettingsAction::SKIP_WRITE || args().WriteSettingsFile();
836+    }
837+    bool overwriteRwSetting(const std::string& name, common::SettingsValue& value, bool write) override

ryanofsky commented at 6:55 pm on September 5, 2024:

Seems like as long as SettingsAction enum exists this function and deleteRwSettings below should probably take SettingsAction action parameters instead of bool write parameters to be more consistent and explicit.

ismaelsadeeq commented at 10:43 pm on September 5, 2024:

Done in #30828

ryanofsky commented at 6:58 pm on September 5, 2024: contributor

Just leaving some notes in case there are more changes to settings api later.

ismaelsadeeq commented at 10:48 pm on September 5, 2024: member

Thanks for your reviews, @stickies-v and @ryanofsky.
I’ve addressed the review comments in #30828.

Thanks. I think a natural cleanup or follow-up to this PR could involve moving the interfaces::Node settings methods (isSettingIgnored, getPersistentSetting, updateRwSetting, forceSetting, and resetSettings) and the interfaces::Chain settings methods (getSetting, getSettingList, getRwSetting, updateRwSetting, overwriteRwSetting, deleteRwSettings) into a new interfaces::Settings class and adding a new interfaces::Init method to return it.

I have started working on this in this commit. I believe this should come after #30828, but let me know if you’d prefer combining the two.

Bugfix: Ensure Atomicity in Wallet Settings Updates from Chain Interface #30697

Code Coverage

Reviews

Conflicts