This PR addresses the remaining review comments from #30697
null value.SettingsAction enum in all settings methods instead of a boolean parameter.overwriteRwSetting to receive the common::SettingsValue parameter by value, enabling it to be moved safely.write_wallet_settings_concurrently unit test, as it is not needed.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For detailed information about the code coverage, see the test coverage report.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | ryanofsky, furszy, achow101 |
| Concept ACK | TheCharlatan |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
360@@ -361,7 +361,7 @@ class Chain
361 virtual bool updateRwSetting(const std::string& name, const SettingsUpdate& update_function) = 0;
362
363 //! Replace a setting in <datadir>/settings.json with a new value.
364- virtual bool overwriteRwSetting(const std::string& name, common::SettingsValue& value, bool write = true) = 0;
365+ virtual bool overwriteRwSetting(const std::string& name, common::SettingsValue value, bool write = true) = 0;
&& to avoid accidental copies?
837 return *action == interfaces::SettingsAction::SKIP_WRITE || args().WriteSettingsFile();
838 }
839 bool overwriteRwSetting(const std::string& name, common::SettingsValue value, interfaces::SettingsAction action) override
840 {
841- if (value.isNull()) return deleteRwSettings(name, action);
842+ if (value.isNull()) return false; // disallow overwriting with null values.
In commit “chain: disallow overwriting settings with null values” (4627174d80838e1c6848f9dff47a2ca4a9a35091)
Would be good to just delete this line entirely and make overwriteRwSetting just call updateRwSetting without making a special case for null. Setting a null setting should just delete a setting consistently and not have different behavior for different functions.
(Sorry I didn’t notice this line before and mistakenly thought overwriteRwSetting would allow writing null values to the settings file, so my earlier comments might have been confusing.)
Setting a null setting should just delete a setting consistently and not have different behavior for different functions.
I would rather fail when a null setting is provided to any of the update or overwrite functions. I believe the deletion intent should be explicit, at least within this settings layer. That’s why the deleteRwSettings function was introduced. It seems inconsistent to have two different methods for deleting a value - one embedded within the update function and another explicit.
So we both agree it is bad for overwrite function to reject null values and update function to accept them, but I would prefer both functions to accept them and you want both functions to reject them.
Here are the reason I think it is better to accept them:
I think it is generally bad practice to accept nullable values and then handle them with runtime errors. If you want to write API’s that don’t accept null values I think you should use non-nullable types.
I think in this specific case, there is no reason these APIs should not accept null values. Ihe purpose of the update function is to be able to atomically read and update settings. But there is no function to atomically read and delete settings and this should be a completely valid use-case for situations where you want to safely reset settings without clobbering concurrently set values. We can easily support it by just not going out of the way to add unnecessary restrictions.
I am fine with existence of deleteRwSetting as a more convenient & readable wrapper around overwriteRwSetting and updateRwSetting, the same way overwriteRwSetting is a convenient and readable wrapper around updateRwSetting. But existence of deleteRwSetting should not be an excuse for crippling the behavior of the other functions and making them less safe to call. Ability to atomically read and delete settings is useful, and trying to deal with null values at runtime rather than compile time is a bad practice that should be a last not first resort.
There are two suggestions here:
@furszy suggested to fail when the value is null: I agree with not checking for null values, for the reasons outlined above.
@ryanofsky suggested removing the null value check: This approach would erase the settings when new value is null.
Setting a null value should consistently delete the setting, rather than having different behavior in different functions.
This makes the method redundant, as calling deleteRwSettings achieves the same result as passing a null SettingsValue to overwriteRwSetting/ making update function in updateRwSetting modify the settings value to null.
I think the issue with allowing the settings to be able to be overridden/updated as null, but making the caller’s responsibility to determine to not pass a null value to overwriteRwSetting or not define an update functions that set’s the settings value to null.
Should outweigh the redundancy concern hence I updated to @ryanofsky suggestion.
A bit delayed but here.
So we both agree it is bad for overwrite function to reject null values and update function to accept them, but I would prefer both functions to accept them and you want both functions to reject them.
Here are the reason I think it is better to accept them:
- I think it is generally bad practice to accept nullable values and then handle them with runtime errors. If you want to write API’s that don’t accept null values I think you should use non-nullable types.
- I think in this specific case, there is no reason these APIs should not accept null values. Ihe purpose of the update function is to be able to atomically read and update settings. But there is no function to atomically read and delete settings and this should be a completely valid use-case for situations where you want to safely reset settings without clobbering concurrently set values. We can easily support it by just not going out of the way to add unnecessary restrictions.
I am fine with existence of deleteRwSetting as a more convenient & readable wrapper around overwriteRwSetting and updateRwSetting, the same way overwriteRwSetting is a convenient and readable wrapper around updateRwSetting. But existence of deleteRwSetting should not be an excuse for crippling the behavior of the other functions and making them less safe to call. Ability to atomically read and delete settings is useful, and trying to deal with null values at runtime rather than compile time is a bad practice that should be a last not first resort.
Yeah ok, thread-safety wise, fully agree. The read+update atomic function for deletion is also needed, deleteRwSetting does not fulfills that purpose for now, unless we turn it into an update wrapper. And also agree about treating null values at compile time rather than during runtime whenever possible.
I think the only thing that “bothers” me a bit about the nullable class is the different meanings it could have, which require additional clarifications at the API level. E.g. “store key with no value”, “disable feature temporarily with no storage, so it can be restored at the next startup” or “delete this item”.
It seems we could achieve the behavior by turning the SettingsAction into a bit flag, returning something like REMOVE_ITEM | WRITE_TO_DISK or UPDATE_ITEM | WRITE_TO_DISK and using optionals instead.
That said, it’s not a strong concern at all. I agree that null values should be treated at compile time and don’t think we want to change the API to move away from UniValue. So, all good.
I think the only thing that “bothers” me a bit about the nullable class is the different meanings it could have, which require additional clarifications at the API level.
Yeah this is real concern, and I think a common thing that happens with null values, true/false values, empty strings, and special integer values. If you are sloppy with these values you will run into problems. If you are thoughtful you can come up with very clean interfaces, but it is hard to be careful. You can avoid problems with ambiguous values by using enums, std::variant, not_null implementations, and custom types, but these can be inconvenient or verbose. We’re just trying to pick a tradeoff that is convenient and doesn’t cause big surprises.
844 {
845 args().LockSettings([&](common::Settings& settings) {
846 settings.rw_settings.erase(name);
847 });
848- return !write || args().WriteSettingsFile();
849+ return action == interfaces::SettingsAction::SKIP_WRITE || args().WriteSettingsFile();
In case we add another action in the future, this could be:
0 return action != interfaces::SettingsAction::WRITE || args().WriteSettingsFile();
I guess you are trying to avoid a situation where we would need to write:
0return action == interfaces::SettingsAction::SKIP_WRITE || action == interfaces::SettingsAction::NEW_ACTION || args().WriteSettingsFile();
In some cases we would still need to modify this to handle the new action if it has additional effects beyond just writing, such as writing and logging, for example.
I’ve updated to your suggestion, as it is more maintainable.
Code review ACK adc019860d7df6848b98aec58f1c3ff47936e06b. These are nice, well-organized improvements to the settings interface to make it more consistent, remove ability to write null values to settings.json, and add ability to atomically read and delete settings.
I think not allowing null values in settings.json is good change to encourage clearer representations of settings, maintain parity between settings.json / bitcoin.conf / command line settings representations, avoid making ArgsManager implementation more complicated, and helpful if we want to have a clean RPC interface for updating settings later, since our JSON-RPC framework does not distinguish between null and unspecified parameter values, and bitcoin-cli doesn’t provide a clean way to pass null parameters.
The PR looks good in it’s current form, but could consider improving documentation and simplifying the deleteRwSettings implementation with:
0--- a/src/interfaces/chain.h
1+++ b/src/interfaces/chain.h
2@@ -356,14 +356,21 @@ public:
3 virtual common::SettingsValue getRwSetting(const std::string& name) = 0;
4
5 //! Updates a setting in <datadir>/settings.json.
6+ //! Null can be passed to erase the setting. There is intentionally no
7+ //! support for writing null values to settings.json.
8 //! Depending on the action returned by the update function, this will either
9 //! update the setting in memory or write the updated settings to disk.
10 virtual bool updateRwSetting(const std::string& name, const SettingsUpdate& update_function) = 0;
11
12 //! Replace a setting in <datadir>/settings.json with a new value.
13+ //! Null can be passed to erase the setting.
14+ //! This method provides a simpler alternative to updateRwSetting when
15+ //! atomically reading and updating the setting is not required.
16 virtual bool overwriteRwSetting(const std::string& name, common::SettingsValue value, SettingsAction action = SettingsAction::WRITE) = 0;
17
18 //! Delete a given setting in <datadir>/settings.json.
19+ //! This method provides a simpler alternative to overwriteRwSetting when
20+ //! erasing a setting, for ease of use and readability.
21 virtual bool deleteRwSettings(const std::string& name, SettingsAction action = SettingsAction::WRITE) = 0;
22
23 //! Synchronously send transactionAddedToMempool notifications about all
24--- a/src/node/interfaces.cpp
25+++ b/src/node/interfaces.cpp
26@@ -841,10 +841,7 @@ public:
27 }
28 bool deleteRwSettings(const std::string& name, interfaces::SettingsAction action) override
29 {
30- args().LockSettings([&](common::Settings& settings) {
31- settings.rw_settings.erase(name);
32- });
33- return action != interfaces::SettingsAction::WRITE || args().WriteSettingsFile();
34+ return overwriteRwSetting(name, {}, action);
35 }
36 void requestMempoolTransactions(Notifications& notifications) override
37 {
Other followups are also possible and could be future PRs:
interface::Chain and interfaces::Node should probably be dropped and moved into an interfaces::Settings class returned by an interfaces::init::makeSettings() method.updateRwSetting implementation into src/common/args or src/common/settings as an ArgsManager method or helper function.src/test/interfaces_tests.cpp.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
- Just call updateRwSetting it will erase the settings when the new
value is null.
Co-authored-by: Ryan Ofsky <ryan@ofsky.org>
The PR looks good in it’s current form, but could consider improving documentation and simplifying the deleteRwSettings implementation with
Done in the latest force push see diff https://github.com/bitcoin/bitcoin/compare/adc019860d7df6848b98aec58f1c3ff47936e06b..84663291275248fd52da644b0c2566bbf9cc780b
In commit “chain: dont check for null settings value in overwriteRwSetting” (f8d91f49c7091102138fcb123850399e8fadfbc7)
Might be good to say in commit message that this change is a refactoring and does not change behavior. Could have “refactor:” prefix in the title.
840@@ -841,10 +841,7 @@ class ChainImpl : public Chain
841 }
842 bool deleteRwSettings(const std::string& name, interfaces::SettingsAction action) override
843 {
844- args().LockSettings([&](common::Settings& settings) {
845- settings.rw_settings.erase(name);
846- });
847- return action != interfaces::SettingsAction::WRITE || args().WriteSettingsFile();
848+ return overwriteRwSetting(name, {}, action);
In commit “chain: simplify deleteRwSettings code and improve it’s doc” (84663291275248fd52da644b0c2566bbf9cc780b)
Might be good to say in commit message that this change is a refactoring and does not change behavior. Could have “refactor:” prefix in the title.
Code review ACK 84663291275248fd52da644b0c2566bbf9cc780b
It seems that with the latest changes, the deleteRwSettings is no longer used.
It would be nice to add test coverage for it and for the null value usage as well.
