Add test coverage for sigop counting in P2SH spends in test_witness_sigops(), addressing the existing TODO.
The test mirrors the existing P2WSH cases by constructing transactions that:
Sigop accounting is adjusted for P2SH by applying the legacy 4x cost factor.
This ensures sigop limits are enforced consistently across both witness and P2SH paths.
<!--e57a25ab6845829454e8d69fc972939a-->
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
<!--006a51241073e994b41acfe9ec718e94-->
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35164.
<!--021abf342d371248e50ceaed478a90ca-->
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| Stale ACK | haishmg |
If your review is incorrectly listed, please copy-paste <code><!--meta-tag:bot-skip--></code> into the comment that the bot should ignore.
<!--5faf32d7da4f0f540f40219e4f7537a3-->
Possible places where comparison-specific test macros should replace generic comparisons:
test/functional/p2p_segwit.py assert p2sh_extra_sigops_available < 100 -> use a comparison helper instead, e.g. assert_greater_than(100, p2sh_extra_sigops_available)<sup>2026-05-16 19:32:08</sup>
ACK f3f1a703137bd9adf492dd67cda3779ae3a2cd8e
Reviewed the added P2SH sigop coverage in test_witness_sigops. The new cases mirror the existing P2WSH boundary checks and exercise below-limit, over-limit, and exact-limit P2SH spends with legacy sigop cost accounting.
Ran locally on macOS arm64:
build/test/functional/p2p_segwit.pygit diff --check upstream/master...HEADI also tried test/lint/lint-python.py test/functional/p2p_segwit.py, but it skipped locally because lief is not installed.
2030 | + 2031 | + tx5.vout.append(CTxOut(total_value, CScript([OP_TRUE]))) 2032 | + 2033 | + # This block should be accepted (sigops exactly at limit) 2034 | + block_8 = self.build_next_block() 2035 | + self.update_witness_block_with_transactions(block_8, [tx5])
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
It looks good. I left some comments to try to optimize the code.
Implement the TODO in test_witness_sigops by adding tests
for sigop accounting in P2SH spends.
Mirror the existing P2WSH cases to verify behavior below,
at, and above the sigop limit. Adjust calculations for the
legacy 4x sigop cost used in P2SH.
Thanks for the review @Bicaru20.
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.
Suggestion taken in e2a4948
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.
I think this additional check is unnecessary because the interaction between input sigops and output sigops is already covered earlier in the test (block_3 and block_4).