feat(eureka): fix condition in commit packet, added test for zero tim…

…eout height (#7109)

* fix condition in commit packet, added test for zero timeout height, change some error messages and add some more comments

* error for verify functions

modules/core/02-client/keeper/keeper.go

@@ -310,7 +310,7 @@ func (k *Keeper) SetCounterparty(ctx sdk.Context, clientID string, counterparty
 	k.ClientStore(ctx, clientID).Set([]byte(types.CounterpartyKey), bz)
 }
 
-// GetCounterparty gets the counterparty client's identifier for a given client identifier.
+// GetCounterparty gets the Counterparty for a given client identifier.
 func (k *Keeper) GetCounterparty(ctx sdk.Context, clientID string) (types.Counterparty, bool) {
 	store := k.ClientStore(ctx, clientID)
 	bz := store.Get([]byte(types.CounterpartyKey))

modules/core/02-client/types/client.go

@@ -80,7 +80,7 @@ func (c Counterparty) Validate() error {
 	}
 
 	if c.MerklePathPrefix.Empty() {
-		return errorsmod.Wrap(ErrInvalidCounterparty, "counterparty prefix cannot be empty")
+		return errorsmod.Wrap(ErrInvalidCounterparty, "prefix cannot be empty")
 	}
 
 	return nil

modules/core/02-client/types/errors.go

@@ -38,5 +38,5 @@ var (
 	ErrFailedNonMembershipVerification        = errorsmod.Register(SubModuleName, 31, "non-membership verification failed")
 	ErrRouteNotFound                          = errorsmod.Register(SubModuleName, 32, "light client module route not found")
 	ErrClientTypeNotSupported                 = errorsmod.Register(SubModuleName, 33, "client type not supported")
-	ErrInvalidCounterparty                    = errorsmod.Register(SubModuleName, 34, "invalid counterparty identifier")
+	ErrInvalidCounterparty                    = errorsmod.Register(SubModuleName, 34, "invalid counterparty")
 )

modules/core/04-channel/types/packet.go

@@ -17,7 +17,7 @@ import (
 // must commit to all fields in the packet apart from the source port
 // source channel and sequence (which will be committed to in the packet commitment key path)
 // and the ProtocolVersion which is defining how to hash the packet fields.
-// NOTE: The commitment MUSTs be a fixed length preimage to prevent relayers from being able
+// NOTE: The commitment MUST be a fixed length preimage to prevent relayers from being able
 // to malleate the packet fields and create a commitment hash that matches the original packet.
 func CommitPacket(packet Packet) []byte {
 	switch packet.ProtocolVersion {
@@ -67,7 +67,7 @@ func commitV2Packet(packet Packet) []byte {
 	timeoutBuf := sdk.Uint64ToBigEndian(packet.GetTimeoutTimestamp())
 
 	// only hash the timeout height if it is non-zero. This will allow us to remove it entirely in the future
-	if timeoutHeight.EQ(clienttypes.ZeroHeight()) {
+	if !timeoutHeight.EQ(clienttypes.ZeroHeight()) {
 		revisionNumber := sdk.Uint64ToBigEndian(timeoutHeight.GetRevisionNumber())
 		timeoutBuf = append(timeoutBuf, revisionNumber...)
 

modules/core/04-channel/types/packet_test.go

@@ -5,30 +5,41 @@ import (
 
 	"github.com/stretchr/testify/require"
 
+	clienttypes "github.com/cosmos/ibc-go/v9/modules/core/02-client/types"
 	"github.com/cosmos/ibc-go/v9/modules/core/04-channel/types"
 )
 
 func TestCommitPacket(t *testing.T) {
-	packet := types.NewPacket(validPacketData, 1, portid, chanid, cpportid, cpchanid, timeoutHeight, timeoutTimestamp)
-
-	commitment1 := types.CommitPacket(packet)
+	// V1 packet1 commitment
+	packet1 := types.NewPacket(validPacketData, 1, portid, chanid, cpportid, cpchanid, timeoutHeight, timeoutTimestamp)
+	commitment1 := types.CommitPacket(packet1)
 	require.NotNil(t, commitment1)
 
+	// V2 packet commitment with empty app version
 	packet2 := types.NewPacketWithVersion(validPacketData, 1, portid, chanid, cpportid, cpchanid, timeoutHeight, timeoutTimestamp, "")
 	commitment2 := types.CommitPacket(packet2)
 	require.NotNil(t, commitment2)
 
-	// even though versions are same empty string,
-	// the commitment is different because we use
-	// Eureka protocol for packet2
+	// even though app version is empty for both packet1 and packet2
+	// the commitment is different because we use Eureka protocol for packet2
 	require.NotEqual(t, commitment1, commitment2)
 
+	// V2 packet commitment with non-empty app version
 	packet3 := types.NewPacketWithVersion(validPacketData, 1, portid, chanid, cpportid, cpchanid, timeoutHeight, timeoutTimestamp, validVersion)
 	commitment3 := types.CommitPacket(packet3)
 	require.NotNil(t, commitment3)
 
 	require.NotEqual(t, commitment1, commitment3)
 	require.NotEqual(t, commitment2, commitment3)
+
+	// V2 packet commitment with non-empty app version and zero timeout height
+	packet4 := types.NewPacketWithVersion(validPacketData, 1, portid, chanid, cpportid, cpchanid, clienttypes.ZeroHeight(), timeoutTimestamp, validVersion)
+	commitment4 := types.CommitPacket(packet4)
+	require.NotNil(t, commitment4)
+
+	require.NotEqual(t, commitment1, commitment4)
+	require.NotEqual(t, commitment2, commitment4)
+	require.NotEqual(t, commitment3, commitment4)
 }
 
 func TestPacketValidateBasic(t *testing.T) {

modules/core/keeper/msg_server.go

@@ -28,6 +28,8 @@ var (
 )
 
 // CreateClient defines a rpc handler method for MsgCreateClient.
+// It stores the signer of MsgCreateClient as the creator of the client, which is afterwards
+// compared against the signer of MsgProvideCounterparty.
 // NOTE: The raw bytes of the concrete types encoded into protobuf.Any is passed to the client keeper.
 // The 02-client handler will route to the appropriate light client module based on client type and it is the responsibility
 // of the light client module to unmarshal and interpret the proto encoded bytes.
@@ -146,16 +148,17 @@ func (k *Keeper) ProvideCounterparty(goCtx context.Context, msg *clienttypes.Msg
 
 	creator, found := k.ClientKeeper.GetCreator(ctx, msg.ClientId)
 	if !found {
-		return nil, errorsmod.Wrap(ibcerrors.ErrUnauthorized, "expected client creator to have been set")
+		return nil, errorsmod.Wrap(ibcerrors.ErrUnauthorized, "client creator must be set")
 	}
 
 	if creator != msg.Signer {
-		return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "expected client creator %s to match signer %s", creator, msg.Signer)
+		return nil, errorsmod.Wrapf(ibcerrors.ErrUnauthorized, "client creator (%s) must match signer (%s)", creator, msg.Signer)
 	}
 
 	if _, ok := k.ClientKeeper.GetCounterparty(ctx, msg.ClientId); ok {
 		return nil, errorsmod.Wrapf(clienttypes.ErrInvalidCounterparty, "counterparty already exists for client %s", msg.ClientId)
 	}
+
 	k.ClientKeeper.SetCounterparty(ctx, msg.ClientId, msg.Counterparty)
 	// Delete client creator from state as it is not needed after this point.
 	k.ClientKeeper.DeleteCreator(ctx, msg.ClientId)

modules/core/packet-server/keeper/keeper.go

@@ -39,6 +39,7 @@ func (Keeper) Logger(ctx sdk.Context) log.Logger {
 	return ctx.Logger().With("module", "x/"+exported.ModuleName)
 }
 
+// TODO: add godoc
 func (k Keeper) SendPacket(
 	ctx sdk.Context,
 	_ *capabilitytypes.Capability,
@@ -69,15 +70,15 @@ func (k Keeper) SendPacket(
 		destPort, destChannel, timeoutHeight, timeoutTimestamp, version)
 
 	if err := packet.ValidateBasic(); err != nil {
-		return 0, errorsmod.Wrap(err, "constructed packet failed basic validation")
+		return 0, errorsmod.Wrapf(channeltypes.ErrInvalidPacket, "constructed packet failed basic validation: %v", err)
 	}
 
-	// check that the client of receiver chain is still active
+	// check that the client of counterparty chain is still active
 	if status := k.ClientKeeper.GetClientStatus(ctx, sourceChannel); status != exported.Active {
-		return 0, errorsmod.Wrapf(clienttypes.ErrClientNotActive, "client state is not active: %s", status)
+		return 0, errorsmod.Wrapf(clienttypes.ErrClientNotActive, "client (%s) status is %s", sourceChannel, status)
 	}
 
-	// retrieve latest height and timestamp of the client of receiver chain
+	// retrieve latest height and timestamp of the client of counterparty chain
 	latestHeight := k.ClientKeeper.GetClientLatestHeight(ctx, sourceChannel)
 	if latestHeight.IsZero() {
 		return 0, errorsmod.Wrapf(clienttypes.ErrInvalidHeight, "cannot send packet using client (%s) with zero height", sourceChannel)
@@ -109,6 +110,7 @@ func (k Keeper) SendPacket(
 	return sequence, nil
 }
 
+// TODO: add godoc
 func (k Keeper) RecvPacket(
 	ctx sdk.Context,
 	_ *capabilitytypes.Capability,
@@ -166,7 +168,7 @@ func (k Keeper) RecvPacket(
 		merklePath,
 		commitment,
 	); err != nil {
-		return "", err
+		return "", errorsmod.Wrapf(err, "failed packet commitment verification for client (%s)", packet.DestinationChannel)
 	}
 
 	// Set Packet Receipt to prevent timeout from occurring on counterparty
@@ -181,6 +183,7 @@ func (k Keeper) RecvPacket(
 	return packet.AppVersion, nil
 }
 
+// TODO: add godoc
 func (k Keeper) WriteAcknowledgement(
 	ctx sdk.Context,
 	_ *capabilitytypes.Capability,
@@ -236,6 +239,7 @@ func (k Keeper) WriteAcknowledgement(
 	return nil
 }
 
+// TODO: add godoc
 func (k Keeper) AcknowledgePacket(
 	ctx sdk.Context,
 	_ *capabilitytypes.Capability,
@@ -289,7 +293,7 @@ func (k Keeper) AcknowledgePacket(
 		merklePath,
 		channeltypes.CommitAcknowledgement(acknowledgement),
 	); err != nil {
-		return "", err
+		return "", errorsmod.Wrapf(err, "failed packet acknowledgement verification for client (%s)", packet.SourceChannel)
 	}
 
 	k.ChannelKeeper.DeletePacketCommitment(ctx, packet.SourcePort, packet.SourceChannel, packet.Sequence)
@@ -303,6 +307,7 @@ func (k Keeper) AcknowledgePacket(
 	return packet.AppVersion, nil
 }
 
+// TODO: add godoc
 func (k Keeper) TimeoutPacket(
 	ctx sdk.Context,
 	_ *capabilitytypes.Capability,