02-client refactor: add CheckForMisbehaviour & UpdateOnMisbehaviour fns #1116
Conversation
seantking
requested review from
colin-axner,
fedekunze and
AdityaSripal
as code owners
| func (cs ClientState) CheckForMisbehaviour( | ||
| ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, | ||
| msg exported.ClientMessage, | ||
| ) (bool, error) { |
There was a problem hiding this comment.
Returning an optional error here to allow for more fine-grained error handling (see Misbehaviour section). Not sure if this is golang best practice.
I think it would be good to sync on our function signatures for these new functions @damiannolan @colin-axner @AdityaSripal
There was a problem hiding this comment.
VerifyClientMessage should return the error. We can panic here (as that implies VerifyClientMessage wasn't called first)
Codecov Report
@@ Coverage Diff @@
## sean/issue#879-rename-check-validity-verify-header #1116 +/- ##
======================================================================================
+ Coverage 79.72% 79.75% +0.02%
======================================================================================
Files 151 151
Lines 10899 10914 +15
======================================================================================
+ Hits 8689 8704 +15
Misses 1786 1786
Partials 424 424
|
|
|
||
| // UpdateStateOnMisbehaviour updates state upon misbehaviour. This method should only be called on misbehaviour | ||
| // as it does not perform any misbehaviour checks. | ||
| func (cs ClientState) UpdateStateOnMisbehaviour( |
There was a problem hiding this comment.
Just realized we should probably refactor the tests as well so there's a specific test for these new functions. (I noticed coverage actually went up somehow with the refactor though :-) )
There was a problem hiding this comment.
Yes, there should be unit tests for each function we add
| func (cs ClientState) CheckForMisbehaviour( | ||
| ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore, | ||
| msg exported.ClientMessage, | ||
| ) (bool, error) { |
There was a problem hiding this comment.
VerifyClientMessage should return the error. We can panic here (as that implies VerifyClientMessage wasn't called first)
|
|
||
| // UpdateStateOnMisbehaviour updates state upon misbehaviour. This method should only be called on misbehaviour | ||
| // as it does not perform any misbehaviour checks. | ||
| func (cs ClientState) UpdateStateOnMisbehaviour( |
There was a problem hiding this comment.
Yes, there should be unit tests for each function we add
| } | ||
| foundMisbehaviour, err := cs.CheckForMisbehaviour(ctx, cdc, clientStore, tmMisbehaviour) | ||
| if foundMisbehaviour { | ||
| return nil, err | ||
| } | ||
|
|
||
| if err := cs.ValidateClientMessage(ctx, clientStore, cdc, nil, misbehaviour, ctx.BlockTime()); err != nil { |
There was a problem hiding this comment.
VerifyClientMessage will be occurring before CheckForMisbehaviour. It doesn't matter though since we will delete this function
| if tmMisbehaviour.Header1.GetHeight().EQ(tmMisbehaviour.Header2.GetHeight()) { | ||
| blockID1, err := tmtypes.BlockIDFromProto(&tmMisbehaviour.Header1.SignedHeader.Commit.BlockID) | ||
| if err != nil { | ||
| return true, sdkerrors.Wrap(err, "invalid block ID from header 1 in misbehaviour") |
There was a problem hiding this comment.
the bool should be false here
seantking
changed the base branch from
sean/issue#879-rename-check-validity-verify-header
to
02-client-refactor
seantking
changed the base branch from
02-client-refactor
to
sean/issue#879-rename-check-validity-verify-header
<!-- 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 There was a potential deadlock for a channel capacity of zero. This code removes that deadlock and adds a comment explaining the code more. Related to cosmos#1069 <!-- 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. --> - [ ] New and updated code has appropriate documentation - [ ] New and updated code has new and/or updated testing - [ ] Required CI checks are passing - [ ] Visual proof for any user facing features like CLI or documentation updates - [ ] Linked issues closed with keywords
Description
Merging into #1115.
CheckForMisbehaviourandUpdateOnMisbehaviourfunctionsCheckForMisbehaviourAndUpdateStateandCheckHeaderAndUpdateState.partially closes: #879
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