From 5e744f423838fe7d45453541271bc1a07cd62eac Mon Sep 17 00:00:00 2001 From: josibake Date: Thu, 28 Jul 2022 12:47:53 +0200 Subject: [PATCH 1/3] 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. --- src/util/system.cpp | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/util/system.cpp b/src/util/system.cpp index c3c6cbfef62..948b3f26736 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -935,6 +935,14 @@ static bool GetConfigOptions(std::istream& stream, const std::string& filepath, return true; } +bool IsConfSupported(KeyInfo& key, std::string& error) { + if (key.name == "conf") { + error = "conf cannot be set in the configuration file; use includeconf= if you want to include additional config files"; + return false; + } + return true; +} + bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& filepath, std::string& error, bool ignore_invalid_keys) { LOCK(cs_args); @@ -945,6 +953,7 @@ bool ArgsManager::ReadConfigStream(std::istream& stream, const std::string& file for (const std::pair& option : options) { KeyInfo key = InterpretKey(option.first); std::optional flags = GetArgFlags('-' + key.name); + if (!IsConfSupported(key, error)) return false; if (flags) { std::optional value = InterpretValue(key, &option.second, *flags, error); if (!value) { From 2e3826cbcd675dcd1d03970233ba5e143e09eb75 Mon Sep 17 00:00:00 2001 From: josibake Date: Fri, 29 Jul 2022 13:56:43 +0200 Subject: [PATCH 2/3] 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. --- src/util/system.cpp | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/util/system.cpp b/src/util/system.cpp index 948b3f26736..390d58b63b8 100644 --- a/src/util/system.cpp +++ b/src/util/system.cpp @@ -940,6 +940,12 @@ bool IsConfSupported(KeyInfo& key, std::string& error) { error = "conf cannot be set in the configuration file; use includeconf= if you want to include additional config files"; return false; } + if (key.name == "reindex") { + // reindex can be set in a config file but it is strongly discouraged as this will cause the node to reindex on + // every restart. Allow the config but throw a warning + 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"); + return true; + } return true; } From deba6fe3158cd0b2283e0901a072e434ba5b594e Mon Sep 17 00:00:00 2001 From: josibake Date: Sun, 31 Jul 2022 16:13:06 +0200 Subject: [PATCH 3/3] test: update feature_config_args.py add two new test cases for conf and reindex --- test/functional/feature_config_args.py | 22 ++++++++++++++++++++++ 1 file changed, 22 insertions(+) diff --git a/test/functional/feature_config_args.py b/test/functional/feature_config_args.py index eb31bca29ae..112dbb9e6ac 100755 --- a/test/functional/feature_config_args.py +++ b/test/functional/feature_config_args.py @@ -20,11 +20,25 @@ class ConfArgsTest(BitcoinTestFramework): self.disable_autoconnect = False def test_config_file_parser(self): + self.log.info('Test config file parser') self.stop_node(0) + # Check that startup fails if conf= is set in bitcoin.conf or in an included conf file + bad_conf_file_path = os.path.join(self.options.tmpdir, 'node0', 'bitcoin_bad.conf') + util.write_config(bad_conf_file_path, n=0, chain='', extra_config=f'conf=some.conf\n') + conf_in_config_file_err = 'Error: Error reading configuration file: conf cannot be set in the configuration file; use includeconf= if you want to include additional config files' + self.nodes[0].assert_start_raises_init_error( + extra_args=[f'-conf={bad_conf_file_path}'], + expected_msg=conf_in_config_file_err, + ) inc_conf_file_path = os.path.join(self.nodes[0].datadir, 'include.conf') with open(os.path.join(self.nodes[0].datadir, 'bitcoin.conf'), 'a', encoding='utf-8') as conf: conf.write(f'includeconf={inc_conf_file_path}\n') + with open(inc_conf_file_path, 'w', encoding='utf-8') as conf: + conf.write('conf=some.conf\n') + self.nodes[0].assert_start_raises_init_error( + expected_msg=conf_in_config_file_err, + ) self.nodes[0].assert_start_raises_init_error( expected_msg='Error: Error parsing command line arguments: Invalid parameter -dash_cli=1', @@ -32,10 +46,18 @@ class ConfArgsTest(BitcoinTestFramework): ) with open(inc_conf_file_path, 'w', encoding='utf-8') as conf: conf.write('dash_conf=1\n') + with self.nodes[0].assert_debug_log(expected_msgs=['Ignoring unknown configuration value dash_conf']): self.start_node(0) self.stop_node(0) + with open(inc_conf_file_path, 'w', encoding='utf-8') as conf: + conf.write('reindex=1\n') + + with self.nodes[0].assert_debug_log(expected_msgs=['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']): + self.start_node(0) + self.stop_node(0) + with open(inc_conf_file_path, 'w', encoding='utf-8') as conf: conf.write('-dash=1\n') self.nodes[0].assert_start_raises_init_error(expected_msg='Error: Error reading configuration file: parse error on line 1: -dash=1, options in configuration file must be specified without leading -')