[refactor] Fix the chainparamsbase -> util circular dependency

Empact commented at 5:53 am on July 12, 2018: member

By moving gArgs references out of chainparamsbase.cpp.

Note both chainparams.cpp, bitcoin-cli.cpp, and all callers of SetupChainParamsBaseOptions include util.h

fanquake added the label Refactoring on Jul 12, 2018

Empact force-pushed on Jul 12, 2018

DrahtBot commented at 11:30 am on July 12, 2018: member

#14045 (Break the chainparamsbase -> util circular dependency by Empact)
#14013 ([doc] Add new regtest ports in man following #10825 ports reattributions by ariard)
#13942 (refactor: Removal of circular dependency between index/txindex, validation and index/base by mgrychow)
#13815 (util: Add [[nodiscard]] to all {Decode,Parse}… functions returning bool by practicalswift)
#13621 (Check for datadir after the config files were read by Flowdalic)
#13311 (Don’t edit Chainparams after initialization 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.

l2a5b1 commented at 3:45 pm on July 12, 2018: contributor

+1 for going after another dependency cycle :)

Extracting MakeUnique seems like a good thing to do.

Does it make sense to refactor the dependency of util on chainparamsbase? https://github.com/bitcoin/bitcoin/blob/9b638c7ce1b93c996b49f02c0bcefb25b6200d8e/src/util.cpp#L8 This relationship seems the crux to the circular dependency to me. Imoutil should have as little dependencies as possible.

Empact commented at 4:21 pm on July 12, 2018: member

Agree on minimizing util, but seems removing chainparamsbase would be a bigger job, since ArgsManager uses CBaseChainParams, and ArgsManager is the bulk of util.

Empact renamed this:
~~Fix the chainparamsbase -> util -> chainparamsbase circular dependency~~
[moveonly] Fix the chainparamsbase -> util circular dependency
on Jul 13, 2018

Empact force-pushed on Jul 17, 2018

Empact commented at 5:50 pm on July 17, 2018: member

Dropped the third commit and extracted #13690 for easier review. ~~Maybe start with that?~~

MarcoFalke referenced this in commit c01ab133d7 on Jul 17, 2018

Empact force-pushed on Jul 17, 2018

Empact commented at 7:16 pm on July 17, 2018: member

Rebased for #13690

Empact force-pushed on Jul 17, 2018

Empact commented at 8:25 pm on July 17, 2018: member

Ran clang-format

Empact force-pushed on Jul 20, 2018

Empact commented at 4:38 pm on July 20, 2018: member

Rebased for #13695

sipa commented at 6:09 pm on July 20, 2018: member

I don’t think moving the arguments for chain selection to util is the right solution here.

Why does util depend on knowing something about chain parameters anyway? It’s supposed to be a bunch of utility functions.

MarcoFalke commented at 7:21 pm on July 20, 2018: member

What about moving argsmanager to a separate module?

Empact commented at 8:16 pm on July 20, 2018: member

I’ll look into that, thanks!

DrahtBot added the label Needs rebase on Jul 24, 2018

Prefer gArgs.m_network over the redundant globalChainBaseParams

These two were being set together in all cases. Using just one removes the
internal reliance of chainparamsbase on gArgs in this case.

Instead expose GetRPCPort and GetDataDir methods, which can be called with
the m_network name stored by gArgs.

2bcc38e0d9

Empact force-pushed on Jul 28, 2018

Empact commented at 7:28 am on July 28, 2018: member

Ok I looked deeper into this and moved to a more comprehensive approach. The story is, there exist an m_network attribute in ArgsManager, and a globalChainBaseParams variable that drives BaseParams(). These are functionally redundant based on the fact that they are always set together. Given that the base params are only two: rpc port and data dir name, we can simply pass the m_network value to a method that returns the port or dir name relative to that network.

This is functionally equivalent and simpler in implementation than the prior alternative. While chainparams has a justification for doing its setup once (the voluminous setup), the mapping for chainparamsbase is trivial.

DrahtBot removed the label Needs rebase on Jul 28, 2018

Remove remaining uses of CreateBaseChainParams

What remains of CBaseChainParams is just a namespace for the network name
strings.

36e5e692a8

Empact force-pushed on Jul 28, 2018

Empact commented at 8:57 am on July 28, 2018: member

A possible follow-up - replacing the string-based handling with an enum class: https://github.com/Empact/bitcoin/compare/chainparamsbase-circ...Empact:chain-enum-class?expand=1

MarcoFalke renamed this:
~~[moveonly] Fix the chainparamsbase -> util circular dependency~~
[refactor] Fix the chainparamsbase -> util circular dependency
on Jul 28, 2018

Move SetupChainParamsBaseOptions to util

These options are intimately tied to ArgsManager::GetChainName, so their
presence here makes sense, and moving them removes the final cause for a
circular dependency between util and chainparamsbase.

9847689005

Empact force-pushed on Aug 24, 2018

Empact commented at 8:41 am on August 24, 2018: member

@sipa you’re right, that’s much cleaner. Closing in favor of #14045

Empact closed this on Aug 24, 2018

PastaPastaPasta referenced this in commit fe8ae1c2bc on Jul 19, 2020

PastaPastaPasta referenced this in commit 5f32f89acc on Jul 24, 2020

PastaPastaPasta referenced this in commit 07bb09cc6b on Jul 27, 2020

UdjinM6 referenced this in commit 0a45392c15 on Jul 27, 2020

UdjinM6 referenced this in commit 9903dcd2aa on Jul 27, 2020

MarcoFalke locked this on Sep 8, 2021

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