test: check P2SH sigop count for coinbase tx #32850

We currently do not test that GetP2SHSigOpCount returns 0 for coinbase transactions (see line L129 at https://corecheck.dev/mutation/src/consensus/tx_verify.cpp). This PR addresses it.

<!--e57a25ab6845829454e8d69fc972939a-->

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

<!--006a51241073e994b41acfe9ec718e94-->

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32850.

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #29060 (Policy: Report reason inputs are non standard by ismaelsadeeq)

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.

<!--5faf32d7da4f0f540f40219e4f7537a3-->

ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3

Good catch. Verified that on master, indeed all unit and functional tests pass if the tested code part in GetP2SHSigOpCount is changed (or removed), whereas unit tests fail with this PR.

ACK https://github.com/bitcoin/bitcoin/pull/32850/commits/d6aaffcb11adcf47480fcc5081af9dcb732decf3

ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3

the branch is dead code outside of tests, so it could also be removed/disabled. Though, it may be better to add a test than to refactor the consensus code here. So lgtm

Yeah, i think it's preferable to remove dead code than to test it:

diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
index 95466b759cb..5036e26e21a 100644
--- a/src/consensus/tx_verify.cpp
+++ b/src/consensus/tx_verify.cpp
@@ -125,8 +125,7 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx)
 
 unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& inputs)
 {
-    if (tx.IsCoinBase())
-        return 0;
+    Assert(!tx.IsCoinBase());
 
     unsigned int nSigOps = 0;
     for (unsigned int i = 0; i < tx.vin.size(); i++)

(If you do this you might need to adapt fuzz targets.)

That said, i guess unit-tested dead consensus code is better than not-unit-tested dead consensus code. utACK d6aaffcb11adcf47480fcc5081af9dcb732decf3

ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3

(coinbase transactions don’t refer to a real previous output and thus cannot be spending P2SH outputs, making the sigop count irrelevant for them).

Backported to 29.x in #32863.

post-merge crACK