-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
versionbits refactoring #29039
base: master
Are you sure you want to change the base?
versionbits refactoring #29039
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, 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. |
e95ed89
to
e8134a5
Compare
e8134a5
to
72b22e3
Compare
At the end of this sequence of patches the This also simplifies and modernises some of the code, much of which has been proposed previously (eg, removing the params arguments from AbstractThresholdConditionChecker was included in the first versions of #21380; making threshold a per-deployment params was part of #19573 and #21392).
|
72b22e3
to
fdef8ea
Compare
fdef8ea
to
ecf89e8
Compare
ecf89e8
to
11d2b78
Compare
Concept ACK. Looks cleaner. Will look in more detail when you undraft it. |
11d2b78
to
b72035f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept NACK, refactoring without a purpose, and probably makes more work for merging BIP8.
b72035f
to
b865392
Compare
b865392
to
d28e150
Compare
For an abstract class, specifying parameters in detail serves no point; and for the concrete implementation, changing the consensus parameters between invocations doesn't make sense. So simplify the class by removing the consensus params from the method arguments, and just make it a member variable in the concrete object where needed. This also allows dropping dummy parameters from the unit/fuzz tests.
Rather than having the rule change period/threshold be constant for all potential deployments on a chain, have it be specific to the deployment itself. This both matches history (BIP 9 specified a 2016 block period and 1916 block threshold; BIP 91 specified a 336 block period and 269 block threshold; and BIP 341 specified a 2016 block period and 1815 block threshold), and allows the code to be simplified, as only the BIP9Deployment structure is needed, not the full Consensus::Params structure.
Rather than having the RPC code have knowledge about how BIP9 is implemented, create a reporting function in the versionbits code, and limit the RPC code to coverting the result of that into Univalue/JSON.
Rather than having the RPC code have knowledge about how BIP9 is implemented, create a reporting function in the versionbits code, and limit the RPC code to coverting the result of that into the appropriate output for getblocktemplate.
Replaces State() (which returned ACTIVE/STARTED/etc) with IsActiveAfter() which just returns a bool, as this was all State was actually used for. Drops Mask(), which was only used in tests and can be replaced with `1<<bit`, and also drops StateSinceHeight() and Statistics(), which are now only used internally for Info().
Rather than essentially duplicating StateName in the unit tests, expose it via the impl header.
Base the unit test directly on `VersionBitsConditionChecker`, slightly improving coverage, in particular adding coverage for the the logic regarding setting the TOP_BITS.
Test `VersionBitsConditionChecker` behaviour directly, rather than reimplementing it, thus slightly improving fuzz test coverage of the real code.
d28e150
to
f981a84
Compare
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
🐙 This pull request conflicts with the target branch and needs rebase. |
Increases the encapsulation/modularity of the versionbits code, moving more of the logic into the versionbits module rather than having it scattered across validation and rpc code. Updates unit/fuzz tests to test the actual code used rather than just a close approximation of it.