UnreceivedPackets grpc query handler only works for unordered channels

[plafer]

Summary of Bug

UnreceivedPackets builds the response only by looking for a corresponding packet receipt for every sequence number included in the query (see here). This only works for unordered channels, as ordered channels use nextSequenceRecv rather than packet receipts. Therefore, on ordered channels, the query response will include all sequence numbers that were present in the query, as no receipt will be found for any of them.

Version

v3.0.0

Steps to Reproduce

I used the interchain accounts demo repo to confirm the problem, as interchain accounts use ordered channels. After installation,

In terminal 1,

$ make init
$ make start-rly

In terminal 2,

export DEMOWALLET_1=$(icad keys show demowallet1 -a --keyring-backend test --home ./data/test-1) && echo $DEMOWALLET_1;
export DEMOWALLET_2=$(icad keys show demowallet2 -a --keyring-backend test --home ./data/test-2) && echo $DEMOWALLET_2;

# This will have hermes relay the channel handshake in terminal 1
icad tx intertx register --from $DEMOWALLET_1 --connection-id connection-0 --chain-id test-1 --home ./data/test-1 --node tcp://localhost:16657 --keyring-backend test -y

# Once the channel is opened on both chains...
export ICA_ADDR=$(icad query intertx interchainaccounts connection-0 $DEMOWALLET_1 --home ./data/test-1 --node tcp://localhost:16657 -o json | jq -r '.interchain_account_address') && echo $ICA_ADDR

At this point, hit ^C in terminal 1 to stop hermes from relaying packets. We will create a sendPacket on test-1, recv it on test-2, and then query unreceived packets on test-2 to confirm that the chain claims that our packet has not been received, even though it has.

In terminal 2, send the packet.

icad tx intertx submit \
'{
    "@type":"/cosmos.bank.v1beta1.MsgSend",
    "from_address":"cosmos15ccshhmp0gsx29qpqq6g4zmltnnvgmyu9ueuadh9y2nc5zj0szls5gtddz",
    "to_address":"cosmos10h9stc5v6ntgeygf5xf945njqq5h32r53uquvw",
    "amount": [
        {
            "denom": "stake",
            "amount": "1000"
        }
    ]
}' --connection-id connection-0 --from $DEMOWALLET_1 --chain-id test-1 --home ./data/test-1 --node tcp://localhost:16657 --keyring-backend test -y

In terminal 1,

# Confirm that we see 1 unreceived packet
hermes -c network/hermes/config.toml query packet unreceived-packets test-2 icahost channel-0

# Recv the packet on test-2 (make sure you run this before the packet times out)
hermes -c network/hermes/config.toml tx raw packet-recv test-2 test-1 icacontroller-cosmos1m9l358xunhhwds0568za49mzhvuxx9uxre5tud channel-0

# Confirm that we *still* see 1 unreceived packet
hermes -c network/hermes/config.toml query packet unreceived-packets test-2 icahost channel-0

---

For Admin Use

• Not duplicate issue
• Appropriate labels applied
• Appropriate contributors tagged/assigned

[crodriguezvega]

Thanks for opening this issue, @plafer! This is a very good catch!

[crodriguezvega]

Some pseudocode to discuss a possible way to fix this issue.

After this line replace this block with something like this:

channel, found := q.GetChannel(ctx, req.PortId, req.ChannelId)
if !found {
	return nil, status.Error(
		codes.NotFound,
		sdkerrors.Wrapf(types.ErrChannelNotFound, "port-id: %s, channel-id %s", req.PortId, req.ChannelId).Error(),
	)
}

unreceivedSequences := []uint64{}

switch channel.Ordering {
case types.UNORDERED:
	for i, seq := range req.PacketCommitmentSequences {
		if seq == 0 {
			return nil, status.Errorf(codes.InvalidArgument, "packet sequence %d cannot be 0", i)
		}

		// if packet receipt exists on the receiving chain, then packet has already been received
		if _, found := q.GetPacketReceipt(ctx, req.PortId, req.ChannelId, seq); !found {
			unreceivedSequences = append(unreceivedSequences, seq)
		}
	}
case types.ORDERED:
	nextSequenceRecv, found = q.GetNextSequenceRecv(ctx, req.PortId, req.ChannelId)
	if !found {
		return nil, status.Errorf(...)
	}

	for _, seq := range req.PacketCommitmentSequences {
		if seq == 0 {
			return nil, status.Errorf(codes.InvalidArgument, "packet sequence %d cannot be 0", i)
		}

		if seq >= nextSequenceRecv {
			unreceivedSequences = append(unreceivedSequences, seq)
		}
	}
default:
	return nil, status.Errorf(...)
}

[colin-axner]

Hi @plafer, what behaviour would be desirable?

The functionality of UnreceivedPackets could be different for ordereded channels. For example, a list of sequence might be provided: 2, 7, 9, 10. If the latest received packet is 5, should we returned 6, 7, 8, 9, 10 - or 7, 9, 10. I'm wondering if we should create a separate gRPC entirely?

Would love to hear your input. In what context are you using this function and how could we best adjust the functionality to fit your needs?

[plafer]

I don't believe a separate gRPC endpoint is necessary. From the perspective of relayers (which I assume are the primary users of this endpoint), using a different gRPC endpoint depending on the channel ordering adds complexity, which I don't believe it justified. I assume that on ordered channels, queries similar to 2, 7, 9, 10 are rare, as there's no point in sending a bunch of packets all at once since most will be dropped unless the user is "lucky" that the packets got relayed in the proper order. In other words, my assumption is that most queries on ordered channels have a single sequence number, or maybe a few packets (e.g. 2, 3, 4).

For example, a list of sequence might be provided: 2, 7, 9, 10. If the latest received packet is 5, should we returned 6, 7, 8, 9, 10 - or 7, 9, 10.

Hence, I believe 7, 9, 10 should be returned to preserve the semantics of the unordered channel case (i.e. as implemented @crodriguezvega's pseudocode).