Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

refactor: address comments of ICS Misbehaviour PRs #826 and #1148 #1223

Merged
merged 4 commits into from
Aug 23, 2023
Merged

refactor: address comments of ICS Misbehaviour PRs #826 and #1148 #1223

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
0e86503
remove interface
sainoe Aug 21, 2023
4535c2b
improve comment
sainoe Aug 21, 2023
52c34c8
update godoc
sainoe Aug 21, 2023
f54041f
address last comments
sainoe Aug 22, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 3 additions & 9 deletions x/ccv/provider/keeper/misbehaviour.go
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,8 +1,6 @@
package keeper

import (
"sort"

"github.com/cosmos/interchain-security/v2/x/ccv/provider/types"

sdk "github.com/cosmos/cosmos-sdk/types"
Expand Down Expand Up @@ -109,7 +107,6 @@ func (k Keeper) GetByzantineValidators(ctx sdk.Context, misbehaviour ibctmtypes.
}
}

sort.Sort(tmtypes.ValidatorsByVotingPower(validators))
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorting not needed because iterating lightBlock2.Commit.Signatures always results in the same ordering of validators or ordering does not matter?

Were you trying to avoid non-deterministic validator ordering initially?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Initially bc CometBFT is doing it when returning the Byzantine validators, see GetByzantineValidators. However the doc isn't explicit about the requirement, so I dropped it.

return validators, nil
}

Expand All @@ -134,10 +131,6 @@ func headerToLightBlock(h ibctmtypes.Header) (*tmtypes.LightBlock, error) {
// CheckMisbehaviour checks that headers in the given misbehaviour forms
// a valid light client attack and that the corresponding light client isn't expired
func (k Keeper) CheckMisbehaviour(ctx sdk.Context, misbehaviour ibctmtypes.Misbehaviour) error {
if err := misbehaviour.ValidateBasic(); err != nil {
MSalopek marked this conversation as resolved.
Show resolved Hide resolved
return err
}

clientState, found := k.clientKeeper.GetClientState(ctx, misbehaviour.GetClientID())
if !found {
return sdkerrors.Wrapf(ibcclienttypes.ErrClientNotFound, "cannot check misbehaviour for client with ID %s", misbehaviour.GetClientID())
Expand All @@ -147,14 +140,15 @@ func (k Keeper) CheckMisbehaviour(ctx sdk.Context, misbehaviour ibctmtypes.Misbe

// Check that the headers are at the same height to ensure that
// the misbehaviour is for a light client attack and not a time violation,
// see https://github.com/cosmos/ibc-go/blob/8f53c21361f9d65448a850c2eafcf3ab3c384a61/modules/light-clients/07-tendermint/types/misbehaviour_handle.go#L56
// https://github.com/cosmos/ibc-go/blob/v4.2.0/modules/light-clients/07-tendermint/types/misbehaviour_handle.go#L53-L58
if !misbehaviour.Header1.GetHeight().EQ(misbehaviour.Header2.GetHeight()) {
return sdkerrors.Wrap(ibcclienttypes.ErrInvalidMisbehaviour, "headers are not at same height")
}

// CheckMisbehaviourAndUpdateState verifies the misbehaviour against the trusted consensus states
// but does NOT update the light client state.
// Note CheckMisbehaviourAndUpdateState returns an error if the trusted consensus states are expired
// Note that the CometBFT CheckMisbehaviourAndUpdateState method returns an error if the trusted consensus states are expired,
// see https://github.com/cosmos/ibc-go/blob/v4.2.0/modules/light-clients/07-tendermint/types/misbehaviour_handle.go#L120
_, err := clientState.CheckMisbehaviourAndUpdateState(ctx, k.cdc, clientStore, &misbehaviour)
if err != nil {
return err
Expand Down
5 changes: 0 additions & 5 deletions x/ccv/provider/types/codec.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,7 +6,6 @@ import (
sdk "github.com/cosmos/cosmos-sdk/types"
"github.com/cosmos/cosmos-sdk/types/msgservice"
govtypes "github.com/cosmos/cosmos-sdk/x/gov/types"
"github.com/cosmos/ibc-go/v4/modules/core/exported"
)

// RegisterLegacyAminoCodec registers the necessary x/ibc transfer interfaces and concrete types
Expand Down Expand Up @@ -41,10 +40,6 @@ func RegisterInterfaces(registry codectypes.InterfaceRegistry) {
(*sdk.Msg)(nil),
&MsgSubmitConsumerMisbehaviour{},
)
registry.RegisterInterface(
"ibc.core.client.v1.Misbehaviour",
(*exported.Misbehaviour)(nil),
)

msgservice.RegisterMsgServiceDesc(registry, &_Msg_serviceDesc)
}
Expand Down
Loading