Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add new bitcoin_rw.conf file that is used for settings modified by this software itself #11082

Open
wants to merge 4 commits into
base: master
from

Conversation

@luke-jr
Copy link
Member

@luke-jr luke-jr commented Aug 17, 2017

This is part of #7510, without the new GUI settings (ie, just the minimal framework for the RW conf file).

@luke-jr luke-jr force-pushed the luke-jr:rwconf branch to f40a96c Aug 17, 2017
@Sjors
Copy link
Member

@Sjors Sjors commented Mar 17, 2018

I'm not a big fan of multiple config files. I would prefer if QT just edited bitcoin.conf and tells the user to do so manually if things gets too complicated. See also #6461.

@Sjors
Copy link
Member

@Sjors Sjors commented Mar 30, 2018

I tried just making bitcoin.conf writeable instead of having two files in #12833, but that seems to raise some objections. So in that case: Concept ACK.

Can you rebase this? From my experience with the other PR that should be easy and it worked quite well.

@luke-jr luke-jr force-pushed the luke-jr:rwconf branch from 59d78f9 Mar 31, 2018
@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Mar 31, 2018

Rebased

@Sjors
Copy link
Member

@Sjors Sjors commented Apr 3, 2018

tACK aac0501 (tested through #12833)

@Sjors
Copy link
Member

@Sjors Sjors commented May 15, 2018

Needs another rebase due to #11862. Perhaps not worth the effort without more Concept ACKs.

@laanwj laanwj added this to Blockers in High-priority for review May 31, 2018
Copy link
Contributor

@ajtowns ajtowns left a 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
try {
gArgs.ReadRWConfigFile(gArgs.GetArg("-confrw", BITCOIN_RW_CONF_FILENAME));
} catch (const std::exception& e) {
// Ignore problems here, since we are responsible for this file

This comment has been minimized.

@ajtowns

ajtowns Jun 2, 2018
Contributor

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?

src/init.cpp Outdated Show resolved Hide resolved
@MarcoFalke MarcoFalke removed this from Blockers in High-priority for review Jun 13, 2018
@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Jun 13, 2018

Let me know when I can add this back to project 8, i.e. when it is ready for review.

src/util/system.cpp Outdated Show resolved Hide resolved
src/util/system.cpp Outdated Show resolved Hide resolved
src/util/system.cpp Outdated Show resolved Hide resolved
@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Nov 7, 2018

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.

@luke-jr luke-jr force-pushed the luke-jr:rwconf branch to 4f5794f Nov 7, 2018
@DrahtBot
Copy link
Contributor

@DrahtBot DrahtBot commented Nov 9, 2018

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #19461 (multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky)
  • #19460 (multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky)
  • #17580 (refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky)
  • #16545 (refactor: Implement missing error checking for ArgsManager flags by ryanofsky)
  • #14866 ([wip] util: Improve evaluation of includeconf lines in network sections of the config file by AkioNak)
  • #10102 ([experimental] Multiprocess bitcoin by ryanofsky)

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.

Copy link
Member

@Sjors Sjors left a 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, bitcoind users 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.

doc/files.md Outdated Show resolved Hide resolved
@laanwj laanwj removed this from Blockers in High-priority for review Aug 13, 2020
@luke-jr luke-jr force-pushed the luke-jr:rwconf branch from 78bc867 to 6095c42 Aug 20, 2020
@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Aug 20, 2020

Rebased

@DrahtBot DrahtBot removed the Needs rebase label Aug 20, 2020
@jnewbery
Copy link
Member

@jnewbery jnewbery commented Aug 20, 2020

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?

@luke-jr
Copy link
Member Author

@luke-jr luke-jr commented Aug 20, 2020

So we can revert settings.json before 0.21 is released and we're locked in to supporting such a bad idea?

@jnewbery
Copy link
Member

@jnewbery jnewbery commented Aug 20, 2020

NACK. Let's close this and move on.

@Sjors
Copy link
Member

@Sjors Sjors commented Oct 15, 2020

I have no objection to swapping the new settings.json out for bitcoin_rw.conf before the release, if this gets enough review. But I probably won't review this, because I don't think the difference is worth it. I don't expect users to manually read or edit this, since they can already access bitcoin.conf. So the choice of file format seems irrelevant.

The (rather large amount of) code in ModifyRWConfigStream presents entirely new file parsing. It would both be more useful, and easier to review, if it was also used to parse bitcoin.conf.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.