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

luke-jr commented at 10:18 pm on August 17, 2017: member

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

luke-jr force-pushed on Aug 17, 2017

Sjors commented at 3:33 pm on March 17, 2018: member

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 referenced this in commit 1dbfbd1d21 on Mar 29, 2018

Sjors commented at 3:09 pm on March 30, 2018: member

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 force-pushed on Mar 31, 2018

luke-jr commented at 9:24 pm on March 31, 2018: member

Rebased

Sjors commented at 10:56 am on April 3, 2018: member

tACK aac0501 (tested through #12833)

Sjors commented at 11:10 am on May 15, 2018: member

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

laanwj added this to the "Blockers" column in a project

in src/bitcoind.cpp:124 in aac0501148 outdated

117@@ -118,6 +118,12 @@ bool AppInit(int argc, char* argv[])
118             return false;
119         }
120 
121+        try {
122+            gArgs.ReadRWConfigFile(gArgs.GetArg("-confrw", BITCOIN_RW_CONF_FILENAME));
123+        } catch (const std::exception& e) {
124+            // Ignore problems here, since we are responsible for this file

ajtowns commented at 10:51 am on June 2, 2018:

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?

in src/init.cpp:335 in aac0501148 outdated

331@@ -332,6 +332,7 @@ std::string HelpMessage(HelpMessageMode mode)
332     if (showDebug)
333         strUsage += HelpMessageOpt("-blocksonly", strprintf(_("Whether to operate in a blocks only mode (default: %u)"), DEFAULT_BLOCKSONLY));
334     strUsage += HelpMessageOpt("-conf=<file>", strprintf(_("Specify configuration file. Relative paths will be prefixed by datadir location. (default: %s)"), BITCOIN_CONF_FILENAME));
335+    strUsage += HelpMessageOpt("-confrw=<file>", strprintf(_("Specify read/write configuration file. Relative paths will be prefixed by the network-specific datadir location. (default: %s)"), BITCOIN_RW_CONF_FILENAME));

ajtowns commented at 10:53 am on June 2, 2018:

“rwconfig” is an implementation detail; it might be better to describe it via it’s purpose instead. “confui – Specify configuration file that stores settings set in the UI” or something?

ryanofsky commented at 6:43 pm on June 4, 2018:

“rwconfig” is an implementation detail; it might be better to describe it via it’s purpose instead. “confui – Specify configuration file that stores settings set in the UI” or something?

IMO, rwconf is nice because it would let us an add an RPC interface for updating settings and persisting them.

ajtowns commented at 11:05 am on June 2, 2018: member

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.

MarcoFalke added the label Needs rebase on Jun 6, 2018

MarcoFalke removed this from the "Blockers" column in a project

MarcoFalke commented at 5:27 pm on June 13, 2018: member

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

luke-jr force-pushed on Nov 7, 2018

DrahtBot removed the label Needs rebase on Nov 7, 2018

in src/util/system.cpp:1106 in 2d2de023a4 outdated

1101+        n = s.find('#');
1102+        has_comment = (n != std::string::npos);
1103+        if (!has_comment) {
1104+            n = s.size();
1105+        }
1106+        n2 = s.find_last_not_of(ws_chars, n - 1);

practicalswift commented at 6:01 pm on November 7, 2018:

An integer wraparound will occur here in the case of n == 0.

in src/util/system.h:234 in 2d2de023a4 outdated

168@@ -162,6 +169,10 @@ class ArgsManager
169     bool ParseParameters(int argc, const char* const argv[], std::string& error);
170     bool ReadConfigFiles(std::string& error, bool ignore_invalid_keys = false);
171 
172+    void ModifyRWConfigFile(const std::map<std::string, std::string>& settings_to_change);
173+    void ModifyRWConfigFile(const std::string& setting_to_change, const std::string& new_value);

practicalswift commented at 6:05 pm on November 7, 2018:

Make sure parameters name match between declaration and definition :-)

promag commented at 4:28 pm on December 18, 2018:

Done.

in src/util/system.cpp:1167 in 2d2de023a4 outdated

1173+                // Just modify the value in-line otherwise
1174+                n2 = s.find_first_not_of(ws_chars, n + 1);
1175+                if (n2 == std::string::npos) {
1176+                    n2 = n + 1;
1177+                }
1178+                s = s.substr(0, n2) + val;

practicalswift commented at 6:14 pm on November 7, 2018:

Nit: Avoid extra allocations by doing:

0s = s.substr(0, n2);
1s += val;

luke-jr commented at 8:19 pm on November 7, 2018:

I think the current style is more readable.

practicalswift commented at 8:34 pm on November 7, 2018:

I see your point and readability is probably more important than allocation efficiency in this case :-)

in src/util/system.cpp:1094 in 2d2de023a4 outdated

1089+{
1090+    static const char * const ws_chars = ModifyRWConfigFile_ws_chars;
1091+    std::set<std::string> setFound;
1092+    std::string s, lineend, linebegin, key;
1093+    std::string::size_type n, n2;
1094+    bool inside_group = false, have_eof_nl = true, has_comment;

practicalswift commented at 6:23 pm on November 7, 2018:

The scope of has_comment can be reduced?

in src/util/system.cpp:1042 in 2d2de023a4 outdated

1037+        i = streamIn.get();
1038+        if (i == std::char_traits<char>::eof()) {
1039+            return false;
1040+        }
1041+        s.clear();
1042+        s.push_back(char(i));

practicalswift commented at 6:24 pm on November 7, 2018:

Use (char)i to get it consistent with the rest of the code base :-)

luke-jr commented at 8:21 pm on November 7, 2018:

C-style casts aren’t good practice in C++.

practicalswift commented at 8:33 pm on November 7, 2018:

Agreed. static_cast<char>(i) then?

The reasons I personally prefer static_cast<char>(i):

luke-jr commented at 10:36 pm on November 7, 2018:

There is no need for a cast at all. C++ allows construction of char just like any other type.

practicalswift commented at 11:15 pm on November 7, 2018:

I’m not sure I follow the “no need for cast” statement. Do we agree on the following two statements? :-)

The functional cast expression char(i) is equivalent to the C-style cast expression (char)i
When a C-style cast expression is encountered, the compiler interprets it as the first named cast that satisfies the requirements of the respective cast operator in the order: a. const_cast<T>(…), b. static_cast<T>(…), …, etc.

sipa commented at 11:31 pm on November 7, 2018:

For primitive types we’ve generally used C-style casts over C++-style ones (they’re equivalent in that case, and much less syntactic burden).

luke-jr commented at 10:36 pm on November 7, 2018: member

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 force-pushed on Nov 7, 2018

DrahtBot commented at 3:48 am on November 9, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#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)

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.

in doc/files.md:4 in 4f5794f776 outdated

0@@ -1,6 +1,7 @@
1 
2 * banlist.dat: stores the IPs/Subnets of banned nodes
3 * bitcoin.conf: contains configuration settings for bitcoind or bitcoin-qt
4+* bitcoin_rw.conf: contains configuration settings modified by bitcoind or bitcoin-qt: since 0.16.0

Sjors commented at 9:38 am on November 10, 2018:

Nit: 0.18.0

Sjors commented at 12:13 pm on November 10, 2018: member

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.

DrahtBot added the label Needs rebase on Nov 19, 2018

luke-jr force-pushed on Dec 13, 2018

DrahtBot removed the label Needs rebase on Dec 13, 2018

luke-jr commented at 1:54 am on December 14, 2018: member

Rebased (and ready for high-prio review list)

meshcollider added this to the "Blockers" column in a project

in src/util/system.cpp:887 in e95124005d outdated

893     std::vector<std::pair<std::string, std::string>> options;
894     m_config_sections.clear();
895     if (!GetConfigOptions(stream, error, options, m_config_sections)) {
896         return false;
897     }
898+    std::map<std::string, size_t> offsets;

ryanofsky commented at 6:00 pm on December 18, 2018:

In commit “util: Support prepending configs in ReadConfigStream” (e95124005d4b0ad1d343eb60662d3e82a9e9194b)

Could add description of offsets map:

0// Map of option name -> number of option values prepended by this ReadConfigStream call.
1// Only used when prepend=true.

in src/util/system.h:154 in e95124005d outdated

150@@ -151,7 +151,7 @@ class ArgsManager
151     std::map<OptionsCategory, std::map<std::string, Arg>> m_available_args GUARDED_BY(cs_args);
152     std::set<std::string> m_config_sections GUARDED_BY(cs_args);
153 
154-    NODISCARD bool ReadConfigStream(std::istream& stream, std::string& error, bool ignore_invalid_keys = false);
155+    NODISCARD bool ReadConfigStream(std::istream& stream, std::string& error, bool ignore_invalid_keys = false, bool prepend = false);

ryanofsky commented at 6:09 pm on December 18, 2018:

In commit “util: Support prepending configs in ReadConfigStream” (e95124005d4b0ad1d343eb60662d3e82a9e9194b)

Could definitely use a c++ unit test checking the prepend behavior. Especially for the negated args part, which I could easily imagine someone screwing up in the future.

in src/util/system.cpp:972 in cfd54102b6 outdated

1003@@ -1004,6 +1004,14 @@ bool ArgsManager::ReadConfigFiles(std::string& error, bool ignore_invalid_keys)
1004         }
1005     }
1006 
1007+    // Check for -testnet or -regtest parameter (BaseParams() calls are only valid after this clause)

ryanofsky commented at 6:40 pm on December 18, 2018:

In commit “util: SelectBaseParams in ReadConfigFiles, before getting final datadir” (cfd54102b60bc1a83c032f041e4199cf89e422f1)

I don’t understand what this commit is supposed to be doing. GetDataDir seems to be called with fNetSpecific=false below so it seems like baseparams wouldn’t be accessed here. Also, it doesn’t seem ideal that now SelectBaseParams will be called multiple times at startup instead of just once. I think right now it is only called once from SelectParams(). Would it be possible to add a unit test or an assert or something that guards against whatever condition this commit is supposed to prevent? Or maybe an english language description of what this commit is supposed to accomplish?

luke-jr commented at 8:55 pm on May 8, 2019:

bitcoin_rw.conf is located in the network-specific data dir.

ryanofsky commented at 7:47 pm on December 18, 2018: member

I started reviewing this, and the first few commits seem reasonable. But the last commit is a lot more complicated than I would have expected, and doesn’t seem especially maintainable or user friendly.

It seems like all we need is basic storage for settings that can be updated in the gui and maybe over rpc. I’d think the simplest way to do to this would be to add a UniValue m_rwsettings; member to the GlobalArgs class, tweak GetArg...() methods to return these settings, and serialize/deserialize the settings member as needed to <datadir>/settings.json by calling existing univalue read and write methods.

Commit “Add new bitcoin_rw.conf file that is used for settings modified by this software itself” (31edb2c940caa3594d5235cc430f6fe5631c464a) is doing something more complicated:

It adds a new -confrw option, for unclear reasons. Maybe to allow users to write settings outside the data dir? Or maybe to allow users control naming of the file, or toggle between different collections of settings within the datadir?
Instead of storing settings in a simple, machine readable format, it stores them in full ini format with support for comments and other complicated parsing rules.
Instead of using the existing ini parser, it adds a new parser, which will have to be kept in sync with the existing one.

I’m happy to do more review on this PR if this is the approach people want to take. But would like some confirmation this is actually the approach people want to take.

jonasschnelli commented at 6:24 am on January 4, 2019: contributor

It seems like all we need is basic storage for settings that can be updated in the gui and maybe over rpc. I’d think the simplest way to do to this would be to add a UniValue m_rwsettings; member to the GlobalArgs class, tweak GetArg…() methods to return these settings, and serialize/deserialize the settings member as needed to /settings.json by calling existing univalue read and write methods.

I agree with @ryanofsky. We do not have JSON configuration files, but for the rw settings, I think this is acceptable and easy to maintain. The code complexity would drop a lot and – since JSON is easy to manipulate with a text editor – still partially editable by humans.

luke-jr commented at 10:21 am on January 4, 2019: member

Pretty sure UniValue is actually more code than this simple INI modification logic…

INI is also a simple machine-readable format, and we’re already using it. The minimal complexity in modifications exists to preserve user edits (which UniValue doesn’t support at all).

jonasschnelli commented at 6:21 am on January 11, 2019: contributor

There has been a quick discussion about UniValue vs INI files during the todays IRC meeting: http://www.erisian.com.au/bitcoin-core-dev/log-2019-01-10.html#l-451

Sjors commented at 7:36 pm on January 15, 2019: member

tl&dr of the IRC discussion seems to be “whatever is less code”, so not much difference to the discussion here. I think there’s something to be said for keeping the same format.

If we go for a different format, then I should repeat my point that I don’t like how bitcoin-cli, and by extension other applications that use the RPC that rely on bitcoin.conf to figure out how to talk to it, can also process this file.

I rebased #12833 again.

DrahtBot added the label Needs rebase on Jan 21, 2019

laanwj removed this from the "Blockers" column in a project

luke-jr force-pushed on Feb 12, 2019

luke-jr commented at 3:17 pm on February 12, 2019: member

Rebased

DrahtBot removed the label Needs rebase on Feb 12, 2019

Sjors commented at 6:29 pm on February 12, 2019: member

At least one of the Travis machines failed with test_bitcoin-qt: util/system.cpp:1213: void ArgsManager::EraseRWConfigFile(): Assertion !rwconf_path.empty()’ failed.`

I also rebased #12833; everything still works afaik.

Also worth noting that this PR could make dynamic wallet loading / unloading in the GUI more useful (because we can remember which wallets are open). (update: see #15454)

DrahtBot added the label Needs rebase on Feb 21, 2019

achow101 commented at 7:57 pm on February 21, 2019: member

This currently only allows each config option to be specified once. However the normal bitcoin.conf file allows specifying some argument multiple times (e.g. wallet). It would be nice if this did that too.

luke-jr commented at 2:46 am on February 23, 2019: member

@achow101 Yes, but that seems better left for a subsequent PR. This is good enough for most use cases right now.

luke-jr force-pushed on Apr 10, 2019

luke-jr force-pushed on Apr 25, 2019

luke-jr force-pushed on May 2, 2019

achow101 commented at 9:56 pm on September 24, 2019: member

@luke-jr Care to rebase?

jtimon commented at 11:00 pm on October 10, 2019: contributor

Concept ACK

luke-jr force-pushed on Oct 12, 2019

DrahtBot removed the label Needs rebase on Oct 12, 2019

DrahtBot added the label Docs on Oct 12, 2019

DrahtBot added the label GUI on Oct 12, 2019

DrahtBot added the label Tests on Oct 12, 2019

DrahtBot added the label Utils/log/libs on Oct 12, 2019

luke-jr force-pushed on Oct 12, 2019

luke-jr commented at 1:49 am on October 13, 2019: member

Rebased

Sjors commented at 8:19 am on October 15, 2019: member

Rebased #12833 (move QSettings to bitcoin_rw.conf) on top.

DrahtBot added the label Needs rebase on Oct 16, 2019

achow101 commented at 9:07 pm on October 16, 2019: member

The bitcoin_rw.conf file is being written to the network specific directories and not the datadir itself, so when using regtest or testnet mode, it won’t be read and handled.

So either the network specific conf needs to be read, or it needs to be written to the datadir, preferably with network specific settings so that things done on each network don’t interfere with each other.

luke-jr commented at 9:12 pm on October 16, 2019: member

It’s being read from the network-specific directory too…

Sjors commented at 9:17 am on October 21, 2019: member

This is consistent with how QT stores network specific settings.

achow101 commented at 3:31 pm on October 21, 2019: member

It’s being read from the network-specific directory too…

It didn’t seem to be working for me in #15454, maybe I was just doing something wrong there. Edit: Other changes to ArgsManager were causing my problems.

luke-jr force-pushed on Oct 29, 2019

luke-jr commented at 2:15 am on October 29, 2019: member

Trivial rebased

DrahtBot removed the label Needs rebase on Oct 29, 2019

DrahtBot added the label Needs rebase on Nov 8, 2019

jonasschnelli commented at 6:59 am on May 29, 2020: contributor

What is the status of this? Shall we close it? It had a few concept ACKs.

jonasschnelli added the label Waiting for author on May 29, 2020

Sjors commented at 8:57 am on May 29, 2020: member

I would still like to see this, or something equivalent…

luke-jr force-pushed on May 29, 2020

luke-jr commented at 2:55 pm on May 29, 2020: member

Forgot to push the last rebase… pushed.

DrahtBot removed the label Needs rebase on May 29, 2020

jonasschnelli commented at 6:56 am on June 5, 2020: contributor

@luke-jr: pull is failing on CIs,… this PR could use some love and care.

luke-jr force-pushed on Jun 18, 2020

luke-jr commented at 4:17 pm on June 18, 2020: member

Rebased yet again

laanwj added this to the "Blockers" column in a project

laanwj removed the label Waiting for author on Jul 16, 2020

MarcoFalke removed the label Docs on Jul 22, 2020

MarcoFalke removed the label GUI on Jul 22, 2020

MarcoFalke removed the label Tests on Jul 22, 2020

DrahtBot added the label Needs rebase on Jul 23, 2020

laanwj removed this from the "Blockers" column in a project

luke-jr force-pushed on Aug 20, 2020

luke-jr commented at 3:46 pm on August 20, 2020: member

Rebased

DrahtBot removed the label Needs rebase on Aug 20, 2020

jnewbery commented at 6:25 pm on August 20, 2020: member

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 commented at 6:57 pm on August 20, 2020: member

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

jnewbery commented at 7:22 pm on August 20, 2020: member

NACK. Let’s close this and move on.

laanwj added this to the "Blockers" column in a project

DrahtBot added the label Needs rebase on Sep 23, 2020

util/settings: Add place to put rwconf settings 24e8a4e394

util: SelectBaseParams in ReadConfigFiles, before getting final datadir e020477473

util/settings: Support ArgsManager::ReadConfigStream into other targets 38e2657032

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

luke-jr force-pushed on Sep 23, 2020

DrahtBot removed the label Needs rebase on Sep 23, 2020

Sjors commented at 6:21 pm on October 15, 2020: member

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.

fanquake commented at 1:22 pm on November 18, 2020: member

Now that we’ve branched off, and are unlikely to be reverting settings.json, I’m going to close this PR.

fanquake closed this on Nov 18, 2020

fanquake removed this from the "Blockers" column in a project

DrahtBot locked this on Feb 15, 2022

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

Conflicts