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.

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

feat: update misbehaviour handling using IBC-Go 7 #1401

Merged
merged 13 commits into from
Nov 15, 2023
101 changes: 56 additions & 45 deletions tests/integration/misbehaviour.go
Original file line number Diff line number Diff line change
Expand Up @@ -385,27 +385,55 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
altSigners[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].Address.String()]
altSigners[clientTMValset.Validators[1].Address.String()] = clientSigners[clientTMValset.Validators[1].Address.String()]

// create an alternative validator set using less than 1/3 of the trusted validator set
altValset2 := tmtypes.NewValidatorSet(s.consumerChain.Vals.Validators[0:1])
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: should we move those after clientHeader because they're not used in clientHeader?

altSigners2 := make(map[string]tmtypes.PrivValidator, 1)
altSigners2[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].Address.String()]

clientHeader := s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
int64(clientHeight.RevisionHeight+1),
clientHeight,
headerTs,
clientTMValset,
clientTMValset,
clientTMValset,
clientSigners,
)

// create a conflicting client header with insufficient voting power
clientHeader2 := s.consumerChain.CreateTMClientHeader(
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: shall we rename clientHeader2 to clientHeaderWithInsufficientVotingPower?

s.consumerChain.ChainID,
int64(clientHeight.RevisionHeight+1),
clientHeight,
// use a different block time to change the header BlockID
headerTs.Add(time.Hour),
altValset2,
altValset2,
clientTMValset,
altSigners2,
)

testCases := []struct {
name string
misbehaviour *ibctmtypes.Misbehaviour
expPass bool
}{
{
"client state not found - shouldn't pass",
"identical headers - shouldn't pass",
&ibctmtypes.Misbehaviour{
ClientId: "clientID",
ClientId: s.path.EndpointA.ClientID,
Header1: clientHeader,
Header2: clientHeader,
},
false,
},
{
"misbehaviour isn't for a consumer chain - shouldn't pass",
&ibctmtypes.Misbehaviour{
ClientId: s.path.EndpointA.ClientID,
Header1: s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
int64(clientHeight.RevisionHeight+1),
clientHeight,
headerTs,
clientTMValset,
clientTMValset,
clientTMValset,
clientSigners,
),
Header2: s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
"aChainID",
int64(clientHeight.RevisionHeight+1),
clientHeight,
headerTs,
Expand All @@ -414,13 +442,15 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
clientTMValset,
altSigners,
),
Header2: clientHeader,
},
false,
},
{
"invalid misbehaviour with empty header1 - shouldn't pass",
"client ID doesn't correspond to the client ID of consumer chain - shouldn't pass",
&ibctmtypes.Misbehaviour{
Header1: &ibctmtypes.Header{},
ClientId: "clientID",
Header1: clientHeader,
Header2: s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
int64(clientHeight.RevisionHeight+1),
Expand All @@ -438,16 +468,7 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
"invalid misbehaviour with different header height - shouldn't pass",
&ibctmtypes.Misbehaviour{
ClientId: s.path.EndpointA.ClientID,
Header1: s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
int64(clientHeight.RevisionHeight+1),
clientHeight,
headerTs,
clientTMValset,
clientTMValset,
clientTMValset,
clientSigners,
),
Header1: clientHeader,
Header2: s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
int64(clientHeight.RevisionHeight+2),
Expand All @@ -461,30 +482,20 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
},
false,
},
// TODO: should pass after 1401 is merged
// {
// "one header of the misbehaviour has insufficient voting power - shouldn't pass",
// &ibctmtypes.Misbehaviour{
// ClientId: s.path.EndpointA.ClientID,
// Header1: clientHeader,
// Header2: clientHeader2,
// },
// false,
// },
{
"one header of the misbehaviour has insufficient voting power - shouldn't pass",
&ibctmtypes.Misbehaviour{
ClientId: s.path.EndpointA.ClientID,
Header1: clientHeader,
Header2: clientHeader2,
},
false,
},
{
"valid misbehaviour - should pass",
&ibctmtypes.Misbehaviour{
ClientId: s.path.EndpointA.ClientID,
Header1: s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
int64(clientHeight.RevisionHeight+1),
clientHeight,
headerTs,
clientTMValset,
clientTMValset,
clientTMValset,
clientSigners,
),
Header1: clientHeader,
// create header using a different validator set
Header2: s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
Expand Down
24 changes: 21 additions & 3 deletions x/ccv/provider/keeper/misbehaviour.go
Original file line number Diff line number Diff line change
Expand Up @@ -83,11 +83,11 @@
// construct the trusted and conflicted light blocks
lightBlock1, err := headerToLightBlock(*misbehaviour.Header1)
if err != nil {
return

Check failure on line 86 in x/ccv/provider/keeper/misbehaviour.go

View workflow job for this annotation

GitHub Actions / lint

naked return in func `GetByzantineValidators` with 53 lines of code (nakedret)
}
lightBlock2, err := headerToLightBlock(*misbehaviour.Header2)
if err != nil {
return

Check failure on line 90 in x/ccv/provider/keeper/misbehaviour.go

View workflow job for this annotation

GitHub Actions / lint

naked return in func `GetByzantineValidators` with 53 lines of code (nakedret)
}

// Check if the misbehaviour corresponds to an Amnesia attack,
Expand All @@ -97,7 +97,7 @@
//
// Note that we cannot differentiate which of the headers is trusted or malicious,
if !headersStateTransitionsAreConflicting(*lightBlock1.Header, *lightBlock2.Header) && lightBlock1.Commit.Round != lightBlock2.Commit.Round {
return

Check failure on line 100 in x/ccv/provider/keeper/misbehaviour.go

View workflow job for this annotation

GitHub Actions / lint

naked return in func `GetByzantineValidators` with 53 lines of code (nakedret)
}

// compare the signatures of the headers
Expand Down Expand Up @@ -153,9 +153,21 @@
}

// CheckMisbehaviour checks that headers in the given misbehaviour forms
// a valid light client attack and that the corresponding light client isn't expired
// a valid light client attack from an ICS consumer chain and that the light client isn't expired
func (k Keeper) CheckMisbehaviour(ctx sdk.Context, misbehaviour ibctmtypes.Misbehaviour) error {
clientState, found := k.clientKeeper.GetClientState(ctx, misbehaviour.ClientId)
// check that the misbehaviour is for an ICS consumer chain
clientId, found := k.GetConsumerClientId(ctx, misbehaviour.Header1.Header.ChainID)
if !found {
return fmt.Errorf("incorrect misbehaviour with conflicting headers from a non-existent consumer chain: %s", misbehaviour.Header1.Header.ChainID)
} else if misbehaviour.ClientId != clientId {
return fmt.Errorf("incorrect misbehaviour: expected client ID for consumer chain %s is %s got %s",
misbehaviour.Header1.Header.ChainID,
clientId,
misbehaviour.ClientId,
)
}

clientState, found := k.clientKeeper.GetClientState(ctx, clientId)
if !found {
return errorsmod.Wrapf(ibcclienttypes.ErrClientNotFound, "cannot check misbehaviour for client with ID %s", misbehaviour.ClientId)
}
Expand All @@ -176,7 +188,13 @@
return errorsmod.Wrapf(ibcclienttypes.ErrInvalidMisbehaviour, "invalid misbehaviour for client-id: %s", misbehaviour.ClientId)
}

// TODO check misb valset signatures here
// VerifyClientMessage calls verifyMisbehaviour which verifies that the headers in the misbehaviour
Copy link
Contributor

Choose a reason for hiding this comment

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

It doesn't seem I can comment in previous lines but we would still need to verify the clientId is a consumer client, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! Just cherry-picked the clientID verification from the #1404.

Copy link
Contributor

Choose a reason for hiding this comment

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

In the previous comment where you have:


	// CheckMisbehaviour verifies that the headers have both he same block height and
	// different blockID hashes

you can remove the "headers have the same block height" because you do the check before as well. In this case we only call CheckMisbehaviour to verify that blockIDs hashes are different.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Updated in fda0b0f8461.

// are valid against their respective trusted consensus states and that at least a TrustLevel of the validator set signed their commit,
// see checkMisbehaviourHeader in ibc-go/blob/v7.3.0/modules/light-clients/07-tendermint/misbehaviour_handle.go#L126
if err := clientState.VerifyClientMessage(ctx, k.cdc, clientStore, &misbehaviour); err != nil {
return err
}

return nil
}

Expand Down
Loading