Network specific conf sections #11862

ajtowns · 2017-12-11T10:06:15Z

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

meshcollider · 2017-12-11T10:33:19Z

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 · 2017-12-11T15:16:56Z

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:

testnet
[testnet]
regtest
[regtest]
notestnet
noregtest

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 · 2017-12-11T18:56:51Z

Concept ACK.
Nice work!

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

ajtowns · 2017-12-12T10:22:03Z

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 907dd64 at https://github.com/ajtowns/bitcoin/tree/netconf-sections-v1 on the off chance someone wants to look at it.

ajtowns · 2018-01-03T05:46:37Z

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!

laanwj · 2018-03-05T19:30:07Z

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 · 2018-03-06T19:22:14Z

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

jnewbery

I've reviewed up to 940e5bc. A couple of comments inline.

jnewbery · 2018-03-06T22:46:24Z

src/util.cpp

-    {
+    if (!streamConfig.good()) {
+        return; // No bitcoin.conf file is OK
+    } else {


This else isn't required. You can just drop through after the if block. (in the commit 4f3766f 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().

jnewbery · 2018-03-06T23:26:22Z

src/util.cpp

+public:
+    inline static bool UseDefaultSection(const ArgsManager& am, const std::string& strArg)
+    {
+        if (am.m_strSection == CBaseChainParams::MAIN)


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

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.

jnewbery · 2018-03-07T14:16:23Z

src/util.cpp

@@ -435,11 +435,116 @@ static void InterpretNegativeSetting(std::string& strKey, std::string& strValue)
    }
 }

+/** Internal helper functions for ArgsManager */
+class ArgsManagerHelper {


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

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.

jnewbery · 2018-03-07T14:17:00Z

src/util.cpp

+/** Internal helper functions for ArgsManager */
+class ArgsManagerHelper {
+public:
+    inline static bool UseDefaultSection(const ArgsManager& am, const std::string& strArg)


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

jnewbery · 2018-03-07T14:50:35Z

test/functional/feature_config_args.py

-        with open(conf_file, 'a', encoding='utf8') as f:
+
+        # datadir needs to be set before [regtest] section
+        conf_file_contents = open(conf_file, encoding='utf8').readlines()


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.

jnewbery · 2018-03-07T15:02:47Z

src/util.cpp

+/** Internal helper functions for ArgsManager */
+class ArgsManagerHelper {
+public:
+    inline static bool UseDefaultSection(const ArgsManager& am, const std::string& strArg)


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).

jnewbery · 2018-03-07T15:09:42Z

src/util.cpp

+    }
+};
+
+ArgsManager::ArgsManager(void) :


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

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.

jnewbery · 2018-03-07T15:21:19Z

src/util.cpp

+    /** Convert regular argument into the section-specific setting */
+    inline static std::string SectionArg(const ArgsManager& am, const std::string& strArg)
+    {
+        assert(strArg.length() > 1 && strArg[0] == '-');


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

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

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

jnewbery · 2018-03-07T15:30:26Z

src/util.cpp

+                return true;
+        }
+
+        if (UseDefaultSection(am, strArg)) {


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.

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

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

laanwj · 2018-03-22T08:49:11Z

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 · 2018-03-22T14:34:19Z

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 · 2018-03-26T14:36:36Z

#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 · 2018-03-28T15:19:13Z

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 · 2018-10-23T19:25:24Z

@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 · 2018-10-23T19:25:46Z

Sure! #14557

NicolasDorier · 2018-10-24T05:50:05Z

@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 · 2018-10-24T11:54:49Z

@NicolasDorier you can use:

    connect=0
    [test]
    connect=0
    [regtest]
    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 · 2018-10-28T05:07:41Z

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 · 2018-10-29T20:34:59Z

@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 · 2018-10-30T06:26:39Z

I confirm you are right
My test was only

[test]
connect=0

So connect=0 was ignored.

…rks, however some apply only to mainnet unless specified in a section. As stands, conf file has no indication that sections are now in use or are in some circumstances mandatory (eg, changing rpcport for testnet.) Proposed change notifies the reader early which options are affected, specifically adds those options affected but not already in the example, adds brief explanation as to what's going on and provides a skeleton template for the sections themselves.

Per bitcoin#11862. Most bitcoin.conf options apply to all three networks, however some apply only to mainnet unless specified in a section. As stands, conf file has no indication that sections are now in use or are in some circumstances mandatory (eg, changing rpcport for testnet.) Proposed change notifies the reader early which options are affected, specifically adds those options affected but not already in the example, adds brief explanation as to what's going on and provides a skeleton template for the sections themselves.

…ation for network-specific sections 77a733a [tests] Add additional unit tests for -nofoo edge cases (Anthony Towns) af173c2 [tests] Check GetChainName works with config entries (Anthony Towns) fa27f1c [tests] Add unit tests for ReadConfigStream (Anthony Towns) 087c5d2 ReadConfigStream: assume the stream is good (Anthony Towns) 6d5815a Separate out ReadConfigStream from ReadConfigFile (Anthony Towns) 834d303 [tests] Add unit tests for GetChainName (Anthony Towns) 11b6b5b Move ChainNameFromCommandLine into ArgsManager and rename to GetChainName (Anthony Towns) Pull request description: This does a bit of refactoring of the configuration handling code in order to add additional tests to make adding support for [test]/[regtest] sections in the config file in bitcoin#11862 easier. Should not cause any behaviour changes. Tree-SHA512: 8d2ce1449fc180de03414e7e569d1a21ba1e9f6564e13d3faf3961f710adc725fa0d4ab49b89ebd2baa11ea36ac5018377f693a84037d386a8b8697c9d6db3e9

@theuni

c25321f Add config changes to release notes (Anthony Towns) 5e3cbe0 [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections (Anthony Towns) 005ad26 ArgsManager: special handling for -regtest and -testnet (Anthony Towns) 608415d [tests] Unit tests for network-specific config entries (Anthony Towns) 68797e2 ArgsManager: Warn when ignoring network-specific config setting (Anthony Towns) d1fc4d9 ArgsManager: limit some options to only apply on mainnet when in default section (Anthony Towns) 8a9817d [tests] Use regtest section in functional tests configs (Anthony Towns) 30f9407 [tests] Unit tests for config file sections (Anthony Towns) 95eb66d ArgsManager: support config file sections (Anthony Towns) 4d34fcc ArgsManager: drop m_negated_args (Anthony Towns) 3673ca3 ArgsManager: keep command line and config file arguments separate (Anthony Towns) Pull request description: The weekly meeting on [2017-12-07](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-12-07-19.00.log.html) 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 Tree-SHA512: f00b5eb75f006189987e5c15e154a42b66ee251777768c1e185d764279070fcb7c41947d8794092b912a03d985843c82e5189871416995436a6260520fb7a4db

…ation for network-specific sections 77a733a [tests] Add additional unit tests for -nofoo edge cases (Anthony Towns) af173c2 [tests] Check GetChainName works with config entries (Anthony Towns) fa27f1c [tests] Add unit tests for ReadConfigStream (Anthony Towns) 087c5d2 ReadConfigStream: assume the stream is good (Anthony Towns) 6d5815a Separate out ReadConfigStream from ReadConfigFile (Anthony Towns) 834d303 [tests] Add unit tests for GetChainName (Anthony Towns) 11b6b5b Move ChainNameFromCommandLine into ArgsManager and rename to GetChainName (Anthony Towns) Pull request description: This does a bit of refactoring of the configuration handling code in order to add additional tests to make adding support for [test]/[regtest] sections in the config file in bitcoin#11862 easier. Should not cause any behaviour changes. Tree-SHA512: 8d2ce1449fc180de03414e7e569d1a21ba1e9f6564e13d3faf3961f710adc725fa0d4ab49b89ebd2baa11ea36ac5018377f693a84037d386a8b8697c9d6db3e9

@theuni

c25321f Add config changes to release notes (Anthony Towns) 5e3cbe0 [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections (Anthony Towns) 005ad26 ArgsManager: special handling for -regtest and -testnet (Anthony Towns) 608415d [tests] Unit tests for network-specific config entries (Anthony Towns) 68797e2 ArgsManager: Warn when ignoring network-specific config setting (Anthony Towns) d1fc4d9 ArgsManager: limit some options to only apply on mainnet when in default section (Anthony Towns) 8a9817d [tests] Use regtest section in functional tests configs (Anthony Towns) 30f9407 [tests] Unit tests for config file sections (Anthony Towns) 95eb66d ArgsManager: support config file sections (Anthony Towns) 4d34fcc ArgsManager: drop m_negated_args (Anthony Towns) 3673ca3 ArgsManager: keep command line and config file arguments separate (Anthony Towns) Pull request description: The weekly meeting on [2017-12-07](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-12-07-19.00.log.html) 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 Tree-SHA512: f00b5eb75f006189987e5c15e154a42b66ee251777768c1e185d764279070fcb7c41947d8794092b912a03d985843c82e5189871416995436a6260520fb7a4db

…ation for network-specific sections 77a733a [tests] Add additional unit tests for -nofoo edge cases (Anthony Towns) af173c2 [tests] Check GetChainName works with config entries (Anthony Towns) fa27f1c [tests] Add unit tests for ReadConfigStream (Anthony Towns) 087c5d2 ReadConfigStream: assume the stream is good (Anthony Towns) 6d5815a Separate out ReadConfigStream from ReadConfigFile (Anthony Towns) 834d303 [tests] Add unit tests for GetChainName (Anthony Towns) 11b6b5b Move ChainNameFromCommandLine into ArgsManager and rename to GetChainName (Anthony Towns) Pull request description: This does a bit of refactoring of the configuration handling code in order to add additional tests to make adding support for [test]/[regtest] sections in the config file in bitcoin#11862 easier. Should not cause any behaviour changes. Tree-SHA512: 8d2ce1449fc180de03414e7e569d1a21ba1e9f6564e13d3faf3961f710adc725fa0d4ab49b89ebd2baa11ea36ac5018377f693a84037d386a8b8697c9d6db3e9

@theuni

c25321f Add config changes to release notes (Anthony Towns) 5e3cbe0 [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections (Anthony Towns) 005ad26 ArgsManager: special handling for -regtest and -testnet (Anthony Towns) 608415d [tests] Unit tests for network-specific config entries (Anthony Towns) 68797e2 ArgsManager: Warn when ignoring network-specific config setting (Anthony Towns) d1fc4d9 ArgsManager: limit some options to only apply on mainnet when in default section (Anthony Towns) 8a9817d [tests] Use regtest section in functional tests configs (Anthony Towns) 30f9407 [tests] Unit tests for config file sections (Anthony Towns) 95eb66d ArgsManager: support config file sections (Anthony Towns) 4d34fcc ArgsManager: drop m_negated_args (Anthony Towns) 3673ca3 ArgsManager: keep command line and config file arguments separate (Anthony Towns) Pull request description: The weekly meeting on [2017-12-07](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-12-07-19.00.log.html) 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 Tree-SHA512: f00b5eb75f006189987e5c15e154a42b66ee251777768c1e185d764279070fcb7c41947d8794092b912a03d985843c82e5189871416995436a6260520fb7a4db

@theuni

c25321f Add config changes to release notes (Anthony Towns) 5e3cbe0 [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections (Anthony Towns) 005ad26 ArgsManager: special handling for -regtest and -testnet (Anthony Towns) 608415d [tests] Unit tests for network-specific config entries (Anthony Towns) 68797e2 ArgsManager: Warn when ignoring network-specific config setting (Anthony Towns) d1fc4d9 ArgsManager: limit some options to only apply on mainnet when in default section (Anthony Towns) 8a9817d [tests] Use regtest section in functional tests configs (Anthony Towns) 30f9407 [tests] Unit tests for config file sections (Anthony Towns) 95eb66d ArgsManager: support config file sections (Anthony Towns) 4d34fcc ArgsManager: drop m_negated_args (Anthony Towns) 3673ca3 ArgsManager: keep command line and config file arguments separate (Anthony Towns) Pull request description: The weekly meeting on [2017-12-07](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-12-07-19.00.log.html) 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 Tree-SHA512: f00b5eb75f006189987e5c15e154a42b66ee251777768c1e185d764279070fcb7c41947d8794092b912a03d985843c82e5189871416995436a6260520fb7a4db

@theuni

c25321f Add config changes to release notes (Anthony Towns) 5e3cbe0 [tests] Unit tests for -testnet/-regtest in [test]/[regtest] sections (Anthony Towns) 005ad26 ArgsManager: special handling for -regtest and -testnet (Anthony Towns) 608415d [tests] Unit tests for network-specific config entries (Anthony Towns) 68797e2 ArgsManager: Warn when ignoring network-specific config setting (Anthony Towns) d1fc4d9 ArgsManager: limit some options to only apply on mainnet when in default section (Anthony Towns) 8a9817d [tests] Use regtest section in functional tests configs (Anthony Towns) 30f9407 [tests] Unit tests for config file sections (Anthony Towns) 95eb66d ArgsManager: support config file sections (Anthony Towns) 4d34fcc ArgsManager: drop m_negated_args (Anthony Towns) 3673ca3 ArgsManager: keep command line and config file arguments separate (Anthony Towns) Pull request description: The weekly meeting on [2017-12-07](http://www.erisian.com.au/meetbot/bitcoin-core-dev/2017/bitcoin-core-dev.2017-12-07-19.00.log.html) 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 Tree-SHA512: f00b5eb75f006189987e5c15e154a42b66ee251777768c1e185d764279070fcb7c41947d8794092b912a03d985843c82e5189871416995436a6260520fb7a4db

jkensik · 2021-03-16T07:18:40Z

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 · 2021-03-16T18:40:06Z

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

fanquake added the Brainstorming label Dec 11, 2017

This was referenced Dec 11, 2017

Add per-network config file network.conf #10996

Closed

New -includeconf argument for including external configuration files #10267

Merged

ajtowns force-pushed the netconf-sections branch from 907dd64 to 91baf3c Compare December 12, 2017 10:22

ajtowns force-pushed the netconf-sections branch 2 times, most recently from 98e1dfd to ee91028 Compare January 3, 2018 02:17

ajtowns changed the title ~~[concept] Network specific conf sections~~ [wip] Network specific conf sections Jan 3, 2018

ajtowns closed this Mar 5, 2018

ajtowns deleted the netconf-sections branch March 5, 2018 19:20

ajtowns restored the netconf-sections branch March 5, 2018 19:28

ajtowns reopened this Mar 5, 2018

ajtowns force-pushed the netconf-sections branch from ee91028 to bcab6fa Compare March 5, 2018 19:28

jnewbery reviewed Mar 6, 2018

View reviewed changes

jnewbery reviewed Mar 7, 2018

View reviewed changes

ajtowns mentioned this pull request Mar 7, 2018

blockmaxsize parameter has no effect #12640

Closed

jnewbery mentioned this pull request Mar 21, 2018

WIP: eliminate dependency on boost::program_options #12744

Closed

laanwj added this to the 0.17.0 milestone Mar 22, 2018

ajtowns force-pushed the netconf-sections branch from bcab6fa to d124df7 Compare March 28, 2018 15:15

ajtowns changed the title ~~[wip] Network specific conf sections~~ Network specific conf sections Mar 28, 2018

Csi18nAlistairMann mentioned this pull request Feb 9, 2019

docs: add "sections" info to example bitcoin.conf #15374

Closed

Csi18nAlistairMann mentioned this pull request Feb 12, 2019

docs: add "sections" info to example bitcoin.conf #15387

Closed

promag mentioned this pull request Mar 21, 2019

-wallet is a "network-only" option but -walletdir is not #15630

Closed

qtum-neil mentioned this pull request Jul 15, 2019

qtum.conf Configuration File not Read Correctly for Testnet qtumproject/qtum#714

Closed

hebasto mentioned this pull request Jul 16, 2019

Refactor: Add Flags enum to ArgsManager class #16097

Merged

jnewbery mentioned this pull request Nov 8, 2019

Merge settings one place instead of five places #15934

Merged

UdjinM6 mentioned this pull request Apr 29, 2020

Backport loadwallet dashpay/dash#3462

Merged

codablock mentioned this pull request Jun 23, 2020

Fix argument handling for devnets dashpay/dash#3549

Merged

strophy mentioned this pull request Oct 8, 2020

fix: use conf sections in dash.conf dashevo/dashmate#153

Merged

5 tasks

bitcoin locked as resolved and limited conversation to collaborators Mar 16, 2021

Network specific conf sections #11862

Network specific conf sections #11862

Uh oh!

Conversation

ajtowns commented Dec 11, 2017

Uh oh!

meshcollider commented Dec 11, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery commented Dec 11, 2017

Uh oh!

jonasschnelli commented Dec 11, 2017

Uh oh!

ajtowns commented Dec 12, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ajtowns commented Jan 3, 2018

Uh oh!

laanwj commented Mar 5, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

meshcollider commented Mar 6, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

jnewbery left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ajtowns Mar 7, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj commented Mar 22, 2018

Uh oh!

jnewbery commented Mar 22, 2018

Uh oh!

jnewbery commented Mar 26, 2018

Uh oh!

ajtowns commented Mar 28, 2018

Uh oh!

jnewbery commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

zciendor commented Oct 23, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

NicolasDorier commented Oct 24, 2018

Uh oh!

ajtowns commented Oct 24, 2018

Uh oh!

NicolasDorier commented Oct 28, 2018

Uh oh!

meshcollider commented Dec 11, 2017 •

edited

Loading

ajtowns commented Dec 12, 2017 •

edited

Loading

laanwj commented Mar 5, 2018 •

edited

Loading

meshcollider commented Mar 6, 2018 •

edited

Loading

ajtowns Mar 7, 2018 •

edited

Loading

jnewbery commented Oct 23, 2018 •

edited

Loading

zciendor commented Oct 23, 2018 •

edited

Loading

jkensik commented Mar 16, 2021 •

edited

Loading