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

AkioNak · 2018-12-04T10:03:30Z

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:

    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]

promag · 2018-12-04T14:39:04Z

This can break existing configurations correct?

AkioNak · 2018-12-05T03:04:25Z

@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?

practicalswift · 2018-12-05T07:57:37Z

src/util/system.cpp

+                if (key == "includeconf") {
+                    if (depth < 0) {
+                        continue;
+                    } else if (depth < MAX_INCLUDECONF_DEPTH) {


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

@practicalswift Thanks. Indeed!

promag · 2018-12-05T08:02:43Z

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

AkioNak · 2018-12-05T11:26:29Z

@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 86ff041) 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.

bitcoin.conf 
----
includeconf=global.conf
regtest.includeconf=port.conf
[regtest]
includeconf=user.conf

global.conf 
----
regtest=1
daemon=1

port.conf 
----
[regtest]
rpcport=18443
port=18442

user.conf 
----
[regtest]
rpcuser=user
rpcpassword=pass

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

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

kallewoof · 2018-12-11T11:24:14Z

@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 · 2018-12-13T11:48:04Z

@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 · 2018-12-13T13:34:03Z

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

DrahtBot · 2018-12-13T22:15:07Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Remove obsolete NODISCARD ifdef forest. Use [[nodiscard]] (C++17). #20499 (Remove obsolete NODISCARD ifdef forest. Use [[nodiscard]] (C++17). by practicalswift)
test: Change feature_config_args.py not to rely on strange regtest=0 behavior #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.

AkioNak · 2019-01-10T06:29:49Z

@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 · 2019-01-11T06:31:03Z

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

AkioNak · 2019-01-15T10:02:51Z

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

potential_deadlock_detected() reports following message:

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

AkioNak · 2019-01-16T15:06:18Z

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

kallewoof · 2019-01-17T04:51:45Z

utACK cca8a9196f491aa8e99d18846578307cd7004c45

AkioNak · 2019-01-21T06:30:35Z

squashed and rebased.

Sjors · 2019-02-21T17:27:12Z

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

[main]
includeconf=inc1.conf
test.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 · 2019-02-22T11:19:55Z

@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 · 2019-02-22T13:10:30Z

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.

AkioNak · 2019-03-04T11:43:23Z

rebased. (no functional change yet)

AkioNak · 2020-05-11T09:19:30Z

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

adamjonas · 2020-06-16T13:33:51Z

Hi @AkioNak - friendly monthly ping.

AkioNak · 2020-06-18T00:22:42Z

@adamjonas Thanks. I will do again.

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]

DrahtBot · 2020-11-30T14:52:53Z

🐙 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 · 2021-12-15T11:22:40Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

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 · 2021-12-16T12:29:02Z

I will solve the conflicts.

DrahtBot · 2022-03-21T13:07:58Z

There hasn't been much activity lately and the patch still needs rebase. What is the status here?

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 · 2022-03-21T16:22:28Z

Going to close with "Up for Grabs".

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

practicalswift reviewed Dec 5, 2018

View reviewed changes

bitcoin deleted a comment from dougstrong77 Dec 6, 2018

DrahtBot added the Needs rebase label Jan 9, 2019

AkioNak force-pushed the includeconf_eval_at_its_position branch from f63984b to 5c55f9a Compare January 10, 2019 08:42

DrahtBot removed the Needs rebase label Jan 10, 2019

AkioNak force-pushed the includeconf_eval_at_its_position branch from cca8a91 to c858ddc Compare January 21, 2019 04:54

AkioNak mentioned this pull request Feb 4, 2019

Fix lack of warning of unrecognized section names #15335

Merged

Sjors mentioned this pull request Feb 22, 2019

Remove the automatic creation and loading of the default wallet #15454

Merged

DrahtBot added the Needs rebase label Mar 2, 2019

AkioNak force-pushed the includeconf_eval_at_its_position branch from c858ddc to 613c62b Compare March 4, 2019 11:32

DrahtBot removed the Needs rebase label Mar 4, 2019

AkioNak force-pushed the includeconf_eval_at_its_position branch from 6430f92 to 042eb42 Compare May 11, 2020 07:51

DrahtBot removed the Needs rebase label May 11, 2020

DrahtBot mentioned this pull request May 11, 2020

test: Change feature_config_args.py not to rely on strange regtest=0 behavior #17556

Merged

DrahtBot mentioned this pull request May 30, 2020

Add new bitcoin_rw.conf file that is used for settings modified by this software itself #11082

Closed

maflcko added the Waiting for author label Jun 16, 2020

AkioNak force-pushed the includeconf_eval_at_its_position branch from 042eb42 to 12bacf3 Compare July 8, 2020 23:15

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

DrahtBot mentioned this pull request Jul 9, 2020

net: Extract download permission from noban #19191

Merged

DrahtBot added the Needs rebase label Jul 9, 2020

AkioNak force-pushed the includeconf_eval_at_its_position branch from 12bacf3 to 3b6b025 Compare July 10, 2020 02:20

DrahtBot removed the Needs rebase label Jul 10, 2020

AkioNak force-pushed the includeconf_eval_at_its_position branch from 3b6b025 to 8b20f4a Compare July 13, 2020 02:19

AkioNak force-pushed the includeconf_eval_at_its_position branch from 8b20f4a to de41f72 Compare August 6, 2020 15:09

dongcarl removed the Waiting for author label Aug 6, 2020

This was referenced Sep 29, 2020

refactor: Remove settings merge reverse precedence code #17581

Draft

util: Forbid ambiguous multiple assignments in config file #17493

Draft

DrahtBot mentioned this pull request Nov 26, 2020

Remove obsolete NODISCARD ifdef forest. Use [[nodiscard]] (C++17). #20499

Merged

DrahtBot added the Needs rebase label Nov 30, 2020

fanquake closed this Mar 21, 2022

fanquake added the Up for grabs label Mar 21, 2022

bitcoin locked and limited conversation to collaborators Mar 21, 2023

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

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

Uh oh!

Conversation

AkioNak commented Dec 4, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

bitcoin.conf

inc1.conf

inc2.conf

Uh oh!

promag commented Dec 4, 2018

Uh oh!

AkioNak commented Dec 5, 2018

Uh oh!

practicalswift Dec 5, 2018 • 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!

AkioNak Dec 5, 2018

Choose a reason for hiding this comment

Uh oh!

AkioNak Dec 6, 2018

Choose a reason for hiding this comment

Uh oh!

promag commented Dec 5, 2018

Uh oh!

AkioNak commented Dec 5, 2018

Uh oh!

kallewoof commented Dec 11, 2018

Uh oh!

laanwj commented Dec 13, 2018

Uh oh!

kallewoof commented Dec 13, 2018

Uh oh!

DrahtBot commented Dec 13, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

AkioNak commented Jan 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkioNak commented Jan 11, 2019

Uh oh!

AkioNak commented Jan 15, 2019

Uh oh!

AkioNak commented Jan 16, 2019

Uh oh!

kallewoof commented Jan 17, 2019

Uh oh!

AkioNak commented Jan 21, 2019

Uh oh!

Sjors commented Feb 21, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

AkioNak commented Feb 22, 2019

Uh oh!

Sjors commented Feb 22, 2019

Uh oh!

AkioNak commented Mar 4, 2019

Uh oh!

AkioNak commented May 11, 2020

Uh oh!

adamjonas commented Jun 16, 2020

Uh oh!

AkioNak commented Jun 18, 2020

Uh oh!

DrahtBot commented Nov 30, 2020

Uh oh!

DrahtBot commented Dec 15, 2021

Uh oh!

AkioNak commented Dec 16, 2021

Uh oh!

DrahtBot commented Mar 21, 2022

Uh oh!

fanquake commented Mar 21, 2022

Uh oh!

Uh oh!

AkioNak commented Dec 4, 2018 •

edited

Loading

practicalswift Dec 5, 2018 •

edited

Loading

DrahtBot commented Dec 13, 2018 •

edited

Loading

AkioNak commented Jan 10, 2019 •

edited

Loading

Sjors commented Feb 21, 2019 •

edited

Loading