By making BaseParams() responsible for holding the current chain name, and layering
ArgsManager atop it.
Without this both m_network of ArgsManager and globalChainBaseParams track the current chain, leading to circular dependencies between them.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
Reviewers, this pull request conflicts with the following ones:
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.
Concept ACK
Let’s get rid of all circular dependencies! :-)
37 throw std::runtime_error(strprintf("%s: Unknown chain %s.", __func__, chain));
38 }
39
40+bool BaseParamsSelected()
41+{
42+ return globalChainBaseParams.get() != nullptr;
std::unique_ptr in this case) allow direct comparison against nullptr. So there’s no need to call get.
42@@ -40,9 +43,9 @@ class CBaseChainParams
43 std::unique_ptr<CBaseChainParams> CreateBaseChainParams(const std::string& chain);
44
45 /**
46- *Set the arguments for chainparams
47+ * @return true if BaseParams have been selected, false if not
48 */
49-void SetupChainParamsBaseOptions();
50+bool BaseParamsSelected();
const, I assume.
const member, but it’s not a member function, so no object to be const on.
ArgsManagerHelper::NetworkArg calls with a test for a
leading - as NetworkArg asserts against the absence, while values without
- are tested for in util_tests. Could drop those tests instead, but this seems coherent.
--less test and asserted against --less options in the relevant calls. Note that --less options are skipped over in ParseParameters.
972@@ -978,6 +973,14 @@ std::string ArgsManager::GetChainName() const
973 return CBaseChainParams::MAIN;
974 }
975
976+void SetupChainParamsBaseOptions()
These options are intimately tied to ArgsManager, so their
presence here makes sense, and moving them removes a final cause for a
circular dependency between util and chainparamsbase.
And call into BaseParams from ArgsManager. Introduce BaseParamsSelected
as it's necessary to prevent access to BaseParams prior to selection.
This eliminates the chainparamsbase -> util -> chainparamsbase
circular dependency.
Note I had to guard ArgsManagerHelper::NetworkArg calls with a test for a
leading '-' as NetworkArg asserts against the alternative, while values without
- are tested for in the util_tests.
779@@ -774,7 +780,7 @@ BOOST_FIXTURE_TEST_CASE(util_ArgsMerge, ArgsMergeTestingSetup)
780 // Results file is formatted like:
781 //
782 // <input> || <IsArgSet/IsArgNegated/GetArg output> | <GetArgs output> | <GetUnsuitable output>
783- BOOST_CHECK_EQUAL(out_sha_hex, "b835eef5977d69114eb039a976201f8c7121f34fe2b7ea2b73cafb516e5c9dc8");
784+ BOOST_CHECK_EQUAL(out_sha_hex, "6e84c6b6a1702f5c2247d175e7e1ad76c342be58b95bf2b7ed7e271d38197a14");
Are the changes here expected? I don’t think they look right. It looks like wallet settings in the top-level of the config file (not in the test section) are now no longer ignored when testnet is enabled.
DIfferences in results.txt before and after this change look like:
0-net=test wallet=c1 wallet=c2 || unset | ignored -wallet
1+net=test wallet=c1 wallet=c2 || c1 | c1 c2
2...
3-net=test wallet=c1 wallet=c2 || unset | ignored -wallet
4+net=test wallet=c1 wallet=c2 || c1 | c1 c2
5...
If this is not intended, it might just be a bug in the test setup, like the parser.m_network = network; line above no longer having an effect, instead of a real bug.
I wrote a feedback a few months ago, not sure if it still applies:
Base params are always selected when the program is running. Only very early in the init sequence (when the chain was not selected) we’d have to (and should) skip the steps that involve the chain name. Imo this is better solved by passing a bool into IsArg*(..., bool net_specific=true), that is set to true by default but only in the few calls during init set to false. E.g. would have to pass it down in HelpRequested…
No strong opinion, though