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.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--174a7506f384e20aa4161008e828411d-->
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;
Small nit: Smart pointers (std::unique_ptr in this case) allow direct comparison against nullptr. So there's no need to call get.
Fixed 👍
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();
Nit: This can be const, I assume.
Assume you mean const member, but it's not a member function, so no object to be const on.
Oh yes indeed - I somehow thought this was a member.
utACK d1e653c2e028c3342cdae4be81e423f96db89214 with a few style nits.
Fixed tests. ~Note I had to guard 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.~
On second thought, removed the --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()
Bike-shed: renamed to SetupChainParamsOptions, these are all chainparams options there are.
Done 👍
Didn't look much at the tests, but the code looks good to me, utACK a590d6d932c3645b138f0c1726cfa78ecd0b434d
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.
Rebased
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:
-net=test wallet=c1 wallet=c2 || unset | ignored -wallet
+net=test wallet=c1 wallet=c2 || c1 | c1 c2
...
-net=test wallet=c1 wallet=c2 || unset | ignored -wallet
+net=test wallet=c1 wallet=c2 || c1 | c1 c2
...
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
<!--cf906140f33d8803c4a75a2196329ecb-->Needs rebase
There hasn't been much activity lately and the patch still needs rebase, so I am closing this for now. Please let me know when you want to continue working on this, so the pull request can be re-opened.