Skip to content

refactor, consensus: remove calls to global Params() in validation layer #23448

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.

Sign up for GitHub

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

Jump to bottom
Closed
lsilva01 wants to merge 1 commit into from
Closed

refactor, consensus: remove calls to global Params() in validation layer #23448

lsilva01 wants to merge 1 commit into from

Conversation

@lsilva01
Copy link
Contributor

@lsilva01 lsilva01 commented Nov 5, 2021

This PR removes calls to global Params() in validation layer (except in the CChainState constructor).

Motivation: Reducing the use of global variables makes code more predictable.

Requires #23437 as it changes the visibility of CChainState::m_params to public.

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 6, 2021 •
edited
Loading

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

  • #23581 (Move BlockManager to node/blockstorage by MarcoFalke)
  • #23174 (validation: have LoadBlockIndex account for snapshot use by jamesob)
  • #22674 (validation: mempool validation and submission for packages of 1 child + parents by glozow)

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.

This was referenced Nov 6, 2021
Merged
Merged
Merged
Closed
Closed
Merged
stratospher
stratospher reviewed Nov 26, 2021
View reviewed changes
Copy link
Contributor

@stratospher stratospher left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Concept ACK.

  • aac6548
    • CChainParams m_params is made a public member of class CChainState.
    • chainstate.m_params is used instead of global Params() in AcceptToMemoryPool in validation.cpp. (with changes in fuzz/tx_pool.cpp done in PR 23465)
  • 20df92b
    • chainstate.m_params is used instead of global Params() in validation.cpp inside functions like ProcessNewPackage(), ProcessNewBlockHeaders(), LoadMempool() and PopulateAndValidateSnapshot().
    • The function signature of LoadBlockIndexDB() is changed to include the first argument as Consensus::Params and call site updated in order to avoid the global call to Params() within the function.

I'm also curious to know if there's any added advantage in making m_params public (in comparison with using getters/setters)?

@DrahtBot DrahtBot mentioned this pull request Dec 2, 2021
Merged
@DrahtBot
Copy link
Contributor

DrahtBot commented Dec 15, 2021

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a "draft".

@fanquake
Copy link
Member

fanquake commented May 6, 2022

@lsilva01 are you still interested in working on this?

@fanquake fanquake closed this Jun 14, 2022
@maflcko
Copy link
Member

maflcko commented Jun 14, 2022

Pretty sure this is already done in master?

@bitcoin bitcoin locked and limited conversation to collaborators Jun 14, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

1 more reviewer

@stratospher stratospher stratospher left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

Needs rebase Refactoring Validation

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@lsilva01 @DrahtBot @fanquake @maflcko @stratospher @glozow