doc: warn that CheckBlock() underestimates sigops #31624

1. 
   Sjors commented at 11:08 am on January 9, 2025: member

   Counting sigops in the witness requires context that CheckBlock() does not have, so it only counts sigops for non-segwit transactions.

   It’s useful to document, but it should not be a problem.

2. doc: warn that CheckBlock() underestimates sigops

   Counting sigops in the witness requires context that CheckBlock()  does not have,
   so it only counts sigops for non-segwit transactions.
   
   It's useful to document, but it should not be a problem.

   0ac19a98f3
3. 
   DrahtBot commented at 11:08 am on January 9, 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/31624.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	ryanofsky

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

4. DrahtBot added the label Docs on Jan 9, 2025
5. in src/validation.cpp:4097 in 0ac19a98f3

   4093@@ -4094,6 +4094,7 @@ bool CheckBlock(const CBlock& block, BlockValidationState& state, const Consensu
   4094                                  strprintf("Transaction check failed (tx hash %s) %s", tx->GetHash().ToString(), tx_state.GetDebugMessage()));
   4095         }
   4096     }
   4097+    // This underestimates the number of sigops, because unlike ConnectBlock it does not count the witness:
   

   ---

   ---

   

   ryanofsky commented at 6:22 pm on April 21, 2025:

   In commit “doc: warn that CheckBlock() underestimates sigops” (0ac19a98f329fe8952f394509a7a29775ab805b0)

   Might be good to say why this check is useful if it’s not a problem for it to be inaccurate.

   Might also be good if PR description mentioned motivation for the change. This all looks ok but it’s a bit confusing because it’s not clear what the context is.

   ---

   

   Sjors commented at 8:21 am on April 22, 2025:

   The original context was that a miner created an invalid block with too many sigops. I wanted to make sure that it was because they made an invalid patch, and not because our code is broken. I wrote this comment while trying to understand the behaviour better.

   It may be a good idea to actually improve the check here, since it’s relied on by getblocktemplate in proposal mode and by #31981. But that would be another PR imo.

   ---

   

   Sjors commented at 12:34 pm on April 25, 2025:

   As explained in #31981 (comment) this is fine. TestBlockValidity also calls the more thorough method.

   I don’t know why this check is here, perhaps just a way to quickly reject a block with lots of sigops?

6. ryanofsky approved
7. 
   ryanofsky commented at 6:23 pm on April 21, 2025: contributor

   Code review ACK 0ac19a98f329fe8952f394509a7a29775ab805b0

Contributors
Sjors DrahtBot ryanofsky

Labels
Docs