Remove taproot chain param and list exception blocks in getdeploymentinfo #26162
Conversation
Co-Authored-By: MarcoFalke <falke.marco@gmail.com> Co-Authored-By: Anthony Towns <aj@erisian.com.au>
Co-Authored-By: Anthony Towns <aj@erisian.com.au>
Co-Authored-By: Anthony Towns <aj@erisian.com.au>
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ConflictsReviewers, this pull request conflicts with the following ones:
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. |
|
Looks like this has nothing to do with removing the chain param, but it is som 8000 e kind of consensus change? |
|
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 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:
EDIT: oh, what the looks like: {
"hash": "000000000000000000051d293d02f6791592fdc53012caa5dbf61cb3a0f4026d",
"height": 755873,
"script_flags": "CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,TAPROOT,WITNESS",
"deployments": {
...
"bip16": {
"type": "buried",
"buried": {
"exceptions": [
{
"height": 170060,
"hash": "00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22",
"applied": true,
"script_flags": ""
}
]
},
"active": true,
"height": 0
},
"taproot": {
"type": "buried",
"buried": {
"exceptions": [
{
"height": 692261,
"hash": "0000000000000000000f14c35b2d841e986ab5441de8c585d5ffe55ea1e395ad",
"applied": true,
"script_flags": "CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,WITNESS"
},
{
"height": 170060,
"hash": "00000000000002dc756eebf4f49723ed8d30cc28a5f108eb94b1ba88ac4f9c22",
"applied": true,
"script_flags": ""
}
]
},
"active": true,
"height": 0
}
}
} |
|
Not sure if I want to overhaul Therefore it seems better to list exception blocks outside the I'm tempted to completely drop
This is more in the direction @MarcoFalke was going, but systematically rather than only for Taproot. However, going through the list of
Even if we can't remove an entry from 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. |
CLTV isn't/wasn't enforced around the heights of the P2SH exception (and likewise isn't excepted for the taproot block):
Huh? You can't remove buried deployment from consensus/params; they're consensus critical...
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, |
Indeed, I got confused in 53b62e3. BIP34 (height in coinbase), BIP65 (CLTV), BIP66 (DER) and CSV all activated after BIP16 (p2sh).
Ah, BIP 147, which is not buried (and not listed by
Then they're not really buried. It seems 2222ea8 wasn't doing that either despite the title. It removed |
It's listed as "segwit", as per BIP 147's "Deployment" section:
The way "buried" is used in consensus/params.h (
|
Ah you're right, and it's not in @ajtowns reworked version ajtowns@809a545 either. I must have introduced this myself in a0edf54 in order to make the RPC result sane.
I don't think I used anything from that commit.
There's three possible interpretations of "buried":
(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" (1c93b9b, #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. |
|
Looks like anything that would come is based on 2222ea8, so my preference would be to just go ahead with that and leave other stuff to follow-ups. |
|
@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. |
Takes over #24737.
Now that the commit (7c08d81) 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
getdeploymentinfoRPC now showstaprootas buried, as it already did forsegwitand earlier softforks.In addition,
getdeploymentinfohas a newexceptionsarray underburiedif 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 thatbip34was listed even though it's older thanbip16.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 todeploymentinfo.h. This paves the way to dropping the other buried BIPs fromChainParamstoo later.