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

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.

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.

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

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

@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.

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?

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

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.

Done now here: #35949

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?

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.

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 (T_N ≥ T_N−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 (T_N ≥ T_N−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.

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.

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.

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.

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

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.

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.

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.

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

nit: max_timewarp.has_value() ?