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

fix!: drop nil votes in misbehaviour handling #1404

Merged
merged 14 commits into from
Nov 10, 2023
162 changes: 93 additions & 69 deletions tests/integration/misbehaviour.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ import (

sdk "github.com/cosmos/cosmos-sdk/types"
ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types"
testutil "github.com/cosmos/interchain-security/v2/testutil/crypto"
"github.com/cosmos/interchain-security/v2/x/ccv/provider/types"
tmtypes "github.com/tendermint/tendermint/types"
)
Expand Down Expand Up @@ -211,6 +212,43 @@ func (s *CCVTestSuite) TestGetByzantineValidators() {
[]*tmtypes.Validator{},
true,
},
{
"validators who did vote nil should not be returned",
func() *ibctmtypes.Misbehaviour {
clientHeader := s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
int64(clientHeight.RevisionHeight+1),
clientHeight,
altTime.Add(time.Minute),
clientTMValset,
clientTMValset,
clientTMValset,
clientSigners,
)

// create conflicting header with 2/4 validators voting nil
clientHeaderWithNilVotes := s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
int64(clientHeight.RevisionHeight+1),
clientHeight,
altTime.Add(time.Hour),
clientTMValset,
clientTMValset,
clientTMValset,
clientSigners,
)
testutil.UpdateHeaderCommitWithNilVotes(clientHeaderWithNilVotes, clientTMValset.Validators[:2])

return &ibctmtypes.Misbehaviour{
ClientId: s.path.EndpointA.ClientID,
Header1: clientHeader,
Header2: clientHeaderWithNilVotes,
}
},
// Expect validators who did NOT vote nil
clientTMValset.Validators[2:],
true,
},
}

for _, tc := range testCases {
Expand All @@ -223,21 +261,17 @@ func (s *CCVTestSuite) TestGetByzantineValidators() {
s.NoError(err)
s.Equal(len(tc.expByzantineValidators), len(byzantineValidators))

// For both lunatic and equivocation attacks all the validators
// who signed the bad header (Header2) should be in returned in the evidence
// For both lunatic and equivocation attacks, all the validators
// who signed both headers and didn't vote nil should be returned
if len(tc.expByzantineValidators) > 0 {
equivocatingVals := tc.getMisbehaviour().Header2.ValidatorSet
s.Equal(len(equivocatingVals.Validators), len(byzantineValidators))

vs, err := tmtypes.ValidatorSetFromProto(equivocatingVals)
expValset := tmtypes.NewValidatorSet(tc.expByzantineValidators)
s.NoError(err)

for _, v := range tc.expByzantineValidators {
idx, _ := vs.GetByAddress(v.Address)
idx, _ := expValset.GetByAddress(v.Address)
s.True(idx >= 0)
}
}

} else {
s.Error(err)
}
Expand Down Expand Up @@ -268,27 +302,53 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
altSigners := make(map[string]tmtypes.PrivValidator, 1)
altSigners[clientTMValset.Validators[0].Address.String()] = clientSigners[clientTMValset.Validators[0].Address.String()]
altSigners[clientTMValset.Validators[1].Address.String()] = clientSigners[clientTMValset.Validators[1].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
// by changing 3/4 of its validators votes to nil
clientHeaderWithNilVotes := s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
int64(clientHeight.RevisionHeight+1),
clientHeight,
// use a different block time to change the header BlockID
headerTs.Add(time.Hour),
clientTMValset,
clientTMValset,
clientTMValset,
clientSigners,
)
testutil.UpdateHeaderCommitWithNilVotes(clientHeaderWithNilVotes, clientTMValset.Validators[:4])

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 @@ -297,13 +357,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 @@ -321,16 +383,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 @@ -345,49 +398,20 @@ func (s *CCVTestSuite) TestCheckMisbehaviour() {
false,
},
{
"valid misbehaviour - should pass",
"one header of the misbehaviour has insufficient voting power - 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,
),
// create header using a different validator set
Header2: s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
int64(clientHeight.RevisionHeight+1),
clientHeight,
headerTs,
altValset,
altValset,
clientTMValset,
altSigners,
),
Header1: clientHeader,
Header2: clientHeaderWithNilVotes,
},
true,
false,
},
{
"valid misbehaviour with already frozen client - should pass",
"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,
),
// the resulting Header2 will have a different BlockID
// than Header1 since doesn't share the same valset and signers
Header1: clientHeader,
// create header using a different validator set
Header2: s.consumerChain.CreateTMClientHeader(
s.consumerChain.ChainID,
int64(clientHeight.RevisionHeight+1),
Expand Down
39 changes: 39 additions & 0 deletions testutil/crypto/evidence.go
Original file line number Diff line number Diff line change
@@ -1,8 +1,10 @@
package crypto

import (
"fmt"
"time"

ibctmtypes "github.com/cosmos/ibc-go/v4/modules/light-clients/07-tendermint/types"
"github.com/tendermint/tendermint/crypto/tmhash"
tmtypes "github.com/tendermint/tendermint/types"
)
Expand Down Expand Up @@ -89,3 +91,40 @@ func MakeAndSignVoteWithForgedValAddress(
vote.Signature = v.Signature
return vote
}

// UpdateHeaderCommitWithNilVotes updates the given light client header
// by changing the commit BlockIDFlag of the given validators to nil
//
// Note that this method is solely used for testing purposes
func UpdateHeaderCommitWithNilVotes(header *ibctmtypes.Header, validators []*tmtypes.Validator) {
if len(validators) > len(header.ValidatorSet.Validators) {
panic(fmt.Sprintf("cannot change more than %d validators votes: got %d",
len(header.ValidatorSet.Validators), len(header.ValidatorSet.Validators)))
}

commit, err := tmtypes.CommitFromProto(header.Commit)
if err != nil {
panic(err)
}

valset, err := tmtypes.ValidatorSetFromProto(header.ValidatorSet)
if err != nil {
panic(err)
}

for _, v := range validators {
// get validator index in valset
idx, _ := valset.GetByAddress(v.Address)
insumity marked this conversation as resolved.
Show resolved Hide resolved
if idx != -1 {
// get validator commit sig
s := commit.Signatures[idx]
// change BlockIDFlag to nil
s.BlockIDFlag = tmtypes.BlockIDFlagNil
// update the signatures
commit.Signatures[idx] = s
}
}

// update the commit in client the header
header.SignedHeader.Commit = commit.ToProto()
}
20 changes: 16 additions & 4 deletions x/ccv/provider/keeper/misbehaviour.go
Original file line number Diff line number Diff line change
Expand Up @@ -103,15 +103,15 @@ func (k Keeper) GetByzantineValidators(ctx sdk.Context, misbehaviour ibctmtypes.
// create a map with the validators' address that signed header1
header1Signers := map[string]struct{}{}
for _, sign := range lightBlock1.Commit.Signatures {
if sign.Absent() {
if !sign.ForBlock() {
continue
}
header1Signers[sign.ValidatorAddress.String()] = struct{}{}
}

// iterate over the header2 signers and check if they signed header1
for _, sign := range lightBlock2.Commit.Signatures {
if sign.Absent() {
if !sign.ForBlock() {
continue
}
if _, ok := header1Signers[sign.ValidatorAddress.String()]; ok {
Expand Down Expand Up @@ -142,9 +142,21 @@ 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
// 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.GetClientID())
// 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)
Copy link
Contributor

Choose a reason for hiding this comment

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

Below in:

	clientStore := k.clientKeeper.ClientStore(ctx, misbehaviour.GetClientID())

there's discrepancy in how we use misbehaviour.GetClientID() versus misbehaviour.ClientId versus simply clientId. I believe it makes sense to use everywhere clientId.

if !found {
return sdkerrors.Wrapf(ibcclienttypes.ErrClientNotFound, "cannot check misbehaviour for client with ID %s", misbehaviour.GetClientID())
}
Expand Down
Loading