Add Packet and Acknowledgement spec to ICS4 along with Commitments #1149

AdityaSripal · 2024-09-23T09:53:47Z

Note to reviewers:

We need feedback on the packet structure and acknowledgement structure to see if it is sufficient for the use cases and platforms people have in mind but we also need feedback on the commitment structure.

The desired properties of the commitments:

Canonical (Every compliant implementation should be able to take the same Packet/Acknowledgement and reach the exact same commitment)
Secure (Two different packets MUST NOT create the same packet commitment)
Implementable (The given commitment structure must be implementable by all desired target platforms: EVM, ZK, Rust, Solana)
Cheap (The commitment structures must be feasibly cheap on desired target platforms: EVM, ZK)
Upgradable (While this is the hardest to upgrade, by splitting each component into its own byte array and hashing them individually they can also be changed independently)

Context for current decision:

The first option was to hash every field to ensure that each field is clearly separated and cannot be malleated. However, with the multiple payloads (5 fields per payload) this will lead to many hashes that all then have to be hashed together. This could be inefficient and also not particularly elegant. Also by hashing even the payload fields directly, the payload is part of the hashing specification, however if the payload is simply the encoded preimage as bytes passed into the hashing function, then the encoded preimage can be changed without changing the entire hashing specification
Another option considered is to prefix the length of each field in order to ensure bytes from one field do not get confused with another. This seemed error prone and is effectively implementing half an encoding standard so we considered simply encoding the payload with an existing standard.
The third option is to encode the payload. In order to do this, we need a canonical encoding standard that is widely implementable. Unfortunately Protobuf is NOT canonical so different implementations can yield different byte arrays. The two canonical encoding standards i considered were BCS and CBOR. BCS is a blockchain specific encoding standard while CBOR is widely adopted and also implemented in Solidity. Thus i suggest canonical CBOR as an option for now however i haven't completed research on whether this is the best option. The downside is that we are now adding a particular encoding format as a core requirement of IBC v2. The benefit is that we can encode the payload and acknowledgements with this encoding format before hashing. This is simpler to implement and verify for correctness, while also being easier to upgrade. Also this requires a well maintained cbor implementation that is also canonical on the platforms we support.

From these above options, we are considering either option 1 or option 3 (with the specific canonical encoding scheme tbd). Help in determing which option is best to accomplish the desired properties above is greatly appreciated!

Dependent on #1144

sangier · 2024-09-23T14:30:57Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ appData: bytes,
+}
+
+enum Encoding {


As follow-up of the discussion about the usage of the enum for the encoding, I have been thinking about it, and I see security wise benefits of constraining the type of encoding to an agreed range of enums. As for flexibility, we can still easily extend this enum upon request. Thus, I am ok keeping it like it is.

I tend to agree!

sangier

Thank you @AdityaSripal for this PR! It brings some clarifications in.
Overall, I am fine with the used nomenclature and think this can be merged once we decided on the canonical encoding scheme.
Per now we have in the alternative list:

bcs
cbor
It may probably make sense to involve amulet in this design decision to check if they can propose a secure-wise accepted standard or just validate the final decision.

sangier · 2024-09-24T09:53:47Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+}
+```
+
+The source and destination identifiers at the top-level of the packet identifiers the chains communicating. The source identifier **must** be unique on the source chain and is a pointer to the destination chain. The destination identifier **must** be a unique identifier on the destination chain and is a pointer to the source chain. The sequence is a monotonically incrementing nonce to uniquely identify packets sent between the source and destination chain.


Suggested change

The source and destination identifiers at the top-level of the packet identifiers the chains communicating. The source identifier **must** be unique on the source chain and is a pointer to the destination chain. The destination identifier **must** be a unique identifier on the destination chain and is a pointer to the source chain. The sequence is a monotonically incrementing nonce to uniquely identify packets sent between the source and destination chain.

The source and destination identifiers at the top-level of the packet indicate the chains communicating. The source identifier **must** be unique on the source chain and is a pointer to the destination chain. The destination identifier **must** be a unique identifier on the destination chain and is a pointer to the source chain. The sequence is a monotonically incrementing nonce to uniquely identify packets sent between the source and destination chain.

spec/core/v2/ics-004-packet-semantics/PACKET.md

soareschen · 2024-09-25T12:24:51Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ // at which point the packet is no longer valid.
+ // It cannot be received on the destination chain and can
+ // be timed out on the source chain
+ timeout: uint64,


Would it make sense to make the timeout field into opaque bytes that may be interpreted differently depending on the chain? This would allow the chain to pack different information with different encoding into the field. Some examples:

Optionality with Option type in Rust.

Packing of either or both timeout height and timeout timestamp.

Using the DA or L1 params to determine timeout.

Ultimately, the semantics of the timeout is tightly coupled with the IBC client. So it would be better to allow the IBC client to customize it.

It could make sense. We are trying to strike a balance between flexibility and just choosing a sane default.

Our thinking was that timestamps are way easier to understand and interpret for users. And most chains can implement either through BFT time directly in their protocol or through a time oracle application.

soareschen · 2024-09-25T12:32:56Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ // IBC core handlers will route the payload to the desired
+ // application using the port identifiers but the rest of the
+ // payload will be processed by the application
+ data: [Payload]


Does this mean multiple Payloads are allowed in one packet? This seems like a completely different behavior as compared to v1. Is this meant to improve performance, or is it to enable new features for IBC applications? I'm not sure if this is meant to allow partial failure, or is it to make it act like an atomic transaction that would fail if either of the payload fail.

It is different behaviour. The point is to enable new features for IBC applications. A key usecase is transfer + ICA

We want to eventually allow either partial failure or full atomic packets.

However, for launch we will only allow a single payload in this list and enable multiple in a future release

Hmm makes sense. Now I get a better understanding behind the design rationale.

Though I wonder what is supposed to be the API for enabling multi-payload packets? For the example case where we want to include both transfer and ICA payloads, would it be triggered by a multi-message transaction that sends multiple messages to transfer and ICA separately? Does this means that the IBC stack needs to somehow wrap around the transaction engine, and build only one IBC packet per src/dst pair for the entire transaction? Or do we need separate message-level APIs for constructing multi-payload packets?

soareschen · 2024-09-25T12:35:26Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ // the strucure expected given the version provided
+ // if the encoding is not supported, receiving application
+ // must be rejected with an error acknowledgement.
+ encoding: Encoding,


I'm not sure it is good to attach the encoding information to individual packet metadata. The payload encoding should be configured during the initialization of an IBC application, and stay the same with the same port.

Yes but two applications sitting on different chains may be initialized with two different encoding. For example, with solidity some encodings may be more efficient and preferred. In this case, a cosmos chain can use that encoding to speak to the application.

In general, we've run into a lot of problems with prenegotiated parameters (like the channel version) as they become quite complicated to renegotiate and change after the fact. And to also coordinate that change across all users synchronously. Having these parameters in the packet allows for very simply asynchronous migration. And if the encoding desired in the packet is not supported by the app, it just errors

Ok, so I suppose with this design, the user gets to instruct the sender application to use a different encoding depending on the counterparty chain.

Would it make sense to set this as a string field and use MIME to define the encoding? We already have standard MIME types such as application/json, text/plain, etc.

soareschen · 2024-09-25T12:42:49Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ // identifier on the dest chain
+ // that dest chain uses to address source chain
+ // this functions as the return address
+ destIdentifier: bytes,


It is not clear what should be the semantics if the given source / destination identifiers are invalid. Should the transaction fail? Or which layer should produce the error acknowledgement?

This is equivalent to passing in an invalid channel ID in IBC v1. It would be a core error that just rejects the tx. Not an error acknowledgement

soareschen · 2024-09-25T12:45:58Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ // expects destination chain to use in processing the message
+ // if dest chain does not support the version, the payload must
+ // be rejected with an error acknowledgement
+ version: string,


I'm not sure what is the use for the version field, and can't it be included within the appData itself if needed?

Even if we keep it, it may make sense to keep it as raw bytes as well.

It could, however this would require a significant change to the IBC apps to support it which i think we would like to avoid.

How would this allow backward compatibility, if existing IBC apps assume that the version stays the same after the handshake? Actually, I'm a bit confused of how classic IBC apps can be initialized, if we remove channel handshake?

soareschen · 2024-09-25T12:50:15Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ timeoutBytes = LittleEndian(timeout)
+ // TODO: Decide on canonical encoding scheme
+ // Suggested CBOR
+ appBytes = cbor.encoding(payload)


I think we should note that the use of CBOR encoding is specific to Cosmos SDK chains with ibc-go implementation. During verification, we should pass the packet as a structured data to the IBC client, so that it can choose the appropriate way to encode and verify the packet commitment on the counterparty chain.

No unfortunately the commitment has to be exactly the same across all chans so we must all agree at least on the commitment paths and the commitment values

soareschen · 2024-09-25T12:51:06Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+
+```typescript
+func commitV2Packet(packet: Packet) {
+ timeoutBytes = LittleEndian(timeout)


If the implementation is using CBOR, why not also use that to encode the timeout field?

soareschen · 2024-09-25T17:19:34Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ destIdentifier: bytes,
+ // the sequence uniquely identifies this packet
+ // in the stream of packets from source to dest chain
+ sequence: uint64,


Would it make sense to rename this to nonce, and also allow bytes? It seems to me that the only purpose remaining for channels is to prevent replay attack.

Perhaps sequence would still make sense if we add back ordered channel later on. But I also can't see how ordered channel could work with a packet design that allows multiple ports and payloads.

Yes, we could do it this way.

soareschen · 2024-09-25T17:22:05Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ // sourcePort identifies the sending application on the source chain
+ sourcePort: bytes,
+ // destPort identifies the receiving application on the dest chain
+ destPort: bytes,


What is the semantics of allowing different combinations of source and destination ports? Should it require a definite binding between the source port and destination port, or should each combination of source/destination port be identified uniquely?

Really all that matters is the portID on your side generally speaking. In case you want a strict mapping from your portID to counterparty portID that must be enforced in the application

I am still a bit confused on how the pairing would work, if there is no handshake and any application can send packet to any other application. It seems like we would need to take both the source and destination identifier pair as unique identification.

As an example, if we have two IBC transfer packets transfer-v1 -> transfer and transfer-v2 -> transfer, they should mint different denoms even when the destination port are the same.

I think it also probably make sense to call this an application identifier instead of calling it port. My understanding is that one application identifier is intended to be used for handling all packets of the same use case. So for example, there should be just one transfer or interchain-account identifier for all the respective packets, correct?

I think I also prefer referring to this as Application rather than port.

Farhad-Shabani

I tossed in a few questions and had a couple more, but I see Soares already covered the rest pretty well.

Farhad-Shabani · 2024-09-25T21:59:57Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+The IBC packet sends application data from a source chain to a destination chain with a timeout that specifies when the packet is no longer valid. The packet will be committed to by the source chain as specified in the ICS-24 specification. The receiver chain will then verify the packet commitment under the ICS-24 specified packet commitment path. If the proof succeeds, the IBC handler sends the application data(s) to the relevant application(s).
+
+```typescript
+interface Packet {


As far as I see, with this new packet structure, we’re no longer backward compatible with IBC v1. We can still reuse the same identifiers and sequence numbers from the v1 channel to create v2 packets and send them to the IBC v2 handlers. However, when IBC v2 interacts with applications, the callback functions still depend on the v1 packet structure. What’s the reasoning behind this? Is this implicitly requiring implementations now treat the packet field in callbacks as abstract and opaque, or has backward compatibility been dropped as a requirement for v2?

Backwards compatibility is not a strict requirement. We might break the application interfaces for the callback though their general expected behaviour can remain the same

Farhad-Shabani · 2024-09-25T22:01:35Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ // identifier on the source chain
+ // that source chain uses to address dest chain
+ // this functions as the sending address
+ sourceIdentifier: bytes,


Once two chains connected, by providing the destIdentifier, the IBC core should be able to retrieve the sourceIdentifier from storage and determine its value. Isn’t requiring this info unnecessary? After all, the sourceIdentifier linked to a destIdentifier will have to be stored anyway so that can perform a sanity check.

On the source chain there is a map from sourceId -> destId and vice versa.

But the source chain does not have a map destId -> sourceId because a counterparty's id for us is not unique from our perspective. So we do need the sourceId in the packet if we are going to use it on acknowledgement/timeout to retrieve the correct information

Are the source and destination identifiers supposed to be used like client ID or channel ID? If there are two separate IBC clients A1 and A2 created on Chain A for Chain B, can they both be used to send packets to the same IBC client B on chain B?

My feeling is that this should be possible, as otherwise we would need to perform redundant client updates. But to allow this, either we have to introduce something like channel ID as an indirection, or the chain should use both source and destination identifiers to form a unique pairing, and use it for nonce verification and sandboxing.

Farhad-Shabani · 2024-09-25T22:01:54Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ data: [Payload]
+}


Doesn't this open the door to a DoS attack through payload flooding? Shouldn't hosts consider a cap for that, depending on their characteristics?

I think we should think about DoS prevention at the full packet level, rather than the individual fields. An IBC packet should be encoded into a reasonable size when submitted as a ReceivePacket message to a chain. A chain may reject an IBC packet that is too large, or more sensibly the message handling would simply runs out of gas. Within that size limit, any field within the packet would also not exceed that size, so it can be reasonably handled, even if it is unusually larger than it usually should.

An application should not assume that an IBC packet can always be successfully relayed to a counterparty chain. Aside from the packet size, there are many ways the packet may fail, such as when there are malformed packet data, or when errors occurred when an application is processing the packet. Aside from that, a relayer should set their own limit of how large an IBC packet they are willing to relay, or how many retries they should make, so that it would not indefinitely try to relay a malformed packet. In general, a chain may contain arbitrary state machine logic that allows it to craft bad IBC packets. So neither the relayer nor the counterparty chain should trust that the packet is always wellformed.

We are only allowing single payload for now in ibc-go

Implementations should probably have a total packet size cap. And indeed relayers should also do this

Farhad-Shabani · 2024-09-25T22:02:17Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ version: string,
+ // encoding allows the sending application to specify which


On a more general note, I suggest we be very cautious about imposing string types at the protocol level. This can become costly in resource-limited environments like smart contracts. One field might not have a huge impact, though not being careful and leaving strings scattered throughout different data structures can increase memory allocations and overall costs due to operations like parsing.

Is it ok if there's just an equality check on the whole string? Even if we aren't parsing the internal details of the string

Farhad-Shabani · 2024-09-25T22:02:39Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+The timeout is the UNIX timestamp in seconds that must be passed on the **destination** chain before the packet is invalid and no longer capable of being received. Note that the timeout timestamp is assessed against the destination chain's clock which may drift relative to the clocks of the sender chain or a third party observer. If a packet is received on the destination chain after the timeout timestamp has passed relative to the destination chain's clock; the packet must be rejected so that it can be safely timed out and reverted by the sender chain.
+


Isn't it restrictive to define the timeout as only a UNIX timestamp? Was there a specific reason we're no longer allowing timeout height?

Unnecessary and unintuitive

Farhad-Shabani · 2024-09-25T22:03:20Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ // sourcePort identifies the sending application on the source chain
+ sourcePort: bytes,


With this definition, each port ID corresponds to a single application. I wonder how this applies to cases like ICS-27, where one app manages multiple ports. It would be helpful to clarify this relationship somewhere.

There is a mapping from portID to app. So multiple portIDs can map to the same application

soareschen · 2024-09-26T10:34:42Z

On the topic of encoding, fundamentally I think the problem lies in the leaky abstraction provided by ICS02-Client. The abstract methods such as verifyMembership are designed to be too general for verifying proofs at any Merkle path, with arbitrary bytes. However in reality, the core IBC logic only needs to handle a handful of specific proof verification. With the v2 specs that we are designing, it is likely that the only proof methods we need is to verify packet commitment and packet timeout.

The current design for ICS02 API also place strong assumption on how the commitment path is supposed to be constructed. By the time the path is passed to the client handler, all essential information such as path separators have been erased, making it difficult for the client handler to verify the proof in some other way, such as by using structured data and ZK proofs.

I think we should consider the option to allow for proof verification by passing in the structured Packet object directly, together with other metadata such as commitment prefix as separate arguments. This way, each chain can have their own way of encoding and committing the packet, without there needing to be a standard way of encoding for all possible chains. This way, Cosmos SDK chain implementations can choose to stick with the original method of encoding packets using CBOR or protobuf, while chains like Starknet can choose to use Cairo encoding and ZK storage proofs.

One way of doing this may be to augment the ICS02-Client API, to allow an ICS02 client handler to encode and verify a packet commitment proof. Alternatively, I think it may make sense to do this at a separate level, where different implementation of packet commitment strategy can be chosen during channel initialization. This would allow one chain to also have multiple ways of committing and verifying packets. For example, this would allow a Cosmos SDK chain to commit a packet by encoding with a different format, such as Cairo, and then hash it using Perdersen hash so that it can be verified more easily on Starknet.

It would probably make sense to introduce one (or maybe two) additional layer between ICS02-Client and ICS04-Packet, so that packet commitment and verification would go through this layer, instead of interacting with ICS02-Client directly. On one side, we have a handler that is responsible for encoding incoming packets from a counterparty chain, before using ICS02-Client to verify for membership proofs. On the other side, we have a handler that is responsible for encoding outgoing packets, and commit it using a prefixed ICS24-Host storage. It is worth noting that this decoupling is necessary so that the same ICS02-Client or ICS24-Host can be used to support multiple packet encoding and verification strategies.

AdityaSripal · 2024-09-23T11:14:36Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ sourcePort: bytes,
+ destPort: bytes,
+ version: string,
+ encoding: Encoding,


Should this be an enum for efficiency or something more like string for flexibility?

AdityaSripal · 2024-09-23T13:38:28Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ sourceIdentifier: bytes,
+ destIdentifier: bytes,
+ sequence: uint64
+ timeout: uint64,


Interpet this as seconds on receiver block time.

Reject timeouts that are too far in future in implementations (ibc-go) to ensure nanoseconds don't accidentally get interpreted as seconds

soareschen · 2024-09-27T10:31:52Z

In addition to the consideration of allowing custom packet commitment verification, for a concrete packet encoding implementation, I think we should also consider the IETF approach of specifying the encoding as packet diagrams.

Here are some resources I find that can help understand how to design packet diagrams:

Some of the things we can consider with this encoding:

Have a magic number prefix at the beginning to help distinguish IBC packets from arbitrary bytes.
Leave some reserved bits for future extensions, and require them to be set to zero for now.
Compact length fields to use a few bits with a reasonable maximum size limit
- Note that for fields that require non-zero length, we can save one bit by using the field value + 1 as the length.

Here is an example rough attempt at defining the IBC packet header encoding using a packet diagram:

  0                   1                   2                   3
  0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |     Magic     | R |     NL    |    SrcIdLen   |    DstIdLen   |
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |   TimeoutLen  |    P Count    |
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                Total Payload Size             |
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                          Nonce                              ...
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                          SrcId                              ...
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                          DstId                              ...
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+
 |                         Timeout                             ...
 +-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+-+

Field descriptions:

Magic - An 8-bit magic number such as 0xBC to identify the datagram as an IBC packet.
Reserve (R) - A 2-bit reserve for future extensions. MUST always be set to zero in the current version.
Nonce Length (NL) - A 6-bit field for the length of the nonce in value+1 bytes, allowing up to 128 bytes (1024-bits) nonce.
Source Identifier Length (SrcIdLen) - An 8-bit field for the length of the source identifier in value+1 bytes, allowing up to 512 bytes as identifier.
Destination Identifier Length (DstIdLen) - An 8-bit field for the length of the source identifier in value+1 bytes, allowing up to 512 bytes as identifier.
Timeout Length (TimeoutLen) - An 8-bit field for the length of the timeout field in value+1 bytes, allowing up to 512 bytes to encode timeout information.
Payload Count (P Count) - An 8-bit field inndicating value+1 number of payloads in the payload body, allowing up to 512 payloads.
Total Payload size - A 16-bit field for the total size of the payload body in value+1 bytes, allowing up to 32 MiB of payload body. This includes both the payload header and the payload data of all payloads.

Note that we leave out the packet payload body in the packet diagram, as we separate the definition of the IBC packet header and the body. We can then define separate mechanism to produce one hash for the packet header, one hash for the packet body, and then concatenate the two hashes together.

soareschen · 2024-09-30T11:47:59Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+}
+```
+
+## Acknowledgement V2


I think we should make the format of failure acknowledgement explicit. Regardless of which part of the IBC stack failed, the error should be formatted in a way that is standard for relayers and middlewares to understand. This would help in diagnostics for failures.

soareschen · 2024-10-01T10:35:33Z

We want to eventually allow either partial failure or full atomic packets.

We just uncovered some potential issues in supporting atomic IBC packets with multiple payloads. The problem is that inside an atomic transaction, if we want to fail and revert the state transition, then it would also not be possible to write any failed acknowledgement to the commitment store. But if we allow the transaction to return successfully, then we may not be able to revert any partial failure when handling the payload.

I guess in Cosmos SDK, it probably provides some kind of revertible context that can be used by ibc-go when calling applications. However, this would require privileged access from ibc-go to allow that to happen. This would not scale to other blockchains, in particular for smart contract platforms such as CosmWasm or Starknet. Each IBC application may be deployed as separate smart contracts. So we cannot revert only one of the success calls to a smart contract, unless we also revert the entire transaction.

On the other hand, in case if atomic payloads cannot be supported, I find it to be unnecessary complex to support multiple payloads in one IBC packet. Doing so would introduce unnecessary coupling between independent payloads. Any failure from one of the payloads can easily force the entire transaction to fail. It may be simpler to keep with the single payload model, and instead rely on multi-message transactions to handle any necessary atomicity.

soareschen · 2024-10-02T07:50:45Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ // in its place
+ // The app acknowledgement must be encoded in the same manner specified in the payload it received
+ // and must be created and processed in the manner expected by the version specified in the payload.
+ appAcknowledgement: [bytes]


What is the semantics if the acknowledgement list has a different size than the original payloads count? If there are less, do the application receive a default sentinel ack? If there are more, do the extra acks get ignored?

Is the handling of acks meant to be also atomic, similar to receiving packets? If one of the ack processing failed, how would it affect other ack handlers? Similarly, how do we handle timeouts if one of the application handler failed?

Do we need to lower the protocol assumption, so that applications are designed to handle the case when neither ack nor timeout is called for a packet?

I think that an acknowledgement should be rejected if if it has a different size than the original payload count. I believe it would fail during packet acknowledgement verification if this was the case anyway.

Regarding atomicity, my understanding of how this should work is that they are not atomic, (we could have some failures and some successes, they would not all share the same result).

However I think if a single application was async, the sending chain should be able to acknowledge the acknowledgement until all async acks have been written. (The receiving chain would not write the acknowledgement until the final application has written the async ack)

chatton

Thanks for the great work on this @AdityaSripal , I think the Packet structure looks good and is intuitive. I just had some concerns around the multi ack structure, and think we should take this opportunity to add a bit more structure if possible.

chatton · 2024-10-02T14:06:44Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ // in its place
+ // The app acknowledgement must be encoded in the same manner specified in the payload it received
+ // and must be created and processed in the manner expected by the version specified in the payload.
+ appAcknowledgement: [bytes]


I think that an acknowledgement should be rejected if if it has a different size than the original payload count. I believe it would fail during packet acknowledgement verification if this was the case anyway.

Regarding atomicity, my understanding of how this should work is that they are not atomic, (we could have some failures and some successes, they would not all share the same result).

However I think if a single application was async, the sending chain should be able to acknowledge the acknowledgement until all async acks have been written. (The receiving chain would not write the acknowledgement until the final application has written the async ack)

chatton · 2024-10-02T14:08:56Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+An application may not need to return an acknowledgment. In this case, it may return a sentinel acknowledgement value `SENTINEL_ACKNOWLEDGMENT` which will be the single byte in the byte array: `bytes(0x01)`. In this case, the IBC `acknowledgePacket` handler will still do the core IBC acknowledgment logic but it will not call the application's acknowledgePacket callback.
+
+```typescript
+interface Acknowledgement {


regarding the [bytes] type, for the appAcknowledgement, is it necessary to be pure bytes? Would there be a benefit to some sort of enum e.g. Failure,Success,Async,Sentinel/NOOP ?

EDIT: I noticed soareschen mentioned a point about an explicit format, I guess this is the same point.

chatton · 2024-10-02T14:11:57Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ // sourcePort identifies the sending application on the source chain
+ sourcePort: bytes,
+ // destPort identifies the receiving application on the dest chain
+ destPort: bytes,


I think I also prefer referring to this as Application rather than port.

chatton · 2024-10-02T14:14:37Z

spec/core/v2/ics-004-packet-semantics/PACKET.md

+ appData: bytes,
+}
+
+enum Encoding {


I tend to agree!

add packet and acknowledgement structs and commitment details

ad8c2db

AdityaSripal requested review from angbrav, colin-axner, damiannolan and sangier as code owners September 23, 2024 09:53

sangier reviewed Sep 23, 2024

View reviewed changes

sangier approved these changes Sep 23, 2024

View reviewed changes

add CBOR as suggested encoding and SENTINEL_ACKNOWLEDGEMENT value

59c848d

sangier reviewed Sep 24, 2024

View reviewed changes

spec/core/v2/ics-004-packet-semantics/PACKET.md Show resolved Hide resolved

explain each field in the code

c7b2e6d

soareschen reviewed Sep 25, 2024

View reviewed changes

Farhad-Shabani reviewed Sep 25, 2024

View reviewed changes

AdityaSripal commented Sep 26, 2024

View reviewed changes

AdityaSripal requested review from chatton, srdtrk and gjermundgaraba September 30, 2024 09:31

soareschen reviewed Sep 30, 2024

View reviewed changes

This was referenced Oct 1, 2024

Add v2 Packet definition to v2 channel protos cosmos/ibc-go#7347

Closed

Investigate addition of IBCModule v2 application interface cosmos/ibc-go#7359

Open

soareschen reviewed Oct 2, 2024

View reviewed changes

chatton reviewed Oct 2, 2024

View reviewed changes

bznein mentioned this pull request Oct 2, 2024

chore: add Acknowledgement message proto. cosmos/ibc-go#7381

Merged

10 tasks

This was referenced Oct 2, 2024

Consider whether to remove IBC acknowledgement in IBC v2 #1150

Open

Generalized v2 IBC payload header structure #1151

Open

change id to channel

0c2f344

		version: string,
		// encoding allows the sending application to specify which

		The timeout is the UNIX timestamp in seconds that must be passed on the destination chain before the packet is invalid and no longer capable of being received. Note that the timeout timestamp is assessed against the destination chain's clock which may drift relative to the clocks of the sender chain or a third party observer. If a packet is received on the destination chain after the timeout timestamp has passed relative to the destination chain's clock; the packet must be rejected so that it can be safely timed out and reverted by the sender chain.

		// sourcePort identifies the sending application on the source chain
		sourcePort: bytes,

Add Packet and Acknowledgement spec to ICS4 along with Commitments #1149

Are you sure you want to change the base?

Add Packet and Acknowledgement spec to ICS4 along with Commitments #1149

Conversation

AdityaSripal commented Sep 23, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sangier 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

AdityaSripal Sep 27, 2024 • 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

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

Farhad-Shabani 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

Farhad-Shabani Sep 25, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soareschen commented Sep 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

soareschen commented Sep 27, 2024

Choose a reason for hiding this comment

soareschen commented Oct 1, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chatton 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

AdityaSripal commented Sep 23, 2024 •

edited

Loading

AdityaSripal Sep 27, 2024 •

edited

Loading

Farhad-Shabani Sep 25, 2024 •

edited

Loading

soareschen commented Sep 26, 2024 •

edited

Loading

soareschen commented Oct 1, 2024 •

edited

Loading