foster/bitcoin-bitcoin-core
0
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-02-12 11:19:08 -05:00
Code Issues Activity

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

Browse source 
8466329127 chain: simplify `deleteRwSettings` code and improve it's doc (ismaelsadeeq)
f8d91f49c7 chain: dont check for null settings value in `overwriteRwSetting` (ismaelsadeeq)
df601993f2 chain: ensure `updateRwSetting` doesn't update to a null settings (ismaelsadeeq)
c8e2eeeffb chain: uniformly use `SettingsAction` enum in settings methods (ismaelsadeeq)
1e9e735670 chain: move new settings safely in `overwriteRwSetting` (ismaelsadeeq)
1c409004c8 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 8466329127
  ryanofsky:
    Code review ACK 8466329127. 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 8466329127

Tree-SHA512: baf2f59ed5aac4a4bda0c84fb6554a466a40d1f7b52b61dc2ff293d83ae60e82b925b7003237b633fecb65eba3a4c108e69166046895d1295809fbe0de67b052
This commit is contained in:
Ava Chow 2024-09-20 13:26:38 -04:00
commit a8a2628b7a
No known key found for this signature in database
GPG key ID: 17565732E08E5E41
4 changed files with 30 additions and 24 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

11
src/interfaces/chain.h
View file

@ -356,15 +356,22 @@ public:
virtual common::SettingsValue getRwSetting(const std::string& name) = 0;
//! Updates a setting in <datadir>/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;
//! 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;
//! 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 <datadir>/settings.json.
virtual bool deleteRwSettings(const std::string& name, bool write = true) = 0;
//! 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
//! current mempool transactions to the specified handler and return after

26
src/node/interfaces.cpp
View file

@ -820,29 +820,29 @@ public:
{
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 (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
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);
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 overwriteRwSetting(name, {}, action);
}
void requestMempoolTransactions(Notifications& notifications) override
{

2
src/wallet/load.cpp
View file

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

15
src/wallet/test/wallet_tests.cpp
View file

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