Bury taproot deployment

MarcoFalke commented at 1:43 pm on November 13, 2021: member

Now that the decision to activate taproot at block height 709632 has been buried under more than 5 months of POW, the deployment should be buried in a future release (see benefits below). If this patch is first included in version 23.0, the decision will be buried by ~10 months of POW at release time.

Benefits:

All deployments will use the same code paths. Thus, in the extremely unlikely case of bugs being introduced in the future in the versionbits activation code, it won’t have any effect on previous deployments.
Less code and complexity of the activation, because the versionbits params can be removed.
Features can be developed assuming that taproot absolutely does activate at a specific block height. Thus, code to handle non-activation of taproot does not need to be maintained.
Easier testing, because on regtest the newly introduced -testactivationheight=name@height (introduced in #22818) can be used.

For reference, previous buried versionbits deployments were:

#11398 (abandoned)
#12360 (abandoned 2)
#16060 (merged)

Unlike previous buried deployments, this one is not a non-backwards compatible change, as explained in BIP90 (section considerations): https://github.com/bitcoin/bips/blob/master/bip-0090.mediawiki#considerations . Due to the use of a minimum activation height, there can not exist an alternative chain where the activation occurred at a lower block height than that. Obviously this patch can still result in a theoretical consensus split (though a “backwards compatible” one) if there were a massive reorg that activated taproot at a higher (or no) block height. In this case the BIP90 considerations on large reorgs apply.

This changes the output of the getblockchaininfo RPC. I will add release notes when and if this patch is merged.

MarcoFalke renamed this:
🥕
Bury taproot deployment
on Nov 13, 2021

DrahtBot added the label Validation on Nov 13, 2021

in src/consensus/params.h:132 in 88884ae97d outdated

138+        case DEPLOYMENT_CSV: return CSVHeight;
139+        case DEPLOYMENT_SEGWIT: return SegwitHeight;
140+        case DEPLOYMENT_TAPROOT: return TaprootHeight;
141         } // no default case, so the compiler can warn about missing cases
142-        return std::numeric_limits<int>::max();
143+        assert(false);

katesalazar commented at 6:33 pm on November 13, 2021:

This looks like unreachable code is being replaced with unreachable code.

If the switch statement is replaced with an if-elsif-else statement, the unreachable code can be made go away.

katesalazar commented at 6:53 pm on November 13, 2021:

https://marcofalke.github.io/btc_cov/total.coverage/src/consensus/params.h.gcov.html

Make the last case branch a default branch and flip the unreachable assertion sitting idle into a reachable assertion normally doing a job.

Or do you prefer adding a test triggering the failure to assert?

MarcoFalke commented at 10:16 am on November 15, 2021:

We can’t use a default statement (or an else without condition), as explained in the comment one line above the one you commented on.

DrahtBot commented at 3:12 am on November 14, 2021: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#23508 (Add getdeploymentinfo RPC 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.

in src/chainparams.cpp:75 in 88884ae97d outdated

71@@ -72,6 +72,7 @@ class CMainParams : public CChainParams {
72         consensus.BIP66Height = 363725; // 00000000000000000379eaa19dce8c9b722d46ae6a57c2f1a988119488b50931
73         consensus.CSVHeight = 419328; // 000000000000000004a1b34462cb8aeebd5799177f7a29cf28f2d1961716b5b5
74         consensus.SegwitHeight = 481824; // 0000000000000000001c8018d9cb3b742ef25114f27563e3fc4a1902167f9893
75+        consensus.TaprootHeight = 709632;

achow101 commented at 9:14 pm on November 15, 2021:

In 88884ae97dee3ea915a35612531f62d1468b911b “Bury taproot deployment”

For consistency with the other buried deployments, we should have the hash listed here as a comment. My node has 0000000000000000000687bca986194dc2c1f949318629b44bb54ec0a94d8244 as block 709632.

MarcoFalke commented at 11:03 am on November 17, 2021:

The block hash was not available when I opened the pull request, but I will include it on the next force push.

MarcoFalke commented at 4:05 pm on November 30, 2021:

Thanks, fixed

JeremyRubin commented at 9:16 pm on November 15, 2021: contributor

Is there an earlier block height we can use?

achow101 commented at 9:20 pm on November 15, 2021: member

ACK 88884ae97dee3ea915a35612531f62d1468b911b

achow101 commented at 11:50 pm on November 15, 2021: member

Is there an earlier block height we can use?

It seems that 692262 is the earliest block height.

JeremyRubin commented at 0:13 am on November 16, 2021: contributor

if it’s just that block with an invalid spend we could do BIP340Exception for that block hash and always on since genesis?

MarcoFalke commented at 11:04 am on November 17, 2021: member

if it’s just that block with an invalid spend we could do BIP340Exception for that block hash and always on since genesis?

Proposed in https://github.com/bitcoin/bitcoin/pull/23536

Sjors commented at 1:47 pm on November 18, 2021: member

if it’s just that block with an invalid spend we could do BIP340Exception for that block hash and always on since genesis?

This would also have my preference.

MarcoFalke commented at 2:00 pm on November 18, 2021: member

This would also have my preference.

I wouldn’t call it “preference”. #23536 and this pull request are doing two different things. One is setting the script verification flags starting from genesis, regardless of the activation status. This one is turning the activation into a height based one. If #23536 is merged first, this one will be an RPC-only change.

theStack approved

theStack commented at 3:14 pm on November 30, 2021: member

Concept and code-review ACK 88884ae97dee3ea915a35612531f62d1468b911b

refactor: Use assert(false) to document unreachable deployment code

It is impossible to hit those lines of code unless there is hardware or
software corruption. Thus, make sure to abort the node in case they are
hit to avoid further corruption.

See also the previous discussions:

* https://github.com/bitcoin/bitcoin/pull/19438/files#r661202215
* https://github.com/bitcoin/bitcoin/pull/19438/files#r662494456

Also, remove whitespace for better readability and grep-ability. (Can be
reviewed with --word-diff-regex=. option)

faef3cbe0a

gruve-p commented at 3:50 pm on November 30, 2021: contributor

ACK https://github.com/bitcoin/bitcoin/pull/23505/commits/88884ae97dee3ea915a35612531f62d1468b911b

Bury taproot deployment fa73540108

MarcoFalke force-pushed on Nov 30, 2021

MarcoFalke commented at 4:08 pm on November 30, 2021: member

Force pushed with two changes:

Rebased to get rid of already removed references to DEPLOYMENT_TAPROOT in master (makes review easier, as the effects are smaller).
Added the block hash of the activation height in a comment, as the chain has passed the activation height since this pull was opened.

Should be easy to re-review.

theStack approved

theStack commented at 4:26 pm on November 30, 2021: member

re-ACK fa7354010811c81780530d3bd5032ff6b9473153

Trivial re-review via git range-diff 88884ae9...fa735401 plus verified that the block hash of height 709632 matches on my node.

luke-jr commented at 7:43 pm on November 30, 2021: member

the decision will be buried by ~10 months of POW at release time.

POW/miners do not decide protocol rules. It would be nice to see more nodes enforcing Taproot first. OTOH, this software will activate Taproot based on the POW regardless of user enforcement, so maybe a separate issue. shrug

MarcoFalke commented at 7:48 pm on November 30, 2021: member

OTOH, this software will activate Taproot based on the POW regardless of user enforcement, so maybe a separate issue. shrug

Indeed, this is a separate discussion.

MarcoFalke commented at 9:50 am on December 7, 2021: member

I don’t think this will be needed, since taproot activated at the genesis block after #23536

MarcoFalke closed this on Dec 7, 2021

MarcoFalke deleted the branch on Dec 7, 2021

MarcoFalke commented at 9:51 am on December 7, 2021: member

I’ll likely open a follow-up after #23536 as an alternative to this pull.

DrahtBot locked this on Dec 23, 2022

Bury taproot deployment #23505

Conflicts