Add new bitcoin_rw.conf file that is used for settings modified by this software itself#11082
Add new bitcoin_rw.conf file that is used for settings modified by this software itself#11082luke-jr wants to merge 4 commits intobitcoin:masterfrom
Conversation
|
I'm not a big fan of multiple config files. I would prefer if QT just edited |
|
I tried just making Can you rebase this? From my experience with the other PR that should be easy and it worked quite well. |
|
Rebased |
|
tACK aac0501 (tested through #12833) |
|
Needs another rebase due to #11862. Perhaps not worth the effort without more Concept ACKs. |
ajtowns
left a comment
ajtowns
left a comment
There was a problem hiding this comment.
ConceptACK. This approach isn't any worse than QSettings, and seems more consistent, and discoverable when anything weird is happening due to conflicting/unexpected settings.
src/bitcoind.cpp
Outdated
There was a problem hiding this comment.
Wouldn't it be better for ReadRWConfigFile not to throw exceptions in any normal case? If there's a weird setting, that's fine it will just get over-written later; if the file is read-only though that error should at least be reported to the user; and if there's some other unexpected sort of error that throws an exception, then that shouldn't be ignored?
|
Let me know when I can add this back to project 8, i.e. when it is ready for review. |
|
Finally rebased (and ready for high-pri for review I think). Edit: Forgot #14532 was in high-pri still. This will have to wait I guess. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
Sjors
left a comment
Sjors
left a comment
There was a problem hiding this comment.
Thanks for the rebase! Works like a charm on my freshly rebased #12833.
Does bitcoin-cli really also need to parse this? I have a light preference for disallowing settings in -confrw that change how bitcoin-cli should behave, i.e.:
datadir: this always need to be in QTSettings anyway- RPC connection info (if someone really needs a custom host/port/binding and non-cookie authentication, they know how to edit
bitcoin.conf) - mainnet / testnet / regtest (the GUI can just have a toggle for this,
bitcoindusers know how to edit the config file, and/or can just use-testnet)
Reason for this preference is that it makes the life easier for other tools to parse the settings. E.g. wallet_tool #13926 might in the future want to parse bitcoin.conf, and external projects like c-lightning already do/did this.
|
Rebased |
|
I'm not sure this is needed now that we have the settings.json file. @luke-jr, can you explain why we should consider adding another settings file? |
|
So we can revert settings.json before 0.21 is released and we're locked in to supporting such a bad idea? |
|
NACK. Let's close this and move on. |
…is software itself
|
I have no objection to swapping the new The (rather large amount of) code in |
|
Now that we've branched off, and are unlikely to be reverting |
16 participants
This is part of #7510, without the new GUI settings (ie, just the minimal framework for the RW conf file).