From 55ffef89aed60905361a7b1757df7788cb9682d5 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 15 Mar 2024 16:49:38 +0100 Subject: [PATCH 1/5] add check for consumer validators in downtime logic --- x/ccv/provider/keeper/relay.go | 8 ++++ x/ccv/provider/keeper/relay_test.go | 66 +++++++++++++++++++---------- 2 files changed, 51 insertions(+), 23 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index 939f6d3995..1644ce263a 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -391,6 +391,14 @@ func (k Keeper) HandleSlashPacket(ctx sdk.Context, chainID string, data ccv.Slas "infractionType", data.Infraction, ) + // Check that the validator belongs to the consumer chain valset + if !k.IsConsumerValidator(ctx, chainID, providerConsAddr) { + k.Logger(ctx).Error("cannot jail validator %s that does not belong to consumer %s valset", + providerConsAddr.String(), chainID) + // drop packet + return + } + // Obtain validator from staking keeper validator, found := k.stakingKeeper.GetValidatorByConsAddr(ctx, providerConsAddr.ToSdkConsAddr()) diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index b7e89b3dc2..644acdc448 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -1,11 +1,12 @@ package keeper_test import ( - cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" - ibctesting "github.com/cosmos/ibc-go/v7/testing" "strings" "testing" + cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" + ibctesting "github.com/cosmos/ibc-go/v7/testing" + clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" "github.com/golang/mock/gomock" @@ -22,6 +23,7 @@ import ( cryptotestutil "github.com/cosmos/interchain-security/v4/testutil/crypto" testkeeper "github.com/cosmos/interchain-security/v4/testutil/keeper" "github.com/cosmos/interchain-security/v4/x/ccv/provider/keeper" + "github.com/cosmos/interchain-security/v4/x/ccv/provider/types" providertypes "github.com/cosmos/interchain-security/v4/x/ccv/provider/types" ccv "github.com/cosmos/interchain-security/v4/x/ccv/types" ) @@ -289,6 +291,8 @@ func TestValidateSlashPacket(t *testing.T) { func TestHandleSlashPacket(t *testing.T) { chainId := "consumer-id" validVscID := uint64(234) + + dumConsAddr := cryptotestutil.NewCryptoIdentityFromIntSeed(784987639).ConsumerConsAddress() providerConsAddr := cryptotestutil.NewCryptoIdentityFromIntSeed(7842334).ProviderConsAddress() consumerConsAddr := cryptotestutil.NewCryptoIdentityFromIntSeed(784987634).ConsumerConsAddress() @@ -299,6 +303,20 @@ func TestHandleSlashPacket(t *testing.T) { expectedCalls func(sdk.Context, testkeeper.MockedKeepers, ccv.SlashPacketData) []*gomock.Call expectedSlashAcksLen int }{ + { + "validator isn't a consumer validator", + ccv.SlashPacketData{ + Validator: abci.Validator{Address: dumConsAddr.ToSdkConsAddr()}, + ValsetUpdateId: validVscID, + Infraction: stakingtypes.Infraction_INFRACTION_DOWNTIME, + }, + func(ctx sdk.Context, mocks testkeeper.MockedKeepers, + expectedPacketData ccv.SlashPacketData, + ) []*gomock.Call { + return []*gomock.Call{} + }, + 0, + }, { "unfound validator", ccv.SlashPacketData{ @@ -403,34 +421,36 @@ func TestHandleSlashPacket(t *testing.T) { } for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx( + t, testkeeper.NewInMemKeeperParams(t)) - providerKeeper, ctx, ctrl, mocks := testkeeper.GetProviderKeeperAndCtx( - t, testkeeper.NewInMemKeeperParams(t)) + // Setup expected mock calls + gomock.InOrder(tc.expectedCalls(ctx, mocks, tc.packetData)...) - // Setup expected mock calls - gomock.InOrder(tc.expectedCalls(ctx, mocks, tc.packetData)...) + // Setup init chain height and a single valid valset update ID to block height mapping. + providerKeeper.SetInitChainHeight(ctx, chainId, 5) + providerKeeper.SetValsetUpdateBlockHeight(ctx, validVscID, 99) - // Setup init chain height and a single valid valset update ID to block height mapping. - providerKeeper.SetInitChainHeight(ctx, chainId, 5) - providerKeeper.SetValsetUpdateBlockHeight(ctx, validVscID, 99) + // Setup consumer address to provider address mapping. + require.NotEmpty(t, tc.packetData.Validator.Address) + providerKeeper.SetValidatorByConsumerAddr(ctx, chainId, consumerConsAddr, providerConsAddr) + providerKeeper.SetConsumerValidator(ctx, chainId, types.ConsumerValidator{ProviderConsAddr: providerConsAddr.Address.Bytes()}) - // Setup consumer address to provider address mapping. - require.NotEmpty(t, tc.packetData.Validator.Address) - providerKeeper.SetValidatorByConsumerAddr(ctx, chainId, consumerConsAddr, providerConsAddr) + // Execute method and assert expected mock calls. + providerKeeper.HandleSlashPacket(ctx, chainId, tc.packetData) - // Execute method and assert expected mock calls. - providerKeeper.HandleSlashPacket(ctx, chainId, tc.packetData) + require.Equal(t, tc.expectedSlashAcksLen, len(providerKeeper.GetSlashAcks(ctx, chainId))) - require.Equal(t, tc.expectedSlashAcksLen, len(providerKeeper.GetSlashAcks(ctx, chainId))) - - if tc.expectedSlashAcksLen == 1 { - // must match the consumer address - require.Equal(t, consumerConsAddr.String(), providerKeeper.GetSlashAcks(ctx, chainId)[0]) - require.NotEqual(t, providerConsAddr.String(), providerKeeper.GetSlashAcks(ctx, chainId)[0]) - require.NotEqual(t, providerConsAddr.String(), consumerConsAddr.String()) - } + if tc.expectedSlashAcksLen == 1 { + // must match the consumer address + require.Equal(t, consumerConsAddr.String(), providerKeeper.GetSlashAcks(ctx, chainId)[0]) + require.NotEqual(t, providerConsAddr.String(), providerKeeper.GetSlashAcks(ctx, chainId)[0]) + require.NotEqual(t, providerConsAddr.String(), consumerConsAddr.String()) + } - ctrl.Finish() + ctrl.Finish() + }) } } From 726f8c1c234bb3dba1c4aec8e238977f2a0fca16 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 19 Mar 2024 08:55:10 +0100 Subject: [PATCH 2/5] fix UT --- x/ccv/provider/keeper/relay_test.go | 3 +++ 1 file changed, 3 insertions(+) diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index 644acdc448..af7475de75 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -138,6 +138,9 @@ func TestOnRecvDowntimeSlashPacket(t *testing.T) { // Now set slash meter to positive value and assert slash packet handled result is returned providerKeeper.SetSlashMeter(ctx, math.NewInt(5)) + // Set the consumer validator + providerKeeper.SetConsumerValidator(ctx, "chain-1", types.ConsumerValidator{ProviderConsAddr: packetData.Validator.Address}) + // Mock call to GetEffectiveValPower, so that it returns 2. providerAddr := providertypes.NewProviderConsAddress(packetData.Validator.Address) calls := []*gomock.Call{ From 002a435431f7be97fc5e9cc61242e7e13fa3d5f9 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 19 Mar 2024 09:03:50 +0100 Subject: [PATCH 3/5] try to fix weird errors in gh worfklow --- x/ccv/provider/keeper/keeper_test.go | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/x/ccv/provider/keeper/keeper_test.go b/x/ccv/provider/keeper/keeper_test.go index 595e01100a..a277437b35 100644 --- a/x/ccv/provider/keeper/keeper_test.go +++ b/x/ccv/provider/keeper/keeper_test.go @@ -10,11 +10,10 @@ import ( ibctesting "github.com/cosmos/ibc-go/v7/testing" "github.com/stretchr/testify/require" - cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" - abci "github.com/cometbft/cometbft/abci/types" tmprotocrypto "github.com/cometbft/cometbft/proto/tendermint/crypto" + cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" sdk "github.com/cosmos/cosmos-sdk/types" cryptotestutil "github.com/cosmos/interchain-security/v4/testutil/crypto" testkeeper "github.com/cosmos/interchain-security/v4/testutil/keeper" From 70872f696d147c24154e6d1c3a1dec77a8a8bada Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Tue, 19 Mar 2024 09:17:16 +0100 Subject: [PATCH 4/5] fix silly merge bug --- x/ccv/provider/keeper/relay_test.go | 3 --- 1 file changed, 3 deletions(-) diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index e7e30903a8..16860f2914 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -4,9 +4,6 @@ import ( "strings" "testing" - cryptocodec "github.com/cosmos/cosmos-sdk/crypto/codec" - ibctesting "github.com/cosmos/ibc-go/v7/testing" - clienttypes "github.com/cosmos/ibc-go/v7/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v7/modules/core/04-channel/types" ibctesting "github.com/cosmos/ibc-go/v7/testing" From f08dc3e2ff0097afb0b77947ed3cb73884bdd08b Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Wed, 20 Mar 2024 08:50:43 +0100 Subject: [PATCH 5/5] nits --- x/ccv/provider/keeper/relay_test.go | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index 16860f2914..bb5a5227c6 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -294,9 +294,10 @@ func TestHandleSlashPacket(t *testing.T) { chainId := "consumer-id" validVscID := uint64(234) - dumConsAddr := cryptotestutil.NewCryptoIdentityFromIntSeed(784987639).ConsumerConsAddress() providerConsAddr := cryptotestutil.NewCryptoIdentityFromIntSeed(7842334).ProviderConsAddress() consumerConsAddr := cryptotestutil.NewCryptoIdentityFromIntSeed(784987634).ConsumerConsAddress() + // this "dummy" consensus address won't be stored on the provider states + dummyConsAddr := cryptotestutil.NewCryptoIdentityFromIntSeed(784987639).ConsumerConsAddress() testCases := []struct { name string @@ -308,7 +309,7 @@ func TestHandleSlashPacket(t *testing.T) { { "validator isn't a consumer validator", ccv.SlashPacketData{ - Validator: abci.Validator{Address: dumConsAddr.ToSdkConsAddr()}, + Validator: abci.Validator{Address: dummyConsAddr.ToSdkConsAddr()}, ValsetUpdateId: validVscID, Infraction: stakingtypes.Infraction_INFRACTION_DOWNTIME, },