Remove taproot chain param and list exception blocks in getdeploymentinfo

Sjors commented at 4:36 pm on September 23, 2022: member

Takes over #24737.

Now that the commit (https://github.com/bitcoin/bitcoin/commit/7c08d81e119570792648fe95bbacddbb1d5f9ae2) which changes taproot to be enforced for all blocks is sufficiently buried by other commits, and thus less likely to be reverted, it seems a good time to remove no longer needed non-consensus code.

The getdeploymentinfo RPC now shows taproot as buried, as it already did for segwit and earlier softforks.

In addition, getdeploymentinfo has a new exceptions array under buried if there are any exceptions.

Finally, for completeness the P2SH BIP16 soft fork and its exception block is added to getdeploymentinfo. This also makes sense given that bip34 was listed even though it’s older than bip16.

TODO: the current version partly reverts #11739, which seems like going in the wrong direction. So rather than stuffing BIP16 and Taproot activation height into the consensus critical ChainParams, I plan to move them to deploymentinfo.h. This paves the way to dropping the other buried BIPs from ChainParams too later.

Remove taproot chain param

Co-Authored-By: MarcoFalke <falke.marco@gmail.com>
Co-Authored-By: Anthony Towns <aj@erisian.com.au>

a0edf54e71

rpc: list exception blocks for buried deployments

Co-Authored-By: Anthony Towns <aj@erisian.com.au>

53b62e301b

Document p2sh exceptions

Co-Authored-By: Anthony Towns <aj@erisian.com.au>

277f84c78b

DrahtBot commented at 4:27 pm on September 24, 2022: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26177 (refactor / kernel: Move non-gArgs chainparams functionality to kernel by TheCharlatan)

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.

MarcoFalke commented at 10:07 pm on September 25, 2022: member

Looks like this has nothing to do with removing the chain param, but it is some kind of consensus change?

glozow added the label Consensus on Sep 26, 2022

ajtowns commented at 3:35 am on September 27, 2022: contributor

I don’t think there’s any point reporting the historical activation height for p2sh/taproot if they’re not things we use in our code. As far as our actual code is concerned, p2sh/taproot were activated at genesis, with some exceptions – so if that’s what we’re doing, that’s what we should report. If we did want to document the historical height, even though we don’t enforce it, I think that should just go in deploymentinfo.h (and rpc/blockchain.cpp), and not touch consensus/params.h.

I had a bit more of a poke around at this in https://github.com/ajtowns/bitcoin/commits/202209-forkexceptionreporting . My thinking ended up at:

we could report the current GetBlockScriptFlags() in getdeploymentinfo. that more directly exposes things like NULLDUMMY getting enforced as of block 481824, which seems like it has some value.
once we were doing that, changing Consensus::Params::script_flag_exceptions to be indexed by height, rather than hash would be easy to verify – run getdeploymentinfo against each block, and check that that none of the values have changed. not the most speedy thing ever, but easier than every reviewer running a full reindex for both mainnet and testnet…
once you’ve got the exceptions indexed by height, you can easily report on whether the exception actually appeared in the current tip’s history or not, which seems important if you’re reporting exceptions to consensus rules

EDIT: oh, what the looks like:

 0{
 1  "hash": "000000000000000000051d293d02f6791592fdc53012caa5dbf61cb3a0f4026d",
 2  "height": 755873,
 3  "script_flags": "CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,TAPROOT,WITNESS",
 4  "deployments": {
 5    ...
 6    "bip16": {
 7      "type": "buried",
 8      "buried": {
 9        "exceptions": [
10          {
11            "height": 170060,
12            "hash": "00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22",
13            "applied": true,
14            "script_flags": ""
15          }
16        ]
17      },
18      "active": true,
19      "height": 0
20    },
21    "taproot": {
22      "type": "buried",
23      "buried": {
24        "exceptions": [
25          {
26            "height": 692261,
27            "hash": "0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad",
28            "applied": true,
29            "script_flags": "CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,WITNESS"
30          },
31          {
32            "height": 170060,
33            "hash": "00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22",
34            "applied": true,
35            "script_flags": ""
36          }
37        ]
38      },
39      "active": true,
40      "height": 0
41    }
42  }
43}

MarcoFalke removed the label Consensus on Sep 27, 2022

Sjors commented at 8:00 am on September 28, 2022: member

Not sure if I want to overhaul script_flag_exceptions unless there’s a really good reason for it. The way these exception blocks work is quite separate from the actual soft fork activation. This is especially obvious with the P2SH exception, which also acts as a CHECKLOCKTIMEVERIFY exception, even though afaik that opcode can be used in bare script (i.e. without p2sh).

Therefore it seems better to list exception blocks outside the deployments dictionary.

I’m tempted to completely drop buried entries from the deployments dictionary once it is removed from consensus/params.h. So the sequence of burying a soft fork would be something like:

Wait a long time after activation (or failure)
Set activation height to 0 and list as burried in getdeploymentinfo (warn that entries are deprecated)
Delete entirely from consensus/params.h in some later release

This is more in the direction @MarcoFalke was going, but systematically rather than only for Taproot. However, going through the list of BIP...Height Taproot does seem the only one we can easily drop this way:

afaik BIP34Height (height in coinbase), BIP65Height (CLTV), BIP66Height (DER) and CSVHeight can’t be dropped, unless we add lots of exception blocks
dropping SegwitHeight would need a refactor of NeedsRedownload() (e.g. check for the presence of a commitment?). Possibly not with the extra complexity.

Even if we can’t remove an entry from consensus/params.h we could still drop the entry from getdeploymentinfo. Right now it’s an incomplete list (which is why I added P2SH to it in this draft PR).

I do think we should document the historical activation blocks and their height, as right now this involves Google and/or looking through old commits. But this may be easier to do in a markdown file.

ajtowns commented at 5:20 pm on September 28, 2022: contributor

Not sure if I want to overhaul script_flag_exceptions unless there’s a really good reason for it. The way these exception blocks work is quite separate from the actual soft fork activation. This is especially obvious with the P2SH exception, which also acts as a CHECKLOCKTIMEVERIFY exception, even though afaik that opcode can be used in bare script (i.e. without p2sh).

CLTV isn’t/wasn’t enforced around the heights of the P2SH exception (and likewise isn’t excepted for the taproot block):

0$ for a in `seq 170059 170061` `seq 692260 692262`; do bitcoin-cli getdeploymentinfo @$a | jq -j '.height," ",.script_flags,"\n"'; done
1170059 P2SH,TAPROOT,WITNESS
2170060 
3170061 P2SH,TAPROOT,WITNESS
4692260 CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,TAPROOT,WITNESS
5692261 CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,WITNESS
6692262 CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,TAPROOT,WITNESS

Therefore it seems better to list exception blocks outside the deployments dictionary.

I’m tempted to completely drop buried entries from the deployments dictionary once it is removed from consensus/params.h.

Huh? You can’t remove buried deployment from consensus/params; they’re consensus critical…

* dropping `SegwitHeight` would need a refactor of `NeedsRedownload()` (e.g. check for the presence of a commitment?). Possibly not with the extra complexity.

SegwitHeight is used for enabling SCRIPT_VERIFY_NULLDUMMY as well.

I do think we should document the historical activation blocks and their height, as right now this involves Google and/or looking through old commits. But this may be easier to do in a markdown file.

The obvious place to document historical activation heights and exceptions would be to via an update to BIP 90 (presumably with a new bip number)…

Cheers, aj

Sjors commented at 5:51 pm on September 28, 2022: member

CLTV isn’t/wasn’t enforced around the heights of the P2SH exception (and likewise isn’t excepted for the taproot block)

Indeed, I got confused in 53b62e301b1a8552fc5906bd3b7697b4182d5ae9. BIP34 (height in coinbase), BIP65 (CLTV), BIP66 (DER) and CSV all activated after BIP16 (p2sh).

SegwitHeight is used for enabling SCRIPT_VERIFY_NULLDUMMY as well.

Ah, BIP 147, which is not buried (and not listed by getdeploymentinfo)

You can’t remove buried deployment from consensus/params; they’re consensus critical…

Then they’re not really buried. It seems https://github.com/bitcoin/bitcoin/pull/24737/commits/2222ea8612ba21a27239c41dd77cd2e92d25037b wasn’t doing that either despite the title. It removed consensus.vDeployments, but not consensus.TaprootHeight, even though the latter is not used in consensus code afaik, i.e. Taproot can be buried (I think).

MarcoFalke commented at 6:17 pm on September 28, 2022: member

It seems https://github.com/bitcoin/bitcoin/commit/2222ea8612ba21a27239c41dd77cd2e92d25037b wasn’t doing that either despite the title. It removed consensus.vDeployments, but not consensus.TaprootHeight

There is no TaprootHeight in 2222ea8612ba21a27239c41dd77cd2e92d25037b

ajtowns commented at 1:39 am on September 29, 2022: contributor

SegwitHeight is used for enabling SCRIPT_VERIFY_NULLDUMMY as well.

Ah, BIP 147, which is not buried (and not listed by getdeploymentinfo)

It’s listed as “segwit”, as per BIP 147’s “Deployment” section: The BIP will be deployed .. using the same parameters for BIP141 .. with the name "segwit"

You can’t remove buried deployment from consensus/params; they’re consensus critical…

Then they’re not really buried.

The way “buried” is used in consensus/params.h (enum BuriedDeployment) and in BIP 90 (“Title: Buried Deployments”) is to refer to a soft fork that has been activated sufficiently long ago (ie, been “buried” under a lot of proof of work) that a reorg of sufficient depth to trigger activation at some other height (or to prevent activation entirely) is vanishingly unlikely.

It seems 2222ea8 wasn’t doing that either despite the title. It removed consensus.vDeployments, but not consensus.TaprootHeight, even though the latter is not used in consensus code afaik, i.e. Taproot can be buried (I think).

TaprootHeight was proposed to be introduced in #23505, but that PR was obsoleted by #23536.

Sjors commented at 8:02 am on September 29, 2022: member

There is no TaprootHeight in 2222ea8

Ah you’re right, and it’s not in @ajtowns reworked version https://github.com/ajtowns/bitcoin/commit/809a545d6e9abb106d5345f3a98ebc7b4031631b either. I must have introduced this myself in a0edf54e71290b571d77de885f1a6c7c8cfd4285 in order to make the RPC result sane.

TaprootHeight was proposed to be introduced in #23505, but that PR was obsoleted by #23536

I don’t think I used anything from that commit.

The way “buried” is used in consensus/params.h (enum BuriedDeployment) and in BIP 90 (“Title: Buried Deployments”) is to refer to a soft fork that has been activated sufficiently long ago (ie, been “buried” under a lot of proof of work) that a reorg of sufficient depth to trigger activation at some other height (or to prevent activation entirely) is vanishingly unlikely.

There’s three possible interpretations of “buried”:

The “natural” process of the activation block ending up buried
The act of hardcoding it, as the comment in consensus/params.h continues “the height of the activation has been hardcoded into the client implementation”.
Applying the rules all the way back to genesis

(3) is what I had in mind with the term, but the intention of BIP 90 seems to be either (1) or (2)

It would be good to have separate terms for these three things. We have previously called (3) “enforce from genesis” (#24567) or “enforced when [some condition other than height]” (#23536). The word “bury” (https://github.com/bitcoin/bitcoin/commit/1c93b9b31c2ab7358f9d55f52dd46340397c906d, #23505) “buried” have been used to indicate (2). But “buried” has also been used in the sense of (1), e.g. in #23536.

This makes me think we need a need term for (2) so we can keep using the more ambiguous “buried” for (1). Perhaps “hardcode height of”, “pin” or “demarcate” to stay closer the archeology analogy (demarcation line, demarcation height).

Closing this PR because it’s too much of a mess. I might reopen when I have more clarity on what would be useful to achieve here.

Sjors closed this on Sep 29, 2022

MarcoFalke commented at 9:40 am on September 29, 2022: member

Looks like anything that would come is based on https://github.com/bitcoin/bitcoin/commit/2222ea8612ba21a27239c41dd77cd2e92d25037b, so my preference would be to just go ahead with that and leave other stuff to follow-ups.

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

@MarcoFalke trying that in #26201, but with a bit more source and code comment and RPC help to (differently) explain the rationale for dropping taproot from the RPC result.

bitcoin locked this on Sep 29, 2023

Remove taproot chain param and list exception blocks in getdeploymentinfo #26162

Conflicts