refactor: adding CheckForMisbehaviour to 07-tendermint client #1163
Conversation
|
I think there is some more test scenarios that need to be covered from looking at |
| tmMisbehaviour := msg | ||
|
|
||
| // if heights are equal check that this is valid misbehaviour of a fork | ||
| // otherwise if heights are unequal check that this is valid misbehavior of BFT time violation | ||
| if tmMisbehaviour.Header1.GetHeight().EQ(tmMisbehaviour.Header2.GetHeight()) { | ||
| blockID1, err := tmtypes.BlockIDFromProto(&tmMisbehaviour.Header1.SignedHeader.Commit.BlockID) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| blockID2, err := tmtypes.BlockIDFromProto(&tmMisbehaviour.Header2.SignedHeader.Commit.BlockID) | ||
| if err != nil { | ||
| return false | ||
| } | ||
|
|
||
| // Ensure that Commit Hashes are different | ||
| if !bytes.Equal(blockID1.Hash, blockID2.Hash) { | ||
| return true | ||
| } | ||
| } else { | ||
| // Header1 is at greater height than Header2, therefore Header1 time must be less than or equal to | ||
| // Header2 time in order to be valid misbehaviour (violation of monotonic time). | ||
| if tmMisbehaviour.Header1.SignedHeader.Header.Time.Before(tmMisbehaviour.Header2.SignedHeader.Header.Time) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
I think we can just return true here. I think this should be done in VerifyClientMessage. If the type is Misbehaviour it should be considered invalid if it isn't evidence of misbehaviour
There was a problem hiding this comment.
Okay, I get you! So if the type is indeed Misbehaviour then it should be validated as such in VerifyClientMessage and CheckForMisbehaviour can just directly return true for that type. So CheckForMisbehaviour should only check for time monotonicity of Header types as well as the previous consensus state checks, correct?
I can go ahead and remove this code and the tests for Misbehaviour types.
There was a problem hiding this comment.
correct! Does this API still make sense? It does seem kinda odd that developers need to know how these functions are used. But at the same time, I guess the perspective is "this is misbehaviour, thus I return true" and "this is a header, does it happen to be evidence of misbehaviour?"
There was a problem hiding this comment.
I do agree, at first I thought it was slightly odd, it does require some thinking and prior knowledge that VerifyClientMessage should be called beforehand. But I guess it is a matter of perspective!
My only concern is that someone would use CheckForMisbehaviour with a malformed Misbehaviour type and freeze a client by mistake. But in reality I'm not sure how likely that is to happen, and the new APIs certainly make for cleaner code and an easier flow to reason about.
| // The correctness of Misbehaviour ClientMessage types is ensured by calling VerifyClientMessage prior to this function | ||
| // Thus, here we can return true, as ClientMessage is of type Misbehaviour |
There was a problem hiding this comment.
Added a comment here. Feel free to suggest any changes
damiannolan
requested review from
colin-axner,
seantking,
charleenfei and
crodriguezvega
…/ibc-go into damian/879-check-for-misbehaviour
…#1163) * adding CheckForMisbehaviour to tendermint ClientState * adding initial testcases for CheckForMisbehaviour * updating tests * updating tests * cleaning up code comments * updating godocs * fixing logic and updating tests * removing Misbehaviour verification and tests * fixing code structure after merge conflict
… right errors (cosmos#1162) <!-- Please read and fill out this form before submitting your PR. Please make sure you have reviewed our contributors guide before submitting your first PR. --> ## Overview Closes: cosmos#1163 <!-- Please provide an explanation of the PR, including the appropriate context, background, goal, and rationale. If there is an issue with this information, please provide a tl;dr and link the issue. --> ## Checklist <!-- Please complete the checklist to ensure that the PR is ready to be reviewed. IMPORTANT: PRs should be left in Draft until the below checklist is completed. --> - [x] New and updated code has appropriate documentation - [x] New and updated code has new and/or updated testing - [x] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [x] Linked issues closed with keywords
Description
CheckForMisbehaviourto07-tendermintref: #879 PR3
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 explorerCodecov Reportin the comment section below once CI passes