Enforce BIP 68 from genesis

maflcko commented at 1:53 pm on March 15, 2022: member

Now that BIP 68 is active, it makes sense to enforce its rules on all blocks, even historic ones, regardless of the deployment status.

Benefits:

BIP 68 rules are applied for all blocks (even blocks not yet created). This may benefit static analysis, code review, and development of new script features that build on BIP 68.
Any future bugs introduced in the deployment code won’t have any effect on the BIP 68 rules, as they are independent of deployment.
Enforcing the BIP 68 rules regardless of the deployment status makes testing easier because invalid blocks after activation are also invalid before activation. So there is no need to differentiate the two cases.
It gives belt-and-suspenders protection against a practically expensive and theoretically impossible IBD reorg attack where the node is eclipsed. nMinimumChainWork makes this attack practically expensive. Also, this attack is currently theoretically impossible because no vector is known to inject a consensus-invalid transaction into the mempool (or even have it mined).

For reference, previously something similar was done for the script verification flags P2SH and WITNESS in commit 0a8b7b4b33c9d78574627fc606267e2d8955cd1c. (And is being done for TAPROOT in #23536).

Considerations

Obviously this change can lead to consensus splits on the network in light of massive reorgs. Currently the last block before BIP 68 activation, that is the last block without the BIP 68 rules applied, is buried by a few months of POW. BIP90 considerations apply when looking at reorgs this large.

maflcko commented at 1:53 pm on March 15, 2022: member

Please review #24565 first, while this stays in draft.

maflcko added the label Consensus on Mar 15, 2022

ajtowns commented at 2:06 pm on March 15, 2022: contributor

Doesn’t seem to save much code, and requires revalidating years worth of blocks to ensure it doesn’t break consensus. Not really seeing the point of this? Concept -1 from me.

maflcko force-pushed on Mar 15, 2022

maflcko commented at 7:01 pm on March 15, 2022: member

Doesn’t seem to save much code

Apologies, the motivation isn’t to save code. I’ve added some text to OP.

ajtowns commented at 7:49 pm on March 15, 2022: contributor

Unlike #23536 I don’t think this has much (any?) possible effect on wallet behaviour; bip68 only allows time locks up to ~15 months, and bip68 was ~68 months ago (slightly more than “a few months of POW”…), and unlike with taproot/segwit/p2sh any transaction that is invalid right now due to bip68, will eventually become valid.

Also, any static analysis tool that can’t cope with conditional consensus logic just doesn’t seem suitable for use on the bitcoin codebase in the first place to me.

So still -1 from me; these sorts of unnecessary changes around consensus code seem far more likely to introduce bugs (as #24421 has) than prevent them.

maflcko commented at 7:32 am on March 16, 2022: member

If a BIP 68 violating tx makes it into the mempool in current master (or a previous release), getblocktemplate will refuse to create a block on the current main chain tip. This pull doesn’t change that. It enforces BIP 68 rules on all blocks (past and future ones) and block templates. I think this is a motivation for the changes here, since a block template that includes a BIP 68 violation on current master will happily go through for blocks that will never end up in the main chain (built on a stale tip, see the unit test) and it will fail for blocks that are built on the current tip.

ajtowns commented at 12:31 pm on March 16, 2022: contributor

If you’re relying on someone giving you a bip68-invalid tx to avoid building blocks with a tip that’s over 5 years out of date, I think you’ve already lost…

jnewbery commented at 4:46 pm on March 17, 2022: contributor

So still -1 from me; these sorts of unnecessary changes around consensus code seem far more likely to introduce bugs (as #24421 has) than prevent them.

24421 doesn’t change consensus code, so I don’t think it’s a relevant comparison here. The bug introduced in that PR was a functional test simulating an impossible scenario.

DrahtBot commented at 3:15 pm on March 19, 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:

#25311 (remove CBlockIndex copy construction by jamesob)
#25073 (test: Cleanup miner_tests by MarcoFalke)
#23897 (refactor: Move calculation logic out from CheckSequenceLocksAtTip() by hebasto)

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.

DrahtBot added the label Needs rebase on May 6, 2022

Enforce BIP 68 from genesis 218b5a479e

Remove unused LOCKTIME_VERIFY_SEQUENCE 2e4e8a8828

maflcko force-pushed on Jun 29, 2022

DrahtBot removed the label Needs rebase on Jun 29, 2022

DrahtBot added the label Needs rebase on Oct 10, 2022

DrahtBot commented at 12:29 pm on October 10, 2022: contributor

🐙 This pull request conflicts with the target branch and needs rebase.

Want to unsubscribe from rebase notifications on this pull request? Just convert this pull request to a “draft”.

DrahtBot commented at 1:31 am on January 8, 2023: contributor

There hasn’t been much activity lately and the patch still needs rebase. What is the status here?

Is it still relevant? ➡️ Please solve the conflicts to make it ready for review and to ensure the CI passes.
Is it no longer relevant? ➡️ Please close.
Did the author lose interest or time to work on this? ➡️ Please close it and mark it ‘Up for grabs’ with the label, so that it can be picked up in the future.

maflcko commented at 9:05 am on January 11, 2023: member

I’ll pick this up later

maflcko closed this on Jan 11, 2023

maflcko deleted the branch on Jan 11, 2023

bitcoin locked this on Jan 11, 2024

Enforce BIP 68 from genesis #24567

Benefits:

Considerations

Conflicts