test: cover P2SH sigop counting in test_witness_sigops #35164

Instead of creating a new transaction, I think we could edit the tx4. That way I think it is clear what is happening and way now the tx is accpeted. Doing this we also keep the behaviour constant as with tx2.

        tx4.vin.pop() # Eliminate the last input with too many sigops
        tx4.vin.append(CTxIn(COutPoint(tx3.txid_int, p2sh_outputs - 1),
                            CScript([redeem_script_justright])))

        # This block should be accepted (sigops exactly at limit)
        block_8 = self.build_next_block()
        self.update_witness_block_with_transactions(block_8, [tx4])

Also this way we can add the test of what happens if we add an output with too many sigops (just as in the previous case with tx2). However I am not sure if this is needed in this case.

tx4.vout.append(CTxOut(0, CScript([OP_CHECKSIG]*1))) # Add the output with too man sigops
block_8 = self.build_next_block()
self.update_witness_block_with_transactions(block_8, [tx4])
test_witness_block(self.nodes[0], self.test_node, block_8, accepted=False, reason='bad-blk-sigops')

tx4.vout.pop() # Eliminate the outputs with too many sigops

These comments are excessive. They make it seem like this was copy-pasted from an LLM without much understanding. I'd prefer if most of them were removed, which would probably also more than halve the diff.

This does indeed catch a consensus drift that would otherwise pass CI:

diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
--- a/src/consensus/tx_verify.cpp	(revision afa5e46bbc6dd750bd71920b659162a945abf0ae)
+++ b/src/consensus/tx_verify.cpp	(revision 0175450a6ae5dc2dd5e8770492d46ca183fcbedd)
@@ -148,7 +148,7 @@
         return nSigOps;

     if (flags & SCRIPT_VERIFY_P2SH) {
-        nSigOps += GetP2SHSigOpCount(tx, inputs) * WITNESS_SCALE_FACTOR;
+        nSigOps += std::min(GetP2SHSigOpCount(tx, inputs), 2'500U) * WITNESS_SCALE_FACTOR;
     }

     for (unsigned int i = 0; i < tx.vin.size(); i++)

With this PR it fails with:

2026-07-24T01:23:19.021298Z TestFramework (ERROR): Unexpected exception:
Traceback (most recent call last):
  File "bitcoin/test/functional/test_framework/test_framework.py", line 145, in main
    self.run_test()
    ~~~~~~~~~~~~~^^
  File "bitcoin/build/test/functional/p2p_segwit.py", line 291, in run_test
    self.test_witness_sigops()
    ~~~~~~~~~~~~~~~~~~~~~~~~^^
  File "bitcoin/build/test/functional/p2p_segwit.py", line 112, in func_wrapper
    func(self, *args, **kwargs)
    ~~~~^^^^^^^^^^^^^^^^^^^^^^^
  File "bitcoin/build/test/functional/p2p_segwit.py", line 2012, in test_witness_sigops
    test_witness_block(self.nodes[0], self.test_node, block_7, accepted=False, reason='bad-blk-sigops')
    ~~~~~~~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "bitcoin/build/test/functional/p2p_segwit.py", line 146, in test_witness_block
    assert_equal(node.getbestblockhash() == block.hash_hex, accepted)
    ~~~~~~~~~~~~^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
  File "bitcoin/test/functional/test_framework/util.py", line 94, in assert_equal
    raise AssertionError("not(%s)" % " == ".join(str(arg) for arg in (thing1, thing2) + args))
AssertionError: not(True == False)

Which revealed another mutation drift that wouldn't be caught by any existing test:

diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
--- a/src/consensus/tx_verify.cpp	(revision 7a585306f2f6fea79706b496b2ebf306e78dd484)
+++ b/src/consensus/tx_verify.cpp	(revision 309a37ffdfb419c87f2fc001fad3b0c611574843)
@@ -147,7 +147,7 @@
     if (tx.IsCoinBase())
         return nSigOps;

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

I added a bug-injection-with-passing-CI/failing-test/revert-bug-passing-CI PR: https://github.com/l0rinc/bitcoin/pull/248

I have simplified the fix presented here, feel free to cherry-pick the fixes (but not the injected bugs) here. Alternatively I can open a new PR and add you as coauthor.

What is this line doing? The test passes without it. I'm also not sure about this case in general. What kind of regression is this supposed to protect against?

I added temporary debug logs around the block_6 / block_7 construction in test/functional/p2p_segwit.py, and in GetTransactionSigOpCost() in src/consensus/tx_verify.cpp, to confirm the actual sigop counts.

The logs show that the old block_7 still failed without the appended witness input because the P2SH portion was already over the limit:

block_6: p2sh_cost=80868 witness_cost=0  total=80868
block_7: p2sh_cost=80868 witness_cost=12 total=80880

The intended regression coverage is mixed P2SH + witness sigop accounting in GetTransactionSigOpCost(): the P2SH cost should be valid by itself, and the added witness input should be what pushes the transaction over the limit.

So the test did not isolate mixed P2SH + witness sigop accounting.

I updated the test so the cases are distinct:

block_6: pure P2SH over limit
         p2sh_cost=80868 witness_cost=0 total=80868
         rejected with bad-blk-sigops

block_7: P2SH + witness over limit
         p2sh_cost=80000 witness_cost=12 total=80012
         rejected with bad-blk-sigops

This catches mutations like:

if ((flags & SCRIPT_VERIFY_P2SH) && !tx.HasWitness()) {
    nSigOps += GetP2SHSigOpCount(tx, inputs) * WITNESS_SCALE_FACTOR;
}

That would incorrectly skip P2SH sigop counting for transactions with witness data. With the new block_7, the mutated code would count only witness_cost=12 and incorrectly accept the block. @l0rinc, hope this matches your original intent.

What is this line doing?

As explained above by @musaHaruna, I suggested this case in #35164 (review) to catch consensus-accounting mutations. The test passes without it because the current consensus code is correct. With the mutation that skips P2SH sigops for transactions containing witness data, it fails as intended.

There are lots of additions in latest push, wouldn't it suffice to comment the line in question (without which consensus invalid code would pass CI):

  # Add witness data to verify that the transaction's P2SH sigops are still counted.
  p2sh_tx.vin.append(CTxIn(COutPoint(tx.txid_int, outputs - 2), b""))