From 12ac66c1701c73a076a46ebd09e3233695f2a722 Mon Sep 17 00:00:00 2001 From: Simon Noetzlin Date: Wed, 21 Sep 2022 18:42:56 +0200 Subject: [PATCH] Cleanup states after consumer chain removal (#268) * * Remove provider client ID from consumer chain before a shutdown due to timeout * Remove PendingVSCPackets from provider states after a consumer chain removal * remove consumer genesis from provider states after removal * Update x/ccv/consumer/module.go Co-authored-by: Marius Poke * make pass the tests * comment tests * fix error check in tests * fix nits * * remove consumer delete operations before shutdown * move unbonding op indexes deletion outside of the iterator in stop consumer * remove unused iterators Co-authored-by: Marius Poke --- app/consumer/app.go | 2 +- tests/e2e/stop_consumer_test.go | 37 +++++++++++++++++++++++++--- x/ccv/consumer/keeper/keeper.go | 20 +++++++-------- x/ccv/consumer/keeper/keeper_test.go | 2 +- x/ccv/consumer/keeper/relay.go | 7 +++--- x/ccv/consumer/module.go | 3 --- x/ccv/consumer/types/keys.go | 5 ++-- x/ccv/provider/keeper/keeper.go | 5 ++++ x/ccv/provider/keeper/proposal.go | 14 ++++++++--- 9 files changed, 67 insertions(+), 28 deletions(-) diff --git a/app/consumer/app.go b/app/consumer/app.go index 7f60c0dc47..7bf07e3851 100644 --- a/app/consumer/app.go +++ b/app/consumer/app.go @@ -347,7 +347,7 @@ func New( app.GetSubspace(slashingtypes.ModuleName), ) - // register slashing module StakingHooks to the consumer keeper + // register slashing module Slashing hooks to the consumer keeper app.ConsumerKeeper = *app.ConsumerKeeper.SetHooks(app.SlashingKeeper.Hooks()) consumerModule := ibcconsumer.NewAppModule(app.ConsumerKeeper) diff --git a/tests/e2e/stop_consumer_test.go b/tests/e2e/stop_consumer_test.go index 7e9c0251e8..b2c4452544 100644 --- a/tests/e2e/stop_consumer_test.go +++ b/tests/e2e/stop_consumer_test.go @@ -7,6 +7,7 @@ import ( stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types" clienttypes "github.com/cosmos/ibc-go/v3/modules/core/02-client/types" channeltypes "github.com/cosmos/ibc-go/v3/modules/core/04-channel/types" + appConsumer "github.com/cosmos/interchain-security/app/consumer" appProvider "github.com/cosmos/interchain-security/app/provider" providertypes "github.com/cosmos/interchain-security/x/ccv/provider/types" ccv "github.com/cosmos/interchain-security/x/ccv/types" @@ -77,6 +78,7 @@ func (s *ProviderTestSuite) TestStopConsumerChain() { func(suite *ProviderTestSuite) error { s.providerChain.App.(*appProvider.App).ProviderKeeper.SetSlashAcks(s.providerCtx(), consumerChainID, []string{"validator-1", "validator-2", "validator-3"}) s.providerChain.App.(*appProvider.App).ProviderKeeper.SetLockUnbondingOnTimeout(s.providerCtx(), consumerChainID) + s.providerChain.App.(*appProvider.App).ProviderKeeper.AppendPendingVSC(s.providerCtx(), consumerChainID, ccv.ValidatorSetChangePacketData{ValsetUpdateId: 1}) return nil }, }, @@ -247,8 +249,10 @@ func (s *ProviderTestSuite) checkConsumerChainIsRemoved(chainID string, lockUbd } // verify consumer chain's states are removed + _, found := providerKeeper.GetConsumerGenesis(s.providerCtx(), chainID) + s.Require().False(found) s.Require().False(providerKeeper.GetLockUnbondingOnTimeout(s.providerCtx(), chainID)) - _, found := providerKeeper.GetConsumerClientId(s.providerCtx(), chainID) + _, found = providerKeeper.GetConsumerClientId(s.providerCtx(), chainID) s.Require().False(found) _, found = providerKeeper.GetChainToChannel(s.providerCtx(), chainID) @@ -259,12 +263,11 @@ func (s *ProviderTestSuite) checkConsumerChainIsRemoved(chainID string, lockUbd s.Require().Nil(providerKeeper.GetSlashAcks(s.providerCtx(), chainID)) s.Require().Zero(providerKeeper.GetInitChainHeight(s.providerCtx(), chainID)) - // TODO Simon: check that pendingVSCPacket are emptied - once - // https://github.com/cosmos/interchain-security/issues/27 is implemented + s.Require().Nil(providerKeeper.GetPendingVSCs(s.providerCtx(), chainID)) } // TODO Simon: duplicated from consumer/keeper_test.go; figure out how it can be refactored -// SendEmptyVSCPacket sends a VSC packet without any changes +// SendEmptyVSCPacket sends a VSC packet with no valset changes // to ensure that the CCV channel gets established func (s *ProviderTestSuite) SendEmptyVSCPacket() { providerKeeper := s.providerChain.App.(*appProvider.App).ProviderKeeper @@ -292,3 +295,29 @@ func (s *ProviderTestSuite) SendEmptyVSCPacket() { err = s.path.EndpointA.RecvPacket(packet) s.Require().NoError(err) } + +// TestProviderChannelClosed checks that a consumer chain panics +// when the provider channel was established and then closed +func (suite *ConsumerKeeperTestSuite) TestProviderChannelClosed() { + + suite.SetupCCVChannel() + // establish provider channel with a first VSC packet + suite.SendEmptyVSCPacket() + + channelID, found := suite.consumerChain.App.(*appConsumer.App).ConsumerKeeper.GetProviderChannel(suite.consumerChain.GetContext()) + suite.Require().True(found) + + // close provider channel + err := suite.consumerChain.App.(*appConsumer.App).ConsumerKeeper.ChanCloseInit(suite.consumerChain.GetContext(), ccv.ConsumerPortID, channelID) + suite.Require().NoError(err) + suite.Require().True(suite.consumerChain.App.(*appConsumer.App).ConsumerKeeper.IsChannelClosed(suite.consumerChain.GetContext(), channelID)) + + // assert begin blocker did panics + defer func() { + if r := recover(); r != nil { + return + } + suite.Require().Fail("Begin blocker did not panic with a closed channel") + }() + suite.consumerChain.App.(*appConsumer.App).BeginBlocker(suite.consumerChain.GetContext(), abci.RequestBeginBlock{}) +} diff --git a/x/ccv/consumer/keeper/keeper.go b/x/ccv/consumer/keeper/keeper.go index 6fbe2e74d5..4bf624ba20 100644 --- a/x/ccv/consumer/keeper/keeper.go +++ b/x/ccv/consumer/keeper/keeper.go @@ -268,7 +268,7 @@ func (k Keeper) GetPacketMaturityTime(ctx sdk.Context, vscId uint64) uint64 { return binary.BigEndian.Uint64(bz) } -// DeletePacketMaturityTime deletes the the maturity time for a given received VSC packet id +// DeletePacketMaturityTime deletes the packet maturity time for a given received VSC packet id func (k Keeper) DeletePacketMaturityTime(ctx sdk.Context, vscId uint64) { store := ctx.KVStore(k.storeKey) store.Delete(types.PacketMaturityTimeKey(vscId)) @@ -334,9 +334,9 @@ func (k Keeper) SetOutstandingDowntime(ctx sdk.Context, address sdk.ConsAddress) store.Set(types.OutstandingDowntimeKey(address), []byte{}) } -// ClearOutstandingDowntime clears the outstanding downtime flag for a given validator -func (k Keeper) ClearOutstandingDowntime(ctx sdk.Context, address string) { - consAddr, err := sdk.ConsAddressFromBech32(address) +// DeleteOutstandingDowntime deletes the outstanding downtime flag for the given validator consensus address +func (k Keeper) DeleteOutstandingDowntime(ctx sdk.Context, consAddress string) { + consAddr, err := sdk.ConsAddressFromBech32(consAddress) if err != nil { return } @@ -417,15 +417,15 @@ func (k Keeper) GetPendingSlashRequests(ctx sdk.Context) []*types.SlashRequest { return sr.GetRequests() } +// ClearPendingSlashRequests clears the pending slash requests in store +func (k Keeper) DeletePendingSlashRequests(ctx sdk.Context) { + store := ctx.KVStore(k.storeKey) + store.Delete([]byte{types.PendingSlashRequestsBytePrefix}) +} + // AppendPendingSlashRequests appends the given slash request to the pending slash requests in store func (k Keeper) AppendPendingSlashRequests(ctx sdk.Context, req types.SlashRequest) { requests := k.GetPendingSlashRequests(ctx) requests = append(requests, &req) k.SetPendingSlashRequests(ctx, requests) } - -// ClearPendingSlashRequests clears the pending slash requests in store -func (k Keeper) ClearPendingSlashRequests(ctx sdk.Context) { - store := ctx.KVStore(k.storeKey) - store.Delete([]byte{types.PendingSlashRequestsBytePrefix}) -} diff --git a/x/ccv/consumer/keeper/keeper_test.go b/x/ccv/consumer/keeper/keeper_test.go index d9f01c3aca..a9c457f29e 100644 --- a/x/ccv/consumer/keeper/keeper_test.go +++ b/x/ccv/consumer/keeper/keeper_test.go @@ -194,7 +194,7 @@ func TestPendingSlashRequests(t *testing.T) { operation: func() { consumerKeeper.AppendPendingSlashRequests(ctx, types.SlashRequest{}) }, expLen: 11, }, { - operation: func() { consumerKeeper.ClearPendingSlashRequests(ctx) }, + operation: func() { consumerKeeper.DeletePendingSlashRequests(ctx) }, expLen: 0, }, } diff --git a/x/ccv/consumer/keeper/relay.go b/x/ccv/consumer/keeper/relay.go index 65f607bc17..5d2c2856c4 100644 --- a/x/ccv/consumer/keeper/relay.go +++ b/x/ccv/consumer/keeper/relay.go @@ -64,9 +64,10 @@ func (k Keeper) OnRecvVSCPacket(ctx sdk.Context, packet channeltypes.Packet, new // set height to VSC id mapping k.SetHeightValsetUpdateID(ctx, uint64(ctx.BlockHeight())+1, newChanges.ValsetUpdateId) - // set outstanding slashing flags to false + // remove outstanding slashing flags of the validators + // for which the slashing was acknowledged by the provider chain for _, addr := range newChanges.GetSlashAcks() { - k.ClearOutstandingDowntime(ctx, addr) + k.DeleteOutstandingDowntime(ctx, addr) } ack := channeltypes.NewResultAcknowledgement([]byte{byte(1)}) @@ -201,7 +202,7 @@ func (k Keeper) SendPendingSlashRequests(ctx sdk.Context) { } // clear pending slash requests - k.ClearPendingSlashRequests(ctx) + k.DeletePendingSlashRequests(ctx) } // OnAcknowledgementPacket executes application logic for acknowledgments of sent VSCMatured and Slash packets diff --git a/x/ccv/consumer/module.go b/x/ccv/consumer/module.go index bb6a9d7cbf..3c8080d5c1 100644 --- a/x/ccv/consumer/module.go +++ b/x/ccv/consumer/module.go @@ -150,9 +150,6 @@ func (am AppModule) BeginBlock(ctx sdk.Context, req abci.RequestBeginBlock) { // the CCV channel was established, but it was then closed; // the consumer chain is no longer safe - // cleanup state - am.keeper.DeleteProviderChannel(ctx) - channelClosedMsg := fmt.Sprintf("CCV channel %q was closed - shutdown consumer chain since it is not secured anymore", channelID) ctx.Logger().Error(channelClosedMsg) panic(channelClosedMsg) diff --git a/x/ccv/consumer/types/keys.go b/x/ccv/consumer/types/keys.go index 0a037f2e7f..a94006840a 100644 --- a/x/ccv/consumer/types/keys.go +++ b/x/ccv/consumer/types/keys.go @@ -4,7 +4,6 @@ import ( "encoding/binary" sdk "github.com/cosmos/cosmos-sdk/types" - "github.com/cosmos/cosmos-sdk/types/address" ) const ( @@ -122,8 +121,8 @@ func HeightValsetUpdateIDKey(height uint64) []byte { } // OutstandingDowntimeKey returns the key to a validators' outstanding downtime by consensus address -func OutstandingDowntimeKey(v sdk.ConsAddress) []byte { - return append([]byte{OutstandingDowntimeBytePrefix}, address.MustLengthPrefix(v.Bytes())...) +func OutstandingDowntimeKey(address sdk.ConsAddress) []byte { + return append([]byte{OutstandingDowntimeBytePrefix}, address.Bytes()...) } // CrossChainValidatorKey returns the key to a cross chain validator by consensus address diff --git a/x/ccv/provider/keeper/keeper.go b/x/ccv/provider/keeper/keeper.go index 495a75115f..9342884690 100644 --- a/x/ccv/provider/keeper/keeper.go +++ b/x/ccv/provider/keeper/keeper.go @@ -228,6 +228,11 @@ func (k Keeper) GetConsumerGenesis(ctx sdk.Context, chainID string) (consumertyp return data, true } +func (k Keeper) DeleteConsumerGenesis(ctx sdk.Context, chainID string) { + store := ctx.KVStore(k.storeKey) + store.Delete(types.ConsumerGenesisKey(chainID)) +} + // VerifyConsumerChain verifies that the chain trying to connect on the channel handshake // is the expected consumer chain. func (k Keeper) VerifyConsumerChain(ctx sdk.Context, channelID string, connectionHops []string) error { diff --git a/x/ccv/provider/keeper/proposal.go b/x/ccv/provider/keeper/proposal.go index e310fa7f32..7e1ed37182 100644 --- a/x/ccv/provider/keeper/proposal.go +++ b/x/ccv/provider/keeper/proposal.go @@ -60,6 +60,7 @@ func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, lockUbd, clos // clean up states k.DeleteConsumerClientId(ctx, chainID) + k.DeleteConsumerGenesis(ctx, chainID) k.DeleteLockUnbondingOnTimeout(ctx, chainID) // close channel and delete the mappings between chain ID and channel ID @@ -71,11 +72,12 @@ func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, lockUbd, clos k.DeleteChannelToChain(ctx, channelID) } - // TODO remove pending VSC packets once https://github.com/cosmos/interchain-security/issues/27 is fixed k.DeleteInitChainHeight(ctx, chainID) k.EmptySlashAcks(ctx, chainID) + k.EmptyPendingVSC(ctx, chainID) // release unbonding operations if they aren't locked + var vscIDs []uint64 if !lockUbd { // iterate over the consumer chain's unbonding operation VSC ids k.IterateOverUnbondingOpIndex(ctx, chainID, func(vscID uint64, ids []uint64) bool { @@ -105,15 +107,21 @@ func (k Keeper) StopConsumerChain(ctx sdk.Context, chainID string, lockUbd, clos if err := k.AppendMaturedUnbondingOps(ctx, maturedIds); err != nil { panic(fmt.Errorf("mature unbonding ops could not be appended: %w", err)) } - // clean up index - k.DeleteUnbondingOpIndex(ctx, chainID, vscID) + + vscIDs = append(vscIDs, vscID) return true }) } + if err != nil { return err } + // clean up indexes + for _, id := range vscIDs { + k.DeleteUnbondingOpIndex(ctx, chainID, id) + } + return nil }