24-host: Reduce provable surface area to only packet flow keys #1144
Conversation
…ovable keys to private keys
AdityaSripal
requested review from
angbrav,
colin-axner,
damiannolan and
sangier
as code owners
| @@ -311,16 +186,6 @@ This permissioning can be implemented with unique references (object capabilitie | |||
|
|
|||
| Modules that wish to make use of particular IBC features MAY implement certain handler functions, e.g. to add additional logic to a channel handshake with an associated module on another state machine. | |||
|
|
|||
| ### Datagram submission | |||
There was a problem hiding this comment.
Just curious: why was this section deleted?
|
Leaving open for now because we may want to remove specification of private keys entirely. it is up to individual implementations to decide how best to execute the logic required to write the provable keys in store. Also, we should be standardizing the values stored under the provable keys in this specification as well. Also blocked on discussion around spec format. i.e. ABI for universal data format and moving to IETF standards for specs |
|
|
||
| #### Packet Acknowledgement | ||
|
|
||
| The acknowledgement will be provided with the packet to the sending chain. Thus we only need to provably associate the acknowledgement with the original packet. This association is already accomplished by the acknowledgment path which contains the destination identifier and the sequence. Thus on the sending chain, we can prove the acknowledgement was indeed sent for the packet we sent. We prove the packet was sent by us by checking that we stored the packet commitment under the packet commitment path. We can retrieve the client from the source identifier and then prove the counterparty stored under the destination identifier and sequence. Thus, we can associate the acknowledgement stored under this path with the unique packet provided. The acknowledgement commitment can therefore simply consist of a hash of the acknowledgment data sent to the state machine. |
There was a problem hiding this comment.
Would be nice to have a visual representation of this
| actually used in the private store implementation. | ||
| | Store | Path format | Value type | Defined in | | ||
| | -------------- | -------------------------------------------------------------------------- | ----------------- | ---------------------- | | ||
| | provableStore | "commitments/channels/{identifier}/sequences/{bigEndianUint64Sequence}" | bytes | [ICS 4](../ics-004-packet-semantics) | |
There was a problem hiding this comment.
I'm wondering if there is some space for commitment collision which could led to double usage, but I cannot really think of a scenario that would allow that. At a first glance the provableStore paths look good to me.
| | Store | Path format | Value type | Defined in | | ||
| | -------------- | -------------------------------------------------------------------------- | ----------------- | ---------------------- | | ||
| | provableStore | "commitments/channels/{identifier}/sequences/{bigEndianUint64Sequence}" | bytes | [ICS 4](../ics-004-packet-semantics) | | ||
| | provableStore | "receipts/channels/{identifier}/sequences/{bigEndianUint64Sequence}" | bytes | [ICS 4](../ics-004-packet-semantics) | | ||
| | provableStore | "acks/channels/{identifier}/sequences/{bigEndianUint64Sequence}" | bytes | [ICS 4](../ics-004-packet-semantics) | |
There was a problem hiding this comment.
Looks like we're missing "nextSequenceSend/ports/{identifier}/channels/{identifier}"?
I guess it doesn't need to be provable, but it should also still be defined?? I think we still want the logical separation of sequence/nonce under its own key in v2. And the current impl still relies on port identifier being included
There was a problem hiding this comment.
Since it won't be provable, but the ICS4 still uses it, I have defined the
function nextSequenceSendPath(sourceChannelID: bytes): Path {
return "nextSequenceSend/{sourceChannelID}"
}
``` in the privateStore. However, being local and private it could be any path that the chains want to set, thus I guess the implementation should be able to avoid relying on the port, correct?
| | Store | Path format | Value type | Defined in | | ||
| | -------------- | -------------------------------------------------------------------------- | ----------------- | ---------------------- | | ||
| | provableStore | "commitments/channels/{identifier}/sequences/{bigEndianUint64Sequence}" | bytes | [ICS 4](../ics-004-packet-semantics) | | ||
| | provableStore | "receipts/channels/{identifier}/sequences/{bigEndianUint64Sequence}" | bytes | [ICS 4](../ics-004-packet-semantics) | |
There was a problem hiding this comment.
seems that sequences will be the main discriminator here, thus would be possible to use a path like "receipts/{identifier}/sequences/{bigEndianUint64Sequence}" so deleting the channels reference?
Context for Reviewers:
This PR introduces a new high level packet and acknowledgement commitment.
The packet commitment must now include the destination identifier
The packet commitment hashes the destination identifier, timeout, and app data all as bytes to create a fixed preimage size and then hashes the result
The acknowledgement is just a hash of the acknowledgement
Closes: #1138