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

pull luke-jr wants to merge 4 commits into bitcoin:master from luke-jr:rwconf changing 10 files +446 −11

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
luke-jr cross-referenced this on Mar 16, 2018 from issue Exposing prune option in GUI by sime
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 cross-referenced this on Mar 29, 2018 from issue [qt] move QSettings to bitcoin_rw.conf where possible by Sjors
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 cross-referenced this on Apr 30, 2018 from issue [qt] OptionsDialog: add prune setting by Sjors
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: contributor

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.
Sjors cross-referenced this on Jun 15, 2018 from issue use IsBlockPruned() where appropriate by kallewoof
Sjors cross-referenced this on Jul 6, 2018 from issue Interpret absense of prune= as prune=1 if there are pruned blocks by Sjors
Sjors cross-referenced this on Jul 31, 2018 from issue More intuitive GUI settings behavior when -proxy is set by Sjors
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:

s = s.substr(0, n2); s += 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):

https://isocpp.org/wiki/faq/style-and-techniques#static-cast

https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#es49-if-you-must-use-a-cast-use-a-named-cast

https://www.aristeia.com/EC3E/3E_item27.pdf
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: contributor


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
Sjors cross-referenced this on Dec 10, 2018 from issue gui: Add dynamic wallets support by promag
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

Sjors cross-referenced this on Dec 15, 2018 from issue [RFC] Long term plan for wallet command-line args by jnewbery

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:

// Map of option name -> number of option values prepended by this ReadConfigStream call.
// 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: contributor
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 <datadir>/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 force-pushed on Feb 12, 2019
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)
Sjors cross-referenced this on Feb 13, 2019 from issue gui: Add Close Wallet action by promag
achow101 cross-referenced this on Feb 20, 2019 from issue Remove the automatic creation and loading of the default wallet by achow101
DrahtBot added the label Needs rebase on Feb 21, 2019
Sjors cross-referenced this on Feb 21, 2019 from issue [wip] util: Improve evaluation of includeconf lines in network sections of the config file by AkioNak
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
luke-jr cross-referenced this on May 2, 2019 from issue Add <datadir>/settings.json persistent settings storage by ryanofsky
luke-jr cross-referenced this on Aug 25, 2019 from issue gui: add prune to intro screen with smart default by Sjors
Sjors cross-referenced this on Sep 16, 2019 from issue Dynamic wallet load / create / unload by jnewbery
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 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
Sjors cross-referenced this on Jan 3, 2020 from issue qt: Force set nPruneSize in QSettings after the intro dialog by hebasto
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
DrahtBot cross-referenced this on May 29, 2020 from issue Reduce cs_main lock accumulation during GUI startup by jonasschnelli
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.
DrahtBot cross-referenced this on Jun 5, 2020 from issue interfaces: Expose settings.json methods to GUI by ryanofsky
DrahtBot cross-referenced this on Jun 5, 2020 from issue Add loadwallet and createwallet load_on_startup options by ryanofsky
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
DrahtBot cross-referenced this on Jul 21, 2020 from issue refactor: Pass ArgsManager into functions that register args by S3RK
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.
DrahtBot cross-referenced this on Sep 2, 2020 from issue refactor: Implement missing error checking for ArgsManager flags by ryanofsky
DrahtBot cross-referenced this on Sep 3, 2020 from issue refactor: Add ALLOW_LIST flags and enforce usage in CheckArgFlags by ryanofsky
DrahtBot cross-referenced this on Sep 3, 2020 from issue util: Fix -norpcwhitelist, -norpcallowip, and similar corner case behavior by ryanofsky
laanwj added this to the "Blockers" column in a project

DrahtBot cross-referenced this on Sep 22, 2020 from issue refactor: Signet fixups by MarcoFalke
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
DrahtBot cross-referenced this on Sep 29, 2020 from issue multiprocess: Add bitcoin-gui -ipcconnect option by ryanofsky
DrahtBot cross-referenced this on Sep 29, 2020 from issue multiprocess: Add bitcoin-wallet -ipcconnect option by ryanofsky
DrahtBot cross-referenced this on Sep 29, 2020 from issue refactor: Remove settings merge reverse precedence code by ryanofsky
DrahtBot cross-referenced this on Sep 29, 2020 from issue util: Forbid ambiguous multiple assignments in config file by ryanofsky
DrahtBot cross-referenced this on Sep 29, 2020 from issue Multiprocess bitcoin by ryanofsky
DrahtBot cross-referenced this on Oct 4, 2020 from issue doc: Update and improve files.md by hebasto
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