Merge settings one place instead of five places #15934

re: #15934 (review)

These can be nice sometimes. They are also called fluent interfaces (https://en.wikipedia.org/wiki/Fluent_interface)

re: #15934 (review)

I guess it doesn’t cause a real negation, more of an effective negation like you described. I renamed the variable result_complete to avoid mentioning negation.

re: #15934 (review)

Looks like github diffs are 120 columns wide. I wrapped this line and put ColumnLimit: 119 in my clang-format

re: #15934 (review)

Potentially out of scope: could we log warnings for this instead of silently ignoring?

I’m planning on making followup PRs that simplify and clean up all these “Weird behavior preserved for backwards compatibility” instances. I’d rather not add warnings in this PR partly because I’m disinclined to mix up behavior changes and refactoring changes in the same PR, but also because having some invalid options result in warnings and other invalid options result in errors seems even more strange and complicated than what the code does now.

Cool test! The formatted output is really helpful. Encourage other reviewers to run and inspect with

0SETTINGS_MERGE_TEST_OUT=results.txt ./src/test/test_bitcoin --run_test=settings_tests/Merge

re: #15934 (review)

Rebased!

Could test precedence difference between commandline and config file with the following diff (if you end up needing to rebase for some other reason):

 0diff --git a/src/test/settings_tests.cpp b/src/test/settings_tests.cpp
 1index 36188f8471..c82ecaea8c 100644
 2--- a/src/test/settings_tests.cpp
 3+++ b/src/test/settings_tests.cpp
 4@@ -18,14 +18,28 @@ BOOST_AUTO_TEST_CASE(Simple)
 5 {
 6     util::Settings settings;
 7     settings.command_line_options["name"].push_back("val1");
 8+    settings.command_line_options["name"].push_back("val2");
 9     settings.ro_config["section"]["name"].push_back(2);
10-    util::SettingsValue single_value = GetSetting(settings, "section", "name", false, false);
11-    util::SettingsValue list_value(util::SettingsValue::VARR);
12-    for (const auto& item : GetListSetting(settings, "section", "name", false)) {
13-        list_value.push_back(item);
14-    }
15-    BOOST_CHECK_EQUAL(single_value.write().c_str(), R"("val1")");
16-    BOOST_CHECK_EQUAL(list_value.write().c_str(), R"(["val1",2])");
17+
18+    auto check_values = [&](util::Settings s, std::string single_val, std::string list_val) {
19+        util::SettingsValue single_value = GetSetting(s, "section", "name", false, false);
20+        util::SettingsValue list_value(util::SettingsValue::VARR);
21+        for (const auto& item : GetListSetting(s, "section", "name", false)) {
22+            list_value.push_back(item);
23+        }
24+        BOOST_CHECK_EQUAL(single_value.write().c_str(), single_val);
25+        BOOST_CHECK_EQUAL(list_value.write().c_str(), list_val);
26+    };
27+
28+    // The last given arg takes precedence when specified via commandline.
29+    check_values(settings, R"("val2")", R"(["val1","val2",2])");
30+
31+    util::Settings settings2;
32+    settings2.ro_config["section"]["name"].push_back("val2");
33+    settings2.ro_config["section"]["name"].push_back("val3");
34+
35+    // The first given arg takes precedence when specified via config file.
36+    check_values(settings2, R"("val2")", R"(["val2","val3"])");
37 }
38 
39 // Test different ways settings can be merged, and verify results. This test can

re: #15934 (review)

Thanks! Added test.

I suppose any other variant type should be easy to use with basic types..

I think boost::variant would be an inferior choice, but just as an experiment I tried dropping it in here with some helper methods to make it work without changes to other code:

 0struct SettingsValue : public boost::variant<std::nullptr_t, bool, int64_t, std::string>
 1{
 2    using variant::variant;
 3    template<typename T> const T* ptr() const { return boost::get<T>(this); }
 4    bool isNull() const { return ptr<std::nullptr_t>(); }
 5    bool isBool() const { return ptr<bool>(); }
 6    bool isNum() const { return ptr<int64_t>(); }
 7    bool isFalse() const { return ptr<bool>() && !*ptr<bool>(); }
 8    bool isTrue() const { return ptr<bool>() && *ptr<bool>(); }
 9    const std::string& get_str() const { return *ptr<std::string>(); }
10    const bool& get_bool() const { return *ptr<bool>(); }
11    const int64_t& get_int64() const { return *ptr<int64_t>(); }
12};

This was sufficient for everything except settings_test.cpp (which uses serialization), so it confirms SettingsValue only requires storing ints, bools, strings and nulls, and the choice of type isn’t very important.

re: #15934 (review)

This is really useful, and could IMO be included as a comment to document what interface is needed if we did want to substitute another SettingsValue class.

Good suggestion to document assumptions about the type. Instead of pasting the whole block of code I just listed the required methods and added a github link, but I can add the example code inline if preferred.

I just listed the required methods and added a github link, but I can add the example code inline if preferred.

Looks great to me as it is. Thanks!

what’s top-level section of the config file ? The non-network one?

Yes, literally the top of the config file before a [section] delimiter. I don’t really like the term non-network settings because the section can contain network related settings that will be used on mainnet.

nit: GetSettingAllOccurence?, given name was expecting to query multiple settings at same time

Sure, will rename.

re: #15934 (review)

nit: GetSettingAllOccurence?, given name was expecting to query multiple settings at same time

Sure, will rename.

Forgot to do this last time. I think I’ll use GetSetting for single valued settings and GetSettingsList for many valued settings. I’d like to keep using the word “list” here to be consistent with #16545 which also uses it with TYPE_STRING_LIST as one of the settings types.

nit: find “value is not set” better than “has ignored config value”

I’m not sure “value is not set” is better. A value is set by the user, it’s just not applied due to the current network. Will expand command though.

nit: any reason why you don’t dereference compare to others?

All three settings sources (forced_settings, command_line_options, ro_config) are different types, so there’s not much reason to expect SettingsSpan construction to be the same for all of them:

https://github.com/bitcoin/bitcoin/blob/61e034f1f6b3beeeed51ef8aaf7762f9244a7c8f/src/util/settings.h#L25-L29

But I think the only reason for passing a pointer was to save a line of code and not add a fourth SettingsSpan constructor. I think I will go back and add this, though, since I think it’s generally better to pass by reference not pointer when the argument can’t be null.

re: #15934 (review)

Just to be sure the IsArgNegated is preserved here in GetListSetting via source.span.negated() > 0 ?

It’s handled implicitly by for (const auto& value : source.span) in GetListSetting, because the SettingsSpan iterator skips negated values in its begin() method.

The source.span.negated() > 0 check you’re referring is used after that to set the result_complete variable, which is used to apply settings precedence command_line_options > rw_settings > ro_config, and make sure for example that a negation on the command line will clear out any lower priority settings in ro_config and rw_settings. I’ll add a comment to say this explicitly on the result_complete line.

re: #15934 (review)

Hmmm I can’t get from where this behavior is extracted ?

This behavior makes no sense and was never intended, so it’s buried away in current code. It happens because of the following code in ArgsManager::ParseParameters:

https://github.com/bitcoin/bitcoin/blob/9dd6bbba613d7462afdb6276c4002bc183478528/src/util/system.cpp#L411-L420

A section section.name command line setting here will fail to trigger the “Invalid parameter” error because FlagsOfKnownArg strips out the section. prefix and finds that name is a known setting. But InterpretOption fails to strip out the section. prefix and stores the setting in the m_override_args map under section.name instead of name, where it is ignored and never has any effect.

The new code improves this by documenting the behavior and making it easy to change in the future. It also avoids bugs like this by splitting the section.name string exactly once in InterpretOption instead of many different places and times.

re: #15934 (review)

Consider marking as const and applying ES.28: Use lambdas for complex initialization, especially of const variables. That re-assignment span = SettingsSpan(*values); is rather “hidden”.

It can definitely be confusing when a non-const variable has a long lifetime and its value changes over a complicated function. But the span variable here is constructed, assigned, used, and destroyed all in about 5 lines of code, so using an initialization lambda would seem make overall flow more complicated instead of less.

re: #15934 (review)

This looks redundant. result is always null here, I don’t see where it can be changed above. All this could be rewritten as a return <something> instead of changing via lambda capturer reference…

I can maybe change the comment above to clarify, but return here is what keeps lower priority values from overwriting higher priority values. Bear in mind this is a lambda called repeatedly, one time for each setting source.

Do we really need to support SettigsSpan s = std::vec... ?

We don’t really need it because the data / size constructor could be called instead but having this makes code in system.cpp less verbose and I’m not sure what the preferred alternative would be: Calling .data() and .size() manually? Helper function to call .data() and .size()? Template constructor that calls .data() and .size() without referencing std::vector?

Oh no, I do not suggest to remove this helper constructor, maybe just adding explicit for that constructor. Currently it implicilty-converts from std::vecotor:

0//...
1DoSomething(const SettingsSpan& span);
2//...
3std::vector<SettingsValue>& vec;
4DoSomething(vec); // implicilty conerts to SettingsSpan, kinda "sneaky".

With explicit constructor:

0DoSomething(SettingsValue{vec}); // we see that we pass SettngsValue, not "just" a vector

Although looking at std::span implementations, these constructors are also implicit there, so I guess you can ignore this comment.

re: #15934 (review)

Although looking at std::span implementations, these constructors are also implicit there, so I guess you can ignore this comment.

Yes, it’s a perfectly safe conversion that doesn’t create ownership or performance issues, and in case of std::span reduces noise and boilerplate in code. But in any case I updated this PR to use explicit here the other day based on your feedback (https://github.com/ryanofsky/bitcoin/compare/pr/mergeset.17..pr/mergeset.18) and I think it’s an improvement.

const auto it as it is not changed later.

P.S. really sorry for not using “Review Changes” feature. I’ve been informed that these “add single comment” spams the notification channel… Will do better next time.

Since SettingsSpan copy constructor is noexcept, this could be too. Though if it would change any time in the future, using noexcept() operator could make it conditionally noexcept, by checking if copy constructor is actually noexcept (if author cares):

0explicit Source(SettingsSpan span) noexcept(noexcept(SettingsSpan{span}) : span(span) {}

re: #15934 (review)

Since SettingsSpan copy constructor is noexcept, this could be too. Though if it would change any time in the future, using noexcept() operator could make it conditionally noexcept, by checking if copy constructor is actually noexcept (if author cares):

0explicit Source(SettingsSpan span) noexcept(noexcept(SettingsSpan{span}) : span(span) {}

Hmm, I’m not an expert on noexcept, and I have no problem adding it on functions that obviously will not throw, or adding it in places where it can really improve performance. But it seems dangerous and not useful to add it whereever it can be added just because some code doesn’t currently happen to throw right now. noexcept is not like const where you get a nice compile time error when a function that shouldn’t throw does. It’s a fatal runtime error, so there are safety considerations with using it, and to me the best approach would seem to be not applying it unless it’s very obviously correct or unless there’s a specific optimization desired.

This function has some rather unusual looping, so if you care about prioritizing usage of standard algorithms, it could be implemented in something like this:

0        const std::reverse_iterator<const SettingsValue*> rbegin(end());
1        const std::reverse_iterator<const SettingsValue*> rend(begin());
2        const auto it = std::find_if(rbegin, rend, [](const SettingsValue &i) { return i.isFalse(); });
3        return static_cast<size_t>(std::distance(it, rend));

re: #15934 (review)

This function has some rather unusual looping, so if you care about prioritizing usage of standard algorithms, it could be implemented in something like this:

0        const std::reverse_iterator<const SettingsValue*> rbegin(end());
1        const std::reverse_iterator<const SettingsValue*> rend(begin());
2        const auto it = std::find_if(rbegin, rend, [](const SettingsValue &i) { return i.isFalse(); });
3        return static_cast<size_t>(std::distance(it, rend));

I like standard algorithms, but wouldn’t prioritize using them unless they made code simpler, and I don’t think they help in that respect here.

re: #15934 (review)

std::count_if would be simplest, but not sure if semantics will be the same, i.e. will there acutally be N sequental .isFalse()‘es.

std::count_if would not be right here, but your initial std::distance suggestion would work. The behavior of negation is a little subtle, which I why I thought the code would be easier to understand as a 2 line for-loop, instead of something that might send someone looking up corner cases in std algorithm man pages.

span.negated() is mostly just called as a boolean check to see if were are any negated values in the span and return as quickly as possible. It returns a size_t instead of a bool because in some cases, the number of negated values is important. A false bool value is negated but also negates (skips, clears) any values that came before it, so they won’t be included in span iteration, or returned by GetArgs or GetSettingsList.

re: #15934 (review)

Now there are some redundant explicit’s. There’s no need to mark SettingsSpan() = default; or more-than-one-argument-taking SettingsSpan(const SettingsValue* data, size_t size) as explicit. It’s only for the cases when constructor can initialize with single argument.

I think this isn’t strictly true. Explicit keyword also affects whether you can implicitly use a constructor from a return statement, I believe. But actually the reason for adding explicit here is just aesthetics (I like the constructors to line up, and explicit as a signpost for “there be constructors here”).

re: #15934 (review)

I believe "" are rather overused on codebases. This invokes std::string’s copy constructor taking const char* that will try to strlen(), etc. Creating default-constructed std::string{}/ std::string()/{} (in right context) would feel more “pedantically correct” :) .

Will keep because constructing strings from "" is ubiquitious in our codebase, not where I would look to improve performance, and I’d prefer it to be clear this is a string lookup.

re: #15934 (review)

"\n" -> '\n', no need for const char* / std::string for single character.

This is moved code, so I would prefer to not change it and create a spurious difference.

re: #15934 (review)

"-" -> '-'

Changed

re: #15934 (review)

const bool?

Changed

re: #15934 (review)

const util::SettingsValue value

Changed

re: #15934 (review)

const util::SettingsValue value

Changed

re: #15934 (review)

const util::SettingsValue value

Changed

re: #15934 (review)

I believe it could be const std::vectorutil::SettingsValue* const includes. Neither value (vector) nor pointer is ever changed (right?).

Changed

could be default-constucted instread of copy-constructed std::string: add_includes({});

Also, both size_t’s could be const.

re: #15934 (review)

could be default-constucted instread of copy-constructed std::string: add_includes({});

Also, both size_t’s could be const.

Changed

re: #15934 (review)

"" -> {} too ?

Changed

re: #15934 (review)

Is this the right place to put this include? Shouldn’t it be way more up, together with other bitcoin stuff?

I don’t think there is a strict convention, but often I see and write lists of internal includes followed by lists of external includes. Univalue is technically an external include and a library that can be installed separately.

Univalue is technically an external include

Oh, right then.

Is it possible for if to be true, but loop be empty, i.e. not to loop not a single time (empty span), so success = false would be never executed? Or this would be expected?

If it is impossible that for() never loops (it always loops at least once), then bool success is not needed, return false can be returned after loop body, and return true in the end of the function.

re: #15934 (review)

Is it possible for if to be true, but loop be empty, i.e. not to loop not a single time (empty span), so success = false would be never executed? Or this would be expected?

If it is impossible that for() never loops (it always loops at least once), then bool success is not needed, return false can be returned after loop body, and return true in the end of the function.

Not impossible, and the assumption won’t be true if -noincludeconf is used. Even if it were true, the suggestion seems like a more fragile way to write the code, and I’m not sure what the perceived benefit would be. I would definitely look askance at code that was returning failure potentially without setting the error string.

re: #15934 (review)

I understand begin(), end() and empty (for std::empty in the future) being lowercase, but shouldn’t it be LastNegated() and Negated() as Bitcoin’s PascalCase style requires?

“Requires” is strong. There’s a decent amount of variation here, and I prefer just being consistent inside a targeted and narrowly used helper class.

re: #15934 (review)

Is there a reason this needs to be a lambda? It’s not capturing any surrounding variables. Would it be clearer to just define this as a regular function above?

Good catch. Made it a reusable function now

nit: a comment here would be useful:

0// command_line_options do not have sections. Only iterate over SET and NEGATE

re: #15934 (review)

nit: a comment here would be useful:

0// command_line_options do not have sections. Only iterate over SET and NEGATE

Added comment

re: #15934 (review)

skip_nonpersistent inline comment is wrong. That positional argument is get_chain_name.

Good catch. This came from messing up a rebase of another PR

re: #15934 (review)

nit: I’d prefer all delimiters to be printed for each test case, so there’s a constant number of columns.

Added separator unconditionally now

Could you add more details here, like you have in the utils_tests.cpp:

0    //   <input> || GetSetting() | GetSettingsList() | HasIgnoredDefaultSectionConfigValue()

re: #15934 (review)

Could you add more details here, like you have in the utils_tests.cpp:

0    //   <input> || GetSetting() | GetSettingsList() | HasIgnoredDefaultSectionConfigValue()

Added comment

These functions (TestArgsManager::ParseParameters(), TestArgsManager::ReadConfigString() and TestArgsManager::UpdateSettings()) are pretty confusing. TestArgsManager is essentially storing its own view of the settings, and the tests are testing that view, rather than what ArgsManager itself is storing. UpdateSettings() is even weirder - it’s reaching into ArgsManager.m_settings and changing settings.

I think it’s ok to leave these in for now and fix up the tests in a future PR (which I’d be happy to review), but could you add some comments explaining what’s going on here?

re: #15934 (review)

These functions (TestArgsManager::ParseParameters(), TestArgsManager::ReadConfigString() and TestArgsManager::UpdateSettings()) are pretty confusing.

It was actually easy to update the tests and drop these so I got rid of them in the pr/mergeset.19 -> pr/mergeset.20 push (compare)

re: #15934 (review)

It wasn’t immediately obvious to me why fn() was being called on span even if hasn’t been set. Moving this line into the if block causes the tests to fail. Can you add a comment to explain why you’re calling this even if no settings were found in the top_level of the config file?

Got rid of this behavior now. This was an attempt to simplify HasIgnoredDefaultSectionConfigValue() by offloading complexity to MergeSetting(), but it just makes both functions less clear and was a bad idea.

re: #15934 (review)

nit: add comment “transform -foo to foo” (over-commenting parsing here doesn’t seem gross to me)

Agree, will add.

I can’t comment on the exact line above (L897), but that comment refers to ‘override args’ and so should be changed in this PR.

I think the comment and bool name are pretty confusing, and could be made much clearer. Suggestion:

 0        // `-includeconf` cannot be included in the command line arguments except
 1        // as `-noincludeconf` (which indicates that no conf file should be used).
 2        bool use_conf_file{true};
 3        {
 4            LOCK(cs_args);
 5            const std::vector<util::SettingsValue>* const includes = util::FindKey(m_settings.command_line_options, "includeconf");
 6            if (includes) {
 7                // ParseParameters() fails if a non-negated -includeconf is passed on the command-line
 8                assert(util::SettingsSpan(*includes).last_negated());
 9                use_conf_file = false;
10            }
11        }
12        if (use_conf_file) {

re: #15934 (review)

I can’t comment on the exact line above (L897), but that comment refers to ‘override args’ and so should be changed in this PR.

I think the comment and bool name are pretty confusing, and could be made much clearer. Suggestion:

Agree, will take your suggestion.

These variable names (which are unchanged by your PR) aren’t very helpful. I’d suggest the following renames for a later PR:

includeconf -> conf_file_names to_include -> conf_file_name include_conf -> conf_file_stream

re: #15934 (review)

These variable names (which are unchanged by your PR) aren’t very helpful. I’d suggest the following renames for a later PR:

includeconf -> conf_file_names to_include -> conf_file_name include_conf -> conf_file_stream

Yes. If it’s ok I think I will do these cleanups first and then rebase my changes on top.

re: #15934 (review)

(unchanged but touched by this PR): I don’t understand why this needs to be its own scope. Perhaps it can be flattened in a follow-up PR?

Probably this was just done as a style thing to make it clearer the variables here won’t be used after the scope. But there’s only one variable left now so this seems fine to remove.

re: #15934 (review)

nit: comment “Read section like testnet.foo or regtest.bar”

Will add this

re: #15934 (review)

For me, this struct doesn’t make things any clearer. An alternative is to pass an enum into the callback function to indicate where the SettingsSpan came from. Take a look at the change here, which also adds a few more comments: jnewbery@fc3e1d4

You’re right. Sorry I doubted you! Struct was actually a holdover from an earlier iteration of this that had other properties like the section name. I’ll squash in your change.

re: #15934 (review)

I got why we need IsArgSet("chain") can’t CBaseChainParams::MAIN be returned by default in this function ?

The result of IsArgSet isn’t needed to set the default, since that’s handled below. It’s only called to trigger an error if -regtest -testnet and -chain= options are used together because you’re only allowed to use one.

For example bitcoind -chain=main -regtest triggers Invalid combination of -regtest, -testnet and -chain. Can use at most one.'

I tested chain=0 and it start anyway on main chain..

I don’t see this. Was it intentional to omit the leading dash before -chain? When I start with -chain=0 I see Error: CreateBaseChainParams: Unknown chain 0. When I start with chain=0 I see Error: Command line contains unexpected token 'chain=0', see bitcoind -h for a list of options.

I don’t see this. Was it intentional to omit the leading dash before -chain?

Ah sorry I tested with chain=0 in config file, but didn’t remember that bitcoind was pretty liberal with non-sense in config file..

That’s said would it change anything to just return CBaseChainParams::MAIN at then of this function ? Assuming there is no conflict with other networks, if -chain isn’t set you’re going anyways to start on mainnet.

re: #15934 (review)

That’s said would it change anything to just return CBaseChainParams::MAIN at then of this function ? Assuming there is no conflict with other networks, if -chain isn’t set you’re going anyways to start on mainnet.

I might be misunderstanding the suggestion, but if the suggestion is to change return GetArg("-chain", CBaseChainParams::MAIN); to return CBaseChainParams::MAIN; then that will break usages bitcoind -chain=test or bitcoind -chain=regtest.

If the question is why the overlapping options exist in the first place, I don’t know the answer, but this was done in #16680, and the discussion there probably has some justifications.

re: #15934 (review)

I’m wondering if there you could return after first match, in AppInitParameterInteraction unsuitables are processed as errors and not warnings

Wow that is pretty funny. At one point I believe this was warning, and then it was changed into an error. I think it’s better to return the complete set here though, so we can improve the error message to mention all the unsuitables instead of just the first.

I agree. The:

0    for (const auto& arg : gArgs.GetUnsuitableSectionOnlyArgs()) {
1        return InitError(strprintf(_("Config setting for %s only applied on %s network when in [%s] section.").translated, arg, network, network));
2    }

in AppInitParameterInteraction() clearly doesn’t make sense.