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>
This commit is contained in:
ismaelsadeeq
2024-08-26 10:32:56 +01:00
parent ee367170cb
commit 1b41d45d46
5 changed files with 100 additions and 25 deletions

View File

@@ -814,14 +814,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();
}