Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 3 commits into from

Conversation

Projects
None yet
6 participants
@Empact
Copy link
Member

commented Jul 12, 2018

By moving gArgs references out of chainparamsbase.cpp.

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

@Empact Empact force-pushed the Empact:chainparamsbase-circ branch Jul 12, 2018

@DrahtBot

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

Note to reviewers: This pull request conflicts with the following ones:
  • #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

This comment has been minimized.

Copy link
Contributor

commented Jul 12, 2018

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

#include <chainparamsbase.h>

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

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jul 12, 2018

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 Empact changed the title Fix the chainparamsbase -> util -> chainparamsbase circular dependency [moveonly] Fix the chainparamsbase -> util circular dependency Jul 13, 2018

@Empact Empact force-pushed the Empact:chainparamsbase-circ branch Jul 17, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2018

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

MarcoFalke added a commit that referenced this pull request Jul 17, 2018

Merge #13690: [moveonly] Extract MakeUnique into utilmemory.h
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 Empact force-pushed the Empact:chainparamsbase-circ branch 2 times, most recently Jul 17, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2018

Rebased for #13690

@Empact Empact force-pushed the Empact:chainparamsbase-circ branch Jul 17, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jul 17, 2018

Ran clang-format

@Empact Empact force-pushed the Empact:chainparamsbase-circ branch Jul 20, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2018

Rebased for #13695

@sipa

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

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

This comment has been minimized.

Copy link
Member

commented Jul 20, 2018

What about moving argsmanager to a separate module?

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jul 20, 2018

I'll look into that, thanks!

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.

@Empact Empact force-pushed the Empact:chainparamsbase-circ branch 3 times, most recently Jul 28, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2018

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 DrahtBot removed the Needs rebase label Jul 28, 2018

Remove remaining uses of CreateBaseChainParams
What remains of CBaseChainParams is just a namespace for the network name
strings.

@Empact Empact force-pushed the Empact:chainparamsbase-circ branch Jul 28, 2018

@Empact Empact force-pushed the Empact:chainparamsbase-circ branch to 0579015 Jul 28, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Jul 28, 2018

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

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.

@Empact Empact force-pushed the Empact:chainparamsbase-circ branch from 0579015 to 9847689 Aug 24, 2018

@Empact

This comment has been minimized.

Copy link
Member Author

commented Aug 24, 2018

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.