versionbits refactoring

ajtowns commented at 4:34 am on December 9, 2023: contributor

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.

DrahtBot commented at 4:34 am on December 9, 2023: contributor

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

Code Coverage

For detailed information about the code coverage, see the test coverage report.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Concept NACK	luke-jr
Concept ACK	naumenkogs

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#30440 (Have createNewBlock() return a BlockTemplate interface by Sjors)
#27433 (getblocktemplate improvements for segwit and sigops by Sjors)
#26201 (Remove Taproot activation height by Sjors)

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.

ajtowns renamed this:
~~202312 vbits simplify~~
versionbits refactoring
on Dec 9, 2023

ajtowns added the label Refactoring on Dec 9, 2023

ajtowns force-pushed on Dec 9, 2023

DrahtBot added the label CI failed on Dec 9, 2023

ajtowns force-pushed on Dec 9, 2023

ajtowns marked this as a draft on Dec 9, 2023

ajtowns commented at 4:59 am on December 9, 2023: contributor

At the end of this sequence of patches the VersionBitsCache object provides a standalone interface to all the “versionbits” features needed by bitcoin core, which means that if the BIP9/speedy trial implementation is changed in future, it should be possible to do that in one place, without needing to change validation/RPC code as well.

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).

After these changes, it should be possible to simplify the unit/fuzz tests a little, as they should now be able to use VersionBitsConditionChecker directly, since it no longer depends on access to Consensus::Params. (actually does this now)

ajtowns force-pushed on Dec 9, 2023

DrahtBot removed the label CI failed on Dec 9, 2023

ajtowns force-pushed on Dec 9, 2023

naumenkogs commented at 9:41 am on December 11, 2023: member

Concept ACK. Looks cleaner. Will look in more detail when you undraft it.

ajtowns force-pushed on Dec 15, 2023

luke-jr commented at 0:49 am on December 17, 2023: member

Concept NACK, refactoring without a purpose, and probably makes more work for merging BIP8.

ajtowns force-pushed on Dec 18, 2023

ajtowns force-pushed on Dec 20, 2023

ajtowns marked this as ready for review on Dec 20, 2023

DrahtBot added the label CI failed on Jan 16, 2024

ajtowns force-pushed on Feb 13, 2024

DrahtBot removed the label CI failed on Feb 13, 2024

achow101 requested review from dergoegge on Apr 9, 2024

DrahtBot added the label CI failed on Apr 20, 2024

DrahtBot removed the label CI failed on Apr 23, 2024

DrahtBot added the label CI failed on May 12, 2024

DrahtBot commented at 11:07 am on May 12, 2024: contributor

🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the documentation.

Possibly this is due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

Leave a comment here, if you need help tracking down a confusing failure.

Debug: https://github.com/bitcoin/bitcoin/runs/21508984099

DrahtBot added the label Needs rebase on May 23, 2024

ajtowns force-pushed on Aug 22, 2024

DrahtBot removed the label Needs rebase on Aug 22, 2024

ajtowns force-pushed on Aug 22, 2024

DrahtBot removed the label CI failed on Aug 22, 2024

DrahtBot added the label Needs rebase on Aug 28, 2024

ajtowns force-pushed on Aug 28, 2024

DrahtBot removed the label Needs rebase on Aug 28, 2024

DrahtBot added the label Needs rebase on Sep 2, 2024

versionbits: Use std::array instead of C-style arrays 14685caac8

versionbits: Remove params from AbstractThresholdConditionChecker

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.

bc6aca0df3

consensus/params: Move version bits period/threshold to bip9 param

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.

3a92544f59

versionbits: Change BIP9Stats to uint32_t types abd7a33e1b

versionbits: Move WarningBits logic from validation to versionbits ae20930df4

versionbits: Move getdeploymentinfo logic to versionbits

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.

e4ec7bd02c

versionbits: Move BIP9 status logic for getblocktemplate to versionbits

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.

9059a1150f

versionbits: Simplify VersionBitsCache API

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().

47df9abdc8

versionbits: Split out internal details into impl header eb1d045d43

versionbits: Expose StateName function

Rather than essentially duplicating StateName in the unit tests, expose
it via the impl header.

55dabed2fb

versionbits: Expose VersionBitsConditionChecker via impl header 7ff74e38e5

tests: refactor versionbits unit test

Base the unit test directly on `VersionBitsConditionChecker`, slightly
improving coverage, in particular adding coverage for the the logic
regarding setting the TOP_BITS.

7df5185279

tests: refactor versionbits fuzz test

Test `VersionBitsConditionChecker` behaviour directly, rather than
reimplementing it, thus slightly improving fuzz test coverage of the
real code.

0bdc31c6cd

ajtowns force-pushed on Sep 2, 2024

DrahtBot removed the label Needs rebase on Sep 3, 2024

DrahtBot commented at 9:09 pm on September 16, 2024: contributor

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

DrahtBot added the label Needs rebase on Sep 16, 2024

versionbits refactoring #29039

Code Coverage

Reviews

Conflicts