From 38320a2333c6c48ac4252e9106b7083172831b63 Mon Sep 17 00:00:00 2001 From: Marius Poke Date: Tue, 3 Sep 2024 17:04:31 +0200 Subject: [PATCH] fix!: handle some panics (#2205) * getTotalPower should not panic * return error from EndBlockVSU * fix UT * fix error message --- x/ccv/provider/keeper/relay.go | 68 +++++++++++-------- x/ccv/provider/keeper/relay_test.go | 31 ++++++--- .../provider/keeper/validator_set_storage.go | 2 +- 3 files changed, 61 insertions(+), 40 deletions(-) diff --git a/x/ccv/provider/keeper/relay.go b/x/ccv/provider/keeper/relay.go index c95bd71079..b008d79d54 100644 --- a/x/ccv/provider/keeper/relay.go +++ b/x/ccv/provider/keeper/relay.go @@ -60,18 +60,25 @@ func (k Keeper) OnTimeoutPacket(ctx sdk.Context, packet channeltypes.Packet) err // the Validator Set Update sub-protocol func (k Keeper) EndBlockVSU(ctx sdk.Context) ([]abci.ValidatorUpdate, error) { // logic to update the provider consensus validator set. - valUpdates := k.ProviderValidatorUpdates(ctx) + valUpdates, err := k.ProviderValidatorUpdates(ctx) + if err != nil { + return []abci.ValidatorUpdate{}, fmt.Errorf("computing the provider consensus validator set: %w", err) + } if k.BlocksUntilNextEpoch(ctx) == 0 { // only queue and send VSCPackets at the boundaries of an epoch // collect validator updates - k.QueueVSCPackets(ctx) + if err := k.QueueVSCPackets(ctx); err != nil { + return []abci.ValidatorUpdate{}, fmt.Errorf("queueing consumer validator updates: %w", err) + } // try sending VSC packets to all registered consumer chains; // if the CCV channel is not established for a consumer chain, // the updates will remain queued until the channel is established - k.SendVSCPackets(ctx) + if err := k.SendVSCPackets(ctx); err != nil { + return []abci.ValidatorUpdate{}, fmt.Errorf("sending consumer validator updates: %w", err) + } } return valUpdates, nil @@ -82,17 +89,17 @@ func (k Keeper) EndBlockVSU(ctx sdk.Context) ([]abci.ValidatorUpdate, error) { // It retrieves the bonded validators from the staking module and creates a `ConsumerValidator` object for each validator. // The maximum number of validators is determined by the `maxValidators` parameter. // The function returns the difference between the current validator set and the next validator set as a list of `abci.ValidatorUpdate` objects. -func (k Keeper) ProviderValidatorUpdates(ctx sdk.Context) []abci.ValidatorUpdate { +func (k Keeper) ProviderValidatorUpdates(ctx sdk.Context) ([]abci.ValidatorUpdate, error) { // get the bonded validators from the staking module bondedValidators, err := k.stakingKeeper.GetBondedValidatorsByPower(ctx) if err != nil { - panic(fmt.Errorf("failed to get bonded validators: %w", err)) + return []abci.ValidatorUpdate{}, fmt.Errorf("getting bonded validators: %w", err) } // get the last validator set sent to consensus currentValidators, err := k.GetLastProviderConsensusValSet(ctx) if err != nil { - panic(fmt.Errorf("failed to get last provider consensus validator set: %w", err)) + return []abci.ValidatorUpdate{}, fmt.Errorf("getting last provider consensus validator set: %w", err) } nextValidators := []types.ConsensusValidator{} @@ -104,8 +111,8 @@ func (k Keeper) ProviderValidatorUpdates(ctx sdk.Context) []abci.ValidatorUpdate for _, val := range bondedValidators[:maxValidators] { nextValidator, err := k.CreateProviderConsensusValidator(ctx, val) if err != nil { - k.Logger(ctx).Error("error when creating provider consensus validator", "error", err, "validator", val) - continue + return []abci.ValidatorUpdate{}, + fmt.Errorf("creating provider consensus validator(%s): %w", val.OperatorAddress, err) } nextValidators = append(nextValidators, nextValidator) } @@ -115,7 +122,7 @@ func (k Keeper) ProviderValidatorUpdates(ctx sdk.Context) []abci.ValidatorUpdate valUpdates := DiffValidators(currentValidators, nextValidators) - return valUpdates + return valUpdates, nil } // BlocksUntilNextEpoch returns the number of blocks until the next epoch starts @@ -135,17 +142,20 @@ func (k Keeper) BlocksUntilNextEpoch(ctx sdk.Context) int64 { // VSC packets to the chains with established CCV channels. // If the CCV channel is not established for a consumer chain, // the updates will remain queued until the channel is established -func (k Keeper) SendVSCPackets(ctx sdk.Context) { +func (k Keeper) SendVSCPackets(ctx sdk.Context) error { for _, chainID := range k.GetAllRegisteredConsumerChainIDs(ctx) { // check if CCV channel is established and send if channelID, found := k.GetChainToChannel(ctx, chainID); found { - k.SendVSCPacketsToChain(ctx, chainID, channelID) + if err := k.SendVSCPacketsToChain(ctx, chainID, channelID); err != nil { + return fmt.Errorf("sending VSCPacket to consumer, chainID(%s): %w", chainID, err) + } } } + return nil } // SendVSCPacketsToChain sends all queued VSC packets to the specified chain -func (k Keeper) SendVSCPacketsToChain(ctx sdk.Context, chainID, channelID string) { +func (k Keeper) SendVSCPacketsToChain(ctx sdk.Context, chainID, channelID string) error { pendingPackets := k.GetPendingVSCPackets(ctx, chainID) for _, data := range pendingPackets { // send packet over IBC @@ -164,54 +174,52 @@ func (k Keeper) SendVSCPacketsToChain(ctx sdk.Context, chainID, channelID string // leave the packet data stored to be sent once the client is upgraded // the client cannot expire during iteration (in the middle of a block) k.Logger(ctx).Info("IBC client is expired, cannot send VSC, leaving packet data stored:", "chainID", chainID, "vscid", data.ValsetUpdateId) - return + return nil } // Not able to send packet over IBC! k.Logger(ctx).Error("cannot send VSC, removing consumer:", "chainID", chainID, "vscid", data.ValsetUpdateId, "err", err.Error()) // If this happens, most likely the consumer is malicious; remove it err := k.StopConsumerChain(ctx, chainID, true) if err != nil { - panic(fmt.Errorf("consumer chain failed to stop: %w", err)) + return fmt.Errorf("stopping consumer, chainID(%s): %w", chainID, err) } - return + return nil } } k.DeletePendingVSCPackets(ctx, chainID) + + return nil } // QueueVSCPackets queues latest validator updates for every registered consumer chain -// failing to GetLastBondedValidators will cause a panic in EndBlock - -// TODO: decide if this func shouldn't return an error to be propagated to BeginBlocker -func (k Keeper) QueueVSCPackets(ctx sdk.Context) { +func (k Keeper) QueueVSCPackets(ctx sdk.Context) error { valUpdateID := k.GetValidatorSetUpdateId(ctx) // current valset update ID // get the bonded validators from the staking module bondedValidators, err := k.GetLastBondedValidators(ctx) if err != nil { - panic(fmt.Errorf("failed to get last validators: %w", err)) + return fmt.Errorf("getting bonded validators: %w", err) + } + + // get the provider active validators + activeValidators, err := k.GetLastProviderConsensusActiveValidators(ctx) + if err != nil { + return fmt.Errorf("getting provider active validators: %w", err) } for _, chainID := range k.GetAllRegisteredConsumerChainIDs(ctx) { currentValidators, err := k.GetConsumerValSet(ctx, chainID) if err != nil { - panic(fmt.Errorf("failed to get consumer validators: %w", err)) + return fmt.Errorf("getting consumer validators, chainID(%s): %w", chainID, err) } topN, _ := k.GetTopN(ctx, chainID) if topN > 0 { // in a Top-N chain, we automatically opt in all validators that belong to the top N // of the active validators - activeValidators, err := k.GetLastProviderConsensusActiveValidators(ctx) - if err != nil { - // something must be broken in the bonded validators, so we have to panic since there is no realistic way to proceed - panic(fmt.Errorf("failed to get active validators: %w", err)) - } - minPower, err := k.ComputeMinPowerInTopN(ctx, activeValidators, topN) if err != nil { - // we panic, since the only way to proceed would be to opt in all validators, which is not the intended behavior - panic(fmt.Errorf("failed to compute min power to opt in for chain %v: %w", chainID, err)) + return fmt.Errorf("computing min power to opt in, chainID(%s): %w", chainID, err) } // set the minimal power of validators in the top N in the store @@ -239,6 +247,8 @@ func (k Keeper) QueueVSCPackets(ctx sdk.Context) { } k.IncrementValidatorSetUpdateId(ctx) + + return nil } // BeginBlockCIS contains the BeginBlock logic needed for the Consumer Initiated Slashing sub-protocol. diff --git a/x/ccv/provider/keeper/relay_test.go b/x/ccv/provider/keeper/relay_test.go index 91de369851..ad427ca6c7 100644 --- a/x/ccv/provider/keeper/relay_test.go +++ b/x/ccv/provider/keeper/relay_test.go @@ -68,11 +68,14 @@ func TestQueueVSCPackets(t *testing.T) { mocks := testkeeper.NewMockedKeepers(ctrl) testkeeper.SetupMocksForLastBondedValidatorsExpectation(mocks.MockStakingKeeper, 0, []stakingtypes.Validator{}, 1) + mocks.MockStakingKeeper.EXPECT().GetBondedValidatorsByPower(gomock.Any()).Return([]stakingtypes.Validator{}, nil).AnyTimes() + pk := testkeeper.NewInMemProviderKeeper(keeperParams, mocks) // no-op if tc.packets is empty pk.AppendPendingVSCPackets(ctx, chainID, tc.packets...) - pk.QueueVSCPackets(ctx) + err := pk.QueueVSCPackets(ctx) + require.NoError(t, err) pending := pk.GetPendingVSCPackets(ctx, chainID) require.Len(t, pending, tc.expectedQueueSize, "pending vsc queue mismatch (%v != %v) in case: '%s'", tc.expectedQueueSize, len(pending), tc.name) @@ -120,7 +123,8 @@ func TestQueueVSCPacketsDoesNotResetConsumerValidatorsHeights(t *testing.T) { // validator for the first time after the `QueueVSCPackets` call. providerKeeper.SetOptedIn(ctx, "chainID", providertypes.NewProviderConsAddress(valBConsAddr)) - providerKeeper.QueueVSCPackets(ctx) + err := providerKeeper.QueueVSCPackets(ctx) + require.NoError(t, err) // the height of consumer validator A should not be modified because A was already a consumer validator cv, _ := providerKeeper.GetConsumerValidator(ctx, "chainID", providertypes.NewProviderConsAddress(valAConsAddr)) @@ -501,8 +505,9 @@ func TestSendVSCPacketsToChainFailure(t *testing.T) { require.NoError(t, err) providerKeeper.SetConsumerClientId(ctx, "consumerChainID", "clientID") - // No panic should occur, StopConsumerChain should be called - providerKeeper.SendVSCPacketsToChain(ctx, "consumerChainID", "CCVChannelID") + // No error should occur, StopConsumerChain should be called + err = providerKeeper.SendVSCPacketsToChain(ctx, "consumerChainID", "CCVChannelID") + require.NoError(t, err) // Pending VSC packets should be deleted in StopConsumerChain require.Empty(t, providerKeeper.GetPendingVSCPackets(ctx, "consumerChainID")) @@ -616,24 +621,28 @@ func TestEndBlockVSU(t *testing.T) { // with block height of 1 we do not expect any queueing of VSC packets ctx = ctx.WithBlockHeight(1) - providerKeeper.EndBlockVSU(ctx) + _, err := providerKeeper.EndBlockVSU(ctx) + require.NoError(t, err) require.Equal(t, 0, len(providerKeeper.GetPendingVSCPackets(ctx, chainID))) // with block height of 5 we do not expect any queueing of VSC packets ctx = ctx.WithBlockHeight(5) - providerKeeper.EndBlockVSU(ctx) + _, err = providerKeeper.EndBlockVSU(ctx) + require.NoError(t, err) require.Equal(t, 0, len(providerKeeper.GetPendingVSCPackets(ctx, chainID))) // with block height of 10 we expect the queueing of one VSC packet ctx = ctx.WithBlockHeight(10) - providerKeeper.EndBlockVSU(ctx) + _, err = providerKeeper.EndBlockVSU(ctx) + require.NoError(t, err) require.Equal(t, 1, len(providerKeeper.GetPendingVSCPackets(ctx, chainID))) // With block height of 15 we expect no additional queueing of a VSC packet. // Note that the pending VSC packet is still there because `SendVSCPackets` does not send the packet. We // need to mock channels, etc. for this to work, and it's out of scope for this test. ctx = ctx.WithBlockHeight(15) - providerKeeper.EndBlockVSU(ctx) + _, err = providerKeeper.EndBlockVSU(ctx) + require.NoError(t, err) require.Equal(t, 1, len(providerKeeper.GetPendingVSCPackets(ctx, chainID))) } @@ -699,7 +708,8 @@ func TestProviderValidatorUpdates(t *testing.T) { } // Execute the function - updates := providerKeeper.ProviderValidatorUpdates(ctx) + updates, err := providerKeeper.ProviderValidatorUpdates(ctx) + require.NoError(t, err) // Assertions require.ElementsMatch(t, expectedUpdates, updates, "The validator updates should match the expected updates") @@ -756,7 +766,8 @@ func TestQueueVSCPacketsWithPowerCapping(t *testing.T) { params.MaxProviderConsensusValidators = 180 providerKeeper.SetParams(ctx, params) - providerKeeper.QueueVSCPackets(ctx) + err := providerKeeper.QueueVSCPackets(ctx) + require.NoError(t, err) actualQueuedVSCPackets := providerKeeper.GetPendingVSCPackets(ctx, "chainID") expectedQueuedVSCPackets := []ccv.ValidatorSetChangePacketData{ diff --git a/x/ccv/provider/keeper/validator_set_storage.go b/x/ccv/provider/keeper/validator_set_storage.go index cad322ed6b..b829f2f743 100644 --- a/x/ccv/provider/keeper/validator_set_storage.go +++ b/x/ccv/provider/keeper/validator_set_storage.go @@ -112,7 +112,7 @@ func (k Keeper) getTotalPower(ctx sdk.Context, prefix []byte) (math.Int, error) totalPower := math.ZeroInt() validators, err := k.getValSet(ctx, prefix) if err != nil { - panic(fmt.Errorf("retrieving validator set: %w", err)) + return totalPower, err } for _, val := range validators { totalPower = totalPower.Add(math.NewInt(val.Power))