Backport versionbits refactoring#77
Merged
ajtowns merged 15 commits intobitcoin-inquisition:29.xfrom Jul 11, 2025
Merged
Conversation
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.
maflcko
reviewed
Jul 9, 2025
| rv.pushKV("bip9", std::move(bip9)); | ||
|
|
||
| rv.pushKV("active", is_active); | ||
| rv.pushKV("bip9", bip9); |
There was a problem hiding this comment.
nit: removing the std::move looks like a rebase error or typo in the upstream pull?
Author
There was a problem hiding this comment.
This is the same commit hash as the upstream PR, no rebasing needed :) I don't think it's a typo either -- the univalue pushKV is pass by value, not reference, let alone rvalue-reference? Or, not a typo, just a mistake, that should be fixed upstream too?
There was a problem hiding this comment.
just a mistake, that should be fixed upstream too?
Yeah, the next time someone creates a commit like d7707d9. Probably shouldn't matter much in this call, just was confused why this was reverted.
maflcko
reviewed
Jul 11, 2025
| case ThresholdState::LOCKED_IN: return "LOCKED_IN"; | ||
| case ThresholdState::ACTIVE: return "ACTIVE"; | ||
| case ThresholdState::FAILED: return "FAILED"; | ||
| } // no default case, so the compiler can warn about missing cases |
There was a problem hiding this comment.
nit in e74a704: Could have moved/preserved this comment, according to the dev notes.
|
lgtm ACK e301401 |
darosior
approved these changes
Jul 11, 2025
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
From bitcoin#29039; affects heretical deployment code.