Extend acknowledgement type #582

ethanfrey · 2021-06-03T13:43:35Z

Note: on hold pending #581 discussion. If that is merged, this will likely be closed. If that is rejected, I would integrate all feedback and revive this proposal.

This is discussing changing the acknowledgement type from application-dependent bytes to actually care more protocol-level metadata, like the Packet type.

So far, I work on defining the type and update some functions to get general feedback.

How a chain / channel / app should handle different types in a backwards compatible way is the huge open question:

Can we require all applications / modules on a v2 chain to use the new acknowledgement format? Or do we do this on a channel/connection-level basis?
We must handle the case where a v2 implementation talks with a v1 implementation. How do we handle backwards-compatibility?

ethanfrey · 2021-06-03T13:49:40Z

One idea for backwards compatibility is to do this on a chain-by-chain basis.

v1 - v1 uses the current acknowledgement bytes both in the application interface and on the wire
v2 - v2 uses the new acknowledgement object both in the application interface and on the wire
v1 - v2 must be supported and a bit tricky. This must be detected somewhere (eg. establishing a connection), and we need to use v1 acknowledgements on the wire.

In this last case, the v2 application can return the entire Acknowledgement object on writeAcknowledgement and the IBC handler (which detects this is sent on a v1 connection) can drop the extra fields and just return the acknowledgement.data on the wire.

When they receive an acknowledgement from v1, they can wrap it in an Acknowledgement object before passing to the applications. In this case, all fields but data will be unset, but the applications can be shielded from this mainly.

Note: the implications is that limited fees would theoretically be possible on a v2-v1 connection. That is packets sent from the v2 chain to the v1 chain could incentivize ack and timeout, but not receive. Maybe they pay extra on ack and hope that gets picked up by the forward relayer (at least to some decent approximation over many runs). The packets sent from v1-v2 would still be unincentivized

ethanfrey

Just a first draft, these were some areas where I am most unclear about

ethanfrey · 2021-06-03T13:51:16Z

spec/core/ics-004-channel-and-packet-semantics/README.md

@@ -124,12 +124,54 @@ interface Packet {

 Note that a `Packet` is never directly serialised. Rather it is an intermediary structure used in certain function calls that may need to be created or processed by modules calling the IBC handler.

+Question: why is this here? Isn't there a protobuf definition for Packet serialization which must be understood by all


There seems to be a well-defined protobuf type in the IBC spec.
Why is this abstracted away to some undefined OpaquePacket?

No, there is not a protobuf for a Packet, because it is never sent over the wire, the fields are implicitly committed to by Merkle path (store) keys.

Thank you for the review. I will respond completely on Monday, but one question Chris.

Please correct me if I am wrong here. In my understanding, when a Packet is passed to the ics-4 handler to send. That will:

emit events that correspond to each field of the packet

one of those fields is app- specific data. The rest what I call "meta data" (defined by core protocol not app packet)

The packet is serialized as protobuf and then hashed

This hash is committed to in the Merkle store

In order to relay the packet, the relayer must parse the fields from the events and reform them into a protobuf message, which serialized to a preimage of the commitment on the source chain.

Is this correct? I am pretty sure there is a protobuf encoding used for the proofs/hashes and that is what I was referring to.

I want to do a similar thing with Acknowledgement as you do with Packet now. As close as I can do without breaking something. But mainly I need to understand better what the process is for Packet

The packet is serialized as protobuf and then hashed

No, this does not happen, at least not as far as I am aware. The only thing that is stored is the commitment to the packet data, timeout height, and timeout timestamp. See here in the specification and here in the code. Perhaps we should consider changing this, but in any case we do not need to serialise other fields (e.g. concerning channel source and destination) because they are implicitly committed to by the Merkle store keys.

Thank you for that clarification. Very useful for me.

I feel that hash function you linked:

sha256(bigEndian(timeoutTimestamp) || bigEndian(timeoutRevNumber) || bigEndian(timeoutRevHeight) || sha256(data))

needs to be in the IBC spec, as it is required for a compatible implementation. At least it would clear a few points up for me.

It would also help me understand this better if the actual flow of commitments and reconstructing the proofs (eg. IBC from PoV of the relayer) was documented in another section. I feel this is critical use case and important for correctness/liveness arguments as well.

ethanfrey · 2021-06-03T19:51:52Z

spec/core/ics-004-channel-and-packet-semantics/README.md

@@ -215,7 +257,7 @@ function packetReceiptPath(portIdentifier: Identifier, channelIdentifier: Identi
 }
 ```

-Packet acknowledgement data are stored under the `packetAcknowledgementPath`:
+Constant-size commitments to packet acknowledgement data are stored under the `packetAcknowledgementPath`:


The pseudo-code below shows hash(acknowledgement) being stored, this suggests the entire acknowledgment is stored. I think this is a valid fix (and nothing to do with my proposed changes)

ethanfrey · 2021-06-03T20:16:46Z

spec/core/ics-004-channel-and-packet-semantics/README.md

 ```typescript
 function recvPacket(
  packet: OpaquePacket,
  proof: CommitmentProof,
  proofHeight: Height,
-  relayer: string): Packet {
+  relayer: string,
+  payOnSource: string /* Maybe<string>? */): Packet {


Actual use of it is for another spec like #581

ethanfrey · 2021-06-03T20:17:28Z

spec/core/ics-004-channel-and-packet-semantics/README.md

@@ -670,20 +716,30 @@ The IBC handler performs the following steps in order:
 ```typescript
 function writeAcknowledgement(
  packet: Packet,
-  acknowledgement: bytes): Packet {
+  acknowledgement: Acknowledgement | bytes): Packet {


Not sure how to write both versions here....

This is a "v2" handler that may be talking on a "v1" or "v2" connection

I think you need to get the connection for the channel, then interpret acknowledgment based on the connection version. Wondering if this should be done in the caller, e.g. handlePacketRecv (ICS-26) for the sync ack

function handlePacketRecv(datagram: PacketRecv) { handler.recvPacket(datagram.packet, datagram.proof, datagram.proofHeight) module = lookupModule(datagram.packet.destPort) ack_data = module.onRecvPacket(datagram.packet) ack_bytes = hash(Acknowledgment(ack_data, datagram.payOnSource, datagram.signer)) handler.writeAcknowledgement(datagram.packet, ack_bytes) }

And similarly call the fee module for relayer payments from handlePacketAcknowledgement, handlePacketTimeout, etc.

ethanfrey · 2021-06-03T20:18:20Z

spec/core/ics-004-channel-and-packet-semantics/README.md

+```typescript
+interface Acknowledgement {
+  data: bytes
+  payOnSource: string  // Maybe<string> ???


I think this should be optional.
Failure to include it (relayer unaware of fees or doesn't care) shouldn't prevent packet/ack relaying

AdityaSripal · 2021-06-03T22:17:37Z

Nice work, I think from a backwards compatibility standpoint. It is easier for the applications to still only create and process opaque bytes as the acknowledgement.

Regardless of what version the chain is on, the modules just return and receive the application-specific acknowledgement bytes.

If the connection version is on V2, then the ICS-4 handlers just wrap/unwrap the acknowledgement bytes in the structure you have and send it over the wire but the application is not aware of this at all.

On write acknowledgement, app returns ack bytes and ICS-4 places it in ack struct along with forward relayer address.
During acknowledgePacket, ICS-4 unwraps ack struct and pays forward relayer, it then passes the raw ack bytes to the application callback.

If the connection version is on V1, then the ICS-4 handlers just commit the acknowledgement bytes directly as it does already.

The only reason I see to bring the structured Acknowledgement all the way to the application is to enable future field extensions that might be used directly by apps but it seems unnecessary for this case.

AdityaSripal · 2021-06-03T22:21:08Z

In order to keep things even easier for relayers to integrate this, we could keep the proto definiton of Acknowledgement still have bytes in all the relayer messages. It is just required that the bytes unmarshal to the structured Acknowledgement if the connection is v2.

In this case, the relayer doesn't even need to send a different type of acknowledgement message. Though this may be too loose of an implementation, and relayers will have to adapt anyway to include forward address.

AdityaSripal · 2021-06-03T22:22:41Z

On write acknowledgement, app returns ack bytes and ICS-4 places it in ack struct along with forward relayer address.

Note that this requires some storage in core IBC in order to support asynchronous acks

ethanfrey · 2021-06-04T13:44:59Z

Nice work, I think from a backwards compatibility standpoint. It is easier for the applications to still only create and process opaque bytes as the acknowledgement.

Regardless of what version the chain is on, the modules just return and receive the application-specific acknowledgement bytes.

If the connection version is on V2, then the ICS-4 handlers just wrap/unwrap the acknowledgement bytes in the structure you have and send it over the wire but the application is not aware of this at all.

On write acknowledgement, app returns ack bytes and ICS-4 places it in ack struct along with forward relayer address.
During acknowledgePacket, ICS-4 unwraps ack struct and pays forward relayer, it then passes the raw ack bytes to the application callback.

If the connection version is on V1, then the ICS-4 handlers just commit the acknowledgement bytes directly as it does already.

The only reason I see to bring the structured Acknowledgement all the way to the application is to enable future field extensions that might be used directly by apps but it seems unnecessary for this case.

This seems like a good design.

My biggest issue in this pseudocode is that it seems to be defining the ICS-4 handlers, but doesn't explicit show what it passes into the application-level.

You propose:

OnReceivePacket - ICS-4 receives all data, stores payOnSource (and forwardRelayer?)based on "packet id". It then calls the application without thepayOnSource`
WriteAcknowledgement - ICS-4 takes bytes from the application. It checks if it is v2 and if so, loads the data stored above and constructs a proper Acknowledgement struct, which is will proto-encode and then commit to that. If the connection is v1, then it just commits the raw acknowledgement.
- For better relayer compatibility, we can just return the pre-hashed commitment as an event, regardless if it is raw app data or wrapper. Does ibc-go emit acknowledgement_hex event as well (like with packet) for better binary compatibility?
OnAcknowledge - ICS-4 will check the connection version. If it is v1, then it will pass those raw ack bytes to the application. If it is v2, it will parse the acknowledgement bytes into Acknowledgement message, call the fee handler with ack relayer and payOnSource from ack packet, and then pass the acknowledgement.data field to the applicaton.

To make this all more clear, it would be nice to define somewhere in this psuedo-code the flow and call from ICS-4 handlers into the application modules. Or is that only done in ibc-go?

cwgoes

Thanks! See comments.

cwgoes · 2021-06-05T13:23:59Z

spec/core/ics-004-channel-and-packet-semantics/README.md

@@ -124,12 +124,54 @@ interface Packet {

 Note that a `Packet` is never directly serialised. Rather it is an intermediary structure used in certain function calls that may need to be created or processed by modules calling the IBC handler.

+Question: why is this here? Isn't there a protobuf definition for Packet serialization which must be understood by all


No, there is not a protobuf for a Packet, because it is never sent over the wire, the fields are implicitly committed to by Merkle path (store) keys.

cwgoes · 2021-06-05T13:24:37Z

spec/core/ics-004-channel-and-packet-semantics/README.md

+**Update:**
+
+The first version of IBC considered `Acknowledgement` simple application dependent bytes. This stood in contrast to the
+`Packet` types, which includes a number of well-defined metadata fields in addition to an application-dependent


No, these work the same way. The contents of a packet may be well-defined, e.g. by ICS 20, but not by core IBC.

cwgoes · 2021-06-05T13:25:12Z

spec/core/ics-004-channel-and-packet-semantics/README.md

+}
+```
+
+- The `data` is an opaque value which can be defined by the application logic of the associated modules. The contents are


Is it still committed to (by a hash function)? Or is the whole Acknowledgement structure now hash-committed-to?

cwgoes · 2021-06-05T13:26:10Z

spec/core/ics-004-channel-and-packet-semantics/README.md

+
+Open discussion:
+
+- Is `forwardRelayer` useful info? This information is available without any work by the *forward relayer* and seems


Seems useful to me, as long as it's not too expensive to store.

cwgoes · 2021-06-05T13:26:38Z

spec/core/ics-004-channel-and-packet-semantics/README.md

+- The `data` is an opaque value which can be defined by the application logic of the associated modules. The contents are
+  identical to a v1 acknowledgement.
+- The `payOnSource` indicates an address on the **source chain** that the *forward relayer* can specify, which should receive
+  any existing reward for relaying the IbcReceivePacket.


What's an existing reward - a fee that has already been pre-paid on the source chain?

cwgoes · 2021-06-05T13:31:26Z

spec/core/ics-004-channel-and-packet-semantics/README.md

+- Is `payOnSource` required? What happens if a relayer doesn't set this field? Is the packet rejected? What if it is an
+  invalid address on source (this is hard to validate on destination chain)
+
+Note that a `Acknowledgement` is never directly serialized. Rather it is an intermediary structure used in certain function calls that may need to be created or processed by modules calling the IBC handler.


Ah, similarly to Packet, then?

cwgoes · 2021-06-05T13:31:48Z

spec/core/ics-004-channel-and-packet-semantics/README.md

    // write the acknowledgement
+    commitment = hash(makeOpaque(acknowledgement))


What is makeOpaque doing?

cwgoes · 2021-06-05T13:32:05Z

spec/core/ics-004-channel-and-packet-semantics/README.md


    // cannot already have written the acknowledgement
    abortTransactionUnless(provableStore.get(packetAcknowledgementPath(packet.destPort, packet.destChannel, packet.sequence) === null))

+    // we need some way to handle older formats?
+    if (typeof acknowledgement === "bytes") {


This is pseudocode, we can't actually do this in Go.

ancazamfir · 2021-06-07T23:04:02Z

spec/core/ics-004-channel-and-packet-semantics/README.md

+Open discussion:
+
+- Is `forwardRelayer` useful info? This information is available without any work by the *forward relayer* and seems
+  more generic than just fee incentivization.


What is forwardRelayer meant for? For a relayer to figure out if it was the one sending the MsgRecvPacket? And what does more generic than just fee incentivization mean?
Can the relayer query/ decode the ack bytes and verify that the payOnSource is in fact its address (hasn't been altered)?

What is forwardRelayer meant for?

I threw it in as the info was there, and I thought it might be useful (future proofing).
Then again, it may be totally useless and premature optimization.

Happy not to include it as well.

Can the relayer query/ decode the ack bytes and verify that the payOnSource is in fact its address (hasn't been altered)?

This info must be exposed in the events, so the relayer can provide this info on the remote side for proving.
I was a bit confused by commitments, so I haven't finalized the hashes and commitments below

ancazamfir · 2021-06-07T23:05:54Z

spec/core/ics-004-channel-and-packet-semantics/README.md

+
+- Is `forwardRelayer` useful info? This information is available without any work by the *forward relayer* and seems
+  more generic than just fee incentivization.
+- Is `payOnSource` required? What happens if a relayer doesn't set this field? Is the packet rejected? What if it is an


It should be optional if we allow un-incentivized channels over a V2 connection.

ancazamfir · 2021-06-07T23:10:41Z

spec/core/ics-004-channel-and-packet-semantics/README.md

@@ -593,12 +635,16 @@ The IBC handler performs the following steps in order:

 We pass the address of the `relayer` that signed and submitted the packet to enable a module to optionally provide some rewards. This provides a foundation for fee payment, but can be used for other techniques as well (like calculating a leaderboard).

+Note: `relayer` and `payOnSource` were added in v2 (v1?) of the IBC spec. They will not be present on older implementations.


The relayer was already in v1, only the payOnSource is added in v2.

The relayer was already in v1

I am a bit confused by version numbers.
Cosmos SDK 0.42 has no relayer
Cosmos SDK 0.43 / IBC-Go 1.0 has relayer
Proposed changes have payOnSource also.

I take your point that v1 channels can support payOnSource as well, not sure how to explain these were added later on.

ancazamfir · 2021-06-07T23:22:20Z

spec/core/ics-004-channel-and-packet-semantics/README.md

@@ -670,20 +716,30 @@ The IBC handler performs the following steps in order:
 ```typescript
 function writeAcknowledgement(
  packet: Packet,
-  acknowledgement: bytes): Packet {
+  acknowledgement: Acknowledgement | bytes): Packet {


I think you need to get the connection for the channel, then interpret acknowledgment based on the connection version. Wondering if this should be done in the caller, e.g. handlePacketRecv (ICS-26) for the sync ack

function handlePacketRecv(datagram: PacketRecv) { handler.recvPacket(datagram.packet, datagram.proof, datagram.proofHeight) module = lookupModule(datagram.packet.destPort) ack_data = module.onRecvPacket(datagram.packet) ack_bytes = hash(Acknowledgment(ack_data, datagram.payOnSource, datagram.signer)) handler.writeAcknowledgement(datagram.packet, ack_bytes) }

And similarly call the fee module for relayer payments from handlePacketAcknowledgement, handlePacketTimeout, etc.

ethanfrey · 2021-06-21T20:07:26Z

I would deprecate this for the new ics-30 middleware solution being developed in #581

If that middleware solution is not followed, I would be happy to update this spec with all comments. But the 2 are mutually exclusive in the current design.

mpoke · 2022-04-11T13:56:30Z

@ethanfrey Is this PR still relevant?

ethanfrey · 2022-04-11T17:01:27Z

Maybe.

I think a discussion of actually providing structure to the acknowledgement would be a good thing.

There is no concrete action item here

ethanfrey added 2 commits June 3, 2021 15:31

Define Acknowledgement type

b213b6a

Start updating handlers

9a9e03c

ethanfrey commented Jun 3, 2021

View reviewed changes

ethanfrey mentioned this pull request Jun 4, 2021

General Fee Payment Spec #581

Merged

cwgoes suggested changes Jun 5, 2021

View reviewed changes

ancazamfir reviewed Jun 8, 2021

View reviewed changes

crodriguezvega closed this Nov 15, 2022

		@@ -124,12 +124,54 @@ interface Packet {

		Note that a `Packet` is never directly serialised. Rather it is an intermediary structure used in certain function calls that may need to be created or processed by modules calling the IBC handler.

		Question: why is this here? Isn't there a protobuf definition for Packet serialization which must be understood by all


		Open discussion:

		- Is `forwardRelayer` useful info? This information is available without any work by the forward relayer and seems

		// write the acknowledgement
		commitment = hash(makeOpaque(acknowledgement))

		@@ -593,12 +635,16 @@ The IBC handler performs the following steps in order:

		We pass the address of the `relayer` that signed and submitted the packet to enable a module to optionally provide some rewards. This provides a foundation for fee payment, but can be used for other techniques as well (like calculating a leaderboard).

		Note: `relayer` and `payOnSource` were added in v2 (v1?) of the IBC spec. They will not be present on older implementations.

Extend acknowledgement type #582

Extend acknowledgement type #582

Conversation

ethanfrey commented Jun 3, 2021 • edited Loading

ethanfrey commented Jun 3, 2021 • edited Loading

ethanfrey left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cwgoes Jun 7, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AdityaSripal commented Jun 3, 2021 • edited Loading

AdityaSripal commented Jun 3, 2021

AdityaSripal commented Jun 3, 2021

ethanfrey commented Jun 4, 2021

cwgoes left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ethanfrey commented Jun 21, 2021

mpoke commented Apr 11, 2022

ethanfrey commented Apr 11, 2022

ethanfrey commented Jun 3, 2021 •

edited

Loading

ethanfrey commented Jun 3, 2021 •

edited

Loading

cwgoes Jun 7, 2021 •

edited

Loading

AdityaSripal commented Jun 3, 2021 •

edited

Loading