policy: make pathological transactions packed with legacy sigops non-standard

darosior commented at 9:05 pm on May 15, 2025: member

The Consensus Cleanup soft fork proposal includes a limit on the number of legacy signature operations potentially executed when validating a transaction. If this change is to be implemented here and activated by Bitcoin users in the future, we should make transactions that are not valid according to the new rules non-standard first because it would otherwise be a trivial DoS to potentially unupgraded miners after the soft fork activates.

DrahtBot commented at 9:05 pm on May 15, 2025: 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/32521.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	Sjors

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32532 (refactor: extract DecodePushData from GetScriptOp and GetLegacySigOpCount by l0rinc)
#31989 (BIP-119 (OP_CHECKTEMPLATEVERIFY) (regtest only) by jamesob)
#31682 ([IBD] specialize CheckBlock’s input & coinbase checks by l0rinc)
#29060 (Policy: Report reason inputs are non standard by ismaelsadeeq)

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 TX fees and policy on May 15, 2025

darosior force-pushed on May 15, 2025

DrahtBot added the label CI failed on May 15, 2025

DrahtBot commented at 9:09 pm on May 15, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/42319362568 LLM reason (✨ experimental): The CI failure is caused by a Python linting error in test/functional/mempool_sigoplimit.py where a variable is redefined.

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.

darosior force-pushed on May 15, 2025

in test/functional/mempool_sigoplimit.py:209 in f13acba6f0 outdated

204+            txid = int.from_bytes(bytes.fromhex(res["txid"]), byteorder="big")
205+            outpoints.append(COutPoint(txid, res["sent_vout"]))
206+        self.generate(self.nodes[0], 1)
207+
208+        # Spending all these outputs at once accounts for 2505 legacy sigops and is non-standard.
209+        tx = CTransaction()

instagibbs commented at 1:13 pm on May 16, 2025:

might be worth getting this one mined to show it’s consensus-valid still

darosior commented at 8:11 pm on May 19, 2025:

Expanded the test to check the original, non-standard, one can still be mined.

l0rinc commented at 2:16 pm on May 16, 2025: contributor

While sigops aren’t necessarily difficult to compute, there’s a lot of them - and now even more. Please consider the related sigop optimization PR I just pushed: https://github.com/bitcoin/bitcoin/pull/32532

in src/policy/policy.h:42 in e3eceb8492 outdated

37@@ -38,6 +38,8 @@ static constexpr unsigned int MIN_STANDARD_TX_NONWITNESS_SIZE{65};
38 static constexpr unsigned int MAX_P2SH_SIGOPS{15};
39 /** The maximum number of sigops we're willing to relay/mine in a single tx */
40 static constexpr unsigned int MAX_STANDARD_TX_SIGOPS_COST{MAX_BLOCK_SIGOPS_COST/5};
41+/** The maximum number of potentially executed legacy signature operations in a single standard tx */
42+static constexpr unsigned int MAX_LEGACY_SIGOPS{2'500};

Sjors commented at 11:06 am on May 19, 2025:

MAX_TX_LEGACY_SIGOPS?

darosior commented at 8:11 pm on May 19, 2025:

That’s actually what i had initially but i reduced it to MAX_LEGACY_SIGOPS in an attempt to avoid too long variable names. With the comment expliciting it’s about transaction inputs, i think it should be fine as-is.

Sjors commented at 7:43 am on May 20, 2025:

I don’t think the 3 character savings is worth making people jump to the documentation. MAX_TX_ vs MAX_BLOCK_ seems like a good convention to keep.

darosior commented at 8:51 pm on May 20, 2025:

Fine, done.

in src/policy/policy.cpp:199 in e3eceb8492 outdated

194 
195+    unsigned int sigops{0};
196     for (unsigned int i = 0; i < tx.vin.size(); i++) {
197         const CTxOut& prev = mapInputs.AccessCoin(tx.vin[i].prevout).out;
198 
199+        sigops += tx.vin[i].scriptSig.GetSigOpCount(true);

Sjors commented at 11:08 am on May 19, 2025:

GetSigOpCount(/*fAccurate=*/true)

And the fAccurate is terribly vague, it’s useful to explain:

0// Unlike the block wide sigop limit, the BIP54 per
1// transaction limit uses "accurate" BIP16 style accounting.

darosior commented at 8:11 pm on May 19, 2025:

Done.

in src/policy/policy.cpp:229 in e3eceb8492 outdated

221                 return false;
222             }
223+            sigops += p2sh_sigops;
224+        }
225+
226+        if (sigops > MAX_LEGACY_SIGOPS) {

Sjors commented at 11:14 am on May 19, 2025:

Although there are no break or continue statements in the loop, it would feel a bit more robust to move this check below the loop (although an early break is more efficient).

darosior commented at 8:00 pm on May 19, 2025:

I understand the slight additional complexity concern but for a transaction submitted to us with no PoW i think we should discard it as soon as we know we won’t accept it. I really don’t think we should keep doing unnecessary work by iterating, solving scripts and counting.

in src/policy/policy.cpp:187 in e3eceb8492 outdated

182@@ -183,16 +183,22 @@ bool IsStandardTx(const CTransaction& tx, const std::optional<unsigned>& max_dat
183  *    as potential new upgrade hooks.
184  *
185  * Note that only the non-witness portion of the transaction is checked here.
186+ *
187+ * We also check the total number of sigops across the whole transaction.

Sjors commented at 11:31 am on May 19, 2025:

Could add: The per transaction sigop limit introduced in BIP54 does not cover the witness.

darosior commented at 8:01 pm on May 19, 2025:

Don’t you think it would be confusing to point here since this function does not concern itself with the witness? Especially as it’s pointed right before.

Sjors commented at 7:39 am on May 20, 2025:

Well that’s what confused me. My first thought was: why does this work if we’re not checking the witness?

But maybe the sigops related comment needs to go above the “only the non-witness” portion.

Or something like:

0* Note that only the non-witness portion of the transaction is checked here.
1* This is fine because the per transaction sigop limit introduced in BIP54 
2* does not cover the witness.

(and then drop the “We also check the total”)

darosior commented at 8:47 pm on May 20, 2025:

Fine, but went for a more compressed version:

0 * We also check the total number of legacy sigops across the whole transaction, as per BIP54.

Looks good?

Sjors commented at 7:54 am on May 21, 2025:

Sounds fine, forgot to push this?

(maybe say “non-witness” instead of “legacy”)

darosior commented at 3:13 pm on May 21, 2025:

Changed to “non-witness” and actually pushed it this time. :)

in src/policy/policy.cpp:200 in e3eceb8492 outdated

195+    unsigned int sigops{0};
196     for (unsigned int i = 0; i < tx.vin.size(); i++) {
197         const CTxOut& prev = mapInputs.AccessCoin(tx.vin[i].prevout).out;
198 
199+        sigops += tx.vin[i].scriptSig.GetSigOpCount(true);
200+        sigops += prev.scriptPubKey.GetSigOpCount(true);

Sjors commented at 11:40 am on May 19, 2025:

0// Unlike the block wide sigop limit, the BIP54 per
1// transaction limit includes the prevout scriptPubKey.

darosior commented at 8:11 pm on May 19, 2025:

Done, added a sentence to this effect.

darosior force-pushed on May 19, 2025

maflcko commented at 8:42 pm on May 20, 2025: member

for reference, the CI failure is:

 0[16:36:22.725] /ci_container_base/src/test/transaction_tests.cpp:1089:20: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
 1[16:36:22.725]  1089 |     tx_create.vout.push_back(CTxOut(424242, max_sigops_p2sh));
 2[16:36:22.725]       |                    ^~~~~~~~~~~~~~~~~                       ~
 3[16:36:22.725]       |                    emplace_back(
 4[16:36:22.725] /ci_container_base/src/test/transaction_tests.cpp:1122:25: error: use emplace_back instead of push_back [modernize-use-emplace,-warnings-as-errors]
 5[16:36:22.725]  1122 |     tx_create_p2pk.vout.push_back(CTxOut(212121, p2pk_script));
 6[16:36:22.725]       |                         ^~~~~~~~~~~~~~~~~                   ~
 7[16:36:22.725]       |                         emplace_back(
 8[16:36:22.725] 1515 warnings generated.
 9[16:36:22.725] 
10[16:36:22.725] ^^^ ⚠️ Failure generated from clang-tidy

darosior commented at 8:42 pm on May 20, 2025: member

Yeah i’m changing for emplace_back and addressing @Sjors’ nits right now.

darosior force-pushed on May 20, 2025

DrahtBot removed the label CI failed on May 21, 2025

in test/functional/mempool_sigoplimit.py:221 in 29e8fe7144 outdated

216+        std_tx = deepcopy(nonstd_tx)
217+        std_tx.vin.pop()
218+        self.nodes[0].sendrawtransaction(std_tx.serialize().hex())
219+
220+        # Make sure the original, non-standard, transaction can be mined.
221+        self.generateblock(self.nodes[0], output="raw(42)", transactions=[nonstd_tx.serialize().hex()])

Sjors commented at 9:46 am on May 21, 2025:

29e8fe71440a7e27ee89527a49753be615f205c7: note to other reviewers, yes generateblocks checks for errors, as you can see if you duplicate nonstd_tx.serialize().hex()

in test/functional/test_framework/script_util.py:26 in 29e8fe7144 outdated

21@@ -22,6 +22,12 @@
22     sha256,
23 )
24 
25+# Maximum number of potentially executed legacy signature operations in validating a transaction.
26+MAX_STD_LEGACY_SIGOPS = 2_500

Sjors commented at 9:48 am on May 21, 2025:

29e8fe71440a7e27ee89527a49753be615f205c7: MAX_TX_LEGACY_SIGOPS would be consistent with the c++ code

darosior commented at 3:13 pm on May 21, 2025:

Meh. Then it would not make it consistent with MAX_STD_P2SH_SIGOPS, and having in the name that’s it’s a standardness rule is useful. I don’t think it’s worth changing.

in src/test/transaction_tests.cpp:1073 in a139a538c2 outdated

1068+    // single transaction, and a transaction spending them.
1069+    CMutableTransaction tx_create, tx_max_sigops;
1070+    const auto p2sh_inputs_count{MAX_TX_LEGACY_SIGOPS / MAX_P2SH_SIGOPS};
1071+    tx_create.vout.reserve(p2sh_inputs_count);
1072+    for (unsigned i{0}; i < p2sh_inputs_count; ++i) {
1073+        tx_create.vout.emplace_back(424242 + i, max_sigops_p2sh);

Sjors commented at 9:50 am on May 21, 2025:

a139a538c298621674ba622e63971704cbb7ceff: what’s this magic 424242 value?

darosior commented at 1:21 pm on May 21, 2025:

The value of a transaction output.

Sjors commented at 2:13 pm on May 21, 2025:

Ah ok, any reason for the + i?

darosior commented at 3:18 pm on May 21, 2025:

Just varying the amounts because why not.

Sjors commented at 3:42 pm on May 21, 2025:

It’s confusing, but otherwise harmless.

in src/test/transaction_tests.cpp:1081 in a139a538c2 outdated

1076+    tx_max_sigops.vin.reserve(p2sh_inputs_count);
1077+    for (unsigned i{0}; i < p2sh_inputs_count; ++i) {
1078+        tx_max_sigops.vin.emplace_back(COutPoint(prev_txid, i), max_sigops_redeem_script);
1079+    }
1080+
1081+    // p2sh_inputs_count is truncated to 166 (from 166.6666..)

Sjors commented at 9:51 am on May 21, 2025:

a139a538c298621674ba622e63971704cbb7ceff: it seems more clear if you define it as an integer instead of auto.

darosior commented at 3:13 pm on May 21, 2025:

Done.

in src/test/transaction_tests.cpp:1120 in a139a538c2 outdated

1115+    AddCoins(coins, CTransaction(tx_create_p2pk), false);
1116+
1117+    // The transaction now contains exactly 2500 sigops, the check should pass.
1118+    BOOST_CHECK(p2sh_inputs_count * MAX_P2SH_SIGOPS + p2pk_inputs_count * 1 == MAX_TX_LEGACY_SIGOPS);
1119+    BOOST_CHECK(::AreInputsStandard(CTransaction(tx_max_sigops), coins));
1120+

Sjors commented at 9:55 am on May 21, 2025:

a139a538c298621674ba622e63971704cbb7ceff: could insert a segwit spend to show that it doesn’t count.

darosior commented at 3:13 pm on May 21, 2025:

Good call, done.

Sjors approved

Sjors commented at 9:56 am on May 21, 2025: member

ACK 29e8fe71440a7e27ee89527a49753be615f205c7

policy: make pathological transactions packed with legacy sigops non-standard.

The Consensus Cleanup soft fork proposal includes a limit on the number of legacy signature
operations potentially executed when validating a transaction. If this change is to be implemented
here and activated by Bitcoin users in the future, we should prevent the ability for someone to
broadcast a transaction through the p2p network that is not valid according to the new rules. This
is because if it was possible it would be a trivial DoS to potentially unupgraded miners after the
soft fork activates.

We do not know for sure whether users will activate the Consensus Cleanup. However if they do such
transactions must have been made non-standard long in advance, due to the time it takes for most
nodes on the network to upgrade. In addition this limit may only be ran into by pathological
transactions which pad the Script with sigops but do not use actual signatures whem spending, as
otherwise they would run into the standard transaction size limit.

1247446f4d

qa: unit test standardness of inputs packed with legacy sigops

Check bounds and different output types.

c1aa5852e0

qa: functional test a transaction running into the legacy sigop limit

It's useful to have an end-to-end test in addition to the unit test to sanity check the RPC error as
well as making sure the transaction is otherwise fully standard.

80d2136779

darosior force-pushed on May 21, 2025

Sjors commented at 3:45 pm on May 21, 2025: member

ACK 80d213677962268a82d65d8915c518f0c02867d9

DrahtBot added the label CI failed on May 21, 2025

glozow referenced this in commit d2c9fc84e1 on May 22, 2025

DrahtBot removed the label CI failed on May 23, 2025

policy: make pathological transactions packed with legacy sigops non-standard #32521

Code Coverage & Benchmarks

Reviews

Conflicts