IBC: Allow onRecvPacket to bypass synchronous call of PacketExecuted

[michaelfig]

Description

NOTE: This PR is superseded by cosmos/ibc#478

Allow an OnRecvPacket callback to prevent the automatic synchronous calling of PacketExecuted by returning a nil acknowledgement.

This bypasses the synchronous call to PacketExecuted and allows
the application module to call it at a later time.

---

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

[codecov]

Codecov Report

Merging #7200 into master will increase coverage by 0.00%.
The diff coverage is 33.33%.

@@           Coverage Diff           @@
##           master    #7200   +/-   ##
=======================================
  Coverage   54.76%   54.76%           
=======================================
  Files         563      563           
  Lines       38608    38609    +1     
=======================================
+ Hits        21144    21145    +1     
  Misses      15733    15733           
  Partials     1731     1731           

[ethanfrey]

Two questions here:

• does this need a new capability to control who can call the PacketExecuted call later?
• is there a safety reason it was removed from the spec/implementation?

If this is a small, safe change and makes things better I am all for it. I really want stargate soon, so I would voice hesitation on any larger feature changes, pushing them to 1.1. (I have no real clue what impact this makes in the ibc code, just a watchdog about shipping something solid)

[michaelfig]

Two questions here:

• does this need a new capability to control who can call the PacketExecuted call later?

I don't think so. The PacketExecuted call is only honoured for callers that hold the channel cap. That's still the case.

• is there a safety reason it was removed from the spec/implementation?

It was just an implementation (not spec) change. I'd have to defer to @colin-axner on this question, but he didn't mention anything about safety, it seems it was done just to help synchronous callers remember to call PacketExecuted.

If this is a small, safe change and makes things better I am all for it.

👍

I really want stargate soon, so I would voice hesitation on any larger feature changes, pushing them to 1.1. (I have no real clue what impact this makes in the ibc code, just a watchdog about shipping something solid)

Fair.

[fedekunze]

LGTM

[colin-axner]

Reposting @cwgoes comments from slack here:

No, we can't do this, because timeouts are based on acks
What we have right now are acknowledgements of receipt
Which can contain synchronous execution results
If you want async execution results, you will need to send another packet back

I think this pr should be closed. Sending another packet is the appropriate design for this

[cwgoes]

Right, the issue here is that we are using this acknowledgement as proof-of-receipt, so if you process the packet asynchronously without writing it, the packet may be treated as timed-out when you've actually received it (just not yet written the acknowledgement). Does that make sense?

For asynchronous acknowledgements, I suggest using a very simple ack-of-receipt and sending another packet back once the processing is complete.

[michaelfig]

Right, the issue here is that we are using this acknowledgement as proof-of-receipt, so if you process the packet asynchronously without writing it, the packet may be treated as timed-out when you've actually received it (just not yet written the acknowledgement). Does that make sense?

I would think this is not a problem, so long as PacketExecuted on a timed out packet returns some kind of error. Even if you lift the async ack to the application level, you have the same basic issue: being in the middle of processing a packet when the channel closes.

I hope there is a way to work around this, or else it is impossible for protocols like ICS20 to be implemented by an async contract (such as all contracts in the Agoric system). Maybe it will be necessary for the standard to separate synchronous "implicit acks" which don't carry data and are never seen by the application from "replies" that are associated with a packet but can be generated asynchronously by the application.

However, let's discuss this at the IBC meeting tomorrow. I agree that it should not hold up Stargate unless there is a simple solution.

[michaelfig]

Closing to reduce noise in the PR list. This PR is, as in the description, superseded by cosmos/ibc#478