#15934 Merge settings one place instead of five places (config)

https://github.com/bitcoin/bitcoin/15934

Host: jnewbery

Notes

  • Bitcoin Core takes config from several places:
    1. A bitcoin.conf file in the data directory
    2. Command line arguments (when Bitcoin Core is started from the command line)
    3. 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 -connect config to connect to specific peers, then by default -listen will be switched off, so the node won’t accept incoming connections (code). See ForceSetArg(), SoftSetArg() and SoftSetBoolArg() for other places where configuration is updated in the code.
  • Since PR 11862, the bitcoin.conf file 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 bitcoin.conf configuration.
  • 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:

Questions

  1. Did you review the PR? Concept ACK, approach ACK, tested ACK, or NACK? (Don’t forget to put your PR review on GitHub.)

  2. What steps did you take, beyond reading the code?

  3. How easy did you find it to review the changes to util/system and util/logging? How about the test changes?

  4. 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?

  5. 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?

Meeting Log

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