test: P2SH sig ops are only counted with `SCRIPT_VERIFY

brunoerg commented at 2:20 pm on October 14, 2025: contributor

This PR adds a test case for GetTransactionSigOpCost to check that P2SH sig ops are only counted when SCRIPT_VERIFY_P2SH flag is set.

Kills the following mutant:

 0diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
 1index 9d09872597..cc7cdaaf8f 100644
 2--- a/src/consensus/tx_verify.cpp
 3+++ b/src/consensus/tx_verify.cpp
 4@@ -147,7 +147,7 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i
 5     if (tx.IsCoinBase())
 6         return nSigOps;
 7
 8-    if (flags & SCRIPT_VERIFY_P2SH) {
 9+    if (1==1) {
10         nSigOps += GetP2SHSigOpCount(tx, inputs) * WITNESS_SCALE_FACTOR;
11     }

DrahtBot added the label Tests on Oct 14, 2025

DrahtBot commented at 2:20 pm on October 14, 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/33624.

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	instagibbs, l0rinc, maflcko, janb84

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

maflcko commented at 3:51 pm on October 14, 2025: member

review ACK fd2ce880e09c686c320811a322d00b9318f317ae 📌

Signature:

0untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
1RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
2trusted comment: review ACK fd2ce880e09c686c320811a322d00b9318f317ae 📌
3a7guwJPj/0LC4GJ0MbGqlb6sKA2KXRIfvwsjinqVvRMBY7d2YgOa1CRK6/m3J6hnBy8ZktVF+ODCSyFZ03sLDQ==

fanquake requested review from instagibbs on Oct 14, 2025

in src/test/sigopcount_tests.cpp:156 in fd2ce880e0

150@@ -151,6 +151,9 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost)
151         BuildTxs(spendingTx, coins, creationTx, scriptPubKey, scriptSig, CScriptWitness());
152         assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 2 * WITNESS_SCALE_FACTOR);
153         assert(VerifyWithFlag(CTransaction(creationTx), spendingTx, flags) == SCRIPT_ERR_CHECKMULTISIGVERIFY);
154+
155+        // Ignore the SCRIPT_VERIFY_P2SH flag
156+        assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, 0) == 0);

instagibbs commented at 4:40 pm on October 14, 2025:

0        assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, /*flags=*/0) == 0);

brunoerg commented at 6:40 pm on October 14, 2025:

Done.

in src/test/sigopcount_tests.cpp:155 in fd2ce880e0

150@@ -151,6 +151,9 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost)
151         BuildTxs(spendingTx, coins, creationTx, scriptPubKey, scriptSig, CScriptWitness());
152         assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 2 * WITNESS_SCALE_FACTOR);
153         assert(VerifyWithFlag(CTransaction(creationTx), spendingTx, flags) == SCRIPT_ERR_CHECKMULTISIGVERIFY);
154+
155+        // Ignore the SCRIPT_VERIFY_P2SH flag

instagibbs commented at 4:41 pm on October 14, 2025:

we’re not ignoring, we’re not setting it

0        // P2SH sigops are not counted if we don't set the SCRIPT_VERIFY_P2SH flag

brunoerg commented at 6:40 pm on October 14, 2025:

Done.

test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH` flag 3a10d700bc

brunoerg force-pushed on Oct 14, 2025

brunoerg commented at 6:40 pm on October 14, 2025: contributor

Force-pushed addressing #33624 (review) and #33624 (review).

instagibbs approved

instagibbs commented at 6:42 pm on October 14, 2025: member

ACK 3a10d700bc1889b3690097efc935c5a4ba5966bb

DrahtBot requested review from maflcko on Oct 14, 2025

l0rinc commented at 9:22 pm on October 14, 2025: contributor

Verified with:

 0diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
 1index 9d09872597..d43bf14b7e 100644
 2--- a/src/consensus/tx_verify.cpp
 3+++ b/src/consensus/tx_verify.cpp
 4@@ -147,7 +147,7 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i
 5     if (tx.IsCoinBase())
 6         return nSigOps;
 7 
 8-    if (flags & SCRIPT_VERIFY_P2SH) {
 9+    if (true) {
10         nSigOps += GetP2SHSigOpCount(tx, inputs) * WITNESS_SCALE_FACTOR;
11     }
12 
13diff --git a/src/test/sigopcount_tests.cpp b/src/test/sigopcount_tests.cpp
14index 41a4304fe0..30c80533d1 100644
15--- a/src/test/sigopcount_tests.cpp
16+++ b/src/test/sigopcount_tests.cpp
17@@ -153,7 +153,7 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost)
18         assert(VerifyWithFlag(CTransaction(creationTx), spendingTx, flags) == SCRIPT_ERR_CHECKMULTISIGVERIFY);
19 
20         // P2SH sigops are not counted if we don't set the SCRIPT_VERIFY_P2SH flag
21-        assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, /*flags=*/0) == 0);
22+        // assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, /*flags=*/0) == 0);
23     }
24 
25     // P2WPKH witness program

And the build did indeed pass without the new assert, good find!

Tested ACK 3a10d700bc1889b3690097efc935c5a4ba5966bb

maflcko commented at 7:54 am on October 15, 2025: member

re-lgtm ACK 3a10d700bc1889b3690097efc935c5a4ba5966bb

Only change seems to be a comment/doc.

janb84 commented at 7:59 am on October 15, 2025: contributor

tested ACK 3a10d700bc1889b3690097efc935c5a4ba5966bb

PR introduces new assert in test that prohibits the if ( 1 == 1) mutation / if(true), because the assert will fail in that condition.

code review ✅
build & ran tests ✅
changed code to true to test assert ✅

fanquake merged this on Oct 15, 2025

fanquake closed this on Oct 15, 2025

test: P2SH sig ops are only counted with SCRIPT_VERIFY_P2SH #33624

Code Coverage & Benchmarks

Reviews

test: P2SH sig ops are only counted with `SCRIPT_VERIFY_P2SH` #33624