doc: warn that CheckBlock() underestimates sigops

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.

The commit message contains some historical context.

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	ismaelsadeeq, ryanofsky

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

DrahtBot added the label Docs on Jan 9, 2025

in src/validation.cpp:4097 in 0ac19a98f3 outdated

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?

ismaelsadeeq commented at 2:18 pm on May 8, 2025:

nit: maybe be specific.

0    // This underestimates the number of sigops, because unlike ConnectBlock it does not count the witness sigops and p2sh sigops:

ismaelsadeeq commented at 3:01 pm on May 8, 2025:

I think it’s okay to have this check here to catch those violations early.

Sjors commented at 4:21 pm on May 8, 2025:

It was introduced by e679ec969c8b22c676ebb10bea1038f6c8f13b33 in preparation for OP_EVAL (which was eventually replaced by P2SH). It seems the initial goal was to have a backward compatible sigop check:

0// This code should be removed when a compatibility-breaking block chain split has passed.
1// Compatibility check for old clients that counted sigops differently:

The commit drops the GetSigOpCount() helper and copy-pasted its implementation into CheckBlock(), where it previously called this method. The same commit implements sigops checking in ConnectBlock.

I still don’t understand in what way this was supposed to help with backward compatibility.

But even if the original intention was for this code to be temporary, things have moved around so much that this would just have to rethought from scratch.

I’ll update the commit message to at least point to this historical commit.

Sjors commented at 4:39 pm on May 8, 2025:

Good catch, and this was helpful in figuring out the history…

ryanofsky approved

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

Code review ACK 0ac19a98f329fe8952f394509a7a29775ab805b0

ismaelsadeeq commented at 3:05 pm on May 8, 2025: member

ACK 0ac19a98f329fe8952f394509a7a29775ab805b0 with a nit and another suggestion

The size limit check above the sigops check also underestimates the block size because it does not include the witness.

doc: warn that CheckBlock() underestimates sigops

Counting sigops in the witness and for p2sh requires
context that CheckBlock()  does not have, so it only
counts a subset of sigops.

The check here was introduced by Satoshi as a "cleanup" in
f1e1fb4bdef878c8fc1564fa418d44e7541a7e83. With the attempted
introduction of OP_EVAL, it was replaced by the check in
ConnectBlock(). Commit e679ec969c8b22c676ebb10bea1038f6c8f13b33
marked this code as a placeholder for backward compatibility.

Then when P2SH replaced OP_EVAL in 922e8e2929a2e78270868385aa46f96002fbcff3
the phrase "compatibility-breaking" was replaced by a simple
observation that before v0.6 this is how sigops were counted.

It's unclear why the check was kept and there were no review comments
about it.

a04f17a188

Sjors force-pushed on May 8, 2025

Sjors commented at 4:41 pm on May 8, 2025: member

I expanded the comment to mention that p2sh isn’t counted either.

I also added some history to the commit message. This seems to be a case of: none of the reviewers of #748 brought it up, but there were no reviewers :-)

Also rebased.

The size limit check above the sigops check also underestimates the block size because it does not include the witness.

Well that’s weird. But better to tackle in a different PR.

ismaelsadeeq approved

ismaelsadeeq commented at 6:45 pm on May 8, 2025: member

ACK a04f17a1882407db09b0a07338e12877ac1d9e92

Thanks for taking my suggestion and providing the context.

I will prefer if the size check is also clarified in same PR. Combining the two in same PR might be better to not miss writing the follow-up.

DrahtBot requested review from ryanofsky on May 8, 2025

in src/validation.cpp:4098 in a04f17a188

4093@@ -4094,6 +4094,8 @@ 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
4098+    // does not count witness and p2sh sigops.

ryanofsky commented at 9:17 pm on May 8, 2025:

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

This comment is good to have but would be nice to clarify:

Why it is not a problem to underestimate?
What this check accomplishes if isn’t accurate?
If it might be better to remove the check or update it?

Might suggest expanding comment to something like “This check underestimates the number of sigops because it does not count witness and p2sh sigops. This is not a problem for validation because ConnectBlock will check them. Having the check here is useful for mining even though it isn’t perfect, and it could be improved in the future.”

Sjors commented at 9:51 am on May 9, 2025:

I don’t think it’s useful at all, given the historical context. But I don’t feel confident enough about dropping it either. That’s why I added the historical context in the commit message, so someone else in the future can try dropping it.

ryanofsky commented at 11:31 am on May 9, 2025:

Thanks for clarifying. I got the impression that you did think it was useful from “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” (https://github.com/bitcoin/bitcoin/pull/31624#discussion_r2053597806). But I guess you were saying it could be useful if it were improved, not that it is useful currently.

Anyway I was mostly asking about this because I was confused. Assuming you are happy with PR in current form I will go ahead and merge this later today based on the two ACKs. Obviously there are more changes that could be made but this is a clear improvement over the status quo.

Sjors commented at 1:05 pm on May 9, 2025:

since it’s relied on by getblocktemplate in proposal mode

That was a mistake on my end, because checkBlock calls connectBlock which does the full check.

l0rinc commented at 7:48 pm on May 9, 2025:

I’m a bit hesitant, because this part of the code has more comments than usual, e.g.

 0/**
 1 * Pre-version-0.6, Bitcoin always counted CHECKMULTISIGs
 2 * as 20 sigops. With pay-to-script-hash, that changed:
 3 * CHECKMULTISIGs serialized in scriptSigs are
 4 * counted more accurately, assuming they are of the form
 5 *  ... OP_N CHECKMULTISIG ...
 6 */
 7unsigned int GetSigOpCount(bool fAccurate) const;
 8
 9/**
10 * Accurately count sigOps, including sigOps in
11 * pay-to-script-hash transactions:
12 */
13unsigned int GetSigOpCount(const CScript& scriptSig) const;

I’m not sure the ones in this PR add to this in a meaningful way.

ryanofsky commented at 7:55 pm on May 9, 2025:

This seems like a meaningful improvement to me because it clearly states that the check is inaccurate, but if there are downsides to merging this let me know!

l0rinc commented at 8:01 pm on May 9, 2025:

clearly states that the check is inaccurate

Isn’t that already clear from calling bool fAccurate with false? In a method called GetLegacySigOpCount with the doc:

0 * Count ECDSA signature operations the old-fashioned (pre-0.6) way

? Seems to me the warnings are already all over the place.

If we need more warnings, can we improve the existing ones instead? Or improve the code to not need so many comments in the first place?

ryanofsky commented at 8:56 pm on May 9, 2025:

Or improve the code to not need so many comments in the first place?

So IIUC having the comment itself is a downside? I think different people use comments differently. I like when comments are there to help me navigate code and call out things that are notable, even repeat things that are “clear,” because I find most comments easier to read than most code, and I like when I can get an overview of what is happening by just reading the comments and not even looking at the code. I think other people don’t use comments like that and see them more like a red flag or tax that has to be paid when code isn’t expressive enough to communicate what is doing.

So I think the disagreement here may mostly come down to personal preference, but this feedback is helpful, and I will hold off merging this to see if there will be another ack or nack.

Also if have a clear alternative you like better feel free to post or link!

l0rinc commented at 9:00 pm on May 9, 2025:

Yes, I usually treat comments as admitting defeat, since if we thought the code was obvious, we wouldn’t feel the need to add more explanation to it. It’s a repetition, a dead weight. But here it’s already over-explained, what’s the exact reason for wanting even more comments (but not even considering code changes like GetSigOpCount(/*fAccurate*/false) - which isn’t just a dead comment since linters are checking it)?

ryanofsky commented at 9:16 pm on May 9, 2025:

But here it’s already over-explained, what’s the exact reason for wanting even more comments

I think we probably disagree that the current code is overexplained. At least 3 of us here think that current code is confusing and a04f17a1882407db09b0a07338e12877ac1d9e92 is an improvement.

(but not even considering code changes like GetSigOpCount(/*fAccurate*/false) - which isn’t just a dead comment since linters are checking it)?

I didn’t consider this just because I didn’t know it was the clear solution you preferred over this PR. It’s easier to compare two compete alternatives than compare a complete proposal with a partial suggestion. If you can implement your preferred solution in a commit, I think that would be great. It looks like a good change.

l0rinc commented at 9:19 pm on May 9, 2025:

It was suggested in #31624 (review) already - but I can do it in a follow-up as well, since I’m working on optimizing these checks anyway - and if you think this comment helps, let’s add it, I won’t block.

ismaelsadeeq commented at 4:20 pm on May 13, 2025:

For me, comments are most useful in the following cases:

When the code is confusing or difficult to understand in such cases, a high-level overview is helpful, especially when the code cannot be simplified.
To provide justification for a function call or explain the rationale behind a particular decision.
To offer warnings, notes, or contextual insights for the reader I actually enjoy reading these kinds of comments, and I’ve noticed this is done in some places in the codebase.
For documenting APIs.

I checked developer-notes.md, and it seems there’s no dedicated section on comments. It would be nice to have one.

What’s currently missing in this PR is a clear explanation of whether the check is redundant or necessary especially given that we already perform a full sigops count check in ConnectBlock. If that rationale were explicitly written, I believe it would be the most important part of the comment, similar to the size check above it.

So, I would categorize this PR as addressing point 3.

Even without this PR, as @l0rinc mentioned, someone could dig into the code and eventually realize that it’s an incomplete check. But I think having a short comment above the function call making it explicit is okay.

That said, I don’t have a strong opinion on this. I just think the comment is correct and also gives an additional history of the check for who wants to work on 3 later hence my ACK.

Sjors commented at 7:05 am on May 14, 2025:

a clear explanation of whether the check is redundant or necessary

The problem is that I don’t know for sure. I think it’s redundant, but I don’t want to mislead some future dev. They should do their own research.

Even without this PR … someone could dig into the code and eventually realize that it’s an incomplete check

The comment is there to prevent someone who doesn’t check from accidentally introducing a consensus bug.

l0rinc commented at 7:09 am on May 14, 2025:

I think we should add tests to avoid consensus bugs, not dead comments - or maybe both, but we definitely shouldn’t rely on comments

Sjors commented at 7:14 am on May 14, 2025:

Unfortunately this function barely has any test coverage. I’m adding some in #31981, but until then a comment is better than nothing.

Sjors commented at 5:10 pm on May 14, 2025:

I rebased #31981 after this was merged and suggested one way we could add test coverage for it, see #31981 (comment)

ryanofsky approved

ryanofsky commented at 9:27 pm on May 8, 2025: contributor

Code review ACK a04f17a1882407db09b0a07338e12877ac1d9e92

Thanks for digging up the history! At least it’s clear how the check get here, though not exactly what it was trying to do.

Sjors commented at 9:52 am on May 9, 2025: member

Combining the two in same PR might be better to not miss writing the follow-up.

Could just open an issue to track it?

in src/validation.cpp:4097 in a04f17a188

4093@@ -4094,6 +4094,8 @@ 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

l0rinc commented at 10:03 am on May 9, 2025:

it does not count witness and p2sh sigops

Isn’t it obvious from the name Get*Legacy*SigOpCount and the parameter GetSigOpCount(bool fAccurate) called as GetSigOpCount(false) that new script types aren’t covered and that it’s not meant to be accurate but rather compatible with old behavior?

Instead (or maybe next to) the comments, I’d find it more helpful if we documented what the false means in GetLegacySigOpCount:

 0unsigned int GetLegacySigOpCount(const CTransaction& tx)
 1{
 2    unsigned int nSigOps = 0;
 3    for (const auto& txin : tx.vin) {
 4        nSigOps += txin.scriptSig.GetSigOpCount(/*fAccurate*/false);
 5    }
 6    for (const auto& txout : tx.vout) {
 7        nSigOps += txout.scriptPubKey.GetSigOpCount(/*fAccurate*/false);
 8    }
 9    return nSigOps;
10}

and maybe to obviate the DecodeOP_N vs MAX_PUBKEYS_PER_MULTISIG accuracy difference which isn’t immediately obvious from the code.

Sjors commented at 10:25 am on May 9, 2025:

Isn’t it obvious from the name

Given our history of bad function names, no :-)

l0rinc changes_requested

l0rinc commented at 10:11 am on May 9, 2025: contributor

I happened to be working on a PR that’s related to the exact same code path.

I’m not sure whether the call is redundant or not, but I can understand why we want to avoid situations like #9049.

I would prefer a more high-level explanation for the difference between the two calls - explained through code and tests preferably instead of comments - or maybe both.

There are a few minor typos in the commit message as well and it’s a bit hand-wavy - can we find some answers here instead of just repeating the review questions in the commit message?

Sjors commented at 10:31 am on May 9, 2025: member

The purpose of this comment isn’t to be throrough, it’s there to:

Warn callers to not rely only on this check
For anyone who wants to get rid of this check, point to the relevant context

I think (2) can be achieved with just a commit message and the fact that our merge script points back to the original PR.

I’m not opposed to writing a longer comment, but I suspect it would take a long time to find a text everyone agrees on. And by the time we do, we might as well change the code itself, which is what I wanted to avoid having to work on.

ryanofsky assigned ryanofsky on May 14, 2025

ryanofsky commented at 1:53 pm on May 14, 2025: contributor

All the points about drawbacks of addressing this issue with a comment (“I usually treat comments as admitting defeat”) are well taken and substantive, but I think it is good to merge this PR anyway because it accurately points out a confusing behavior that could cause bugs. I don’t think we need to admit defeat if we can clear the way for new approaches by not spending too much time debating this particular approach. So am planning to merge this shortly.

ryanofsky merged this on May 14, 2025

ryanofsky closed this on May 14, 2025

l0rinc commented at 2:05 pm on May 16, 2025: contributor

I have pushed the mentioned SigOps calculation optimization, adding the /*fAccurate=*/ argument to each usage, and renamed GetSigOpCount to GetLegacySigOpCount to highlight how this differs from the post-segwit implementation. I’ve also covered it with an extensive suite of tests and a benchmark, see: https://github.com/bitcoin/bitcoin/pull/32532

doc: warn that CheckBlock() underestimates sigops #31624

Code Coverage & Benchmarks

Reviews