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:

diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 9d09872597..cc7cdaaf8f 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -147,7 +147,7 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i
     if (tx.IsCoinBase())
         return nSigOps;

-    if (flags & SCRIPT_VERIFY_P2SH) {
+    if (1==1) {
         nSigOps += GetP2SHSigOpCount(tx, inputs) * WITNESS_SCALE_FACTOR;
     }

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 📌

<details><summary>Show signature</summary>

Signature:

untrusted 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}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: review ACK fd2ce880e09c686c320811a322d00b9318f317ae 📌
a7guwJPj/0LC4GJ0MbGqlb6sKA2KXRIfvwsjinqVvRMBY7d2YgOa1CRK6/m3J6hnBy8ZktVF+ODCSyFZ03sLDQ==

</details>

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:

        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

        // 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:

diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 9d09872597..d43bf14b7e 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -147,7 +147,7 @@ int64_t GetTransactionSigOpCost(const CTransaction& tx, const CCoinsViewCache& i
     if (tx.IsCoinBase())
         return nSigOps;
 
-    if (flags & SCRIPT_VERIFY_P2SH) {
+    if (true) {
         nSigOps += GetP2SHSigOpCount(tx, inputs) * WITNESS_SCALE_FACTOR;
     }
 
diff --git a/src/test/sigopcount_tests.cpp b/src/test/sigopcount_tests.cpp
index 41a4304fe0..30c80533d1 100644
--- a/src/test/sigopcount_tests.cpp
+++ b/src/test/sigopcount_tests.cpp
@@ -153,7 +153,7 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost)
         assert(VerifyWithFlag(CTransaction(creationTx), spendingTx, flags) == SCRIPT_ERR_CHECKMULTISIGVERIFY);
 
         // P2SH sigops are not counted if we don't set the SCRIPT_VERIFY_P2SH flag
-        assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, /*flags=*/0) == 0);
+        // assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, /*flags=*/0) == 0);
     }
 
     // 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

sedited referenced this in commit f50d90fcbb on Oct 29, 2025

stickies-v referenced this in commit a785fe6759 on Nov 5, 2025

stringintech referenced this in commit b82132c4d1 on Nov 6, 2025

yuvicc referenced this in commit f620a2206b on Nov 16, 2025

Kino1994 referenced this in commit fdb953d0f7 on Jun 28, 2026

BigcoinBGC referenced this in commit 84c73ad326 on Jun 30, 2026

Kino1994 referenced this in commit 14906cd73d on Aug 19, 2026

bitcoin locked this on Oct 15, 2026

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

Code Coverage & Benchmarks

Reviews