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

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/31625.

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:

• #31117 (miner: Reorg Testnet4 minimum difficulty blocks by fjahr)
• #26201 (Remove Taproot activation height by Sjors)

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.

Not sure about this. If someone goes back in the main chain to an earlier block to mine on top of it for testing (or fun), this could lead to the mined block being rejected from the own node. Conceptually, there is an argument to be self-consistent, but I am not sure how to weight that against removing the lines of code.

the mined block being rejected from the own node

Currently getblocktemplate has a guard on mainnet against miner.isInitialBlockDownload(), which among other things checks MinimumChainWork. So without patching the code you can't generate a pre-segwit block using getblocktemplate.

But if you patched that check away, why would a SegWit block be rejected?

You'd also need a (bunch of) ASIC(s) for such an experiment.

But if you patched that check away, why would a SegWit block be rejected?

Witness data is not allowed per consensus rules before segwit activation.

Ok, so someone could roll back their node to before SegWit activation, patch getblocktemplate to skip the IsInitialBlockDownload() check (or remove the MinimumChainWork and max_tip_age checks from from the latter).

If they then call getblocktemplate and if they have any SegWit transactions in their mempool (I didn't check if that's actually possible), it would generate a template with SegWit transactions and a commitment. Since it verifies the template at the end, the RPC call would fail with unexpected-witness.

They could get around that by changing consensus.SegwitHeight to 0, if their goal is to write an alternative history where SegWit was always active. If instead they want to create an alternative history where SegWit never activated, they would need to exclude SegWit transactions from their mempool. If on top of that they want to make a statement by including SegWit-violating transactions, it would be a bit more complicated. I don't think that's a use-case we need to cater for though.

Ok, so someone could roll back their node to before SegWit activation, patch getblocktemplate to skip the IsInitialBlockDownload() check (or remove the MinimumChainWork and max_tip_age checks from from the latter).

I don't think a patch is needed. ibd will latch, so in theory one could use that. I am not claiming this is a valid use case, just that internal self-consistency was the reason why this hasn't been done previously, IIRC.

It would be good if someone has a link to the earlier reasoning.

Not sure if it was ever mentioned publicly where a link exists.

Obviously, if you want to reproduce the inconsistency, it would be easier in regtest, and the change may theoretically break someone's test deployment.

It seems useful to illustrate the change here by regenerating the alternative mainnet blocks introduced in #31583, switching their coinbase outputs to taproot addresses. Before this PR the last block, which spends a coinbase output, would fail with unexpected-witness.

Marking as draft when that lands.

It would also seem more consistent to drop the unexpected-witness check entirely, rather than just change the miner code. That would dramatically increase the scope of this PR though. Leaving up for grabs for now.