chore: making changes based on nits from 02-client refactor audit (#1585

)
cosmos · Jun 30, 2022 · aab9ca2 · aab9ca2
1 parent e2bdd1f
commit aab9ca2
Show file tree

Hide file tree

Showing 3 changed files with 66 additions and 78 deletions.
diff --git a/modules/core/02-client/keeper/client.go b/modules/core/02-client/keeper/client.go
@@ -1,8 +1,6 @@
 package keeper
 
 import (
-	"encoding/hex"
-
 	"github.com/armon/go-metrics"
 	"github.com/cosmos/cosmos-sdk/telemetry"
 	sdk "github.com/cosmos/cosmos-sdk/types"
@@ -27,7 +25,6 @@ func (k Keeper) CreateClient(
 
 	clientID := k.GenerateClientIdentifier(ctx, clientState.ClientType())
 
-	k.SetClientState(ctx, clientID, clientState)
 	k.Logger(ctx).Info("client created at height", "client-id", clientID, "height", clientState.GetLatestHeight().String())
 
 	// verifies initial consensus state against client state and initializes client store with any client-specific metadata
@@ -36,17 +33,16 @@ func (k Keeper) CreateClient(
 		return "", err
 	}
 
+	k.SetClientState(ctx, clientID, clientState)
 	k.SetClientConsensusState(ctx, clientID, clientState.GetLatestHeight(), consensusState)
 
 	k.Logger(ctx).Info("client created at height", "client-id", clientID, "height", clientState.GetLatestHeight().String())
 
-	defer func() {
-		telemetry.IncrCounterWithLabels(
-			[]string{"ibc", "client", "create"},
-			1,
-			[]metrics.Label{telemetry.NewLabel(types.LabelClientType, clientState.ClientType())},
-		)
-	}()
+	defer telemetry.IncrCounterWithLabels(
+		[]string{"ibc", "client", "create"},
+		1,
+		[]metrics.Label{telemetry.NewLabel(types.LabelClientType, clientState.ClientType())},
+	)
 
 	EmitCreateClientEvent(ctx, clientID, clientState)
 
@@ -76,17 +72,15 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, clientMsg exporte
 
 		k.Logger(ctx).Info("client frozen due to misbehaviour", "client-id", clientID)
 
-		defer func() {
-			telemetry.IncrCounterWithLabels(
-				[]string{"ibc", "client", "misbehaviour"},
-				1,
-				[]metrics.Label{
-					telemetry.NewLabel(types.LabelClientType, clientState.ClientType()),
-					telemetry.NewLabel(types.LabelClientID, clientID),
-					telemetry.NewLabel(types.LabelMsgType, "update"),
-				},
-			)
-		}()
+		defer telemetry.IncrCounterWithLabels(
+			[]string{"ibc", "client", "misbehaviour"},
+			1,
+			[]metrics.Label{
+				telemetry.NewLabel(types.LabelClientType, clientState.ClientType()),
+				telemetry.NewLabel(types.LabelClientID, clientID),
+				telemetry.NewLabel(types.LabelMsgType, "update"),
+			},
+		)
 
 		EmitSubmitMisbehaviourEvent(ctx, clientID, clientState)
 
@@ -97,25 +91,18 @@ func (k Keeper) UpdateClient(ctx sdk.Context, clientID string, clientMsg exporte
 
 	k.Logger(ctx).Info("client state updated", "client-id", clientID, "heights", consensusHeights)
 
-	defer func() {
-		telemetry.IncrCounterWithLabels(
-			[]string{"ibc", "client", "update"},
-			1,
-			[]metrics.Label{
-				telemetry.NewLabel(types.LabelClientType, clientState.ClientType()),
-				telemetry.NewLabel(types.LabelClientID, clientID),
-				telemetry.NewLabel(types.LabelUpdateType, "msg"),
-			},
-		)
-	}()
-
-	// Marshal the ClientMessage as an Any and encode the resulting bytes to hex.
-	// This prevents the event value from containing invalid UTF-8 characters
-	// which may cause data to be lost when JSON encoding/decoding.
-	clientMsgStr := hex.EncodeToString(types.MustMarshalClientMessage(k.cdc, clientMsg))
+	defer telemetry.IncrCounterWithLabels(
+		[]string{"ibc", "client", "update"},
+		1,
+		[]metrics.Label{
+			telemetry.NewLabel(types.LabelClientType, clientState.ClientType()),
+			telemetry.NewLabel(types.LabelClientID, clientID),
+			telemetry.NewLabel(types.LabelUpdateType, "msg"),
+		},
+	)
 
 	// emitting events in the keeper emits for both begin block and handler client updates
-	EmitUpdateClientEvent(ctx, clientID, clientState.ClientType(), consensusHeights, clientMsgStr)
+	EmitUpdateClientEvent(ctx, clientID, clientState.ClientType(), consensusHeights, k.cdc, clientMsg)
 
 	return nil
 }
@@ -143,16 +130,14 @@ func (k Keeper) UpgradeClient(ctx sdk.Context, clientID string, upgradedClient e
 
 	k.Logger(ctx).Info("client state upgraded", "client-id", clientID, "height", upgradedClient.GetLatestHeight().String())
 
-	defer func() {
-		telemetry.IncrCounterWithLabels(
-			[]string{"ibc", "client", "upgrade"},
-			1,
-			[]metrics.Label{
-				telemetry.NewLabel(types.LabelClientType, upgradedClient.ClientType()),
-				telemetry.NewLabel(types.LabelClientID, clientID),
-			},
-		)
-	}()
+	defer telemetry.IncrCounterWithLabels(
+		[]string{"ibc", "client", "upgrade"},
+		1,
+		[]metrics.Label{
+			telemetry.NewLabel(types.LabelClientType, upgradedClient.ClientType()),
+			telemetry.NewLabel(types.LabelClientID, clientID),
+		},
+	)
 
 	EmitUpgradeClientEvent(ctx, clientID, upgradedClient)
 

diff --git a/modules/core/02-client/keeper/events.go b/modules/core/02-client/keeper/events.go
@@ -1,8 +1,10 @@
 package keeper
 
 import (
+	"encoding/hex"
 	"strings"
 
+	"github.com/cosmos/cosmos-sdk/codec"
 	sdk "github.com/cosmos/cosmos-sdk/types"
 
 	"github.com/cosmos/ibc-go/v3/modules/core/02-client/types"
@@ -26,7 +28,13 @@ func EmitCreateClientEvent(ctx sdk.Context, clientID string, clientState exporte
 }
 
 // EmitUpdateClientEvent emits an update client event
-func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientType string, consensusHeights []exported.Height, clientMsgStr string) {
+func EmitUpdateClientEvent(ctx sdk.Context, clientID string, clientType string, consensusHeights []exported.Height, cdc codec.BinaryCodec, clientMsg exported.ClientMessage) {
+
+	// Marshal the ClientMessage as an Any and encode the resulting bytes to hex.
+	// This prevents the event value from containing invalid UTF-8 characters
+	// which may cause data to be lost when JSON encoding/decoding.
+	clientMsgStr := hex.EncodeToString(types.MustMarshalClientMessage(cdc, clientMsg))
+
 	var consensusHeightAttr string
 	if len(consensusHeights) != 0 {
 		consensusHeightAttr = consensusHeights[0].String()

diff --git a/modules/light-clients/07-tendermint/types/update.go b/modules/light-clients/07-tendermint/types/update.go
@@ -17,26 +17,6 @@ import (
 	"github.com/cosmos/ibc-go/v3/modules/core/exported"
 )
 
-// checkTrustedHeader checks that consensus state matches trusted fields of Header
-func checkTrustedHeader(header *Header, consState *ConsensusState) error {
-	tmTrustedValidators, err := tmtypes.ValidatorSetFromProto(header.TrustedValidators)
-	if err != nil {
-		return sdkerrors.Wrap(err, "trusted validator set in not tendermint validator set type")
-	}
-
-	// assert that trustedVals is NextValidators of last trusted header
-	// to do this, we check that trustedVals.Hash() == consState.NextValidatorsHash
-	tvalHash := tmTrustedValidators.Hash()
-	if !bytes.Equal(consState.NextValidatorsHash, tvalHash) {
-		return sdkerrors.Wrapf(
-			ErrInvalidValidatorSet,
-			"trusted validators %s, does not hash to latest trusted validators. Expected: %X, got: %X",
-			header.TrustedValidators, consState.NextValidatorsHash, tvalHash,
-		)
-	}
-	return nil
-}
-
 // VerifyClientMessage checks if the clientMessage is of type Header or Misbehaviour and verifies the message
 func (cs *ClientState) VerifyClientMessage(
 	ctx sdk.Context, cdc codec.BinaryCodec, clientStore sdk.KVStore,
@@ -190,15 +170,13 @@ func (cs ClientState) pruneOldestConsensusState(ctx sdk.Context, cdc codec.Binar
 	// so that we can delete consensus state and all associated metadata.
 	var (
 		pruneHeight exported.Height
-		pruneError  error
 	)
 
 	pruneCb := func(height exported.Height) bool {
 		consState, found := GetConsensusState(clientStore, cdc, height)
 		// this error should never occur
 		if !found {
-			pruneError = sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "failed to retrieve consensus state at height: %s", height)
-			return true
+			panic(sdkerrors.Wrapf(clienttypes.ErrConsensusStateNotFound, "failed to retrieve consensus state at height: %s", height))
 		}
 
 		if cs.IsExpired(consState.Timestamp, ctx.BlockTime()) {
@@ -209,9 +187,6 @@ func (cs ClientState) pruneOldestConsensusState(ctx sdk.Context, cdc codec.Binar
 	}
 
 	IterateConsensusStateAscending(clientStore, pruneCb)
-	if pruneError != nil {
-		panic(pruneError)
-	}
 
 	// if pruneHeight is set, delete consensus state and metadata
 	if pruneHeight != nil {
@@ -230,11 +205,11 @@ func (cs ClientState) CheckForMisbehaviour(ctx sdk.Context, cdc codec.BinaryCode
 		// Check if the Client store already has a consensus state for the header's height
 		// If the consensus state exists, and it matches the header then we return early
 		// since header has already been submitted in a previous UpdateClient.
-		prevConsState, _ := GetConsensusState(clientStore, cdc, tmHeader.GetHeight())
-		if prevConsState != nil {
+		existingConsState, _ := GetConsensusState(clientStore, cdc, tmHeader.GetHeight())
+		if existingConsState != nil {
 			// This header has already been submitted and the necessary state is already stored
 			// in client store, thus we can return early without further validation.
-			if reflect.DeepEqual(prevConsState, tmHeader.ConsensusState()) {
+			if reflect.DeepEqual(existingConsState, tmHeader.ConsensusState()) {
 				return false
 			}
 
@@ -272,3 +247,23 @@ func (cs ClientState) UpdateStateOnMisbehaviour(ctx sdk.Context, cdc codec.Binar
 
 	clientStore.Set(host.ClientStateKey(), clienttypes.MustMarshalClientState(cdc, &cs))
 }
+
+// checkTrustedHeader checks that consensus state matches trusted fields of Header
+func checkTrustedHeader(header *Header, consState *ConsensusState) error {
+	tmTrustedValidators, err := tmtypes.ValidatorSetFromProto(header.TrustedValidators)
+	if err != nil {
+		return sdkerrors.Wrap(err, "trusted validator set in not tendermint validator set type")
+	}
+
+	// assert that trustedVals is NextValidators of last trusted header
+	// to do this, we check that trustedVals.Hash() == consState.NextValidatorsHash
+	tvalHash := tmTrustedValidators.Hash()
+	if !bytes.Equal(consState.NextValidatorsHash, tvalHash) {
+		return sdkerrors.Wrapf(
+			ErrInvalidValidatorSet,
+			"trusted validators %s, does not hash to latest trusted validators. Expected: %X, got: %X",
+			header.TrustedValidators, consState.NextValidatorsHash, tvalHash,
+		)
+	}
+	return nil
+}