VerifyCommitLight and VerifyCommitLightTrusting _never_ check all signatures

[sergio-mena]

Contributes to #1749

Please see the description of #1749 for a thorough description of the problem.

This PR consists of two commits (initially):

• The first commit are changes in our e2e tests that allow for a controlled end-to-end reproduction of the problem, and extra logic in the e2e logic to properly catch it as test failure. Check the little ❌ next to the e2e CI run on this commit to see the error.
• The second commit is the fix on production code. Although the initially proposed fix was to set parameter countAllSignatures to true in all calls to verifyCommitBatch and verifyCommitSingle, it would actually be overkill
  • the light client algorithm does not need to verify all signatures, as it doesn't produce or check any evidence
  • so the proposed fix is rather setting that parameter to true whenever a light client attack evidence is being checked, but to false when it is being used by the light client.
  • I also adapted some of the UTs testing this functionality to also check that VerifyCommitLightandVerifyCommitLightTrustingare also working as expected when called withcountAllSignatures` set to to true
  • Check the little ✅ next to the e2e CI run on this commit to see that the error is now gone (i.e., malformed evidences are rejected, and correctly constructed evidences are accepted)

Since this code was introduced on main via tendermint/tendermint#9255 (in the summer of 2022, after v0.37.x had been cut), only v0.38.x and later show receive a backport PR.

---

PR checklist

• Tests written/updated
• Changelog entry added in .changelog (we use unclog to manage our changelog)
• Updated relevant documentation (docs/ or spec/) and code comments

[insumity]

Remove "does not check all the signatures." because this now depends on countAllSignatures. Similarly for VerifyCommitLightTrusting below.

[insumity]

If the nop parameter in mutateValidatorSet is a no-op, then maybe it would make sense to keep mutateValidatorSet as is and do (?)

if validEvidence {
    pv, conflictingVals, err := mutateValidatorSet(ctx, privVals, vals)
} else {
   // reuse privVals and vals
}

[sergio-mena]

The thing is, what I'd need in the else is inside mutateValidatorSet. So I decided to modify mutateValidatorSet instead

[insumity]

Thanks Sergio!
I left some minor comments but looks really good to me.

Another idea: I wonder if, for simplicity, it would make sense to leave VerifyCommitLight and VerifyCommitLightTrusting as is and just change GetByzantineValidators. GetByzantineValidators could be the one that verifies the signatures and return an error if one of the signatures cannot be verified. Then ValidateABCIEvidence called from inside VerifyLightClientAttack would fail and hence the evidence won't be added in the evidence pool. This would be similar to what Simon did in interchain-security where we verify the signatures when trying to extract the byzantine validators.

[sergio-mena]

@insumity

Another idea

Looks like a good alternative, but I'll stick with the present solution if you don't mind, as we spent some time testing it. Also, the solution laid out here looks more surgical to me (don't forget it is applied to sensitive code we didn't develop and so we're not sure we understand 100% of its corner cases... particularly in the light client).

[insumity]

nit: Maybe rename [...]_ReturnsAsSoonAsMajorityOfVotingPowerSigned as well because it checks something else as well.

[sergio-mena]

Good catch

[insumity]

👍 LGTM

[thanethomson]

Great work folks! 🎉

[thanethomson]

Should we not also backport this to v0.37.x and v0.34.x?

[sergio-mena]

Should we not also backport this to v0.37.x and v0.34.x?

No, this was introduced on main in the summer of 2022 (so v0.37.x was already cut). But your comment makes me realize this is not explained in the description. Let me fix that

[melekes]

👍

[ValarDragon]

(Being pedantic) I actually don't think this is right. This line is using the received second block's commit, whose voteset isn't finalized.

Agreed that every other usage is correct (and important to fix)!

So it should suffice for correctness here that 2/3rds of signatures are valid.

However for reasons noted in #1830 , I think the new change is better, and we refactor the logic to have the next two calls not verify these signatures.

[sergio-mena]

@ValarDragon Not pedantic at all. Spot on, I'd say.

The reason why we are using VerifyCommitLightAllSignatures here is that we decided to leave the previous behavior (i.e., skipping signatures) only in the light client logic, for the rest we took a conservative approach (bear in mind that the raison d'être of this PR was that skipping signatures was problematic when dealing with evidence verification).

However, after careful consideration of your proposal, we realize that the usage of VerifyCommitLight* in blocksync can be seen as a light client-ish verification as well, in that it does not produce or verify any evidence of misbehavior. So we'll open a PR to change this shortly. Bear with us.

[sergio-mena]

#1858