Network specific conf sections

ajtowns commented at 10:06 am on December 11, 2017: member

The weekly meeting on 2017-12-07 discussed allowing options to bitcoin to have some sensitivity to what network is in use. @theuni suggested having sections in the config file:

<cfields> an alternative to that would be sections in a config file. and on the
          cmdline they'd look like namespaces. so, [testnet] port=5. or -testnet::port=5.

This approach is (more or less) supported by boost::program_options::detail::config_file_iterator – when it sees a [testnet] section with port=5, it will treat that the same as “testnet.port=5”. So [testnet] port=5 (or testnet.port=5 without the section header) in bitcoin.conf and -testnet.port=5 on the command line.

The other aspect to this question is possibly limiting some options so that there is no possibility of accidental cross-contamination across networks. For example, if you’re using a particular wallet.dat on mainnet, you may not want to accidentally use the same wallet on testnet and risk reusing keys.

I’ve set this up so that the -addnode and -wallet options are NETWORK_ONLY, so that if you have a bitcoin.conf:

wallet=/secret/wallet.dat
upnp=1

and you run bitcoind -testnet or bitcoind -regtest, then the wallet= setting will be ignored, and should behave as if your bitcoin.conf had specified:

upnp=1

[main]
wallet=/secret/wallet.dat

For any NETWORK_ONLY options, if you’re using -testnet or -regtest, you’ll have to add the prefix to any command line options. This was necessary for multiwallet.py for instance.

I’ve left the “default” options as taking precedence over network specific ones, which might be backwards. So if you have:

maxmempool=200
[regtest]
maxmempool=100

your maxmempool will still be 200 on regtest. The advantage of doing it this way is that if you have [regtest] maxmempool=100 in bitcoin.conf, and then say bitcoind -regtest -maxmempool=200, the same result is probably in line with what you expect…

The other thing to note is that I’m using the chain names from chainparamsbase.cpp / ChainNameFromCommandLine, so the sections are [main], [test] and [regtest]; not [mainnet] or [testnet] as might be expected.

Thoughts? Ping @MeshCollider @laanwj @jonasschnelli @morcos

fanquake added the label Brainstorming on Dec 11, 2017

meshcollider commented at 10:33 am on December 11, 2017: contributor

Ah you beat me to it, will take a look as soon as I can

Early note: as you mention, net-specific should take precedence over default, whereas explicit command line arguments should always take precedence over the config file, so following your example, if -maxmempool=200 is specified as an argument it should use 200 even on regtest, whereas if it is not, the regtest.maxmempool=100 should override the default 200. Then if -regtest.maxmempool=100 is an explicit argument as well as -maxmempool=200, the explicit regtest one should take precedence IMO.

Then following this logic, -wallet specified explicitly as an argument should be used on regtest and testnet as well, using regtest.wallet should only be necessary in the config file to save confusion

jnewbery commented at 3:16 pm on December 11, 2017: member

Great. Thanks for tackling this @ajtowns. A few high-level suggestions:

I agree with @MeshCollider that the order of precedence should be Command Line > Config File network-specific > Config File default.
I think there shouldn’t be network-specific command-line options (that’s almost an implication of the above). I agree with MeshCollider that the user should call bitcoind with -wallet, not -regtest.wallet (side-note - choosing a wallet from the wrong network is perhaps less of a problem than you might assume, since -wallet is already kind of network-specific as it refers to the name of the wallet in the particular network directory).
as well as NETWORK_ONLY, perhaps we want a DEFAULT_ONLY for options that shouldn’t be overriden in the network-specific config. Namely those options would be -regtest and -testnet. For example don’t want the following pathological config file to be valid:

0testnet
1[testnet]
2regtest
3[regtest]
4notestnet
5noregtest

taking this further, perhaps we should deprecate the use of testnet and regtest in the config file entirely, so you can only select the network by using a command-line option?
I’ve avoided nitting your code at this early point, but I’d point you at the developer notes here: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md, specifically symbol naming. Your new ArgsManager members should be prefixed m_ for example.

jonasschnelli commented at 6:56 pm on December 11, 2017: contributor

Concept ACK. Nice work!

Agree with @MeshCollider and @jnewbery (except the point of deprecating the use of testnet/regtest in config file).

ajtowns commented at 10:22 am on December 12, 2017: member

Okay, getting that behaviour requires marginally more invasive changes, but I think it’s worth it (and they’re not that invasive). New patch series accompanying this comment which:

has arg precedence being: command-line arguments, net-specific config file options, default config file options
will only use config file options for -addnode and -wallet if you’re on mainnet or put them in the right [regtest] or [testnet] section or add a regtest. or testnet. prefix
only allows -testnet and -regtest to be specified in the default section or on the command line (ie, [regtest] testnet=1 silliness is silently ignored)
for single-value options, precedence is (a) last value specified on the command line, (b) first value specified in the network section of the config file, and finally (c) first value specified in the default section of the config file. the last vs first difference between cli and conf is weird, but matches current behaviour. the code would be slightly improved by making it consistent.

I think that should behave pretty much as expected – specifying things on the command line will always take effect, having a testnet specific config and just invoking “bitcoind -conf=testnet.conf” should work fine, having separate configs for mainnet, regtest and testnet in a single file with different options should work too.

From the code side of things, I’ve made a bunch of changes:

I’ve dropped mapArgs, and split mapMultiArgs into mapOverrideArgs (for cli arguments are ForceSetArg) and mapConfigArgs (for config files)
I’ve listed the net-specific options explicitly in util.cpp rather than having each use of the options specify that they’re net-specific; seems simpler and much more robust.
I’ve moved ChainNameFromCommandLine from chainparamsbase into util, so it can have more precise access to how -testnet/-regtest were specified
I’ve dropped the filename argument from ReadConfigFile and split most of the logic into ReadConfigStream, which makes it more unit-testable

I’ve added some unit tests, but they’re a bit basic; I haven’t done a functional test either. (And it’s still a bit early for detailed code style nits, though I’ve tried to make a token effort to avoid them :) )

Edit: Oh, the previous pass is commit 907dd64994e945a30833200ce1131840b06c8ab7 at https://github.com/ajtowns/bitcoin/tree/netconf-sections-v1 on the off chance someone wants to look at it.

ajtowns force-pushed on Dec 12, 2017

ajtowns force-pushed on Jan 2, 2018

ajtowns force-pushed on Jan 3, 2018

ajtowns commented at 5:46 am on January 3, 2018: member

Okay, here’s something for first pass reviews I think.

The first five commits do some refactoring on ChainNameFromCommandLine (moving it from chainparamsbase.cpp into util.cpp:ArgsManager) and ReadConfigFile, primarily to make it easier to add tests for both of them.

c5b5997e1 Move ChainNameFromCommandLine into ArgsManager
eedd60951 [tests] Add unit tests for ChainNameFromCommandLine
f1665f160 Separate out ReadConfigStream from ReadConfigFile
0e78382c3 [tests] Add unit tests for ReadConfigStream
d4367e1c6 [tests] Check ChainNameFromCommandLine works with config entries

The next commit switches from mapArgs and mapMultiArgs (for single and multi value options) to mapConfigArgs and mapOverrideArgs (for config file options and commandline/forceset options).

bdb0c2857 ArgsManager: keep command line and config file arguments separate

The next commit is the first one that should change behaviour, and does most of the work. Unit test, and update to functional test are in the following commits.

0e6679466 ArgManager: add support for config sections
7a51198d3 [tests] Unit tests for config file sections
359c78e2e [tests] Use regtest section in functional tests configs

The next commit changes the behaviour of the addnode, connect, port, bind, rpcport, rpcbind and wallet options: when they’re specified in the config file they’ll only apply to mainnet unless you use a [regtest] or [test] header (or a “regtest.” or “test.” prefix).

7be1ddd31 ArgsManager: limit some options to only apply on mainnet when in default section
c031709db [tests] Unit tests for section-specific config entries

Penultimately, there’s special handling for the -regtest and -testnet options, so that ChainNameFromCommandLine will ignore bogus entries like “[regtest] testnet=1” or “[regtest] noregtest” and won’t return weird results if invoked after the initial call to Select(Base)Params() with a pathological config file like that. cf jnewbery’s comments in #11862 (comment)

2933f6016 ArgsManager: special handling for -regtest and -testnet
22da57f19 [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections

And finally there’s also an update for the release notes.

ee91028e7 Add config changes to release notes

Notes:

I’ve called the member variables things like “m_mapConfigArgs” rather than fully snake-case map_config_args or similar as a compromise between the coding style and what’s already in use in the class.

I needed a few helper functions with access to protected ArgsManager member variables; rather than add them to util.h directly, I made a friend class for them, ArgsManagerHelper. It’s not crazy pretty but it seems alright. Maybe they should go in util_helper.h or similar though?

I got the locks wrong for cs_args and csPathCached initially – was tempted to add “AssetLockNotHeld(gArgs.cs_args)” to GetDataDir to help avoid this, but it runs into two problems: in some threads GetDataDir() is called before any locks are acquired leaving lockstack==nullptr and causing a segfault (which can easily be avoided by just adding the assert after LOCK(csPathCached)); and cs_args is protected so it would’ve required another ArgsManagerHelper functions.

The datadir option is a little tricky. Currently you have three options:

don’t specify -datadir anywhere, just use the default
specify -datadir on the command line
specify -datadir in the config file only (the config file may be located via the default datadir)

With these patches it would be possible to have a fourth option: specify datadir in the applicable network section of the configfile, allowing a single config file to specify different datadirs for regtest and testnet. At present the code locks datadir into place before selecting the network, so that hasn’t been implemented in this PR.

The fact that -addnode, -connect, etc only apply to main net is specified in util.cpp rather than in the files that care about the options. This is something that can be cleaned up in the rework of argument handling that @MeshCollider has been working on.

I think the approach makes sense at this point, so reviews appreciated and nits welcome I guess!

ajtowns renamed this:
~~[concept] Network specific conf sections~~
[wip] Network specific conf sections
on Jan 3, 2018

ajtowns closed this on Mar 5, 2018

ajtowns deleted the branch on Mar 5, 2018

ajtowns restored the branch on Mar 5, 2018

ajtowns reopened this on Mar 5, 2018

ajtowns force-pushed on Mar 5, 2018

laanwj commented at 7:30 pm on March 5, 2018: member

Was already wondering why this was closed!

Edit: overall looks ok to me! had some small comments in person, but nothing major. Will test this.

meshcollider commented at 7:22 pm on March 6, 2018: contributor

I think this is far enough along to have the brainstorming tag removed because at least to me it feels almost unready to be reviewed if it has that tag, more of a high-level discussion thing.

Definite Concept ACK if I haven’t already said so. I’ll review soon.

For reference, this also supersedes the issue #9374

in src/util.cpp:772 in bcab6fa16a outdated

772+    AssertLockNotHeld(cs_args);
773 
774-    {
775+    if (!streamConfig.good()) {
776+        return; // No bitcoin.conf file is OK
777+    } else {

jnewbery commented at 10:46 pm on March 6, 2018:

This else isn’t required. You can just drop through after the if block. (in the commit 4f3766f7d you can remove the code unit from around the LOCK in ReadConfigStream. It was only there so the lock was released before the call to ClearDatadirCache().

in src/util.cpp:443 in bcab6fa16a outdated

434@@ -435,11 +435,116 @@ static void InterpretNegativeSetting(std::string& strKey, std::string& strValue)
435     }
436 }
437 
438+/** Internal helper functions for ArgsManager */
439+class ArgsManagerHelper {
440+public:
441+    inline static bool UseDefaultSection(const ArgsManager& am, const std::string& strArg)
442+    {
443+        if (am.m_strSection == CBaseChainParams::MAIN)

jnewbery commented at 11:26 pm on March 6, 2018:

style nit: if clauses should either be on the same line as the if conditional, or with braces (https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md)

jnewbery commented at 11:27 pm on March 6, 2018: member

I’ve reviewed up to 940e5bc61f861aae97983d4be52ff87c101e29cc. A couple of comments inline.

in src/util.cpp:459 in bcab6fa16a outdated

434@@ -435,11 +435,116 @@ static void InterpretNegativeSetting(std::string& strKey, std::string& strValue)
435     }
436 }
437 
438+/** Internal helper functions for ArgsManager */
439+class ArgsManagerHelper {

jnewbery commented at 2:16 pm on March 7, 2018:

Is there any reason not to make this a namespace, rather than a class with a bunch of static functions?

ajtowns commented at 4:58 pm on March 12, 2018:

Yeah: It’s a class so that it can be declared a friend of ArgsManager, which allows the functions in the class to access private/protected members of ArgsManager.

These functions could just be private member functions of ArgsManager, but that would mean bumping util.h every time any of them need to be changed, which causes a lot of unnecessary recompilation.

in src/util.cpp:441 in bcab6fa16a outdated

434@@ -435,11 +435,116 @@ static void InterpretNegativeSetting(std::string& strKey, std::string& strValue)
435     }
436 }
437 
438+/** Internal helper functions for ArgsManager */
439+class ArgsManagerHelper {
440+public:
441+    inline static bool UseDefaultSection(const ArgsManager& am, const std::string& strArg)

jnewbery commented at 2:17 pm on March 7, 2018:

style nit: new code should use snake_case for arguments and variables.

jnewbery commented at 3:02 pm on March 7, 2018:

Perhaps add a comment for why we need this function (and another comment lower down next to m_setSectionOnlyArgs explaining why those particular arguments shouldn’t be applied to non-mainnet configs by default).

in test/functional/feature_config_args.py:34 in bcab6fa16a outdated

28@@ -29,8 +29,14 @@ def run_test(self):
29 
30         # Check that using non-existent datadir in conf file fails
31         conf_file = os.path.join(default_data_dir, "bitcoin.conf")
32-        with open(conf_file, 'a', encoding='utf8') as f:
33+
34+        # datadir needs to be set before [regtest] section
35+        conf_file_contents = open(conf_file, encoding='utf8').readlines()

jnewbery commented at 2:50 pm on March 7, 2018:

can replace the readlines() with read() and the write below with f.write("datadir=" + new_data_dir + "\n" + conf_file_contents). That’s a bit more compact, but both are fine.

in src/util.cpp:528 in bcab6fa16a outdated

523+        }
524+        return InterpretBool(v); // is set, so evaluate
525+    }
526+};
527+
528+ArgsManager::ArgsManager(void) :

jnewbery commented at 3:09 pm on March 7, 2018:

Does it makes more sense to have this as a file-level constant rather than a member of ArgsManager?

ajtowns commented at 2:20 pm on March 28, 2018:

Having it be a member of ArgsManager makes it easier to deal with unit tests. But slightly longer term, they should be file-level constants in the files that use the arguments, rather than util.cpp; but that’s future work.

in src/util.cpp:456 in bcab6fa16a outdated

451+    typedef std::map<std::string, std::vector<std::string>> map_args;
452+
453+    /** Convert regular argument into the section-specific setting */
454+    inline static std::string SectionArg(const ArgsManager& am, const std::string& strArg)
455+    {
456+        assert(strArg.length() > 1 && strArg[0] == '-');

jnewbery commented at 3:21 pm on March 7, 2018:

This assert can be hit. Try running feature_pruning.py for example:

0bitcoind: util.cpp:456: static std::__cxx11::string ArgsManagerHelper::SectionArg(const ArgsManager&, const string&): Assertion `strArg.length() > 1 && strArg[0] == '-'' failed.

ajtowns commented at 5:35 pm on March 7, 2018:

I believe this is catching a legitimate bug in the caller; confirming at the minute.

Edit: Yeah it’s a legit bug as far as I can see – #12640

in src/util.cpp:505 in bcab6fa16a outdated

500+        if (!am.m_strSection.empty()) {
501+            if (GetArgHelper(result, am.m_mapConfigArgs, SectionArg(am, strArg)))
502+                return true;
503+        }
504+
505+        if (UseDefaultSection(am, strArg)) {

jnewbery commented at 3:30 pm on March 7, 2018:

Is it possible to log when a default section config is not used? It could be confusing for users who specify -addnode, -connect, etc in the default section, expecting the config to be picked up.

ajtowns commented at 5:36 pm on March 7, 2018:

As in “Warning: you’re running on testnet, so your -addnode setting didn’t get applied” ?

laanwj commented at 5:38 pm on March 7, 2018:

Or “Warning: Setting X specified outside configuration section is only applied on mainnet”

jnewbery commented at 3:31 pm on March 7, 2018: member

A bunch of review comments inline. The new assert in SectionArg() can be hit, causing the node to crash.

I also think it would be a good idea to update the help text for -addnode, -connect to explain that they’re not used for non-mainnet config unless explicitly specified.

Note that this can be a breaking api change for users who have -addnode, -connect, etc in their config files for testnet and regtest.

laanwj added this to the milestone 0.17.0 on Mar 22, 2018

laanwj commented at 8:49 am on March 22, 2018: member

I think this is far enough along to have the brainstorming tag removed because at least to me it feels almost unready to be reviewed if it has that tag, more of a high-level discussion thing.

I tend to agree. Let’s remove the WIP tag and try to get this in for 0.17.0. As I understand it, the issues left are pretty much documentation and message related.

jnewbery commented at 2:34 pm on March 22, 2018: member

Let’s … try to get this in for 0.17.0

Current implementation is blocked on issue #12640 , which is fixed in #12756. Please review that (simple) PR if you want this to be merged :)

jnewbery commented at 2:36 pm on March 26, 2018: member

#12756 is in master and I wanted to test this, so I rebased. There was a trivial merge conflict in feature_config_args.py. It might be quicker if you grab my rebased branch here: https://github.com/jnewbery/bitcoin/tree/pr11862.1

I can confirm with #12756, we no longer crash in any of the functional tests.

I think there are still a few review comments for you to address.

ajtowns force-pushed on Mar 28, 2018

ajtowns renamed this:
~~[wip] Network specific conf sections~~
Network specific conf sections
on Mar 28, 2018

ajtowns commented at 3:19 pm on March 28, 2018: member

Should address most of the comments, including the style nits.

I haven’t updated the help for -addnode or -connect; not sure what to update it to… Also have not added a log message warning when no config option has been found despite a config option being present in the default section. Do have an idea how to do that, so may add it tomorrow.

Dropped [wip] from the subject; someone might want to drop the brainstorming label, ‘cause I can’t. :)

jnewbery added the label Feature on Mar 28, 2018

jnewbery removed the label Brainstorming on Mar 28, 2018

jnewbery commented at 3:40 pm on March 28, 2018: member

may add it tomorrow.

I’ll review once you’ve decided how to tackle this :)

ajtowns force-pushed on Mar 29, 2018

ajtowns commented at 7:05 am on March 29, 2018: member

Took the opportunity to bump the dates on the commits to encourage github to list them in the right order.

It now gives a warning at startup if you’re on testnet or regtest, have one of the section-only arguments in the default section of the config file, and haven’t overridden that argument either on the command line or in the appropriate config section. The warnings are checked via InitParameterInteraction() to avoid updating multiple places and it’s kind-of a parameter interaction.

travis failures seem to just be the currently expected ones – mingw32 tests timing out, and the one fixed by #12821.

ajtowns force-pushed on Apr 4, 2018

ajtowns commented at 8:36 am on April 4, 2018: member

Rebased again. #12713 made this a bit more complicated – how negated options (-nofoo) work with the config files is (in my opinion) a bit confusing as implemented, and coping with negated options and sections makes it a bit more confusing. As a consequence, I’ve broken out the first set of refactoring/testing patches into its on PR (#12878) which includes some additional tests for the current behaviour of negated options. For this PR, the way I’ve handled things is to make “-nofoo” empty the vector of values for “foo”, and to consider an empty vector an indication that the arg is negated. I think that ends up behaving reasonably logically, and the changes are hopefully indicated in the corresponding unit tests.

EDIT: bah, github ordering is wrong, i’ll fix the dates and bump the PR again. fixed and bumped.

ajtowns force-pushed on Apr 4, 2018

jnewbery commented at 3:12 pm on April 4, 2018: member

I’ve reviewed #12878. Will review this once that PR is merged and this is rebased.

jtimon commented at 8:12 pm on April 5, 2018: contributor

Concept ACK. Right now, If I introduce a regtest2 section, for example, it will simply be ignored, correct? I’m wondering how this will interact with https://github.com/bitcoin/bitcoin/pull/8994

jonasschnelli referenced this in commit 25c56cdbe7 on Apr 8, 2018

ajtowns force-pushed on Apr 9, 2018

ajtowns commented at 6:48 am on April 9, 2018: member

@jtimon If you put a [regtest2] section in bitcoind.conf today, it will be ignored – under the hood the options are just prefixed with the section, so you’d just be setting -regtest2.foo=1 which would never get looked up. I think as far as #8994 goes, the call to SelectConfigSection() that this PR adds inside SelectBaseParams() should keep the config section used (by this PR) in sync with the chain used (by your PR), so things should work out okay.

ajtowns commented at 6:50 am on April 9, 2018: member

Rebased post #12878 merge.

in src/util.cpp:504 in 88ec08ddeb outdated

499+        } else {
500+            return std::make_pair(true, it->second.front());
501+        }
502+    }
503+
504+    /* Get the string value of an argument, returning true if found, or

jnewbery commented at 3:29 pm on April 9, 2018:

Comment is wrong. This function returns a std::pair

in src/util.cpp:516 in 88ec08ddeb outdated

507+    static inline std::pair<bool,std::string> GetArg(const ArgsManager &am, const std::string& arg)
508+    {
509+        LOCK(am.cs_args);
510+        std::pair<bool,std::string> found_result(false, std::string());
511+
512+        found_result = GetArgHelper(am.m_override_args, arg, true);

jnewbery commented at 3:30 pm on April 9, 2018:

Can you add a comment why you want the last item from m_override_args, but the first item from m_config_args below?

ajtowns commented at 9:04 am on April 10, 2018:

I’m not sure; the reason I’m doing it is backwards compatibility. The original reason might have just been a side-effect of using a single variable and updating it every time you see a command line argument, but only if it’s not already set when you see a config file argument. I’ll add a comment that you can re-nit though. :)

jnewbery commented at 7:45 pm on April 12, 2018:

comment seems good, no nits from me :)

in src/util.cpp:717 in 88ec08ddeb outdated

726-    auto it = mapArgs.find(strArg);
727-    if (it != mapArgs.end()) return it->second;
728-    return strDefault;
729+    if (IsArgNegated(strArg)) return "0";
730+    std::pair<bool,std::string> found_res = ArgsManagerHelper::GetArg(*this, strArg);
731+    if (found_res.first) {

jnewbery commented at 3:38 pm on April 9, 2018:

I prefer the more compact representation that was here before:

0if (thing) return something;
1return something_else;

than:

0if (thing) {
1    return something;
2} else {
3    return something_else;
4}

I believe both are acceptable in our style guide.

This is just personal preference though, so feel free to ignore.

ajtowns commented at 9:08 am on April 10, 2018:

Yeah, that style is a bit more consistent with other parts of the file.

in src/util.cpp:950 in 88ec08ddeb outdated

942 
943 void ArgsManager::ReadConfigFile(const std::string& confPath)
944 {
945+    {
946+        LOCK(cs_args);
947+        m_config_args.clear();

jnewbery commented at 3:54 pm on April 9, 2018:

I don’t understand why this needs to move from ParseParameters(). It seems needlessly restrictive that m_config_args is cleared each time that ReadConfigFile() (and will need to be removed for #10267 for example).

ajtowns commented at 9:13 am on April 10, 2018:

I was thinking #10267 would involve an additional call to ReadConfigStream() from within ReadConfigFile() (ie, you read the config file, find out what additional file(s) to include, then read those, and possibly repeat). I think it makes more sense for ParseParams() to only clear the member variable it populates, and ReadConfigFile() to clear the member variable it populates. If a later PR wants to change this that’s fine, but at least for now it seems fine here to me.

in src/util.cpp:578 in 88ec08ddeb outdated

575         if (!bool_val ) {
576             // Double negatives like -nofoo=0 are supported (but discouraged)
577             LogPrintf("Warning: parsed potentially confusing double-negative %s=%s\n", key, val);
578+            val = "1";
579+        } else {
580+            val = "0";

jnewbery commented at 5:34 pm on April 9, 2018:

This val is never used (callers to InterpretNegatedOption() just clear the m_override_options map if the function returns true). Suggest you don’t set it here.

in src/util.cpp:589 in 88ec08ddeb outdated

567+    if (option_index == std::string::npos) {
568+        option_index = 1;
569+    } else {
570+        ++option_index;
571+    }
572+    if (key.substr(option_index, 2) == "no") {

jnewbery commented at 5:39 pm on April 9, 2018:

nit: I have a preference for early exit instead of deeply nested ifs (ie you could change this to:

 0    if (key.substr(option_index, 2) != "no") return false;
 1
 2    bool bool_val = InterpretBool(val);
 3    key.erase(option_index, 2);
 4    if (!bool_val ) {
 5        // Double negatives like -nofoo=0 are supported (but discouraged)
 6        LogPrintf("Warning: parsed potentially confusing double-negative %s=%s\n", key, val);
 7        val = "1";
 8        return false;
 9    }
10    return true;

)

Again, this is personal preference. Feel free to ignore.

ajtowns commented at 12:37 pm on April 11, 2018:

Fair point, but I’m going to choose to ignore it :)

in src/util.cpp:660 in 88ec08ddeb outdated

657@@ -511,55 +658,89 @@ void ArgsManager::ParseParameters(int argc, const char* const argv[])
658             key.erase(0, 1);
659 
660         // Transform -nofoo to -foo=0

jnewbery commented at 5:41 pm on April 9, 2018:

This comment isn’t accurate anymore. Remove.

in src/test/util_tests.cpp:296 in 88ec08ddeb outdated

293+    // and the value.
294     BOOST_CHECK(!testArgs.IsArgNegated("-foo"));
295-    BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == "0");
296+    BOOST_CHECK(testArgs.GetArg("-foo", "xxx") == "1");
297 
298     // A double negative is a positive.

jnewbery commented at 5:45 pm on April 9, 2018:

Remove duplicate line

in src/chainparamsbase.cpp:51 in 88ec08ddeb outdated

47@@ -48,4 +48,5 @@ std::unique_ptr<CBaseChainParams> CreateBaseChainParams(const std::string& chain
48 void SelectBaseParams(const std::string& chain)
49 {
50     globalChainBaseParams = CreateBaseChainParams(chain);
51+    gArgs.SelectConfigSection(chain);

jnewbery commented at 6:31 pm on April 9, 2018:

I’m not sure if it makes sense to call this from SelectBaseParams(). This function is about setting the blockchain parameters, not the global node config.

Doesn’t it make sense just to set gArgs.m_section directly at the end of ReadConfigFile()?

ajtowns commented at 9:30 am on April 10, 2018:

I don’t think so – ReadConfigFile() only gets to know -testnet was set, you need to call GetChainName() to work out that means chain=="test", and the place that’s currently called is when passing the arguments to SelectParams or SelectBaseParams. Also, SelectBaseParams is already updating globalChainBaseParams which seems like “global node config” to me. :)

Changing that might not be a bad idea (and I think it might benefit jtimon’s #8994), but I think it could wait for a different PR?

jnewbery commented at 8:43 pm on April 12, 2018:

fine. Can wait for a future PR.

in src/util.cpp:465 in 88ec08ddeb outdated

460+public:
461+    typedef std::map<std::string, std::vector<std::string>> MapArgs;
462+
463+    /** Determine whether to use config settings in the default section,
464+     *  See also comments around ArgsManager::ArgsManager() below. */
465+    inline static bool UseDefaultSection(const ArgsManager& am, const std::string& arg)

jnewbery commented at 6:36 pm on April 9, 2018:

nanonit: use inline static or static inline consistently :)

in src/util.h:232 in 88ec08ddeb outdated

230-    std::map<std::string, std::string> mapArgs;
231-    std::map<std::string, std::vector<std::string>> mapMultiArgs;
232-    std::unordered_set<std::string> m_negated_args;
233+    std::map<std::string, std::vector<std::string>> m_override_args;
234+    std::map<std::string, std::vector<std::string>> m_config_args;
235+    std::string m_section;

jnewbery commented at 6:39 pm on April 9, 2018:

I think this variable could be named better (and commented). If I’m not mistaken, this is the network that the node has been configured to run on (mainnet, testnet or regtest). Would a better name be m_network?

in src/util.cpp:608 in 88ec08ddeb outdated

612+    // if there's no section selected, don't worry
613+    if (m_section.empty()) return;
614+
615+    for (const auto& arg : m_section_only_args) {
616+        // if it's okay to use the default section for this chain, don't worry
617+        if (ArgsManagerHelper::UseDefaultSection(*this, arg)) return;

jnewbery commented at 6:46 pm on April 9, 2018:

This will only return true if am.m_section == CBaseChainParams::MAIN. Why don’t you explictly check for that above the loop and return?

in src/test/util_tests.cpp:473 in 88ec08ddeb outdated

578+    BOOST_CHECK_EQUAL(test_args.GetChainName(), "test");
579 
580-    test_args.ParseParameters(2, (char**)argv_regtest);
581+    test_args.ParseParameters(2, (char**)argv_testnet);
582     test_args.ReadConfigString(testnetconf);
583-    BOOST_CHECK_THROW(test_args.GetChainName(), std::runtime_error);

jnewbery commented at 6:51 pm on April 9, 2018:

You’ve removed this test that GetChainName() will throw if both regtest and testnet have been set. Is that intentional?

ajtowns commented at 1:00 pm on April 10, 2018:

No, I think that was a mistake while rebasing.

jnewbery commented at 6:52 pm on April 9, 2018: member

Looks really good. A bunch of nits and suggestions inline.

ArgsManager: keep command line and config file arguments separate 3673ca36ef

ajtowns force-pushed on Apr 11, 2018

ajtowns commented at 12:43 pm on April 11, 2018: member

Fixed a bunch of @jnewbery’s nits/complaints/suggestions. Rebased against master to pick up the automated test improvements.

For anyone who got part way through reviewing, piecemeal changes from then to now are on https://github.com/ajtowns/bitcoin/commits/netconf-sections-track with the same underlying tree:

0$ for a in netconf-sections{,-track}; do git log -1 --pretty="%h -> %T" $a; done
1c25321ff96 -> 4265c57b9695d653dc825116f39e773c5e0a1ea3
21600fc69aa -> 4265c57b9695d653dc825116f39e773c5e0a1ea3

EDIT: oops, missed a trailing space

ArgsManager: drop m_negated_args

When a -nofoo option is seen, instead of adding it to a separate
set of negated args, set the arg as being an empty vector of strings.

This changes the behaviour in some ways:
 - -nofoo=0 still sets foo=1 but no longer treats it as a negated arg
 - -nofoo=1 -foo=2 has GetArgs() return [2] rather than [2,0]
 - "foo=2 \n -nofoo=1" in a config file no longer returns [2,0], just [0]
 - GetArgs returns an empty vector for negated args

4d34fcc713

ArgsManager: support config file sections 95eb66d584

[tests] Unit tests for config file sections 30f94074c8

[tests] Use regtest section in functional tests configs 8a9817d175

ArgsManager: limit some options to only apply on mainnet when in default section

When specified in bitcoin.conf without using the [regtest] or [test]
section header, or a "regtest." or "test." prefix, the "addnode",
"connect", "port", "bind", "rpcport", "rpcbind", and "wallet" settings
will only be applied when running on mainnet.

d1fc4d95af

ArgsManager: Warn when ignoring network-specific config setting

When network-specific options such as -addnode, -connect, etc are
specified in the default section of the config file, but that setting is
ignored due to testnet or regtest being in use, and it is not overridden
by either a command line option or a setting in the [regtest] or [test]
section of the config file, a warning is added to the log, eg:

  Warning: Config setting for -connect only applied on regtest network when in [regtest] section.

68797e20f4

[tests] Unit tests for network-specific config entries 608415d4e6

ArgsManager: special handling for -regtest and -testnet 005ad26649

[tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections 5e3cbe020d

Add config changes to release notes c25321ff96

ajtowns force-pushed on Apr 11, 2018

jnewbery commented at 9:08 pm on April 12, 2018: member

utACK c25321ff96737bdba80d626d2425ef02c7a4c181. Thanks for addressing all of my nits!

laanwj commented at 6:56 pm on April 13, 2018: member

I’ve tested this a bit and it appears to work as expected. I’ve tested that it’s possible to override the port, and proxy configuration per network. I’ve also tested that putting testnet=1 in the general section still works.

utACK c25321ff96737bdba80d626d2425ef02c7a4c181

laanwj merged this on Apr 16, 2018

laanwj closed this on Apr 16, 2018

laanwj referenced this in commit 4366f61cc9 on Apr 16, 2018

in src/util.cpp:497 in 3673ca36ef outdated

492+     * if it was found (or the empty string if not found).
493+     */
494+    static inline std::pair<bool,std::string> GetArg(const ArgsManager &am, const std::string& arg)
495+    {
496+        LOCK(am.cs_args);
497+        std::pair<bool,std::string> found_result(false, std::string());

ryanofsky commented at 8:13 pm on April 18, 2018:

boost::optional<std::string> would be a more natural way to represent this.

ajtowns commented at 9:21 pm on April 18, 2018:

Doh, I thought about using std::optional, but it’s post-C++11; didn’t think to check for the boost variant, and it’s even already in use in util.h.

in src/util.cpp:584 in 4d34fcc713 outdated

583-        InterpretNegatedOption(key, val);
584-
585-        m_override_args[key].push_back(val);
586+        // Check for -nofoo
587+        if (InterpretNegatedOption(key, val)) {
588+            m_override_args[key].clear();

ryanofsky commented at 8:18 pm on April 18, 2018:

Shouldn’t a negated command line argument clear m_config_args too? As an example, if you had arg=a arg=b in config and -arg=c -noarg -arg=d on the command line, it would seem weird for the result to be [a b d] instead of [d].

ajtowns commented at 9:32 pm on April 18, 2018:

ParseParameters is always called prior to ReadConfigFile. Might be worth noting somewhere that you have to do things that way (though it isn’t really a choice: if you specify the config file via -conf, you don’t know what file to pass to ReadConfigFile until you’ve called ParseParameters).

ryanofsky commented at 3:50 pm on April 19, 2018:

#11862 (review)

ParseParameters is always called prior to ReadConfigFile

So this means the change I suggested wouldn’t actually fix the bug. But you agree this is introducing a bug that wasn’t in @eklitzke’s initial implementation from #12713?

ryanofsky commented at 3:59 pm on April 19, 2018:

But you agree this is introducing a bug

Never mind, this bug is actually present in #12713 too because of the initialization order. I guess I thought negated option tracking would provide a clean way to override config file list options instead of just appending to them, but I guess this isn’t the way it works.

in src/util.cpp:586 in 4d34fcc713 outdated

585-        m_override_args[key].push_back(val);
586+        // Check for -nofoo
587+        if (InterpretNegatedOption(key, val)) {
588+            m_override_args[key].clear();
589+        } else {
590+            m_override_args[key].push_back(val);

ryanofsky commented at 8:28 pm on April 18, 2018:

emplace_back(std::move(val)) would be a little more efficient

in src/util.cpp:578 in 95eb66d584 outdated

574@@ -553,6 +575,11 @@ static bool InterpretNegatedOption(std::string& key, std::string& val)
575     return false;
576 }
577 
578+void ArgsManager::SelectConfigNetwork(const std::string& network)

ryanofsky commented at 8:36 pm on April 18, 2018:

Would be slightly more efficiecnt to take a normal (non-reference) string argument, and std::move() it below to avoid a copy.

in src/util.cpp:554 in 005ad26649 outdated

549+            found_result = GetArgHelper(am.m_config_args, net_arg, true);
550+            if (!found_result.first) {
551+                return false; // not set
552+            }
553+        }
554+        return InterpretBool(found_result.second); // is set, so evaluate

ryanofsky commented at 8:53 pm on April 18, 2018:

It seems like code above this line is duplicating code in ArgsManagerHelper::GetArg in slightly inconsistent ways (passing getLast [true, true] to the two GetArgHelper calls instead of [true, false] and failing to acquire cs_args). Is there a reason for these inconsistencies and for duplicating code here instead of calling getArgs?

ajtowns commented at 9:27 pm on April 18, 2018:

Using ArgsManagerHelper::GetArg here would mean that calling GetChainName() a second (or third or..) time after SelectParams(GetChainName()) could return a different result than the name of the chain actually being used if people do ridiculous things with their bitcoin.conf.

This should get cleaned up with some of the other config reworking that’s going on though.

ajtowns commented at 9:37 pm on April 18, 2018:

I think GetArgHelper(m_config_args, net_arg, true) probably should have been , false), well spotted.

NicolasDorier commented at 5:14 am on October 11, 2018: contributor

I saw that too late but this PR is a major pain in the ass. Please be more careful with breaking changes, especially there was no good reason to break anything.

Not only it broke existing configuration file, but on top of it it call the testnet section [test] instead of [testnet] .

This mean I can’t easily migrate such templated configuration without doing major refactoring, or hack in the entrypoint of my docker compose to replace [test] by [testnet].

zciendor commented at 5:04 pm on October 23, 2018: none

Is there even a documentation of the section names? What we have used so far as pseudo-documentation is the config generator from @jlopp https://jlopp.github.io/bitcoin-core-config-generator/ However, it neither contains a [general] section like mentioned in #11862 (comment) and places the testnet=1 option currently under the [debug] section. This doesn’t work anymore and bitcoind seems to completely ignore the testnet=1 option if it’s under the [debug] section.

We second @NicolasDorier, 0.17 breaks our templated approach and install assistant as well, which we use on the zHIVErbox - a low cost, TAILS-inspired security fortess. Looks like a refactoring is needed to support 0.17, which might be OK if it’s backwards compatible and the config templates still work on <0.17? But if we need to maintain separate templates for different bitcoin versions it get’s ugly.

ryanofsky commented at 5:35 pm on October 23, 2018: member

Unless I’m mistaken, I think we can restore complete backwards compatibility by just not having “network-only” options, and only warning when one of these options is used outside of a network section, instead of warning and dropping them. I suggested this previously here, #14523 (comment).

We could also go further and not even warn about these options if testnet=1 is specified in the config file (as opposed to on the command line). @zciendor, I’m not sure what your comment about a [debug] section is referring to. As far as I know that’s not a recognized section, so I’d expect any options you enter there to be ignored.

sipa commented at 5:54 pm on October 23, 2018: member

@ryanofsky I agree, I think we should revert the “ignore certain options outside of network-specific sections” behavior; it seems to gratuitously have broken compatibility. @zciendor There is documentation in the release notes: https://github.com/bitcoin/bitcoin/blob/v0.17.0/doc/release-notes.md#configuration-sections-for-testnet-and-regtest

jnewbery commented at 6:16 pm on October 23, 2018: member

I agree that removing the ’network-only’ options is a sensible thing to do in order to restore complete backwards compatibility. Network-only options was a good idea to prevent users from accidentally polluting their mainnet config with testnet or regtest config, but the amount of user pain inflicted by not having backwards compatibility means we should revert this change quickly.

To be honest, it didn’t even occur to me that this PR would break backwards compatibility. I certainly don’t think it was an intentional break.

Alternatively, we could only enforce the ’network-only’ rule if the config file contains a network section. That means that old config files would continue to be valid, but new config files with network sections would be protected from certain config errors. That can be added later (after the reversion) if people think that ’network-only’ config is a feature worth having.

zciendor commented at 7:01 pm on October 23, 2018: none

I was talking about the sections the config file generator adds. However, I just noticed that they are commented out, so this should have no effect…

 0# Generated by https://jlopp.github.io/bitcoin-core-config-generator/
 1
 2# This config should be placed in following path:
 3# ~/.bitcoin/bitcoin.conf
 4
 5# [core]
 6# Keep the transaction memory pool below <n> megabytes.
 7maxmempool=1500
 8
 9# [network]
10# Use UPnP to map the listening port.
11upnp=1
12
13# [zeromq]
14# Enable publishing of block hashes to <address>.
15zmqpubhashblock=127.0.0.1
16
17# [debug]
18# Log IP Addresses in debug output.
19logips=1
20# Run this node on the Bitcoin Test Network.
21testnet=1
22
23# [relay]
24# Force relay of transactions from whitelisted peers even if they violate local relay policy.
25whitelistforcerelay=0
26
27# [mining]
28# Set lowest fee rate (in BTC/kB) for transactions to be included in block creation.
29blockmintxfee=0.0001
30
31# [rpc]
32# Accept public REST requests.
33rest=1
34
35# [wallet]
36# Do not load the wallet and disable wallet RPC calls.
37disablewallet=1

jnewbery commented at 7:10 pm on October 23, 2018: member

Is there even a documentation of the section names?

Yes: https://bitcoincore.org/en/releases/0.17.0/#configuration-sections-for-testnet-and-regtest

zciendor commented at 7:22 pm on October 23, 2018: none

I just realized we seem to have a completely different problem with 0.17. No matter where I specify testnet=1 (either in the conf file or on the command line) bitcoind tries to write to the default mainnet folder (~/.bitcoin/):

 0$ /usr/local/bin/bitcoind -conf=/etc/bitcoin/test_bitcoin.conf -testnet
 1Bitcoin Core version v0.17.0 (release build)
 2InitParameterInteraction: parameter interaction: -proxy set -> setting -discover=0
 3InitParameterInteraction: parameter interaction: -whitelistforcerelay=1 -> setting -whitelistrelay=1
 4 
 5************************
 6EXCEPTION: N5boost10filesystem16filesystem_errorE       
 7boost::filesystem::create_directory: Permission denied: "/mnt/data/bitcoind/.bitcoin/blocks"       
 8bitcoin in AppInit()       
 9
10
11
12************************
13EXCEPTION: N5boost10filesystem16filesystem_errorE       
14boost::filesystem::create_directory: Permission denied: "/mnt/data/bitcoind/.bitcoin/blocks"       
15bitcoin in AppInit()       
16
17Shutdown: In progress...
18Shutdown: done

The issue is that 0.16 and earlier tried to read/write the /mnt/data/bitcoind/.bitcoin/testnet3 folder when the -testnet (or testnet=1) parameter was specified. This seems to have changed and now you have to specify the datadir explicitely along with testnet=1?

jnewbery commented at 7:25 pm on October 23, 2018: member

@zciendor - that looks like a separate problem. Do you mind opening a new issue to track it?

edit: please @ me once you’ve opened that issue.

zciendor commented at 7:25 pm on October 23, 2018: none

Sure! https://github.com/bitcoin/bitcoin/issues/14557

NicolasDorier commented at 5:50 am on October 24, 2018: contributor

@zciendor unsure if it is related to your problem, but a non obvious consequence of this PR is that on testnet or regtest:

You can’t use Bitcoin-CLI 0.17 on Bitcoind < 0.17
You can’t use Bitcoin-CLI < 0.17 on Bitcoind 0.17

Because they can’t read each other config files. I banged my head against the wall for some hour because of this.

ajtowns commented at 11:54 am on October 24, 2018: member

@NicolasDorier you can use:

0    connect=0
1    [test]
2    connect=0
3    [regtest]
4    connect=0

to have the connect=0 option (or any of the other mainnet-only options) seen on mainnet/testnet/regtest for pre and post 0.17.

NicolasDorier commented at 5:07 am on October 28, 2018: contributor

I don’t think it works, my memory tells that I had issue because 0.16.3 can’t parse sections, and thus consider the configuration file invalid.

ajtowns commented at 8:34 pm on October 29, 2018: member

@NicolasDorier 0.16.3 works fine here with the above – the boost config parser has always parsed sections, it’s just ignored what was in them (because it would treat it as an option named “regtest.connect” which was then never actually used).

NicolasDorier commented at 6:26 am on October 30, 2018: contributor

I confirm you are right My test was only

0[test]
1connect=0

So connect=0 was ignored.

Csi18nAlistairMann referenced this in commit 76660fcb09 on Feb 12, 2019

Csi18nAlistairMann referenced this in commit 832b049ba8 on Feb 12, 2019

Csi18nAlistairMann referenced this in commit 4f686c90e2 on Feb 12, 2019

UdjinM6 referenced this in commit 33f45f0f2c on Apr 29, 2020

UdjinM6 referenced this in commit 27f033b554 on Apr 29, 2020

PastaPastaPasta referenced this in commit 6ac5823423 on Apr 30, 2020

PastaPastaPasta referenced this in commit 9802c6b013 on Apr 30, 2020

PastaPastaPasta referenced this in commit aaf5bf1a7e on May 9, 2020

PastaPastaPasta referenced this in commit d55d362077 on May 9, 2020

PastaPastaPasta referenced this in commit 970da3524b on May 10, 2020

PastaPastaPasta referenced this in commit b0dc2ab3c1 on May 10, 2020

jkensik commented at 7:18 am on March 16, 2021: none

I get an error about my bitcoin.conf file when I set the port setting in the regtest section. Error: Config setting for -port only applied on regtest network when in [regtest] section. This is my bitcoin.conf file

` regtest=1 server=1

[regtest] port=18333 `

the error occurs even if you do not use the [regtest] section.

laanwj commented at 6:40 pm on March 16, 2021: member

@jkensik you are responding to a closed issue from 2018, if you have a problem please report a new issue

laanwj locked this on Mar 16, 2021

Network specific conf sections #11862