Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Implementation Feedback #145

Open
mossid opened this issue Jun 30, 2019 · 3 comments

Comments

@mossid
Copy link
Collaborator

commented Jun 30, 2019

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.

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.

  1. 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. Root can be updated

In the implementation, merkle.Root(which implements Root) is defined as a pair of (keypath merkle.KeyPath, keyprefix []byte, hash []byte). This is for recognising a subnode of the merkle tree as its root - any proof that is verified over the root will automatically prefixed with keypath and keyprefix.

For example, if the IBC module in the SDK is under the rootmultistore with storekey "ibc", and the key prefix for all the key-value pairs is []byte("v1/"). In this case the root will be merkle.Root{keypath: merkle.KeyPath{"ibc"}), keyprefix: []byte("v1/"), hash: (...)}If a proof {Key: []byte("clients/clientid/consensusState"), Proof: (...)} is verified over this root, the root will automatically construct a merkle.KeyPath{"ibc", []byte("v1/clients/clientid/consensusState")} and provide that keypath to the proof.

This enables to run IBC module with other applications(including other IBC module instances) inside a single machine, while consistently using a single key format for proofs.

However, the Header coming from the other chain might not include the full merkle root data. For example, tendermint headers only contains AppHash, which corresponds to merkle.Root.hash, but not the other two fields. This means we also have to introduce the duality of ConsensusState - Header in ICS 02, where the former is the data stored on chain and the latter is the data used to update the ConsensusState. In the implementation, Root has a method Update([]byte) Root which takes a bytestring and returns updated Root(so there is no new type defined for updating part for now).

The spec can reflect this change, possibly using a unified naming convention over State - Update pattern. (ConsensusState - ConsensusStateUpdate, CommitmentRoot - CommitmentRootUpdate?)

NOTE: I'm trying to separate the keypath logic from the ICS 23 and put it in the ICS 03. This makes much more sense, considering the case of multiple IBC instances on a chain, because in that case having KeyPath in the merkle.Root will force the counterparty to store multiple clients for each of the IBC instances. Having the keypath in the ICS 03 will eliminate this problem. ICS 02 Client should be only work as a light client for other chains, without storing any other information, and having KeyPath in ICS 23 Root will break this assumption.

  1. 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

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.

  1. 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). NextSequenceReceive, NextSequenceSend, PacketCommitment will not be effected.

ICS 25

// Not finished yet

@mossid mossid changed the title ICS 23 Implementation Feedback Implementation Feedback Jun 30, 2019

@cwgoes cwgoes added this to the IBC "1.0" milestone Jul 11, 2019

@cwgoes cwgoes self-assigned this Jul 11, 2019

@cwgoes

This comment has been minimized.

Copy link
Collaborator

commented Jul 29, 2019

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

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 30, 2019

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 cwgoes referenced this issue Jul 30, 2019

Merged

Start 1.0.0-rc1 #157

cwgoes added a commit that referenced this issue Jul 30, 2019

@cwgoes

This comment has been minimized.

Copy link
Collaborator

commented Jul 30, 2019

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?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.