From 1c348aff1f2645894bd7328edb99368c78092e87 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Fri, 16 Dec 2022 16:22:52 +0100 Subject: [PATCH] fix: verify consumer proposals execution to prevent provider halts (#602) * feat: ensure consumer proposals execution can't panic during BeginBlock - Verify proposals execution using a cached context in BeginBlock and consumer proposal handlers - Drop invalid consumer props in BeginBlock instead of panicing * fix integration test submit prop to have enough gas * fix linter --- tests/e2e/unbonding.go | 10 +- tests/integration/actions.go | 1 + testutil/keeper/expectations.go | 15 +- x/ccv/provider/keeper/proposal.go | 72 ++++++-- x/ccv/provider/keeper/proposal_test.go | 239 +++++++++++++++---------- x/ccv/types/errors.go | 2 + 6 files changed, 213 insertions(+), 126 deletions(-) diff --git a/tests/e2e/unbonding.go b/tests/e2e/unbonding.go index 03c23bc580..b5b02d896e 100644 --- a/tests/e2e/unbonding.go +++ b/tests/e2e/unbonding.go @@ -304,9 +304,10 @@ func (s *CCVTestSuite) TestUnbondingNoConsumer() { providerKeeper := s.providerApp.GetProviderKeeper() providerStakingKeeper := s.providerApp.GetE2eStakingKeeper() - // remove all consumer chains, which were already registered during setup + // remove all consumer chains, which were already started during setup for chainID := range s.consumerBundles { - providerKeeper.DeleteConsumerClientId(s.providerCtx(), chainID) + err := providerKeeper.StopConsumerChain(s.providerCtx(), chainID, true) + s.Require().NoError(err) } // delegate bondAmt and undelegate 1/2 of it @@ -341,8 +342,9 @@ func (s *CCVTestSuite) TestRedelegationNoConsumer() { providerKeeper := s.providerApp.GetProviderKeeper() stakingKeeper := s.providerApp.GetE2eStakingKeeper() - // remove the consumer chain, which was already registered during setup - providerKeeper.DeleteConsumerClientId(s.providerCtx(), s.consumerChain.ChainID) + // stop the consumer chain, which was already started during setup + err := providerKeeper.StopConsumerChain(s.providerCtx(), s.consumerChain.ChainID, true) + s.Require().NoError(err) // Setup delegator, bond amount, and src/dst validators bondAmt := sdk.NewInt(10000000) diff --git a/tests/integration/actions.go b/tests/integration/actions.go index 8c27ac4493..2d7adbfbf7 100644 --- a/tests/integration/actions.go +++ b/tests/integration/actions.go @@ -255,6 +255,7 @@ func (tr TestRun) submitConsumerAdditionProposal( `--from`, `validator`+fmt.Sprint(action.from), `--chain-id`, string(tr.chainConfigs[action.chain].chainId), `--home`, tr.getValidatorHome(action.chain, action.from), + `--gas`, `900000`, `--node`, tr.getValidatorNode(action.chain, action.from), `--keyring-backend`, `test`, `-b`, `block`, diff --git a/testutil/keeper/expectations.go b/testutil/keeper/expectations.go index a7d7bb163f..02687785c7 100644 --- a/testutil/keeper/expectations.go +++ b/testutil/keeper/expectations.go @@ -31,7 +31,7 @@ func GetMocksForCreateConsumerClient(ctx sdk.Context, mocks *MockedKeepers, // append MakeConsumerGenesis and CreateClient expectations expectations := GetMocksForMakeConsumerGenesis(ctx, mocks, time.Hour) createClientExp := mocks.MockClientKeeper.EXPECT().CreateClient( - ctx, + gomock.Any(), // Allows us to expect a match by field. These are the only two client state values // that are dependant on parameters passed to CreateConsumerClient. extra.StructMatcher().Field( @@ -48,13 +48,14 @@ func GetMocksForCreateConsumerClient(ctx sdk.Context, mocks *MockedKeepers, // GetMocksForMakeConsumerGenesis returns mock expectations needed to call MakeConsumerGenesis(). func GetMocksForMakeConsumerGenesis(ctx sdk.Context, mocks *MockedKeepers, unbondingTimeToInject time.Duration) []*gomock.Call { + return []*gomock.Call{ - mocks.MockStakingKeeper.EXPECT().UnbondingTime(ctx).Return(unbondingTimeToInject).Times(1), + mocks.MockStakingKeeper.EXPECT().UnbondingTime(gomock.Any()).Return(unbondingTimeToInject).Times(1), - mocks.MockClientKeeper.EXPECT().GetSelfConsensusState(ctx, + mocks.MockClientKeeper.EXPECT().GetSelfConsensusState(gomock.Any(), clienttypes.GetSelfHeight(ctx)).Return(&ibctmtypes.ConsensusState{}, nil).Times(1), - mocks.MockStakingKeeper.EXPECT().IterateLastValidatorPowers(ctx, gomock.Any()).Times(1), + mocks.MockStakingKeeper.EXPECT().IterateLastValidatorPowers(gomock.Any(), gomock.Any()).Times(1), } } @@ -82,11 +83,11 @@ func GetMocksForSetConsumerChain(ctx sdk.Context, mocks *MockedKeepers, func GetMocksForStopConsumerChain(ctx sdk.Context, mocks *MockedKeepers) []*gomock.Call { dummyCap := &capabilitytypes.Capability{} return []*gomock.Call{ - mocks.MockChannelKeeper.EXPECT().GetChannel(ctx, ccv.ProviderPortID, "channelID").Return( + mocks.MockChannelKeeper.EXPECT().GetChannel(gomock.Any(), ccv.ProviderPortID, "channelID").Return( channeltypes.Channel{State: channeltypes.OPEN}, true, ).Times(1), - mocks.MockScopedKeeper.EXPECT().GetCapability(ctx, gomock.Any()).Return(dummyCap, true).Times(1), - mocks.MockChannelKeeper.EXPECT().ChanCloseInit(ctx, ccv.ProviderPortID, "channelID", dummyCap).Times(1), + mocks.MockScopedKeeper.EXPECT().GetCapability(gomock.Any(), gomock.Any()).Return(dummyCap, true).Times(1), + mocks.MockChannelKeeper.EXPECT().ChanCloseInit(gomock.Any(), ccv.ProviderPortID, "channelID", dummyCap).Times(1), } } diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index 1b4b3094ca..b19a285179 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -22,15 +22,17 @@ import ( ) // HandleConsumerAdditionProposal will receive the consumer chain's client state from the proposal. -// If the spawn time has already passed, then set the consumer chain. Otherwise store the client -// as a pending client, and set once spawn time has passed. +// If the client can be successfully created in a cached context, it stores the proposal as a pending proposal. // // Note: This method implements SpawnConsumerChainProposalHandler in spec. // See: https://github.com/cosmos/ibc/blob/main/spec/app/ics-028-cross-chain-validation/methods.md#ccv-pcf-hcaprop1 // Spec tag: [CCV-PCF-HCAPROP.1] func (k Keeper) HandleConsumerAdditionProposal(ctx sdk.Context, p *types.ConsumerAdditionProposal) error { - if !ctx.BlockTime().Before(p.SpawnTime) { - return k.CreateConsumerClient(ctx, p) + + // verify the consumer addition proposal execution + // in cached context and discard the cached writes + if _, _, err := k.CreateConsumerClientInCachedCtx(ctx, *p); err != nil { + return err } err := k.SetPendingConsumerAdditionProp(ctx, p) @@ -50,9 +52,9 @@ func (k Keeper) CreateConsumerClient(ctx sdk.Context, prop *types.ConsumerAdditi chainID := prop.ChainId // check that a client for this chain does not exist - if _, found := k.GetConsumerClientId(ctx, prop.ChainId); found { - // drop the proposal - return nil + if _, found := k.GetConsumerClientId(ctx, chainID); found { + return sdkerrors.Wrap(ccv.ErrDuplicateConsumerChain, + fmt.Sprintf("cannot create client for existent consumer chain: %s", chainID)) } // Consumers always start out with the default unbonding period @@ -60,7 +62,7 @@ func (k Keeper) CreateConsumerClient(ctx sdk.Context, prop *types.ConsumerAdditi // Create client state by getting template client from parameters and filling in zeroed fields from proposal. clientState := k.GetTemplateClient(ctx) - clientState.ChainId = prop.ChainId + clientState.ChainId = chainID clientState.LatestHeight = prop.InitialHeight clientState.TrustingPeriod = consumerUnbondingPeriod / time.Duration(k.GetTrustingPeriodFraction(ctx)) clientState.UnbondingPeriod = consumerUnbondingPeriod @@ -108,15 +110,16 @@ func (k Keeper) CreateConsumerClient(ctx sdk.Context, prop *types.ConsumerAdditi } // HandleConsumerRemovalProposal stops a consumer chain and released the outstanding unbonding operations. -// If the stop time hasn't already passed, it stores the proposal as a pending proposal. +// If the consumer can be successfully stopped in a cached context, it stores the proposal as a pending proposal. // // This method implements StopConsumerChainProposalHandler from spec. // See: https://github.com/cosmos/ibc/blob/main/spec/app/ics-028-cross-chain-validation/methods.md#ccv-pcf-hcrprop1 // Spec tag: [CCV-PCF-HCRPROP.1] func (k Keeper) HandleConsumerRemovalProposal(ctx sdk.Context, p *types.ConsumerRemovalProposal) error { - - if !ctx.BlockTime().Before(p.StopTime) { - return k.StopConsumerChain(ctx, p.ChainId, true) + // verify the consumer removal proposal execution + // in cached context and discard the cached writes + if _, _, err := k.StopConsumerChainInCachedCtx(ctx, *p); err != nil { + return err } k.SetPendingConsumerRemovalProp(ctx, p.ChainId, p.StopTime) @@ -132,8 +135,8 @@ func (k Keeper) HandleConsumerRemovalProposal(ctx sdk.Context, p *types.Consumer func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, closeChan bool) (err error) { // check that a client for chainID exists if _, found := k.GetConsumerClientId(ctx, chainID); !found { - // drop the proposal - return nil + return sdkerrors.Wrap(ccv.ErrConsumerChainNotFound, + fmt.Sprintf("cannot stop non-existent consumer chain: %s", chainID)) } // clean up states @@ -332,11 +335,18 @@ func (k Keeper) BeginBlockInit(ctx sdk.Context) { propsToExecute := k.ConsumerAdditionPropsToExecute(ctx) for _, prop := range propsToExecute { - p := prop - err := k.CreateConsumerClient(ctx, &p) + // create consumer client in a cached context to handle errors + cachedCtx, writeFn, err := k.CreateConsumerClientInCachedCtx(ctx, prop) if err != nil { - panic(fmt.Errorf("consumer client could not be created: %w", err)) + // drop the proposal + ctx.Logger().Info("consumer client could not be created: %w", err) + continue } + // The cached context is created with a new EventManager so we merge the event + // into the original context + ctx.EventManager().EmitEvents(cachedCtx.EventManager().Events()) + // write cache + writeFn() } // delete the executed proposals k.DeletePendingConsumerAdditionProps(ctx, propsToExecute...) @@ -457,10 +467,18 @@ func (k Keeper) BeginBlockCCR(ctx sdk.Context) { propsToExecute := k.ConsumerRemovalPropsToExecute(ctx) for _, prop := range propsToExecute { - err := k.StopConsumerChain(ctx, prop.ChainId, true) + // stop consumer chain in a cached context to handle errors + cachedCtx, writeFn, err := k.StopConsumerChainInCachedCtx(ctx, prop) if err != nil { - panic(fmt.Errorf("consumer chain failed to stop: %w", err)) + // drop the proposal + ctx.Logger().Info("consumer chain could not be stopped: %w", err) + continue } + // The cached context is created with a new EventManager so we merge the event + // into the original context + ctx.EventManager().EmitEvents(cachedCtx.EventManager().Events()) + // write cache + writeFn() } // delete the executed proposals k.DeletePendingConsumerRemovalProps(ctx, propsToExecute...) @@ -544,3 +562,19 @@ func (k Keeper) CloseChannel(ctx sdk.Context, channelID string) { } } } + +// CreateConsumerClientInCachedCtx creates a consumer client +// from a given consumer addition proposal in a cached context +func (k Keeper) CreateConsumerClientInCachedCtx(ctx sdk.Context, p types.ConsumerAdditionProposal) (cc sdk.Context, writeCache func(), err error) { + cc, writeCache = ctx.CacheContext() + err = k.CreateConsumerClient(cc, &p) + return +} + +// StopConsumerChainInCachedCtx stop a consumer chain +// from a given consumer removal proposal in a cached context +func (k Keeper) StopConsumerChainInCachedCtx(ctx sdk.Context, p types.ConsumerRemovalProposal) (cc sdk.Context, writeCache func(), err error) { + cc, writeCache = ctx.CacheContext() + err = k.StopConsumerChain(ctx, p.ChainId, true) + return +} diff --git a/x/ccv/provider/keeper/proposal_test.go b/x/ccv/provider/keeper/proposal_test.go index 5d0d5e101f..0e1a2721d3 100644 --- a/x/ccv/provider/keeper/proposal_test.go +++ b/x/ccv/provider/keeper/proposal_test.go @@ -18,7 +18,6 @@ import ( testkeeper "github.com/cosmos/interchain-security/testutil/keeper" consumertypes "github.com/cosmos/interchain-security/x/ccv/consumer/types" providerkeeper "github.com/cosmos/interchain-security/x/ccv/provider/keeper" - "github.com/cosmos/interchain-security/x/ccv/provider/types" providertypes "github.com/cosmos/interchain-security/x/ccv/provider/types" ccvtypes "github.com/cosmos/interchain-security/x/ccv/types" ) @@ -34,20 +33,22 @@ func TestHandleConsumerAdditionProposal(t *testing.T) { type testCase struct { description string + malleate func(ctx sdk.Context, k providerkeeper.Keeper, chainID string) prop *providertypes.ConsumerAdditionProposal // Time when prop is handled blockTime time.Time - // Whether it's expected that the spawn time has passed and client should be created - expCreatedClient bool + // Whether it's expected that the proposal is successfully verified + // and appended to the pending proposals + expAppendProp bool } // Snapshot times asserted in tests now := time.Now().UTC() - hourFromNow := now.Add(time.Hour).UTC() tests := []testCase{ { - description: "ctx block time is after proposal's spawn time, expected that client is created", + description: "expect to append valid proposal", + malleate: func(ctx sdk.Context, k providerkeeper.Keeper, chainID string) {}, prop: providertypes.NewConsumerAdditionProposal( "title", "description", @@ -63,12 +64,15 @@ func TestHandleConsumerAdditionProposal(t *testing.T) { 100000000000, 100000000000, ).(*providertypes.ConsumerAdditionProposal), - blockTime: hourFromNow, - expCreatedClient: true, + blockTime: now, + expAppendProp: true, }, { - description: `ctx block time is before proposal's spawn time, - expected that no client is created and the proposal is persisted as pending`, + description: "expect to not append invalid proposal using an already existing chain id", + malleate: func(ctx sdk.Context, k providerkeeper.Keeper, chainID string) { + k.SetConsumerClientId(ctx, chainID, "anyClientId") + }, + prop: providertypes.NewConsumerAdditionProposal( "title", "description", @@ -76,16 +80,16 @@ func TestHandleConsumerAdditionProposal(t *testing.T) { clienttypes.NewHeight(2, 3), []byte("gen_hash"), []byte("bin_hash"), - hourFromNow, // Spawn time + now, "0.75", 10, 10000, 100000000000, 100000000000, 100000000000, - ).(*types.ConsumerAdditionProposal), - blockTime: now, - expCreatedClient: false, + ).(*providertypes.ConsumerAdditionProposal), + blockTime: now, + expAppendProp: false, }, } @@ -96,27 +100,30 @@ func TestHandleConsumerAdditionProposal(t *testing.T) { providerKeeper.SetParams(ctx, providertypes.DefaultParams()) ctx = ctx.WithBlockTime(tc.blockTime) - if tc.expCreatedClient { + if tc.expAppendProp { // Mock calls are only asserted if we expect a client to be created. gomock.InOrder( - testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chainID", clienttypes.NewHeight(2, 3))..., + testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, tc.prop.ChainId, clienttypes.NewHeight(2, 3))..., ) } + tc.malleate(ctx, providerKeeper, tc.prop.ChainId) + err := providerKeeper.HandleConsumerAdditionProposal(ctx, tc.prop) - require.NoError(t, err) - if tc.expCreatedClient { - testCreatedConsumerClient(t, ctx, providerKeeper, tc.prop.ChainId, "clientID") - } else { - // check that stored pending prop is exactly the same as the initially instantiated prop + if tc.expAppendProp { + require.NoError(t, err) + // check that prop was added to the stored pending props gotProposal, found := providerKeeper.GetPendingConsumerAdditionProp(ctx, tc.prop.SpawnTime, tc.prop.ChainId) require.True(t, found) require.Equal(t, *tc.prop, gotProposal) - // double check that a client for this chain does not exist - _, found = providerKeeper.GetConsumerClientId(ctx, tc.prop.ChainId) + } else { + require.Error(t, err) + // check that prop wasn't added to the stored pending props + _, found := providerKeeper.GetPendingConsumerAdditionProp(ctx, tc.prop.SpawnTime, tc.prop.ChainId) require.False(t, found) } + ctrl.Finish() } } @@ -173,13 +180,13 @@ func TestCreateConsumerClient(t *testing.T) { tc.setup(&providerKeeper, ctx, &mocks) // Call method with same arbitrary values as defined above in mock expectations. - err := providerKeeper.CreateConsumerClient( - ctx, testkeeper.GetTestConsumerAdditionProp()) - - require.NoError(t, err) + err := providerKeeper.CreateConsumerClient(ctx, testkeeper.GetTestConsumerAdditionProp()) if tc.expClientCreated { + require.NoError(t, err) testCreatedConsumerClient(t, ctx, providerKeeper, "chainID", "clientID") + } else { + require.Error(t, err) } // Assert mock calls from setup functions @@ -209,15 +216,15 @@ func testCreatedConsumerClient(t *testing.T, func TestPendingConsumerAdditionPropDeletion(t *testing.T) { testCases := []struct { - types.ConsumerAdditionProposal + providertypes.ConsumerAdditionProposal ExpDeleted bool }{ { - ConsumerAdditionProposal: types.ConsumerAdditionProposal{ChainId: "0", SpawnTime: time.Now().UTC()}, + ConsumerAdditionProposal: providertypes.ConsumerAdditionProposal{ChainId: "0", SpawnTime: time.Now().UTC()}, ExpDeleted: true, }, { - ConsumerAdditionProposal: types.ConsumerAdditionProposal{ChainId: "1", SpawnTime: time.Now().UTC().Add(time.Hour)}, + ConsumerAdditionProposal: providertypes.ConsumerAdditionProposal{ChainId: "1", SpawnTime: time.Now().UTC().Add(time.Hour)}, ExpDeleted: false, }, } @@ -255,41 +262,41 @@ func TestPendingConsumerAdditionPropOrder(t *testing.T) { now := time.Now().UTC() // props with unique chain ids and spawn times - sampleProp1 := types.ConsumerAdditionProposal{ChainId: "1", SpawnTime: now} - sampleProp2 := types.ConsumerAdditionProposal{ChainId: "2", SpawnTime: now.Add(1 * time.Hour)} - sampleProp3 := types.ConsumerAdditionProposal{ChainId: "3", SpawnTime: now.Add(2 * time.Hour)} - sampleProp4 := types.ConsumerAdditionProposal{ChainId: "4", SpawnTime: now.Add(3 * time.Hour)} - sampleProp5 := types.ConsumerAdditionProposal{ChainId: "5", SpawnTime: now.Add(4 * time.Hour)} + sampleProp1 := providertypes.ConsumerAdditionProposal{ChainId: "1", SpawnTime: now} + sampleProp2 := providertypes.ConsumerAdditionProposal{ChainId: "2", SpawnTime: now.Add(1 * time.Hour)} + sampleProp3 := providertypes.ConsumerAdditionProposal{ChainId: "3", SpawnTime: now.Add(2 * time.Hour)} + sampleProp4 := providertypes.ConsumerAdditionProposal{ChainId: "4", SpawnTime: now.Add(3 * time.Hour)} + sampleProp5 := providertypes.ConsumerAdditionProposal{ChainId: "5", SpawnTime: now.Add(4 * time.Hour)} testCases := []struct { - propSubmitOrder []types.ConsumerAdditionProposal + propSubmitOrder []providertypes.ConsumerAdditionProposal accessTime time.Time - expectedOrderedProps []types.ConsumerAdditionProposal + expectedOrderedProps []providertypes.ConsumerAdditionProposal }{ { - propSubmitOrder: []types.ConsumerAdditionProposal{ + propSubmitOrder: []providertypes.ConsumerAdditionProposal{ sampleProp1, sampleProp2, sampleProp3, sampleProp4, sampleProp5, }, accessTime: now.Add(30 * time.Minute), - expectedOrderedProps: []types.ConsumerAdditionProposal{ + expectedOrderedProps: []providertypes.ConsumerAdditionProposal{ sampleProp1, }, }, { - propSubmitOrder: []types.ConsumerAdditionProposal{ + propSubmitOrder: []providertypes.ConsumerAdditionProposal{ sampleProp3, sampleProp2, sampleProp1, sampleProp5, sampleProp4, }, accessTime: now.Add(3 * time.Hour).Add(30 * time.Minute), - expectedOrderedProps: []types.ConsumerAdditionProposal{ + expectedOrderedProps: []providertypes.ConsumerAdditionProposal{ sampleProp1, sampleProp2, sampleProp3, sampleProp4, }, }, { - propSubmitOrder: []types.ConsumerAdditionProposal{ + propSubmitOrder: []providertypes.ConsumerAdditionProposal{ sampleProp5, sampleProp4, sampleProp3, sampleProp2, sampleProp1, }, accessTime: now.Add(5 * time.Hour), - expectedOrderedProps: []types.ConsumerAdditionProposal{ + expectedOrderedProps: []providertypes.ConsumerAdditionProposal{ sampleProp1, sampleProp2, sampleProp3, sampleProp4, sampleProp5, }, }, @@ -322,12 +329,15 @@ func TestHandleConsumerRemovalProposal(t *testing.T) { type testCase struct { description string + malleate func(ctx sdk.Context, k providerkeeper.Keeper, chainID string) + // Consumer removal proposal to handle - prop *types.ConsumerRemovalProposal + prop *providertypes.ConsumerRemovalProposal // Time when prop is handled blockTime time.Time - // Whether consumer chain should have been stopped - expStop bool + // Whether it's expected that the proposal is successfully verified + // and appended to the pending proposals + expAppendProp bool } // Snapshot times asserted in tests @@ -336,26 +346,30 @@ func TestHandleConsumerRemovalProposal(t *testing.T) { tests := []testCase{ { - description: "valid proposal: stop time reached", + description: "valid proposal", + malleate: func(ctx sdk.Context, k providerkeeper.Keeper, chainID string) { + k.SetConsumerClientId(ctx, chainID, "ClientID") + }, prop: providertypes.NewConsumerRemovalProposal( "title", "description", "chainID", now, ).(*providertypes.ConsumerRemovalProposal), - blockTime: hourFromNow, // After stop time. - expStop: true, + blockTime: hourFromNow, // After stop time. + expAppendProp: true, }, { - description: "valid proposal: stop time has not yet been reached", + description: "invalid valid proposal: consumer chain does not exist", + malleate: func(ctx sdk.Context, k providerkeeper.Keeper, chainID string) {}, prop: providertypes.NewConsumerRemovalProposal( "title", "description", - "chainID", + "chainID-2", hourFromNow, ).(*providertypes.ConsumerRemovalProposal), - blockTime: now, // Before proposal's stop time - expStop: false, + blockTime: hourFromNow, // After stop time. + expAppendProp: false, }, } @@ -368,25 +382,29 @@ func TestHandleConsumerRemovalProposal(t *testing.T) { ctx = ctx.WithBlockTime(tc.blockTime) // Mock expectations and setup for stopping the consumer chain, if applicable - if tc.expStop { + if tc.expAppendProp { testkeeper.SetupForStoppingConsumerChain(t, ctx, &providerKeeper, mocks) } - // Note: when expStop is false, no mocks are setup, + // Note: when expAppendProp is false, no mocks are setup, // meaning no external keeper methods are allowed to be called. + tc.malleate(ctx, providerKeeper, tc.prop.ChainId) + err := providerKeeper.HandleConsumerRemovalProposal(ctx, tc.prop) - require.NoError(t, err) - if tc.expStop { - // Expect no pending proposal to exist - found := providerKeeper.GetPendingConsumerRemovalProp(ctx, tc.prop.ChainId, tc.prop.StopTime) - require.False(t, found) + if tc.expAppendProp { + require.NoError(t, err) - testProviderStateIsCleaned(t, ctx, providerKeeper, tc.prop.ChainId, "channelID") - } else { // Proposal should be stored as pending found := providerKeeper.GetPendingConsumerRemovalProp(ctx, tc.prop.ChainId, tc.prop.StopTime) require.True(t, found) + + } else { + require.Error(t, err) + + // Expect no pending proposal to exist + found := providerKeeper.GetPendingConsumerRemovalProp(ctx, tc.prop.ChainId, tc.prop.StopTime) + require.False(t, found) } // Assert mock calls from setup function @@ -424,7 +442,7 @@ func TestStopConsumerChain(t *testing.T) { setup: func(ctx sdk.Context, providerKeeper *providerkeeper.Keeper, mocks testkeeper.MockedKeepers) { // No mocks, meaning no external keeper methods are allowed to be called. }, - expErr: false, + expErr: true, }, { description: "valid stop of consumer chain, all mock calls hit", @@ -502,7 +520,7 @@ func testProviderStateIsCleaned(t *testing.T, ctx sdk.Context, providerKeeper pr }) require.False(t, found) found = false - providerKeeper.IterateConsumerAddrsToPrune(ctx, expectedChainID, func(_ uint64, _ types.AddressList) (stop bool) { + providerKeeper.IterateConsumerAddrsToPrune(ctx, expectedChainID, func(_ uint64, _ providertypes.AddressList) (stop bool) { found = true return true // stop the iteration }) @@ -514,15 +532,15 @@ func testProviderStateIsCleaned(t *testing.T, ctx sdk.Context, providerKeeper pr func TestPendingConsumerRemovalPropDeletion(t *testing.T) { testCases := []struct { - types.ConsumerRemovalProposal + providertypes.ConsumerRemovalProposal ExpDeleted bool }{ { - ConsumerRemovalProposal: types.ConsumerRemovalProposal{ChainId: "8", StopTime: time.Now().UTC()}, + ConsumerRemovalProposal: providertypes.ConsumerRemovalProposal{ChainId: "8", StopTime: time.Now().UTC()}, ExpDeleted: true, }, { - ConsumerRemovalProposal: types.ConsumerRemovalProposal{ChainId: "9", StopTime: time.Now().UTC().Add(time.Hour)}, + ConsumerRemovalProposal: providertypes.ConsumerRemovalProposal{ChainId: "9", StopTime: time.Now().UTC().Add(time.Hour)}, ExpDeleted: false, }, } @@ -557,41 +575,41 @@ func TestPendingConsumerRemovalPropOrder(t *testing.T) { now := time.Now().UTC() // props with unique chain ids and spawn times - sampleProp1 := types.ConsumerRemovalProposal{ChainId: "1", StopTime: now} - sampleProp2 := types.ConsumerRemovalProposal{ChainId: "2", StopTime: now.Add(1 * time.Hour)} - sampleProp3 := types.ConsumerRemovalProposal{ChainId: "3", StopTime: now.Add(2 * time.Hour)} - sampleProp4 := types.ConsumerRemovalProposal{ChainId: "4", StopTime: now.Add(3 * time.Hour)} - sampleProp5 := types.ConsumerRemovalProposal{ChainId: "5", StopTime: now.Add(4 * time.Hour)} + sampleProp1 := providertypes.ConsumerRemovalProposal{ChainId: "1", StopTime: now} + sampleProp2 := providertypes.ConsumerRemovalProposal{ChainId: "2", StopTime: now.Add(1 * time.Hour)} + sampleProp3 := providertypes.ConsumerRemovalProposal{ChainId: "3", StopTime: now.Add(2 * time.Hour)} + sampleProp4 := providertypes.ConsumerRemovalProposal{ChainId: "4", StopTime: now.Add(3 * time.Hour)} + sampleProp5 := providertypes.ConsumerRemovalProposal{ChainId: "5", StopTime: now.Add(4 * time.Hour)} testCases := []struct { - propSubmitOrder []types.ConsumerRemovalProposal + propSubmitOrder []providertypes.ConsumerRemovalProposal accessTime time.Time - expectedOrderedProps []types.ConsumerRemovalProposal + expectedOrderedProps []providertypes.ConsumerRemovalProposal }{ { - propSubmitOrder: []types.ConsumerRemovalProposal{ + propSubmitOrder: []providertypes.ConsumerRemovalProposal{ sampleProp1, sampleProp2, sampleProp3, sampleProp4, sampleProp5, }, accessTime: now.Add(30 * time.Minute), - expectedOrderedProps: []types.ConsumerRemovalProposal{ + expectedOrderedProps: []providertypes.ConsumerRemovalProposal{ sampleProp1, }, }, { - propSubmitOrder: []types.ConsumerRemovalProposal{ + propSubmitOrder: []providertypes.ConsumerRemovalProposal{ sampleProp3, sampleProp2, sampleProp1, sampleProp5, sampleProp4, }, accessTime: now.Add(3 * time.Hour).Add(30 * time.Minute), - expectedOrderedProps: []types.ConsumerRemovalProposal{ + expectedOrderedProps: []providertypes.ConsumerRemovalProposal{ sampleProp1, sampleProp2, sampleProp3, sampleProp4, }, }, { - propSubmitOrder: []types.ConsumerRemovalProposal{ + propSubmitOrder: []providertypes.ConsumerRemovalProposal{ sampleProp5, sampleProp4, sampleProp3, sampleProp2, sampleProp1, }, accessTime: now.Add(5 * time.Hour), - expectedOrderedProps: []types.ConsumerRemovalProposal{ + expectedOrderedProps: []providertypes.ConsumerRemovalProposal{ sampleProp1, sampleProp2, sampleProp3, sampleProp4, sampleProp5, }, }, @@ -666,11 +684,11 @@ func TestMakeConsumerGenesis(t *testing.T) { // They must be populated with reasonable values to satisfy SetParams though. TrustingPeriodFraction: providertypes.DefaultTrustingPeriodFraction, CcvTimeoutPeriod: ccvtypes.DefaultCCVTimeoutPeriod, - InitTimeoutPeriod: types.DefaultInitTimeoutPeriod, - VscTimeoutPeriod: types.DefaultVscTimeoutPeriod, - SlashMeterReplenishPeriod: types.DefaultSlashMeterReplenishPeriod, - SlashMeterReplenishFraction: types.DefaultSlashMeterReplenishFraction, - MaxPendingSlashPackets: types.DefaultMaxPendingSlashPackets, + InitTimeoutPeriod: providertypes.DefaultInitTimeoutPeriod, + VscTimeoutPeriod: providertypes.DefaultVscTimeoutPeriod, + SlashMeterReplenishPeriod: providertypes.DefaultSlashMeterReplenishPeriod, + SlashMeterReplenishFraction: providertypes.DefaultSlashMeterReplenishFraction, + MaxPendingSlashPackets: providertypes.DefaultMaxPendingSlashPackets, } providerKeeper.SetParams(ctx, moduleParams) defer ctrl.Finish() @@ -728,36 +746,49 @@ func TestBeginBlockInit(t *testing.T) { pendingProps := []*providertypes.ConsumerAdditionProposal{ providertypes.NewConsumerAdditionProposal( - "title", "description", "chain1", clienttypes.NewHeight(3, 4), []byte{}, []byte{}, - now.Add(-time.Hour).UTC(), + "title", "spawn time passed", "chain1", clienttypes.NewHeight(3, 4), []byte{}, []byte{}, + now.Add(-time.Hour*2).UTC(), "0.75", 10, 10000, 100000000000, 100000000000, - 100000000000).(*providertypes.ConsumerAdditionProposal), + 100000000000, + ).(*providertypes.ConsumerAdditionProposal), providertypes.NewConsumerAdditionProposal( - "title", "description", "chain2", clienttypes.NewHeight(3, 4), []byte{}, []byte{}, - now.UTC(), + "title", "spawn time passed", "chain2", clienttypes.NewHeight(3, 4), []byte{}, []byte{}, + now.Add(-time.Hour*1).UTC(), "0.75", 10, 10000, 100000000000, 100000000000, - 100000000000).(*providertypes.ConsumerAdditionProposal), + 100000000000, + ).(*providertypes.ConsumerAdditionProposal), providertypes.NewConsumerAdditionProposal( - "title", "description", "chain3", clienttypes.NewHeight(3, 4), []byte{}, []byte{}, + "title", "spawn time not passed", "chain3", clienttypes.NewHeight(3, 4), []byte{}, []byte{}, now.Add(time.Hour).UTC(), "0.75", 10, 10000, 100000000000, 100000000000, - 100000000000).(*providertypes.ConsumerAdditionProposal), + 100000000000, + ).(*providertypes.ConsumerAdditionProposal), + providertypes.NewConsumerAdditionProposal( + "title", "invalid proposal: chain id already exists", "chain2", clienttypes.NewHeight(4, 5), []byte{}, []byte{}, + now.UTC(), + "0.75", + 10, + 10000, + 100000000000, + 100000000000, + 100000000000, + ).(*providertypes.ConsumerAdditionProposal), } + // Expect client creation for only for the 1st and second proposals (spawn time already passed and valid) gomock.InOrder( - // Expect client creation for the 1st and second proposals (spawn time already passed) append(testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chain1", clienttypes.NewHeight(3, 4)), testkeeper.GetMocksForCreateConsumerClient(ctx, &mocks, "chain2", clienttypes.NewHeight(3, 4))...)..., ) @@ -769,16 +800,23 @@ func TestBeginBlockInit(t *testing.T) { providerKeeper.BeginBlockInit(ctx) - // Only the 3rd (final) proposal is still stored as pending + // Only the third proposal is still stored as pending _, found := providerKeeper.GetPendingConsumerAdditionProp( ctx, pendingProps[0].SpawnTime, pendingProps[0].ChainId) require.False(t, found) + _, found = providerKeeper.GetPendingConsumerAdditionProp( ctx, pendingProps[1].SpawnTime, pendingProps[1].ChainId) require.False(t, found) + _, found = providerKeeper.GetPendingConsumerAdditionProp( ctx, pendingProps[2].SpawnTime, pendingProps[2].ChainId) require.True(t, found) + + // check that the invalid proposal was dropped + _, found = providerKeeper.GetPendingConsumerAdditionProp( + ctx, pendingProps[3].SpawnTime, pendingProps[3].ChainId) + require.False(t, found) } // TestBeginBlockCCR tests BeginBlockCCR against the spec. @@ -839,6 +877,12 @@ func TestBeginBlockCCR(t *testing.T) { providerKeeper.SetPendingConsumerRemovalProp(ctx, prop.ChainId, prop.StopTime) } + // Add an invalid prop to the store with an non-existing chain id + invalidProp := providertypes.NewConsumerRemovalProposal( + "title", "description", "chain4", now.Add(-time.Hour).UTC(), + ).(*providertypes.ConsumerRemovalProposal) + providerKeeper.SetPendingConsumerRemovalProp(ctx, invalidProp.ChainId, invalidProp.StopTime) + // // Test execution // @@ -854,13 +898,16 @@ func TestBeginBlockCCR(t *testing.T) { found = providerKeeper.GetPendingConsumerRemovalProp( ctx, pendingProps[2].ChainId, pendingProps[2].StopTime) require.True(t, found) + found = providerKeeper.GetPendingConsumerRemovalProp( + ctx, invalidProp.ChainId, invalidProp.StopTime) + require.False(t, found) } // Test getting both matured and pending comnsumer addition proposals func TestGetAllConsumerAdditionProps(t *testing.T) { now := time.Now().UTC() - props := []types.ConsumerAdditionProposal{ + props := []providertypes.ConsumerAdditionProposal{ {ChainId: "1", SpawnTime: now.Add(1 * time.Hour)}, {ChainId: "2", SpawnTime: now.Add(2 * time.Hour)}, {ChainId: "3", SpawnTime: now.Add(3 * time.Hour)}, @@ -889,7 +936,7 @@ func TestGetAllConsumerAdditionProps(t *testing.T) { func TestGetAllConsumerRemovalProps(t *testing.T) { now := time.Now().UTC() - props := []types.ConsumerRemovalProposal{ + props := []providertypes.ConsumerRemovalProposal{ {ChainId: "1", StopTime: now.Add(1 * time.Hour)}, {ChainId: "2", StopTime: now.Add(2 * time.Hour)}, {ChainId: "3", StopTime: now.Add(3 * time.Hour)}, diff --git a/x/ccv/types/errors.go b/x/ccv/types/errors.go index f20f294f1e..7b67a02d60 100644 --- a/x/ccv/types/errors.go +++ b/x/ccv/types/errors.go @@ -23,4 +23,6 @@ var ( ErrInvalidHandshakeMetadata = sdkerrors.Register(ModuleName, 16, "invalid provider handshake metadata") ErrChannelNotFound = sdkerrors.Register(ModuleName, 17, "channel not found") ErrClientNotFound = sdkerrors.Register(ModuleName, 18, "client not found") + ErrDuplicateConsumerChain = sdkerrors.Register(ModuleName, 19, "consumer chain already exists") + ErrConsumerChainNotFound = sdkerrors.Register(ModuleName, 20, "consumer chain not found") )