Fix QueryUnreceivedPackets/Acks

[AdityaSripal]

Description

closes: #7319

---

Before we can merge this PR, please make sure that all the following items have been
checked off. If any of the checklist items are not applicable, please leave them but
write a little note why.

• Targeted PR against correct branch (see CONTRIBUTING.md)
• Linked to Github issue with discussion and accepted design OR link to spec that describes this work.
• Code follows the module structure standards.
• Wrote unit and integration tests
• Updated relevant documentation (docs/) or specification (x/<module>/spec/)
• Added relevant godoc comments.
• Added a relevant changelog entry to the Unreleased section in CHANGELOG.md
• Re-reviewed Files changed in the Github PR explorer
• Review Codecov Report in the comment section below once CI passes

[AdityaSripal]

Not exactly sure what all the other proto changes are about. I reran make proto-tools and this was the result.

Also make proto-format errors for me with these errors:

Error reading /Users/adityasripal/go/src/github.com/cosmos/cosmos-sdk/./.clang-format: Invalid argument
YAML:94:22: error: unknown key 'Delimiter'
  - Delimiter:       pb
                     ^~

EDIT: Fixed by reverting to Makefile before proto-tools script. Everything works now except proto-format still gives me the above issue

[colin-axner]

Not exactly sure what all the other proto changes are about. I reran make proto-tools and this was the result.

Also make proto-format errors for me with these errors:

Error reading /Users/adityasripal/go/src/github.com/cosmos/cosmos-sdk/./.clang-format: Invalid argument
YAML:94:22: error: unknown key 'Delimiter'
  - Delimiter:       pb
                     ^~

do you have the correct versions installed?

[colin-axner]

This differs from the godoc

// Given a list of counterparty packet commitments, the querier checks if the packet
// has already been received by checking if an acknowledgement exists on this
// chain for the packet sequence.

It seems the implemented flow here is trying to take in a list of acknowledgement sequences and check if the packet commitment exists to see the ack needs to be relayed? How does a relayer determine which acknowledgement sequences to send? Unlike packet commitments, acknowledgements live forever so a relayer would have to already know which acknowledgements have not been acknowledged on the original chain, which is the purpose of this function.

The previous flow is to use the receiving chain as the indicator. A packet has 3 life cycles:

• sent but not received
• received but not ack'd
• ack'd and deleted

The sender chain indicates if the packet is in the first two stages (querying packet commitments) and the receiving chain indicates which of the first two stages it is in (ack doesn't exist, stage 1. ack does exist, stage 2).

[colin-axner]

I assume the line of thinking that lead to this function is for it to be used in conjunction with the QueryUnreceivedPackets. This issue here arises with the introduction of async acknowledgements. Even if you relay unreceived packets, the acknowledgement may not be written for some time. You could keep a cache of those sequences, but you'd still need to query the receiving chain to find out if the acknowledgements have been written (verifying that your unreceived packet sequences has now become unreceived acknowledgements). You see what I am saying?

Regardless, the relayer should not rely on these functions beyond the short term. As per recent decisions in the IBC call, acknowledgements do not have to be written for proper functioning channels. This means packet commitments could live forever and relayers could get stuck trying to determine if an ack needs to be relayed. It should primarily rely on events. Maybe we should add this to the godoc?

[AdityaSripal]

It seems the implemented flow here is trying to take in a list of acknowledgement sequences and check if the packet commitment exists to see the ack needs to be relayed? How does a relayer determine which acknowledgement sequences to send? Unlike packet commitments, acknowledgements live forever so a relayer would have to already know which acknowledgements have not been acknowledged on the original chain, which is the purpose of this function.

Ahh fair point. My thinking was that the relayer gives the entire list of acknowledgements to this function but that may be too large and inefficient in a long lived channel.

The original intention was to send packet commitments that still exist on the sending chain to the receiving chain and see if the ack exists? I can reimplement that, tho I still think it's better to make it a separate RPC call. Unfortunately this doesn't have a symmetry with the QueryUnreceivedPackets call which makes it non-intuitive.

This issue here arises with the introduction of async acknowledgements. Even if you relay unreceived packets, the acknowledgement may not be written for some time. You could keep a cache of those sequences, but you'd still need to query the receiving chain to find out if the acknowledgements have been written (verifying that your unreceived packet sequences has now become unreceived acknowledgements). You see what I am saying?

Yes its possible even with the original implementation that once async acknowledgements are introduced that packet commitments exist for a long time before the ack gets written. I don't think this is a huge deal and hard to avoid. I think these functions should probably be used on relayer startup and once all pending packets/acks in the queue are cleared, relayer should primarily rely on events

cc: @jackzampolin

[codecov]

Codecov Report

Merging #7320 into master will decrease coverage by 0.09%.
The diff coverage is 15.88%.

@@            Coverage Diff             @@
##           master    #7320      +/-   ##
==========================================
- Coverage   55.41%   55.32%   -0.10%     
==========================================
  Files         583      583              
  Lines       35550    35637      +87     
==========================================
+ Hits        19700    19715      +15     
- Misses      13942    14014      +72     
  Partials     1908     1908              

[AdityaSripal]

I kept the implementation of retrieving Acks the same but separated it into two RPC calls for clarity:

UnreceivedPackets: I call this unreceived because it is an RPC call sent to the intended destination chain of the packets, so we're querying the chain to see which packets the chain has not yet received.

UnrelayedAcks: I call this unrelayed because it is an RPC call sent to the chain that is sending the acks, thus it is will return all the acks on this chain that have not yet been relayed.

The name change and separation should make things clearer along with the usage docs, and the unsymmetric names should reflect the unsymmetric nature of the calls and make usage more intuitive.

UnreceivedPackets will need to diverge from UnrelayedPackets implementation anyway with async acknowledgments as mentioned in the TODO docs

[alessio]

Please paste here the output of:

clang-format -version

[AdityaSripal]

@alessio

cosmos-sdk git:(master) ✗ clang-format -version
clang-format version 10.0.1 

[colin-axner]

LGTM, thanks for updating this!