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.

Sign up for GitHub

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

extend 03-connection with a proof object for chains that can't introspect their own consensus state. #839

Merged
merged 2 commits into from
Nov 30, 2022
+14 −6
Merged

extend 03-connection with a proof object for chains that can't introspect their own consensus state. #839

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
18fca9c
extend 03-connection with HostConsensusStateProof
seunlanlege Sep 14, 2022
924800f
make hostConsensusStateProof optional
seunlanlege Sep 26, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
6 changes: 4 additions & 2 deletions spec/core/ics-003-connection-semantics/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -360,13 +360,14 @@ function connOpenTry(
proofClient: CommitmentProof,
proofConsensus: CommitmentProof,
proofHeight: Height,
proofHostConsensusState: CommitmentProof,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to signal that this is optional?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also my understanding is that in your implementation the proof field here is actually a marshalled header.

This is the flow from my understanding:

  1. You pass in the marshalled header here in proofHostConsensusState
  2. in your handler you check that the passed in header hashes to one of your stored header hashes
  3. you create a consensus state out of the header
  4. you prove that the counterparty stored that consensus state in its client.

So this field shouldn't actually contain a CommitmentProof object. Instead it contains some arbitrary chain-specific data, that will be used to verify proofConsensus that is passed in above.

So I think instead of referring to it this way, it makes more sense to call this something like:

hostConsensusInfo: bytes

or something similar, and to signal in documentation that this is optional for chains to accept.

Copy link
Contributor Author

@seunlanlege seunlanlege Sep 19, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a way to signal that this is optional?

yeah there is

hostConsensusInfo: bytes

how about hostConsensusStateProof?: bytes

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still think it shouldn't be named proof. Because the object in question is not actually a proof.

It is metadata that is constructing the value that will get proven agains the consensusStateProof which is a separate argument to this function.

Open to a different name other than hostConsensusInfo but I do think we should avoid Proof in the name since semantically it doesn't match with the way we're using Proof arguments in IBC spec functions

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any other information a chain may request to extend the connection handshake info with? Or is this the only expected addition?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the only addition

consensusHeight: Height) {
// generate a new identifier
identifier = generateIdentifier()

abortTransactionUnless(validateSelfClient(clientState))
abortTransactionUnless(consensusHeight < getCurrentHeight())
expectedConsensusState = getConsensusState(consensusHeight)
expectedConsensusState = getConsensusState(consensusHeight, proofHostConsensusState)
expectedConnectionEnd = ConnectionEnd{INIT, "", getCommitmentPrefix(), counterpartyClientIdentifier,
clientIdentifier, counterpartyVersions, delayPeriodTime, delayPeriodBlocks}

Expand Down Expand Up @@ -396,13 +397,14 @@ function connOpenAck(
proofTry: CommitmentProof,
proofClient: CommitmentProof,
proofConsensus: CommitmentProof,
proofHostConsensusState: CommitmentProof,
proofHeight: Height,
consensusHeight: Height) {
abortTransactionUnless(consensusHeight < getCurrentHeight())
abortTransactionUnless(validateSelfClient(clientState))
connection = provableStore.get(connectionPath(identifier))
abortTransactionUnless((connection.state === INIT && connection.version.indexOf(version) !== -1)
expectedConsensusState = getConsensusState(consensusHeight)
expectedConsensusState = getConsensusState(consensusHeight, proofHostConsensusState)
expectedConnectionEnd = ConnectionEnd{TRYOPEN, identifier, getCommitmentPrefix(),
connection.counterpartyClientIdentifier, connection.clientIdentifier,
version, connection.delayPeriodTime, connection.delayPeriodBlocks}
Expand Down
8 changes: 6 additions & 2 deletions spec/core/ics-024-host-requirements/README.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ requires: 23
required-by: 2, 3, 4, 5, 18
author: Christopher Goes <cwgoes@tendermint.com>
created: 2019-04-16
modified: 2022-07-05
modified: 2022-09-14
---

## Synopsis
Expand Down Expand Up @@ -170,11 +170,15 @@ Host state machines MUST define a unique `ConsensusState` type fulfilling the re
Host state machines MUST provide the ability to introspect their own consensus state, with `getConsensusState`:

```typescript
type getConsensusState = (height: Height) => ConsensusState
type getConsensusState = (height: Height, proof?: []byte) => ConsensusState
```

`getConsensusState` MUST return the consensus state for at least some number `n` of contiguous recent heights, where `n` is constant for the host state machine. Heights older than `n` MAY be safely pruned (causing future calls to fail for those heights).

We provide an optional proof object which comes from the `MsgConnectionOpenAck` or `MsgConnectionOpenTry` for host state machines which are unable to introspect their own `ConsensusState` and must rely on off-chain data.
<br />
In this case host state machines MUST maintain a map of `n` block numbers to header hashes where the proof would contain full header which can be hashed and compared with the on-chain record.

Host state machines MUST provide the ability to introspect this stored recent consensus state count `n`, with `getStoredRecentConsensusStateCount`:

```typescript
Expand Down