Implement BIP 8 based Speedy Trial activation #21392

achow101 · 2021-03-08T22:31:04Z

Implements the block height (BIP 8) based Speedy Trial activation proposal that was discussed on the bitcoin-dev mailing list.

In order to do so, this PR first changes the versionbits (BIP 9) implementation to use heights rather than MTP. Then the minimum activation height parameter is introduced. This also adds the minimum activation height as a third parameter of -vbparams. Additionally, the threshold is made a parameter of each deployment so that it can be customized for each deployment. Lastly several unit and functional tests are added.

In order to make this easier to backport, the names of functions and classes are largely unchanged (except where necessary for the height based things). This means that some things are misnamed as they refer to BIP 9 even though this is really BIP 8.

This PR borrows 2 commits from #19573 to implement height based versionbits, one commit from #21377 for tests, and 1 commit formerly part of #21380 for the threshold parameterization.

src/validation.h

src/chainparamsbase.cpp

ajtowns · 2021-03-08T22:56:22Z

Maybe consider rebasing on #21380 ; it adds fuzz testing and removes the need for the MinActivationHeight() virtual functions, and should make the "per-deployment-threshold" part a bit simpler.

achow101 · 2021-03-09T01:47:11Z

Maybe consider rebasing on #21380 ; it adds fuzz testing and removes the need for the MinActivationHeight() virtual functions, and should make the "per-deployment-threshold" part a bit simpler.

Done. It did indeed make this simpler. The fuzzer also caught one minor issue.

JeremyRubin

generally looks ok to me couple minor comments; can do a more detailed review later

JeremyRubin · 2021-03-09T01:54:18Z

src/consensus/params.h

-    /** Timeout/expiry MedianTime for the deployment attempt. */
-    int64_t nTimeout;
+    /** Start block height for version bits miner confirmation. Must be a retarget block, can be in the past. */
+    int64_t startheight;


Should this be set to NEVER_ACTIVE?

The potential for uninitialized members here is concerning to me, so I am working on a followup to deal with those. That may be rolled into this PR.

src/versionbits.cpp

DrahtBot · 2021-03-09T04:23:02Z

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

Refactor versionbits deployments to avoid potential uninitialized variables #21401 (Refactor versionbits deployments to avoid potential uninitialized variables by achow101)
[Bundle 5/n] Prune g_chainman usage in RPC modules #21391 ([Bundle 5/n] Prune g_chainman usage in RPC modules by dongcarl)
Convert taproot to flag day activation #21378 (Convert taproot to flag day activation by ajtowns)
Speedy trial support for versionbits #21377 (Speedy trial support for versionbits by ajtowns)
test: Add feature_taproot.py --previous_release #20354 (test: Add feature_taproot.py --previous_release by MarcoFalke)
Introduce deploymentstatus #19438 (Introduce deploymentstatus by ajtowns)

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.

Sjors

Concept ACK. Did some light review of 9e50401.

Would love to see a functional test that shows the speedy trial in action, by looking at softforks in getblockchaininfo (I might write one myself)

doc/bips.md

src/rpc/blockchain.cpp

src/chainparamsbase.cpp

src/rpc/blockchain.cpp

test/functional/feature_taproot.py

Sjors · 2021-03-25T14:12:16Z

re-ACK 2e55bce

michaelfolkson · 2021-03-25T20:46:20Z

ACK 2e55bce

Ideally a choice on a Speedy Trial PR would have been made by now so I could focus review and testing on one PR and one approach. Spreading review over two competing PRs is doing none of us any favors. I would have liked to have spent more time testing this PR before ACKing it but I think the biggest danger at this point is spreading review too thinly across two PRs.

JeremyRubin · 2021-03-25T21:16:16Z

src/consensus/params.h

+    int startheight;
+    /** Timeout/expiry block height for the deployment attempt. Must be a retarget block. */
+    int timeoutheight;
+    /** Threshold for lockin. Must be at least nRuleChangeActivationThreshold for that network. */


I think this variable name changed.

Will fix if I retouch.

luke-jr · 2021-03-25T19:37:55Z

src/chainparams.cpp

+        return false;
+    }
+    if (timeoutheight < startheight + (2 * (int)consensus.nMinerConfirmationWindow)) {
+        error = strprintf("Invalid timeoutheight (%d), must be at least two periods greater than the startheight (%d)", timeoutheight, startheight);


Why? Just one period should be enough...

Enforces this sentence in BIP 8:

timeoutheight must be at least 4096 blocks (2 retarget intervals) after startheight.

That's needed for lot=true/MUST_SIGNAL to work consistently with lot=false. Could also be solved by adding a transition directly from DEFINED to MUST_SIGNAL (skipping STARTED) I think.

Nah, probably not worth changing. One period is probably crazy anyway. And if we ever do need it, we can add the extra logic for it at that time.

luke-jr · 2021-03-25T22:35:32Z

src/rpc/blockchain.cpp

-    bip9.pushKV("timeout", consensusParams.vDeployments[id].nTimeout);
+    bip9.pushKV("startheight", consensusParams.vDeployments[id].startheight);
+    bip9.pushKV("timeoutheight", consensusParams.vDeployments[id].timeoutheight);
+    bip9.pushKV("minimum_activation_height", consensusParams.vDeployments[id].m_min_activation_height);


Probably should either hide this when 0, or use starttime+2*period

I think it's fine to always display.

harding · 2021-03-26T03:16:36Z

As of commit 2e55bce, lightly tested the likely outcomes of ST and confirmed that getblockchainfo returned the expected values whether activation was achieved or not, before and after the minimum_activation_period. I did not check close enough to detect off-by-one errors or other small details.

Tested creating a regtest chain with this branch and -vbparams=taproot:@144:@576:@1008, producing enough blocks to lockin and activate taproot, synced them to a taproot-oblivious 0.20.1 node, then upgraded that node to this branch with the same vbparams and confirmed that the upgraded node's getblockchaininfo results indicated it would be enforcing taproot.

I skimmed the code and didn't see any problems, but I don't feel confident enough in my reviewing abilities to say it definitely is fine.

maflcko

I looked at 2e55bce 🚴

Show signature and timestamp

Signature:

Timestamp of file with hash 9fd4467b454c576b05f9c797bd20cd8620ad58c2b63ca8dd4e36f27ec7059a14 -

maflcko · 2021-03-26T08:41:35Z

src/test/versionbits_tests.cpp

-    const Consensus::Params &mainnetParams = chainParams->GetConsensus();
+    const auto period = CreateChainParams(*m_node.args, CBaseChainParams::REGTEST)->GetConsensus().nMinerConfirmationWindow;
+    gArgs.ForceSetArg("-vbparams", strprintf("testdummy:@%s:@%s", period, period * 3));
+    const auto chainParams = CreateChainParams(*m_node.args, CBaseChainParams::REGTEST);


Any reason to use gArgs in the previous line, then m_node.args here? Might be better to just use a local symbol to avoid potentially polluting other tests

If I need to retouch

src/test/versionbits_tests.cpp

maflcko · 2021-03-26T08:44:37Z

src/chainparams.cpp

+    }
+
+    // Actual params must be on retarget block
+    if (startheight < 0) {


Any reason to not enforce startheight >= nMinerConfirmationWindow? Otherwise it is possible to violate the "2 period rule" by setting @0:@288.

If I need to retouch

maflcko · 2021-03-26T08:51:28Z

src/chainparams.cpp

+{
+    // Special always or never active cases
+    if ((startheight == Consensus::BIP9Deployment::NEVER_ACTIVE && timeoutheight == Consensus::BIP9Deployment::NEVER_ACTIVE)
+        || startheight == Consensus::BIP9Deployment::ALWAYS_ACTIVE) {


Then it could be ignored for NEVER as well or what is the reason to treat ALWAYS different from NEVER?

maflcko · 2021-03-26T09:01:53Z

src/versionbits.cpp

        switch (state) {
            case ThresholdState::DEFINED: {
-                if (pindexPrev->GetMedianTimePast() >= nTimeTimeout) {
-                    stateNext = ThresholdState::FAILED;


Any reason to not assert this transition can't happen?

I think that's being checked by the test cases.

The fuzz test has all transitions of the state machine implemented (in a "come from" form rather than a "go to" form). I don't think it's possible to make incorrect transitions that it won't detect.

devrandom · 2021-03-26T14:28:33Z

ACK 2e55bce

In addition to the testing in #21392 (comment) I also checked that the fuzzer detects various off-by-one errors I manually introduced into the implementation.

luke-jr

utACK 2e55bce

benthecarman · 2021-03-26T15:55:54Z

reACK 2e55bce

sipa

Code review ACK 2e55bce

sipa · 2021-03-27T01:21:47Z

src/chainparams.cpp

+{
+    // Special always or never active cases
+    if ((startheight == Consensus::BIP9Deployment::NEVER_ACTIVE && timeoutheight == Consensus::BIP9Deployment::NEVER_ACTIVE)
+        || startheight == Consensus::BIP9Deployment::ALWAYS_ACTIVE) {


@MarcoFalke I think that the rationale is that NO_TIMEOUT isn't a special value (unlike ALWAYS_ACTIVE and NEVER_ACTIVE), it's just a timestamp very far in the future.

sipa · 2021-03-27T01:22:41Z

src/rpc/blockchain.cpp

@@ -1243,8 +1242,8 @@ static void BIP9SoftForkDescPushBack(UniValue& softforks, const std::string &nam
    {
        bip9.pushKV("bit", consensusParams.vDeployments[id].bit);
    }
-    bip9.pushKV("start_time", consensusParams.vDeployments[id].nStartTime);
-    bip9.pushKV("timeout", consensusParams.vDeployments[id].nTimeout);
+    bip9.pushKV("startheight", consensusParams.vDeployments[id].startheight);


@benthecarman To what?

ajtowns · 2021-03-28T04:56:27Z

I think I'm an approach NACK, or at least approach -1, on using heights for activation at this point: they're incompatible with activating on testnet due to hash rate inconsistency, and they're incompatible with activation on signet due to custom signets being able to have completely random heights, since anyone can start a new one at any time. They're not especially convenient for activating on mainnet either: we're choosing timeframes and then converting that to expected block heights, but that can have significant errors -- if we want to give people "3 months" to review an update before deploying it, and choose block heights to reflect that, we risk both increased hashpower reducing that time substantially, or reduced hashpower introducing unwanted delays.

The main arguments I've seen for the height based approach is to avoid the signalling phase being skipped entirely, and to prevent a mandatory signalling phase (UASF/bip148/bip91/etc) from being skipped entirely, even in the event of a substantial loss of hashrate. But those goals can be achieved with the MTP approach as well -- see the top commit on https://github.com/ajtowns/bitcoin/commits/202103-bip9-uasf .

There's the additional benefit that it's simpler to explain "signalling begins at height X and ends at height Y" than "signalling begins at the first retarget period after time X, and ends in the last retarget period before time Y", but that seems like a much lower priority than being able to test activations on testnet and signet prior to activation on mainnet...

michaelfolkson · 2021-03-28T11:55:15Z

For the sake of respecting other reviewers' time, Taproot is already active on the default Signet for testing and experimentation with Taproot transactions. Activating Taproot on testnet seems like a very low priority, certainly a lower priority than ensuring the optimal code is merged for mainnet activation.

If you want a summary of the arguments for a consistent use of block height versus using a mix of block height and MTP there is this SE question with two answers, one from harding and one from me summarizing the arguments expressed in the two Speedy Trial PRs.

This appears to be the major differentiator between the two PRs. If you would prefer a consistent use of block height I would recommend reviewing this PR. If you would prefer a mix of block height and MTP I would recommend reviewing AJ's alternative Speedy Trial PR.

As I've communicated on AJ's PR, spreading review over two competing PRs is doing none of us any favors at this point.

michaelfolkson · 2021-03-28T12:35:09Z

Also this shows preferences for a consistent use of block height.

I don't have a strong view but it appears that @achow101 in #21392 and just by reading the comments in this PR @luke-jr @benthecarman @JeremyRubin @Sjors @roconnor-blockstream @harding all have a preference for a fully height based approach over any use of MTP.

If that isn't the case please correct me. And if anyone else has a view on entirely block height versus some limited use of MTP can you post it on IRC (##taproot-activation)?

The fuzzing PR looks very cool. On that PR @MarcoFalke also expresses an expectation that activation will be based on block heights.

edit: Some additional comments from today's Core dev meeting

"it comes down to mtp vs block height" @achow101

"the main difference between these two PRs are using block height vs MTP " @amitiuttarwar

"with mtp, we run the risk of losing 2 signaling periods. with already few signaling periods, this has the possibility of failing to activate due to bad luck" @achow101

"the property that achow101's PR improves is a fixed (number) of signals" @JeremyRubin

In addition @JeremyRubin stated here "I have a preference for fully height based design, correct."

This is becoming a farce, pure and simple.

benthecarman · 2021-03-28T12:48:22Z

I think I'm an approach NACK, or at least approach -1, on using heights for activation at this point: they're incompatible with activating on testnet due to hash rate inconsistency, and they're incompatible with activation on signet due to custom signets being able to have completely random heights, since anyone can start a new one at any time

I don't think test networks should be relevant to mainnet's activation mechanisms / consensus rules

jnewbery · 2021-03-28T19:46:59Z

src/versionbits.cpp

@@ -49,20 +57,17 @@ ThresholdState AbstractThresholdConditionChecker::GetStateFor(const CBlockIndex*
        pindexPrev = vToCompute.back();
        vToCompute.pop_back();

+        // We track state by previous-block, so the height we should be comparing is +1
+        const int64_t height = pindexPrev->nHeight + 1;


Why are you widening height from an int (32 bit) to a int64_t, and then comparing with ints below?

Will fix if I need to retouch.

jnewbery · 2021-03-28T20:00:02Z