ibc: async acknowledgements (#7361)

* rename packet ack abs

* update packet executed

* write ack

* update clients

* update transfer keeper

* changes from reviews

* rename ReceiveExecuted -> WriteReceipt

* tests

* fix tests

* Apply suggestions from code review

Co-authored-by: colin axnér <[email protected]>

* Update x/ibc/07-tendermint/types/client_state_test.go

Co-authored-by: Aditya <[email protected]>

* comments from review

* update RecvPacket

* spec typo

* test fixes

Co-authored-by: colin axnér <[email protected]>
Co-authored-by: Aditya <[email protected]>
Co-authored-by: mergify[bot] <37929162+mergify[bot]@users.noreply.github.com>

proto/ibc/lightclients/solomachine/v1/solomachine.proto

@@ -142,9 +142,9 @@ message PacketAcknowledgementData {
   bytes acknowledgement = 2;
 }
 
-// PacketAcknowledgementAbsenceSignBytes returns the SignBytes data for
-// acknowledgement absence verification.
-message PacketAcknowledgementAbsenseData {
+// PacketReceiptAbsenceSignBytes returns the SignBytes data for
+// packet receipt absence verification.
+message PacketReceiptAbsenseData {
   bytes path = 1;
 }
 

x/ibc-transfer/keeper/keeper.go

@@ -16,7 +16,6 @@ import (
 	"github.com/cosmos/cosmos-sdk/x/ibc-transfer/types"
 	channeltypes "github.com/cosmos/cosmos-sdk/x/ibc/04-channel/types"
 	host "github.com/cosmos/cosmos-sdk/x/ibc/24-host"
-	ibcexported "github.com/cosmos/cosmos-sdk/x/ibc/exported"
 	paramtypes "github.com/cosmos/cosmos-sdk/x/params/types"
 )
 
@@ -72,19 +71,8 @@ func (k Keeper) GetTransferAccount(ctx sdk.Context) authtypes.ModuleAccountI {
 	return k.authKeeper.GetModuleAccount(ctx, types.ModuleName)
 }
 
-// ReceiveExecuted defines a wrapper function for the channel Keeper's function
-// in order to expose it to the ICS20 transfer handler.
-// Keeper retrieves channel capability and passes it into channel keeper for authentication
-func (k Keeper) ReceiveExecuted(ctx sdk.Context, packet ibcexported.PacketI, acknowledgement []byte) error {
-	chanCap, ok := k.scopedKeeper.GetCapability(ctx, host.ChannelCapabilityPath(packet.GetDestPort(), packet.GetDestChannel()))
-	if !ok {
-		return sdkerrors.Wrap(channeltypes.ErrChannelCapabilityNotFound, "channel capability could not be retrieved for packet")
-	}
-	return k.channelKeeper.ReceiveExecuted(ctx, chanCap, packet, acknowledgement)
-}
-
 // ChanCloseInit defines a wrapper function for the channel Keeper's function
-// in order to expose it to the ICS20 trasfer handler.
+// in order to expose it to the ICS20 transfer handler.
 func (k Keeper) ChanCloseInit(ctx sdk.Context, portID, channelID string) error {
 	capName := host.ChannelCapabilityPath(portID, channelID)
 	chanCap, ok := k.scopedKeeper.GetCapability(ctx, capName)

x/ibc-transfer/module.go

@@ -323,6 +323,7 @@ func (am AppModule) OnRecvPacket(
 		),
 	)
 
+	// NOTE: acknowledgement will be written synchronously during IBC handler execution.
 	return &sdk.Result{
 		Events: ctx.EventManager().Events().ToABCIEvents(),
 	}, acknowledgement.GetBytes(), nil

x/ibc-transfer/types/expected_keepers.go

@@ -29,7 +29,6 @@ type ChannelKeeper interface {
 	GetChannel(ctx sdk.Context, srcPort, srcChan string) (channel channeltypes.Channel, found bool)
 	GetNextSequenceSend(ctx sdk.Context, portID, channelID string) (uint64, bool)
 	SendPacket(ctx sdk.Context, channelCap *capabilitytypes.Capability, packet ibcexported.PacketI) error
-	ReceiveExecuted(ctx sdk.Context, chanCap *capabilitytypes.Capability, packet ibcexported.PacketI, acknowledgement []byte) error
 	ChanCloseInit(ctx sdk.Context, portID, channelID string, chanCap *capabilitytypes.Capability) error
 }
 

x/ibc/02-client/types/errors.go

@@ -23,7 +23,7 @@ var (
 	ErrFailedChannelStateVerification         = sdkerrors.Register(SubModuleName, 16, "channel state verification failed")
 	ErrFailedPacketCommitmentVerification     = sdkerrors.Register(SubModuleName, 17, "packet commitment verification failed")
 	ErrFailedPacketAckVerification            = sdkerrors.Register(SubModuleName, 18, "packet acknowledgement verification failed")
-	ErrFailedPacketAckAbsenceVerification     = sdkerrors.Register(SubModuleName, 19, "packet acknowledgement absence verification failed")
+	ErrFailedPacketReceiptVerification        = sdkerrors.Register(SubModuleName, 19, "packet receipt verification failed")
 	ErrFailedNextSeqRecvVerification          = sdkerrors.Register(SubModuleName, 20, "next sequence receive verification failed")
 	ErrSelfConsensusStateNotFound             = sdkerrors.Register(SubModuleName, 21, "self consensus state not found")
 	ErrUpdateClientFailed                     = sdkerrors.Register(SubModuleName, 22, "unable to update light client")

x/ibc/03-connection/keeper/verify.go

@@ -175,10 +175,10 @@ func (k Keeper) VerifyPacketAcknowledgement(
 	return nil
 }
 
-// VerifyPacketAcknowledgementAbsence verifies a proof of the absence of an
-// incoming packet acknowledgement at the specified port, specified channel, and
+// VerifyPacketReceiptAbsence verifies a proof of the absence of an
+// incoming packet receipt at the specified port, specified channel, and
 // specified sequence.
-func (k Keeper) VerifyPacketAcknowledgementAbsence(
+func (k Keeper) VerifyPacketReceiptAbsence(
 	ctx sdk.Context,
 	connection exported.ConnectionI,
 	height exported.Height,
@@ -192,12 +192,12 @@ func (k Keeper) VerifyPacketAcknowledgementAbsence(
 		return sdkerrors.Wrap(clienttypes.ErrClientNotFound, connection.GetClientID())
 	}
 
-	if err := clientState.VerifyPacketAcknowledgementAbsence(
+	if err := clientState.VerifyPacketReceiptAbsence(
 		k.clientKeeper.ClientStore(ctx, connection.GetClientID()), k.cdc, height,
 		connection.GetCounterparty().GetPrefix(), proof, portID, channelID,
 		sequence,
 	); err != nil {
-		return sdkerrors.Wrapf(err, "failed packet acknowledgement absence verification for client (%s)", connection.GetClientID())
+		return sdkerrors.Wrapf(err, "failed packet receipt absence verification for client (%s)", connection.GetClientID())
 	}
 
 	return nil

x/ibc/03-connection/keeper/verify_test.go

@@ -336,7 +336,10 @@ func (suite *KeeperTestSuite) TestVerifyPacketAcknowledgement() {
 			err := suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB)
 			suite.Require().NoError(err)
 
-			err = suite.coordinator.ReceiveExecuted(suite.chainB, suite.chainA, packet, clientA)
+			err = suite.coordinator.WriteReceipt(suite.chainB, suite.chainA, packet, clientA)
+			suite.Require().NoError(err)
+
+			err = suite.coordinator.WriteAcknowledgement(suite.chainB, suite.chainA, packet, clientA)
 			suite.Require().NoError(err)
 
 			packetAckKey := host.KeyPacketAcknowledgement(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
@@ -361,10 +364,10 @@ func (suite *KeeperTestSuite) TestVerifyPacketAcknowledgement() {
 	}
 }
 
-// TestVerifyPacketAcknowledgementAbsence has chainA verify the acknowledgement
+// TestVerifyPacketReceiptAbsence has chainA verify the receipt
 // absence on channelB. The channels on chainA and chainB are fully opened and
 // a packet is sent from chainA to chainB and not received.
-func (suite *KeeperTestSuite) TestVerifyPacketAcknowledgementAbsence() {
+func (suite *KeeperTestSuite) TestVerifyPacketReceiptAbsence() {
 	cases := []struct {
 		msg            string
 		changeClientID bool
@@ -396,18 +399,18 @@ func (suite *KeeperTestSuite) TestVerifyPacketAcknowledgementAbsence() {
 			suite.Require().NoError(err)
 
 			if tc.recvAck {
-				err = suite.coordinator.ReceiveExecuted(suite.chainB, suite.chainA, packet, clientA)
+				err = suite.coordinator.WriteReceipt(suite.chainB, suite.chainA, packet, clientA)
 				suite.Require().NoError(err)
 			} else {
 				// need to update height to prove absence
 				suite.coordinator.CommitBlock(suite.chainA, suite.chainB)
 				suite.coordinator.UpdateClient(suite.chainA, suite.chainB, clientA, ibctesting.Tendermint)
 			}
 
-			packetAckKey := host.KeyPacketAcknowledgement(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
-			proof, proofHeight := suite.chainB.QueryProof(packetAckKey)
+			packetReceiptKey := host.KeyPacketReceipt(packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
+			proof, proofHeight := suite.chainB.QueryProof(packetReceiptKey)
 
-			err = suite.chainA.App.IBCKeeper.ConnectionKeeper.VerifyPacketAcknowledgementAbsence(
+			err = suite.chainA.App.IBCKeeper.ConnectionKeeper.VerifyPacketReceiptAbsence(
 				suite.chainA.GetContext(), connection, malleateHeight(proofHeight, tc.heightDiff), proof,
 				packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
 			)
@@ -455,7 +458,7 @@ func (suite *KeeperTestSuite) TestVerifyNextSequenceRecv() {
 			err := suite.coordinator.SendPacket(suite.chainA, suite.chainB, packet, clientB)
 			suite.Require().NoError(err)
 
-			err = suite.coordinator.ReceiveExecuted(suite.chainB, suite.chainA, packet, clientA)
+			err = suite.coordinator.WriteReceipt(suite.chainB, suite.chainA, packet, clientA)
 			suite.Require().NoError(err)
 
 			nextSeqRecvKey := host.KeyNextSequenceRecv(packet.GetDestPort(), packet.GetDestChannel())

x/ibc/04-channel/keeper/keeper.go

@@ -125,6 +125,23 @@ func (k Keeper) SetNextSequenceAck(ctx sdk.Context, portID, channelID string, se
 	store.Set(host.KeyNextSequenceAck(portID, channelID), bz)
 }
 
+// GetPacketReceipt gets a packet receipt from the store
+func (k Keeper) GetPacketReceipt(ctx sdk.Context, portID, channelID string, sequence uint64) (string, bool) {
+	store := ctx.KVStore(k.storeKey)
+	bz := store.Get(host.KeyPacketReceipt(portID, channelID, sequence))
+	if bz == nil {
+		return "", false
+	}
+
+	return string(bz), true
+}
+
+// SetPacketReceipt sets an empty packet receipt to the store
+func (k Keeper) SetPacketReceipt(ctx sdk.Context, portID, channelID string, sequence uint64) {
+	store := ctx.KVStore(k.storeKey)
+	store.Set(host.KeyPacketReceipt(portID, channelID, sequence), []byte(""))
+}
+
 // GetPacketCommitment gets the packet commitment hash from the store
 func (k Keeper) GetPacketCommitment(ctx sdk.Context, portID, channelID string, sequence uint64) []byte {
 	store := ctx.KVStore(k.storeKey)
@@ -165,6 +182,12 @@ func (k Keeper) GetPacketAcknowledgement(ctx sdk.Context, portID, channelID stri
 	return bz, true
 }
 
+// HasPacketAcknowledgement check if the packet ack hash is already on the store
+func (k Keeper) HasPacketAcknowledgement(ctx sdk.Context, portID, channelID string, sequence uint64) bool {
+	store := ctx.KVStore(k.storeKey)
+	return store.Has(host.KeyPacketAcknowledgement(portID, channelID, sequence))
+}
+
 // IteratePacketSequence provides an iterator over all send, receive or ack sequences.
 // For each sequence, cb will be called. If the cb returns true, the iterator
 // will close and stop.

x/ibc/04-channel/keeper/keeper_test.go

@@ -291,13 +291,14 @@ func (suite *KeeperTestSuite) TestSetPacketAcknowledgement() {
 	seq := uint64(10)
 
 	storedAckHash, found := suite.chainA.App.IBCKeeper.ChannelKeeper.GetPacketAcknowledgement(ctxA, channelA.PortID, channelA.ID, seq)
-	suite.False(found)
-	suite.Nil(storedAckHash)
+	suite.Require().False(found)
+	suite.Require().Nil(storedAckHash)
 
 	ackHash := []byte("ackhash")
 	suite.chainA.App.IBCKeeper.ChannelKeeper.SetPacketAcknowledgement(ctxA, channelA.PortID, channelA.ID, seq, ackHash)
 
 	storedAckHash, found = suite.chainA.App.IBCKeeper.ChannelKeeper.GetPacketAcknowledgement(ctxA, channelA.PortID, channelA.ID, seq)
-	suite.True(found)
-	suite.Equal(ackHash, storedAckHash)
+	suite.Require().True(found)
+	suite.Require().Equal(ackHash, storedAckHash)
+	suite.Require().True(suite.chainA.App.IBCKeeper.ChannelKeeper.HasPacketAcknowledgement(ctxA, channelA.PortID, channelA.ID, seq))
 }

x/ibc/04-channel/keeper/packet.go

@@ -201,19 +201,19 @@ func (k Keeper) RecvPacket(
 		)
 	}
 
-	// check if the packet acknowledgement has been received already for unordered channels
-	if channel.Ordering == types.UNORDERED {
-		_, found := k.GetPacketAcknowledgement(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
+	switch channel.Ordering {
+	case types.UNORDERED:
+		// check if the packet receipt has been received already for unordered channels
+		_, found := k.GetPacketReceipt(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
 		if found {
 			return sdkerrors.Wrapf(
 				types.ErrInvalidPacket,
 				"packet sequence (%d) already has been received", packet.GetSequence(),
 			)
 		}
-	}
 
-	// check if the packet is being received in order
-	if channel.Ordering == types.ORDERED {
+	case types.ORDERED:
+		// check if the packet is being received in order
 		nextSequenceRecv, found := k.GetNextSequenceRecv(ctx, packet.GetDestPort(), packet.GetDestChannel())
 		if !found {
 			return sdkerrors.Wrapf(
@@ -238,19 +238,18 @@ func (k Keeper) RecvPacket(
 		return sdkerrors.Wrap(err, "couldn't verify counterparty packet commitment")
 	}
 
-	// NOTE: the remaining code is located in the ReceiveExecuted function
+	// NOTE: the remaining code is located in the WriteReceipt function
 	return nil
 }
 
-// ReceiveExecuted writes the packet execution acknowledgement to the state,
-// which will be verified by the counterparty chain using AcknowledgePacket.
+// WriteReceipt updates the receive sequence in the case of an ordered channel or sets an empty receipt
+// if the channel is unordered.
 //
 // CONTRACT: this function must be called in the IBC handler
-func (k Keeper) ReceiveExecuted(
+func (k Keeper) WriteReceipt(
 	ctx sdk.Context,
 	chanCap *capabilitytypes.Capability,
 	packet exported.PacketI,
-	acknowledgement []byte,
 ) error {
 	channel, found := k.GetChannel(ctx, packet.GetDestPort(), packet.GetDestChannel())
 	if !found {
@@ -273,17 +272,8 @@ func (k Keeper) ReceiveExecuted(
 		)
 	}
 
-	if len(acknowledgement) == 0 {
-		return sdkerrors.Wrap(types.ErrInvalidAcknowledgement, "acknowledgement cannot be empty")
-	}
-
-	// always set the acknowledgement so that it can be verified on the other side
-	k.SetPacketAcknowledgement(
-		ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
-		types.CommitAcknowledgement(acknowledgement),
-	)
-
-	if channel.Ordering == types.ORDERED {
+	switch channel.Ordering {
+	case types.ORDERED:
 		nextSequenceRecv, found := k.GetNextSequenceRecv(ctx, packet.GetDestPort(), packet.GetDestChannel())
 		if !found {
 			return sdkerrors.Wrapf(
@@ -294,20 +284,33 @@ func (k Keeper) ReceiveExecuted(
 
 		nextSequenceRecv++
 
-		// incrementng nextSequenceRecv and storing under this chain's channelEnd identifiers
+		// incrementing nextSequenceRecv and storing under this chain's channelEnd identifiers
 		// Since this is the receiving chain, our channelEnd is packet's destination port and channel
 		k.SetNextSequenceRecv(ctx, packet.GetDestPort(), packet.GetDestChannel(), nextSequenceRecv)
+
+	case types.UNORDERED:
+		// For unordered channels we must set the receipt so it can be verified on the other side.
+		// This receipt does not contain any data, since the packet has not yet been processed,
+		// it's just a single store key set to an empty string to indicate that the packet has been received
+		_, found := k.GetPacketReceipt(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
+		if found {
+			return sdkerrors.Wrapf(
+				types.ErrPacketReceived,
+				"destination port: %s, destination channel: %s, sequence: %d", packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
+			)
+		}
+
+		k.SetPacketReceipt(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence())
 	}
 
 	// log that a packet has been received & executed
-	k.Logger(ctx).Info(fmt.Sprintf("packet received & executed: %v", packet))
+	k.Logger(ctx).Info("packet received", "packet", fmt.Sprintf("%v", packet))
 
 	// emit an event that the relayer can query for
 	ctx.EventManager().EmitEvents(sdk.Events{
 		sdk.NewEvent(
 			types.EventTypeRecvPacket,
 			sdk.NewAttribute(types.AttributeKeyData, string(packet.GetData())),
-			sdk.NewAttribute(types.AttributeKeyAck, string(acknowledgement)),
 			sdk.NewAttribute(types.AttributeKeyTimeoutHeight, packet.GetTimeoutHeight().String()),
 			sdk.NewAttribute(types.AttributeKeyTimeoutTimestamp, fmt.Sprintf("%d", packet.GetTimeoutTimestamp())),
 			sdk.NewAttribute(types.AttributeKeySequence, fmt.Sprintf("%d", packet.GetSequence())),
@@ -326,6 +329,56 @@ func (k Keeper) ReceiveExecuted(
 	return nil
 }
 
+// WriteAcknowledgement writes the packet execution acknowledgement to the state,
+// which will be verified by the counterparty chain using AcknowledgePacket.
+//
+// CONTRACT:
+//
+// 1) For synchronous execution, this function is be called in the IBC handler .
+// For async handling, it needs to be called directly by the module which originally
+// processed the packet.
+//
+// 2) Assumes that packet receipt has been writted previously by WriteReceipt.
+func (k Keeper) WriteAcknowledgement(
+	ctx sdk.Context,
+	packet exported.PacketI,
+	acknowledgement []byte,
+) error {
+	// NOTE: IBC app modules might have written the acknowledgement synchronously on
+	// the OnRecvPacket callback so we need to check if the acknowledgement is already
+	// set on the store and return an error if so.
+	if k.HasPacketAcknowledgement(ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence()) {
+		return types.ErrAcknowledgementExists
+	}
+
+	if len(acknowledgement) == 0 {
+		return sdkerrors.Wrap(types.ErrInvalidAcknowledgement, "acknowledgement cannot be empty")
+	}
+
+	// always set the acknowledgement so that it can be verified on the other side
+	k.SetPacketAcknowledgement(
+		ctx, packet.GetDestPort(), packet.GetDestChannel(), packet.GetSequence(),
+		types.CommitAcknowledgement(acknowledgement),
+	)
+
+	// log that a packet has been acknowledged
+	k.Logger(ctx).Info("packet acknowledged", "packet", fmt.Sprintf("%v", packet))
+
+	// emit an event that the relayer can query for
+	ctx.EventManager().EmitEvents(sdk.Events{
+		sdk.NewEvent(
+			types.EventTypeRecvPacket,
+			sdk.NewAttribute(types.AttributeKeyAck, string(acknowledgement)),
+		),
+		sdk.NewEvent(
+			sdk.EventTypeMessage,
+			sdk.NewAttribute(sdk.AttributeKeyModule, types.AttributeValueCategory),
+		),
+	})
+
+	return nil
+}
+
 // AcknowledgePacket is called by a module to process the acknowledgement of a
 // packet previously sent by the calling module on a channel to a counterparty
 // module on the counterparty chain. Its intended usage is within the ante
@@ -459,7 +512,7 @@ func (k Keeper) AcknowledgementExecuted(
 
 		nextSequenceAck++
 
-		// incrementng NextSequenceAck and storing under this chain's channelEnd identifiers
+		// incrementing NextSequenceAck and storing under this chain's channelEnd identifiers
 		// Since this is the original sending chain, our channelEnd is packet's source port and channel
 		k.SetNextSequenceAck(ctx, packet.GetSourcePort(), packet.GetSourceChannel(), nextSequenceAck)
 	}