test: Enhance GetTxSigOpCost tests for coinbase transactions

average-gary commented at 7:04 pm on June 30, 2025: none

Added assertions to the GetTxSigOpCost test cases to verify that the witness of a coinbase transaction is not considered in the signature operation cost calculations.

Using spendingTx.vin[0].prevout.SetNull() we create a coinbase transaction that evaluates to true for IsCoinbase(). Doing this to transactions (spendingTx in this case) that evaluate to a non-zero sigop output, we more concretely test that the witness of a coinbase transaction is not taken into account for SigOp maths.

In my experimentation in mining software (mostly Stratum v2) I encountered the SigOps budget and began exploring the considerations as it applies to coinbase transactions. It was unclear how commitment-type addresses for coinbase were handled compared to bare script when it came to SigOp calculation. Upon further investigation, I saw that the test suite could have added vectors that clearly demonstrate that the witness for a coinbase transaction is not considered for GetTransactionSigOpCost.

Adding these tests makes it more clear for someone in the future how SigOp maths work while exploring the intersection of SigOps and coinbase transactions.

DrahtBot added the label Tests on Jun 30, 2025

DrahtBot commented at 7:04 pm on June 30, 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/32840.

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#32729 (test,refactor: extract script template helpers & widen sigop count coverage by l0rinc)
#32317 (kernel: Separate UTXO set access from validation functions by sedited)

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.

average-gary force-pushed on Jun 30, 2025

DrahtBot added the label CI failed on Jun 30, 2025

DrahtBot commented at 7:11 pm on June 30, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/45076576501 LLM reason (✨ experimental): Lint check failed due to presence of trailing whitespace in source files.

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.

average-gary force-pushed on Jun 30, 2025

DrahtBot removed the label CI failed on Jun 30, 2025

Sammie05 commented at 3:04 am on July 13, 2025: none

Thanks for adding these assertions! I like how you explicitly test that coinbase witnesses don’t contribute to SigOp cost. Makes it clearer for future maintainers. LGTM 👍

DrahtBot added the label Needs rebase on Oct 15, 2025

achow101 requested review from Sjors on Oct 22, 2025

achow101 requested review from theStack on Oct 22, 2025

achow101 requested review from l0rinc on Oct 22, 2025

achow101 requested review from davidgumberg on Oct 22, 2025

average-gary force-pushed on Oct 23, 2025

DrahtBot removed the label Needs rebase on Oct 23, 2025

in src/test/sigopcount_tests.cpp:188 in 6b6b0f4af6

185@@ -182,6 +186,7 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost)
186         // The witness of a coinbase transaction is not taken into account.
187         spendingTx.vin[0].prevout.SetNull();
188         assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 0);

sedited commented at 1:10 pm on October 31, 2025:

Why did you think this single test was not enough? Is there additional clarity or coverage from the additional checks?

average-gary commented at 2:07 pm on November 3, 2025:

It seemed lacking since it was only tested for one script type (p2wpkh).

sedited commented at 10:07 pm on November 3, 2025:

Mmh, I think that could clarify a bit how the sigops are accounted for, but if you can read the test, you can also just go and read the original code? I guess it could still guard against dumb regressions of the accounting code though.

in src/test/sigopcount_tests.cpp:158 in 6b6b0f4af6

153@@ -154,6 +154,11 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost)
154 
155         // P2SH sigops are not counted if we don't set the SCRIPT_VERIFY_P2SH flag
156         assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, /*flags=*/0) == 0);
157+
158+        // The witness of a coinbase transaction is not taken into account.

sedited commented at 10:03 pm on November 3, 2025:

This comment is misleading. It is not just the witness that is not taken into account, but also any p2sh sigops.

in src/test/sigopcount_tests.cpp:161 in 6b6b0f4af6 outdated

153@@ -154,6 +154,11 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost)
154

sedited commented at 10:04 pm on November 3, 2025:

Can you add a case to the multisig case too? There we should be expecting a sigop cost, even if the first input’s prevout is null.

l0rinc referenced this in commit 24bcad3d4d on Nov 4, 2025

in src/test/sigopcount_tests.cpp:159 in 6b6b0f4af6

153@@ -154,6 +154,11 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost)
154 
155         // P2SH sigops are not counted if we don't set the SCRIPT_VERIFY_P2SH flag
156         assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, /*flags=*/0) == 0);
157+
158+        // The witness of a coinbase transaction is not taken into account.
159+        spendingTx.vin[0].prevout.SetNull();

l0rinc commented at 10:10 pm on November 4, 2025:

it seems to me scriptWitness is actually empty here, so the comment and code are a bit confusing here. I think a BOOST_REQUIRE(!spendingTx.vin[0].scriptWitness.IsNull()); would document the requirements here better than a comment. BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), coins, flags), 0); already checks that coinbases don’t have sigops

in src/test/sigopcount_tests.cpp:161 in 6b6b0f4af6

153@@ -154,6 +154,11 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost)
154 
155         // P2SH sigops are not counted if we don't set the SCRIPT_VERIFY_P2SH flag
156         assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, /*flags=*/0) == 0);
157+
158+        // The witness of a coinbase transaction is not taken into account.
159+        spendingTx.vin[0].prevout.SetNull();
160+        assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 0);
161+        assert(GetTransactionSigOpCost(CTransaction(creationTx), coins, flags) == 0);

l0rinc commented at 10:34 pm on November 4, 2025:

this is already a coinbase tx, basically equivalent to the above, let’s put this before mutating the spending tx, it’s not related to it

in src/test/sigopcount_tests.cpp:206 in 6b6b0f4af6

200@@ -196,6 +201,11 @@ BOOST_AUTO_TEST_CASE(GetTxSigOpCost)
201         BuildTxs(spendingTx, coins, creationTx, scriptPubKey, scriptSig, scriptWitness);
202         assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 1);
203         assert(VerifyWithFlag(CTransaction(creationTx), spendingTx, flags) == SCRIPT_ERR_EQUALVERIFY);
204+
205+        // The witness of a coinbase transaction is not taken into account.
206+        spendingTx.vin[0].prevout.SetNull();

l0rinc commented at 10:36 pm on November 4, 2025:

After we make this a coinbase, it’s confusing to refer to it as a “spending” transaction… We could add another helper that copies it and makes it a coinbase

in src/test/sigopcount_tests.cpp:128 in 6b6b0f4af6

l0rinc commented at 10:38 pm on November 4, 2025:

I know you didn’t introduce them, but these are basically self-contained tests that could be split out to separate BOOST_AUTO_TEST_CASE

GetSigOpCount
GetTxSigOpCost_Multisig
GetTxSigOpCost_MultisigP2SH
GetTxSigOpCost_P2WPKH
GetTxSigOpCost_P2WPKH_P2SH
GetTxSigOpCost_P2WSH
GetTxSigOpCost_P2WSH_P2SH

l0rinc changes_requested

l0rinc commented at 11:41 pm on November 4, 2025: contributor

I have gone through the cases, I think we should take this opportunity and unify the test to use BOOST_ checkers for better error messages, to split the big test into smaller self-contained tests (otherwise the first failure will break the remaining ones - though this will result in some setup-repetition, but we can add helpers for those).

I have applied the cleanup that I would like to see here, if you decide to accept any of it, please do it in multiple focused commits.

  0// Copyright (c) 2012-present The Bitcoin Core developers
  1// Distributed under the MIT software license, see the accompanying
  2// file COPYING or http://www.opensource.org/licenses/mit-license.php.
  3
  4#include <addresstype.h>
  5#include <coins.h>
  6#include <consensus/consensus.h>
  7#include <consensus/tx_verify.h>
  8#include <key.h>
  9#include <pubkey.h>
 10#include <script/interpreter.h>
 11#include <script/script.h>
 12#include <script/solver.h>
 13#include <test/util/setup_common.h>
 14#include <uint256.h>
 15
 16#include <vector>
 17
 18#include <boost/test/unit_test.hpp>
 19
 20static constexpr script_verify_flags STANDARD_SCRIPT_VERIFY_FLAGS{SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_P2SH};
 21
 22struct TestCoinsViewCache {
 23    CCoinsView dummy;
 24    CCoinsViewCache cache{&dummy};
 25};
 26
 27static CPubKey GenerateTestPubKey()
 28{
 29    return GenerateRandomKey().GetPubKey();
 30}
 31
 32static std::vector<unsigned char> Serialize(const CScript& s)
 33{
 34    std::vector<unsigned char> sSerialized(s.begin(), s.end());
 35    return sSerialized;
 36}
 37
 38static CTransaction MakeCoinBase(const CMutableTransaction& tx)
 39{
 40    CMutableTransaction coinbase{tx};
 41    coinbase.vin[0].prevout.SetNull();
 42    return CTransaction{coinbase};
 43}
 44
 45BOOST_FIXTURE_TEST_SUITE(sigopcount_tests, BasicTestingSetup)
 46
 47BOOST_AUTO_TEST_CASE(GetSigOpCount)
 48{
 49    CScript s1;
 50    BOOST_CHECK_EQUAL(s1.GetSigOpCount(false), 0);
 51    BOOST_CHECK_EQUAL(s1.GetSigOpCount(true), 0);
 52
 53    uint160 dummy;
 54    s1 << OP_1 << ToByteVector(dummy) << ToByteVector(dummy) << OP_2 << OP_CHECKMULTISIG;
 55    BOOST_CHECK_EQUAL(s1.GetSigOpCount(true), 2);
 56    s1 << OP_IF << OP_CHECKSIG << OP_ENDIF;
 57    BOOST_CHECK_EQUAL(s1.GetSigOpCount(true), 3);
 58    BOOST_CHECK_EQUAL(s1.GetSigOpCount(false), 21);
 59
 60    CScript p2sh = GetScriptForDestination(ScriptHash(s1));
 61    CScript scriptSig;
 62    scriptSig << OP_0 << Serialize(s1);
 63    BOOST_CHECK_EQUAL(p2sh.GetSigOpCount(scriptSig), 3);
 64
 65    std::vector<CPubKey> keys;
 66    for (int i{0}; i < 3; i++) {
 67        CKey k = GenerateRandomKey();
 68        keys.push_back(k.GetPubKey());
 69    }
 70    CScript s2 = GetScriptForMultisig(1, keys);
 71    BOOST_CHECK_EQUAL(s2.GetSigOpCount(true), 3);
 72    BOOST_CHECK_EQUAL(s2.GetSigOpCount(false), 20);
 73
 74    p2sh = GetScriptForDestination(ScriptHash(s2));
 75    BOOST_CHECK_EQUAL(p2sh.GetSigOpCount(true), 0);
 76    BOOST_CHECK_EQUAL(p2sh.GetSigOpCount(false), 0);
 77    CScript scriptSig2;
 78    scriptSig2 << OP_1 << ToByteVector(dummy) << ToByteVector(dummy) << Serialize(s2);
 79    BOOST_CHECK_EQUAL(p2sh.GetSigOpCount(scriptSig2), 3);
 80}
 81
 82/**
 83 * Verifies script execution of the zeroth scriptPubKey of tx output and
 84 * zeroth scriptSig and witness of tx input.
 85 */
 86static ScriptError VerifyWithFlag(const CTransaction& output, const CMutableTransaction& input, script_verify_flags flags)
 87{
 88    ScriptError error;
 89    CTransaction inputi(input);
 90    bool ret = VerifyScript(inputi.vin[0].scriptSig, output.vout[0].scriptPubKey, &inputi.vin[0].scriptWitness, flags,
 91                            TransactionSignatureChecker(&inputi, 0, output.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &error);
 92    BOOST_CHECK((ret == true) == (error == SCRIPT_ERR_OK));
 93
 94    return error;
 95}
 96
 97/**
 98 * Builds a creationTx from scriptPubKey and a spendingTx from scriptSig
 99 * and witness such that spendingTx spends output zero of creationTx.
100 * Also inserts creationTx's output into the coins view.
101 */
102static void BuildTxs(CMutableTransaction& spendingTx, CCoinsViewCache& coins, CMutableTransaction& creationTx, const CScript& scriptPubKey, const CScript& scriptSig, const CScriptWitness& witness)
103{
104    creationTx.version = 1;
105    creationTx.vin.resize(1);
106    creationTx.vin[0].prevout.SetNull();
107    creationTx.vin[0].scriptSig = CScript();
108    creationTx.vout.resize(1);
109    creationTx.vout[0].nValue = 1;
110    creationTx.vout[0].scriptPubKey = scriptPubKey;
111    BOOST_REQUIRE(CTransaction(creationTx).IsCoinBase());
112
113    spendingTx.version = 1;
114    spendingTx.vin.resize(1);
115    spendingTx.vin[0].prevout.hash = creationTx.GetHash();
116    spendingTx.vin[0].prevout.n = 0;
117    spendingTx.vin[0].scriptSig = scriptSig;
118    spendingTx.vin[0].scriptWitness = witness;
119    spendingTx.vout.resize(1);
120    spendingTx.vout[0].nValue = 1;
121    spendingTx.vout[0].scriptPubKey = CScript();
122    BOOST_REQUIRE(!CTransaction(spendingTx).IsCoinBase());
123
124    AddCoins(coins, CTransaction(creationTx), 0);
125}
126
127BOOST_AUTO_TEST_CASE(GetTxSigOpCost_Multisig)
128{
129    CMutableTransaction creationTx, spendingTx;
130    TestCoinsViewCache test_coins;
131    CPubKey pubkey{GenerateTestPubKey()};
132
133    CScript scriptPubKey = CScript() << 1 << ToByteVector(pubkey) << ToByteVector(pubkey) << 2 << OP_CHECKMULTISIGVERIFY;
134    // Do not use a valid signature to avoid using wallet operations.
135    CScript scriptSig = CScript() << OP_0 << OP_0;
136
137    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, CScriptWitness());
138    // Legacy counting only includes signature operations in scriptSigs and scriptPubKeys
139    // of a transaction and does not take the actual executed sig operations into account.
140    // spendingTx in itself does not contain a signature operation.
141    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
142    // creationTx contains two signature operations in its scriptPubKey, but legacy counting
143    // is not accurate.
144    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), MAX_PUBKEYS_PER_MULTISIG * WITNESS_SCALE_FACTOR);
145
146    // Sanity check: script verification fails because of an invalid signature.
147    BOOST_CHECK_EQUAL(VerifyWithFlag(CTransaction(creationTx), spendingTx, STANDARD_SCRIPT_VERIFY_FLAGS), SCRIPT_ERR_CHECKMULTISIGVERIFY);
148    // Coinbase input sigops are not counted (P2SH included).
149    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(MakeCoinBase(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
150}
151
152BOOST_AUTO_TEST_CASE(GetTxSigOpCost_MultisigP2SH)
153{
154    CMutableTransaction creationTx, spendingTx;
155    TestCoinsViewCache test_coins;
156    CPubKey pubkey{GenerateTestPubKey()};
157
158    CScript redeemScript = CScript() << 1 << ToByteVector(pubkey) << ToByteVector(pubkey) << 2 << OP_CHECKMULTISIGVERIFY;
159    CScript scriptPubKey = GetScriptForDestination(ScriptHash(redeemScript));
160    CScript scriptSig = CScript() << OP_0 << OP_0 << ToByteVector(redeemScript);
161
162    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, CScriptWitness());
163    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 2 * WITNESS_SCALE_FACTOR);
164    BOOST_CHECK_EQUAL(VerifyWithFlag(CTransaction(creationTx), spendingTx, STANDARD_SCRIPT_VERIFY_FLAGS), SCRIPT_ERR_CHECKMULTISIGVERIFY);
165
166    // P2SH sigops are not counted if we don't set the SCRIPT_VERIFY_P2SH flag
167    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, /*flags=*/0), 0);
168
169    // Coinbase tx does not trigger signature operations in the output script
170    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
171    // Coinbase input sigops are not counted (P2SH included).
172    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(MakeCoinBase(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
173}
174
175BOOST_AUTO_TEST_CASE(GetTxSigOpCost_P2WPKH)
176{
177    CMutableTransaction creationTx, spendingTx;
178    TestCoinsViewCache test_coins;
179    CPubKey pubkey{GenerateTestPubKey()};
180
181    CScript scriptPubKey = GetScriptForDestination(WitnessV0KeyHash(pubkey));
182    CScript scriptSig = CScript();
183    CScriptWitness scriptWitness;
184    scriptWitness.stack.emplace_back(0);
185    scriptWitness.stack.emplace_back(0);
186
187    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, scriptWitness);
188    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 1);
189    // No signature operations if we don't verify the witness.
190    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS & ~SCRIPT_VERIFY_WITNESS), 0);
191    BOOST_CHECK_EQUAL(VerifyWithFlag(CTransaction(creationTx), spendingTx, STANDARD_SCRIPT_VERIFY_FLAGS), SCRIPT_ERR_EQUALVERIFY);
192
193    // The sig op cost for witness version != 0 is zero.
194    BOOST_CHECK_EQUAL(scriptPubKey[0], OP_0);
195    scriptPubKey[0] = 0x51;
196    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, scriptWitness);
197    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
198    scriptPubKey[0] = 0x00;
199    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, scriptWitness);
200
201    // Coinbase tx does not trigger witness program validation
202    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
203    // The witness of a coinbase transaction is not taken into account.
204    BOOST_REQUIRE(!spendingTx.vin[0].scriptWitness.IsNull());
205    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(MakeCoinBase(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
206}
207
208BOOST_AUTO_TEST_CASE(GetTxSigOpCost_P2WPKH_P2SH)
209{
210    CMutableTransaction creationTx, spendingTx;
211    TestCoinsViewCache test_coins;
212    CPubKey pubkey{GenerateTestPubKey()};
213
214    CScript scriptSig = GetScriptForDestination(WitnessV0KeyHash(pubkey));
215    CScript scriptPubKey = GetScriptForDestination(ScriptHash(scriptSig));
216    scriptSig = CScript() << ToByteVector(scriptSig);
217    CScriptWitness scriptWitness;
218    scriptWitness.stack.emplace_back(0);
219    scriptWitness.stack.emplace_back(0);
220
221    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, scriptWitness);
222    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 1);
223    BOOST_CHECK_EQUAL(VerifyWithFlag(CTransaction(creationTx), spendingTx, STANDARD_SCRIPT_VERIFY_FLAGS), SCRIPT_ERR_EQUALVERIFY);
224
225    // Coinbase tx does not trigger P2SH witness program validation
226    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
227    // The witness of a coinbase transaction is not taken into account.
228    BOOST_REQUIRE(!spendingTx.vin[0].scriptWitness.IsNull());
229    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(MakeCoinBase(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
230}
231
232BOOST_AUTO_TEST_CASE(GetTxSigOpCost_P2WSH)
233{
234    CMutableTransaction creationTx, spendingTx;
235    TestCoinsViewCache test_coins;
236    CPubKey pubkey{GenerateTestPubKey()};
237
238    CScript witnessScript = CScript() << 1 << ToByteVector(pubkey) << ToByteVector(pubkey) << 2 << OP_CHECKMULTISIGVERIFY;
239    CScript scriptPubKey = GetScriptForDestination(WitnessV0ScriptHash(witnessScript));
240    CScript scriptSig = CScript();
241    CScriptWitness scriptWitness;
242    scriptWitness.stack.emplace_back(0);
243    scriptWitness.stack.emplace_back(0);
244    scriptWitness.stack.emplace_back(witnessScript.begin(), witnessScript.end());
245
246    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, scriptWitness);
247    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 2);
248    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS & ~SCRIPT_VERIFY_WITNESS), 0);
249    BOOST_CHECK_EQUAL(VerifyWithFlag(CTransaction(creationTx), spendingTx, STANDARD_SCRIPT_VERIFY_FLAGS), SCRIPT_ERR_CHECKMULTISIGVERIFY);
250
251    // Coinbase tx does not trigger witness script validation
252    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
253    // The witness of a coinbase transaction is not taken into account.
254    BOOST_REQUIRE(!spendingTx.vin[0].scriptWitness.IsNull());
255    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(MakeCoinBase(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
256}
257
258BOOST_AUTO_TEST_CASE(GetTxSigOpCost_P2WSH_P2SH)
259{
260    CMutableTransaction creationTx, spendingTx;
261    TestCoinsViewCache test_coins;
262    CPubKey pubkey{GenerateTestPubKey()};
263
264    CScript witnessScript = CScript() << 1 << ToByteVector(pubkey) << ToByteVector(pubkey) << 2 << OP_CHECKMULTISIGVERIFY;
265    CScript redeemScript = GetScriptForDestination(WitnessV0ScriptHash(witnessScript));
266    CScript scriptPubKey = GetScriptForDestination(ScriptHash(redeemScript));
267    CScript scriptSig = CScript() << ToByteVector(redeemScript);
268    CScriptWitness scriptWitness;
269    scriptWitness.stack.emplace_back(0);
270    scriptWitness.stack.emplace_back(0);
271    scriptWitness.stack.emplace_back(witnessScript.begin(), witnessScript.end());
272
273    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, scriptWitness);
274    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 2);
275    BOOST_CHECK_EQUAL(VerifyWithFlag(CTransaction(creationTx), spendingTx, STANDARD_SCRIPT_VERIFY_FLAGS), SCRIPT_ERR_CHECKMULTISIGVERIFY);
276
277    // Coinbase tx does not trigger P2SH witness script validation
278    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
279    // The witness of a coinbase transaction is not taken into account.
280    BOOST_REQUIRE(!spendingTx.vin[0].scriptWitness.IsNull());
281    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(MakeCoinBase(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
282}
283
284BOOST_AUTO_TEST_SUITE_END()

Note: during review I also found some dead code that I pushed separately in https://github.com/bitcoin/bitcoin/pull/33786

fanquake referenced this in commit 93e79181da on Nov 7, 2025

test: Enhance GetTxSigOpCost tests for coinbase transactions

Added assertions to the GetTxSigOpCost test cases to verify that the witness of a coinbase transaction is not considered in the signature operation cost calculations.

Using spendingTx.vin[0].prevout.SetNull() we create a coinbase transaction that evaluates to true for IsCoinbase(). Doing this to transactions (spendingTx in this case) that evaluate to a non-zero sigop output, we more concretely test that the witness of a coinbase transaction is not taken into account for SigOp maths.

a4376abd72

test: Add helper functions and update copyright in sigopcount_tests

- Update copyright year to 2012-present
- Add STANDARD_SCRIPT_VERIFY_FLAGS constant to reduce duplication
- Add TestCoinsViewCache fixture for cleaner test setup
- Add GenerateTestPubKey() helper function
- Add MakeCoinBase() helper for creating coinbase transactions
- Improve Serialize() function formatting

These helpers will be used in subsequent test refactoring commits.

Co-Authored-By: l0rinc <pap.lorinc@gmail.com>

f86991ac33

test: Clean up GetSigOpCount test formatting

- Remove unnecessary U suffix from integer literals
- Update for-loop to use brace initialization
- Remove redundant comment
- Improve brace style consistency

Co-Authored-By: l0rinc <pap.lorinc@gmail.com>

c623e58cd5

test: Split GetTxSigOpCost into separate test cases

Split the monolithic GetTxSigOpCost test into 6 focused test cases:
- GetTxSigOpCost_Multisig: Tests legacy multisig sig op counting
- GetTxSigOpCost_MultisigP2SH: Tests multisig nested in P2SH
- GetTxSigOpCost_P2WPKH: Tests P2WPKH witness programs
- GetTxSigOpCost_P2WPKH_P2SH: Tests P2WPKH nested in P2SH
- GetTxSigOpCost_P2WSH: Tests P2WSH witness programs
- GetTxSigOpCost_P2WSH_P2SH: Tests P2WSH nested in P2SH

Benefits:
- Each test is self-contained and can be run independently
- Failures in one test don't break other tests
- Improved test isolation and readability
- Uses TestCoinsViewCache fixture and helper functions
- Better test organization and maintainability

Also adds BOOST_REQUIRE assertions to BuildTxs to verify that
creationTx is a coinbase and spendingTx is not.

Co-Authored-By: l0rinc <pap.lorinc@gmail.com>

dd97b553fc

test: Replace assert() with BOOST_CHECK_EQUAL() in sigopcount tests

Replace all assert() calls with BOOST_CHECK_EQUAL() to provide better
error messages when tests fail. BOOST_CHECK_EQUAL() shows the actual
and expected values on failure, making it easier to debug test failures.

This improves test diagnostics without changing test behavior.

Co-Authored-By: l0rinc <pap.lorinc@gmail.com>

81f08dd7bf

average-gary force-pushed on Nov 7, 2025

average-gary commented at 4:52 pm on November 7, 2025: none

@l0rinc I applied your patch in commits that I hope are focused enough and made you co-author. Thank you.

I’ll find some time to review the rest of the comments and address as needed after this patch.

I also rebased onto 513a0da2e0c89d4833aeeb9f799cf43548d6441f

l0rinc commented at 5:09 pm on November 7, 2025: contributor

Thanks @average-gary. Would it be possible to group the commits so that they’re telling a story, so that the commit messages explain the “why”, not the “what”. The “what” is already in the code. The commit messages seem LLM generated, they don’t add a lot of value this way.

The first commit still adds a test that claims we’re testing the witness when in fact it’s empty: a4376ab (#32840) The second commit adds dead code, only used in later commit - not a clean separation of features. The third does random refactors - we can group these in a cleanup commit, preferably after the test cases are already split.

I would make the test split the first commit, containing the smallest change that can allow us to untangle them, with the minimum helpers since we’re already uindenting all lines, we shouldn’t do anything else that isn’t strictly necessary for the separation. The second commit could do the cleanup, refactor only, low risk - BOOST helpers, suffix removals, useless comments, brace inits, for loops, etc. We can even split the cleanup, but after the tests are separated this should be easy to review. After cleanups, the very last commit could be the one you’re suggesting, done in clean environment for clarity and ease of review.

Let me know if there’s any way I can help.

test: Enhance GetTxSigOpCost tests for coinbase transactions #32840

Code Coverage & Benchmarks

Reviews

Conflicts