moved non-verification misbehaviour checks to checkForMisbehaviour #2971
Conversation
Daniyal98
requested review from
colin-axner,
AdityaSripal,
damiannolan,
crodriguezvega,
charleenfei and
chatton
as code owners
|
hey @Daniyal98, thanks for the pr, there are some failing tests in the 07 tendermint client, i think it's because the test is checking to see if |
There was a problem hiding this comment.
Thanks for getting this started @Daniyal98!
Left some comments regarding the logic as we are now expecting to return a boolean value rather than an error case. I think some of the test cases would also need to be refactored to accommodate the restructuring of the code.
| // 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.
I think this comment can be removed
| blockID1, err := tmtypes.BlockIDFromProto(&msg.Header1.SignedHeader.Commit.BlockID) | ||
| if err != nil { | ||
| return true | ||
| } | ||
|
|
||
| blockID2, err := tmtypes.BlockIDFromProto(&msg.Header2.SignedHeader.Commit.BlockID) | ||
| if err != nil { | ||
| return true | ||
| } |
There was a problem hiding this comment.
Given the method signature only returns a bool I think it would be more appropriate to return false in both of these conditionals. In reality, BlockIDFromProto should never return errors here as its checked in ValidateBasic before this code would be reached.
The alternative option is to panic given its unreachable code, but I think I would rather opt for returning false.
| if bytes.Equal(blockID1.Hash, blockID2.Hash) { | ||
| return true | ||
| } |
There was a problem hiding this comment.
If both headers are at the same height then the hashes must be different in order to be valid fork misbehaviour.
| if bytes.Equal(blockID1.Hash, blockID2.Hash) { | |
| return true | |
| } | |
| if !bytes.Equal(blockID1.Hash, blockID2.Hash) { | |
| return true | |
| } |
| } else if msg.Header1.SignedHeader.Header.Time.After(msg.Header2.SignedHeader.Header.Time) { | ||
| // 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). | ||
| return true |
There was a problem hiding this comment.
| } else if msg.Header1.SignedHeader.Header.Time.After(msg.Header2.SignedHeader.Header.Time) { | |
| // 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). | |
| return true | |
| } else if !msg.Header1.SignedHeader.Header.Time.After(msg.Header2.SignedHeader.Header.Time) { | |
| // 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). | |
| return true |
ValidateBasic ensures that header1 height is greater than or equal to header2 height. Therefore, header1 time must be after header2. If it is not, then we violate time monotonicity and should return true as this is considered misbehaviour.
| // 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). | ||
| return true | ||
| } | ||
| return true |
There was a problem hiding this comment.
If we do not hit a case for valid misbehaviour above we should return false.
edit; this could actually be removed as we would fall out of the switch case and return false on line 82.
|
Hey @Daniyal98, if its okay for you we can push some updates to address some of the comments here if you don't have the capacity. |
|
Hey @damiannolan , sorry, it took me a little to resolve this. I have addressed these problems now. Let me know if there is anything missing. Thank you! |
|
hey @Daniyal98 it looks like there are still failing tests -- would you mind taking a look and fixing? or let us know if we should take over this pr. thank you! |
|
Hey @charleenfei, I am unable to find the time nowadays to work on this. Could you please take over this PR? Thank you |
Thanks for letting us know @Daniyal98, we can take this over. Thanks for the work you've done on it, much appreciated! ❤️ |
|
closing in favour of #3046 |
Description
closes: #2007
Commit Message / Changelog Entry
see the guidelines for commit messages. (view raw markdown for examples)
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.Files changedin the Github PR explorer.Codecov Reportin the comment section below once CI passes.