refactor: Fix the chainparamsbase -> util circular dependency #14045

Empact · 2018-08-24T08:40:16Z

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.

DrahtBot · 2018-08-24T08:42:09Z

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#16508 (Disallow double negation syntax (-nofoo=0) for options by hebasto)
#16411 (Signet support by kallewoof)
#16097 (Refactor: Add Flags enum to ArgsManager class by hebasto)
#16060 (Bury bip9 deployments by jnewbery)
#15934 (Separate settings merging from parsing by ryanofsky)
#8994 (Testchains: Introduce custom chain whose constructor... by jtimon)

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.

practicalswift · 2018-08-24T09:16:23Z

Concept ACK

Let’s get rid of all circular dependencies! :-)

domob1812

utACK d1e653c2e028c3342cdae4be81e423f96db89214 with a few style nits.

src/chainparamsbase.cpp

src/chainparamsbase.h

Empact · 2018-08-24T23:55:54Z

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.

Empact · 2018-08-25T00:58:49Z

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.

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.

Empact · 2019-06-05T11:58:06Z

Rebased

ryanofsky · 2019-06-06T10:45:05Z

src/test/util_tests.cpp

@@ -774,7 +780,7 @@ BOOST_FIXTURE_TEST_CASE(util_ArgsMerge, ArgsMergeTestingSetup)
    // Results file is formatted like:
    //
    //   <input> || <IsArgSet/IsArgNegated/GetArg output> | <GetArgs output> | <GetUnsuitable output>
-    BOOST_CHECK_EQUAL(out_sha_hex, "b835eef5977d69114eb039a976201f8c7121f34fe2b7ea2b73cafb516e5c9dc8");
+    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.

maflcko · 2019-06-06T13:47:10Z

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

DrahtBot · 2019-08-02T16:27:00Z

Needs rebase

DrahtBot · 2020-07-14T07:37:04Z

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.

Empact mentioned this pull request Aug 24, 2018

[refactor] Fix the chainparamsbase -> util circular dependency #13639

Closed

Empact force-pushed the chain-params-base-circ branch from b77e9ba to d1e653c Compare August 24, 2018 08:59

Empact changed the title ~~Break the chainparamsbase -> util circular dependency~~ refactor: Fix the chainparamsbase -> util circular dependency Aug 24, 2018

DrahtBot mentioned this pull request Aug 24, 2018

refactor: Removal of circular dependency between index/txindex, validation and index/base #13942

Closed

DrahtBot mentioned this pull request Aug 24, 2018

util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #13815

Merged

domob1812 approved these changes Aug 24, 2018

View reviewed changes

src/chainparamsbase.cpp Outdated Show resolved Hide resolved

src/chainparamsbase.h Show resolved Hide resolved

Empact force-pushed the chain-params-base-circ branch 4 times, most recently from d5c2ab0 to 10652ff Compare August 24, 2018 23:54

Empact force-pushed the chain-params-base-circ branch 2 times, most recently from 27f8ab9 to eca5a9f Compare August 25, 2018 00:57

Empact force-pushed the chain-params-base-circ branch from eca5a9f to d5c2ab0 Compare August 26, 2018 21:17

DrahtBot added the Needs rebase label Aug 30, 2018

Empact force-pushed the chain-params-base-circ branch from d5c2ab0 to 613c28d Compare September 1, 2018 04:17

DrahtBot removed the Needs rebase label Sep 1, 2018

DrahtBot mentioned this pull request Sep 1, 2018

Minor style enhancement in documentation #14091

Closed

Empact force-pushed the chain-params-base-circ branch 2 times, most recently from 85191a6 to 03f21f0 Compare September 2, 2018 07:20

This was referenced Sep 6, 2018

fix various uses of inline #14158

Closed

Don't edit Chainparams after initialization #13311

Merged

DrahtBot mentioned this pull request Sep 24, 2018

Use non-throwing type-safe ChainType where possible #14309

Closed

DrahtBot added the Needs rebase label Sep 24, 2018

Empact force-pushed the chain-params-base-circ branch from 03f21f0 to d04063b Compare September 25, 2018 03:05

DrahtBot removed the Needs rebase label Sep 25, 2018

Empact force-pushed the chain-params-base-circ branch from d04063b to e9035ea Compare September 25, 2018 16:16

DrahtBot removed the Needs rebase label Nov 6, 2018

Empact changed the title ~~refactor: Fix the chainparamsbase -> util circular dependency~~ WIP: refactor: Fix the chainparamsbase -> util circular dependency Nov 12, 2018

DrahtBot added the Needs rebase label Nov 15, 2018

Empact force-pushed the chain-params-base-circ branch from 55c3b8c to 8f3b025 Compare December 13, 2018 02:03

DrahtBot removed the Needs rebase label Dec 13, 2018

Empact force-pushed the chain-params-base-circ branch from 8f3b025 to 60ee8a4 Compare December 14, 2018 09:08

Empact changed the title ~~WIP: refactor: Fix the chainparamsbase -> util circular dependency~~ refactor: Fix the chainparamsbase -> util circular dependency Dec 14, 2018

Empact force-pushed the chain-params-base-circ branch 3 times, most recently from 91f62a7 to fa190c6 Compare December 18, 2018 03:38

Empact force-pushed the chain-params-base-circ branch from fa190c6 to 0c0f506 Compare January 7, 2019 19:39

fanquake added the Refactoring label Jan 8, 2019

DrahtBot added the Needs rebase label Feb 21, 2019

Empact force-pushed the chain-params-base-circ branch from 0c0f506 to 7588c1b Compare March 4, 2019 09:37

DrahtBot removed the Needs rebase label Mar 4, 2019

Empact force-pushed the chain-params-base-circ branch 2 times, most recently from 96b2d9b to ab5b67e Compare March 9, 2019 16:23

DrahtBot added the Needs rebase label Apr 19, 2019

Empact added 2 commits June 5, 2019 12:55

Move SetupChainParamsBaseOptions to util

3ced0b5

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.

Empact force-pushed the chain-params-base-circ branch from ab5b67e to 764106e Compare June 5, 2019 11:57

DrahtBot removed the Needs rebase label Jun 5, 2019

ryanofsky reviewed Jun 6, 2019

View reviewed changes

DrahtBot added the Needs rebase label Aug 2, 2019

DrahtBot closed this Jul 14, 2020

bitcoin locked as resolved and limited conversation to collaborators Feb 15, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

refactor: Fix the chainparamsbase -> util circular dependency #14045

refactor: Fix the chainparamsbase -> util circular dependency #14045

Uh oh!

Empact commented Aug 24, 2018 •

edited

Loading

Uh oh!

DrahtBot commented Aug 24, 2018 •

edited

Loading

Uh oh!

practicalswift commented Aug 24, 2018

Uh oh!

domob1812 left a comment

Uh oh!

Uh oh!

Uh oh!

Empact commented Aug 24, 2018 •

edited

Loading

Uh oh!

Empact commented Aug 25, 2018

Uh oh!

Empact commented Jun 5, 2019

Uh oh!

ryanofsky Jun 6, 2019

Uh oh!

maflcko commented Jun 6, 2019

Uh oh!

DrahtBot commented Aug 2, 2019

Uh oh!

DrahtBot commented Jul 14, 2020

Uh oh!

Uh oh!

refactor: Fix the chainparamsbase -> util circular dependency #14045

refactor: Fix the chainparamsbase -> util circular dependency #14045

Uh oh!

Conversation

Empact commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

DrahtBot commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

practicalswift commented Aug 24, 2018

Uh oh!

domob1812 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Empact commented Aug 24, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Empact commented Aug 25, 2018

Uh oh!

Empact commented Jun 5, 2019

Uh oh!

ryanofsky Jun 6, 2019

Choose a reason for hiding this comment

Uh oh!

maflcko commented Jun 6, 2019

Uh oh!

DrahtBot commented Aug 2, 2019

Uh oh!

DrahtBot commented Jul 14, 2020

Uh oh!

Uh oh!

Empact commented Aug 24, 2018 •

edited

Loading

DrahtBot commented Aug 24, 2018 •

edited

Loading

Empact commented Aug 24, 2018 •

edited

Loading