-
Notifications
You must be signed in to change notification settings - Fork 383
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
Update paths for consistency; revise ICS 2 for client type sub-specs #348
Changes from 10 commits
85f6e64
695fdf1
b5ca351
2fd4010
67d7e69
db96e9d
5d94828
19c6190
19e3ef2
c6e6eb5
6455a84
faee6f9
7519265
24a3113
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -8,7 +8,7 @@ requires: 23, 24 | |
required-by: 3 | ||
author: Juwoon Yun <joon@tendermint.com>, Christopher Goes <cwgoes@tendermint.com> | ||
created: 2019-02-25 | ||
modified: 2019-08-25 | ||
modified: 2020-01-13 | ||
--- | ||
|
||
## Synopsis | ||
|
@@ -253,10 +253,18 @@ but they must expose this common set of query functions to the IBC handler. | |
type ClientState = bytes | ||
``` | ||
|
||
Client types must also define a method to initialize a client state with a provided consensus state: | ||
Client types MUST define a method to initialize a client state with a provided consensus state, writing to state as appropriate. | ||
|
||
```typescript | ||
type initialize = (state: ConsensusState) => ClientState | ||
type initialize = (state: ConsensusState) => () | ||
``` | ||
|
||
Client types MUST define a method to fetch the current height (height of the most recent validated header). | ||
|
||
```typescript | ||
type latestHeight = ( | ||
clientState: ClientState) | ||
=> uint64 | ||
``` | ||
|
||
#### CommitmentProof | ||
|
@@ -280,6 +288,7 @@ type verifyClientConsensusState = ( | |
height: uint64, | ||
proof: CommitmentProof, | ||
clientIdentifier: Identifier, | ||
consensusStateHeight: uint64, | ||
consensusState: ConsensusState) | ||
=> boolean | ||
``` | ||
|
@@ -420,34 +429,6 @@ type validateClientIdentifier = (id: Identifier) => boolean | |
|
||
If not provided, the default `validateClientIdentifier` will always return `true`. | ||
|
||
#### Path-space | ||
|
||
`clientStatePath` takes an `Identifier` and returns a `Path` under which to store a particular client state. | ||
|
||
```typescript | ||
function clientStatePath(id: Identifier): Path { | ||
return "clients/{id}/state" | ||
} | ||
``` | ||
|
||
`clientTypePath` takes an `Identifier` and returns `Path` under which to store the type of a particular client. | ||
|
||
```typescript | ||
function clientTypePath(id: Identifier): Path { | ||
return "clients/{id}/type" | ||
} | ||
``` | ||
|
||
Consensus states MUST be stored separately so that they can be independently verified. | ||
|
||
`consensusStatePath` takes an `Identifier` and returns a `Path` under which to store the consensus state of a client. | ||
|
||
```typescript | ||
function consensusStatePath(id: Identifier): Path { | ||
return "clients/{id}/consensusState" | ||
} | ||
``` | ||
|
||
##### Utilising past roots | ||
|
||
To avoid race conditions between client updates (which change the state root) and proof-carrying | ||
|
@@ -468,28 +449,15 @@ function createClient( | |
abortTransactionUnless(validateClientIdentifier(id)) | ||
abortTransactionUnless(privateStore.get(clientStatePath(id)) === null) | ||
abortSystemUnless(provableStore.get(clientTypePath(id)) === null) | ||
clientState = clientType.initialize(consensusState) | ||
privateStore.set(clientStatePath(id), clientState) | ||
clientType.initialise(consensusState) | ||
provableStore.set(clientTypePath(id), clientType) | ||
} | ||
``` | ||
|
||
#### Query | ||
|
||
Client consensus state and client internal state can be queried by identifier. The returned | ||
client state must fulfil an interface allowing membership / non-membership verification. | ||
|
||
```typescript | ||
function queryClientConsensusState(id: Identifier): ConsensusState { | ||
return provableStore.get(consensusStatePath(id)) | ||
} | ||
``` | ||
|
||
```typescript | ||
function queryClient(id: Identifier): ClientState { | ||
return privateStore.get(clientStatePath(id)) | ||
} | ||
``` | ||
Client consensus state and client internal state can be queried by identifier, but | ||
the specific paths which must be queried are defined by each client type. | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
why is this now defined per client? All clients need to still be able to query the consensus and client states There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The client state, yes, but the consensus state, not necessarily, e.g. for a solo machine there is not a consensus state stored for each height. Client types can be required to expose There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
👍 It'd be awesome if we could define a standard interface for all clients with the functions that they need to implement. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Aye, agreed. I'll add that. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. With regards to a standard interface, I completely agree, but the only parts of that I can reliably guess are those which will be required by relayers (querying clients), so if you want additional functions for convenience or CLI UX, let me know. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I defined a few interfaces for clients in cosmos/cosmos-sdk#5485 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah. I think I'll split that out to a separate issue & we can discuss first. #356 |
||
|
||
#### Update | ||
|
||
|
@@ -576,12 +544,13 @@ function commit( | |
} | ||
|
||
// initialisation function defined by the client type | ||
function initialize(consensusState: ConsensusState): ClientState { | ||
cwgoes marked this conversation as resolved.
Show resolved
Hide resolved
|
||
return { | ||
function initialize(consensusState: ConsensusState): () { | ||
clientState = { | ||
frozen: false, | ||
pastPublicKeys: Set.singleton(consensusState.publicKey), | ||
verifiedRoots: Map.empty() | ||
} | ||
privateStore.set(identifier, clientState) | ||
} | ||
|
||
// validity predicate function defined by the client type | ||
|
@@ -605,7 +574,7 @@ function verifyClientConsensusState( | |
proof: CommitmentProof, | ||
clientIdentifier: Identifier, | ||
consensusState: ConsensusState) { | ||
path = applyPrefix(prefix, "clients/{clientIdentifier}/consensusState") | ||
path = applyPrefix(prefix, "clients/{clientIdentifier}/consensusStates/{height}") | ||
abortTransactionUnless(!clientState.frozen) | ||
return clientState.verifiedRoots[sequence].verifyMembership(path, consensusState, proof) | ||
} | ||
|
@@ -617,7 +586,7 @@ function verifyConnectionState( | |
proof: CommitmentProof, | ||
connectionIdentifier: Identifier, | ||
connectionEnd: ConnectionEnd) { | ||
path = applyPrefix(prefix, "connection/{connectionIdentifier}") | ||
path = applyPrefix(prefix, "connections/{connectionIdentifier}") | ||
abortTransactionUnless(!clientState.frozen) | ||
return clientState.verifiedRoots[sequence].verifyMembership(path, connectionEnd, proof) | ||
} | ||
|
@@ -733,6 +702,8 @@ May 29, 2019 - Various revisions, notably multiple commitment-roots | |
|
||
Aug 15, 2019 - Major rework for clarity around client interface | ||
|
||
Jan 13, 2020 - Revisions for client type separation & path alterations | ||
|
||
## Copyright | ||
|
||
All content herein is licensed under [Apache 2.0](https://www.apache.org/licenses/LICENSE-2.0). |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -99,11 +99,14 @@ Parts of the private store MAY safely be used for other purposes as long as the | |
Keys used in the private store MAY safely vary as long as there exists a bipartite mapping between the key formats defined herein and the ones | ||
actually used in the private store implementation. | ||
|
||
Note that the client-related paths listed below reflect the Tendermint client as defined in [ICS 7](../ics-007-tendermint-client) and may vary for other client types. | ||
|
||
| Store | Path format | Value type | Defined in | | ||
| -------------- | ------------------------------------------------------------------------------ | ----------------- | ---------------------- | | ||
| privateStore | "clients/{identifier}" | ClientState | [ICS 2](../ics-002-client-semantics) | | ||
| provableStore | "clients/{identifier}/consensusState" | ConsensusState | [ICS 2](../ics-002-client-semantics) | | ||
| provableStore | "clients/{identifier}/type" | ClientType | [ICS 2](../ics-002-client-semantics) | | ||
| privateStore | "clients/{identifier}" | ClientState | [ICS 2](../ics-007-tendermint-client) | | ||
| provableStore | "clients/{identifier}/consensusStates/{height}" | ConsensusState | [ICS 7](../ics-007-tendermint-client) | | ||
| privateStore | "clients/{identifier}/connections | []Identifier | [ICS 3](../ics-003-connection-semantics) | | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. what is this path for? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. It's used for reverse lookup of what connections are using a client. |
||
| provableStore | "connections/{identifier}" | ConnectionEnd | [ICS 3](../ics-003-connection-semantics) | | ||
| privateStore | "ports/{identifier}" | CapabilityKey | [ICS 5](../ics-005-port-allocation) | | ||
| provableStore | "ports/{identifier}/channels/{identifier}" | ChannelEnd | [ICS 4](../ics-004-channel-and-packet-semantics) | | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this was confusing for me at least in the beginning. The naming of the third-party client consensus state vs the self consensus state was not clear. So whenever it said
getConsensusState(height)
it meant self consensus state (i.e block header and validator info), while thegetConsensusState(clientID, height)
meant the counterparty client.I'd suggest adding the
client
orself
prefix to theConsensusState
to clarify this item.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
or, alternatively
clientState.latestHeight()
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aye, that makes sense, I'll change the client function name to
getClientConsensusState
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, I can't find any uses of
getConsensusState
now which take aclientID
andheight
. Can you point to a specific location which is confusing?