Add per-network config file network.conf #10996

ajtowns · 2017-08-05T17:55:55Z

Adds a per-net network.conf as per #9374 ; this is pretty much the same as #9402 which was closed for lack of interest.

It's somewhat interesting in the context of #10994 because it provides somewhere to stick the network-specific vbignore settings. Unless I'm missing something, I don't think #10267 provides an easy way of doing per-network config files, so wouldn't work well for that particular use case.

This patch doesn't include any tests, so is marked as WIP. There's some test code in #10267 that would presumably be able to be adapted pretty easily.

laanwj · 2017-08-05T19:13:28Z

Concept ACK

laanwj · 2017-08-05T19:13:49Z

doc/man/bitcoin-cli.1

@@ -22,6 +22,10 @@ This help message
 .IP
 Specify configuration file (default: bitcoin.conf)
 .HP
+\fB\-netconf=\fR<file>


I wonder - is there any practical point in being able to override this?
If not, it's a waste of an option (and help translation).

It could be useful if you wanted to temporarily restart bitcoind without using the network.conf file. You could have a bitcoin-alt.conf that specified netconf=network-alt.conf so that you could load bitcoind with -conf=bitcoin-alt.conf and have that load network specific alternative configurations too. Not sure how realistic/valuable those are, though. Not having the option would be inconsistent; and having the option makes the name of the config file at least a little discoverable...

Aren't the doc/man/ files automatically generated anyway so shouldn't be modified directly? Or is this information out of date

@meshcollider Yep, that's correct; worse the help text for bitcoin-cli is missing. Will fix.

ajtowns · 2017-08-06T11:22:43Z

Commit 7a71b09 adds basic functional tests; it makes use of -netconf being a modifiable parameter to point bitcoind at different files in different ways and checks that has different results.

Commit 8b48aa5 cleans up the code a little, but also allows you to specify an absolute path for network.conf if you want to for some reason (not that doing so actually makes a lot of sense).

Commit 70f5388 removes the default for fNetSpecific for calls to Get/ReadConfigFile, which seems sensible since the default would need to be false to be backwards compatible with the old calls for bitcoin.conf, but is the opposite to the default for the same parameter for GetDataDir().

ajtowns · 2017-08-06T11:29:02Z

One possibly confusing behaviour: possible to set "-testnet" or "-regtest" in a network.conf where those parameters would be wrong; if you do so, they won't take effect or cause problems because ChainNameFromCommandLine() has already been called and isn't called again; but if some future change to the code does try to call ChainNameFromCommandLine() sometime after startup, and a network.conf sets a wrong parameter, it could blow up in your face.

I'm not sure if this is worth fixing...

(Adding a "static const string *result;" to ChainNameFromCommandLine() to memoise it at first call, and just reuse the result without reexamining the arguments later seems to be sufficient to fix the problem if it is worth fixing. Giving an error if regtest/testnet is specified in network.conf would probably be better, but seems like it violates the abstraction layer a fair bit...)

jnewbery · 2017-08-06T21:10:17Z

I've wanted to rebase and revive #9402 for some time, but I was waiting for #10267, both because this conflicts heavily with that PR, and also because I think once #10267 is merged, then this PR becomes more straightforward.

ajtowns · 2017-08-07T02:07:43Z

@jnewbery Hmm, looking at #10267 in a bit more detail, the current patch seems to only allow a single includeconf directive to have an effect anyway, rather than multiple includes or recursive includes; so you could actually get the same effect as that patch with this one by just giving an absolute path to the netconf directory here. It doesn't look to me like there's really much difficulty in combining the two patches either once one or the other gets merged (I'm not seeing much room for either to get much simpler, though -- you still have to finish loading bitcoin.conf then load the chain params to work out which network you're on before you can load the right network.conf).

ajtowns · 2017-08-25T11:46:23Z

Squashed most of the commits, and rebased to current master.

ajtowns · 2017-10-10T05:22:39Z

Rebased to current master. Also: no longer committing changes to manpages, functional test updated due to API changes on master, help text for bitcoin-cli added, and minor code style fixes.

ajtowns · 2017-11-21T16:27:27Z

Rebased to fix test_runner conflicts

meshcollider · 2017-11-21T20:11:05Z

Big concept ACK on the default network specific config files. EDIT: utACK 8da2270
Will test it out shortly
It does seem a bit weird though having both conf and netconf. What if netconf was just a boolean to decide whether to look for conf in the net specific datadir or not? I'm worried about user confusion over the interaction between both those parameters, will discuss on IRC

ajtowns · 2017-11-21T21:49:08Z

@meshcollider In normal usage, -netconf isn't needed -- you just add the network.conf file to .bitcoin/testnet3/ (or wherever) if you want to use it, and you don't if you don't want to use it. The main benefits to having the option are (a) it makes it easier to test (can just specify different config files to check different behaviour, rather than having to move files around), and (b) it means it shows up when you say --help so people have some chance of knowing it exists. I think that's enough to justify having the option, but happy to drop it if people think that makes it simpler.

meshcollider · 2017-11-21T22:58:53Z

Upon discussion on IRC: I suggested using network.conf if -conf is not specified but using the -conf if it is and not using network.conf at all. This removes the need for the netconf argument. Reasoning is that if a user specified a config file they probably want to use it on the network they specify as well, so we really only need to change the default behaviour. But unsure if that's too limiting

sipa · 2017-11-21T23:39:40Z

Concept ACK

ajtowns · 2017-11-22T06:04:36Z

@meshcollider see ajtowns#2 . not a big fan of repeating the conditionals still, but it's not too bad.

promag

Meanwhile some nits.

promag · 2017-11-23T11:53:34Z

test/functional/netconf.py

+
+        # otherwise loads specified network conf
+        self.stop_node(0)
+        self.start_node(0, ["-conf=bitcoin-altnet.conf"])


Use restart_node.

promag · 2017-11-23T11:53:49Z

test/functional/netconf.py

+
+        # netconf on command line overrides specification in bitcoin.conf
+        self.stop_node(0)
+        self.start_node(0, ["-conf=bitcoin-altnet.conf", "-netconf=network.conf"])


Same as above.

promag · 2017-11-23T11:53:56Z

test/functional/netconf.py

+
+        # if file doesn't exist, everything is fine
+        self.stop_node(0)
+        self.start_node(0, ["-netconf=no-network.conf"])


Same as above.

promag · 2017-11-23T11:57:29Z

test/functional/netconf.py

+# Distributed under the MIT software license, see the accompanying
+# file COPYING or http://www.opensource.org/licenses/mit-license.php.
+"""
+Tests the network-specific config file


Nit, missing period. One line?

"""Tests the network-specific config file."""

promag · 2017-11-23T11:58:38Z

test/functional/netconf.py

+import os
+from test_framework.test_framework import BitcoinTestFramework
+
+class IncludeConfTest(BitcoinTestFramework):


Should mirror the filename? class NetconfTest?

promag · 2017-11-23T12:01:23Z

src/util.h

@@ -200,7 +201,7 @@ class ArgsManager
    std::map<std::string, std::vector<std::string>> mapMultiArgs;
 public:
    void ParseParameters(int argc, const char*const argv[]);
-    void ReadConfigFile(const std::string& confPath);
+    void ReadConfigFile(const std::string& confPath, bool fNetSpecific);


Same as above.

promag · 2017-11-23T12:02:38Z

src/util.h

@@ -171,7 +172,7 @@ bool TryCreateDirectories(const fs::path& p);
 fs::path GetDefaultDataDir();
 const fs::path &GetDataDir(bool fNetSpecific = true);
 void ClearDatadirCache();
-fs::path GetConfigFile(const std::string& confPath);
+fs::path GetConfigFile(const std::string& confPath, bool fNetSpecific);


Set default value bool fNetSpecific = false? In that case remove changes where it's false.

The "Be explicit whether config file is net specific" commit removes the fNetSpecific=false default argument, because otherwise it would have the opposite default to GetDataDir() which seems confusing to me.

Err, the implication being that just dropping/not merging that commit is how you'd get the fNetSpecific=false default.

ajtowns · 2017-11-27T13:08:52Z

Rebased, small improvements to test case, nit fixes.

Remove the default for whether GetConfigFile and ReadConfigFile expects the file to be network specific or not, requiring true or false to be passed in for fNetworkSpecific on any call.

ajtowns · 2017-12-05T08:30:49Z

Rebased to fix test_runner.py conflict, renamed test case to conform to naming convention of #11796

meshcollider · 2017-12-11T10:39:23Z

This is now superseded by #11862

ajtowns · 2018-03-06T18:05:47Z

Abandoning in favour of #11862

laanwj reviewed Aug 5, 2017

View reviewed changes

ghost approved these changes Aug 6, 2017

View reviewed changes

ajtowns force-pushed the netconf branch 3 times, most recently from 7d7eea2 to e3fe835 Compare August 25, 2017 05:21

ajtowns force-pushed the netconf branch 2 times, most recently from 262abf1 to e609aba Compare October 10, 2017 05:18

ajtowns changed the title ~~[WIP] Add per-network config file network.conf~~ Add per-network config file network.conf Oct 10, 2017

laanwj added the Feature label Nov 9, 2017

This was referenced Nov 21, 2017

Add -logdir option to control where debug.log lives #11741

Closed

Cleanups + nit fixes for walletdir PR #11726

Merged

ajtowns force-pushed the netconf branch from e609aba to 8da2270 Compare November 21, 2017 16:26

promag reviewed Nov 23, 2017

View reviewed changes

ajtowns force-pushed the netconf branch from 9220cd8 to 35772b0 Compare November 27, 2017 11:57

ajtowns added 2 commits December 5, 2017 17:05

Add per-network config file network.conf

e9cc1c0

Be explicit whether config file is net specific

5a63c5c

Remove the default for whether GetConfigFile and ReadConfigFile expects the file to be network specific or not, requiring true or false to be passed in for fNetworkSpecific on any call.

ajtowns force-pushed the netconf branch from 35772b0 to 5a63c5c Compare December 5, 2017 07:26

sipa mentioned this pull request Dec 6, 2017

Test datadir specified in conf file exists #11829

Merged

maflcko added Utils/log/libs and removed Feature labels Jan 7, 2018

ajtowns closed this Mar 6, 2018

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add per-network config file network.conf #10996

Add per-network config file network.conf #10996

Uh oh!

Conversation

ajtowns commented Aug 5, 2017

Uh oh!

laanwj commented Aug 5, 2017

Uh oh!

laanwj Aug 5, 2017 • 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!

meshcollider Sep 15, 2017 • 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!

ajtowns commented Aug 6, 2017

Uh oh!

ajtowns commented Aug 6, 2017

Uh oh!

jnewbery commented Aug 6, 2017

Uh oh!

ajtowns commented Aug 7, 2017

Uh oh!

ajtowns commented Aug 25, 2017

Uh oh!

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

Uh oh!

Uh oh!

ajtowns commented Nov 21, 2017

Uh oh!

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

Uh oh!

Uh oh!

ajtowns commented Nov 21, 2017

Uh oh!

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

Uh oh!

Uh oh!

sipa commented Nov 21, 2017

Uh oh!

ajtowns commented Nov 22, 2017

Uh oh!

promag 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!

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 commented Nov 27, 2017

Uh oh!

laanwj Aug 5, 2017 •

edited

Loading

meshcollider Sep 15, 2017 •

edited

Loading

ajtowns commented Oct 10, 2017 •

edited

Loading

meshcollider commented Nov 21, 2017 •

edited

Loading

meshcollider commented Nov 21, 2017 •

edited

Loading