chaincfg+blockchain: abstract/refactor BIP 9 version bits implementation to work w/ BIP 8 block heights #1700

Roasbeef · 2021-03-20T00:26:13Z

In this PR, we further abstract our Version Bits implementation to be able to support both BIP 9 time based timeouts, as well as BIP 8 block based timeouts. In order to bridge the two mechanisms, we introduces a series of new light interfaces that allow us to abstract away from when exactly a soft fork deployment "starts" and "ends". This PR doesn't yet add any new BIP 8 compatible deployments, as it's mean to be a pure refactoring that doesn't introduce any new behavior. It's also the case that BIP 8 has some extra Version Bits states that need to be added to our Version Bits implementation.

The only thing missing from this PR is a few additional tests to confirm that no change in behavior has occurred. Reviewers should examine the change to how we define deployments that are "always started" and "never ended", as well as the usage of time methods to determine if a deployment has started or ended.

Roasbeef · 2021-03-20T00:32:48Z

The alternative to this PR is to just go with the old: "lower than this value is a block height" approach which is used for lock time related logic elsewhere. However, I figured it's better to just further abstract things and hide more of the implementation details from the call sits within the blockchain package.

qustavo · 2021-03-23T14:31:48Z

blockchain/versionbits.go

+
+	// Can't fail as we make sure to set the clock above.
+	header := blkNode.Header()
+	started, _ := deploymentStarter.HasStarted(&header)


Since deploymentStarter is an interface, and you don't know how the the actual implementation looks like, wouldn't if be better to check for the error?

Roasbeef · 2021-03-24T03:47:54Z

So for this to potentially be "taproot ready", assuming this "speed trial" thing happens, then we'll need to also add a minimum_activation_height parameter. Basically this would be a new gate here: https://github.com/btcsuite/btcd/blob/master/blockchain/thresholdstate.go#L243 to only go to active if that height has passed (and it's defined for that deployment).

wpaulino

I really like this change, even though Taproot didn't activate through BIP 8, it paves the way forward for any future soft forks that do.

As you mentioned, this PR is still missing the state transition changes as part of Speedy Trial.

chaincfg/deployment_time_frame.go

chaincfg/params.go

chaincfg/deployment_time_frame.go

blockchain/versionbits.go

chaincfg/params.go

coveralls · 2022-01-12T03:22:53Z

Pull Request Test Coverage Report for Build 1748228076

0 of 0 changed or added relevant lines in 0 files are covered.
No unchanged relevant lines lost coverage.
Overall coverage remained the same at 79.676%

Totals
Change from base Build 1684921388:	0.0%
Covered Lines:	1231
Relevant Lines:	1545

💛 - Coveralls

Roasbeef · 2022-01-12T04:00:52Z

Initial comments addressed, next commit with add the possibility for a custom threshold per deployment and also the min activation height. Will then add a new dummy deployment to test against that has both of these new values specified.

blockchain/versionbits.go

Roasbeef · 2022-01-14T01:54:13Z

Ok, this should be good to go now. Reviews should ensure they've internalized the commit body in this commit: c1842e2

;

Sjors · 2022-01-14T13:42:12Z

From c1842e2:

One thing worth noting here is that logic at the end of BIP 341 doesn't
match bitcoind as implemented. The bitcoind logic fixes an off-by-one
error in the BIP itself.

Just to make sure I understand what you're saying here. Are you referring to the transition from LOCKED_IN to ACTIVE?

BIP 341 states:

case LOCKED_IN:
        if (block.nHeight < min_activation_height) {
            return LOCKED_IN;
        }
        return ACTIVE;

The returned value (LOCKED_IN or ACTIVE) in this pseudo code refers to the state for the current block, i.e. the block at block.nHeight. This follows the convention in BIP 9, where e.g. the pseudo code returns DEFINED for the genesis block.

So the current block is ACTIVE when its height is at or above the activation height.

Bitcoin Core does the following:

case ThresholdState::LOCKED_IN: {
                // Progresses into ACTIVE provided activation height will have been reached.
                if (pindexPrev->nHeight + 1 >= min_activation_height) {
                    stateNext = ThresholdState::ACTIVE;

This is part of the function:

    /** Returns the state for pindex A based on parent pindexPrev B. Applies any state transition if conditions are present.
     *  Caches state from first block of period. */
    ThresholdState GetStateFor(const CBlockIndex* pindexPrev,

When this function is called with the Tip block, it returns the state for the next (unmined!) block. So pindexPrev->nHeight + 1 refers to the upcoming block, i.e. the rules that apply in the mempool. So the fork becomes ACTIVE for the mempool when height of the tip block + 1 is min_activation_height.

So I don't think there's an off by one error in the BIP, but both the BIP and the Bitcoin Core implementation are confusing enough to be likely to cause off-by-one errors (and in fact they did with SegWit2x).

~~As an aside: afaik the deployment info was added to BIP 341 after Taproot locked in with the speedy trial.~~ (neh they were written around the same time)

Roasbeef · 2022-01-14T20:58:38Z

Decided ultimately to special case things a bit just so it matches 1:1, once this base PR is in, I want to follow up with some additional tests for the hybrid state machine.

Roasbeef · 2022-01-24T02:14:02Z

Hey @Sjors, yeh I explained this on #btcd IRC. I was just initially tripped up by the fact that the BIP and the bitcoind implementation don't match up (one uses the current block (to be mined/accepted) while the other uses the previous. Our BIP 9 implementation always uses the previous block.

Roasbeef · 2022-01-24T03:11:44Z

Implemented the BIP 9 modifications, and added a new unit test here: 75d3489.

This PR should be g2g now.

guggero

I have limited understanding of activation mechanisms in general, but from what I see in this diff the changes make sense to me. Always nice to see how a well-defined interface can make things very straightforward to understand!

chaincfg/deployment_time_frame.go

chaincfg/params.go

blockchain/thresholdstate.go

integration/bip0009_test.go

In this commit, we create a series of new interfaces that'll allow us to abstract "when" exactly a deployment starts and ends. As is, all deployments start/end based on a unix timestamp, which is compared against the MTP of a given block to determine if a new deployment has started or ended. This works fine for BIP 9 which uses time based timeouts, but not so much for BIP 8. In order to prep a future refactor that allows our version bits implementation to support both time and block based start/end times, this new abstraction has been introduced.

…times In this commit, we utilize the recently added ConsensusDeploymentStarter and ConsensusDeploymentEnder interfaces. Concrete implementations of this interface based on the median time past comparison are used now in the ConsensusDeployment struct instead of hard coded start/end times. Along the way, we had to switch to using the "zero time": time.Time{}, in place of 0 and math.MaxInt64 as comparison (After/Before) seems to be broken in the Go stdlib for times very far in the future. It appears Go isn't ready to handle the heat death of the universe.

…deployments In this commit, we update our version bits logic to use the newly added HasStarted and HasEnded methods for consensus deployments. Along the way, wee modify the thresholdConditionChecker` interface to be based off the new chaincfg interfaces. In addition, we add a new method `PastMedianTime`, in order to allow the chain itself to be used as a `chaincfg.BlockClock`. This serves to make the logic more generic in order to support both block height and time based soft fork timeouts.

In this commit, we add a new "dummy" deployment that adds the new params used to activate taproot. We chose to add a new deployment as unlike the bitcoind codebase, we don't currently "bury" soft forks that have happened in the past (hard code an activation height). The old taproot deployment has been removed as with the way the array works, a deployment needs to be defined for _all_ networks.

…tom thresholds In this commit, we extend the existing version bits state machine to add support for the new minimum activation height and custom block threshold for activation. We then extend the existing BIP 9 tests (tho this isn't really BIP 9 anymore...) to exercise the new min activation height logic.

In this commit, we extract the BIP 9 state transition logic from the thresholdState method into a new thresholdStateTransition function that allows us to test all the defined state transitions, including the modified "speedy trial" logic.

guggero

LGTM 🎉

wpaulino · 2022-01-26T21:40:26Z

Post-merge ACK 👍

Roasbeef added consensus BIP 9 BIP 8 soft forks labels Mar 20, 2021

Sjors mentioned this pull request Mar 20, 2021

BIP 341: Specify Speedy Trial activation parameters bitcoin/bips#1081

Closed

qustavo reviewed Mar 23, 2021

View reviewed changes

Roasbeef mentioned this pull request Apr 15, 2021

BIP341: speedy trial activation parameters bitcoin/bips#1104

Merged

Roasbeef mentioned this pull request Jan 7, 2022

multi: implement BIP 341 and 342 a.k.a complete taproot and tapscript consensus verification logic #1787

Merged

5 tasks

wpaulino reviewed Jan 11, 2022

View reviewed changes

Roasbeef force-pushed the bip-8-prep branch from 7574c5a to a8735ed Compare January 12, 2022 03:21

Roasbeef force-pushed the bip-8-prep branch 2 times, most recently from ed86f68 to a50756b Compare January 12, 2022 03:39

wpaulino reviewed Jan 12, 2022

View reviewed changes

blockchain/versionbits.go Show resolved Hide resolved

Roasbeef force-pushed the bip-8-prep branch from 6e9666c to fed3a3e Compare January 14, 2022 01:53

Roasbeef requested a review from wpaulino January 14, 2022 01:55

kcalvinalvin reviewed Jan 14, 2022

View reviewed changes

; Outdated Show resolved Hide resolved

Roasbeef force-pushed the bip-8-prep branch from fed3a3e to e87961e Compare January 24, 2022 03:09

Roasbeef force-pushed the bip-8-prep branch from e87961e to 9771d4a Compare January 24, 2022 03:14

guggero reviewed Jan 25, 2022

View reviewed changes

Roasbeef added 2 commits January 25, 2022 15:27

Roasbeef added 6 commits January 25, 2022 15:27

build: don't run the integration tests w/ -race

0556c70

btcjson+rpc: add min activation height to soft fork RPC response

0b245cc

Roasbeef force-pushed the bip-8-prep branch from 9771d4a to 0b245cc Compare January 25, 2022 23:28

guggero approved these changes Jan 26, 2022

View reviewed changes

Roasbeef merged commit 588c071 into btcsuite:master Jan 26, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

chaincfg+blockchain: abstract/refactor BIP 9 version bits implementation to work w/ BIP 8 block heights #1700

chaincfg+blockchain: abstract/refactor BIP 9 version bits implementation to work w/ BIP 8 block heights #1700

Roasbeef commented Mar 20, 2021

Roasbeef commented Mar 20, 2021

qustavo Mar 23, 2021

Roasbeef commented Mar 24, 2021 •

edited

wpaulino left a comment

coveralls commented Jan 12, 2022 •

edited

Roasbeef commented Jan 12, 2022

Roasbeef commented Jan 14, 2022

Sjors commented Jan 14, 2022 •

edited

Roasbeef commented Jan 14, 2022

Roasbeef commented Jan 24, 2022

Roasbeef commented Jan 24, 2022

guggero left a comment

guggero left a comment

wpaulino commented Jan 26, 2022

chaincfg+blockchain: abstract/refactor BIP 9 version bits implementation to work w/ BIP 8 block heights #1700

chaincfg+blockchain: abstract/refactor BIP 9 version bits implementation to work w/ BIP 8 block heights #1700

Conversation

Roasbeef commented Mar 20, 2021

Roasbeef commented Mar 20, 2021

qustavo Mar 23, 2021

Choose a reason for hiding this comment

Roasbeef commented Mar 24, 2021 • edited

wpaulino left a comment

Choose a reason for hiding this comment

coveralls commented Jan 12, 2022 • edited

Pull Request Test Coverage Report for Build 1748228076

💛 - Coveralls

Roasbeef commented Jan 12, 2022

Roasbeef commented Jan 14, 2022

Sjors commented Jan 14, 2022 • edited

Roasbeef commented Jan 14, 2022

Roasbeef commented Jan 24, 2022

Roasbeef commented Jan 24, 2022

guggero left a comment

Choose a reason for hiding this comment

guggero left a comment

Choose a reason for hiding this comment

wpaulino commented Jan 26, 2022

Roasbeef commented Mar 24, 2021 •

edited

coveralls commented Jan 12, 2022 •

edited

Sjors commented Jan 14, 2022 •

edited