Implement BIP 54 (Consensus Cleanup) without mainnet activation #35793

darosior commented at 2:18 PM on July 24, 2026: member

This implements the Consensus Cleanup validation rules proposed in BIP 54. These rules are only enabled on regtest. Mainnet activation, if any, is to be considered separately.

This patchset is based on the code that was previously reviewed (1, 2) and tested (for instance here) on Bitcoin Inquisition. The tests and documentation have since been improved, but the consensus-critical commits have been carried over with only minor differences (the only behavioural one being the addition of the stripped-size check to PreChecks).

Roughly 95% of the added lines are tests or test data. The format, contents, and reproduction procedure for the BIP 54 test vectors are documented here. The bip54_tests unit test module exercises each mitigation extensively in isolation. The feature_bip54.py functional test verifies all of them end-to-end after and prior to activation. For the timestamp rules, it simulates timewarp and Murch–Zawy attacks, demonstrating the new timestamp rules prevent these exploits. A fuzz harness for the sigop accounting logic is also included, which can be seeded from the BIP test vectors (implemented here).

See commit messages for details.

DrahtBot commented at 2:18 PM on July 24, 2026: contributor

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/35793.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	dergoegge, fanquake, polespinasa, theStack, hsjoberg, fjahr, stickies-v, nervana21

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#36192 (test: cover unsatisfiable mining timestamp by Sjors)
#36122 (BIP460: CISA for Taproot key path spends by fjahr)
#35570 (refactor: Change some validation.cpp methods to return BlockValidationState by optout21)
#35569 (Encapsulation for CTransaction by purpleKarrot)
#34864 (coins: tighten cache entry state invariants by l0rinc)
#32468 (rpc: generateblock to allow multiple outputs by polespinasa)
#29491 ([EXPERIMENTAL] Schnorr batch verification for blocks by fjahr)
#28690 (build: Introduce internal kernel library by sedited)

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.

darosior force-pushed on Jul 24, 2026

DrahtBot added the label CI failed on Jul 24, 2026

DrahtBot commented at 2:43 PM on July 24, 2026: contributor

🚧 At least one of the CI tasks failed. Task macOS native: https://github.com/bitcoin/bitcoin/actions/runs/30100472888/job/89504946717 LLM reason (✨ experimental): CI failed because bip54_tests aborted on an assertion failure in CExtKey::SetSeed (seed.size() was outside the expected 16–64 byte range) during bip54_legacy_sigops.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Jul 24, 2026

Christewart commented at 5:49 PM on July 24, 2026: contributor

Congrats! Excited to see this work make it to this point 🎉

in src/consensus/consensus.h:44 in 9630491bf2

  40 |   */
  41 | -static constexpr int64_t MAX_TIMEWARP = 600;
  42 | +static constexpr int64_t MAX_TIMEWARP_TESTNET4 = 600;
  43 | +
  44 | +/** The maximum number of potentially executed legacy signature operations in a single tx */
  45 | +static constexpr unsigned int MAX_TX_BIP54_SIGOPS{2'500};

ariard commented at 2:38 AM on July 29, 2026:

there could be a definition of what is understood as a "legacy signature ops" even if it's just echoing the bip doc, it's consensus/consensus.h where one can reasonably expect to find the definition of a "legacy signature" in opposition to signatures included in the witness for SegWit spends.

darosior commented at 8:22 PM on July 31, 2026:

Ok, i can replace with "non-Segwit signature operations". I've also checked all other instances of "legacy" in the diff here, and it's only used in tests, where the term is already present and i think fine to use.

in src/consensus/tx_verify.cpp:176 in 9630491bf2 outdated

 172 | +
 173 | +        // Unlike the existing block wide sigop limit which counts sigops present in the block
 174 | +        // itself (including the scriptPubKey which is not executed until spending later), BIP54
 175 | +        // counts sigops in the block where they are potentially executed (only).
 176 | +        // This means sigops in the spent scriptPubKey count toward the limit.
 177 | +        // `fAccurate` means correctly accounting sigops for CHECKMULTISIGs(VERIFY) with 16 pubkeys

ariard commented at 2:49 AM on July 29, 2026:

i was reading again the BIP part on the new CheckSigops limit and the computation price for each type of legacy checksig operations, i.e CHECKSIG counts as 1 signature operation, CHECKMULTISIG as 1 to 16 signature operation and (empty) CHECKMULTISIG as 20 signatures operation.

the asymmetry between OP_1 CHECKMULTISIG and (empty) CHECKMULTISIG sounds striking as the vbyte cost of a single CHECKMULTISIG is going to be 1 vbyte and the fee cost of a OP_1 CHECKMULTISIG, so in any collaborative txn among multi parties, one can force the tx to reach the new 2500 limit at a vbyte cost inferior rather than putting a OP_1 CHECKMULTISIG coming with a higher absolute vbyte cost, whatever the feerate.

curious if there is any rational somewhere in all the design notes, that's explaining that choice for the asymmetry in the legacy signature accounting for CHECKMULTISIG

darosior commented at 9:24 PM on August 4, 2026:

Absolutely, the rationale is to reuse BIP 16 accounting, instead of introducing another opcode counting method in consensus rules.

ariard commented at 2:25 AM on August 8, 2026:

@darosior So I'm saying this because in fact before the fix for this class of vulnerabilities: https://bitcoincore.org/en/2025/10/24/disclose-cve-2025-46598/ one could have abuse the new 2500 sigops rule, at a lower marginal feerate cost.

Before #33050 a tx could have been executed twice, the first with standardness rule, the second with consensus check. An attacker could have done a marginal attack, where the tx would have had a huge OP_SHA256 element to hash and the minimal number of accounted for 20 OP_CHECKMULTISIG.

Therefore the transaction would have failed a first time on the standard check and then a second time on the consensus check with a reduced liquidity cost for the attacker (due to the lower feerate surface). and there was easy way to optimize the attack.

This one of the attack I did mention there even if never went to communicate it properly. I might have still have the python test vectors I played with at the time. Somehow the 20 OP_CHECKMULTISIG is a compact trick to render a tx invalid with the minimal of tx surface.

darosior commented at 6:38 PM on August 10, 2026:

I benchmarked the worst case processing time for various types of standard transactions back when i disclosed CVE-2025-46598. As far as i know the worst case for legacy transaction does not involve OP_SHA256. It does involve OP_CHECKMULTISIG, but not with the minimal amount of sigop accounted for (since you need it to actually do compute).

I am not sure what you are trying to get at. Could your point be related to this previous one you raised on the list?

ariard commented at 12:25 AM on September 10, 2026:

I am not sure what you are trying to get at. Could your point be related to this previous one you raised on the list?

No it's something quite different. What I'm talking about in the present issue, it was relying on crafting small weight unit transaction to provoke the most of OP_SHA256 (it's less expensive than sigs, but it's still consuming some CPU cycles) during a validation, and therefore the smallest cost.

Note, it was strictly before #33050 that I did the test. It's not only the worst-case in CPU time to consider it's for a minimal mempool min fee, what's the average satoshis liquidity must avails to consume useless CPU cycles. I'll find back my notes on this and come back to you.

ariard commented at 2:50 AM on July 29, 2026: contributor

Let's start at least the review for the fix of the long validation block time.

dergoegge commented at 2:25 PM on July 29, 2026: member

Concept ACK

fanquake commented at 10:20 AM on July 30, 2026: member

Concept ACK

polespinasa commented at 10:50 AM on July 30, 2026: member

Concept ACK

theStack commented at 12:41 PM on July 30, 2026: contributor

Concept ACK

hsjoberg commented at 12:50 PM on July 30, 2026: contributor

Concept ACK

fjahr commented at 7:31 PM on August 1, 2026: contributor

Concept ACK

in src/node/miner.cpp:67 in 9630491bf2 outdated

  65 | +    // activation safer since BIP94 sets a tighter bound.
  66 |      if (height % difficulty_adjustment_interval == 0) {
  67 | -        min_time = std::max<int64_t>(min_time, pindexPrev->GetBlockTime() - MAX_TIMEWARP);
  68 | +        min_time = std::max<int64_t>(min_time, pindexPrev->GetBlockTime() - MAX_TIMEWARP_TESTNET4);
  69 |      }
  70 |      return min_time;

fjahr commented at 8:36 AM on August 3, 2026:

The miner currently does not enforce the murch-zawy rule but I think it should. Suggested change along with a test: https://github.com/fjahr/bitcoin/commit/edbc841eb546b1c1c41cc6ed16021ddd7adb7b29

darosior commented at 2:28 PM on August 3, 2026:

I considered it for #31376 and decided against because it's strictly theoretical. But your patch looks good. Could you PR it to master? This one is already large, and i think that commit can be merged on its own.

fjahr commented at 1:36 PM on August 11, 2026:

Done now here: #35949

darosior commented at 8:58 PM on September 13, 2026:

Rebased now that #35949 is merged.

in src/kernel/chainparams.cpp:619 in 4a6b1c6ffb outdated

 614 | +        consensus.vDeployments[Consensus::DEPLOYMENT_CONSENSUSCLEANUP].bit = 3;
 615 | +        consensus.vDeployments[Consensus::DEPLOYMENT_CONSENSUSCLEANUP].nStartTime = Consensus::BIP9Deployment::ALWAYS_ACTIVE;
 616 | +        consensus.vDeployments[Consensus::DEPLOYMENT_CONSENSUSCLEANUP].nTimeout = Consensus::BIP9Deployment::NO_TIMEOUT;
 617 | +        consensus.vDeployments[Consensus::DEPLOYMENT_CONSENSUSCLEANUP].min_activation_height = 0; // No activation delay
 618 | +        consensus.vDeployments[Consensus::DEPLOYMENT_CONSENSUSCLEANUP].threshold = 108; // 75%
 619 | +        consensus.vDeployments[Consensus::DEPLOYMENT_CONSENSUSCLEANUP].period = 144;

l0rinc commented at 9:52 PM on August 6, 2026:

4a6b1c6 chainparams: add versionbits deployment for BIP 54:

I understand that the BIP mentions 2016/2015 because it describes mainnet's adjustment period. Since this PR applies the deployment only to regtest by default, could we adjust the BIP text, or add a note here to clarify that these values are derived from the network's adjustment period rather than fixed constants?

darosior commented at 7:01 PM on August 10, 2026:

4a6b1c6 chainparams: add versionbits deployment for BIP 54:

I think your comment applies to the difficulty adjustment period values used by the timewarp and Murch-Zawy mitigations, not that commit (which sets a largely inconsequential dummy BIP 9 deployment for regtest), correct?

So i went and looked for a way to maybe clarify this for the code implementing those checks, but it's already pretty clear these values are derived from the network's adjustment period. Timewarp:

    // If the block is the first of a difficulty adjustment interval, check its timestamp against the previous
    // one to prevent timewarp attacks (see BIP 54).
    const int dai{static_cast<int>(consensusParams.DifficultyAdjustmentInterval())};
    const bool is_first_block{nHeight % dai == 0};
    if (is_first_block) {

Murch-Zawy:

    // Fix for the Murch-Zawy attack. See https://delvingbitcoin.org/t/zawy-s-alternating-timestamp-attack/1062 .
    // TL;DR: the duration of a retarget period must not be negative or the difficulty adjustment limit may be exploited to unduly
    // reduce difficulty similarly to the timewarp vulnerability. Along with the timewarp fix, this effectively makes retarget periods
    // monotonic (modulo the timewarp fix grace period).
    const bool is_last_block{nHeight % dai == dai - 1};
    if (is_last_block && DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_CONSENSUSCLEANUP)) {
        int first_height{nHeight - dai + 1};

Happy to consider any improvement to the comments there.

l0rinc commented at 9:59 PM on August 12, 2026:

Yes, added the code comment to that line simply to demonstrate that regtest has a different difficulty adjustment period than what's documented in the BIP itself:

Given a block at height N:

if N % 2016 is equal to 0, the timestamp of the block must be set to a value higher than or equal to the value of the timestamp of block at height N-1 minus 7200 (TN ≥ TN−1 − 7200);

if N % 2016 is equal to 2015, the timestamp of the block must be set to a value higher than or equal to the value of the timestamp of the block at height N-2015 (TN ≥ TN−2015).

The code looks fine, was just wondering if we could update the BIP to make 2016 and 2015 variables, and in this PR draw attention to the fact that - strictly speaking - we're diverging from the BIP.

darosior commented at 10:59 PM on September 11, 2026:

I think the BIP should always optimize to be maximally clear for mainnet. When it does not affect clarity of the mainnet specifications, diverging values for public test networks may be integrated in the BIP (probably in a separate section). I don't think the values for a Bitcoin Core specific local test network should be mentioned in the BIP.

stickies-v commented at 5:16 PM on August 7, 2026: contributor

Concept ACK

in test/functional/feature_bip54.py:121 in 0acde965fa outdated

 116 | +    def submit_block_many_sigops(self, node):
 117 | +        """Create and submit a block that violates the BIP54 sigops limit."""
 118 | +        txs = [tx.serialize().hex() for tx in self.create_txs_many_sigops()]
 119 | +        self.mine_and_submit(node, txs)
 120 | +
 121 | +    def submit_block_many_sigops_split(self, node):

ariard commented at 1:47 AM on August 8, 2026:

there could be a test for the case where the tx has 2501 sigops and as such is invalid per BIP54 new rule. to demonstrate the code is not doing < 2500 => good ; 2500 => not good ; > 2500 => good.

there might be a test for it elsewhere for the exact test vector. good to have it in the python framework.

darosior commented at 7:35 PM on August 10, 2026:

Bound checking for sigops accounting is tested extensively in unit tests (d75143d39354d723c6a67a290dd03359841f2901). I think it makes more sense for the integration tests to check higher-level boundaries, like for instance activation periods, than duplicate those tests.

in src/test/fuzz/bip54.cpp:36 in 243e7a7623 outdated

  31 | +            tx.vin.emplace_back(dummy_txid, i);
  32 | +            Unserialize(reader, tx.vin.back().scriptSig);
  33 | +
  34 | +            // Either the scriptPubKey (bare) or the redeemScript (P2SH).
  35 | +            CScript spent_script;
  36 | +            Unserialize(reader, spent_script);

ariard commented at 1:57 AM on August 8, 2026:

i would check the test before if it's covered though getting tortuous and valid control flow redeemScript with 10-depth OP_IF then the OP_CHECKSIG, then 10-depth OP_ENDIF, or with OP_ELSE to exerciser the consensus sigops script parser. not guarantee the fuzzer randomness might never yield a structured script for nested control flow.

darosior commented at 11:05 PM on September 11, 2026:

What are you suggesting doing here? I agree code coverage guidance is sometimes lacking when fuzzing stateful logic. I think we can improve things a little with tweaks like Libfuzzer's extra counters. I had briefly looked into it in the past, and may pick it up in the future, but this improvement is largely orthogonal to this PR.

in src/validation.cpp:897 in 39cca6c97e outdated

 893 | @@ -894,7 +894,7 @@ bool MemPoolAccept::PreChecks(ATMPArgs& args, Workspace& ws)
 894 |      }
 895 |  
 896 |      // The mempool holds txs for the next block, so pass height+1 to CheckTxInputs
 897 | -    if (!Consensus::CheckTxInputs(tx, state, m_view, m_active_chainstate.m_chain.Height() + 1, ws.m_base_fees)) {
 898 | +    if (!Consensus::CheckTxInputs(tx, state, m_view, m_active_chainstate.m_chain.Height() + 1, ws.m_base_fees, /*enforce_bip54=*/true)) {

ariard commented at 2:03 AM on August 8, 2026:

should this check be reflected in getblocktemplate given it's already caring about "sigoplimit", "weightlimit" etc. not infringing tx should be selected in the template and yield back in the template.

now of course, this check is enforced at tx mempool processing, so it should never appear in the template already, though having a sanitization check in getblocktemplate might good.

and there is the more transient aspect, than until BIP54 is activated a block is legit to have an infringing tx, even if it has been deployed as policy, though once it's activated, it should not be okay anymore (so don't go to produce an invalid block for the boundary block).

ariard commented at 2:05 AM on August 8, 2026:

in fact it's a dumb nuisance attack vector for the few transition blocks during activation if the miners are running "lax" mempools. a tx might be okay at mempool reception, is stuck in the mempools, and even after few blocks is selected in a template rendering it invalid.

darosior commented at 3:43 PM on August 11, 2026:

Yes, it's already an assumption we make that no invalid transaction make it to our mempool. Note how this commit changes two things in this regard:

The BIP 54 sigops check can't be bypassed anymore with -acceptnonstdtxs.
The mempool sanity checks now assert that it does not contain any transaction that violates the BIP 54 sigops rule.

ariard commented at 12:16 AM on September 10, 2026:

Okay if -acceptnonstdtxs effect starts to kickoff as short as there is a release including this code, I think this is limiting the risk surface for transaction blocks that could violate the rule by accident.

I'll re-check that.

ariard commented at 2:06 AM on August 8, 2026: contributor

Did a 1st read of the fix for long block validation time, I'll continue after the vacation.

Faut pas deconner c'est le mois d'aout.

DrahtBot added the label Needs rebase on Aug 14, 2026

darosior force-pushed on Aug 24, 2026

in src/consensus/tx_verify.cpp:205 in 29d1f3fd45 outdated

 201 |          return state.Invalid(TxValidationResult::TX_MISSING_INPUTS, "bad-txns-inputs-missingorspent",
 202 |                           strprintf("%s: inputs missing/spent", __func__));
 203 |      }
 204 |  
 205 | +    if (enforce_bip54 && !Consensus::CheckSigopsBIP54(tx, inputs)) {
 206 | +        return state.Invalid(TxValidationResult::TX_CONSENSUS, "bad-txns-legacy-sigops", "too many legacy sigops (BIP54)");

polespinasa commented at 10:48 AM on August 24, 2026:

in 29d1f3fd458acf67830948707c483942dff56778 validation: make BIP54 sigops check consensus-critical

This signals the state as TX_CONSENSUS even if this is a policy check. Does not affect in debug/log messages or the rejected reason message, but feels weird to miss-categorize it.

If we care about this, could set a three value struct{consensus, policy, ignore} and set the state according to it.

darosior commented at 11:21 PM on September 11, 2026:

I went back and forth on this. I ended up picking the current version because the error type is not used to choose whether to disconnect a peer anymore, nor exposed to the user.

Making the error depend on the activation status would only change the error string on RPC errors from "bad-txns-legacy-sigops"/"txn-size-64" to "bad-txns-nonstandard-inputs"/"tx-size-small" prior to activation. I have implemented this in https://github.com/bitcoin-inquisition/bitcoin/pull/118 before, but ended up discarding it because i don't think it is worth introducing more complexity and divergence from what has been tested on Inquisition just for this.

See this comment and the comments it links to for more details.

polespinasa commented at 6:10 AM on September 12, 2026:

Then a comment in the code could be usefull, to not create confusion with the consensus word

darosior commented at 8:22 PM on September 13, 2026:

Sure. I gave the rationale for this decision in the commit message though, because i could not find a satisfactory place where to put a comment about it.

polespinasa commented at 11:05 AM on August 24, 2026: member

Did a swift first read to the code (skipping test code) while preparing for the PR review club. The code looks pretty good, just left a comment. Will re-review again in more detail.

DrahtBot removed the label Needs rebase on Aug 24, 2026

DrahtBot added the label CI failed on Aug 24, 2026

in src/validation.cpp:4127 in 67ebb0d57d outdated

4131 | +            static_assert(MAX_TIMEWARP_TESTNET4 <= MAX_TIMEWARP_BIP54);
4132 | +            max_timewarp = MAX_TIMEWARP_TESTNET4;
4133 | +        } else if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_CONSENSUSCLEANUP)) {
4134 | +            max_timewarp = MAX_TIMEWARP_BIP54;
4135 | +        }
4136 | +        if (max_timewarp && block.GetBlockTime() < pindexPrev->GetBlockTime() - *max_timewarp) {

polespinasa commented at 3:32 PM on August 25, 2026:

in 67ebb0d57d9eaf4e1998733c8d818dafd0422791 validation: prevent timewarp attacks with a 2h grace period

nit: max_timewarp.has_value() ?

darosior commented at 11:25 PM on September 11, 2026:

If i did that it would make sense to change the dereference to a max_timewarp.value() as well and together it makes the line unnecessarily long, in my opinion.

I did check and in the entire diff of this PR there is no use of has_value()/value(), so that change also can't be justified on consistency grounds.

For these reasons and because this is just a style nit, i'll keep it like that and resolve this comment.

darosior force-pushed on Sep 2, 2026

DrahtBot removed the label CI failed on Sep 2, 2026

sedited referenced this in commit 35104a8654 on Sep 7, 2026

in src/consensus/consensus.h:35 in e2b4d95942 outdated

  31 | @@ -32,7 +32,7 @@ inline constexpr unsigned int LOCKTIME_VERIFY_SEQUENCE = (1 << 0);
  32 |   * block of a difficulty adjustment period is allowed to
  33 |   * be earlier than the last block of the previous period (BIP94).
  34 |   */
  35 | -inline constexpr int64_t MAX_TIMEWARP = 600;
  36 | +inline constexpr int64_t MAX_TIMEWARP_TESTNET4 = 600;

ariard commented at 11:52 PM on September 9, 2026:

I think the description comment can be adjusted above to say it's only applied to testnet4 and not to signet, regtest, testnet3 (I guess from the enforce_BIP94).

darosior commented at 7:39 PM on September 13, 2026:

The constant was renamed with a TESTNET4 a suffix with the purpose of making it clear it's only used for testnet4. I think adding it to the description too would be redundant because it already mentions BIP 94 (which defines BIP 94), and because it's in the name of the constant already.

in src/consensus/consensus.h:31 in 67ebb0d57d

  26 | @@ -27,6 +27,12 @@ inline constexpr size_t MIN_SERIALIZABLE_TRANSACTION_WEIGHT = WITNESS_SCALE_FACT
  27 |  /** Interpret sequence numbers as relative lock-time constraints. */
  28 |  inline constexpr unsigned int LOCKTIME_VERIFY_SEQUENCE = (1 << 0);
  29 |  
  30 | +/**
  31 | + * Under BIP54, the first block in a difficulty adjustment period must not be more than 2

ariard commented at 12:06 AM on September 10, 2026:

the comment could be "strictly inferior" which is more defined mathematically than "must not be more than 2 hours", where you never know if = is included, at least imho

darosior commented at 7:54 PM on September 13, 2026:

"No more" pretty explicitly means it can be equal, just not more. But fair enough, i updated the comment to use "superior or equal" language, and took the opportunity to define it positively "must be superior or equal to" rather than negatively "must not be inferior or equal".

in src/validation.cpp:4128 in 67ebb0d57d outdated

4132 | +            max_timewarp = MAX_TIMEWARP_TESTNET4;
4133 | +        } else if (DeploymentActiveAfter(pindexPrev, chainman, Consensus::DEPLOYMENT_CONSENSUSCLEANUP)) {
4134 | +            max_timewarp = MAX_TIMEWARP_BIP54;
4135 | +        }
4136 | +        if (max_timewarp && block.GetBlockTime() < pindexPrev->GetBlockTime() - *max_timewarp) {
4137 | +            return state.Invalid(BlockValidationResult::BLOCK_INVALID_HEADER, "time-timewarp-attack", "block's timestamp is too early on diff adjustment block");

ariard commented at 12:08 AM on September 10, 2026:

BLOCK_INVALID_HEADER comment in src/consensus/validation.h could be updated to include "invalid proof of work or time too old".

darosior commented at 7:58 PM on September 13, 2026:

This is already what the comment says: https://github.com/bitcoin/bitcoin/blob/17817818da3cb67c95320b7a5d4d69bd649c1fbf/src/consensus/validation.h#L68

ariard commented at 12:09 AM on September 10, 2026: contributor

started to review the timewarp fix.

darosior force-pushed on Sep 13, 2026

darosior commented at 9:00 PM on September 13, 2026: member

Addressed all outstanding comments and rebased on master. The most significant change is the modification of the test introduced in 6e60c362bc1e373a284911381e2a513f57f5f26b to use BIP54-valid blocks, prior to enforcing the sigops rule by consensus.

darosior force-pushed on Sep 13, 2026

DrahtBot added the label CI failed on Sep 13, 2026

DrahtBot commented at 9:04 PM on September 13, 2026: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/actions/runs/34782490024/job/103791923572 LLM reason (✨ experimental): CI failed due to a Python lint (ruff) error: test_framework.script_util.MAX_TX_BIP54_SIGOPS is imported but unused in test/functional/p2p_segwit.py.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Sep 13, 2026

Kino1994 referenced this in commit a051933787 on Sep 13, 2026

nervana21 commented at 8:23 PM on September 21, 2026: contributor

Concept ACK

DrahtBot added the label Needs rebase on Sep 24, 2026

chainparams: add versionbits deployment for BIP 54 913520ac13

scripted-diff: rename BIP54-sigops constants to MAX_TX_BIP54_SIGOPS

BIP54 counts sigops differently from existing sigops-based checks. Since
we are overloading the sigops term, make clear the constant refers to
BIP54-sigops, not other kinds of pre-existing sigops.

Also update the functional test framework's constant that has
"standardness" in its name, since we are about to make it consensus
critical (func test scripted-diff courtesy of Anthony Towns).

-BEGIN VERIFY SCRIPT-
sed -i 's/MAX_TX_LEGACY_SIGOPS/MAX_TX_BIP54_SIGOPS/g' $(git grep -l MAX_TX_LEGACY_SIGOPS src/)
sed -i 's/MAX_STD_LEGACY_SIGOPS/MAX_TX_BIP54_SIGOPS/g' $(git grep -l MAX_STD_LEGACY_SIGOPS)
sed -i 's/signature operations in validating a transaction./signature operations in a single transaction, per BIP54./' test/functional/test_framework/script_util.py
-END VERIFY SCRIPT-

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

b52976de09

moveonly: move CheckSigopsBIP54 from policy to consensus

Move the function that checks whether a transaction respects the BIP54 sigops rule to the
consensus folder (along with the accompanying constant), as it will be made consensus-critical
in the next commit. Can be reviewed with git's --color-moved option.

e0d882b660

consensus: clarify legacy sigops means non-segwit sigops 9eef92b39e

qa: use valid BIP 54 blocks to test "bad-blk-sigops" in p2p_segwit.py

Commit 6e60c362bc1e373a284911381e2a513f57f5f26b introduced a test using
BIP54-invalid blocks to test legacy sigops accounting. This commit
updates this test to break down the outputs spent in multiple
transactions, in order to make the test BIP 54 valid. Failing that, the
following commit would change the failure reason from "bad-blk-sigops"
to "bad-txns-legacy-sigops".

d55cbdacc8

validation: make BIP54 sigops check consensus-critical

When BIP54 is active, enforce that block transactions do not violate the BIP54 limit on the
number of legacy sigops present in Scripts that get executed during block validation.

Note how the BIP 54 sigops check can't be bypassed anymore in the mempool with -acceptnonstdtxn.
Furthermore, a BIP 54 failure now always returns `TxValidationResult::TX_CONSENSUS`. It could be
possible to preserve the `TxValidationResult::TX_INPUTS_NOT_STANDARD` error prior to activation
instead. But it would diverge more from the code tested on Bitcoin Inquisition and introduce
complexity for little benefits: the error is not used for disconnection decisions anymore, and is
not exposed to users in RPC errors. Hence, we prefer not to.

2afc853fc2

qa: add to utilities a version of SignSignature for Taproot inputs

In Taproot the signature commits to the list of spent outputs.

cf27ad7479

qa: extensive unit tests for BIP54 legacy sigops limit

Test the newly introduced limit with various combinations of inputs and outputs types,
historical transactions, and exercise some implementation-specific edge cases. Record
each test case and optionally write them to disk as JSON to generate the BIP test vectors.

a6864faca0

fuzz: add a fuzz target for the BIP54 sigops check

The fuzz target was specifically crafted to support seeding it with the BIP54 test vectors
generated by the unit test in the previous commit.

d1f123e242

scripted-diff: rename testnet4 timewarp constant

We are going to introduce the timewarp fix for mainnet with a greater grace period. Rename
the MAX_TIMEWARP value for testnet to differentiate them.

-BEGIN VERIFY SCRIPT-

for f in $(git grep -l MAX_TIMEWARP); do sed -i "s/MAX_TIMEWARP/MAX_TIMEWARP_TESTNET4/g" "$f"; done

-END VERIFY SCRIPT-

18494dde7f

miner: update a timewarp comment to refer specifically to BIP 54 188fd17cf9

validation: prevent timewarp attacks with a 2h grace period 127e2ea2a2

validation: prevent negative difficulty adjustment intervals 5cc9d4c0f2

qa: BIP54 test vectors for timewarp and Murch-Zawy

Documentation about the test vectors, including about their structure and content, as well as
reproduction instructions, is available here: https://github.com/bitcoin/bips/tree/master/bip-0054/test_vectors

b3587dcce4

validation: enforce that coinbase transactions are timelocked to block height

When BIP 54 is active, coinbase transactions must have their nLockTime field set to the block height
minus 1 (since it encodes the last height at which the transaction is invalid), and their nSequence
field may be anything but the maximum value (which indicates "final", bypassing timelock
validation).

d2094c8f48

qa: BIP54 test vectors for restrictions on coinbase transactions

Documentation about the test vectors' structure and content, as well as instructions for generating
them is available at https://github.com/bitcoin/bips/tree/master/bip-0054/test_vectors .

4477a3a43f

Avoid creating <= 64-byte transactions in most functional tests. f09286f609

[test] Separate 64B and 63B tx size tests 586799c3a4

validation: make 64-byte transactions invalid

64-byte transactions are also now treated as a consensus failure in
PreChecks, like BIP54-sigops check failures. Note this only changes the
RPC error, and not the disconnection behaviour in P2P since
266dd0e10d08c0bfde63205db15d6c210a021b90.

9342bd4e21

qa: unit tests for BIP54 rule on 64-byte transactions (with JSON test vectors)

This adds tests exercising the bounds of the checks on the invalid transaction size, for various
types of transactions (legacy, Segwit, bytes in input/output to get to 64 bytes) as well as
sanity checking against some known historical violations.

Thanks to Chris Stewart for digging up the historical violations to this rule.

20dd051ab9

qa: end-to-end test all BIP54 mitigations

The previously introduced unit tests extensively test the specific implementation of each
mitigation. This functional test complements them by end-to-end testing all mitigations.
For the added timestamp constraints, it mimicks how they would get exploited (by implementing pseudo
timewarp and Murch-Zawy attacks) and demonstrates those exploits are not possible anymore after
BIP54 activates.

c59faaa081

doc: add a BIP 54 entry to bips.md fa6b7a7480

darosior force-pushed on Sep 25, 2026

DrahtBot removed the label Needs rebase on Sep 25, 2026

DrahtBot added the label CI failed on Sep 25, 2026

DrahtBot commented at 4:41 PM on September 25, 2026: contributor

🚧 At least one of the CI tasks failed. Task ASan + LSan + UBSan + integer: https://github.com/bitcoin/bitcoin/actions/runs/36150952253/job/108123775607 LLM reason (✨ experimental): CI failed because ipc_tests crashed with a kj::ExceptionImpl timer error (“can't advance backwards in time”), aborting the subprocess.

<details><summary>Hints</summary>

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

</details>

DrahtBot removed the label CI failed on Sep 26, 2026

ariard commented at 12:07 AM on September 29, 2026: contributor

With recent actualities, I was musing if there can be issues with the caches, including the disconnectpool one with the novel consensus rules, especially for the new sigops limit and the proposed 64-byte size tx inclusion. I don't think there is issues with MaybeUpdateMempoolForReorg, including if there is a reorg on the transient block of consensus rules activation, as there is a PolicyChecks done at some point.

Same with the cache as they're either script-indexed (m_script_execution_cache) or signature-indexed (m_signature_cache) and all the new checks are at the tx-level. I don't remember to have seen test coverage for all those edge cases, even if it's to demonstrate there is no unexpected interactions, though I'll re-check that. Better safe than not safe.