util, config: error on startup if `conf` or `reindex` are set in config file #25727

josibake commented at 11:11 AM on July 28, 2022: member

In help from bitcoind -h it specifes that conf can only be used from the commandline. However, if conf is set in a bitcoin.conf file, there is no error and from reading the logs it seems as if the conf=<other file> is being used, despite it being ignored. To recreate, you can setup a bitcoin.conf file in the default directory, add conf=<some other file>.conf and in the separate config file set whichever config value you want and verify that it is being ignored. alternatively, if you set includeconf=<some other file>.conf , your config in <some other file> will be picked up.

This PR fixes this by having the node error when reading the config file if conf= is set.

Additionally, it was mentioned in a recent PR review club that if reindex=1 is set in the config file, the node will reindex on every startup, which is undesirable:

17:14 <larryruane> michaelfolkson: Reindex is requested by the user (node operator) as a configuration option (command line or in the config file, tho you probably would never put it in the file, or else it would reindex on every startup!)

This PR also has a commit to warn if reindex=1 is set in the config file.

josibake renamed this:
~~util, config: update config error messages for `conf` and `reindex`~~
util, config: error on startup if `conf` or `reindex` are set in config file
on Jul 28, 2022

in src/init.cpp:433 in 79e5606e75 outdated

 429 | @@ -430,7 +430,7 @@ void SetupServerArgs(ArgsManager& argsman)
 430 |      argsman.AddArg("-prune=<n>", strprintf("Reduce storage requirements by enabling pruning (deleting) of old blocks. This allows the pruneblockchain RPC to be called to delete specific blocks and enables automatic pruning of old blocks if a target size in MiB is provided. This mode is incompatible with -txindex. "
 431 |              "Warning: Reverting this setting requires re-downloading the entire blockchain. "
 432 |              "(default: 0 = disable pruning blocks, 1 = allow manual pruning via RPC, >=%u = automatically prune block files to stay under the specified target size in MiB)", MIN_DISK_SPACE_FOR_BLOCK_FILES / 1024 / 1024), ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 433 | -    argsman.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk. This will also rebuild active optional indexes.", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);
 434 | +    argsman.AddArg("-reindex", "Rebuild chain state and block index from the blk*.dat files on disk. This will also rebuild active optional indexes (only useable from command line, not configuration file)", ArgsManager::ALLOW_ANY, OptionsCategory::OPTIONS);

aureleoules commented at 11:44 AM on July 28, 2022:

You removed the period but I realize the punctuation is not consistent across the commands

josibake commented at 11:49 AM on July 28, 2022:

yeah, it's mixed in the file, but im used to (and prefer) not having punctuation in stdout help text

aureleoules commented at 12:13 PM on July 28, 2022:

you also mixed it here 😄 https://github.com/bitcoin/bitcoin/blob/79e5606e750b28fa2123f96ff07757c931ae5f91/src/util/system.cpp#L948-L955

josibake commented at 12:17 PM on July 28, 2022:

oops! good catch, ill make them consistent

aureleoules commented at 11:46 AM on July 28, 2022: member

ACK 79e5606e750b28fa2123f96ff07757c931ae5f91.

I think it is reasonable to throw an error if conf is set in the config file as it doesn't make sense. At first I thought showing a warning if -reindex=1 is set in the config would be better than an error but I don't see any use-case for reindexing on every startup so I believe error-ing is better.

josibake force-pushed on Jul 28, 2022

josibake commented at 12:21 PM on July 28, 2022: member

force pushed to format error messages consistently; git range-diff master 79e5606 c7ebf00

aureleoules commented at 12:21 PM on July 28, 2022: member

re-ACK c7ebf00cdf1e9f275a847e07c3d7c6c940ab9cb2

in src/util/system.cpp:955 in c7ebf00cdf outdated

 950 | +            return false;
 951 | +        }
 952 | +        if (key.name == "reindex") {
 953 | +            error = "reindex cannot be set in the configuratoin file; use -reindex=1 from the commandline";
 954 | +            return false;
 955 | +        }

furszy commented at 2:27 PM on July 28, 2022:

quick thing: would be better to decouple this into a different function IsConfKeySupported (or something similar) to keep ArgsManager::ReadConfigStream as general as possible and not pollute it with the inclusion of every single not supported option (might be more in the future).

josibake commented at 12:07 PM on July 29, 2022:

great suggestion; added!

ryanofsky commented at 2:42 PM on July 28, 2022: contributor

I think it would be good to log a warning if reindex is specified in the configuration file, but still allow it. It seems to me there may be a lot of cases where it is awkward or difficult to change command line arguments and easier to temporarily add or uncomment reindex=1 from the config file. Examples might be when bitcoind is running as a systemd service (especially on a system with an immutable or autogenerated /etc) or where bitcoin-qt is started from an icon shortcut with hardcoded arguments.

Also I doubt reindex=1 is the only configuration option that most people won't want in their config file, so making it a hard error and treating it so differently than other configurations options seems a bit of a scattershot solution.

josibake commented at 3:03 PM on July 28, 2022: member

Examples might be when bitcoind is running as a systemd service (especially on a system with an immutable or autogenerated /etc)

That's a great point, I'll change it to a warning. Initially, I was worried a warning would go unnoticed by most users, although thinking about this a bit more, if the node were to take forever on every restart, most users would likely check the logs first and see the warning.

DrahtBot commented at 3:58 PM on July 28, 2022: contributor

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

Conflicts

No conflicts as of last run.

josibake force-pushed on Jul 29, 2022

josibake commented at 12:10 PM on July 29, 2022: member

updated to only warn for reindex per @ryanofsky and added IsConfSupported function per @furszy

in src/util/system.cpp:946 in 5cf1b465f4 outdated

 939 | @@ -940,6 +940,12 @@ bool IsConfSupported(KeyInfo key, std::string& error) {
 940 |          error = "conf cannot be set in the configuration file; use includeconf= if you want to include additional config files";
 941 |          return false;
 942 |      }
 943 | +    if (key.name == "reindex") {
 944 | +        // reindex can be set in a config file but it is strongly discouraged as this will cause the node to reindex on
 945 | +        // every restart. Allow the config but throw a warning
 946 | +        LogPrintf("Warning: reindex is set which will cause this node to reindex on every restart. Consider using -reindex=1 from the command-line\n");

ryanofsky commented at 6:34 PM on July 29, 2022:

In commit "util: warn if reindex is used in conf" (5cf1b465f4f9352415604c3eb31fc8cd01e4bd46)

This wording is not good because it doesn't clearly state the cause of the problem. It makes it sound like just setting reindex in general causes reindexing every restart. Also it makes it sound like adding -reindex=1 to the command line would fix the problem, when it wouldn't and the actual fix is to remove reindex=1 from the config file.

Would suggest: "Warning: reindex=1 is set in the configuration file, which will significantly slow down startup. Consider removing or commenting out this option for better performance, unless there is currently a condition which makes rebuilding the indexes necessary."

josibake commented at 2:19 PM on July 31, 2022:

nice, i like the suggested wording much better.

ryanofsky approved

ryanofsky commented at 6:47 PM on July 29, 2022: contributor

Code review ACK 5cf1b465f4f9352415604c3eb31fc8cd01e4bd46. Code changes look good but I would consider improving the warning text, see below. Also it would be good to add either unit tests [1] [2] or functional tests [3] for this

EDIT: fixed second link

josibake force-pushed on Jul 31, 2022

josibake commented at 4:25 PM on July 31, 2022: member

updated the wording per @ryanofsky 's suggestion and updated test/functional/feature_config_args.py to test for conf and reindex; git range-diff master 5cf1b46 8f13984

josibake force-pushed on Jul 31, 2022

ryanofsky approved

ryanofsky commented at 5:11 PM on August 1, 2022: contributor

Code review ACK 019e02cb26d87db9781353df8de70ed692069517. Thanks for update and new tests!

theStack commented at 1:14 AM on August 7, 2022: contributor

Concept ACK

luke-jr approved

luke-jr commented at 11:07 PM on August 9, 2022: member

utACK

Note there is a downside to a warning for reindex: by the time the user sees it, it is too late to cancel and keep the old index.

unknown approved

unknown commented at 1:08 AM on August 10, 2022: none

ACK https://github.com/bitcoin/bitcoin/pull/25727/commits/5f94b0210a8d4cf16f73f300c12d370af71bf9d8

Agreeing with only the changes in this commit

in src/util/system.cpp:938 in 019e02cb26 outdated

 934 | @@ -935,6 +935,20 @@ static bool GetConfigOptions(std::istream& stream, const std::string& filepath,
 935 |      return true;
 936 |  }
 937 |  
 938 | +bool IsConfSupported(KeyInfo key, std::string& error) {

luke-jr commented at 11:20 PM on August 25, 2022:

Maybe pass key by reference

bitcoin deleted a comment on Aug 26, 2022

util: disallow setting conf in bitcoin.conf

Help from `bitcoind -h` states that conf can only be used from the commandline.
However, if conf is set in a bitcoin.conf file, it is ignored but there is no error.

Show an error to user if conf is set in a .conf file and prompt them to use
`includeconf` if they wish to specify additional config files.

Adds `IsConfSupported` function to allow for easily adding conf options
to disallow or throw warnings for.

5e744f4238

util: warn if reindex is used in conf

using reindex in a conf file can lead to the node reindexing on every restart.
we still allow it but throw a warning.

2e3826cbcd

test: update feature_config_args.py

add two new test cases for conf and reindex

deba6fe315

josibake force-pushed on Oct 6, 2022

josibake commented at 10:22 PM on October 6, 2022: member

updated IsConfSupported to be pass by reference, per @luke-jr 's feedback; git range-diff master 019e02c deba6fe

in src/util/system.cpp:938 in deba6fe315

 934 | @@ -935,6 +935,20 @@ static bool GetConfigOptions(std::istream& stream, const std::string& filepath,
 935 |      return true;
 936 |  }
 937 |  
 938 | +bool IsConfSupported(KeyInfo& key, std::string& error) {

hebasto commented at 8:29 PM on October 13, 2022:

As this function is used in the only compilation unit it could be declared within an anonymous namespace or as static to keep global namespace hygiene (the former option is my personal preference :tiger2: ).

See: https://isocpp.github.io/CppCoreGuidelines/CppCoreGuidelines#sf22-use-an-unnamed-anonymous-namespace-for-all-internalnon-exported-entities

in src/util/system.cpp:946 in deba6fe315

 941 | +        return false;
 942 | +    }
 943 | +    if (key.name == "reindex") {
 944 | +        // reindex can be set in a config file but it is strongly discouraged as this will cause the node to reindex on
 945 | +        // every restart. Allow the config but throw a warning
 946 | +        LogPrintf("Warning: reindex=1 is set in the configuration file, which will significantly slow down startup. Consider removing or commenting out this option for better performance, unless there is currently a condition which makes rebuilding the indexes necessary\n");

hebasto commented at 8:35 PM on October 13, 2022:

In a follow up, it would be nice to show this warning in the GUI as well.

hebasto approved

hebasto commented at 8:35 PM on October 13, 2022: member

ACK deba6fe3158cd0b2283e0901a072e434ba5b594e, tested on Ubuntu 22.04.

in src/util/system.cpp:938 in 5e744f4238 outdated

 934 | @@ -935,6 +935,14 @@ static bool GetConfigOptions(std::istream& stream, const std::string& filepath,
 935 |      return true;
 936 |  }
 937 |  
 938 | +bool IsConfSupported(KeyInfo& key, std::string& error) {

aureleoules commented at 11:02 AM on October 14, 2022:

5e744f423838fe7d45453541271bc1a07cd62eac nit

bool IsConfSupported(const KeyInfo& key, std::string& error) {

aureleoules approved

aureleoules commented at 11:08 AM on October 14, 2022: member

tACK deba6fe3158cd0b2283e0901a072e434ba5b594e

ryanofsky approved

ryanofsky commented at 1:17 PM on October 14, 2022: contributor

Code review ACK deba6fe3158cd0b2283e0901a072e434ba5b594e.

This looks good to merge, but I do agree with other comments the function should be static and KeyInfo reference should be const. Since IsConfSupported is a nice example of a function that returns errors and warnings, I think I will convert it to use util::Result later #25665, and I could make these cleanups then.

ryanofsky commented at 1:34 PM on October 14, 2022: contributor

Note there is a downside to a warning for reindex: by the time the user sees it, it is too late to cancel and keep the old index.

That's a fair point, but I think the point of the warning is to give users a clue of what is causing their node to take a long time to start if they accidentally misconfigured it, or turned on the reindex flag one time and forgot about it. If we make it an error we get rid of a convenient way of enabling reindexing, so the warning is compromise a way to help someone stuck with a node that starts slowly due to a misconfiguration but otherwise works, without inconveniencing a user who is setting reindex intentionally.

fanquake merged this on Oct 21, 2022

fanquake closed this on Oct 21, 2022

fanquake commented at 8:40 AM on October 21, 2022: member

and I could make these cleanups then. @ryanofsky Sounds good.

sidhujag referenced this in commit f51751c93e on Oct 23, 2022

bitcoin locked this on Oct 21, 2023

josibake deleted the branch on Jan 26, 2024