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: make HandleConsumerDoubleVoting works with provider pubkeys #1254

Merged
merged 6 commits into from
Sep 4, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
104 changes: 77 additions & 27 deletions tests/integration/double_vote.go
Original file line number Diff line number Diff line change
Expand Up @@ -19,32 +19,58 @@ func (s *CCVTestSuite) TestHandleConsumerDoubleVoting() {
s.setDefaultValSigningInfo(*v)
}

valSet, err := tmtypes.ValidatorSetFromProto(s.consumerChain.LastHeader.ValidatorSet)
consuValSet, err := tmtypes.ValidatorSetFromProto(s.consumerChain.LastHeader.ValidatorSet)
s.Require().NoError(err)
consuVal := consuValSet.Validators[0]
s.Require().NoError(err)
consuSigner := s.consumerChain.Signers[consuVal.Address.String()]

val := valSet.Validators[0]
signer := s.consumerChain.Signers[val.Address.String()]
provValSet, err := tmtypes.ValidatorSetFromProto(s.providerChain.LastHeader.ValidatorSet)
s.Require().NoError(err)
provVal := provValSet.Validators[0]
provSigner := s.providerChain.Signers[provVal.Address.String()]

blockID1 := testutil.MakeBlockID([]byte("blockhash"), 1000, []byte("partshash"))
blockID2 := testutil.MakeBlockID([]byte("blockhash2"), 1000, []byte("partshash"))

// Note that votes are signed along with the chain ID
// see VoteSignBytes in https://github.com/cometbft/cometbft/blob/main/types/vote.go#L139
vote1 := testutil.MakeAndSignVote(

// create two votes using the consumer validator key
consuVote := testutil.MakeAndSignVote(
blockID1,
s.consumerCtx().BlockHeight(),
s.consumerCtx().BlockTime(),
consuValSet,
consuSigner,
s.consumerChain.ChainID,
)

consuBadVote := testutil.MakeAndSignVote(
blockID2,
s.consumerCtx().BlockHeight(),
s.consumerCtx().BlockTime(),
consuValSet,
consuSigner,
s.consumerChain.ChainID,
)

// create two votes using the provider validator key
provVote := testutil.MakeAndSignVote(
blockID1,
s.consumerCtx().BlockHeight(),
s.consumerCtx().BlockTime(),
valSet,
signer,
provValSet,
provSigner,
s.consumerChain.ChainID,
)

badVote := testutil.MakeAndSignVote(
provBadVote := testutil.MakeAndSignVote(
blockID2,
s.consumerCtx().BlockHeight(),
s.consumerCtx().BlockTime(),
valSet,
signer,
provValSet,
provSigner,
s.consumerChain.ChainID,
)

Expand All @@ -57,23 +83,23 @@ func (s *CCVTestSuite) TestHandleConsumerDoubleVoting() {
{
"invalid consumer chain id - shouldn't pass",
&tmtypes.DuplicateVoteEvidence{
VoteA: vote1,
VoteB: badVote,
ValidatorPower: val.VotingPower,
TotalVotingPower: val.VotingPower,
VoteA: consuVote,
VoteB: consuBadVote,
ValidatorPower: consuVal.VotingPower,
TotalVotingPower: consuVal.VotingPower,
Timestamp: s.consumerCtx().BlockTime(),
},
"chainID",
false,
},
{
// create an invalid evidence containing two identical votes
"invalid double voting evidence - shouldn't pass",
"invalid double voting evidence with identical votes - shouldn't pass",
&tmtypes.DuplicateVoteEvidence{
VoteA: vote1,
VoteB: vote1,
ValidatorPower: val.VotingPower,
TotalVotingPower: val.VotingPower,
VoteA: consuVote,
VoteB: consuVote,
ValidatorPower: consuVal.VotingPower,
TotalVotingPower: consuVal.VotingPower,
Timestamp: s.consumerCtx().BlockTime(),
},
s.consumerChain.ChainID,
Expand All @@ -84,39 +110,63 @@ func (s *CCVTestSuite) TestHandleConsumerDoubleVoting() {
// we create two votes that only differ by their Block IDs and
// signed them using the same validator private key and chain ID
// of the consumer chain
"valid double voting evidence - should pass",
"valid double voting evidence 1 - should pass",
&tmtypes.DuplicateVoteEvidence{
VoteA: vote1,
VoteB: badVote,
ValidatorPower: val.VotingPower,
TotalVotingPower: val.VotingPower,
VoteA: consuVote,
VoteB: consuBadVote,
ValidatorPower: consuVal.VotingPower,
TotalVotingPower: consuVal.VotingPower,
Timestamp: s.consumerCtx().BlockTime(),
},
s.consumerChain.ChainID,
true,
},
{
// create a double voting evidence using the provider validator key
"valid double voting evidence 2 - should pass",
&tmtypes.DuplicateVoteEvidence{
VoteA: provVote,
VoteB: provBadVote,
ValidatorPower: consuVal.VotingPower,
TotalVotingPower: consuVal.VotingPower,
Timestamp: s.consumerCtx().BlockTime(),
},
s.consumerChain.ChainID,
true,
},
}

consuAddr := types.NewConsumerConsAddress(sdk.ConsAddress(val.Address.Bytes()))
consuAddr := types.NewConsumerConsAddress(sdk.ConsAddress(consuVal.Address.Bytes()))
provAddr := s.providerApp.GetProviderKeeper().GetProviderAddrFromConsumerAddr(s.providerCtx(), s.consumerChain.ChainID, consuAddr)

for _, tc := range testCases {
s.Run(tc.name, func() {
// reset context for each run
provCtx := s.providerCtx()

// if the evidence was built using the validator provider address and key,
// we remove the consumer key assigned to the validator otherwise
// HandleConsumerDoubleVoting uses the consumer key to verify the signature
if tc.ev.VoteA.ValidatorAddress.String() != consuVal.Address.String() {
s.providerApp.GetProviderKeeper().DeleteKeyAssignments(provCtx, s.consumerChain.ChainID)
}

err = s.providerApp.GetProviderKeeper().HandleConsumerDoubleVoting(
s.providerCtx(),
provCtx,
tc.ev,
tc.chainID,
)

if tc.expPass {
s.Require().NoError(err)

// verifies that the jailing has occurred
s.Require().True(s.providerApp.GetTestStakingKeeper().IsValidatorJailed(s.providerCtx(), provAddr.ToSdkConsAddr()))
s.Require().True(s.providerApp.GetTestStakingKeeper().IsValidatorJailed(provCtx, provAddr.ToSdkConsAddr()))
} else {
s.Require().Error(err)

// verifies that no jailing and has occurred
s.Require().False(s.providerApp.GetTestStakingKeeper().IsValidatorJailed(s.providerCtx(), provAddr.ToSdkConsAddr()))
s.Require().False(s.providerApp.GetTestStakingKeeper().IsValidatorJailed(provCtx, provAddr.ToSdkConsAddr()))
}
})
}
Expand Down
4 changes: 2 additions & 2 deletions x/ccv/provider/client/cli/tx.go
Original file line number Diff line number Diff line change
Expand Up @@ -153,7 +153,7 @@ Examples:

func NewSubmitConsumerDoubleVotingCmd() *cobra.Command {
Copy link
Contributor Author

@sainoe sainoe Sep 1, 2023

Choose a reason for hiding this comment

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

Changes from #1247 also appear here weirdly.

cmd := &cobra.Command{
Use: "submit consumer-double-voting [evidence] [infraction_header]",
Use: "submit-consumer-double-voting [evidence] [infraction_header]",
Short: "submit a double voting evidence for a consumer chain",
Args: cobra.ExactArgs(2),
RunE: func(cmd *cobra.Command, args []string) error {
Expand All @@ -176,7 +176,7 @@ func NewSubmitConsumerDoubleVotingCmd() *cobra.Command {
return err
}

msg, err := types.NewMsgSubmitConsumerDoubleVoting(submitter, ev, nil)
msg, err := types.NewMsgSubmitConsumerDoubleVoting(submitter, ev, &header)
if err != nil {
return err
}
Expand Down
59 changes: 44 additions & 15 deletions x/ccv/provider/keeper/double_vote.go
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,12 @@ import (
"fmt"

cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"

sdk "github.com/cosmos/cosmos-sdk/types"
sdkerrors "github.com/cosmos/cosmos-sdk/types/errors"
"github.com/cosmos/interchain-security/v2/x/ccv/provider/types"
ccvtypes "github.com/cosmos/interchain-security/v2/x/ccv/types"
tmprotocrypto "github.com/tendermint/tendermint/proto/tendermint/crypto"
tmtypes "github.com/tendermint/tendermint/types"
)

Expand All @@ -23,14 +24,14 @@ func (k Keeper) HandleConsumerDoubleVoting(ctx sdk.Context, evidence *tmtypes.Du
types.NewConsumerConsAddress(sdk.ConsAddress(evidence.VoteA.ValidatorAddress.Bytes())),
)

// get the consumer chain public key assigned to the validator
consuPubkey, ok := k.GetValidatorConsumerPubKey(ctx, chainID, providerAddr)
if !ok {
return fmt.Errorf("cannot find public key for validator %s and consumer chain %s", providerAddr.String(), chainID)
// get validator pubkey used on the consumer chain
pubkey, err := k.getValidatorPubkeyOnConsumer(ctx, chainID, providerAddr)
if err != nil {
return err
}

// verifies the double voting evidence using the consumer chain public key
if err := k.VerifyDoubleVotingEvidence(ctx, *evidence, chainID, consuPubkey); err != nil {
if err := k.VerifyDoubleVotingEvidence(ctx, *evidence, chainID, pubkey); err != nil {
return err
}

Expand All @@ -39,7 +40,7 @@ func (k Keeper) HandleConsumerDoubleVoting(ctx sdk.Context, evidence *tmtypes.Du

k.Logger(ctx).Info(
"confirmed equivocation",
"byzantine validator address", providerAddr,
"byzantine validator address", providerAddr.String(),
)

return nil
Expand All @@ -51,8 +52,12 @@ func (k Keeper) VerifyDoubleVotingEvidence(
ctx sdk.Context,
evidence tmtypes.DuplicateVoteEvidence,
chainID string,
pubkey tmprotocrypto.PublicKey,
pubkey cryptotypes.PubKey,
) error {
if pubkey == nil {
return fmt.Errorf("validator public key cannot be empty")
}

// Note that since we're only jailing validators for double voting on a consumer chain,
// the age of the evidence is irrelevant and therefore isn't checked.

Expand Down Expand Up @@ -89,21 +94,45 @@ func (k Keeper) VerifyDoubleVotingEvidence(
)
}

pk, err := cryptocodec.FromTmProtoPublicKey(pubkey)
if err != nil {
return err
}

va := evidence.VoteA.ToProto()
vb := evidence.VoteB.ToProto()

// signatures must be valid
if !pk.VerifySignature(tmtypes.VoteSignBytes(chainID, va), evidence.VoteA.Signature) {
if !pubkey.VerifySignature(tmtypes.VoteSignBytes(chainID, va), evidence.VoteA.Signature) {
return fmt.Errorf("verifying VoteA: %w", tmtypes.ErrVoteInvalidSignature)
}
if !pk.VerifySignature(tmtypes.VoteSignBytes(chainID, vb), evidence.VoteB.Signature) {
if !pubkey.VerifySignature(tmtypes.VoteSignBytes(chainID, vb), evidence.VoteB.Signature) {
return fmt.Errorf("verifying VoteB: %w", tmtypes.ErrVoteInvalidSignature)
}

return nil
}

// getValidatorPubkeyOnConsumer returns the public key a validator used on a given consumer chain.
// Note that it can either be an assigned public key or the same public key the validator uses
// on the provider chain.
func (k Keeper) getValidatorPubkeyOnConsumer(
ctx sdk.Context,
chainID string,
providerAddr types.ProviderConsAddress,
) (pubkey cryptotypes.PubKey, err error) {
tmPK, ok := k.GetValidatorConsumerPubKey(ctx, chainID, providerAddr)
if ok {
pubkey, err = cryptocodec.FromTmProtoPublicKey(tmPK)
if err != nil {
return
}
} else {
val, ok := k.stakingKeeper.GetValidatorByConsAddr(ctx, providerAddr.ToSdkConsAddr())
if !ok {
err = fmt.Errorf("cannot find validator %s", providerAddr.String())
return
}
pubkey, err = val.ConsPubKey()
if err != nil {
return
}
}

return
}
12 changes: 6 additions & 6 deletions x/ccv/provider/keeper/double_vote_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -4,11 +4,11 @@ import (
"testing"
"time"

cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec"
cryptotypes "github.com/cosmos/cosmos-sdk/crypto/types"
testutil "github.com/cosmos/interchain-security/v2/testutil/crypto"
testkeeper "github.com/cosmos/interchain-security/v2/testutil/keeper"
"github.com/stretchr/testify/require"
tmencoding "github.com/tendermint/tendermint/crypto/encoding"
tmprotocrypto "github.com/tendermint/tendermint/proto/tendermint/crypto"
tmtypes "github.com/tendermint/tendermint/types"
)

Expand All @@ -31,17 +31,17 @@ func TestVerifyDoubleVotingEvidence(t *testing.T) {

ctx = ctx.WithBlockTime(time.Now())

valPubkey1, err := tmencoding.PubKeyToProto(val1.PubKey)
valPubkey1, err := cryptocodec.FromTmPubKeyInterface(val1.PubKey)
require.NoError(t, err)

valPubkey2, err := tmencoding.PubKeyToProto(val2.PubKey)
valPubkey2, err := cryptocodec.FromTmPubKeyInterface(val2.PubKey)
require.NoError(t, err)

testCases := []struct {
name string
votes []*tmtypes.Vote
chainID string
pubkey tmprotocrypto.PublicKey
pubkey cryptotypes.PubKey
expPass bool
}{
{
Expand Down Expand Up @@ -209,7 +209,7 @@ func TestVerifyDoubleVotingEvidence(t *testing.T) {
),
},
chainID,
tmprotocrypto.PublicKey{},
Copy link
Contributor

@bermuell bermuell Sep 4, 2023

Choose a reason for hiding this comment

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

maybe still a valid negtive test with empty content.
might be good to have both nil and {} as PK for 'invalid PK tests'?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a valid point when using concrete type! Now the SDK public key interface is used.

nil,
false,
},
{
Expand Down
12 changes: 10 additions & 2 deletions x/ccv/provider/types/msg.go
Original file line number Diff line number Diff line change
Expand Up @@ -210,8 +210,16 @@ func (msg MsgSubmitConsumerDoubleVoting) ValidateBasic() error {
return fmt.Errorf("double voting evidence cannot be nil")
}

if msg.InfractionBlockHeader.Header == nil {
return fmt.Errorf("infraction header cannot be nil")
if msg.InfractionBlockHeader == nil {
Copy link
Contributor Author

Choose a reason for hiding this comment

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

These missing validations were causing the handler to panic during E2E tests. This issue stemmed from the Hermes relayer, which used to let the InfractionBlockHeader field empty.

return fmt.Errorf("infraction block header cannot be nil")
}

if msg.InfractionBlockHeader.SignedHeader == nil {
return fmt.Errorf("signed header in infraction block header cannot be nil")
}

if msg.InfractionBlockHeader.SignedHeader.Header == nil {
return fmt.Errorf("invalid signed header in infraction block header, 'SignedHeader.Header' is nil")
}

return nil
Expand Down
Loading