doc: warn that CheckBlock() underestimates sigops #31624

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.

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.

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?

nit: maybe be specific.

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

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

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:

// This code should be removed when a compatibility-breaking block chain split has passed.
// 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.

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

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

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

1. Why it is not a problem to underestimate?
2. What this check accomplishes if isn't accurate?
3. 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."

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.

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.

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.

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

/**
 * Pre-version-0.6, Bitcoin always counted CHECKMULTISIGs
 * as 20 sigops. With pay-to-script-hash, that changed:
 * CHECKMULTISIGs serialized in scriptSigs are
 * counted more accurately, assuming they are of the form
 *  ... OP_N CHECKMULTISIG ...
 */
unsigned int GetSigOpCount(bool fAccurate) const;

/**
 * Accurately count sigOps, including sigOps in
 * pay-to-script-hash transactions:
 */
unsigned int GetSigOpCount(const CScript& scriptSig) const;

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

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!

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:

 * 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?

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!

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

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.

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.

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

1. 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.
2. To provide justification for a function call or explain the rationale behind a particular decision.
3. 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.
4. 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.

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.

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

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

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

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:

unsigned int GetLegacySigOpCount(const CTransaction& tx)
{
    unsigned int nSigOps = 0;
    for (const auto& txin : tx.vin) {
        nSigOps += txin.scriptSig.GetSigOpCount(/*fAccurate*/false);
    }
    for (const auto& txout : tx.vout) {
        nSigOps += txout.scriptPubKey.GetSigOpCount(/*fAccurate*/false);
    }
    return nSigOps;
}

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

Isn't it obvious from the name

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