Ack message relaying may fail in a 3 chain setup

[ancazamfir]

We just fixed a similar issue with the Rust relayer (informalsystems/hermes#614).

Steps to reproduce:

• Start ibc-0, ibc-1, ibc-2 using the three-chainz script
• Add paths zeroone and onetwo (some changes in the .json required):

rly paths add ibc-0 ibc-1 zeroone -f configs/three/demo.json
rly paths add ibc-0 ibc-1 onetwo -f configs/three/demo2.json

• Setup paths and make sure ibc-0 and ibc-2 use the same channel id

 rly tx full-path zeroone -d -o 3s
 rly tx full-path onetwo -d -o 3s

• Send one transfer to each of ibc-0 and ibc-2

  rly tx raw transfer ibc-0 ibc-1 100000stake $(rly ch addr ibc-1) -p zeroone
  rly tx raw transfer ibc-2 ibc-1 100000stake $(rly ch addr ibc-1) -p onetwo

• Relay the MsgReceivePacket:

  rly tx rly zeroone -d
  rly tx rly onetwo -d

• Try to relay the MsgAcknowledgment to ibc-1 on the onetwo path:

$ rly tx acks onetwo -d
Error: more than one transaction returned with query

[ancazamfir]

The problem is the ack query here:

relayer/relayer/naive-strategy.go

Lines 816 to 819 in 9f13a93

	func ackPacketQuery(channelID string, seq int) []string {
	return []string{fmt.Sprintf("%s.packet_src_channel='%s'", waTag, channelID),
	fmt.Sprintf("%s.packet_sequence='%d'", waTag, seq)}
	}

It is performed with packet_src_channel which is the same for both paths.

Following diffs seem to fix the issue:

diff --git a/relayer/naive-strategy.go b/relayer/naive-strategy.go
index 9a0d16b..84769db 100644
--- a/relayer/naive-strategy.go
+++ b/relayer/naive-strategy.go
@@ -624,7 +624,7 @@ func relayPacketFromSequence(src, dst *Chain, sh *SyncHeaders, seq uint64) (rela
 }

 func acknowledgementFromSequence(src, dst *Chain, sh *SyncHeaders, seq uint64) (relayPacket, error) {
-       txs, err := src.QueryTxs(sh.GetHeight(src.ChainID), 1, 1000, ackPacketQuery(dst.PathEnd.ChannelID, int(seq)))
+       txs, err := src.QueryTxs(sh.GetHeight(src.ChainID), 1, 1000, ackPacketQuery(src.PathEnd.ChannelID, int(seq)))
        switch {
        case err != nil:
                return nil, err
@@ -801,5 +801,5 @@ func rcvPacketQuery(channelID string, seq int) []string {
 }

 func ackPacketQuery(channelID string, seq int) []string {
-       return []string{fmt.Sprintf("%s.packet_src_channel='%s'", waTag, channelID), fmt.Sprintf("%s.packet_sequence='%d'", waTag, seq)}
+       return []string{fmt.Sprintf("%s.packet_dst_channel='%s'", waTag, channelID), fmt.Sprintf("%s.packet_sequence='%d'", waTag, seq)}
 }

With the fix:

$ rly tx acks onetwo -d
I[2021-02-10|09:56:57.320] ✔ [ibc-2]@{1574} - msg(0:update_client,1:acknowledge_packet) hash(137187013506E720DD72CEFDA7D644304E52033A16425ED7E9357CDA9C28BE52)
I[2021-02-10|09:56:57.320] ★ Relayed 1 packets: [ibc-1]port{transfer}->[ibc-2]port{transfer}

[colin-axner]

thanks @ancazamfir for the bug report and the proposed fix! This logic makes sense to me. Essentially managing acknowledgements happens from the destination chain perspective (which happens to be labeled src in this function). This is why we need src.PathEnd and packet_dst_channel