07-tendermint: ignore misbehaviour if age is greater than consensus params #6422
Conversation
fedekunze
requested review from
aaronc,
alessio,
alexanderbez,
clevinson and
jgimeno
as code owners
| // - ValidatorSet must have 2/3 similarity with trusted FromValidatorSet | ||
| // - ValidatorSets on both headers are valid given the last trusted ValidatorSet | ||
| if err := consensusState.ValidatorSet.VerifyCommitTrusting( | ||
| evidence.ChainID, evidence.Header1.Commit.BlockID, evidence.Header1.Height, | ||
| evidence.Header1.Commit, lite.DefaultTrustLevel, | ||
| evidence.Header1.Commit, clientState.TrustLevel, |
There was a problem hiding this comment.
looks like we were still using the default for misbehavior 👀
Codecov Report
@@ Coverage Diff @@
## master #6422 +/- ##
==========================================
+ Coverage 56.06% 56.14% +0.08%
==========================================
Files 466 466
Lines 27904 27921 +17
==========================================
+ Hits 15643 15675 +32
+ Misses 11149 11140 -9
+ Partials 1112 1106 -6 |
…smos-sdk into fedekunze/6316-evidence-height
Co-authored-by: Anton Kaliaev <anton.kalyaev@gmail.com>
| ageDuration, ageBlocks, consensusParams.Evidence.MaxAgeDuration, consensusParams.Evidence.MaxAgeNumBlocks, | ||
| ) | ||
| } | ||
|
|
||
| // check if provided height matches the headers' height | ||
| if height > uint64(evidence.GetHeight()) { |
There was a problem hiding this comment.
I don't quite understand what this is checking? If we want to make sure that the evidence isn't at a height in the future shouldn't we check the opposite i.e that if height < evidence.GetHeight()
There was a problem hiding this comment.
No, this height is the height from which we retrieve the old header to test from, so it must be before the two conflicting headers.
…6316-evidence-height
…smos-sdk into fedekunze/6316-evidence-height
There was a problem hiding this comment.
I don't think this quite matches Tendermint still, see https://github.com/tendermint/tendermint/blob/95d2d136cdcff2a3bb29d4e4271d757e4f88867c/types/evidence.go#L289 - I don't understand why Tendermint does it this way, though.
@anton or @cmwaters do you know why we sort votes lexicographically based on BlockID? |
As far as I can reason (so far) this is just arbitrary & should be fixed in Tendermint - tendermint/tendermint#5030. |
fedekunze
dismissed
cwgoes’s stale review
Per discussion, I will add an issue to track the tendermint fix and merge it as is
Description
closes: #6316
Also bumped coverage to cover all misbehaviour cases
Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.
docs/) or specification (x/<module>/spec/)godoccomments.Unreleasedsection inCHANGELOG.mdFiles changedin the Github PR explorer