forked from bitpay/bitcoin
-
Notifications
You must be signed in to change notification settings - Fork 0
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
Merge bitcoin#15934: Merge settings one place instead of five places
083c954 Add settings_tests (Russell Yanofsky) 7f40528 Deduplicate settings merge code (Russell Yanofsky) 9dcb952 Add util::Settings struct and helper functions. (Russell Yanofsky) e2e37cf Remove includeconf nested scope (Russell Yanofsky) 5a84aa8 Rename includeconf variables for clarity (Russell Yanofsky) dc8e1e7 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`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L626-L822) and [`util_ChainMerge`](https://github.com/bitcoin/bitcoin/blob/deb2327b435925c6a39ca654a79283b8eb6aeb86/src/test/util_tests.cpp#L843-L924) tests added in bitcoin#15869 and bitcoin#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 70675c3 from bitcoin#15935). - Separates parsing of settings from merging of settings, and deduplicates merging code so it doesn't happen five different places ([GetArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L221-L244), [GetNetBoolArg](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L255-L261), [GetArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L460-L467), [IsArgNegated](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/src/util/system.cpp#L482-L491), [GetUnsuitableSectionOnlyArgs](https://github.com/bitcoin/bitcoin/blob/c459c5f70176928adcee4935813a2dbe7f4dbd51/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](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/system.cpp#L323-L326) and [more ignored arguments](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L67-L72), and config file [reverse precedence](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L61-L65), [inconsistently applied top-level settings](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/src/util/settings.cpp#L55-L59), and [zombie values](https://github.com/ryanofsky/bitcoin/blob/69d44f3cc75a68d404ca0e1ca2b4831fd2bac4bb/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: * 70675c3 from bitcoin#15935 – _Add \<datadir>/settings.json persistent settings storage_ * 04c80c4 from bitcoin#15937 – _Add loadwallet and createwallet RPC load_on_startup options_ ACKs for top commit: ariard: ACK 083c954 jnewbery: ACK 083c954 jamesob: ACK 083c954 Tree-SHA512: 5d106746a44d64d3963c4ef3f4a2fa668a4bedcc9018d3ea12c86beae2fda48a0b036241665837f68685712366f70f2e1faba84d193fa1f456013503097b7659
- Loading branch information
Showing
8 changed files
with
613 additions
and
286 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -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() |
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Oops, something went wrong.