test: Enhance GetTxSigOpCost tests for coinbase transactions by average-gary · Pull Request #32840 · bitcoin/bitcoin

average-gary · 2025-06-30T19:04:18Z

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 · 2025-06-30T19:04:22Z

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:

#34124 (refactor: make CCoinsView a purely virtual abstract base class by l0rinc)
#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.

DrahtBot · 2025-06-30T19:11:29Z

🚧 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.}

Hints

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.

sedited · 2025-10-31T13:10:31Z

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


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

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

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.

sedited · 2025-11-03T22:03:08Z

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

        // P2SH sigops are not counted if we don't set the SCRIPT_VERIFY_P2SH flag
        assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, /*flags=*/0) == 0);
+
+        // The witness of a coinbase transaction is not taken into account.


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

sedited · 2025-11-03T22:04:24Z

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



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.

sedited · 2025-11-03T22:07:22Z

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


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.

Found while reviewing bitcoin#32840 The `nullptr` witness path was dead in normal code paths: removing it deletes unreachable logic. Code coverage proof: https://maflcko.github.io/b-c-cov/total.coverage/src/script/interpreter.cpp.gcov.html#L2135

l0rinc

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.

Full patch

// Copyright (c) 2012-present The Bitcoin Core developers
// Distributed under the MIT software license, see the accompanying
// file COPYING or http://www.opensource.org/licenses/mit-license.php.

#include <addresstype.h>
#include <coins.h>
#include <consensus/consensus.h>
#include <consensus/tx_verify.h>
#include <key.h>
#include <pubkey.h>
#include <script/interpreter.h>
#include <script/script.h>
#include <script/solver.h>
#include <test/util/setup_common.h>
#include <uint256.h>

#include <vector>

#include <boost/test/unit_test.hpp>

static constexpr script_verify_flags STANDARD_SCRIPT_VERIFY_FLAGS{SCRIPT_VERIFY_WITNESS | SCRIPT_VERIFY_P2SH};

struct TestCoinsViewCache {
    CCoinsView dummy;
    CCoinsViewCache cache{&dummy};
};

static CPubKey GenerateTestPubKey()
{
    return GenerateRandomKey().GetPubKey();
}

static std::vector<unsigned char> Serialize(const CScript& s)
{
    std::vector<unsigned char> sSerialized(s.begin(), s.end());
    return sSerialized;
}

static CTransaction MakeCoinBase(const CMutableTransaction& tx)
{
    CMutableTransaction coinbase{tx};
    coinbase.vin[0].prevout.SetNull();
    return CTransaction{coinbase};
}

BOOST_FIXTURE_TEST_SUITE(sigopcount_tests, BasicTestingSetup)

BOOST_AUTO_TEST_CASE(GetSigOpCount)
{
    CScript s1;
    BOOST_CHECK_EQUAL(s1.GetSigOpCount(false), 0);
    BOOST_CHECK_EQUAL(s1.GetSigOpCount(true), 0);

    uint160 dummy;
    s1 << OP_1 << ToByteVector(dummy) << ToByteVector(dummy) << OP_2 << OP_CHECKMULTISIG;
    BOOST_CHECK_EQUAL(s1.GetSigOpCount(true), 2);
    s1 << OP_IF << OP_CHECKSIG << OP_ENDIF;
    BOOST_CHECK_EQUAL(s1.GetSigOpCount(true), 3);
    BOOST_CHECK_EQUAL(s1.GetSigOpCount(false), 21);

    CScript p2sh = GetScriptForDestination(ScriptHash(s1));
    CScript scriptSig;
    scriptSig << OP_0 << Serialize(s1);
    BOOST_CHECK_EQUAL(p2sh.GetSigOpCount(scriptSig), 3);

    std::vector<CPubKey> keys;
    for (int i{0}; i < 3; i++) {
        CKey k = GenerateRandomKey();
        keys.push_back(k.GetPubKey());
    }
    CScript s2 = GetScriptForMultisig(1, keys);
    BOOST_CHECK_EQUAL(s2.GetSigOpCount(true), 3);
    BOOST_CHECK_EQUAL(s2.GetSigOpCount(false), 20);

    p2sh = GetScriptForDestination(ScriptHash(s2));
    BOOST_CHECK_EQUAL(p2sh.GetSigOpCount(true), 0);
    BOOST_CHECK_EQUAL(p2sh.GetSigOpCount(false), 0);
    CScript scriptSig2;
    scriptSig2 << OP_1 << ToByteVector(dummy) << ToByteVector(dummy) << Serialize(s2);
    BOOST_CHECK_EQUAL(p2sh.GetSigOpCount(scriptSig2), 3);
}

/**
 * Verifies script execution of the zeroth scriptPubKey of tx output and
 * zeroth scriptSig and witness of tx input.
 */
static ScriptError VerifyWithFlag(const CTransaction& output, const CMutableTransaction& input, script_verify_flags flags)
{
    ScriptError error;
    CTransaction inputi(input);
    bool ret = VerifyScript(inputi.vin[0].scriptSig, output.vout[0].scriptPubKey, &inputi.vin[0].scriptWitness, flags,
                            TransactionSignatureChecker(&inputi, 0, output.vout[0].nValue, MissingDataBehavior::ASSERT_FAIL), &error);
    BOOST_CHECK((ret == true) == (error == SCRIPT_ERR_OK));

    return error;
}

/**
 * Builds a creationTx from scriptPubKey and a spendingTx from scriptSig
 * and witness such that spendingTx spends output zero of creationTx.
 * Also inserts creationTx's output into the coins view.
 */
static void BuildTxs(CMutableTransaction& spendingTx, CCoinsViewCache& coins, CMutableTransaction& creationTx, const CScript& scriptPubKey, const CScript& scriptSig, const CScriptWitness& witness)
{
    creationTx.version = 1;
    creationTx.vin.resize(1);
    creationTx.vin[0].prevout.SetNull();
    creationTx.vin[0].scriptSig = CScript();
    creationTx.vout.resize(1);
    creationTx.vout[0].nValue = 1;
    creationTx.vout[0].scriptPubKey = scriptPubKey;
    BOOST_REQUIRE(CTransaction(creationTx).IsCoinBase());

    spendingTx.version = 1;
    spendingTx.vin.resize(1);
    spendingTx.vin[0].prevout.hash = creationTx.GetHash();
    spendingTx.vin[0].prevout.n = 0;
    spendingTx.vin[0].scriptSig = scriptSig;
    spendingTx.vin[0].scriptWitness = witness;
    spendingTx.vout.resize(1);
    spendingTx.vout[0].nValue = 1;
    spendingTx.vout[0].scriptPubKey = CScript();
    BOOST_REQUIRE(!CTransaction(spendingTx).IsCoinBase());

    AddCoins(coins, CTransaction(creationTx), 0);
}

BOOST_AUTO_TEST_CASE(GetTxSigOpCost_Multisig)
{
    CMutableTransaction creationTx, spendingTx;
    TestCoinsViewCache test_coins;
    CPubKey pubkey{GenerateTestPubKey()};

    CScript scriptPubKey = CScript() << 1 << ToByteVector(pubkey) << ToByteVector(pubkey) << 2 << OP_CHECKMULTISIGVERIFY;
    // Do not use a valid signature to avoid using wallet operations.
    CScript scriptSig = CScript() << OP_0 << OP_0;

    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, CScriptWitness());
    // Legacy counting only includes signature operations in scriptSigs and scriptPubKeys
    // of a transaction and does not take the actual executed sig operations into account.
    // spendingTx in itself does not contain a signature operation.
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
    // creationTx contains two signature operations in its scriptPubKey, but legacy counting
    // is not accurate.
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), MAX_PUBKEYS_PER_MULTISIG * WITNESS_SCALE_FACTOR);

    // Sanity check: script verification fails because of an invalid signature.
    BOOST_CHECK_EQUAL(VerifyWithFlag(CTransaction(creationTx), spendingTx, STANDARD_SCRIPT_VERIFY_FLAGS), SCRIPT_ERR_CHECKMULTISIGVERIFY);
    // Coinbase input sigops are not counted (P2SH included).
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(MakeCoinBase(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
}

BOOST_AUTO_TEST_CASE(GetTxSigOpCost_MultisigP2SH)
{
    CMutableTransaction creationTx, spendingTx;
    TestCoinsViewCache test_coins;
    CPubKey pubkey{GenerateTestPubKey()};

    CScript redeemScript = CScript() << 1 << ToByteVector(pubkey) << ToByteVector(pubkey) << 2 << OP_CHECKMULTISIGVERIFY;
    CScript scriptPubKey = GetScriptForDestination(ScriptHash(redeemScript));
    CScript scriptSig = CScript() << OP_0 << OP_0 << ToByteVector(redeemScript);

    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, CScriptWitness());
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 2 * WITNESS_SCALE_FACTOR);
    BOOST_CHECK_EQUAL(VerifyWithFlag(CTransaction(creationTx), spendingTx, STANDARD_SCRIPT_VERIFY_FLAGS), SCRIPT_ERR_CHECKMULTISIGVERIFY);

    // P2SH sigops are not counted if we don't set the SCRIPT_VERIFY_P2SH flag
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, /*flags=*/0), 0);

    // Coinbase tx does not trigger signature operations in the output script
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
    // Coinbase input sigops are not counted (P2SH included).
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(MakeCoinBase(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
}

BOOST_AUTO_TEST_CASE(GetTxSigOpCost_P2WPKH)
{
    CMutableTransaction creationTx, spendingTx;
    TestCoinsViewCache test_coins;
    CPubKey pubkey{GenerateTestPubKey()};

    CScript scriptPubKey = GetScriptForDestination(WitnessV0KeyHash(pubkey));
    CScript scriptSig = CScript();
    CScriptWitness scriptWitness;
    scriptWitness.stack.emplace_back(0);
    scriptWitness.stack.emplace_back(0);

    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, scriptWitness);
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 1);
    // No signature operations if we don't verify the witness.
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS & ~SCRIPT_VERIFY_WITNESS), 0);
    BOOST_CHECK_EQUAL(VerifyWithFlag(CTransaction(creationTx), spendingTx, STANDARD_SCRIPT_VERIFY_FLAGS), SCRIPT_ERR_EQUALVERIFY);

    // The sig op cost for witness version != 0 is zero.
    BOOST_CHECK_EQUAL(scriptPubKey[0], OP_0);
    scriptPubKey[0] = 0x51;
    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, scriptWitness);
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
    scriptPubKey[0] = 0x00;
    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, scriptWitness);

    // Coinbase tx does not trigger witness program validation
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
    // The witness of a coinbase transaction is not taken into account.
    BOOST_REQUIRE(!spendingTx.vin[0].scriptWitness.IsNull());
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(MakeCoinBase(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
}

BOOST_AUTO_TEST_CASE(GetTxSigOpCost_P2WPKH_P2SH)
{
    CMutableTransaction creationTx, spendingTx;
    TestCoinsViewCache test_coins;
    CPubKey pubkey{GenerateTestPubKey()};

    CScript scriptSig = GetScriptForDestination(WitnessV0KeyHash(pubkey));
    CScript scriptPubKey = GetScriptForDestination(ScriptHash(scriptSig));
    scriptSig = CScript() << ToByteVector(scriptSig);
    CScriptWitness scriptWitness;
    scriptWitness.stack.emplace_back(0);
    scriptWitness.stack.emplace_back(0);

    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, scriptWitness);
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 1);
    BOOST_CHECK_EQUAL(VerifyWithFlag(CTransaction(creationTx), spendingTx, STANDARD_SCRIPT_VERIFY_FLAGS), SCRIPT_ERR_EQUALVERIFY);

    // Coinbase tx does not trigger P2SH witness program validation
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
    // The witness of a coinbase transaction is not taken into account.
    BOOST_REQUIRE(!spendingTx.vin[0].scriptWitness.IsNull());
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(MakeCoinBase(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
}

BOOST_AUTO_TEST_CASE(GetTxSigOpCost_P2WSH)
{
    CMutableTransaction creationTx, spendingTx;
    TestCoinsViewCache test_coins;
    CPubKey pubkey{GenerateTestPubKey()};

    CScript witnessScript = CScript() << 1 << ToByteVector(pubkey) << ToByteVector(pubkey) << 2 << OP_CHECKMULTISIGVERIFY;
    CScript scriptPubKey = GetScriptForDestination(WitnessV0ScriptHash(witnessScript));
    CScript scriptSig = CScript();
    CScriptWitness scriptWitness;
    scriptWitness.stack.emplace_back(0);
    scriptWitness.stack.emplace_back(0);
    scriptWitness.stack.emplace_back(witnessScript.begin(), witnessScript.end());

    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, scriptWitness);
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 2);
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS & ~SCRIPT_VERIFY_WITNESS), 0);
    BOOST_CHECK_EQUAL(VerifyWithFlag(CTransaction(creationTx), spendingTx, STANDARD_SCRIPT_VERIFY_FLAGS), SCRIPT_ERR_CHECKMULTISIGVERIFY);

    // Coinbase tx does not trigger witness script validation
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
    // The witness of a coinbase transaction is not taken into account.
    BOOST_REQUIRE(!spendingTx.vin[0].scriptWitness.IsNull());
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(MakeCoinBase(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
}

BOOST_AUTO_TEST_CASE(GetTxSigOpCost_P2WSH_P2SH)
{
    CMutableTransaction creationTx, spendingTx;
    TestCoinsViewCache test_coins;
    CPubKey pubkey{GenerateTestPubKey()};

    CScript witnessScript = CScript() << 1 << ToByteVector(pubkey) << ToByteVector(pubkey) << 2 << OP_CHECKMULTISIGVERIFY;
    CScript redeemScript = GetScriptForDestination(WitnessV0ScriptHash(witnessScript));
    CScript scriptPubKey = GetScriptForDestination(ScriptHash(redeemScript));
    CScript scriptSig = CScript() << ToByteVector(redeemScript);
    CScriptWitness scriptWitness;
    scriptWitness.stack.emplace_back(0);
    scriptWitness.stack.emplace_back(0);
    scriptWitness.stack.emplace_back(witnessScript.begin(), witnessScript.end());

    BuildTxs(spendingTx, test_coins.cache, creationTx, scriptPubKey, scriptSig, scriptWitness);
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 2);
    BOOST_CHECK_EQUAL(VerifyWithFlag(CTransaction(creationTx), spendingTx, STANDARD_SCRIPT_VERIFY_FLAGS), SCRIPT_ERR_CHECKMULTISIGVERIFY);

    // Coinbase tx does not trigger P2SH witness script validation
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(CTransaction(creationTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
    // The witness of a coinbase transaction is not taken into account.
    BOOST_REQUIRE(!spendingTx.vin[0].scriptWitness.IsNull());
    BOOST_CHECK_EQUAL(GetTransactionSigOpCost(MakeCoinBase(spendingTx), test_coins.cache, STANDARD_SCRIPT_VERIFY_FLAGS), 0);
}

BOOST_AUTO_TEST_SUITE_END()

Note: during review I also found some dead code that I pushed separately in #33786

l0rinc · 2025-11-04T22:10:04Z

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

        // P2SH sigops are not counted if we don't set the SCRIPT_VERIFY_P2SH flag
        assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, /*flags=*/0) == 0);
+
+        // The witness of a coinbase transaction is not taken into account.
+        spendingTx.vin[0].prevout.SetNull();


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

l0rinc · 2025-11-04T22:34:21Z

+        // The witness of a coinbase transaction is not taken into account.
+        spendingTx.vin[0].prevout.SetNull();
+        assert(GetTransactionSigOpCost(CTransaction(spendingTx), coins, flags) == 0);
+        assert(GetTransactionSigOpCost(CTransaction(creationTx), coins, flags) == 0);


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

l0rinc · 2025-11-04T22:36:01Z

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


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

24bcad3 refactor: remove dead code in `CountWitnessSigOps` (Lőrinc) Pull request description: Found while reviewing #32840 The `nullptr` witness path was dead in normal code paths: replacing it with reference enables us deleting unreachable logic. Code coverage proof: https://maflcko.github.io/b-c-cov/total.coverage/src/script/interpreter.cpp.gcov.html#L2135 ACKs for top commit: kevkevinpal: ACK [24bcad3](24bcad3) maflcko: review ACK 24bcad3 🐏 darosior: Neat. utACK 24bcad3. stickies-v: ACK 24bcad3 Tree-SHA512: 92c87e431f06a15d8eeb02e20e9154b272c4586ddacf77c8d83783091485fb82c24ecbd711db7043a92cf6169746db24ad46a5904d694aea9d3c3aa96da725f0

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.

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

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

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>

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>

average-gary · 2025-11-07T16:52:23Z

@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 513a0da

l0rinc · 2025-11-07T17:09:04Z

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.

average-gary · 2026-01-12T15:24:07Z

Apologies for the delay. I am not confident I'll be circling back to this PR any time soon, feel free to close it.

DrahtBot added the Tests label Jun 30, 2025

average-gary force-pushed the 2025-06-30-sigop-coinbase branch from 871c671 to 2449a7c Compare June 30, 2025 19:11

DrahtBot added the CI failed label Jun 30, 2025

average-gary force-pushed the 2025-06-30-sigop-coinbase branch from 2449a7c to 0aef3d9 Compare June 30, 2025 19:25

DrahtBot removed the CI failed label Jun 30, 2025

DrahtBot mentioned this pull request Jun 30, 2025

kernel: Separate UTXO set access from validation functions #32317

Open

This comment was marked as spam.

Sign in to view

DrahtBot added the Needs rebase label Oct 15, 2025

achow101 requested review from Sjors, davidgumberg, l0rinc and theStack October 22, 2025 15:36

average-gary force-pushed the 2025-06-30-sigop-coinbase branch from 0aef3d9 to 6b6b0f4 Compare October 23, 2025 19:41

DrahtBot removed the Needs rebase label Oct 23, 2025

sedited reviewed Oct 31, 2025

View reviewed changes

sedited reviewed Nov 3, 2025

View reviewed changes

l0rinc mentioned this pull request Nov 4, 2025

script: remove dead code in CountWitnessSigOps #33786

Merged

l0rinc suggested changes Nov 4, 2025

View reviewed changes

average-gary and others added 5 commits November 7, 2025 11:50

test: Clean up GetSigOpCount test formatting

c623e58

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

average-gary force-pushed the 2025-06-30-sigop-coinbase branch from 380f4c3 to 81f08dd Compare November 7, 2025 16:50

DrahtBot mentioned this pull request Nov 8, 2025

test, refactor: extract script template helpers and expand sigop coverage #32729

Open

DrahtBot mentioned this pull request Dec 20, 2025

validation: make CCoinsView a pure virtual interface #34124

Merged

fanquake closed this Jan 12, 2026

l0rinc mentioned this pull request Jan 12, 2026

test: clarify coinbase sigop accounting in GetTransactionSigOpCost l0rinc/bitcoin#92

Draft

Uh oh!

Conversation

average-gary commented Jun 30, 2025

Uh oh!

DrahtBot commented Jun 30, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Code Coverage & Benchmarks

Reviews

Conflicts

Uh oh!

DrahtBot commented Jun 30, 2025

Uh oh!

This comment was marked as spam.

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l0rinc left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

average-gary commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

l0rinc commented Nov 7, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

average-gary commented Jan 12, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

DrahtBot commented Jun 30, 2025 •

edited

Loading

l0rinc left a comment •

edited

Loading

average-gary commented Nov 7, 2025 •

edited

Loading

l0rinc commented Nov 7, 2025 •

edited

Loading