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

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

Code Coverage & Benchmarks

Reviews