fix(vtransfer): store in-flight packet data (#10921)

Closes: IOU an issue



## Description

While improving the vtransfer test coverage for multiple packet-forward-middleware hops, I made its implementation more robust to transformations of packet data through the transfer stack and across hops.

### Security Considerations

Reduces sensitivity to packet data being changed by middleware.

### Scaling Considerations

Additional (though not large) storage is needed for data of certain in-flight packets.

### Documentation Considerations

No user-level changes.

### Testing Considerations

It would be good to add a test showing that the newly added `x/vstorage` `originalData` storage tree does not contain entries after packets on all the relevant chains have been acknowledged or timed out.

### Upgrade Considerations

It might be desirable to add support for `originalData` to `ExportGenesis` and `ImportGenesis`.

golang/cosmos/types/address_hooks.go

@@ -14,11 +14,15 @@ import (
 )
 
 type AddressRole string
+type PacketOrigin string
 
 const (
 	RoleSender   AddressRole = "Sender"
 	RoleReceiver AddressRole = "Receiver"
 
+	PacketSrc PacketOrigin = "src"
+	PacketDst PacketOrigin = "dst"
+
 	AddressHookVersion     = 0
 	BaseAddressLengthBytes = 2
 )
@@ -173,17 +177,12 @@ func extractBaseTransferData(transferData transfertypes.FungibleTokenPacketData,
 // If newPacket is not nil, it is populated with a new transfer packet whose
 // corresponding Sender or Receiver is replaced with the extracted base address.
 func ExtractBaseAddressFromPacket(cdc codec.Codec, packet ibcexported.PacketI, role AddressRole, newPacket *channeltypes.Packet) (string, error) {
-	transferData := transfertypes.FungibleTokenPacketData{}
-	if err := cdc.UnmarshalJSON(packet.GetData(), &transferData); err != nil {
-		return "", err
-	}
-
-	var newTransferData *transfertypes.FungibleTokenPacketData
+	var newDataP *[]byte
 	if newPacket != nil {
-		// Capture the transfer data for the new packet.
-		newTransferData = &transfertypes.FungibleTokenPacketData{}
+		// Capture the data for the new packet.
+		newDataP = new([]byte)
 	}
-	target, err := extractBaseTransferData(transferData, role, newTransferData)
+	target, err := ExtractBaseAddressFromData(cdc, packet.GetData(), role, newDataP)
 	if err != nil {
 		return target, err
 	}
@@ -193,21 +192,51 @@ func ExtractBaseAddressFromPacket(cdc codec.Codec, packet ibcexported.PacketI, r
 	}
 
 	// Create a new packet with the new transfer packet data.
-	// Re-serialize the packet data with the base addresses.
-	newData, err := cdc.MarshalJSON(newTransferData)
-	if err != nil {
-		return target, err
-	}
-
-	// Create the new packet.
-	th := packet.GetTimeoutHeight()
 	*newPacket = channeltypes.NewPacket(
-		newData, packet.GetSequence(),
+		*newDataP, packet.GetSequence(),
 		packet.GetSourcePort(), packet.GetSourceChannel(),
 		packet.GetDestPort(), packet.GetDestChannel(),
-		clienttypes.NewHeight(th.GetRevisionNumber(), th.GetRevisionHeight()),
+		clienttypes.MustParseHeight(packet.GetTimeoutHeight().String()),
 		packet.GetTimeoutTimestamp(),
 	)
 
 	return target, nil
 }
+
+// ExtractBaseAddressFromData returns the base address from a transfer packet's data,
+// either Sender (if role is RoleSender) or Receiver (if role is RoleReceiver).
+// Errors in determining the base address are ignored... we then assume the base
+// address is exactly the original address.
+// If newDataP is not nil, it is populated with new transfer packet data whose
+// corresponding Sender or Receiver is replaced with the extracted base address.
+func ExtractBaseAddressFromData(cdc codec.Codec, data []byte, role AddressRole, newDataP *[]byte) (string, error) {
+	transferData := transfertypes.FungibleTokenPacketData{}
+
+	if err := cdc.UnmarshalJSON(data, &transferData); err != nil {
+		return "", err
+	}
+
+	var newTransferData *transfertypes.FungibleTokenPacketData
+	if newDataP != nil {
+		// Capture the transfer data for the new packet data.
+		newTransferData = &transfertypes.FungibleTokenPacketData{}
+	}
+	target, err := extractBaseTransferData(transferData, role, newTransferData)
+	if err != nil {
+		return target, err
+	}
+
+	if newDataP == nil {
+		return target, nil
+	}
+
+	// Reuse the original data if we didn't transform it.
+	if transferData == *newTransferData {
+		*newDataP = bytes.Clone(data)
+	} else {
+		// Re-serialize the packet data with the new base address.
+		*newDataP = newTransferData.GetBytes()
+	}
+
+	return target, nil
+}

golang/cosmos/types/address_hooks_test.go

@@ -171,8 +171,7 @@ func TestExtractBaseAddressFromPacket(t *testing.T) {
 		tc := tc
 		t.Run(tc.name, func(t *testing.T) {
 			ftPacketData := transfertypes.NewFungibleTokenPacketData("denom", "100", tc.addrs[types.RoleSender].addr, tc.addrs[types.RoleReceiver].addr, "my-favourite-memo")
-			packetBz, err := cdc.MarshalJSON(&ftPacketData)
-			require.NoError(t, err)
+			packetBz := ftPacketData.GetBytes()
 			packet := channeltypes.NewPacket(packetBz, 1234, "my-port", "my-channel", "their-port", "their-channel", clienttypes.NewHeight(133, 445), 10999)
 
 			for role, addrs := range tc.addrs {
@@ -184,9 +183,13 @@ func TestExtractBaseAddressFromPacket(t *testing.T) {
 					require.NoError(t, err)
 					require.Equal(t, addrs.baseAddr, baseAddr)
 
-					packetBaseAddr, err := types.ExtractBaseAddressFromPacket(cdc, packet, role, nil)
+					packetBaseAddr0, err := types.ExtractBaseAddressFromData(cdc, packet.GetData(), role, nil)
 					require.NoError(t, err)
-					require.Equal(t, addrs.baseAddr, packetBaseAddr)
+					require.Equal(t, addrs.baseAddr, packetBaseAddr0)
+
+					packetBaseAddr1, err := types.ExtractBaseAddressFromPacket(cdc, packet, role, nil)
+					require.NoError(t, err)
+					require.Equal(t, addrs.baseAddr, packetBaseAddr1)
 
 					var newPacket channeltypes.Packet
 					packetBaseAddr2, err := types.ExtractBaseAddressFromPacket(cdc, packet, role, &newPacket)

golang/cosmos/x/swingset/testing/queue.go

@@ -15,3 +15,11 @@ func GetActionQueueRecords(t *testing.T, ctx sdk.Context, swingsetKeeper keeper.
 	actionQueueName := keeper.StoragePathActionQueue
 	return vstoragetesting.GetQueueItems(ctx, vstorageKeeper, actionQueueName)
 }
+
+// ResetActionQueue resets the action queue.
+// This is a testing utility function.
+func ResetActionQueue(t *testing.T, ctx sdk.Context, swingsetKeeper keeper.Keeper) error {
+	vstorageKeeper := keeper.GetVstorageKeeper(t, swingsetKeeper)
+	actionQueueName := keeper.StoragePathActionQueue
+	return vstoragetesting.ResetQueue(ctx, vstorageKeeper, actionQueueName)
+}

golang/cosmos/x/vibc/keeper/keeper.go

@@ -113,10 +113,7 @@ func (k Keeper) ReceiveChanOpenInit(ctx sdk.Context, order channeltypes.Order, c
 func (k Keeper) ReceiveSendPacket(ctx sdk.Context, packet ibcexported.PacketI) (uint64, error) {
 	sourcePort := packet.GetSourcePort()
 	sourceChannel := packet.GetSourceChannel()
-	timeoutHeight := packet.GetTimeoutHeight()
-	timeoutRevisionNumber := timeoutHeight.GetRevisionNumber()
-	timeoutRevisionHeight := timeoutHeight.GetRevisionHeight()
-	clientTimeoutHeight := clienttypes.NewHeight(timeoutRevisionNumber, timeoutRevisionHeight)
+	timeoutHeight := clienttypes.MustParseHeight(packet.GetTimeoutHeight().String())
 	timeoutTimestamp := packet.GetTimeoutTimestamp()
 	data := packet.GetData()
 
@@ -125,7 +122,7 @@ func (k Keeper) ReceiveSendPacket(ctx sdk.Context, packet ibcexported.PacketI) (
 	if !ok {
 		return 0, sdkioerrors.Wrapf(channeltypes.ErrChannelCapabilityNotFound, "could not retrieve channel capability at: %s", capName)
 	}
-	return k.SendPacket(ctx, chanCap, sourcePort, sourceChannel, clientTimeoutHeight, timeoutTimestamp, data)
+	return k.SendPacket(ctx, chanCap, sourcePort, sourceChannel, timeoutHeight, timeoutTimestamp, data)
 }
 
 // SendPacket defines a wrapper function for the channel Keeper's function

golang/cosmos/x/vibc/keeper/triggers.go

@@ -12,19 +12,16 @@ import (
 )
 
 func reifyPacket(packet ibcexported.PacketI) channeltypes.Packet {
-	height := packet.GetTimeoutHeight()
-	ctHeight := clienttypes.Height{
-		RevisionHeight: height.GetRevisionHeight(),
-		RevisionNumber: height.GetRevisionNumber(),
-	}
+
+	timeoutHeight := clienttypes.MustParseHeight(packet.GetTimeoutHeight().String())
 	return channeltypes.Packet{
 		Sequence:           packet.GetSequence(),
 		SourcePort:         packet.GetSourcePort(),
 		SourceChannel:      packet.GetSourceChannel(),
 		DestinationPort:    packet.GetDestPort(),
 		DestinationChannel: packet.GetDestChannel(),
 		Data:               packet.GetData(),
-		TimeoutHeight:      ctHeight,
+		TimeoutHeight:      timeoutHeight,
 		TimeoutTimestamp:   packet.GetTimeoutTimestamp(),
 	}
 }

golang/cosmos/x/vibc/types/receiver.go

@@ -15,7 +15,7 @@ import (
 
 var (
 	_ vm.PortHandler           = (*Receiver)(nil)
-	_ exported.Acknowledgement = (*rawAcknowledgement)(nil)
+	_ exported.Acknowledgement = (*RawAcknowledgement)(nil)
 )
 
 type ReceiverImpl interface {
@@ -71,15 +71,21 @@ func orderToString(order channeltypes.Order) string {
 	}
 }
 
-type rawAcknowledgement struct {
+type RawAcknowledgement struct {
 	data []byte
 }
 
-func (r rawAcknowledgement) Acknowledgement() []byte {
+func NewRawAcknowledgement(data []byte) RawAcknowledgement {
+	return RawAcknowledgement{
+		data: data,
+	}
+}
+
+func (r RawAcknowledgement) Acknowledgement() []byte {
 	return r.data
 }
 
-func (r rawAcknowledgement) Success() bool {
+func (r RawAcknowledgement) Success() bool {
 	return true
 }
 
@@ -137,7 +143,7 @@ func (ir Receiver) Receive(cctx context.Context, jsonRequest string) (jsonReply
 		)
 
 	case "receiveExecuted":
-		ack := rawAcknowledgement{
+		ack := RawAcknowledgement{
 			data: msg.Ack,
 		}
 		err = impl.ReceiveWriteAcknowledgement(ctx, msg.Packet, ack)

golang/cosmos/x/vstorage/testing/queue.go

@@ -26,3 +26,9 @@ func GetQueueItems(ctx sdk.Context, vstorageKeeper keeper.Keeper, queuePath stri
 	}
 	return values, nil
 }
+
+func ResetQueue(ctx sdk.Context, vstorageKeeper keeper.Keeper, queuePath string) error {
+	unlimitedCtx := ctx.WithGasMeter(sdk.NewInfiniteGasMeter())
+	vstorageKeeper.RemoveEntriesWithPrefix(unlimitedCtx, queuePath)
+	return nil
+}

golang/cosmos/x/vtransfer/ibc_middleware.go

@@ -160,7 +160,11 @@ func (im IBCMiddleware) WriteAcknowledgement(
 	packet exported.PacketI,
 	ack exported.Acknowledgement,
 ) error {
-	return im.vtransferKeeper.InterceptWriteAcknowledgement(ctx, chanCap, packet, ack)
+	syncAck, origPacket := im.vtransferKeeper.InterceptWriteAcknowledgement(ctx, chanCap, packet, ack)
+	if syncAck != nil {
+		return im.vtransferKeeper.WriteAcknowledgement(ctx, chanCap, origPacket, syncAck)
+	}
+	return nil
 }
 
 ///////////////////////////////////