test: check P2SH sigop count for coinbase tx #32850

1. 
   brunoerg commented at 3:10 pm on July 1, 2025: contributor

   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.
2. test: check P2SH sigop count for coinbase tx d6aaffcb11
3. DrahtBot added the label Tests on Jul 1, 2025
4. 
   DrahtBot commented at 3:10 pm on July 1, 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/32850.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	theStack, w0xlt, ishaanam, darosior, pablomartin4btc

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

   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.

5. theStack approved
6. 
   theStack commented at 5:25 pm on July 1, 2025: contributor

   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.

7. 
   w0xlt commented at 6:42 pm on July 1, 2025: contributor

   ACK https://github.com/bitcoin/bitcoin/pull/32850/commits/d6aaffcb11adcf47480fcc5081af9dcb732decf3
8. 
   ishaanam commented at 7:44 pm on July 1, 2025: contributor

   ACK d6aaffcb11adcf47480fcc5081af9dcb732decf3
9. bitcoin deleted a comment on Jul 2, 2025
10. 
    maflcko commented at 7:49 am on July 2, 2025: member

    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
11. 
    darosior commented at 9:23 am on July 2, 2025: member

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

     0diff --git a/src/consensus/tx_verify.cpp b/src/consensus/tx_verify.cpp
     1index 95466b759cb..5036e26e21a 100644
     2--- a/src/consensus/tx_verify.cpp
     3+++ b/src/consensus/tx_verify.cpp
     4@@ -125,8 +125,7 @@ unsigned int GetLegacySigOpCount(const CTransaction& tx)
     5 
     6 unsigned int GetP2SHSigOpCount(const CTransaction& tx, const CCoinsViewCache& inputs)
     7 {
     8-    if (tx.IsCoinBase())
     9-        return 0;
    10+    Assert(!tx.IsCoinBase());
    11 
    12     unsigned int nSigOps = 0;
    13     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

12. 
    pablomartin4btc commented at 11:41 pm on July 2, 2025: member

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

13. fanquake merged this on Jul 3, 2025
14. fanquake closed this on Jul 3, 2025

    ---

15. fanquake referenced this in commit 222fbfcc6a on Jul 3, 2025
16. 
    fanquake commented at 11:17 am on July 3, 2025: member

    Backported to 29.x in #32863.
17. 
    l0rinc commented at 11:32 am on July 3, 2025: contributor

    post-merge crACK
18. brunoerg deleted the branch on Jul 3, 2025

Contributors
brunoerg DrahtBot theStack w0xlt ishaanam maflcko darosior pablomartin4btc fanquake l0rinc

Labels
Tests