Merge settings one place instead of five places (config)

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

Host: jnewbery  -  PR author: ryanofsky

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

  113:00 <emilengler> Hello
  213:00 <jnewbery> hi
  313:00 <ajonas> Hi
  413:00 <amiti> hi
  513:00 <michaelfolkson> Hey
  613:00 <fjahr> hi
  713:01 <jnewbery> Notes and questions: https://bitcoincore.reviews/15934.html
  813:01 <ariard> hi
  913:01 <lightlike> hello
 1013:01 <jnewbery> what did everyone think of this week's PR?
 1113:01 <emzy> Hi
 1213:01 <jnewbery> (apologies for only posting on Monday. I'll try to post before the weekend for next week)
 1313:02 <emilengler> jnewbery: I like the idea but I am still skeptical about backwards compatibility
 1413:02 <emzy> same here.
 1513:02 <fjahr> good cleanup but also a lot to unpack. I really liked the regression test.
 1613:02 -!- Irssi: #bitcoin-core-pr-reviews: Total of 75 nicks [0 ops, 0 halfops, 0 voices, 75 normal]
 1713:02 <emilengler> IMO the user should not really notice anything about this change
 1813:03 <ariard> I've reviewed it, a lot of back and forth between old and new code to be sure current behaviors are maintained
 1913:03 <emilengler> Ergo, he should net need to move config files after update
 2013:03 <kanzure> hi
 2113:04 <ariard> maybe cd49914 could have been split a bit to ease rw
 2213:04 <jnewbery> emilengler: right, the author claims that there's no bahaviour change in this PR. I found it quite difficult to verify that
 2313:04 <lightlike> it looks like a nice cleanup, haven't reviewed every line of code yet, but did some testing.
 2413:04 <fjahr> emilengler: but that's the point of backwards compatibility
 2513:04 <fjahr> why do you think they have to move sth?
 2613:04 <emilengler> jnewbery: I also didn't saw something like a backwards option which moves old config files
 2713:04 <jnewbery> ariard: that's the middle commit (_Deduplicate settings merge code_)? I did wonder if it would be possible to break it up
 2813:05 <jonatack> hi
 2913:05 <emilengler> But in general I like the concept about storing everything in one place
 3013:05 <ariard> jnewbery: yes this one, cd59914
 3113:05 <jnewbery> emilengler: there's no moving of files in the PR
 3213:05 <ajonas> fjahr: that test was so great
 3313:06 <emilengler> jnewbery: How is backwards compatibility guaranteed then?
 3413:07 <fjahr> emilengler: are you maybe talking about 15935?
 3513:07 <jnewbery> emilengler: the author included regression tests to test for compatibility
 3613:08 <jnewbery> ariard: I see that you've ACKed the PR. Perhaps you can talk about how you went about testing/reviewing?
 3713:08 <ryanofsky> hey all, just catching up
 3813: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.
 3913:08 <jnewbery> hi ryanofsky. Thanks for joining!
 4013:08 <ryanofsky> backwards compatibility should be guaranteed by the test added previously: https://github.com/bitcoin/bitcoin/pull/15869/files
 4113:08 <emilengler> ryanofsky: Good to have you here
 4213:09 <ryanofsky> as long as the hash in that test isn't changing, none of the merge behaviors can change either
 4313: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
 4413:10 <fjahr> I did even cherry-pick the test and ran it against master
 4513:10 <jnewbery> jonatack / fjahr: you also left review comments (thanks!) did you have any tips on reviewing or testing?
 4613:12 <jnewbery> Were there any particular parts of the code that anyone found difficult/interesting?
 4713:13 <ariard> jnewbery: so I went first reviewing new settings.cpp/settings.h files
 4813: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.
 4913: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
 5013: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
 5113:15 <ajonas> I had trouble recreating what JO'Beirne did here https://github.com/bitcoin/bitcoin/pull/15934#pullrequestreview-242727281
 5213:15 <ariard> and read from ArgsManager to see how ParseParameters and ReadConfigFiles interact, both in master and on merge-settings branch
 5313:15 <jonatack> explained to me during review of another PR related to this.
 5413: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.
 5513: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.
 5613: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?
 5713:17 <jnewbery> joanatack: I definitely agree with that!
 5813:17 <michaelfolkson> Some things like having listening switched off if connecting to specific peers I didn't understand the logic behind
 5913:17 <ryanofsky> michaelfolkson, the test covers all the inputs to argsmanager and all the outputs so there's nothing qt specific
 6013:18 <jnewbery> Can anyone explain to me what's going on here: https://github.com/bitcoin/bitcoin/pull/15934/commits/4a5e736dc4a22643b4f09181b3d7245727cee876#diff-b372feef646fe8b25a4ad50c22e64b19R74
 6113:18 <jnewbery> template <typename Map, typename Key>
 6213:18 <jnewbery> auto FindKey(Map&& map, Key&& key) -> decltype(&map.at(key))
 6313: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
 6413:21 <jnewbery> thanks ryanofsky. Did anyone figure out the && in the function arguments?
 6513:22 <jonatack> jnewbery: for that i refer to a great comment written once by ryanofsky on when to use double &&
 6613:22 <jnewbery> jonatack: oh yeah? Where's the great comment?
 6713:23 <jnewbery> So, && in a function argument means the argument is an rvalue reference
 6813: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
 6913: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
 7013: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
 7113:26 <jnewbery> ryanofsky: I think that's right. Correct me if I'm mistaken
 7213:27 <jnewbery> jonatack: here's the original: https://github.com/bitcoin/bitcoin/pull/15849#pullrequestreview-231748721
 7313: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
 7413: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
 7513:29 <jonatack> jnewbery: that's the one! Ty
 7613:29 <jnewbery> there's a bit more about it here: https://stackoverflow.com/questions/3582001/advantages-of-using-forward/3582313#3582313
 7713:29 <jnewbery> and here: https://www.justsoftwaresolutions.co.uk/cplusplus/rvalue_references_and_perfect_forwarding.html
 7813:30 <jnewbery> ok, I have another question that wasn't the notes. Can anyone explain what the ArgsManagerHelper is and why it exists?
 7913:30 <jnewbery> https://github.com/bitcoin/bitcoin/blob/8f14d2002b114195fccfe8479a70e323c5f3aa09/src/util/system.cpp#L165
 8013: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)
 8113: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
 8213:34 <emilengler> Got to go now, thank you already
 8313:35 <michaelfolkson> Sorry, it takes some time to go through the links posted :)
 8413: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
 8513:36 <lightlike> jnewbery: it is for helper functions that need Access to ArgsManager's internal. But why it is not just part of ArgsManager.
 8613:37 <michaelfolkson> For security reasons?
 8713: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)
 8813:37 <fjahr> AJ says to save compilation time, which I would have never thought of on my own
 8913:37 <jonatack> For friendly convenience
 9013:38 <jnewbery> fjahr: s/compilation time/recompilation time/
 9113:38 <jonatack> perhaps an unneeded abstraction now
 9213: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?
 9313: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 ;)
 9413:39 <jnewbery> (this _was_ in the notes, so you'll have had a bit more time to prepare for it)
 9513:39 <jnewbery> fjahr: that's a fair point
 9613:39 <jonatack> https://github.com/bitcoin-core/univalue
 9713: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?)
 9813:40 <jnewbery> ajonas: exactly correct!
 9913:40 <jonatack> A fork of the original for stability.
10013: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
10113: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)
10213:43 <michaelfolkson> Are there downsides to using the univalue library?
10313:44 <jnewbery> michaelfolkson: not that I can think of
10413:44 <jnewbery> the advantage is that it makes https://github.com/bitcoin/bitcoin/pull/15935 a lot easier
10513: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?
10613:46 <jonatack> There was luke-jr's https://github.com/bitcoin/bitcoin/pull/11082
10713:47 <jonatack> I haven't reviewed to have a preference yet.
10813:48 <jnewbery> jonatack: yes, that's the only other one that I'm aware of
10913:48 <jnewbery> people have been talking about writeable config for years though
11013:49 <jonatack> One possible criticism of univalue ("more code") from luke-jr here: https://github.com/bitcoin/bitcoin/pull/15935#issuecomment-510023127
11113:50 <jnewbery> what do other people think about that?
11213: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.
11313: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
11413:53 <jonatack> Reasonable viewpoints on both sides AFAICT, these are typical legacy codebase and backward compat issues.
11513: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? :)
11613:53 <Talkless> jnewbery: I don't believe I am able to "actually" review, just passing by
11713:53 <Talkless> oh, notifications.. haven't thought about that
11813:54 <Talkless> sorry
11913:54 <jonatack> Talkless: click on "Files changed" at the top and you should see a green "Review changes" button on the upper right.
12013: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
12113:55 <jnewbery> Talkless: no problem. Thanks for reviewing!
12213:55 <michaelfolkson> <fjahr> Yeah I was thinking about that config in logs PR as well a lot during this.
12313:55 <jnewbery> 5 minutes left. Any final questions?
12413:56 <michaelfolkson> Why is it assumed I don't want to be listening node if I connect to specific peers?
12513:56 <jnewbery> michaelfolkson: because if you specify `-connect=`, it's assumed that you _only_ want to connect to that peer
12613:57 <jnewbery> you can connect to a specified peer and listen by also specifying `-listen=1`
12713: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.
12813:57 <fjahr> How are chances we can remove these backward compatiblities? Has this been discussed?
12913:57 <michaelfolkson> <jnewbery> Ah ok. Personally I would want both but I'm sample size of 1
13013: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
13113:58 <jnewbery> michaelfolkson: then specify `-connect=<peer address> -listen`
13213:58 <jonatack> ryanofsky: though I am unsure who typical users are and what are their capabilities.
13313:58 <fjahr> How is the general mood towards a change that potentially breaks local node configs
13413:58 <michaelfolkson> <jnewbery> Yup got it ;) Just querying the default behavior
13513:59 <jnewbery> fjahr: we try not to, but if it's a clear win then we might do it
13613: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
13714:00 <jnewbery> DING!
13814:00 <jnewbery> that's time
13914:00 <jnewbery> thanks everyone :)
14014:00 <sebastianvstaa> thanks
14114:00 <fjahr> Thanks jnewbery!
14214:00 <michaelfolkson> Thanks <jnewbery> and everyone!
14314:00 <jnewbery> any specific requests for next week?
14414:01 <jonatack> Thanks jnewbery, ryanofsky, and everyone!
14514:01 <lightlike> thanks!
14614:01 <ryanofsky> i'd request #15931 wallet depth in chain pr (rereviewing that now)
14714:01 <jnewbery> yes, thanks ryanofsky!
14814:02 <jnewbery> ryanofsky: you missed it: https://bitcoincore.reviews/15931.html :)
14914:02 <ryanofsky> wow, ok!
15014:03 <ajonas> thanks jnewbery