Docs: Add note on precedence of options in bitcoin.conf #16076

torkelrogstad commented at 6:32 PM on May 22, 2019: contributor

This has been a confusing point for me, and I'm sure that applies for other people as well.

I figured this out by just experimenting a bit with different options and seeing what happens. If it is incorrect I'll of course change the PR.

torkelrogstad renamed this:
~~Add note on precedence of options in bitcoin.conf~~
Docs: Add note on precedence of options in bitcoin.conf
on May 22, 2019

fanquake added the label Docs on May 22, 2019

MarcoFalke commented at 6:38 PM on May 22, 2019: member

Great docs! Mind to add a test for this, so we don't accidentally regress on this?

in doc/bitcoin-conf.md:33 in 3c75ca618b outdated

  29 | @@ -30,6 +30,19 @@ Network specific options can be:
  30 |  - placed into sections with headers `[main]` (not `[mainnet]`), `[test]` (not `[testnet]`) or `[regtest]`;
  31 |  - prefixed with a chain name; e.g., `regtest.maxmempool=100`.
  32 |  
  33 | +Options prefixed with a chain name take precedence over options placed into sections with headers, which themselves

ryanofsky commented at 7:22 PM on May 22, 2019:

I think this is only true because there's no syntax for ending sections in the config file. Options prefixed with a section name and options written inside a section aren't treated differently or given different precedence. The reason why the rpcport is 3000 instead of 4000, is that the 3000 line is above the 4000 line and when options are duplicated in the config file, the first value is usually the one that sticks.

torkelrogstad commented at 8:54 PM on May 22, 2019:

That sounds correct to me. IMO it'd be a good idea to have syntax to end sections, but I'd say that's outside the scope of this PR.

hebasto commented at 5:51 PM on May 27, 2019:

https://github.com/bitcoin/bitcoin/blob/76e2cded477bc483ec610212bdadf21fe35292d4/src/util/system.cpp#L229-L237

laanwj commented at 9:48 AM on May 28, 2019:

ini format doesn't have a way to end sections, and IMO also doesn't particularly need it

ryanofsky commented at 11:39 AM on May 28, 2019:

My comment is saying the new documentation is wrong. It is not saying that that format should have a way to end sections. It is not true that "Options prefixed with a chain name take precedence over options placed into sections with headers." In fact, section.option=value and [section]↵option=value are interpreted exactly the same and have the same precedence. The difference in the example just comes from the order values are specified in the config file.

If we were going to change the format, my priority would be to first clean up the inconsistent behaviors documented in #15934: (command line ignored arguments and more ignored arguments, and config file reverse precedence, inconsistently applied top-level settings, and zombie values).

hebasto commented at 11:50 AM on May 28, 2019:

... and zombie values

IIIC, these zombie values appeared after 4d34fcc7138f0ffc831f0f8601c50cc7f494c197.

torkelrogstad commented at 10:30 AM on May 29, 2019:

@ryanofsky Do you have an example config file where what I described in this PR doesn't hold?

ryanofsky commented at 7:28 PM on May 22, 2019: member

Mind to add a test for this, so we don't accidentally regress on this?

I think util_ArgsMerge covers this and would prevent regressions (unless we actually did start treating [section]↵option=value and section.option=value differently).

torkelrogstad commented at 9:16 PM on May 22, 2019: contributor

@MarcoFalke I have not written a line of C++ in my life. Would the test be in C++ or Python? With some pointers I might be able to get something done.

hebasto commented at 9:28 AM on May 23, 2019: member

Concept ACK

JosuGZ commented at 12:34 PM on May 26, 2019: contributor

Concept ACK

laanwj commented at 9:46 AM on May 28, 2019: member

@MarcoFalke I have not written a line of C++ in my life. Would the test be in C++ or Python? With some pointers I might be able to get something done.

I don't think writing a test is required as part of a documentation PR, can do that later.

laanwj commented at 1:17 PM on June 13, 2019: member

So if the documentation here is incorrect, and does not actually describe the working of the software, can you please improve it, or should we close it?

torkelrogstad commented at 7:54 AM on June 14, 2019: contributor

It is still a bit unclear to me how the documentation is incorrect. Asked a question about it a while ago: https://github.com/bitcoin/bitcoin/pull/16076/files#r288499268

ping @ryanofsky

hebasto commented at 8:04 AM on June 14, 2019: member

@torkelrogstad

It is still a bit unclear to me how the documentation is incorrect.

The code itself does not give options prefixed with a chain name any precedence over options placed into sections with headers.

Add note on precedence of options in bitcoin.conf f6ba375086

torkelrogstad force-pushed on Jun 14, 2019

torkelrogstad commented at 9:42 AM on June 14, 2019: contributor

Updated the docs, I believe I have addressed your comments.

in doc/bitcoin-conf.md:33 in f6ba375086

  29 | @@ -30,6 +30,28 @@ Network specific options can be:
  30 |  - placed into sections with headers `[main]` (not `[mainnet]`), `[test]` (not `[testnet]`) or `[regtest]`;
  31 |  - prefixed with a chain name; e.g., `regtest.maxmempool=100`.
  32 |  
  33 | +Options that are either prefixed with a chain name or placed

ryanofsky commented at 4:49 PM on June 27, 2019:

In commit "Add note on precedence of options in bitcoin.conf" (f6ba37508663237c6f9d72d2d71a0307cbc6b78d)

I think this first sentence is long and awkward. I'd replace it with "Network specific options take precedence over non-network specific options." This new paragraph is under the "Network specific options" section, and the previous paragraph explains clearly what network specific options are, so explaining them differently here just makes this sentence complicated and disconnected.

in doc/bitcoin-conf.md:50 in f6ba375086

  45 | +
  46 | +[regtest]
  47 | +rpcport=4000
  48 | +```
  49 | +
  50 | +Note that there's no way to end a network section without

ryanofsky commented at 5:00 PM on June 27, 2019:

In commit "Add note on precedence of options in bitcoin.conf" (f6ba37508663237c6f9d72d2d71a0307cbc6b78d)

Would drop this paragraph. I think it's pointing out trivia that's not useful (and also kind of obvious if you think about it). I'm pretty sure no one has any need or desire to end network sections. The only reason I brought up ending network sections earlier is to explain why I couldn't write an example of network setting in a section taking precedence over a network setting with a prefix.

ryanofsky approved

ryanofsky commented at 5:05 PM on June 27, 2019: member

ACK f6ba37508663237c6f9d72d2d71a0307cbc6b78d. Thanks for the correction and new note about earlier options taking precedence.

ryanofsky commented at 5:28 PM on June 27, 2019: member

Looks like a linter on travis is also failing due to a trailing whitespace error.

torkelrogstad closed this on Jul 22, 2019

fanquake added the label Up for grabs on Jul 22, 2019

fanquake commented at 6:33 AM on July 24, 2019: member

Picked this up in #16448.

fanquake removed the label Up for grabs on Jul 24, 2019

DrahtBot locked this on Dec 16, 2021