Implementation side Feedback

[mossid]

This issue is for documenting the difference between the spec and the current implementation. Discussion will be followed to determine which part we want to merge back to the spec and which part we don't want.

NOTE: the current implementation code is based on pre-1.0.0 spec

ICS 23

The implementation can be found here: cosmos/cosmos-sdk#4515

1. verifyMembership / verifyNonmembership distinction

In the spec, the functions are defined separately and takes value only if it is verifyMembership. In the implementation, there is one method Verify and it takes nil as its value if it is an absent proof, which means there is no type distinction between membership proofs and non membership proofs. This approach is easier on implementation(as nil means empty value on KVStore.Get() too), but also they are semantically equivalent and type distinction in spec is better for its purpose, I suggest to not merge back to spec.

2. verify* takes key as argument

verify* functions takes both key and value as argument in order to prove the proof. In the implementation, in order to store the proofs in a map, the proofs already contains its key and the Verify() function takes only the value. Like as above, this is also equivalent to the spec.

3. Add Path to the types

Paths are a prefix distinguisher for the keys. In many cases IBC logic will not be able to occupy the key format that is declared in the specification. For example in Cosmos-SDK case the merkle proof will be prefixed with a RootMultistore proof.

Path can be understood as a prefix on keys. For a single KVStore with multiple IBC modules, each module can be stored under prefix "IBC{index}", preventing collision of the keys. The counterparties of the modules will store Path with "IBC{index}", which will be prepended to the specified keys.

The verify* functions now also have to take Path in addition to the Root, key, value. The "kind" of Proof and Path should match(merkle.Proof should only be used with merkle.Path).

The Path should be, I think, unique per connection. If we store Path per client, this will make the light clients have more than one purpose(verifying the counterparty consensus). ICS 02 Client should be only working as a light client for other chains, without storing any other information.

ICS 02

The implementation can be found here: cosmos/cosmos-sdk#4516

1. May rename client* => verifier*?

The name "client" sometimes conflicts with the client side command line tools, not an importand suggestion.

ICS 03

The implementation can be found here: cosmos/cosmos-sdk#4517

1. Separation between connection elements

type ConnectionEnd is defined as

interface ConnectionEnd {
  state: ConnectionState
  counterpartyConnectionIdentifier: Identifier
  clientIdentifier: Identifier
  counterpartyClientIdentifier: Identifier
  nextTimeoutHeight: uint64
}

However elements in this data type is splitted in the implementation.

Also, considering that non-handshake connections(e.g. broadcaster) will be added later, it might be better to separate handshake logic from the core connection fields, and reserve a field for indicating connection type.

So It can be modified as:

1. Connection Connection under "connection/{connectionIdentfier}"
2. Available bool under "connection/{connectionIdentifier}/available"
3. ConnectionKind string/enum under "connection/{connectionIdentnfier}/kind"
4. State under "connection/{connectionIdentifier}/state"
5. CounterpartyClient under "connection/{connectionIdentifier}/counterpartyClient"
6. NextTimeout uint under "connection/{connectionIdentifier}/nextTimeout"

where Connection is (ClientIdentifier, CounterpartyConnectionIdentifier). Available MUST be equal with State == Opened || State == TryClose.

For all kinds of connections, once it became Available, the only information the user have to access is Available and Connection. Connection and ConnectionKind should not be modified after set. CounterpartyClient, NextTimeout should not be relevant after the opening handshake is finished.

2. getConsensusState inspects past state

In the current definition, getConsensusState has two use cases: inspecting the current height of this chain, and inspecting one of its past consensus states.

function connOpenTry(...) {
  assert(getConsensusState().getHeight() <= timeoutHeight)
  ...
  expectedConsensusState = getConsensusState()
  ...
}

The functions which use getConsensusState for the latter purpose should take an additional argument consensusStateHeight, which is provided to getConsensusState. It will return the ConsensusState of this chain from that height.

ICS 04

The implementation can be found here: cosmos/cosmos-sdk#4548

1. Separation between channel elements

Equivalent to ICS 03 feedback, except for the CounterpartyClient as it doesn't exist in Channels. ChannelEnd will be defined as (PortIdentifier, CounterpartyChannelIdentifier, CounterpartyPortIdentifier, ConnectionHops). NextSequenceReceive, NextSequenceSend, PacketCommitment will not be effected.

2. ConnectionHops should be list of length 1, not 2

ConnectionHops should be the list of connection that can reach to the origin chain. For example, If there is a channel for chains A-B-C-D, then the ConnectionHops should be ChainAonB-ChainBonC-ChainConD in chain D's perspective, and ChainDonC-ChainConB-ChainBonA in chain A's perspective. In general case the len(ConnectionHops) should be len(chaind)-1.

ICS 25

// Not finished yet

[cwgoes]

The functions which use getConsensusState for the latter purpose should take an additional argument consensusStateHeight, which is provided to getConsensusState. It will return the ConsensusState of this chain from that height.

Are you saying that we want to accept past consensus states in certain cases in the handshakes?

(I agree)

[mossid]

I think this is inevitable, in the handshake process, chain A at height H can only verify the consensus state of itself from height <H stored in chain B, unless two chains are working fully synchronous, so the chain have to verify the existence of one of its past consensus state on the counterparty. Also see: cosmos/cosmos-sdk#4647

[cwgoes]

I think this is inevitable, in the handshake process, chain A at height H can only verify the consensus state of itself from height <H stored in chain B, unless two chains are working fully synchronous, so the chain have to verify the existence of one of its past consensus state on the counterparty. Also see: cosmos/cosmos-sdk#4647

Concurred; does 3f45935 match more or less what you have?

[cwgoes]

@mossid Is the OP up-to-date with the latest spec?

Alternatively, besides CommitmentPath, is there anything here you think should be in rc3?

[mossid]

Separations between connection/channel elements is the only thing not yet included, but can be included in a latter rc(if exists, if not then should be in rc3).

[cwgoes]

There will be a latter rc, I expect. PRs welcome! (or we can discuss, as you prefer)

[cwgoes]

Separations between connection/channel elements is the only thing not yet included, but can be included in a latter rc(if exists, if not then should be in rc3).

Hmm - why is this advantageous?

It means that multiple proofs are required during handshakes, which seems unfortunate.

I guess it reduces the amount of data read during some parts of packet handling. Is that the reason?

[cwgoes]

We should separate out only state which has to be read individually.

[mossid]

Current implementation-side separation, client/connection/channel prefix ignored:

02

• id => latest consensus state (immutable)
• id + "/roots/" => state roots : (uint64 => CommitmentRoot) (part of the client state, privatestore) (mutable)
• id + "/freeze" => frozen : bool (part of the client state, privatestore) (mutable)

03

persistent

• id => connection end (immutable)
• id + "/available" => availability : bool (set true once handshake established) (immutable, connections cannot be closed)
• id + "/kind" => connection kind : string (immutable)

handshake

• id + "/state" => handshake state : enum (mutable, prunable)
• id + "/counterpartyClient" => counterparty client identifier (mutable, prunable)

04

persistent

• id => channel end (immutable)
• id + "/available" => availability : bool (mutable)
• id + "/nextSequenceSend" => sending sequence : uint64 (mutable)
• id + "/nextSequenceRecv" => receiving sequence : uint64 (mutable)
• id + "/packet/" => packets : (uint64 => Packet) (mutable)
• (ports are not stored in state but managed by objcap, privatestore)

handshake

• id + "/state" => handshake state : enum (mutable, prunable)

[mossid]

Also in ICS 025: we have clauses for the privatestore that it

MAY support external proofs, but is not required to - the IBC handler will never write data to it which needs to be proved.
MAY use canonical proto3 data structures, but is not required to - it can use whatever format is preferred by the application environment.

but it is MUST for the key format:

At present, IBC/TAO utilises the following path prefixes for the provableStore and privateStore. Future paths may be used in future versions of the protocol, so the entire key-space in both stores MUST be reserved for the IBC handler.

And the key format also need to be restricted in MAY level in case of the privatestore.

[cwgoes]

I generally like the store layout - two questions:

• Don't channel orderings need to be retrieved to handle timeouts?
• What is a "connection kind"?

[mossid]

• Don't channel orderings need to be retrieved to handle timeouts?

The current implementation does not include the unordered channel but it will.

• What is a "connection kind"?

Type of the connection {handshake, broadcastint, subscription, ...}. Not in the spec, added for possible future compatability

[cwgoes]

@mossid FYI the store layout will now be standardised per-client instead of in the spec - #288

[cwgoes]

@mossid Does the store layout above still reflect the implementation? What remains that we need to agree upon between the spec & implementation (w.r.t. this issue)?

[cwgoes]

Bump @mossid (also @fedekunze - I recall you mentioned a question about store keys & iterations, and a desire to change the layout a bit).

[fedekunze]

Bump @mossid (also @fedekunze - I recall you mentioned a question about store keys & iterations, and a desire to change the layout a bit).

Yeah, one problem is that the store keys conflict with each other since there's no prefix that you can use to use iterators and thus aggregate the data.

[cwgoes]

Bump @mossid (also @fedekunze - I recall you mentioned a question about store keys & iterations, and a desire to change the layout a bit).

Yeah, one problem is that the store keys conflict with each other since there's no prefix that you can use to use iterators and thus aggregate the data.

That can be fixed - do you want to write up the new suggested store layout or should I?

[cwgoes]

I am assuming that the store layout changes (made by #332) are the only remaining actionable here.

Please re-open if there are other spec changes which have not yet been made.