highperfocused/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2026-06-02 01:04:43 +02:00
Code Issues Packages Projects Releases Wiki Activity

Merge #15934: Merge settings one place instead of five places

Browse Source 
083c954b02 Add settings_tests (Russell Yanofsky)
7f40528cd5 Deduplicate settings merge code (Russell Yanofsky)
9dcb952fe5 Add util::Settings struct and helper functions. (Russell Yanofsky)
e2e37cfe8a Remove includeconf nested scope (Russell Yanofsky)
5a84aa880f Rename includeconf variables for clarity (Russell Yanofsky)
dc8e1e7548 Clarify emptyIncludeConf logic (Russell Yanofsky)

Pull request description:

  This is a refactoring-only change that makes it easier to add a new settings source.

  This PR doesn't change behavior. The [`util_ArgsMerge`](deb2327b43/src/test/util_tests.cpp (L626-L822)) and [`util_ChainMerge`](deb2327b43/src/test/util_tests.cpp (L843-L924)) tests added in #15869 and #15988 were written specifically to confirm that ArgsManager settings are parsed, merged, and returned the same way before and after this change.

  This change:

  - Makes it easier to add new settings sources that can get merged with existing sources (see 70675c3e4975203ad6222ba2b00c83b4e4213793 from #15935).
  - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](c459c5f701/src/util/system.cpp (L221-L244)), [GetNetBoolArg](c459c5f701/src/util/system.cpp (L255-L261)), [GetArgs](c459c5f701/src/util/system.cpp (L460-L467)), [IsArgNegated](c459c5f701/src/util/system.cpp (L482-L491)), [GetUnsuitableSectionOnlyArgs](c459c5f701/src/util/system.cpp (L343-L352))) in inconsistent ways.
  - Documents and tests current strange merging behaviors, so they be cleaned up in the future if resulting code simplifications and UX improvements warrant loss of backwards compatibility. The newly documented behaviors are: command line [ignored arguments](69d44f3cc7/src/util/system.cpp (L323-L326)) and [more ignored arguments](69d44f3cc7/src/util/settings.cpp (L67-L72)), and config file [reverse precedence](69d44f3cc7/src/util/settings.cpp (L61-L65)), [inconsistently applied top-level settings](69d44f3cc7/src/util/settings.cpp (L55-L59)), and [zombie values](69d44f3cc7/src/util/settings.cpp (L101-L108)).

  The original motivation for this change was to make it easy to add a new persistent setting source without introducing more bugs and inconsistencies. Two commits building on top of this to add a persistent `-wallet` setting are pretty straightforward and show how the new code can be extended:

  * 70675c3e4975203ad6222ba2b00c83b4e4213793 from #15935 – _Add \<datadir>/settings.json persistent settings storage_
  * 04c80c40df9fc6f4734ba238ea7f65607cf88089 from #15937 – _Add loadwallet and createwallet RPC load_on_startup options_

ACKs for top commit:
  ariard:
    ACK 083c954
  jnewbery:
    ACK 083c954b02
  jamesob:
    ACK 083c954b02

Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659
This commit is contained in:
Wladimir J. van der Laan
2019-11-08 23:22:50 +01:00
parent adceca2ba5 083c954b02
commit a7aec7ad97
8 changed files with 610 additions and 283 deletions
Download Patch File Download Diff File Expand all files Collapse all files

163
src/test/settings_tests.cpp Normal file
View File

@@ -0,0 +1,163 @@
// Copyright (c) 2011-2019 The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.
#include <util/settings.h>
#include <test/util.h>
#include <test/util/setup_common.h>
#include <boost/test/unit_test.hpp>
#include <univalue.h>
#include <util/strencodings.h>
#include <vector>
BOOST_FIXTURE_TEST_SUITE(settings_tests, BasicTestingSetup)
//! Check settings struct contents against expected json strings.
static void CheckValues(const util::Settings& settings, const std::string& single_val, const std::string& list_val)
{
util::SettingsValue single_value = GetSetting(settings, "section", "name", false, false);
util::SettingsValue list_value(util::SettingsValue::VARR);
for (const auto& item : GetSettingsList(settings, "section", "name", false)) {
list_value.push_back(item);
}
BOOST_CHECK_EQUAL(single_value.write().c_str(), single_val);
BOOST_CHECK_EQUAL(list_value.write().c_str(), list_val);
};
// Simple settings merge test case.
BOOST_AUTO_TEST_CASE(Simple)
{
util::Settings settings;
settings.command_line_options["name"].push_back("val1");
settings.command_line_options["name"].push_back("val2");
settings.ro_config["section"]["name"].push_back(2);
// The last given arg takes precedence when specified via commandline.
CheckValues(settings, R"("val2")", R"(["val1","val2",2])");
util::Settings settings2;
settings2.ro_config["section"]["name"].push_back("val2");
settings2.ro_config["section"]["name"].push_back("val3");
// The first given arg takes precedence when specified via config file.
CheckValues(settings2, R"("val2")", R"(["val2","val3"])");
}
// Test different ways settings can be merged, and verify results. This test can
// be used to confirm that updates to settings code don't change behavior
// unintentionally.
struct MergeTestingSetup : public BasicTestingSetup {
//! Max number of actions to sequence together. Can decrease this when
//! debugging to make test results easier to understand.
static constexpr int MAX_ACTIONS = 3;
enum Action { END, SET, NEGATE, SECTION_SET, SECTION_NEGATE };
using ActionList = Action[MAX_ACTIONS];
//! Enumerate all possible test configurations.
template <typename Fn>
void ForEachMergeSetup(Fn&& fn)
{
ActionList arg_actions = {};
// command_line_options do not have sections. Only iterate over SET and NEGATE
ForEachNoDup(arg_actions, SET, NEGATE, [&]{
ActionList conf_actions = {};
ForEachNoDup(conf_actions, SET, SECTION_NEGATE, [&]{
for (bool force_set : {false, true}) {
for (bool ignore_default_section_config : {false, true}) {
fn(arg_actions, conf_actions, force_set, ignore_default_section_config);
}
}
});
});
}
};
// Regression test covering different ways config settings can be merged. The
// test parses and merges settings, representing the results as strings that get
// compared against an expected hash. To debug, the result strings can be dumped
// to a file (see comments below).
BOOST_FIXTURE_TEST_CASE(Merge, MergeTestingSetup)
{
CHash256 out_sha;
FILE* out_file = nullptr;
if (const char* out_path = getenv("SETTINGS_MERGE_TEST_OUT")) {
out_file = fsbridge::fopen(out_path, "w");
if (!out_file) throw std::system_error(errno, std::generic_category(), "fopen failed");
}
const std::string& network = CBaseChainParams::MAIN;
ForEachMergeSetup([&](const ActionList& arg_actions, const ActionList& conf_actions, bool force_set,
bool ignore_default_section_config) {
std::string desc;
int value_suffix = 0;
util::Settings settings;
const std::string& name = ignore_default_section_config ? "wallet" : "server";
auto push_values = [&](Action action, const char* value_prefix, const std::string& name_prefix,
std::vector<util::SettingsValue>& dest) {
if (action == SET || action == SECTION_SET) {
for (int i = 0; i < 2; ++i) {
dest.push_back(value_prefix + std::to_string(++value_suffix));
desc += " " + name_prefix + name + "=" + dest.back().get_str();
}
} else if (action == NEGATE || action == SECTION_NEGATE) {
dest.push_back(false);
desc += " " + name_prefix + "no" + name;
}
};
if (force_set) {
settings.forced_settings[name] = "forced";
desc += " " + name + "=forced";
}
for (Action arg_action : arg_actions) {
push_values(arg_action, "a", "-", settings.command_line_options[name]);
}
for (Action conf_action : conf_actions) {
bool use_section = conf_action == SECTION_SET || conf_action == SECTION_NEGATE;
push_values(conf_action, "c", use_section ? network + "." : "",
settings.ro_config[use_section ? network : ""][name]);
}
desc += " || ";
desc += GetSetting(settings, network, name, ignore_default_section_config, /* get_chain_name= */ false).write();
desc += " |";
for (const auto& s : GetSettingsList(settings, network, name, ignore_default_section_config)) {
desc += " ";
desc += s.write();
}
desc += " |";
if (OnlyHasDefaultSectionSetting(settings, network, name)) desc += " ignored";
desc += "\n";
out_sha.Write((const unsigned char*)desc.data(), desc.size());
if (out_file) {
BOOST_REQUIRE(fwrite(desc.data(), 1, desc.size(), out_file) == desc.size());
}
});
if (out_file) {
if (fclose(out_file)) throw std::system_error(errno, std::generic_category(), "fclose failed");
out_file = nullptr;
}
unsigned char out_sha_bytes[CSHA256::OUTPUT_SIZE];
out_sha.Finalize(out_sha_bytes);
std::string out_sha_hex = HexStr(std::begin(out_sha_bytes), std::end(out_sha_bytes));
// If check below fails, should manually dump the results with:
//
// SETTINGS_MERGE_TEST_OUT=results.txt ./test_bitcoin --run_test=settings_tests/Merge
//
// And verify diff against previous results to make sure the changes are expected.
//
// Results file is formatted like:
//
// <input> || GetSetting() | GetSettingsList() | OnlyHasDefaultSectionSetting()
BOOST_CHECK_EQUAL(out_sha_hex, "79db02d74e3e193196541b67c068b40ebd0c124a24b3ecbe9cbf7e85b1c4ba7a");
}
BOOST_AUTO_TEST_SUITE_END()

91
src/test/util_tests.cpp
View File

@@ -17,6 +17,7 @@
#include <stdint.h>
#include <thread>
#include <univalue.h>
#include <utility>
#include <vector>
#ifndef WIN32
@@ -166,14 +167,12 @@ BOOST_AUTO_TEST_CASE(util_FormatISO8601Date)
struct TestArgsManager : public ArgsManager
{
TestArgsManager() { m_network_only_args.clear(); }
std::map<std::string, std::vector<std::string> >& GetOverrideArgs() { return m_override_args; }
std::map<std::string, std::vector<std::string> >& GetConfigArgs() { return m_config_args; }
void ReadConfigString(const std::string str_config)
{
std::istringstream streamConfig(str_config);
{
LOCK(cs_args);
m_config_args.clear();
m_settings.ro_config.clear();
m_config_sections.clear();
}
std::string error;
@@ -193,6 +192,7 @@ struct TestArgsManager : public ArgsManager
using ArgsManager::ReadConfigStream;
using ArgsManager::cs_args;
using ArgsManager::m_network;
using ArgsManager::m_settings;
};
BOOST_AUTO_TEST_CASE(util_ParseParameters)
@@ -206,28 +206,29 @@ BOOST_AUTO_TEST_CASE(util_ParseParameters)
const char *argv_test[] = {"-ignored", "-a", "-b", "-ccc=argument", "-ccc=multiple", "f", "-d=e"};
std::string error;
LOCK(testArgs.cs_args);
testArgs.SetupArgs({a, b, ccc, d});
BOOST_CHECK(testArgs.ParseParameters(0, (char**)argv_test, error));
BOOST_CHECK(testArgs.GetOverrideArgs().empty() && testArgs.GetConfigArgs().empty());
BOOST_CHECK(testArgs.m_settings.command_line_options.empty() && testArgs.m_settings.ro_config.empty());
BOOST_CHECK(testArgs.ParseParameters(1, (char**)argv_test, error));
BOOST_CHECK(testArgs.GetOverrideArgs().empty() && testArgs.GetConfigArgs().empty());
BOOST_CHECK(testArgs.m_settings.command_line_options.empty() && testArgs.m_settings.ro_config.empty());
BOOST_CHECK(testArgs.ParseParameters(7, (char**)argv_test, error));
// expectation: -ignored is ignored (program name argument),
// -a, -b and -ccc end up in map, -d ignored because it is after
// a non-option argument (non-GNU option parsing)
BOOST_CHECK(testArgs.GetOverrideArgs().size() == 3 && testArgs.GetConfigArgs().empty());
BOOST_CHECK(testArgs.m_settings.command_line_options.size() == 3 && testArgs.m_settings.ro_config.empty());
BOOST_CHECK(testArgs.IsArgSet("-a") && testArgs.IsArgSet("-b") && testArgs.IsArgSet("-ccc")
&& !testArgs.IsArgSet("f") && !testArgs.IsArgSet("-d"));
BOOST_CHECK(testArgs.GetOverrideArgs().count("-a") && testArgs.GetOverrideArgs().count("-b") && testArgs.GetOverrideArgs().count("-ccc")
&& !testArgs.GetOverrideArgs().count("f") && !testArgs.GetOverrideArgs().count("-d"));
BOOST_CHECK(testArgs.m_settings.command_line_options.count("a") && testArgs.m_settings.command_line_options.count("b") && testArgs.m_settings.command_line_options.count("ccc")
&& !testArgs.m_settings.command_line_options.count("f") && !testArgs.m_settings.command_line_options.count("d"));
BOOST_CHECK(testArgs.GetOverrideArgs()["-a"].size() == 1);
BOOST_CHECK(testArgs.GetOverrideArgs()["-a"].front() == "");
BOOST_CHECK(testArgs.GetOverrideArgs()["-ccc"].size() == 2);
BOOST_CHECK(testArgs.GetOverrideArgs()["-ccc"].front() == "argument");
BOOST_CHECK(testArgs.GetOverrideArgs()["-ccc"].back() == "multiple");
BOOST_CHECK(testArgs.m_settings.command_line_options["a"].size() == 1);
BOOST_CHECK(testArgs.m_settings.command_line_options["a"].front().get_str() == "");
BOOST_CHECK(testArgs.m_settings.command_line_options["ccc"].size() == 2);
BOOST_CHECK(testArgs.m_settings.command_line_options["ccc"].front().get_str() == "argument");
BOOST_CHECK(testArgs.m_settings.command_line_options["ccc"].back().get_str() == "multiple");
BOOST_CHECK(testArgs.GetArgs("-ccc").size() == 2);
}
@@ -298,6 +299,7 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg)
const char *argv_test[] = {
"ignored", "-a", "-nob", "-c=0", "-d=1", "-e=false", "-f=true"};
std::string error;
LOCK(testArgs.cs_args);
testArgs.SetupArgs({a, b, c, d, e, f});
BOOST_CHECK(testArgs.ParseParameters(7, (char**)argv_test, error));
@@ -306,8 +308,8 @@ BOOST_AUTO_TEST_CASE(util_GetBoolArg)
BOOST_CHECK(testArgs.IsArgSet({'-', opt}) || !opt);
// Nothing else should be in the map
BOOST_CHECK(testArgs.GetOverrideArgs().size() == 6 &&
testArgs.GetConfigArgs().empty());
BOOST_CHECK(testArgs.m_settings.command_line_options.size() == 6 &&
testArgs.m_settings.ro_config.empty());
// The -no prefix should get stripped on the way in.
BOOST_CHECK(!testArgs.IsArgSet("-nob"));
@@ -403,6 +405,7 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
"iii=2\n";
TestArgsManager test_args;
LOCK(test_args.cs_args);
const auto a = std::make_pair("-a", ArgsManager::ALLOW_BOOL);
const auto b = std::make_pair("-b", ArgsManager::ALLOW_BOOL);
const auto ccc = std::make_pair("-ccc", ArgsManager::ALLOW_STRING);
@@ -419,22 +422,25 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
// expectation: a, b, ccc, d, fff, ggg, h, i end up in map
// so do sec1.ccc, sec1.d, sec1.h, sec2.ccc, sec2.iii
BOOST_CHECK(test_args.GetOverrideArgs().empty());
BOOST_CHECK(test_args.GetConfigArgs().size() == 13);
BOOST_CHECK(test_args.m_settings.command_line_options.empty());
BOOST_CHECK(test_args.m_settings.ro_config.size() == 3);
BOOST_CHECK(test_args.m_settings.ro_config[""].size() == 8);
BOOST_CHECK(test_args.m_settings.ro_config["sec1"].size() == 3);
BOOST_CHECK(test_args.m_settings.ro_config["sec2"].size() == 2);
BOOST_CHECK(test_args.GetConfigArgs().count("-a")
&& test_args.GetConfigArgs().count("-b")
&& test_args.GetConfigArgs().count("-ccc")
&& test_args.GetConfigArgs().count("-d")
&& test_args.GetConfigArgs().count("-fff")
&& test_args.GetConfigArgs().count("-ggg")
&& test_args.GetConfigArgs().count("-h")
&& test_args.GetConfigArgs().count("-i")
BOOST_CHECK(test_args.m_settings.ro_config[""].count("a")
&& test_args.m_settings.ro_config[""].count("b")
&& test_args.m_settings.ro_config[""].count("ccc")
&& test_args.m_settings.ro_config[""].count("d")
&& test_args.m_settings.ro_config[""].count("fff")
&& test_args.m_settings.ro_config[""].count("ggg")
&& test_args.m_settings.ro_config[""].count("h")
&& test_args.m_settings.ro_config[""].count("i")
);
BOOST_CHECK(test_args.GetConfigArgs().count("-sec1.ccc")
&& test_args.GetConfigArgs().count("-sec1.h")
&& test_args.GetConfigArgs().count("-sec2.ccc")
&& test_args.GetConfigArgs().count("-sec2.iii")
BOOST_CHECK(test_args.m_settings.ro_config["sec1"].count("ccc")
&& test_args.m_settings.ro_config["sec1"].count("h")
&& test_args.m_settings.ro_config["sec2"].count("ccc")
&& test_args.m_settings.ro_config["sec2"].count("iii")
);
BOOST_CHECK(test_args.IsArgSet("-a")
@@ -573,24 +579,25 @@ BOOST_AUTO_TEST_CASE(util_ReadConfigStream)
BOOST_AUTO_TEST_CASE(util_GetArg)
{
TestArgsManager testArgs;
testArgs.GetOverrideArgs().clear();
testArgs.GetOverrideArgs()["strtest1"] = {"string..."};
LOCK(testArgs.cs_args);
testArgs.m_settings.command_line_options.clear();
testArgs.m_settings.command_line_options["strtest1"] = {"string..."};
// strtest2 undefined on purpose
testArgs.GetOverrideArgs()["inttest1"] = {"12345"};
testArgs.GetOverrideArgs()["inttest2"] = {"81985529216486895"};
testArgs.m_settings.command_line_options["inttest1"] = {"12345"};
testArgs.m_settings.command_line_options["inttest2"] = {"81985529216486895"};
// inttest3 undefined on purpose
testArgs.GetOverrideArgs()["booltest1"] = {""};
testArgs.m_settings.command_line_options["booltest1"] = {""};
// booltest2 undefined on purpose
testArgs.GetOverrideArgs()["booltest3"] = {"0"};
testArgs.GetOverrideArgs()["booltest4"] = {"1"};
testArgs.m_settings.command_line_options["booltest3"] = {"0"};
testArgs.m_settings.command_line_options["booltest4"] = {"1"};
// priorities
testArgs.GetOverrideArgs()["pritest1"] = {"a", "b"};
testArgs.GetConfigArgs()["pritest2"] = {"a", "b"};
testArgs.GetOverrideArgs()["pritest3"] = {"a"};
testArgs.GetConfigArgs()["pritest3"] = {"b"};
testArgs.GetOverrideArgs()["pritest4"] = {"a","b"};
testArgs.GetConfigArgs()["pritest4"] = {"c","d"};
testArgs.m_settings.command_line_options["pritest1"] = {"a", "b"};
testArgs.m_settings.ro_config[""]["pritest2"] = {"a", "b"};
testArgs.m_settings.command_line_options["pritest3"] = {"a"};
testArgs.m_settings.ro_config[""]["pritest3"] = {"b"};
testArgs.m_settings.command_line_options["pritest4"] = {"a","b"};
testArgs.m_settings.ro_config[""]["pritest4"] = {"c","d"};
BOOST_CHECK_EQUAL(testArgs.GetArg("strtest1", "default"), "string...");
BOOST_CHECK_EQUAL(testArgs.GetArg("strtest2", "default"), "default");