Merge settings one place instead of five places (config)
- Bitcoin Core takes config from several places:
bitcoin.conffile in the data directory
- Command line arguments (when Bitcoin Core is started from the command line)
- QT settings (only for bitcoin-qt, not bitcoind or the utilities)
- Settings are also sometimes updated within the code. Usually this happens
if some user-provided config implies other config. For example, if the
user starts Bitcoin Core with
-connectconfig to connect to specific peers, then by default
-listenwill be switched off, so the node won’t accept incoming connections (code). See
SoftSetBoolArg()for other places where configuration is updated in the code.
- Since PR 11862, the
bitcoin.conffile has allowed network-specific sections. Different config can be specified for mainnet, testnet and regtest in the same config file by using section headings.
- QT settings are used for GUI-only persistent configuration. For example, the window location and whether the application is minimized to the tray is persisted between sessions in the QT settings. These settings are saved in the windows registry or platform specific config files.
- QT settings also store application configuration that can be updated in the GUI, such as whether to prune or to use tor. This config is saved but is only applied when running bitcoin-qt, not when running bitcoind.
- Generally, QT settings override command-line arguments, which override
- There are a lot of quirks in the way that configuration is parsed and merged (eg command-line argument precedence is treated differently from config file precedence, some command-line arguments are ignored, etc). ryanofsky lists these quirks in the PR.
- PR 15869 added test coverage for settings merging to prevent new code from introducing regressions.
- Several PRs are built on top of this:
Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? (Don’t forget to put your PR review on GitHub.)
What steps did you take, beyond reading the code?
How easy did you find it to review the changes to
util/logging? How about the test changes?
What is the univalue library, and how is it used in Bitcoin Core? How is it used in this PR? What are your thoughts about that?
15935 adds a persistent read/write config file to the data directory. Have there been any other attempts to do this? Which do you prefer?
13:00 < emilengler> Hello 13:00 < jnewbery> hi 13:00 < ajonas> Hi 13:00 < amiti> hi 13:00 < michaelfolkson> Hey 13:00 < fjahr> hi 13:01 < jnewbery> Notes and questions: https://bitcoincore.reviews/15934.html 13:01 < ariard> hi 13:01 < lightlike> hello 13:01 < jnewbery> what did everyone think of this week's PR? 13:01 < emzy> Hi 13:01 < jnewbery> (apologies for only posting on Monday. I'll try to post before the weekend for next week) 13:02 < emilengler> jnewbery: I like the idea but I am still skeptical about backwards compatibility 13:02 < emzy> same here. 13:02 < fjahr> good cleanup but also a lot to unpack. I really liked the regression test. 13:02 -!- Irssi: #bitcoin-core-pr-reviews: Total of 75 nicks [0 ops, 0 halfops, 0 voices, 75 normal] 13:02 < emilengler> IMO the user should not really notice anything about this change 13:03 < ariard> I've reviewed it, a lot of back and forth between old and new code to be sure current behaviors are maintained 13:03 < emilengler> Ergo, he should net need to move config files after update 13:03 < kanzure> hi 13:04 < ariard> maybe cd49914 could have been split a bit to ease rw 13:04 < jnewbery> emilengler: right, the author claims that there's no bahaviour change in this PR. I found it quite difficult to verify that 13:04 < lightlike> it looks like a nice cleanup, haven't reviewed every line of code yet, but did some testing. 13:04 < fjahr> emilengler: but that's the point of backwards compatibility 13:04 < fjahr> why do you think they have to move sth? 13:04 < emilengler> jnewbery: I also didn't saw something like a backwards option which moves old config files 13:04 < jnewbery> ariard: that's the middle commit (_Deduplicate settings merge code_)? I did wonder if it would be possible to break it up 13:05 < jonatack> hi 13:05 < emilengler> But in general I like the concept about storing everything in one place 13:05 < ariard> jnewbery: yes this one, cd59914 13:05 < jnewbery> emilengler: there's no moving of files in the PR 13:05 < ajonas> fjahr: that test was so great 13:06 < emilengler> jnewbery: How is backwards compatibility guaranteed then? 13:07 < fjahr> emilengler: are you maybe talking about 15935? 13:07 < jnewbery> emilengler: the author included regression tests to test for compatibility 13:08 < jnewbery> ariard: I see that you've ACKed the PR. Perhaps you can talk about how you went about testing/reviewing? 13:08 < ryanofsky> hey all, just catching up 13:08 < jnewbery> I found that middle commit really hard to review. Looking at the diff wasn't really helpful for me because it was basically a rewrite. 13:08 < jnewbery> hi ryanofsky. Thanks for joining! 13:08 < ryanofsky> backwards compatibility should be guaranteed by the test added previously: https://github.com/bitcoin/bitcoin/pull/15869/files 13:08 < emilengler> ryanofsky: Good to have you here 13:09 < ryanofsky> as long as the hash in that test isn't changing, none of the merge behaviors can change either 13:09 < fjahr> jnewbery: dito on that, I think it could be possible to split it but now it is probably not worth the effort anymore 13:10 < fjahr> I did even cherry-pick the test and ran it against master 13:10 < jnewbery> jonatack / fjahr: you also left review comments (thanks!) did you have any tips on reviewing or testing? 13:12 < jnewbery> Were there any particular parts of the code that anyone found difficult/interesting? 13:13 < ariard> jnewbery: so I went first reviewing new settings.cpp/settings.h files 13:13 < fjahr> Not sure I was really efficient, I just re-read the code many times until I felt I had a good understanding. I felt the tests were really helpful in getting confidence in the code. As I said I cherry-picked the regression test to verify since it was last in the commit history. 13:14 < ariard> then reviewing every old settings merge code like GetArg,GetNetBoolArg and compare against their new versions to be sure effect stay the same 13:15 < jonatack> The test was fun to tweak and did a bit of manual testing. The most time-consuming was reading up on the context and the many idiosyncracies of the current behavior, which ryanofsky already kindly 13:15 < ajonas> I had trouble recreating what JO'Beirne did here https://github.com/bitcoin/bitcoin/pull/15934#pullrequestreview-242727281 13:15 < ariard> and read from ArgsManager to see how ParseParameters and ReadConfigFiles interact, both in master and on merge-settings branch 13:15 < jonatack> explained to me during review of another PR related to this. 13:15 < jnewbery> ariard: yeah, I think that's really the only safe way to do it. Time-consuming but the only way to be sure it is indeed a non-behaviour change. 13:17 < jonatack> I like that a unit test was added instead functional ones. We sometimes see cases where functional tests are written out of ease where unit tests could have been added instead. 13:17 < michaelfolkson> So <ryanofsky> for that test in #15869 you literally sketched out all the combinations of changing settings in the config and QT and in different orders? And the test checks the resulting behavior is the same pre and post PR? 13:17 < jnewbery> joanatack: I definitely agree with that! 13:17 < michaelfolkson> Some things like having listening switched off if connecting to specific peers I didn't understand the logic behind 13:17 < ryanofsky> michaelfolkson, the test covers all the inputs to argsmanager and all the outputs so there's nothing qt specific 13:18 < jnewbery> Can anyone explain to me what's going on here: https://github.com/bitcoin/bitcoin/pull/15934/commits/4a5e736dc4a22643b4f09181b3d7245727cee876#diff-b372feef646fe8b25a4ad50c22e64b19R74 13:18 < jnewbery> template <typename Map, typename Key> 13:18 < jnewbery> auto FindKey(Map&& map, Key&& key) -> decltype(&map.at(key)) 13:19 < ryanofsky> "auto Function() -> ReturnType" is a weird way to write "ReturnType Function()" which you are forced to do when the return type depends on the function arguments 13:21 < jnewbery> thanks ryanofsky. Did anyone figure out the && in the function arguments? 13:22 < jonatack> jnewbery: for that i refer to a great comment written once by ryanofsky on when to use double && 13:22 < jnewbery> jonatack: oh yeah? Where's the great comment? 13:23 < jnewbery> So, && in a function argument means the argument is an rvalue reference 13:24 < jonatack> trying to find the original. I copied it into my notes a few months back here https://github.com/jonatack/bitcoin-development/blob/master/notes.txt#L264 13:24 < jnewbery> I found these articles quite useful in explaining rvalue references: https://www.internalpointers.com/post/understanding-meaning-lvalues-and-rvalues-c https://www.internalpointers.com/post/c-rvalue-references-and-move-semantics-beginners 13:26 < jnewbery> if a templated function has rvalue references in its arguments, then we get forwarding references: if an lvalue is provided, then the reference is an lvalue, and if an rvalue is provided, the reference is an rvalue 13:26 < jnewbery> ryanofsky: I think that's right. Correct me if I'm mistaken 13:27 < jnewbery> jonatack: here's the original: https://github.com/bitcoin/bitcoin/pull/15849#pullrequestreview-231748721 13:28 < jonatack> jnewbery: ryanofsky also refers to it in this recent comment in reviewing PR 16202: https://github.com/bitcoin/bitcoin/pull/16202#discussion_r336612138 13:28 < ryanofsky> That's right I think. There are special rules for templates combined with && arguments. But if you want a template argument to accept any argument const or nonconst lvalue or rvalue you can use && to do that 13:29 < jonatack> jnewbery: that's the one! Ty 13:29 < jnewbery> there's a bit more about it here: https://stackoverflow.com/questions/3582001/advantages-of-using-forward/3582313#3582313 13:29 < jnewbery> and here: https://www.justsoftwaresolutions.co.uk/cplusplus/rvalue_references_and_perfect_forwarding.html 13:30 < jnewbery> ok, I have another question that wasn't the notes. Can anyone explain what the ArgsManagerHelper is and why it exists? 13:30 < jnewbery> https://github.com/bitcoin/bitcoin/blob/8f14d2002b114195fccfe8479a70e323c5f3aa09/src/util/system.cpp#L165 13:33 < jnewbery> (you should all feel free to ask any questions at any point. Don't feel like you can't talk because I've asked a question) 13:34 < jnewbery> the ArgsManagerHelper was introduced in PR 11862. ajtowns explains why he added it here: https://github.com/bitcoin/bitcoin/pull/11862#discussion_r173870835 13:34 < emilengler> Got to go now, thank you already 13:35 < michaelfolkson> Sorry, it takes some time to go through the links posted :) 13:36 < jnewbery> I think that now ArgsManagerHelper has been reduced to just two functions, we should just get rid of it entirely and move those to be private functions of ArgsManager 13:36 < lightlike> jnewbery: it is for helper functions that need Access to ArgsManager's internal. But why it is not just part of ArgsManager. 13:37 < michaelfolkson> For security reasons? 13:37 < jnewbery> lightlike: I asked aj that question in the original PR. His response "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." (https://github.com/bitcoin/bitcoin/pull/11862#discussion_r173870835) 13:37 < fjahr> AJ says to save compilation time, which I would have never thought of on my own 13:37 < jonatack> For friendly convenience 13:38 < jnewbery> fjahr: s/compilation time/recompilation time/ 13:38 < jonatack> perhaps an unneeded abstraction now 13:38 < jnewbery> next question: What is the univalue library, and how is it used in Bitcoin Core? How is it used in this PR? What are your thoughts about that? 13:39 < fjahr> I would say there is at least a comment missing if we have to look into old PR discussions to make sense of the existence of a class ;) 13:39 < jnewbery> (this _was_ in the notes, so you'll have had a bit more time to prepare for it) 13:39 < jnewbery> fjahr: that's a fair point 13:39 < jonatack> https://github.com/bitcoin-core/univalue 13:39 < ajonas> univalue lib is a universal type that encapsulates a JSON value. Used for communication with external utilities through the RPC interface (exclusively until #15935?) 13:40 < jnewbery> ajonas: exactly correct! 13:40 < jonatack> A fork of the original for stability. 13:41 < jnewbery> I like the review comment here: https://github.com/bitcoin/bitcoin/pull/15934/commits/4a5e736dc4a22643b4f09181b3d7245727cee876#r337691812 where russ lists the methods expected in the interface 13:42 < jnewbery> (the code comment at https://github.com/bitcoin/bitcoin/pull/15934/commits/4a5e736dc4a22643b4f09181b3d7245727cee876 says that univalue could be replaced but doesn't give the interface requirements) 13:43 < michaelfolkson> Are there downsides to using the univalue library? 13:44 < jnewbery> michaelfolkson: not that I can think of 13:44 < jnewbery> the advantage is that it makes https://github.com/bitcoin/bitcoin/pull/15935 a lot easier 13:45 < jnewbery> on that point, let's go to the last question. 15935 adds a persistent read/write config file to the data directory. Have there been any other attempts to do this? Which do you prefer? 13:46 < jonatack> There was luke-jr's https://github.com/bitcoin/bitcoin/pull/11082 13:47 < jonatack> I haven't reviewed to have a preference yet. 13:48 < jnewbery> jonatack: yes, that's the only other one that I'm aware of 13:48 < jnewbery> people have been talking about writeable config for years though 13:49 < jonatack> One possible criticism of univalue ("more code") from luke-jr here: https://github.com/bitcoin/bitcoin/pull/15935#issuecomment-510023127 13:50 < jnewbery> what do other people think about that? 13:52 < fjahr> I did not have time to through either PRs but I feel people regularly stumble over how/where args are defined. So a single place makes sense to me. There was also this PR recently where the configs should be printed out in the logs. It shows that it's a pain for people. 13:52 < michaelfolkson> I don't like the idea of multiple of .conf files if I'm understood it correctly. As a user I just want to see the latest settings, I don't care the path those settings followed to the latest settings 13:53 < jonatack> Reasonable viewpoints on both sides AFAICT, these are typical legacy codebase and backward compat issues. 13:53 < jnewbery> Talkless: I see you're reviewing the PR right now (thanks!) Are you able to use github's 'review' feature rather than leave individual comments to spare people's notifications a bit? :) 13:53 < Talkless> jnewbery: I don't believe I am able to "actually" review, just passing by 13:53 < Talkless> oh, notifications.. haven't thought about that 13:54 < Talkless> sorry 13:54 < jonatack> Talkless: click on "Files changed" at the top and you should see a green "Review changes" button on the upper right. 13:55 < jnewbery> michaelfolkson: I think it's unavoidable that there should be a different files for read-only config and read-write config. I think ryanofsky's comment here is a good summary: https://github.com/bitcoin/bitcoin/pull/15935#issuecomment-515534538 13:55 < jnewbery> Talkless: no problem. Thanks for reviewing! 13:55 < michaelfolkson> <fjahr> Yeah I was thinking about that config in logs PR as well a lot during this. 13:55 < jnewbery> 5 minutes left. Any final questions? 13:56 < michaelfolkson> Why is it assumed I don't want to be listening node if I connect to specific peers? 13:56 < jnewbery> michaelfolkson: because if you specify `-connect=`, it's assumed that you _only_ want to connect to that peer 13:57 < jnewbery> you can connect to a specified peer and listen by also specifying `-listen=1` 13:57 < jonatack> ryanofsky: I agree with your last paragraph in https://github.com/bitcoin/bitcoin/pull/15935#issuecomment-515534538 on advanced users versus typical users. 13:57 < fjahr> How are chances we can remove these backward compatiblities? Has this been discussed? 13:57 < michaelfolkson> <jnewbery> Ah ok. Personally I would want both but I'm sample size of 1 13:58 < ariard> I found the point made here interesting on how config files should be thought according to whom is going to use them https://github.com/bitcoin/bitcoin/pull/15935#issuecomment-515534538 13:58 < jnewbery> michaelfolkson: then specify `-connect=<peer address> -listen` 13:58 < jonatack> ryanofsky: though I am unsure who typical users are and what are their capabilities. 13:58 < fjahr> How is the general mood towards a change that potentially breaks local node configs 13:58 < michaelfolkson> <jnewbery> Yup got it ;) Just querying the default behavior 13:59 < jnewbery> fjahr: we try not to, but if it's a clear win then we might do it 13:59 < ryanofsky> fjahr, I think a lot of the strange behaviors can be probably be cleaned up. I think they just need to be considered individually 14:00 < jnewbery> DING! 14:00 < jnewbery> that's time 14:00 < jnewbery> thanks everyone :) 14:00 < sebastianvstaa> thanks 14:00 < fjahr> Thanks jnewbery! 14:00 < michaelfolkson> Thanks <jnewbery> and everyone! 14:00 < jnewbery> any specific requests for next week? 14:01 < jonatack> Thanks jnewbery, ryanofsky, and everyone! 14:01 < lightlike> thanks! 14:01 < ryanofsky> i'd request #15931 wallet depth in chain pr (rereviewing that now) 14:01 < jnewbery> yes, thanks ryanofsky! 14:02 < jnewbery> ryanofsky: you missed it: https://bitcoincore.reviews/15931.html :) 14:02 < ryanofsky> wow, ok! 14:03 < ajonas> thanks jnewbery