test: fix wrong transaction in GetP2SHSigOpCount assertion #35863

jeanpablojp commented at 9:26 PM on August 1, 2026: contributor

While reading through script_p2sh_tests.cpp I noticed one of the assertions in ValidateInputsStandardness checks the wrong transaction.

The test builds txToNonStd2_no_scriptSig (which spends a P2SH prevout with an empty scriptSig) and checks its standardness result ("input 0 P2SH redeemscript missing"), but the GetP2SHSigOpCount assertion right after it re-checks the previous transaction: line 433 is byte-identical to line 419. Looks like a copy-paste slip from 248c175e3d, which added a GetP2SHSigOpCount check after each constructed transaction.

This PR points the assertion at txToNonStd2_no_scriptSig and expects 0 sigops. With an empty scriptSig there's no redeemScript push, so GetSigOpCount(scriptSig) ends up counting an empty subscript and returns 0. This case wasn't asserted anywhere before. The line above covers the other side, where the same prevout spent with the actual redeemScript counts 20.

To make sure the fix isn't vacuous I also ran the assertion expecting 20, and it fails with [0 != 20].

Tested with:

cmake --build build --target test_bitcoin
build/bin/test_bitcoin --run_test=script_p2sh_tests

test: fix wrong transaction in GetP2SHSigOpCount assertion 5559fa464b

DrahtBot added the label Tests on Aug 1, 2026

DrahtBot commented at 9:26 PM on August 1, 2026: 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/35863.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
ACK	l0rinc, sedited

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

LLM Linter (✨ experimental)

Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

SignSignature(keystore, CTransaction(txFrom), txTo, 0, SIGHASH_ALL, empty) in src/test/script_p2sh_tests.cpp
SignSignature(keystore, CTransaction(txFrom), txTo, 1, SIGHASH_ALL, empty_b) in src/test/script_p2sh_tests.cpp
SignSignature(keystore, CTransaction(txFrom), txTo, 2, SIGHASH_ALL, empty_c) in src/test/script_p2sh_tests.cpp

test: give each ValidateInputsStandardness case its own scope 756afe14b5

in src/test/script_p2sh_tests.cpp:433 in 5559fa464b

 429 | @@ -430,7 +430,7 @@ BOOST_AUTO_TEST_CASE(ValidateInputsStandardness)
 430 |      BOOST_CHECK(txToNonStd2_no_scriptSig_res.IsInvalid());
 431 |      BOOST_CHECK_EQUAL(txToNonStd2_no_scriptSig_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
 432 |      BOOST_CHECK_EQUAL(txToNonStd2_no_scriptSig_res.GetDebugMessage(), "input 0 P2SH redeemscript missing");
 433 | -    BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2), coins), 20U);
 434 | +    BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2_no_scriptSig), coins), 0U);

maflcko commented at 9:26 AM on August 2, 2026:

Introduced in 248c175e3dc355301d948d55abdcebb1af736f4a.

Generally, it is tedious to review a single test case that spans several hundred lines of scope and "shadows" a bunch of similarly named variables.

If the test case is so large, because it shares a common setup, my recommendation would still be to put each "sub-test-case" into it's own scope: { ... }.

This way, variables can't leak from that scope and shadow later ones or accidentally get re-used later.

jeanpablojp commented at 9:12 PM on August 2, 2026:

Thanks for the suggestion, I agree with it. Applied in a second commit (756afe14b5): each sub-case now gets its own scope, and the shared setup stays outside. If you look at that commit's diff with whitespace ignored, the only real change is the braces. A nice side effect: with the scopes in place, the original mistake wouldn't even compile.

l0rinc commented at 9:23 PM on August 3, 2026:

That's fair

in src/test/script_p2sh_tests.cpp:430 in 756afe14b5

 497 | +        BOOST_CHECK_EQUAL(txToNonStd2_res.GetDebugMessage(), "p2sh redeemscript sigops exceed limit (input 0: 20 > 15)");
 498 | +        BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2), coins), 20U);
 499 | +    }
 500 | +
 501 | +    {
 502 | +        CMutableTransaction txToNonStd2_no_scriptSig;

l0rinc commented at 8:44 PM on August 3, 2026:

We don't actually need to complicate ourselves with this, the 20-sigop and missing-redeemScript cases differ only in scriptSig

<details><summary>reuse `txToNonStd2`</summary>

diff --git a/src/test/script_p2sh_tests.cpp b/src/test/script_p2sh_tests.cpp
index df59385b3e..8cf51bd89d 100644
--- a/src/test/script_p2sh_tests.cpp
+++ b/src/test/script_p2sh_tests.cpp
@@ -424,22 +424,14 @@ BOOST_AUTO_TEST_CASE(ValidateInputsStandardness)
         BOOST_CHECK_EQUAL(txToNonStd2_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
         BOOST_CHECK_EQUAL(txToNonStd2_res.GetDebugMessage(), "p2sh redeemscript sigops exceed limit (input 0: 20 > 15)");
         BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2), coins), 20U);
-    }
 
-    {
-        CMutableTransaction txToNonStd2_no_scriptSig;
-        txToNonStd2_no_scriptSig.vout.resize(1);
-        txToNonStd2_no_scriptSig.vout[0].scriptPubKey = GetScriptForDestination(PKHash(key[1].GetPubKey()));
-        txToNonStd2_no_scriptSig.vout[0].nValue = 1000;
-        txToNonStd2_no_scriptSig.vin.resize(1);
-        txToNonStd2_no_scriptSig.vin[0].prevout.n = 6;
-        txToNonStd2_no_scriptSig.vin[0].prevout.hash = txFrom.GetHash();
-
-        const auto txToNonStd2_no_scriptSig_res = ::ValidateInputsStandardness(CTransaction(txToNonStd2_no_scriptSig), coins);
+        txToNonStd2.vin[0].scriptSig.clear();
+
+        const auto txToNonStd2_no_scriptSig_res = ::ValidateInputsStandardness(CTransaction(txToNonStd2), coins);
         BOOST_CHECK(txToNonStd2_no_scriptSig_res.IsInvalid());
         BOOST_CHECK_EQUAL(txToNonStd2_no_scriptSig_res.GetRejectReason(), "bad-txns-nonstandard-inputs");
         BOOST_CHECK_EQUAL(txToNonStd2_no_scriptSig_res.GetDebugMessage(), "input 0 P2SH redeemscript missing");
-        BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2_no_scriptSig), coins), 0U);
+        BOOST_CHECK_EQUAL(GetP2SHSigOpCount(CTransaction(txToNonStd2), coins), 0U);
     }
 
     // TxoutType::NONSTANDARD

</details>

jeanpablojp commented at 9:18 PM on August 3, 2026:

I tried your version and it works, tests still pass.

My only hesitation is that it brings back the shape that allowed the original mistake: GetP2SHSigOpCount(CTransaction(txToNonStd2), coins) appears twice with different expected values, separated only by the scriptSig mutation. With a separate transaction the second case can't be confused with the first.

That said, I don't feel strongly about it. Happy to apply your version if you and @maflcko prefer the smaller diff.

l0rinc approved

l0rinc commented at 9:24 PM on August 3, 2026: contributor

ACK 756afe14b5c1c5f7a0b21f2143289d2c6dea5e2e

Fixed a copy paste error and separated each chunk to make sure cross-polination is prevented

sedited approved

sedited commented at 11:07 AM on August 4, 2026: contributor

ACK 756afe14b5c1c5f7a0b21f2143289d2c6dea5e2e

sedited merged this on Aug 4, 2026

sedited closed this on Aug 4, 2026