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

Empact · 2018-07-12T05:53:00Z

By moving gArgs references out of chainparamsbase.cpp.

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

DrahtBot · 2018-07-12T11:30:22Z

Note to reviewers: This pull request conflicts with the following ones:

refactor: Fix the chainparamsbase -> util circular dependency #14045 (Break the chainparamsbase -> util circular dependency by Empact)
[doc] Add new regtest ports in man following #10825 ports reattributions #14013 ([doc] Add new regtest ports in man following net: set regtest JSON-RPC port to 18443 to avoid conflict with testnet 18332 #10825 ports reattributions by ariard)
refactor: Removal of circular dependency between index/txindex, validation and index/base #13942 (refactor: Removal of circular dependency between index/txindex, validation and index/base by mgrychow)
util: Add [[nodiscard]] to all {Decode,Parse}[...](...) functions returning bool #13815 (util: Add [[nodiscard]] to all {Decode,Parse}... functions returning bool by practicalswift)
Check for datadir after the config files were read #13621 (Check for datadir after the config files were read by Flowdalic)
Don't edit Chainparams after initialization #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 · 2018-07-12T15:45:53Z

+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?

bitcoin/src/util.cpp

Line 8 in 9b638c7

#include <chainparamsbase.h>

This relationship seems the crux to the circular dependency to me. Imoutil should have as little dependencies as possible.

Empact · 2018-07-12T16:21:45Z

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 · 2018-07-17T17:50:32Z

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

33539cb Extract MakeUnique into utilmemory.h (Ben Woosley) Pull request description: And use it to reduce chainparamsbase.cpp's and remove interfaces/handler.cpp's reliance on util.h This is a step toward fixing the chainparamsbase -> util circular dependency. Confirmed no need for the util.h include via iwyu and visual inspection. Extracted from #13639 for easier review. Tree-SHA512: 61cbf9eafb68c3e3706d21c70aeb0586a85364dad32cc49c2d40e963ac3b2c44424ad1522788a0a6b2a689fd9294ebce4482a392ceb88a94eabe09a84f070ce4

Empact · 2018-07-17T19:16:47Z

Rebased for #13690

Empact · 2018-07-17T20:25:35Z

Ran clang-format

Empact · 2018-07-20T16:38:14Z

Rebased for #13695

sipa · 2018-07-20T18:09:13Z

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.

maflcko · 2018-07-20T19:21:35Z

What about moving argsmanager to a separate module?

Empact · 2018-07-20T20:16:33Z

I'll look into that, thanks!

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.

Empact · 2018-07-28T07:28:19Z

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.

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

Empact · 2018-07-28T08:57:05Z

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

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.

Empact · 2018-08-24T08:41:02Z

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

33539cb Extract MakeUnique into utilmemory.h (Ben Woosley) Pull request description: And use it to reduce chainparamsbase.cpp's and remove interfaces/handler.cpp's reliance on util.h This is a step toward fixing the chainparamsbase -> util circular dependency. Confirmed no need for the util.h include via iwyu and visual inspection. Extracted from bitcoin#13639 for easier review. Tree-SHA512: 61cbf9eafb68c3e3706d21c70aeb0586a85364dad32cc49c2d40e963ac3b2c44424ad1522788a0a6b2a689fd9294ebce4482a392ceb88a94eabe09a84f070ce4

fanquake added the Refactoring label Jul 12, 2018

Empact force-pushed the chainparamsbase-circ branch from bbc6d2d to 6640590 Compare July 12, 2018 05:55

Empact changed the title ~~Fix the chainparamsbase -> util -> chainparamsbase circular dependency~~ [moveonly] Fix the chainparamsbase -> util circular dependency Jul 13, 2018

Empact force-pushed the chainparamsbase-circ branch from 6640590 to 3461ebd Compare July 17, 2018 17:45

Empact mentioned this pull request Jul 17, 2018

[moveonly] Extract MakeUnique into utilmemory.h #13690

Merged

Empact force-pushed the chainparamsbase-circ branch 2 times, most recently from 0504a02 to 6ebcfab Compare July 17, 2018 19:15

Empact force-pushed the chainparamsbase-circ branch from 6ebcfab to a216630 Compare July 17, 2018 20:25

Empact force-pushed the chainparamsbase-circ branch from a216630 to 73219c8 Compare July 20, 2018 16:37

DrahtBot mentioned this pull request Jul 22, 2018

bitcoin-cli: -stdinwalletpassphrase and non-echo stdin passwords #13716

Merged

DrahtBot added the Needs rebase label Jul 24, 2018

Empact force-pushed the chainparamsbase-circ branch 3 times, most recently from 253e66b to af5754e Compare July 28, 2018 07:16

DrahtBot removed the Needs rebase label Jul 28, 2018

Remove remaining uses of CreateBaseChainParams

36e5e69

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

Empact force-pushed the chainparamsbase-circ branch from af5754e to 5194e16 Compare July 28, 2018 08:02

Empact force-pushed the chainparamsbase-circ branch from 5194e16 to 0579015 Compare July 28, 2018 08:46

This was referenced Jul 28, 2018

Check for datadir after the config files were read #13621

Closed

[bugfix] Fix encoding issue for Windows #13426

Closed

maflcko changed the title ~~[moveonly] Fix the chainparamsbase -> util circular dependency~~ [refactor] Fix the chainparamsbase -> util circular dependency Jul 28, 2018

This was referenced Aug 20, 2018

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

Closed

[doc] Add new regtest ports in man following #10825 ports reattributions #14013

Merged

Move SetupChainParamsBaseOptions to util

9847689

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.

Empact force-pushed the chainparamsbase-circ branch from 0579015 to 9847689 Compare August 24, 2018 00:17

Empact closed this Aug 24, 2018

DrahtBot mentioned this pull request Aug 24, 2018

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

Closed

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

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

Uh oh!

Empact commented Jul 12, 2018 •

edited

Loading

Uh oh!

DrahtBot commented Jul 12, 2018 •

edited

Loading

Uh oh!

l2a5b1 commented Jul 12, 2018

Uh oh!

Empact commented Jul 12, 2018

Uh oh!

Empact commented Jul 17, 2018 •

edited

Loading

Uh oh!

Empact commented Jul 17, 2018

Uh oh!

Empact commented Jul 17, 2018

Uh oh!

Empact commented Jul 20, 2018

Uh oh!

sipa commented Jul 20, 2018

Uh oh!

maflcko commented Jul 20, 2018

Uh oh!

Empact commented Jul 20, 2018

Uh oh!

Empact commented Jul 28, 2018

Uh oh!

Empact commented Jul 28, 2018 •

edited

Loading

Uh oh!

Empact commented Aug 24, 2018

Uh oh!

Uh oh!

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

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

Uh oh!

Conversation

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

Uh oh!

Uh oh!

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

Uh oh!

Uh oh!

l2a5b1 commented Jul 12, 2018

Uh oh!

Empact commented Jul 12, 2018

Uh oh!

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

Uh oh!

Uh oh!

Empact commented Jul 17, 2018

Uh oh!

Empact commented Jul 17, 2018

Uh oh!

Empact commented Jul 20, 2018

Uh oh!

sipa commented Jul 20, 2018

Uh oh!

maflcko commented Jul 20, 2018

Uh oh!

Empact commented Jul 20, 2018

Uh oh!

Empact commented Jul 28, 2018

Uh oh!

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

Uh oh!

Uh oh!

Empact commented Aug 24, 2018

Uh oh!

Uh oh!

Empact commented Jul 12, 2018 •

edited

Loading

DrahtBot commented Jul 12, 2018 •

edited

Loading

Empact commented Jul 17, 2018 •

edited

Loading

Empact commented Jul 28, 2018 •

edited

Loading