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.

Looks like this has nothing to do with removing the chain param, but it is some 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 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:

{
  "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 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:

1. Wait a long time after activation (or failure)
2. Set activation height to 0 and list as burried in getdeploymentinfo (warn that entries are deprecated)
3. 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.

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):

$ for a in `seq 170059 170061` `seq 692260 692262`; do bitcoin-cli getdeploymentinfo @$a | jq -j '.height," ",.script_flags,"\n"'; done
170059 P2SH,TAPROOT,WITNESS
170060 
170061 P2SH,TAPROOT,WITNESS
692260 CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,TAPROOT,WITNESS
692261 CHECKLOCKTIMEVERIFY,CHECKSEQUENCEVERIFY,DERSIG,NULLDUMMY,P2SH,WITNESS
692262 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

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 53b62e3. 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 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).

It seems 2222ea8 wasn't doing that either despite the title. It removed consensus.vDeployments, but not consensus.TaprootHeight

There is no TaprootHeight in 2222ea8

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.

There is no TaprootHeight in 2222ea8

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.

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":

1. The "natural" process of the activation block ending up buried
2. 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".
3. 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" (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.