[wip] util: Improve evaluation of includeconf lines in network sections of the config file

AkioNak commented at 10:03 am on December 4, 2018: contributor

This PR intends to make it easy to understand how the configuration files are interpreted.

Evaluate “includeconf” at the position described in the config file (like #include directive in C/C++), rather than at the end.
If once a section is specified with square brackets like [main], the only way to configure another network is to use square brackets again, e.g. [test].
In the “included” config file, the section is taken over from the “including” file. If no sections are specified yet, using square brackets or section prefix is still possible in the “included” file.
Changing the section in the “included” config file does not affects the including file. The section of the including file is kept.
If the “included” config file is included after the section has been specified by using square brackets or if they are included by using section prefix, any square brackets can not be used in that file.

ex) With Following 3 files, the properties are read as:

0    port=8444, main.port=8445, test.port=8442, regtest.port: undefined

bitcoin.conf

0test.includeconf=inc1.conf
1includeconf=inc2.conf
2port=8441   # ignored (2nd appearance of w/o section)

inc1.conf

0 port=8442   # test
1 [regtest]
2 port=8443   # ignored

inc2.conf

0 port=8444   # w/o section
1 [main]
2 port=8445   # main
3 [regtest]

fanquake added the label Utils/log/libs on Dec 4, 2018

promag commented at 2:39 pm on December 4, 2018: member

This can break existing configurations correct?

AkioNak commented at 3:04 am on December 5, 2018: contributor

@promag Thanks for your comment. Yes. Sometimes this may be different from previous results when interpreting existing configurations. Especially the possibility of interpretation unmatch increases if using “includeconf” and sections are specified with square brackets in the “included” configuration file. Is it necessary for this change to add a flag to specify how to read the configuration file?

in src/util/system.cpp:887 in af6f964439 outdated

889-                    sections.insert(name.substr(0, pos));
890+                options.emplace_back(name.empty() ? key : name + '.' + key, value);
891+                if (key == "includeconf") {
892+                    if (depth < 0) {
893+                        continue;
894+                    } else if (depth < MAX_INCLUDECONF_DEPTH) {

practicalswift commented at 7:57 am on December 5, 2018:

Nit: The else here is redundant due to the continue above.

AkioNak commented at 11:28 am on December 5, 2018:

@practicalswift Thanks. Indeed!

AkioNak commented at 7:44 am on December 6, 2018:

Done.

promag commented at 8:02 am on December 5, 2018: member

You should not break existing configurations, especially if there is no strong reason to do it.

AkioNak commented at 11:26 am on December 5, 2018: contributor

@promag My motivation of this PR is that I think it is very difficult to effectively use sections explicitly specified using prefix or use “includeconf”.

For example, If using following 4 config file, bitcoind (build from master 86ff0413b) never read port.conf and user.conf. Furthermore, it warns these files as nested. This cause is that the chain may be specipied in the course of processing the “includeconf"s collectively later.

 0bitcoin.conf 
 1----
 2includeconf=global.conf
 3regtest.includeconf=port.conf
 4[regtest]
 5includeconf=user.conf
 6
 7global.conf 
 8----
 9regtest=1
10daemon=1
11
12port.conf 
13----
14[regtest]
15rpcport=18443
16port=18442
17
18user.conf 
19----
20[regtest]
21rpcuser=user
22rpcpassword=pass

0$ ./src/bitcoind --version | head -1
1Bitcoin Core Daemon version v0.17.99.0-86ff0413b
2$ ./src/bitcoind 
3warning: -includeconf cannot be used from included files; ignoring -includeconf=port.conf
4warning: -includeconf cannot be used from included files; ignoring -includeconf=user.conf
5Bitcoin server starting

So I think evaluate "includeconf" step by step is better .

fanquake deleted a comment on Dec 6, 2018

kallewoof commented at 11:24 am on December 11, 2018: member

@promag The includeconf and sections features are both relatively recent. I think this is a bug-fix that should be back-ported, in fact, as the current behavior is completely nonsensical.

laanwj commented at 11:48 am on December 13, 2018: member

@promag The includeconf and sections features are both relatively recent

I think that’s a good point, does this only affect use of -includeconf?

kallewoof commented at 1:34 pm on December 13, 2018: member

@laanwj I believe so, yes, except for maybe the minor detail (item 4 in the OP list). @AkioNak?

DrahtBot commented at 10:15 pm on December 13, 2018: member

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20499 (Remove obsolete NODISCARD ifdef forest. Use [[nodiscard]] (C++17). by practicalswift)
#17556 (test: Change feature_config_args.py not to rely on strange regtest=0 behavior by ryanofsky)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

DrahtBot added the label Needs rebase on Jan 9, 2019

AkioNak commented at 6:29 am on January 10, 2019: contributor

@kallewoof @laanwj I’m so sorry for my late reply.

I believe so, yes, except for maybe the minor detail (item 4 in the OP list).

I think so too.

AkioNak force-pushed on Jan 10, 2019

DrahtBot removed the label Needs rebase on Jan 10, 2019

AkioNak commented at 6:31 am on January 11, 2019: contributor

Rebased but travis failed. Hmm,,, I will check what’s going on.

AkioNak commented at 10:02 am on January 15, 2019: contributor

The direct cause of failing travis was potential deadlock detection. The investigation will continue …

potential_deadlock_detected() reports following message:

0POTENTIAL DEADLOCK DETECTED
1Previous lock order was:
2 (1) cs_args  util/system.cpp:921
3 (2) csPathCached  util/system.cpp:790
4Current lock order is:
5 (2) csPathCached  util/system.cpp:790
6 (1) cs_args  util/system.cpp:531

AkioNak commented at 3:06 pm on January 16, 2019: contributor

Addressed to the potential deadlock by narrowing the range of LOCK(cs_args).

kallewoof commented at 4:51 am on January 17, 2019: member

utACK cca8a9196f491aa8e99d18846578307cd7004c45

AkioNak force-pushed on Jan 21, 2019

AkioNak commented at 6:30 am on January 21, 2019: contributor

squashed and rebased.

Sjors commented at 5:27 pm on February 21, 2019: member

Concept ACK, except I think the following example (from the description) should just fail:

0[main]
1includeconf=inc1.conf
2test.includeconf=inc2.conf

Once a network is specified with square brackets like [main], the only way to configure another network should be to use square brackets again, e.g. [testsnet]. That rule should also apply to anything included by includeconf.

I’m also tempted to suggest a rule that square brackets may not exist in both bitcoin.conf and and includeconf files. Or, slightly less strict, that square brackets may not exist in includeconf files if they are included after a square bracket.

It’s important to simplify this behavior sufficiently so we stand a chance of one day merging #11082 :-)

AkioNak commented at 11:19 am on February 22, 2019: contributor

@Sjors Thank you for your surggestions.

Once a network is specified with square brackets like [main], the only way to configure another network should be to use square brackets again, e.g. [testsnet]. That rule should also apply to anything included by includeconf.

Good idea. I agree.

Or, slightly less strict, that square brackets may not exist in includeconf files if they are included after a square bracket.

I like this. And I think square brackets may not exist in the files that are included by using “section.includeconf” style, too.

Sjors commented at 1:10 pm on February 22, 2019: member

Ok, looking forward to the new version. I think it would be very useful to add a lot more tests to feature_config_args.py, given that you were able to make these changes without breaking any existing test.

DrahtBot added the label Needs rebase on Mar 2, 2019

AkioNak force-pushed on Mar 4, 2019

AkioNak commented at 11:43 am on March 4, 2019: contributor

rebased. (no functional change yet)

DrahtBot removed the label Needs rebase on Mar 4, 2019

AkioNak commented at 4:54 pm on March 4, 2019: contributor

Once a network is specified with square brackets like [main], the only way to configure another network should be to use square brackets again, e.g. [testsnet]. That rule should also apply to anything included by includeconf.

Added rules for how to use square brackets & preriods. Also added some tests. @Sjors Could you review this?

AkioNak force-pushed on Mar 13, 2019

AkioNak force-pushed on Mar 14, 2019

AkioNak commented at 5:30 am on March 14, 2019: contributor

Squashed. (Also updated commit log and pull request description.)

DrahtBot added the label Needs rebase on Jun 18, 2019

AkioNak force-pushed on Jun 20, 2019

DrahtBot removed the label Needs rebase on Jun 20, 2019

AkioNak commented at 8:34 am on June 20, 2019: contributor

Rebased.

DrahtBot added the label Needs rebase on Jul 24, 2019

AkioNak force-pushed on Aug 13, 2019

DrahtBot removed the label Needs rebase on Aug 13, 2019

AkioNak commented at 5:19 am on August 13, 2019: contributor

Rebased.

jtimon commented at 10:49 pm on October 10, 2019: contributor

I don’t like that in the example port=8441 in bitcoin.conf is applied only to regtest just because inc2.conf ended with [regtest]. I feel like I could get crazy for a few hours just by forgetting inc2.conf ends with [regtest] if, let’s say, it previously ended with [main] but I moved chain sections around in inc.conf or something.

Looking at the example, I just can’t avoid wondering why the user can’t just add [regtest] to bitcoin.conf like this:

0test.includeconf=inc1.conf
1includeconf=inc2.conf
2[regtest]
3port=8441   # regtest

or just put the regtest port in inc2.conf like this.

0port=8444   # w/o section
1[main]
2port=8445   # main
3[regtest]
4port=8441   # regtest

I’ve personally never used includeconf myself, so perhaps I just don’t understand how people are using this feature.

Perhaps this can be done in another PR, but I feel like after includeconf, you should go back to whatever section you were in bitcoin.conf, not remain in the last one set by the included file.

AkioNak commented at 5:27 am on October 11, 2019: contributor

@jtimon Thank you for the comment!

after includeconf, you should go back to whatever section you were in bitcoin.conf, not remain in the last one set by the included file.

It makes sense. That behavior is natural so I will address.

AkioNak force-pushed on Oct 17, 2019

AkioNak force-pushed on Oct 18, 2019

AkioNak commented at 9:01 am on October 18, 2019: contributor

@jtimon I have addressed. (Changing section in the included file never affect to the including file.)

DrahtBot added the label Needs rebase on Oct 28, 2019

AkioNak force-pushed on Oct 29, 2019

DrahtBot removed the label Needs rebase on Oct 29, 2019

AkioNak commented at 10:01 am on October 29, 2019: contributor

Rebased.

DrahtBot added the label Needs rebase on Nov 8, 2019

Sjors commented at 1:26 pm on November 15, 2019: member

Concept re-ACK. The latest set of 5 criteria in the PR description look very reasonable (update: as ryanofsky points out, some of this is already the case, which we should document, and maybe test) .

Needs rebase after @ryanofsky’s settings merge refactor in #15934 (which also added lots of useful tests). I think the increased sanity warrants (if needed) breaking backwards compatibility.

ryanofsky commented at 2:20 pm on November 15, 2019: member

Concept ACK, I think. But I am having a trouble understanding the description, because it seems to be describing a lot of current behavior (like points 2 and 4) that the PR doesn’t actually affect, and not be clear about what is changing.

Would the following be an accurate description of what the PR does?

Included configurations from includeconf lines in [main] [test] and [regtest] sections of the config file are now evaluated in context of that section and not treated like top-level includes. Config files included within a section are also now disallowed from changing settings in other sections.

If that is accurate, it would also be helpful to add it as an entry to: https://github.com/bitcoin/bitcoin/blob/master/doc/release-notes.md

The documentation https://github.com/bitcoin/bitcoin/blob/master/doc/bitcoin-conf.md should also be updated. I think most of the text in the current PR description should be removed and incorporated into that file instead.

I would also change the title of the PR to better describe what it is doing. Instead of “Improve property evaluation way in bitcoin.conf” maybe “util: Improve evaluation of includeconf lines in network sections of the config file”

AkioNak commented at 1:14 am on November 18, 2019: contributor

@Sjors @ryanofsky Thank you for your comments/suggestions. I will rebase and also address the suggestions above.

adamjonas commented at 0:54 am on May 1, 2020: member

@AkioNak pinging for a rebase and addressing the comments by ryanofsky and Sjors.

AkioNak commented at 2:38 am on May 1, 2020: contributor

@adamjonas I will do.

AkioNak force-pushed on May 11, 2020

DrahtBot removed the label Needs rebase on May 11, 2020

AkioNak commented at 9:19 am on May 11, 2020: contributor

Rebased. I am still working on addressing the comments by ryanofsky and Sjors.

adamjonas commented at 1:33 pm on June 16, 2020: member

Hi @AkioNak - friendly monthly ping.

MarcoFalke added the label Waiting for author on Jun 16, 2020

AkioNak commented at 0:22 am on June 18, 2020: contributor

@adamjonas Thanks. I will do again.

AkioNak force-pushed on Jul 8, 2020

AkioNak renamed this:
~~Improve property evaluation way in bitcoin.conf~~
[wip] util: Improve evaluation of includeconf lines in network sections of the config file
on Jul 8, 2020

DrahtBot added the label Needs rebase on Jul 9, 2020

AkioNak force-pushed on Jul 10, 2020

DrahtBot removed the label Needs rebase on Jul 10, 2020

AkioNak force-pushed on Jul 13, 2020

util: Improve evaluation of includeconf lines

Improve property evaluation way in bitcoin.conf

This PR intends to make it easy to understand how the configuration
files are interpreted.

1. Evaluate "includeconf" at the position described in the config file
(like #include directive in C/C++), rather than at the end.

2. If once a section is specified with square brackets like [main],
the only way to configure another network is to use square brackets
again, e.g. [test].

3. In the "included" config file, the section is taken over from
the "including" file. If no sections are specified yet, using square
brackets or section prefix is still possible in the "included" file.

4. Changing the section in the "included" config file does not affects
the including file. The section of the including file is kept.

5. If the "included" config file is included after the section has been
specified by using square brackets or if they are included by using
section prefix, any square brackets can not be used in that file.

ex) With Following 3 files, the properties are read as:
port=8444, main.port=8445, test.port=8442, regtest.port: undefined

bitcoin.conf
---
test.includeconf=inc1.conf
includeconf=inc2.conf
port=8441   # ignored (2nd appearance of w/o section)

inc1.conf
---
port=8442   # test
[regtest]
port=8443   # ignored

inc2.conf
---
port=8444   # w/o section
[main]
port=8445   # main
[regtest]

de41f72b4a

AkioNak force-pushed on Aug 6, 2020

dongcarl removed the label Waiting for author on Aug 6, 2020

DrahtBot added the label Needs rebase on Nov 30, 2020

DrahtBot commented at 2:52 pm on November 30, 2020: member

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot commented at 11:22 am on December 15, 2021: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

AkioNak commented at 12:29 pm on December 16, 2021: contributor

I will solve the conflicts.

DrahtBot commented at 1:07 pm on March 21, 2022: member

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

fanquake commented at 4:22 pm on March 21, 2022: member

Going to close with “Up for Grabs”.

fanquake closed this on Mar 21, 2022

fanquake added the label Up for grabs on Mar 21, 2022

DrahtBot locked this on Mar 21, 2023

[wip] util: Improve evaluation of includeconf lines in network sections of the config file #14866

bitcoin.conf

inc1.conf

inc2.conf

Conflicts