Remove Taproot activation height

Sjors commented at 11:16 am on September 29, 2022: member

Drop DEPLOYMENT_TAPROOT from consensus.vDeployments.

Now that the commit (https://github.com/bitcoin/bitcoin/commit/7c08d81e119570792648fe95bbacddbb1d5f9ae2) which changes taproot to be enforced for all blocks is included in v24.0, it seems a good time to remove no longer needed non-consensus code.

Clarify what is considered a BuriedDeployment.

We drop taproot from getdeploymentinfo RPC, rather than mark it as buried, because this is not a buried deployment in the sense of BIP 90. This is because the activation height has been completely removed from the code, rather than hard coded.

See discussion in #24737 and #26162.

DrahtBot commented at 2:34 pm on September 30, 2022: 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. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#29039 (versionbits refactoring 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.

luke-jr commented at 2:04 pm on October 2, 2022: member

Concept ~0, but I object the claim in general that commits “sufficiently buried by other commits, and thus less likely to be reverted”. Git isn’t PoW, and “burying” commits does not make them less likely to be reverted.

Sjors commented at 9:22 am on October 3, 2022: member

@luke-jr changed the wording to “is (probably) included in v24.0”.

ajtowns commented at 3:47 am on October 8, 2022: contributor

This should be updating MinBIP9WarningHeight above the taproot activation height (otherwise you should see "warnings": "Unknown new rules activated (versionbit 2)" due to miner signalling)

Sjors force-pushed on Nov 1, 2022

Sjors commented at 9:41 am on November 1, 2022: member

@ajtowns done

in src/consensus/params.h:99 in 8f96e52405 outdated

 95@@ -94,7 +96,7 @@ struct Params {
 96      * BIP 16 exception blocks. */
 97     int SegwitHeight;
 98     /** Don't warn about unknown BIP 9 activations below this height.
 99-     * This prevents us from warning about the CSV and segwit activations. */
100+     * This prevents us from warning about the CSV,segwit and taproot activations. */

maflcko commented at 9:48 am on November 1, 2022:

0     * This prevents us from warning about the CSV, segwit and taproot activations. */

nit

in doc/release-notes-26201.md:4 in 8f96e52405 outdated

0@@ -0,0 +1,5 @@
1+RPC
2+---
3+
4+- 'taproot' has been removed from 'getdeploymentinfo', rather than marked as buried,

maflcko commented at 9:49 am on November 1, 2022:

0- 'taproot' has been removed from 'getdeploymentinfo',

nit: I think release notes should describe changes, not provide extended discussion about alternatives

Sjors force-pushed on Nov 1, 2022

DrahtBot added the label Needs rebase on Mar 16, 2023

achow101 commented at 3:23 pm on April 25, 2023: member

Are you still working on this?

Sjors commented at 2:12 pm on May 8, 2023: member

I’ll rebase soon(tm).

in src/deploymentinfo.cpp:19 in 20ef70f750 outdated

10@@ -11,10 +11,6 @@ const struct VBDeploymentInfo VersionBitsDeploymentInfo[Consensus::MAX_VERSION_B
11         /*.name =*/ "testdummy",
12         /*.gbt_force =*/ true,
13     },
14-    {
15-        /*.name =*/ "taproot",
16-        /*.gbt_force =*/ true,
17-    },

luke-jr commented at 0:54 am on July 27, 2023:

This will need aRules in rpc/mining.cpp updated

luke-jr changes_requested

Sjors force-pushed on Aug 18, 2023

in src/rpc/mining.cpp:866 in 7fb3287fd1 outdated

860@@ -861,7 +861,10 @@ static RPCHelpMan getblocktemplate()
861 
862     UniValue aRules(UniValue::VARR);
863     aRules.push_back("csv");
864-    if (!fPreSegWit) aRules.push_back("!segwit");
865+    if (!fPreSegWit) {
866+        aRules.push_back("!segwit");
867+        aRules.push_back("!taproot");

Sjors commented at 10:30 am on August 18, 2023:

@luke-jr like this?

This will need aRules in rpc/mining.cpp updated

Sjors commented at 10:50 am on August 18, 2023:

Oh wait, without !

I’ll push a fix and add a test…

Before and after this change it should be:

0bitcoin-cli getblocktemplate '{"rules": ["segwit"]}' | jq '.rules'
1[
2  "csv",
3  "!segwit",
4  "taproot"
5]

DrahtBot removed the label Needs rebase on Aug 18, 2023

Sjors force-pushed on Aug 18, 2023

Sjors commented at 11:12 am on August 18, 2023: member

Rebased after kernel changes. Fixed a regression in getblocktemplate’s rules result, and added a test for tit.

in src/rpc/mining.cpp:892 in b5d8ddbcb9 outdated

859@@ -860,8 +860,14 @@ static RPCHelpMan getblocktemplate()
860     result.pushKV("capabilities", aCaps);
861 
862     UniValue aRules(UniValue::VARR);
863+    // See getblocktemplate changes in BIP 9:
864+    // ! indicates a more subtle change to the block structure or generation transaction
865+    // Otherwise clients may assume the rule will not impact usage of the template as-is.
866     aRules.push_back("csv");
867-    if (!fPreSegWit) aRules.push_back("!segwit");
868+    if (!fPreSegWit) {

Sjors commented at 11:16 am on August 18, 2023:

In #27433 I propose getting rid of fPreSegWit.

DrahtBot added the label CI failed on Sep 12, 2023

DrahtBot removed the label CI failed on Sep 13, 2023

DrahtBot added the label Needs rebase on Oct 9, 2023

Sjors commented at 2:41 pm on October 9, 2023: member

Rebased after #28591.

Sjors force-pushed on Oct 9, 2023

DrahtBot removed the label Needs rebase on Oct 9, 2023

in src/consensus/params.h:34 in db4344fa9c outdated

32 };
33 constexpr bool ValidDeployment(BuriedDeployment dep) { return dep <= DEPLOYMENT_SEGWIT; }
34 
35 enum DeploymentPos : uint16_t {
36     DEPLOYMENT_TESTDUMMY,
37-    DEPLOYMENT_TAPROOT, // Deployment of Schnorr/Taproot (BIPs 340-342)

maflcko commented at 2:43 pm on October 20, 2023:

Maybe add a note here that removing an entry may require bumping MinBIP9WarningHeight?

Sjors commented at 7:38 am on October 23, 2023:

Added. IIUC the WarningBitsConditionChecker uses ComputeBlockVersion (also used by the miner), which in turn uses VersionBitsConditionChecker, which uses vDeployments to determine if a softfork is STARTED or LOCKED_IN. If so it won’t issue the warning. This logic is rather convoluted…

Sjors force-pushed on Oct 23, 2023

DrahtBot added the label CI failed on Oct 23, 2023

DrahtBot removed the label CI failed on Oct 26, 2023

barrystyle referenced this in commit 6eb671c734 on Dec 16, 2023

Sjors force-pushed on Jan 9, 2024

Sjors commented at 9:06 am on January 9, 2024: member

Rebased because #27433 needed it.

DrahtBot added the label CI failed on Jan 14, 2024

Sjors force-pushed on Feb 13, 2024

DrahtBot removed the label CI failed on Feb 13, 2024

DrahtBot added the label Needs rebase on Mar 5, 2024

Sjors force-pushed on Mar 7, 2024

Sjors commented at 10:36 am on March 7, 2024: member

Rebased due to (trivial) merge conflict after #29547.

I should also point out that Silent Payments makes use of the Taproot activation height (e.g. for a more efficient indexer), but I don’t think that’s a good reason to keep these consensus params around.

DrahtBot removed the label Needs rebase on Mar 7, 2024

Sjors force-pushed on May 30, 2024

DrahtBot added the label CI failed on May 30, 2024

DrahtBot removed the label CI failed on May 30, 2024

DrahtBot added the label CI failed on Jul 15, 2024

DrahtBot removed the label CI failed on Jul 20, 2024

DrahtBot added the label Needs rebase on Aug 5, 2024

Sjors force-pushed on Aug 12, 2024

Sjors commented at 8:38 am on August 12, 2024: member

Rebased after #30560 and #29775.

DrahtBot removed the label Needs rebase on Aug 12, 2024

DrahtBot added the label Needs rebase on Aug 22, 2024

Sjors force-pushed on Aug 26, 2024

Sjors commented at 8:42 am on August 26, 2024: member

Rebased after #30658.

DrahtBot removed the label Needs rebase on Aug 26, 2024

DrahtBot added the label CI failed on Sep 12, 2024

DrahtBot removed the label CI failed on Sep 15, 2024

Remove Taproot activation height

Drop DEPLOYMENT_TAPROOT from consensus.vDeployments.

Bump MinBIP9WarningHeight.

Clarify what is considered a BuriedDeployment and
drop taproot from getdeploymentinfo RPC.

Add a test to getblocktemplate to ensure the taproot
rule is still set.

Co-Authored-By: MarcoFalke <*~=`'#}+{/-|&$^_@721217.xyz>

9fc563be38

Sjors force-pushed on Sep 17, 2024

Sjors commented at 7:42 am on September 17, 2024: member

#27433 needed the rebase here

Sjors commented at 8:10 am on September 18, 2024: member

Removed ~ from the description (wasn’t relevant anymore anyway), since @jonatack pointed out in #28358 that these don’t get rendered nicely in merges.

DrahtBot added the label CI failed on Oct 25, 2024

DrahtBot removed the label CI failed on Oct 25, 2024

Remove Taproot activation height #26201

Code Coverage

Reviews

Conflicts