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

pull Empact wants to merge 3 commits into bitcoin:master from Empact:chainparamsbase-circ changing 9 files +53 −76
  1. 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

  2. fanquake added the label Refactoring on Jul 12, 2018
  3. Empact force-pushed on Jul 12, 2018
  4. 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.

  5. 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.

  6. 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.
  7. Empact renamed this:
    Fix the chainparamsbase -> util -> chainparamsbase circular dependency
    [moveonly] Fix the chainparamsbase -> util circular dependency
    on Jul 13, 2018
  8. Empact force-pushed on Jul 17, 2018
  9. 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?
  10. MarcoFalke referenced this in commit c01ab133d7 on Jul 17, 2018
  11. Empact force-pushed on Jul 17, 2018
  12. Empact force-pushed on Jul 17, 2018
  13. Empact commented at 7:16 pm on July 17, 2018: member
    Rebased for #13690
  14. Empact force-pushed on Jul 17, 2018
  15. Empact commented at 8:25 pm on July 17, 2018: member
    Ran clang-format
  16. Empact force-pushed on Jul 20, 2018
  17. Empact commented at 4:38 pm on July 20, 2018: member
    Rebased for #13695
  18. 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.

  19. MarcoFalke commented at 7:21 pm on July 20, 2018: member
    What about moving argsmanager to a separate module?
  20. Empact commented at 8:16 pm on July 20, 2018: member
    I’ll look into that, thanks!
  21. DrahtBot added the label Needs rebase on Jul 24, 2018
  22. 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
  23. Empact force-pushed on Jul 28, 2018
  24. Empact force-pushed on Jul 28, 2018
  25. Empact force-pushed on Jul 28, 2018
  26. 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.

  27. DrahtBot removed the label Needs rebase on Jul 28, 2018
  28. Remove remaining uses of CreateBaseChainParams
    What remains of CBaseChainParams is just a namespace for the network name
    strings.
    36e5e692a8
  29. Empact force-pushed on Jul 28, 2018
  30. Empact force-pushed on Jul 28, 2018
  31. 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
  32. MarcoFalke renamed this:
    [moveonly] Fix the chainparamsbase -> util circular dependency
    [refactor] Fix the chainparamsbase -> util circular dependency
    on Jul 28, 2018
  33. 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
  34. Empact force-pushed on Aug 24, 2018
  35. Empact commented at 8:41 am on August 24, 2018: member
    @sipa you’re right, that’s much cleaner. Closing in favor of #14045
  36. Empact closed this on Aug 24, 2018

  37. PastaPastaPasta referenced this in commit fe8ae1c2bc on Jul 19, 2020
  38. PastaPastaPasta referenced this in commit 5f32f89acc on Jul 24, 2020
  39. PastaPastaPasta referenced this in commit 07bb09cc6b on Jul 27, 2020
  40. UdjinM6 referenced this in commit 0a45392c15 on Jul 27, 2020
  41. UdjinM6 referenced this in commit 9903dcd2aa on Jul 27, 2020
  42. MarcoFalke locked this on Sep 8, 2021

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-11-17 09:12 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me