8000 refactor: Remove almost all validation option globals by maflcko · Pull Request #25704 · bitcoin/bitcoin · GitHub
[go: up one dir, main page]
More Web Proxy on the site http://driver.im/
Skip to content

refactor: Remove almost all validation option globals #25704

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
Merged
merged 7 commits into from
Oct 26, 2022
Merged

refactor: Remove almost all validation option globals #25704

merged 7 commits into from
Oct 26, 2022

Conversation

maflcko
Copy link
Member
@maflcko maflcko commented Jul 26, 2022

It seems preferable to assign globals to a class (in this case ChainstateManager), than to leave them dangling. This should clarify scope for code-readers, as well as clarifying unit test behaviour.

@glozow
Copy link
Member
glozow commented Jul 26, 2022

nice, Concept ACK

@DrahtBot
Copy link
Contributor
DrahtBot commented Jul 26, 2022 •
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:

  • #26251 (refactor: add cs_main.h by fanquake)
  • #25862 (refactor, kernel: Remove gArgs accesses from dbwrapper and txdb by ryanofsky)
  • #25740 (assumeutxo: background validation completion by jamesob)
  • #25193 (indexes: Read the locator's top block during init, allow interaction with reindex-chainstate by mzumsande)

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 Jul 26, 2022
Merged
Merged
Merged
Merged
Merged
Closed
ryanofsky
ryanofsky approved these changes Aug 2, 2022
View reviewed changes
Copy link
Contributor
@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK fa82b44

@maflcko maflcko force-pushed the 2207-val-globals- branch from fa82b44 to fa3cac2 Compare August 3, 2022 10:49
@glozow
Copy link
Member
glozow commented Aug 4, 2022

Seeing conflicts with a couple kernel PRs, so pinging @dongcarl to see if this interferes with anything

@maflcko maflcko changed the title refactor: Remove all validation option globals refactor: Remove almost all validation option globals Aug 4, 2022
@maflcko maflcko force-pushed the 2207-val-globals- branch 3 times, most recently from 33ea57b to fa78faa Compare August 5, 2022 09:29
This was referenced Aug 5, 2022
Merged
Merged
@dongcarl
Copy link
Contributor

Concept and light Code Review ACK, very satisfying PR.

@dongcarl
Copy link
Contributor

Code Review ACK fa738b8

Shuffling between AppInitParameterInteraction and AppInitMain seem safe because the code being moved only logically depend on static variables and chainparams, which is const throughout and set before AppInitParameterInteraction and AppInitMain are called.

Followups:

  1. Have ChainstateManager own an std::optional scriptcheckqueue so there’s a single (local) source of truth for whether or not to enable parallel script checks
  2. Add a ApplyArgsManOptions for ChainstateManager::Options (already done in [kernel 3e/n] Decouple CDBWrapper and its kernel users from ArgsManager #25623, but needs to incorporate the new members introduced in this PR)

Marco: Any reason these commits are "Unverified"?

@maflcko maflcko force-pushed the 2207-val-globals- branch from fa78faa to fa68b9e Compare August 19, 2022 12:04
@DrahtBot DrahtBot mentioned this pull request Aug 19, 2022
Merged
@maflcko
Copy link
Member Author
maflcko commented Aug 19, 2022

Followups:

Good idea.

Marco: Any reason these commits are "Unverified"?

Yes, this is a stick I threw in the mud so that you can't trust Microsoft/GitHub to mark my commits as verified. If you care about this, you can verify it locally, but it seems no one does that anyway.

@maflcko maflcko force-pushed the 2207-val-globals- branch from fa68b9e to fa39cf8 Compare August 22, 2022 09:39
ryanofsky
ryanofsky approved these changes Oct 5, 2022
View reviewed changes
Copy link
Contributor
@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK fa227bd. Left some suggestions, but could be merged as-is.

Main change since last review is rebasing and making assumed valid block, minimum chain work, and check block index options in kernel API match bitcoind defaults, using std::optional type and a Flatten function.

(Flatten isn't my favorite approach but it solves problem of making kernel API straightforward to use and having kernel API defaults match bitcoind defaults. Preferred approach to doing this would have been to keep m_options fixed and just add accessor methods to merge option values and runtime defaults on demand. But difference between the approaches are internal to ChainManager, so not very significant.)

@maflcko maflcko force-pushed the 2207-val-globals- branch from fa227bd to fa76987 Compare October 5, 2022 16:40
ryanofsky
ryanofsky approved these changes Oct 5, 2022
View reviewed changes
Copy link
Contributor
@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK fa76987. Just suggested code and comment cleanups since last review, and moving the -minimumchainwork check back to AppInitParameterInteraction, before daemonization.

@DrahtBot DrahtBot mentioned this pull request Oct 5, 2022
Merged
@glozow glozow requested a review from TheCharlatan October 6, 2022 09:52
@DrahtBot DrahtBot mentioned this pull request Oct 12, 2022
Merged
MacroFake added 7 commits October 18, 2022 14:07
Move ::nMaxTipAge into ChainstateManager
faf4487
Move ::hashAssumeValid into ChainstateManager
fa29d0b 
This changes the assumed valid block for the bitcoin-chainstate
executable. Previously it was uint256{}, now it is defaultAssumeValid.
Move ::nMinimumChainWork into ChainstateManager
cccca83 
This changes the minimum chain work for the bitcoin-chainstate
executable. Previously it was uint256{}, now it is the chain's default
minimum chain work.
Move ::fCheckpointsEnabled into ChainstateManager
fa43188
Move ::fCheckBlockIndex into ChainstateManager 8000
fa7c834 
This changes the flag for the bitcoin-chainstate executable. Previously
it was false, now it is the chain's default value (still false for the
main chain).
Remove g_parallel_script_checks
fa9ebec
iwyu: Add missing includes
aaaa7bd
ryanofsky
ryanofsky approved these changes Oct 18, 2022
View reviewed changes
Copy link
Contributor
@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

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

Code review ACK aaaa7bd. No changes since last review, other than rebase

@dergoegge
Copy link
Member

Concept ACK

aureleoules
aureleoules approved these changes Oct 19, 2022
View reviewed changes
Copy link
Contributor
@aureleoules aureleoules left a comment

Choose a reason for hiding this comment

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

reACK aaaa7bd

@jamesob
Copy link
Contributor
jamesob commented Oct 25, 2022

Concept ACK, will try to review today or tomorrow.

@dergoegge
Copy link
Member

Code review ACK aaaa7bd

@maflcko maflcko merged commit a1fff27 into bitcoin:master Oct 26, 2022
@maflcko maflcko deleted the 2207-val-globals-💧 branch October 26, 2022 09:47
sidhujag pushed a commit to syscoin/syscoin that referenced this pull request Oct 27, 2022
Merge bitcoin#25704: refactor: Remove almost all validation option gl…
7de7cb3 
…obals
@hebasto
Copy link
Member
hebasto commented Oct 27, 2022

GCC throws multiple -Wmissing-field-initializers warnings with this code.

@maflcko
Copy link
Member Author
maflcko commented Oct 27, 2022

Which version of GCC and what are the steps to reproduce?

@hebasto
Copy link
Member
hebasto commented Oct 27, 2022

Which version of GCC and what are the steps to reproduce?

Debian 11 + gcc 10.2, Ubuntu 22.04 gcc 11.3:

$ ./autogen.sh
$ ./configure
$ make clean
$ make

@maflcko
Copy link
Member Author
maflcko commented Oct 28, 2022

Thanks, fixed in #26409

@jamesob
Copy link
Contributor
jamesob commented Nov 9, 2022

There was a bug in here, though I certainly don't think I would have caught it: #26477

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

@theuni theuni theuni left review comments

@dongcarl dongcarl dongcarl left review comments

@ryanofsky ryanofsky ryanofsky approved these changes

@aureleoules aureleoules aureleoules approved these changes

@TheCharlatan TheCharlatan Awaiting requested review from TheCharlatan

Assignees
No one assigned
Labels
Refactoring
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
0