Fixes: #10071.
Done:
-includeconf=<path>, where <path> is relative to datadir or to the path of the file being read, if in a file0- ~~~I am not sure how to test this in a neat manner. Feedback on this would be nice. Will dig/think though.~~~
Yeah. Why not. This can be useful.
-addconf= (otherwise user may think it replaces the bitcoin.conf configuration file).GetConfigFile() correctly, you can also use absolut paths, right?
-config a repeatable argument.
-addconf=. Yes, you can use absolute paths. My worry above is for when a user presumes the path is relative to the config file when it is in fact not.
@NicolasDorier I think -config simply tells what name to use and defaults to bitcoin.conf – it doesn’t actually load the file. This feature lets you load other files arbitrarily from within bitcoin.conf.
Concept ACK
My worry above is for when a user presumes the path is relative to the config file when it is in fact not.
Yes, making it relative to the data directory is a good choice. I think we should handle all relative paths in bitcoind that way.
Why not making the existing -config a repeatable argument.
That was also my first thought, but it may just be confusing as it changes the meaning of the option slightly. It’s possible that some setups already use multiple -conf options, and rely on the overriding behavior.
So I’m good with making it an explicit option. Another suggestion for the name would be -includeconf.
From the given suggestions I think includeconf is the most clear so I’ll switch to that.
@laanwj:
Yes, making it relative to the data directory is a good choice. I think we should handle all relative paths in bitcoind that way.
To clarify, you mean that the relative path inside /dir/file.conf should be /dir/, not [bitcoin datadir], right? It will require some lines of code I bet but I think that makes sense too.
The includeconf feature now defines the path relative to the file being read, if any. For command line, it is [datadir], for /dir/abc.conf it is /dir/. I tested this with
0src/testreadconfig/bitcoin.conf: [...] includeconf=../global.conf
1src/global.conf: includeconf=secrets.conf
2src/secrets.conf: rpcpassword=foo
with bitcoind -datadir=testreadconfig. Ensured bitcoin-cli with password foo worked and password bar did not.
To clarify, you mean that the relative path inside /dir/file.conf should be /dir/, not [bitcoin datadir], right? It > will require some lines of code I bet but I think that makes sense too.
Yes, seems good to me too. So it’s like C’s include "" - I wasn’t thinking about relative includes in other includes.
I am not sure how to test this in a neat manner. Feedback on this would be nice. Will dig/think though.
Some ideas:
-includeconf=<path>To achieve the latter the option uacomment= is useful, as these will be added to an array, then querying getnetworkinfo to see if the (...) part of the subversion matches expected content and order.
Thanks for the suggestion! I added a test that checks for load order and ensures circular include is guarded against. @jnewbery review would be wonderful :)
Edit: If anyone has ideas why travis is failing I’d appreciate it. It works fine on all the machines I test it on locally (mac, linux).
If anyone has ideas why travis is failing I’d appreciate it
You’ve made ReadConfigFile() recursive (through ProcessSetting()). ReadConfigFile() locks cs_args, and then at the end calls ClearDatadirCache(), which locks csPathCached. That means that the bottom-most ReadConfigFile() locks csPathCached while cs_args is still held.
There’s already a function that locks in the other order: GetDataDir() locks csPathCached and then locks cs_args (in its call to IsArgSet()).
If those two functions are called in different threads, we’d have a deadlock.
There’s a CPP_FLAG option that checks lock ordering CPPFLAGS=-DDEBUG_LOCKORDER, which is used in Travis build 5. That’s why that build is failing. You can repro locally by running configure with that option.
You can fix this by not locking cs_args recursively.
@jnewbery Thanks a lot for the explanation! I should’ve paid closer attention to locks considering the added recursiveness.
97ee63b fixes this by moving the conditionally-locked code into a new ReadConfigStream function which is called with locking/clearing in one case and without in the other, based on a bool lockAndClear added to ReadConfigFile.
(Also had to tweak tests a tiny bit; 8fb6511.)
I think you’ve introduced a subtle bug here. If -datadir is configured in one of the additional config files, then the datadir cache won’t be cleared, which means that bitcoind will continue to use the old datadir.
I think you should try to not make ReadConfigFile recursive. For me, it would be acceptable to only allow one level of redirection here (ie the “base” config file can specify -includeconf config file, but those included config files cannot themselves include other config files). I think that would be a simpler model and would remove a whole bunch of potential bugs (circular references, blowing the stack through too many -includeconf redirects, etc).
@jnewbery Hm, no the datadir cache is cleared after any recursions happen, which means it is always cleared, just not directly after the config file has been parsed. There are two cases:
ReadConfigFile with the lockAndClear flag set; recursion then happens in ProcessSetting (L401) via ReadConfigStream (L622) with lockAndClear unset. Eventually this gets back to original caller which leaves ReadConfigStream and gets to util.cpp:649 which clears the datadir cache.
Or am I missing something?As for forbidding multiple levels of recursion, I think the value outweighs the issues personally (and I addressed circular refs I believe), but if people think it’s not worth it I’ll restrict it to one include.
@kallewoof yes you’re right. datadir cache is cleared after all files are read. My mistake.
I still don’t like the recursion and the fact that there can be multiple levels of imports. It means there are more edge cases and unexpected behaviour. For example, if -includeconf is included as a command line parameter, then the includeconf file is read before the regular conf file, and so takes precedence. If an includeconf line is included in the regular conf file, then it is read during the regular config file, and which settings are taken from the regular conf file and which are taken from the includeconf file depend on the ordering of settings in the regular conf file.
The new warnOnFailure and lockAndClear bool arguments to ReadConfigFile() seem pretty strange to me. They’re only used when ReadConfigFile() is being called recursively, and they control a large chunk of the behaviour within ReadConfigFile(). That’s a clue to me that maybe the functionality isn’t split up correctly - perhaps the locking/clearing should be in an outer function which calls an inner function for each of the config files?
Finally, you’ve introduced a new crash bug. If -conf or -includeconf don’t refer to a valid file, the bitcoind will crash on startup. Here’s the backtrace:
0Core was generated by `bitcoind -datadir=/tmp/user/1000/test6p7xn_xt/856/node0 -server -keypool=1 -dis'.
1Program terminated with signal SIGABRT, Aborted.
2[#0](/bitcoin-bitcoin/0/) 0x00007f3b77df0428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
354 ../sysdeps/unix/sysv/linux/raise.c: No such file or directory.
4(gdb) bt
5[#0](/bitcoin-bitcoin/0/) 0x00007f3b77df0428 in __GI_raise (sig=sig@entry=6) at ../sysdeps/unix/sysv/linux/raise.c:54
6[#1](/bitcoin-bitcoin/1/) 0x00007f3b77df202a in __GI_abort () at abort.c:89
7[#2](/bitcoin-bitcoin/2/) 0x00007f3b7873284d in __gnu_cxx::__verbose_terminate_handler() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
8[#3](/bitcoin-bitcoin/3/) 0x00007f3b787306b6 in ?? () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
9[#4](/bitcoin-bitcoin/4/) 0x00007f3b78730701 in std::terminate() () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
10[#5](/bitcoin-bitcoin/5/) 0x00007f3b78730919 in __cxa_throw () from /usr/lib/x86_64-linux-gnu/libstdc++.so.6
11[#6](/bitcoin-bitcoin/6/) 0x00007f3b79f29f82 in boost::filesystem::detail::canonical(boost::filesystem::path const&, boost::filesystem::path const&, boost::system::error_code*) () from /usr/lib/x86_64-linux-gnu/libboost_filesystem.so.1.58.0
12[#7](/bitcoin-bitcoin/7/) 0x000055e5c58b2ae2 in boost::filesystem::canonical (base=..., p=...) at /usr/include/boost/filesystem/operations.hpp:459
13[#8](/bitcoin-bitcoin/8/) GetConfigFile (confPath="global.conf", relativePath="") at util.cpp:603
14[#9](/bitcoin-bitcoin/9/) 0x000055e5c58b4353 in ArgsManager::ProcessSetting (this=this@entry=0x55e5c5db1fa0 <gArgs>, strKey="-includeconf", strValue="global.conf", relativePath="") at util.cpp:386
15[#10](/bitcoin-bitcoin/10/) 0x000055e5c58b487d in ArgsManager::ParseParameters (this=0x55e5c5db1fa0 <gArgs>, argc=<optimized out>, argv=<optimized out>) at util.cpp:424
16[#11](/bitcoin-bitcoin/11/) 0x000055e5c5653d4a in ParseParameters (argv=0x7fff5492e618, argc=14) at util.h:263
17[#12](/bitcoin-bitcoin/12/) AppInit (argc=14, argv=0x7fff5492e618) at bitcoind.cpp:75
18[#13](/bitcoin-bitcoin/13/) 0x000055e5c5648bef in main (argc=14, argv=0x7fff5492e618) at bitcoind.cpp:196
19(gdb) quit
If I use an invalid filename for -conf on master. I don’t see this crash.
@jnewbery Thanks a lot for all the feedback.
I still don’t like the recursion and the fact that there can be multiple levels of imports. It means there are more edge cases and unexpected behaviour. For example, if -includeconf is included as a command line parameter, then the includeconf file is read before the regular conf file, and so takes precedence. If an includeconf line is included in the regular conf file, then it is read during the regular config file, and which settings are taken from the regular conf file and which are taken from the includeconf file depend on the ordering of settings in the regular conf file.
That is the case without recursion as well, unless we forbid command line case. (Which I can do already, even while keeping recursion.)
The new warnOnFailure and lockAndClear bool arguments to ReadConfigFile() seem pretty strange to me. They’re only used when ReadConfigFile() is being called recursively, and they control a large chunk of the behaviour within ReadConfigFile(). That’s a clue to me that maybe the functionality isn’t split up correctly - perhaps the locking/clearing should be in an outer function which calls an inner function for each of the config files?
Hm.. the warnOnFailure was just a nice-to-have to inform the user when an explicitly included file didn’t actually exist, but I can remove it for cleanliness. Since I moved most of ReadConfigFile into ReadConfigStream, the only remaining stuff was the caching stuff, which doesn’t feel odd to me. I’m not actually sure why you consider this to be a problem: the ReadConfigFile is mostly there to do or not do the locking and data cache clearing, and the ReadConfigStream is there to do the actual reading/parsing part.
That said, I’m not overly attached to the idea of allowing recursion, so unless someone speaks for it I am going to try to simplify the code to only allow one single include and to only allow it in the file, i.e. not from command line. I believe that would address most of your concerns.
Edit: Oh, and thanks for finding the crash – I was sure I tested that, but I guess not.
Edit 2: Yeah, I never tested the case where the path was not a valid path, only when it was a non-existent one.
607+ }
608+ mapMultiArgs[strKey].push_back(strValue);
609+ }
610+}
611+
612 void ArgsManager::ReadConfigFile(const std::string& confPath)
I think it’d be clearer to rename this to ReadConfigFiles(). You could remove the confPath argument since it’s a property of the ArgsManager class. The three places where this is called can just call gArgs.ReadConfigFiles().
The responsibilities of this function then becomes very clear: read all the config files.
591@@ -592,26 +592,41 @@ fs::path GetConfigFile(const std::string& confPath)
592 return pathConfigFile;
593 }
594
595+void ArgsManager::ReadConfigStream(fs::ifstream& streamConfig)
ReadInputStream() is a little misleading.
ifstream as argument, but you’re right that it’s probably better to just name it ReadConfigFile.
634+ if (mapArgs.count("-includeconf")) includeconf = mapArgs["-includeconf"];
635+ }
636+ if (includeconf != "") {
637+ fs::path includeFile = GetConfigFile(includeconf);
638+ fs::ifstream includeConfig(includeFile);
639+ if (includeConfig.good()) {
632- mapMultiArgs[strKey].push_back(strValue);
633+ ReadConfigStream(streamConfig);
634+ if (mapArgs.count("-includeconf")) includeconf = mapArgs["-includeconf"];
635+ }
636+ if (includeconf != "") {
637+ fs::path includeFile = GetConfigFile(includeconf);
Why not directly:
0fs::ifstream includeConfig(GetConfigFile(includeconf));
(like above)
591@@ -592,26 +592,41 @@ fs::path GetConfigFile(const std::string& confPath)
592 return pathConfigFile;
593 }
594
595+void ArgsManager::ReadConfigStream(fs::ifstream& streamConfig)
596+{
597+ std::set<std::string> setOptions;
Is it possible to add a log to this function to output which file it’s reading, or is it too early to start logging?
Sorry - by my earlier comment, I didn’t mean you should remove the logging on failure, just that the structure of the function suggested to me that it shouldn’t be called recursively.
22+ def __init__(self):
23+ super().__init__()
24+ self.setup_clean_chain = False
25+ self.num_nodes = 1
26+
27+ def setup_network(self):
setup_network() in the base class just calls setup_nodes() when there’s only one node.
25+ self.num_nodes = 1
26+
27+ def setup_network(self):
28+ self.setup_nodes()
29+
30+ def run_test (self):
11+
12+ def setup_chain(self):
13+ super().setup_chain()
14+ # Create additional config file
15+ # - tmpdir/node0/relative.conf
16+ with open(os.path.join(self.options.tmpdir+"/node0", "relative.conf"), "w", encoding="utf8") as f:
+ please
26+
27+ def setup_network(self):
28+ self.setup_nodes()
29+
30+ def run_test (self):
31+ '''
0@@ -0,0 +1,43 @@
1+#!/usr/bin/env python3
It’d be nice to have a couple of other subtests:
--includeconf in the command line argument has no effectincludeconf in an includeconf file has no effect.
if __name__ == '__main__':?
These don’t actually need to be separate tests. You can just have the following in setup_chain():
0 with open(os.path.join(self.options.tmpdir + "/node0", "relative.conf"), "w", encoding="utf8") as f:
1 f.write("uacomment=relative\nincludeconf=relativeofrelative.conf\n")
2 with open(os.path.join(self.options.tmpdir + "/node0", "bitcoin.conf"), 'a', encoding='utf8') as f:
3 f.write("uacomment=main\nincludeconf=relative.conf\n")
4 with open(os.path.join(self.options.tmpdir + "/node0", "commandline.conf"), "w", encoding="utf8") as f:
5 f.write("uacomment=commandline\n")
6 with open(os.path.join(self.options.tmpdir + "/node0", "relativeofrelative.conf"), "w", encoding="utf8") as f:
7 f.write("uacomment=relativeofrelative\n")
and the following line in __init__():
0 self.extra_args = [['-includeconf=commandline.conf']]
and then keep the same assert:
0 assert subversion.endswith("main; relative)/")
to check that neither comandline.conf nor relativeofrelative.conf have been included.
(I recommend you also update the docstring to describe what’s happening)
This looks much much better. Thanks @kallewoof! I much prefer this implementation, which restricts the way --includeconf can be used and cuts out lots of edge cases.
If it turns out that people need more flexibility, we can extend this later to allow multiple includeconfs, including multiple levels. That’s much easier than going the other way (from less restrictive to more restrictive).
I have a few nits, mainly around naming.
I think there’s one bug, or at least confusing behaviour. You said in your comment that you don’t allow -includeconf from the command line, but your new code is allowing that. Further, if an -includeconf is included on the command line and in the base config file, then the command line -importconf takes precedence. I don’t think that’s what we want. To fix that, I think the first thing ReadConfigFile() should do clear the -includeconf argument if it’s set.
Looks great. Tested ACK bc4f7a4b3f614dc2125c6af60da448606b622688
One suggestion for adding to the testcase. Up to you whether you want to take it.
This conflicts a little bit with https://github.com/bitcoin/bitcoin/pull/8994/commits/7246faea3e4421e609e6a24294bd23a45d2343de
There I use the old ArgsManager::ReadConfigFile(path) which this PR remove. Could you conserve that method even if it’s temporarily unused (although preferrably using it internally like “ArgsManager::ReadConfigFile(fs::ifstream& streamConfig)”)?
0void ArgsManager::ReadConfigFile(const std::string& confPath)
1{
2 fs::ifstream streamConfig(GetConfigFile(confPath));
3 if (!streamConfig.good())
4 return; // No bitcoin.conf file is OK
5// ...
6}
Or perhaps I should restore it later if this PR gets merged first. utACK bc4f7a4b3f614dc2125c6af60da448606b622688 beyond that.
650+ ReadConfigFile(includeConfig);
651+ } else {
652+ LogPrintf("Failed to include configuration file %s\n", includeconf.c_str());
653 }
654 }
655 // If datadir is changed in .conf file:
ArgsManager::ReadConfigFile(const std::string& confPath), then that will presumably call call ArgsManager::ReadConfigFile(fs::ifstream& streamConfig too.
ArgsManager::ReadConfigFile(const std::string& confPath). The new ReadConfigFiles() will work just fine here, but in #8994 I cannot use it, because want I want is precisely to load from a different file (and not allow -includeconf or command line for “custom chainparams” configuration). If I use AgsManager::ReadConfigFile(fs::ifstream& streamConfig) directly from chainparams.cpp, not only I will duplicate code, but also call things like GetConfigFile(), which I would prefer not to call from chainparams.cpp. So my conclusion is that I would just restore ArgsManager::ReadConfigFile(const std::string& confPath). That’s why I ask that you maintain it even if you don’t need it for anything (but it can also be removed and then restored, it’s not a big deal)
I don’t want the chain custom parameters to be perceived as “config”.
I actually think having the customchain config file contain general config could be useful. There seemed to be some enthusiasm for #9374 , which is similar in nature - it allows a separate config file for each separate chain.
This is a bit of a sidetrack from this PR though, which I think is a good and useful improvement. This needs rebasing because of a conflict in test_runner.py. Assuming just that needs changing, then I still ACK this. I’ll give some more feedback in #9374.
alternative: this PR could move the:
0fs::ifstream streamConfig(GetConfigFile(confPath));
1if (!streamConfig.good())
lines into the new ReadConfigFile() function instead of leaving them in ReadConfigFiles(), and have ReadConfigFile() take a std::string instead of a fs::ifstream&. That would remove some code duplication (since that’s called for both the ‘base’ config file and the -includeconf file).
#9374 could then call ReadConfigFile() exactly as before.
@jnewbery I would need to either change the return value to be a success flag for streamConfig.good(), or add an additional bool warnOnFailure flag, like I had before, as we currently don’t warn for the main config missing, but we do warn for includeconfs.
Rebased, btw.
Required rebase after gArgs PR #10607. I’ve rebased here: https://github.com/jnewbery/bitcoin/tree/pr10267
I’ve also implemented my suggested change to make ReadConfigFile take a std::string here: https://github.com/jnewbery/bitcoin/tree/pr10267.1.
@kallewoof - can you let me know what you think? I think this resolves @jtimon’s concerns so this doesn’t conflict badly with his PR.
@kallewoof - no problem. My branch was just an example. Feel free to do whatever you want with it. (for future reference if you want to take the commits from my branch, easiest way is to add github.com/jnewbery/bitcoin as a remote, then git fetch from my remote, then git reset --hard <sha of my branch> from your branch).
For snake case convention, see ‘symbol naming conventions’ here: https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#developer-notes . No need to change old code to use the conventions, but new PRs should follow them if possible.
@jnewbery That’s what I did, but when I tried rebasing master on top of it things exploded. I’ll try again.
As for the symbol naming convention, I’m amazed I didn’t catch that considering I’m a big fan of conventions myself. Will definitely start using it. The m_foo thing for class member vars looks ugly as hell to me, but ah well.
Edit: worked fine this time. Must’ve fatfingered something. Thanks!
@jnewbery Digging, but:
0POTENTIAL DEADLOCK DETECTED
1Previous lock order was:
2 (1) cs_args util.cpp:628
3 (2) csPathCached util.cpp:558
4Current lock order is:
5 (2) csPathCached util.cpp:558
6 (1) cs_args util.cpp:432
7Assertion failed: (false), function potential_deadlock_detected, file sync.cpp, line 98.
It looks like there are two paths that may end up in a deadlock:
ReadConfigFiles() [locks cs_args]ReadConfigFile(conf_path)GetConfigFile(conf_path)GetDataDir(false) [locks csPathCached]gArgs.IsArgSet("-datadir") [locks cs_args]GetDataDir(_) [locks csPathCached]gArgs.IsArgSet("-datadir") [locks cs_args]Path 1 will lock cs_args, then csPathCached, and path 2 will lock csPathCached, then cs_args. I’m not sure the effect of path 1 locking cs_args twice. Perhaps it’s a NOP when already locked, but in either case, this seems to be the issue.
I’ve reset to the original version (rebased) for now as I can’t figure this one out right now.
-regtest.includeconf for example
43@@ -44,6 +44,10 @@ Specify configuration file (default: bitcoin.conf)
44 .IP
45 Specify data directory
46 .HP
47+\fB\-includeconf=\fR<dir>
48@@ -49,6 +49,10 @@ Run in the background as a daemon and accept commands
49 .IP
50 Specify data directory
51 .HP
52+\fB\-includeconf=\fR<dir>
381@@ -382,6 +382,7 @@ QT_TRANSLATE_NOOP("bitcoin-core", "Specified -walletdir \"%s\" is not a director
382 QT_TRANSLATE_NOOP("bitcoin-core", "Specify configuration file (default: %s)"),
383 QT_TRANSLATE_NOOP("bitcoin-core", "Specify connection timeout in milliseconds (minimum: 1, default: %d)"),
384 QT_TRANSLATE_NOOP("bitcoin-core", "Specify data directory"),
385+QT_TRANSLATE_NOOP("bitcoin-core", "Specify additional configuration file, relative to the -datadir path"),
337@@ -338,6 +338,7 @@ std::string HelpMessage(HelpMessageMode mode)
338 if (showDebug) {
339 strUsage += HelpMessageOpt("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize));
340 }
341+ strUsage += HelpMessageOpt("-includeconf=<file>", _("Specify additional configuration file, relative to the -datadir path"));
673- InterpretNegativeSetting(strKey, strValue);
674- if (mapArgs.count(strKey) == 0)
675- mapArgs[strKey] = strValue;
676- mapMultiArgs[strKey].push_back(strValue);
677+ ReadConfigFile(streamConfig);
678+ if (mapArgs.count("-includeconf")) includeconf = mapArgs["-includeconf"];
-includeconf arguments in the main config file (for example, if we want a main.includeconf, test.includeconf and regtest.includeconf). Can you make this a string vector and then iterate through the includeconfs?
0@@ -0,0 +1,38 @@
1+#!/usr/bin/env python3
feature_includeconf
A few comments inline.
I suggest you squash commits 1 and 2 (and please remove the # from the commit message!)
I’ve made some suggested changes to the functional test here: https://github.com/jnewbery/bitcoin/commit/968c9158f0bffac8c91282f1789a5a1808ed13b9 . Feel free to take any of the code you want and squash into your commit.
Needs release notes.
The travis job is timing out when running make check (the unit tests):
0$ if [ "$RUN_TESTS" = "true" ]; then travis_wait 40 make $MAKEJOBS check VERBOSE=1; fi 1913.41s
Here’s the end of the output from that command:
0make[1]: Leaving directory `/home/travis/build/bitcoin/bitcoin/build/bitcoin-i686-w64-mingw32'
1/home/travis/.travis/job_stages: line 169: 28077 Terminated travis_jigger $! $timeout $cmd
@MarcoFalke / @laanwj - any idea why the unit tests would stall for half an hour here?
@MarcoFalke / @laanwj - any idea why the unit tests would stall for half an hour here?
Something must have changed recently, but I think one reason why tests are slow is that they are running using wine to emulate windows. I’m also seeing the same timeouts with 11851.
10+ file to be loaded in the correct order.
11+2. includeconf cannot be used as a command line argument.
12+3. includeconf cannot be used recursively (ie includeconf can only
13+ be used from the base config file).
14+4. multiple includeconf arguments can be specified in the main config
15+ file (Currently fails).
(Currently fails)
I am fine with bumping to 40 or 50 minutes. In case we need that much time, it will time out due to the global limit anyway. The long term solution would be to somehow speed up the unit tests on Wine, but that doesn’t have to block this (and other) pull requests.
Could cherry-pick that change to a separate pull request, though.
"-includeconf" and "-" + GetChainName() + ".includeconf" explicitly.
357@@ -358,6 +358,7 @@ std::string HelpMessage(HelpMessageMode mode)
358 if (showDebug) {
359 strUsage += HelpMessageOpt("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize));
360 }
361+ strUsage += HelpMessageOpt("-includeconf=<file>", _("Specify additional configuration file, relative to the -datadir path"));
961 // ok to not have a config file
962 if (stream.good()) {
963 ReadConfigStream(stream);
964+ std::vector<std::string> includeconf(GetArgs("-includeconf"));
965+ {
966+ std::vector<std::string> includeconf_net(GetArgs(std::string("-") + GetChainName() + ".includeconf"));
I was going to say that this isn’t required because ArgsManager::GetArgs() already checks the network-specific config. However, we haven’t yet set m_network (that only happens in SelectParams()) so you’re right and you do need this manual check.
I don’t know if there’s a better way to avoid doing this manually. Perhaps @ajtowns can suggest something? At the least, I think you should add a comment like:
0 // We haven't set m_network yet (that happens in SelectParms()), so manually check
1 // for network.includeconf args.
You can also remove the block/braces.
includeconf_net out of scope as it was no longer used, and this makes that obvious.
I think this is subtly “wrong” fwiw: if you have [main] includeconf=foo.conf in bitcoin.conf, and then set testnet=1 in foo.conf, you’ll have foo.conf included, and be running testnet not mainnet, so you “shouldn’t” have included foo.conf (and any other settings from the [main] section won’t have been used). That might be too subtle to worry about though.
It’s perhaps less subtly wrong in that if you said includeconf=force-testnet.conf [testnet] includeconf=testnet-params.conf, with force-testnet.conf setting testnet=1, you would not end up including testnet-params.conf, whereas if you set -testnet on the command line, you would.
The easiest approach might be to not allow [regtest] includeconf=rt.conf at all, but that would probably be annoying (particularly with custom chains).
A thought I had was that maybe [regtest] includeconf=rt.conf should treat all the options in rt.conf as being network-specific, so if rt.conf says connect=10.10.10.10 [main] connect=10.10.10.20 [regtest] connect=10.10.10.30, that would get interpreted as if it was [regtest] connect=10.10.10.10 connect=10.10.10.30. That would avoid the inconsistency above, in that it would turn [main] includeconf=foo.conf into essentially [main] testnet=1 which would just be ignored.
I think it might be better to do it as four phases though: (1) read “-conf”, (2) read the includeconfs that aren’t under a section, (3) work out the chain based on everything noted so far, and (4) read the “-chain.includeconf” options from -conf (but not letting that change the chain).
I think that these are good observations, but that we don’t necessarily need to police such pathological edge-cases. The special casing for [regtest] testnet=1 etc in https://github.com/bitcoin/bitcoin/pull/11862/commits/005ad266491f43d7a9bfd959396037416cb32a55 was useful because it’s easy enough to have a config file like:
0option1=0
1option2=1
2[regtest]
3option3=1
and then append testnet=1 to the end, without realising that you’re actually adding regtest.testnet=1. (in fact, this PR does something similar here: https://github.com/bitcoin/bitcoin/pull/10267/files#diff-a3677de1e28a0e9b37d66c11cc9a760bR31 by appending an argument that becomes a network-specific argument). This kind of config error is easy to make, ambiguous, and ought to be policed.
The config that you’re describing here:
if you said includeconf=force-testnet.conf [testnet] includeconf=testnet-params.conf, with force-testnet.conf setting testnet=1, you would not end up including testnet-params.conf, whereas if you set -testnet on the command line, you would.
seems much more contorted. I don’t think a user should reasonably expect that setting the network in an includeconf file should change how the network-specific includeconfs in the base config file are interpreted. That behaviour also feels a bit too multi-leveled in that we’re reading a specific includeconf file based on the content of a different includeconf file (I argued against such behaviour here: #10267 (comment) because I think it leads to an overly complex model).
I added braces to throw includeconf_net out of scope
I don’t know of any other places in the codebase that add blocks to throw local variables out of scope. Obviously functionally the same, but I’d personally have a preference to not add code blocks like this, since it makes the code slightly less clear.
Looks great. Thanks for sticking with this.
A couple of nits inline.
95@@ -96,6 +96,13 @@ Low-level RPC changes
96 with any `-wallet=<path>` options, there is no change in behavior, and the
97 name of any wallet is just its `<path>` string.
98
99+Changed command-line options
100+-----------------------------
doc/release-notes.md? Never seen that way of adding release notes before.
357@@ -358,7 +358,7 @@ std::string HelpMessage(HelpMessageMode mode)
358 if (showDebug) {
359 strUsage += HelpMessageOpt("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize));
360 }
361- strUsage += HelpMessageOpt("-includeconf=<file>", _("Specify additional configuration file, relative to the -datadir path"));
362+ strUsage += HelpMessageOpt("-includeconf=<file>", _("Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)"));
-includepath commit
-includeconf commit? I tend to separate doc and code commits, usually.
950 LOCK(cs_args);
951 m_config_args.clear();
952+
953+ // we do not allow -includeconf from command line, so we clear it here
954+ auto it = m_override_args.find("-includeconf");
955+ if (it != m_override_args.end()) m_override_args.erase(it);
ParseParameters fwiw.
ParseParameters.
fprintf(stderr, ..) though, cause it hasn’t processed -printtoconsole or anything yet.
45+ self.restart_node(0, extra_args=["-includeconf=relative2.conf"])
46+
47+ subversion = self.nodes[0].getnetworkinfo()["subversion"]
48+ assert subversion.endswith("main; relative)/")
49+
50+ self.log.info("-includeconf cannot be used recursively. subversion should end with 'main)/'")
uacomment=relative\n to relative.conf and checking that subversion ends with main; relative).
I think this is subtly “wrong” fwiw: if you have [main] includeconf=foo.conf in bitcoin.conf, and then set testnet=1 in foo.conf, you’ll have foo.conf included, and be running testnet not mainnet, so you “shouldn’t” have included foo.conf (and any other settings from the [main] section won’t have been used). That might be too subtle to worry about though.
I don’t think we need to care about that. If the user explicitly includes foo.conf from [main] and foo.conf sets to testnet, arguably they would want to use testnet “whenever main is used”. Maybe they want to guarantee never mainnet?
A thought I had was that maybe [regtest] includeconf=rt.conf should treat all the options in rt.conf as being network-specific, so if rt.conf says connect=10.10.10.10 [main] connect=10.10.10.20 [regtest] connect=10.10.10.30, that would get interpreted as if it was [regtest] connect=10.10.10.10 connect=10.10.10.30. That would avoid the inconsistency above, in that it would turn [main] includeconf=foo.conf into essentially [main] testnet=1 which would just be ignored.
I’m not sure I follow here. includeconf has the exact same semantics as #include in C++. The current default behavior seems the most logical one.
I think it might be better to do it as four phases though: (1) read “-conf”, (2) read the includeconfs that aren’t under a section, (3) work out the chain based on everything noted so far, and (4) read the “-chain.includeconf” options from -conf (but not letting that change the chain).
I noted above a case where a user want an included conf to change the chain. Is it worth the effort to prevent this?
46+
47+ subversion = self.nodes[0].getnetworkinfo()["subversion"]
48+ assert subversion.endswith("main; relative)/")
49+
50+ self.log.info("-includeconf cannot be used recursively. subversion should end with 'main)/'")
51+ with open(os.path.join(self.options.tmpdir + "/node0", "relative.conf"), "w", encoding="utf8") as f:
May be clearer to append to the config file, rather than rewrite the uacomment param:
0 with open(os.path.join(self.options.tmpdir + "/node0", "relative.conf"), "a", encoding="utf8") as f:
1 f.write("includeconf=relative2.conf\n")
although you may disagree, in which case it’s fine to leave as is!
You should certainly update the log above though:
0self.log.info("-includeconf cannot be used recursively. subversion should end with 'main; relative)/'")
ACK f083a0e57ca7c991af56425e44cb5aeeaa383a50 with a couple of comments.
I agree with @kallewoof that the include precedence in the current implementation is fine. Perhaps more detailed documentation would satisfy @ajtowns ?
681@@ -682,6 +682,15 @@ void ArgsManager::ParseParameters(int argc, const char* const argv[])
682 m_override_args[key].push_back(val);
683 }
684 }
685+
686+ // we do not allow -includeconf from command line, so we clear it here
687+ auto it = m_override_args.find("-includeconf");
-includeconf is removed from m_override_args. Also update comment above accordingly.
std::map<std::string, std::vector<std::string>>, which may only have one entry per key. It removes the vector itself, which may contain multiple values. (I am only showing the first for simplicity in the error output.)
979+ LogPrintf("Attempting to include configuration file %s\n", to_include.c_str());
980+ fs::ifstream include_config(GetConfigFile(to_include));
981+ if (include_config.good()) {
982+ ReadConfigStream(include_config);
983+ } else {
984+ LogPrintf("Failed to include configuration file %s\n", to_include.c_str());
Skipping invalid configuration file ...?
Addressing the nit below (log success), I believe the current form looks better:
0 LogPrintf("Included configuration file %s\n", to_include.c_str());
1 } else {
2 LogPrintf("Failed to include configuration file %s\n", to_include.c_str());
974+ includeconf.insert(includeconf.end(), includeconf_net.begin(), includeconf_net.end());
975+ }
976+
977+ if (includeconf.size() > 0) {
978+ for (const std::string& to_include : includeconf) {
979+ LogPrintf("Attempting to include configuration file %s\n", to_include.c_str());
357@@ -358,6 +358,7 @@ std::string HelpMessage(HelpMessageMode mode)
358 if (showDebug) {
359 strUsage += HelpMessageOpt("-dbbatchsize", strprintf("Maximum database write batch size in bytes (default: %u)", nDefaultDbBatchSize));
360 }
361+ strUsage += HelpMessageOpt("-includeconf=<file>", _("Specify additional configuration file, relative to the -datadir path (only useable from configuration file, not command line)"));
0@@ -0,0 +1,6 @@
1+Changed command-line options
2+-----------------------------
-.
972+ // for network.includeconf args.
973+ std::vector<std::string> includeconf_net(GetArgs(std::string("-") + GetChainName() + ".includeconf"));
974+ includeconf.insert(includeconf.end(), includeconf_net.begin(), includeconf_net.end());
975+ }
976+
977+ if (includeconf.size() > 0) {
-noincludeconf, I believe it will exist, but the vector will be empty. See line 682, which adds the key to the map, but clears it (the vector, not the map).
m_override_args.
-noincludeconf, I did some tweaks so that you can do -noincludeconf from command line to disable includeconf stuff inside the main config file.
979+
980+ for (const std::string& to_include : includeconf) {
981+ fs::ifstream include_config(GetConfigFile(to_include));
982+ if (include_config.good()) {
983+ ReadConfigStream(include_config);
984+ LogPrintf("Included configuration file %s\n", to_include.c_str());
printf?
981+ fs::ifstream include_config(GetConfigFile(to_include));
982+ if (include_config.good()) {
983+ ReadConfigStream(include_config);
984+ LogPrintf("Included configuration file %s\n", to_include.c_str());
985+ } else {
986+ LogPrintf("Failed to include configuration file %s\n", to_include.c_str());
fprintf(stderr?
printtoconsole=0 is specified in the main conf, then printtoconsole=1 in included conf are ignored. But arguments with multiple values, like `-wallet, are concatenated.
979+
980+ for (const std::string& to_include : includeconf) {
981+ fs::ifstream include_config(GetConfigFile(to_include));
982+ if (include_config.good()) {
983+ ReadConfigStream(include_config);
984+ printf("Included configuration file %s\n", to_include.c_str());
I don’t understand why this is a print statement. It’ll get printed every time bitcoind or bitcoin-qt is started, and each time bitcoin-cli is called.
I think just a normal log here is fine.
-printtoconsole or -debug etc. have not been activated yet. A log would effectively be a NOP.
966 fs::ifstream stream(GetConfigFile(confPath));
967
968 // ok to not have a config file
969 if (stream.good()) {
970 ReadConfigStream(stream);
971+ if (m_override_args.count("-includeconf") == 0) {
681@@ -682,6 +682,17 @@ void ArgsManager::ParseParameters(int argc, const char* const argv[])
682 m_override_args[key].push_back(val);
683 }
684 }
685+
686+ // we do not allow -includeconf from command line, so we clear it here
687+ auto it = m_override_args.find("-includeconf");
688+ if (it != m_override_args.end()) {
689+ if (it->second.size() > 0) {
It’d be nice if we could be more aggressive here and fail to start if a -includeconf argument is provided on the command line (since continuing to startup would be continuing with config that the user has specified that they don’t want).
ParseParameters() doesn’t currently have a way to return errors, and callers don’t wrap calls with try-except (unlike GetChainName() for example). Perhaps a future PR could change the function to return a bool, so we could indicate an error to the caller and abort startup.
21+class IncludeConfTest(BitcoinTestFramework):
22+ def setup_chain(self):
23+ super().setup_chain()
24+ # Create additional config files
25+ # - tmpdir/node0/relative.conf
26+ with open(os.path.join(self.options.tmpdir + "/node0", "relative.conf"), "w", encoding="utf8") as f:
os.path.join(self.options.tmpdir, "node0", "relative.conf") for better portability.
Looks great. Just a few more nits!
Thanks for the release notes
449+ // we do not allow -includeconf from command line, so we clear it here
450+ auto it = m_override_args.find("-includeconf");
451+ if (it != m_override_args.end()) {
452+ if (it->second.size() > 0) {
453+ for (const auto& ic : it->second) {
454+ printf("warning: -includeconf cannot be used from commandline; ignoring -includeconf=%s\n", ic.c_str());
fprintf(stderr, ...?
test_runner.py, not travis. Digging more.
I’d prefer to not to have special stderr checking for this test. I have a PR open here: #12755 which adds stderr checking to the test framework.
I’ve also prepared a branch here: https://github.com/bitcoin/bitcoin/compare/master...jnewbery:pr10267.3?expand=1 which rebases this PR on #12755 and adds explicit stderr checking.
Would you mind either:
15+ file.
16+"""
17+import os
18+
19+from test_framework.test_framework import BitcoinTestFramework, assert_equal
20+import tempfile
18+
19+from test_framework.test_framework import BitcoinTestFramework, assert_equal
20+import tempfile
21+
22+class IncludeConfTest(BitcoinTestFramework):
23+ def setup_chain(self):
set_test_params() as the first method. I think that makes more logical sense since that method is called by __init__(). Do you mind doing the same here?
Tested ACK 6d5605517b477538f0447d41fe0bc734da27d10c. A couple of nits in the new test code, but nothing that should prevent merge.
@jnewbery Wouldn’t it be simpler to fix this in a follow-up PR once #12755 is merged?
Not 100% sure what you mean by ‘fix this’, but I’m guessing you mean merge this as is and then remove the specific stderr checking code from this test when the generic stderr checking is added in #12755? Sure - that’s fine, as long as you don’t mind reviewing #12755 for me :)
Tested ACK 25b7ab9c02f691be6c1aa71a9cf51ac1a6ea9db4.
I’m a little confused. Should I close this PR in favor of #12755? I thought #12755 only added the error stuff, but it includes this PR as well now.
Sorry - I haven’t been very clear in my updates. Now that this PR includes stderr testing, it conflicts with #12755. My initial suggestions were to remove the stderr testing from this test, or rebase on top of #12755. But now I think it would make more sense to rebase #12755 on this, since this PR has been around for so long and should almost be ready for merge.
I’ve rebased #12755 on this. As soon as this PR is merged, #12755 will be ready to go in.
utACK 25b7ab9.
I suggest to add a warning if -includeconf is used in an included file (follow up PR).
When working on the locking annotations (see #13126) I noticed that ArgsManager::ReadConfigFiles() introduced in this PR accesses m_override_args without first locking cs_args.
The PR #13126 adds the correct locking. FWIW Travis will catch this type of cs_args locking violations when #13126 is merged :-)
49+
50+ subversion = self.nodes[0].getnetworkinfo()["subversion"]
51+ assert subversion.endswith("main; relative)/")
52+ log_stderr.seek(0)
53+ stderr = log_stderr.read().decode('utf-8').strip()
54+ assert_equal(stderr, 'warning: -includeconf cannot be used from commandline; ignoring -includeconf=relative2.conf')
746+ fs::ifstream include_config(GetConfigFile(to_include));
747+ if (include_config.good()) {
748+ ReadConfigStream(include_config);
749+ LogPrintf("Included configuration file %s\n", to_include.c_str());
750+ } else {
751+ fprintf(stderr, "Failed to include configuration file %s\n", to_include.c_str());
Post-merge ACK 25b7ab9
See #13367 to fix my nit about the errors.