Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Network specific conf sections #11862

Merged
merged 11 commits into from Apr 16, 2018

Conversation

@ajtowns
Copy link
Contributor

ajtowns commented Dec 11, 2017

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

This comment has been minimized.

Copy link
Member

meshcollider commented Dec 11, 2017

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

This comment has been minimized.

Copy link
Member

jnewbery commented Dec 11, 2017

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

This comment has been minimized.

Copy link
Member

jonasschnelli commented Dec 11, 2017

Concept ACK.
Nice work!

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

@ajtowns

This comment has been minimized.

Copy link
Contributor Author

ajtowns commented Dec 12, 2017

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 ajtowns force-pushed the ajtowns:netconf-sections branch from 907dd64 to 91baf3c Dec 12, 2017
@ajtowns ajtowns force-pushed the ajtowns:netconf-sections branch 2 times, most recently Jan 2, 2018
@ajtowns

This comment has been minimized.

Copy link
Contributor Author

ajtowns commented Jan 3, 2018

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 ajtowns changed the title [concept] Network specific conf sections [wip] Network specific conf sections Jan 3, 2018
@ajtowns ajtowns closed this Mar 5, 2018
@ajtowns ajtowns deleted the ajtowns:netconf-sections branch Mar 5, 2018
@ajtowns ajtowns restored the ajtowns:netconf-sections branch Mar 5, 2018
@ajtowns ajtowns reopened this Mar 5, 2018
@ajtowns ajtowns force-pushed the ajtowns:netconf-sections branch to bcab6fa Mar 5, 2018
@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 5, 2018

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

This comment has been minimized.

Copy link
Member

meshcollider commented Mar 6, 2018

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

Copy link
Member

jnewbery left a comment

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

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 6, 2018

Member

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

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 6, 2018

Member

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)

Copy link
Member

jnewbery left a comment

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.

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

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 7, 2018

Member

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Mar 12, 2018

Author Contributor

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.

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 7, 2018

Member

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

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 7, 2018

Member

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.

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

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 7, 2018

Member

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

}
};

ArgsManager::ArgsManager(void) :

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 7, 2018

Member

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Mar 28, 2018

Author Contributor

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.

/** 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 comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 7, 2018

Member

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.

This comment has been minimized.

Copy link
@ajtowns

ajtowns Mar 7, 2018

Author Contributor

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

return true;
}

if (UseDefaultSection(am, strArg)) {

This comment has been minimized.

Copy link
@jnewbery

jnewbery Mar 7, 2018

Member

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.

This comment has been minimized.

Copy link
@ajtowns

ajtowns Mar 7, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@laanwj

laanwj Mar 7, 2018

Member

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Mar 22, 2018

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

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 22, 2018

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

This comment has been minimized.

Copy link
Member

jnewbery commented Mar 26, 2018

#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 ajtowns force-pushed the ajtowns:netconf-sections branch from bcab6fa Mar 28, 2018
@ajtowns ajtowns changed the title [wip] Network specific conf sections Network specific conf sections Mar 28, 2018
@ajtowns

This comment has been minimized.

Copy link
Contributor Author

ajtowns commented Mar 28, 2018

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

static inline std::pair<bool,std::string> GetArg(const ArgsManager &am, const std::string& arg)
{
LOCK(am.cs_args);
std::pair<bool,std::string> found_result(false, std::string());

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 18, 2018

Contributor

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Apr 18, 2018

Author Contributor

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.

m_override_args[key].push_back(val);
// Check for -nofoo
if (InterpretNegatedOption(key, val)) {
m_override_args[key].clear();

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 18, 2018

Contributor

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

This comment has been minimized.

Copy link
@ajtowns

ajtowns Apr 18, 2018

Author Contributor

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

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 19, 2018

Contributor

#11862 (comment)

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?

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 19, 2018

Contributor

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.

if (InterpretNegatedOption(key, val)) {
m_override_args[key].clear();
} else {
m_override_args[key].push_back(val);

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 18, 2018

Contributor

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

@@ -553,6 +575,11 @@ static bool InterpretNegatedOption(std::string& key, std::string& val)
return false;
}

void ArgsManager::SelectConfigNetwork(const std::string& network)

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 18, 2018

Contributor

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

return false; // not set
}
}
return InterpretBool(found_result.second); // is set, so evaluate

This comment has been minimized.

Copy link
@ryanofsky

ryanofsky Apr 18, 2018

Contributor

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?

This comment has been minimized.

Copy link
@ajtowns

ajtowns Apr 18, 2018

Author Contributor

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.

This comment has been minimized.

Copy link
@ajtowns

ajtowns Apr 18, 2018

Author Contributor

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

@NicolasDorier

This comment has been minimized.

Copy link
Member

NicolasDorier commented Oct 11, 2018

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

This comment has been minimized.

Copy link

zciendor commented Oct 23, 2018

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

This comment has been minimized.

Copy link
Contributor

ryanofsky commented Oct 23, 2018

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

This comment has been minimized.

Copy link
Member

sipa commented Oct 23, 2018

@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

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 23, 2018

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

This comment has been minimized.

Copy link

zciendor commented Oct 23, 2018

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

# Generated by https://jlopp.github.io/bitcoin-core-config-generator/

# This config should be placed in following path:
# ~/.bitcoin/bitcoin.conf

# [core]
# Keep the transaction memory pool below <n> megabytes.
maxmempool=1500

# [network]
# Use UPnP to map the listening port.
upnp=1

# [zeromq]
# Enable publishing of block hashes to <address>.
zmqpubhashblock=127.0.0.1

# [debug]
# Log IP Addresses in debug output.
logips=1
# Run this node on the Bitcoin Test Network.
testnet=1

# [relay]
# Force relay of transactions from whitelisted peers even if they violate local relay policy.
whitelistforcerelay=0

# [mining]
# Set lowest fee rate (in BTC/kB) for transactions to be included in block creation.
blockmintxfee=0.0001

# [rpc]
# Accept public REST requests.
rest=1

# [wallet]
# Do not load the wallet and disable wallet RPC calls.
disablewallet=1
@jnewbery

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 23, 2018

@zciendor

This comment has been minimized.

Copy link

zciendor commented Oct 23, 2018

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/):

$ /usr/local/bin/bitcoind -conf=/etc/bitcoin/test_bitcoin.conf -testnet
Bitcoin Core version v0.17.0 (release build)
InitParameterInteraction: parameter interaction: -proxy set -> setting -discover=0
InitParameterInteraction: parameter interaction: -whitelistforcerelay=1 -> setting -whitelistrelay=1
 
************************
EXCEPTION: N5boost10filesystem16filesystem_errorE       
boost::filesystem::create_directory: Permission denied: "/mnt/data/bitcoind/.bitcoin/blocks"       
bitcoin in AppInit()       



************************
EXCEPTION: N5boost10filesystem16filesystem_errorE       
boost::filesystem::create_directory: Permission denied: "/mnt/data/bitcoind/.bitcoin/blocks"       
bitcoin in AppInit()       

Shutdown: In progress...
Shutdown: 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

This comment has been minimized.

Copy link
Member

jnewbery commented Oct 23, 2018

@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

This comment has been minimized.

Copy link

zciendor commented Oct 23, 2018

Sure! #14557

@NicolasDorier

This comment has been minimized.

Copy link
Member

NicolasDorier commented Oct 24, 2018

@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

This comment has been minimized.

Copy link
Contributor Author

ajtowns commented Oct 24, 2018

@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

This comment has been minimized.

Copy link
Member

NicolasDorier commented Oct 28, 2018

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

This comment has been minimized.

Copy link
Contributor Author

ajtowns commented Oct 29, 2018

@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

This comment has been minimized.

Copy link
Member

NicolasDorier commented Oct 30, 2018

I confirm you are right
My test was only

[test]
connect=0

So connect=0 was ignored.

Csi18nAlistairMann added a commit to Csi18nAlistairMann/bitcoin that referenced this pull request Feb 12, 2019
…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.
Csi18nAlistairMann added a commit to Csi18nAlistairMann/bitcoin that referenced this pull request Feb 12, 2019
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.
Csi18nAlistairMann added a commit to Csi18nAlistairMann/bitcoin that referenced this pull request Feb 12, 2019
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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.