Consumer panic cleanup

x/ccv/consumer/keeper/genesis.go

@@ -33,6 +33,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, state *consumertypes.GenesisState)
 		// and claims the returned capability
 		err := k.BindPort(ctx, ccv.ConsumerPortID)
 		if err != nil {
+			// If the binding fails, the chain MUST NOT start
 			panic(fmt.Sprintf("could not claim port capability: %v", err))
 		}
 	}
@@ -43,6 +44,7 @@ func (k Keeper) InitGenesis(ctx sdk.Context, state *consumertypes.GenesisState)
 		// create the provider client in InitGenesis for new consumer chain. CCV Handshake must be established with this client id.
 		clientID, err := k.clientKeeper.CreateClient(ctx, state.ProviderClientState, state.ProviderConsensusState)
 		if err != nil {
+			// If the client creation fails, the chain MUST NOT start
 			panic(err)
 		}
 
@@ -106,16 +108,14 @@ func (k Keeper) ExportGenesis(ctx sdk.Context) (genesis *consumertypes.GenesisSt
 	}
 
 	// export the current validator set
-	valset, err := k.GetCurrentValidatorsAsABCIUpdates(ctx)
-	if err != nil {
-		panic(fmt.Sprintf("fail to retrieve the validator set: %s", err))
-	}
+	valset := k.MustGetCurrentValidatorsAsABCIUpdates(ctx)
 
 	// export all the states created after a provider channel got established
 	if channelID, ok := k.GetProviderChannel(ctx); ok {
-		clientID, ok := k.GetProviderClientID(ctx)
-		if !ok {
-			panic("provider client does not exist")
+		clientID, found := k.GetProviderClientID(ctx)
+		if !found {
+			// This should never happen
+			panic("provider client does not exist although provider channel does exist")
 		}
 
 		// TODO: update GetLastTransmissionBlockHeight to not return an error

x/ccv/consumer/keeper/keeper.go

@@ -83,6 +83,8 @@ func (k Keeper) Logger(ctx sdk.Context) log.Logger {
 
 func (k *Keeper) SetHooks(sh ccv.ConsumerHooks) *Keeper {
 	if k.hooks != nil {
+		// This should never happen as SetHooks is expected
+		// to be called only once in app.go
 		panic("cannot set validator hooks twice")
 	}
 
@@ -177,14 +179,14 @@ func (k Keeper) DeleteProviderChannel(ctx sdk.Context) {
 }
 
 // SetPendingChanges sets the pending validator set change packet that haven't been flushed to ABCI
-func (k Keeper) SetPendingChanges(ctx sdk.Context, updates ccv.ValidatorSetChangePacketData) error {
+func (k Keeper) SetPendingChanges(ctx sdk.Context, updates ccv.ValidatorSetChangePacketData) {
 	store := ctx.KVStore(k.storeKey)
 	bz, err := updates.Marshal()
 	if err != nil {
-		return err
+		// This should never happen
+		panic(fmt.Errorf("failed to marshal PendingChanges: %w", err))
 	}
 	store.Set(types.PendingChangesKey(), bz)
-	return nil
 }
 
 // GetPendingChanges gets the pending changes that haven't been flushed over ABCI
@@ -196,7 +198,9 @@ func (k Keeper) GetPendingChanges(ctx sdk.Context) (*ccv.ValidatorSetChangePacke
 	}
 	var data ccv.ValidatorSetChangePacketData
 	if err := data.Unmarshal(bz); err != nil {
-		panic(fmt.Errorf("pending changes could not be unmarshaled: %w", err))
+		// This should never happen as PendingChanges is expected
+		// to be correctly serialized in SetPendingChanges
+		panic(fmt.Errorf("failed to unmarshal PendingChanges: %w", err))
 	}
 	return &data, true
 }
@@ -218,6 +222,8 @@ func (k Keeper) GetElapsedPacketMaturityTimes(ctx sdk.Context) (maturingVSCPacke
 	for ; iterator.Valid(); iterator.Next() {
 		var maturingVSCPacket consumertypes.MaturingVSCPacket
 		if err := maturingVSCPacket.Unmarshal(iterator.Value()); err != nil {
+			// An error here would indicate something is very wrong,
+			// the MaturingVSCPackets are assumed to be correctly serialized in SetPacketMaturityTime.
 			panic(fmt.Errorf("failed to unmarshal MaturingVSCPacket: %w", err))
 		}
 
@@ -247,6 +253,8 @@ func (k Keeper) GetAllPacketMaturityTimes(ctx sdk.Context) (maturingVSCPackets [
 	for ; iterator.Valid(); iterator.Next() {
 		var maturingVSCPacket consumertypes.MaturingVSCPacket
 		if err := maturingVSCPacket.Unmarshal(iterator.Value()); err != nil {
+			// An error here would indicate something is very wrong,
+			// the MaturingVSCPackets are assumed to be correctly serialized in SetPacketMaturityTime.
 			panic(fmt.Errorf("failed to unmarshal MaturingVSCPacket: %w", err))
 		}
 
@@ -264,6 +272,8 @@ func (k Keeper) SetPacketMaturityTime(ctx sdk.Context, vscId uint64, maturityTim
 	}
 	bz, err := maturingVSCPacket.Marshal()
 	if err != nil {
+		// An error here would indicate something is very wrong,
+		// maturingVSCPacket is instantiated in this method and should be able to be marshaled.
 		panic(fmt.Errorf("failed to marshal MaturingVSCPacket: %w", err))
 	}
 	store.Set(types.PacketMaturityTimeKey(vscId, maturityTime), bz)
@@ -450,7 +460,8 @@ func (k Keeper) SetPendingPackets(ctx sdk.Context, packets ccv.ConsumerPacketDat
 	store := ctx.KVStore(k.storeKey)
 	bz, err := packets.Marshal()
 	if err != nil {
-		panic(fmt.Errorf("failed to encode packet: %w", err))
+		// This should never happen
+		panic(fmt.Errorf("failed to marshal ConsumerPacketDataList: %w", err))
 	}
 	store.Set([]byte{types.PendingDataPacketsBytePrefix}, bz)
 }
@@ -467,6 +478,8 @@ func (k Keeper) GetPendingPackets(ctx sdk.Context) ccv.ConsumerPacketDataList {
 
 	err := packets.Unmarshal(bz)
 	if err != nil {
+		// An error here would indicate something is very wrong,
+		// the PendingPackets are assumed to be correctly serialized in SetPendingPackets.
 		panic(fmt.Errorf("failed to unmarshal pending data packets: %w", err))
 	}
 

x/ccv/consumer/keeper/keeper_test.go

@@ -73,8 +73,7 @@ func TestPendingChanges(t *testing.T) {
 	consumerKeeper, ctx, ctrl, _ := testkeeper.GetConsumerKeeperAndCtx(t, testkeeper.NewInMemKeeperParams(t))
 	defer ctrl.Finish()
 
-	err = consumerKeeper.SetPendingChanges(ctx, pd)
-	require.NoError(t, err)
+	consumerKeeper.SetPendingChanges(ctx, pd)
 	gotPd, ok := consumerKeeper.GetPendingChanges(ctx)
 	require.True(t, ok)
 	require.Equal(t, &pd, gotPd, "packet data in store does not equal packet data set")

x/ccv/consumer/keeper/relay.go

@@ -56,12 +56,9 @@ func (k Keeper) OnRecvVSCPacket(ctx sdk.Context, packet channeltypes.Packet, new
 		pendingChanges = utils.AccumulateChanges(currentChanges.ValidatorUpdates, newChanges.ValidatorUpdates)
 	}
 
-	err := k.SetPendingChanges(ctx, ccv.ValidatorSetChangePacketData{
+	k.SetPendingChanges(ctx, ccv.ValidatorSetChangePacketData{
 		ValidatorUpdates: pendingChanges,
 	})
-	if err != nil {
-		panic(fmt.Errorf("pending validator set change could not be persisted: %w", err))
-	}
 
 	// Save maturity time and packet
 	maturityTime := ctx.BlockTime().Add(k.GetUnbondingPeriod(ctx))
@@ -99,9 +96,6 @@ func (k Keeper) OnRecvVSCPacket(ctx sdk.Context, packet channeltypes.Packet, new
 // the consumer chain.
 func (k Keeper) QueueVSCMaturedPackets(ctx sdk.Context) {
 	for _, maturityTime := range k.GetElapsedPacketMaturityTimes(ctx) {
-		if ctx.BlockTime().Before(maturityTime.MaturityTime) {
-			panic(fmt.Errorf("maturity time %s is after than current time %s", maturityTime.MaturityTime, ctx.BlockTime()))
-		}
 		// construct validator set change packet data
 		vscPacket := ccv.NewVSCMaturedPacketData(maturityTime.VscId)
 
@@ -211,6 +205,7 @@ func (k Keeper) SendPackets(ctx sdk.Context) {
 				return
 			}
 			// something went wrong when sending the packet
+			// TODO do not panic if the send fails
 			panic(fmt.Errorf("packet could not be sent over IBC: %w", err))
 		}
 	}

x/ccv/consumer/keeper/validators.go

@@ -8,7 +8,6 @@ import (
 	sdk "github.com/cosmos/cosmos-sdk/types"
 	stakingtypes "github.com/cosmos/cosmos-sdk/x/staking/types"
 	"github.com/cosmos/interchain-security/x/ccv/consumer/types"
-	"github.com/cosmos/interchain-security/x/ccv/utils"
 	abci "github.com/tendermint/tendermint/abci/types"
 )
 
@@ -17,7 +16,14 @@ import (
 func (k Keeper) ApplyCCValidatorChanges(ctx sdk.Context, changes []abci.ValidatorUpdate) []abci.ValidatorUpdate {
 	ret := []abci.ValidatorUpdate{}
 	for _, change := range changes {
-		addr := utils.GetChangePubKeyAddress(change)
+		// convert TM pubkey to SDK pubkey
+		pubkey, err := cryptocodec.FromTmProtoPublicKey(change.GetPubKey())
+		if err != nil {
+			// An error here would indicate that the validator updates
+			// received from the provider are invalid.
+			panic(err)
+		}
+		addr := pubkey.Address()
 		val, found := k.GetCCValidator(ctx, addr)
 
 		if found {
@@ -33,12 +39,10 @@ func (k Keeper) ApplyCCValidatorChanges(ctx sdk.Context, changes []abci.Validato
 			// create a new validator
 			consAddr := sdk.ConsAddress(addr)
 
-			pubkey, err := cryptocodec.FromTmProtoPublicKey(change.GetPubKey())
-			if err != nil {
-				panic(err)
-			}
 			ccVal, err := types.NewCCValidator(addr, change.Power, pubkey)
 			if err != nil {
+				// An error here would indicate that the validator updates
+				// received from the provider are invalid.
 				panic(err)
 			}
 
@@ -186,10 +190,14 @@ func (k Keeper) TrackHistoricalInfo(ctx sdk.Context) {
 	for _, v := range k.GetAllCCValidator(ctx) {
 		pk, err := v.ConsPubKey()
 		if err != nil {
+			// This should never happen as the pubkey is assumed
+			// to be stored correctly in ApplyCCValidatorChanges.
 			panic(err)
 		}
 		val, err := stakingtypes.NewValidator(nil, pk, stakingtypes.Description{})
 		if err != nil {
+			// This should never happen as the pubkey is assumed
+			// to be stored correctly in ApplyCCValidatorChanges.
 			panic(err)
 		}
 
@@ -207,20 +215,25 @@ func (k Keeper) TrackHistoricalInfo(ctx sdk.Context) {
 	k.SetHistoricalInfo(ctx, ctx.BlockHeight(), &historicalEntry)
 }
 
-// GetCurrentValidatorsAsABCIUpdates gets all cross-chain validators converted to the ABCI validator update type
-func (k Keeper) GetCurrentValidatorsAsABCIUpdates(ctx sdk.Context) ([]abci.ValidatorUpdate, error) {
+// MustGetCurrentValidatorsAsABCIUpdates gets all cross-chain validators converted
+// to the ABCI validator update type. It panics in case of failure.
+func (k Keeper) MustGetCurrentValidatorsAsABCIUpdates(ctx sdk.Context) []abci.ValidatorUpdate {
 	vals := k.GetAllCCValidator(ctx)
 	valUpdates := make([]abci.ValidatorUpdate, 0, len(vals))
 	for _, v := range vals {
 		pk, err := v.ConsPubKey()
 		if err != nil {
-			return nil, err
+			// This should never happen as the pubkey is assumed
+			// to be stored correctly in ApplyCCValidatorChanges.
+			panic(err)
 		}
 		tmPK, err := cryptocodec.ToTmProtoPublicKey(pk)
 		if err != nil {
-			return nil, err
+			// This should never happen as the pubkey is assumed
+			// to be stored correctly in ApplyCCValidatorChanges.
+			panic(err)
 		}
 		valUpdates = append(valUpdates, abci.ValidatorUpdate{PubKey: tmPK, Power: v.Power})
 	}
-	return valUpdates, nil
+	return valUpdates
 }

x/ccv/utils/utils.go

@@ -44,15 +44,6 @@ func AccumulateChanges(currentChanges, newChanges []abci.ValidatorUpdate) []abci
 	return out
 }
 
-func GetChangePubKeyAddress(change abci.ValidatorUpdate) (addr []byte) {
-	pk, err := cryptocodec.FromTmProtoPublicKey(change.PubKey)
-	if err != nil {
-		panic(err)
-	}
-
-	return pk.Address()
-}
-
 // TMCryptoPublicKeyToConsAddr converts a TM public key to an SDK public key
 // and returns the associated consensus address
 func TMCryptoPublicKeyToConsAddr(k tmprotocrypto.PublicKey) (sdk.ConsAddress, error) {