Add per-network config file network.conf #10996

ajtowns commented at 5:55 PM on August 5, 2017: member

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 commented at 7:13 PM on August 5, 2017: member

Concept ACK

in doc/man/bitcoin-cli.1:25 in aed4b81e8c outdated

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

laanwj commented at 7:13 PM on August 5, 2017:

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

ajtowns commented at 4:55 AM on August 6, 2017:

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

meshcollider commented at 11:30 AM on September 15, 2017:

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

ajtowns commented at 12:37 PM on September 15, 2017:

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

ajtowns commented at 11:22 AM on August 6, 2017: member

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 commented at 11:29 AM on August 6, 2017: member

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 commented at 9:10 PM on August 6, 2017: member

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.

unknown approved

ajtowns commented at 2:07 AM on August 7, 2017: member

@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 force-pushed on Aug 25, 2017

ajtowns commented at 11:46 AM on August 25, 2017: member

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

ajtowns force-pushed on Oct 10, 2017

ajtowns commented at 5:22 AM on October 10, 2017: member

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 renamed this:
~~[WIP] Add per-network config file network.conf~~
Add per-network config file network.conf
on Oct 10, 2017

laanwj added the label Feature on Nov 9, 2017

ajtowns force-pushed on Nov 21, 2017

ajtowns commented at 4:27 PM on November 21, 2017: member

Rebased to fix test_runner conflicts

meshcollider commented at 8:11 PM on November 21, 2017: contributor

Big concept ACK on the default network specific config files. EDIT: utACK https://github.com/bitcoin/bitcoin/pull/10996/commits/8da227021988ac6da0d31fb8887d2e1b0a7a05fc 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 commented at 9:49 PM on November 21, 2017: member

@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 commented at 10:58 PM on November 21, 2017: contributor

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 commented at 11:39 PM on November 21, 2017: member

Concept ACK

ajtowns commented at 6:04 AM on November 22, 2017: member

@MeshCollider see https://github.com/ajtowns/bitcoin/pull/2 . not a big fan of repeating the conditionals still, but it's not too bad.

in test/functional/netconf.py:64 in 9220cd89fd outdated

  59 | +        self.check_subversion("main", "net")
  60 | +        self.check_maxmempool(111)
  61 | +
  62 | +        # otherwise loads specified network conf
  63 | +        self.stop_node(0)
  64 | +        self.start_node(0, ["-conf=bitcoin-altnet.conf"])

promag commented at 11:53 AM on November 23, 2017:

Use restart_node.

ajtowns commented at 11:57 AM on November 27, 2017:

Changed.

in test/functional/netconf.py:70 in 9220cd89fd outdated

  65 | +        self.check_subversion("mainalt", "altnet", "extra")
  66 | +        self.check_maxmempool(333)
  67 | +
  68 | +        # netconf on command line overrides specification in bitcoin.conf
  69 | +        self.stop_node(0)
  70 | +        self.start_node(0, ["-conf=bitcoin-altnet.conf", "-netconf=network.conf"])

promag commented at 11:53 AM on November 23, 2017:

Same as above.

ajtowns commented at 11:57 AM on November 27, 2017:

Changed.

in test/functional/netconf.py:76 in 9220cd89fd outdated

  71 | +        self.check_subversion("mainalt", "net")
  72 | +        self.check_maxmempool(333)
  73 | +
  74 | +        # if file doesn't exist, everything is fine
  75 | +        self.stop_node(0)
  76 | +        self.start_node(0, ["-netconf=no-network.conf"])

promag commented at 11:53 AM on November 23, 2017:

Same as above.

ajtowns commented at 11:57 AM on November 27, 2017:

Changed.

in test/functional/netconf.py:6 in 9220cd89fd outdated

   0 | @@ -0,0 +1,82 @@
   1 | +#!/usr/bin/env python3
   2 | +# Copyright (c) 2017 The Bitcoin Core developers
   3 | +# Distributed under the MIT software license, see the accompanying
   4 | +# file COPYING or http://www.opensource.org/licenses/mit-license.php.
   5 | +"""
   6 | +Tests the network-specific config file

promag commented at 11:57 AM on November 23, 2017:

Nit, missing period. One line?

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

ajtowns commented at 11:57 AM on November 27, 2017:

Changed.

in test/functional/netconf.py:12 in 9220cd89fd outdated

   7 | +"""
   8 | +
   9 | +import os
  10 | +from test_framework.test_framework import BitcoinTestFramework
  11 | +
  12 | +class IncludeConfTest(BitcoinTestFramework):

promag commented at 11:58 AM on November 23, 2017:

Should mirror the filename? class NetconfTest?

ajtowns commented at 11:57 AM on November 27, 2017:

Changed.

in src/util.h:207 in 9220cd89fd outdated

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

promag commented at 12:01 PM on November 23, 2017:

Same as above.

in src/util.h:177 in 9220cd89fd outdated

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

promag commented at 12:02 PM on November 23, 2017:

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

ajtowns commented at 7:24 AM on November 27, 2017:

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.

ajtowns commented at 1:10 PM on November 27, 2017:

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

promag commented at 12:19 PM on November 23, 2017: member

Meanwhile some nits.

ajtowns force-pushed on Nov 27, 2017

ajtowns commented at 1:08 PM on November 27, 2017: member

Rebased, small improvements to test case, nit fixes.

Add per-network config file network.conf e9cc1c0e5a

Be explicit whether config file is net specific

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.

5a63c5ce36

ajtowns force-pushed on Dec 5, 2017

ajtowns commented at 8:30 AM on December 5, 2017: member

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

meshcollider commented at 10:39 AM on December 11, 2017: contributor

This is now superseded by https://github.com/bitcoin/bitcoin/pull/11862

MarcoFalke added the label Utils/log/libs on Jan 7, 2018

MarcoFalke removed the label Feature on Jan 7, 2018

ajtowns commented at 6:05 PM on March 6, 2018: member

Abandoning in favour of #11862

ajtowns closed this on Mar 6, 2018

MarcoFalke locked this on Sep 8, 2021