refactor: Remove ::Params() global from CChainState #21789

pull MarcoFalke wants to merge 2 commits into bitcoin:master from MarcoFalke:2104-ChainstateParamsRefactor changing 10 files +149 −147
  1. MarcoFalke commented at 6:51 PM on April 27, 2021: member

    The ::Params() global is verbose and confusing. Also it makes tests a bit harder to write because they'd have to mock a global.

    Fix all issues by simply using a member variable that points to the right params.

    (Can be reviewed with --word-diff-regex=.)

  2. MarcoFalke force-pushed on Apr 27, 2021
  3. jamesob commented at 7:24 PM on April 27, 2021: member

    Concept ACK, will review soon

  4. DrahtBot added the label P2P on Apr 27, 2021
  5. DrahtBot added the label Refactoring on Apr 27, 2021
  6. DrahtBot added the label RPC/REST/ZMQ on Apr 27, 2021
  7. DrahtBot added the label Validation on Apr 27, 2021
  8. practicalswift commented at 8:42 PM on April 27, 2021: contributor

    Concept ACK

  9. fanquake commented at 3:00 AM on April 28, 2021: member

    Concept ACK

  10. MarcoFalke removed the label P2P on Apr 28, 2021
  11. MarcoFalke removed the label RPC/REST/ZMQ on Apr 28, 2021
  12. MarcoFalke force-pushed on Apr 28, 2021
  13. DrahtBot commented at 6:14 AM on April 28, 2021: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    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.

  14. in src/validation.h:598 in fa4b981b80 outdated 
     563 | @@ -564,6 +564,8 @@ class CChainState
 564 |      //! mempool that is kept in sync with the chain
 565 |      CTxMemPool& m_mempool;
 566 |  
 567 | +    const CChainParams& m_params;
    kiminuo commented at 6:18 AM on April 28, 2021:

    nit: This member variable looks like the only one without a Doxygen comment in this section. Maybe add one?

    Is protected better here than private? It compiled for me even when the variable is private.

    MarcoFalke commented at 6:38 AM on April 28, 2021:

    Thanks, fixed. Also fixed up some typos.

    MarcoFalke commented at 6:54 AM on April 28, 2021:

    Actually, changing to private didn't compile, so I left this as-is for now.

    kiminuo commented at 7:03 AM on April 28, 2021:

    I moved only const CChainParams& m_params; to private section and it did compile for me. It confuses me but ok.

    MarcoFalke commented at 7:21 AM on April 28, 2021:

    private and protected are the same anyway, unless the class is derived

  15. kiminuo commented at 6:25 AM on April 28, 2021: contributor

    Concept ACK.

    It looks like a very nice simplification which makes it harder to make a mistake.

  16. MarcoFalke force-pushed on Apr 28, 2021
  17. DrahtBot cross-referenced this on Apr 28, 2021 from issue ChainstateManager locking improvements by jamesob
  18. DrahtBot cross-referenced this on Apr 28, 2021 from issue fuzz: Add fuzzing syscall sandbox: detect use of unexpected syscalls when fuzzing ("syscall sanitizer") by practicalswift
  19. DrahtBot cross-referenced this on Apr 28, 2021 from issue Add syscall sandboxing using seccomp-bpf (Linux secure computing mode) by practicalswift
  20. DrahtBot cross-referenced this on Apr 28, 2021 from issue allow -loadblock blocks to be unsorted by LarryRuane
  21. DrahtBot cross-referenced this on Apr 28, 2021 from issue consensus: Simplify ConnectTrace by jnewbery
  22. DrahtBot cross-referenced this on Apr 28, 2021 from issue Improve runtime performance of --reindex by LarryRuane
  23. MarcoFalke cross-referenced this on Apr 30, 2021 from issue Introduce deploymentstatus by ajtowns
  24. MarcoFalke cross-referenced this on Apr 30, 2021 from issue [docs] Update references to RewindBlockIndex() by dhruv
  25. DrahtBot cross-referenced this on May 2, 2021 from issue scripted-diff: Replace three dots with ellipsis in the UI strings by hebasto
  26. DrahtBot commented at 9:31 AM on May 3, 2021: contributor

    <!--4a62be1de6b64f3ed646cdc7932c8cf5-->

    🕵️ @sipa has been requested to review this pull request as specified in the REVIEWERS file.

  27. kiminuo cross-referenced this on May 4, 2021 from issue Remove `GetDataDir(net_specific)` function by kiminuo
  28. MarcoFalke force-pushed on May 5, 2021
  29. DrahtBot cross-referenced this on May 6, 2021 from issue [Bundle 7/7] validation: Farewell, global Chainstate! by dongcarl
  30. DrahtBot added the label Needs rebase on May 10, 2021
  31. MarcoFalke force-pushed on May 10, 2021
  32. DrahtBot removed the label Needs rebase on May 10, 2021
  33. DrahtBot cross-referenced this on May 13, 2021 from issue [p2p] Introduce node rebroadcast module by amitiuttarwar
  34. MarcoFalke referenced this in commit 599000903e on May 24, 2021
  35. DrahtBot cross-referenced this on May 28, 2021 from issue assumeutxo by jamesob
  36. DrahtBot cross-referenced this on May 31, 2021 from issue refactor: address ProcessNewBlock comments from #21713 by fanquake
  37. DrahtBot added the label Needs rebase on Jun 2, 2021
  38. MarcoFalke force-pushed on Jun 2, 2021
  39. DrahtBot removed the label Needs rebase on Jun 2, 2021
  40. in src/chain.h:185 in faa72ee5af outdated 
     181 | @@ -182,7 +182,7 @@ class CBlockIndex
 182 |      //!
 183 |      //! Note: this value is modified to show BLOCK_OPT_WITNESS during UTXO snapshot
 184 |      //! load to avoid the block index being spuriously rewound.
 185 | -    //! @sa RewindBlockIndex
 186 | +    //! @sa NeedsRedownload
    kiminuo commented at 10:18 AM on June 2, 2021:

    (For the sake of completeness, context for the change is here.)

  41. kiminuo commented at 7:32 AM on June 3, 2021: contributor

    The main change in the first commit (fa216cc7) seems to be: "refactor: Add CChainState member to CChainState class" rather than "refactor: Remove ::Params() global from CChainState".

    Also, the changes like this https://github.com/bitcoin/bitcoin/pull/21789/commits/fa216cc79267ba25fa3709982626def61f4abe2a#diff-97c3a52bc5fad452d82670a7fd291800bae20c7bc35bb82686c2c0a4ea7b5b98R2284 in the first commit (fa216cc7) can probably be moved to the second commit (fa06aaacf3b9c35d9e30c883bcd7780dcf0bceda) so that the first commit would only introduce m_params.

    Changes in fa216cc79267ba25fa3709982626def61f4abe2a & fa06aaacf3b9c35d9e30c883bcd7780dcf0bceda LGTM.

  42. MarcoFalke added the label Waiting for author on Jun 3, 2021
  43. jnewbery commented at 10:58 AM on June 9, 2021: member

    Concept ACK

  44. MarcoFalke removed the label Waiting for author on Jun 10, 2021
  45. MarcoFalke commented at 9:49 AM on June 10, 2021: member

    I think the two commits are nicely split up. The first one removes it from inside the functions, the second removes the arg of the functions.

  46. MarcoFalke force-pushed on Jun 10, 2021
  47. in src/test/util/setup_common.cpp:188 in 2222da8320 outdated 
     184 | @@ -185,12 +185,12 @@ TestingSetup::TestingSetup(const std::string& chainName, const std::vector<const
 185 |      assert(!::ChainstateActive().CanFlushToDisk());
 186 |      ::ChainstateActive().InitCoinsCache(1 << 23);
 187 |      assert(::ChainstateActive().CanFlushToDisk());
 188 | -    if (!::ChainstateActive().LoadGenesisBlock(chainparams)) {
 189 | +    if (!::ChainstateActive().LoadGenesisBlock()) {
    jnewbery commented at 10:22 AM on June 10, 2021:

    If you touch this branch again, consider removing the local chainparams reference and just calling Params() in the call to PeerManager::make() below.

    MarcoFalke commented at 7:46 AM on June 13, 2021:

    The line will be touched again in the follow-up that removes ::Params(), so I'll leave it as is for now.

  48. jnewbery commented at 10:24 AM on June 10, 2021: member

    ACK 2222da832066d06be3ff8752e49008fef71eb668

  49. DrahtBot added the label Needs rebase on Jun 12, 2021
  50. refactor: Remove ::Params() global from inside CChainState member functions
    It is confusing and verbose to repeatedly access the global when a
member variable can simply refer to it.
    fa38947125
  51. refactor: Remove chainparams arg from CChainState member functions
    Passing this is confusing and redundant with the m_params member.
    fa0d9211ef
  52. MarcoFalke force-pushed on Jun 13, 2021
  53. DrahtBot removed the label Needs rebase on Jun 13, 2021
  54. MarcoFalke commented at 7:22 AM on June 14, 2021: member

    Rebased

  55. jnewbery commented at 8:40 AM on June 14, 2021: member

    ACK fa0d9211ef87a682573aaae932c0c440acbcb8a8

  56. kiminuo commented at 9:40 AM on June 14, 2021: contributor

    utACK fa0d9211

  57. in src/test/interfaces_tests.cpp:101 in fa0d9211ef 
      97 | @@ -98,7 +98,7 @@ BOOST_AUTO_TEST_CASE(findCommonAncestor)
  98 |      auto* orig_tip = active.Tip();
  99 |      for (int i = 0; i < 10; ++i) {
 100 |          BlockValidationState state;
 101 | -        m_node.chainman->ActiveChainstate().InvalidateBlock(state, Params(), active.Tip());
 102 | +        m_node.chainman->ActiveChainstate().InvalidateBlock(state, active.Tip());
    kiminuo commented at 9:56 AM on June 14, 2021:

    Is #include <chainparams.h> still needed?

    MarcoFalke commented at 1:57 PM on June 14, 2021:

    Thanks, will try to remove if I have to force push

  58. DrahtBot cross-referenced this on Jun 14, 2021 from issue Move CBlockTreeDB to node/blockstorage by MarcoFalke
  59. theStack approved
  60. theStack commented at 9:47 PM on June 28, 2021: contributor

    ACK fa0d9211ef87a682573aaae932c0c440acbcb8a8 🍉 Nice refactoring idea, glad to see this simplification of the CChainState interface.

  61. fanquake merged this on Jun 29, 2021
  62. fanquake closed this on Jun 29, 2021
  63. MarcoFalke deleted the branch on Jun 29, 2021
  64. sidhujag referenced this in commit 950bf6039a on Jun 29, 2021
  65. gwillen referenced this in commit cac878cf89 on Jun 1, 2022
  66. bitcoin locked this on Aug 18, 2022
Contributors
MarcoFalkejamesobpracticalswiftfanquakeDrahtBotkiminuojnewberytheStack
Labels
RefactoringValidation
Linked (view graph)
#19438 Introduce deploymentstatus#21816 [docs] Update references to RewindBlockIndex()#21850 Remove `GetDataDir(net_specific)` function
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: 2026-05-20 06:54 UTC