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

ICS03: Historical header module for Handshaking #5078

Closed
mossid opened this issue Sep 20, 2019 · 12 comments · Fixed by #5340
Closed

ICS03: Historical header module for Handshaking #5078

mossid opened this issue Sep 20, 2019 · 12 comments · Fixed by #5340
Assignees

Comments

@mossid
Copy link
Contributor

mossid commented Sep 20, 2019

Connection handshaking requires to check the existence of a header of the verifying chain on the verified chain. Opening handshake under ICS03

function connOpenTry(
  desiredIdentifier: Identifier,
  counterpartyConnectionIdentifier: Identifier,
  counterpartyPrefix: CommitmentPrefix,
  counterpartyClientIdentifier: Identifier,
  clientIdentifier: Identifier,
  counterpartyVersions: string[],
  proofInit: CommitmentProof,
  proofHeight: uint64,
  consensusHeight: uint64) {
    ...
    expectedConsensusState = getConsensusState(consensusHeight)
    ...
    abortTransactionUnless(
      connection.verifyMembership(proofHeight, proofInit,
                                  consensusStatePath(counterpartyClientIdentifier),
                                  expectedConsensusState))
    ...
}

This requires the verifying chain able to introspect its own headers. ICS02 module needs to hold the reference to the historical header module and retrieve headers from recent n heights.

Dependent on: #4647
Replaces [^2] in #4723

@mossid mossid added the x/ibc label Sep 20, 2019
@mossid mossid mentioned this issue Oct 1, 2019
6 tasks
@alexanderbez alexanderbez added this to the IBC Implementation & Integration milestone Oct 1, 2019
@fedekunze
Copy link
Collaborator

I'm not sure what are the actionable items here. cc: @cwgoes

@cwgoes
Copy link
Contributor

cwgoes commented Nov 13, 2019

We need internal header introspection in SDK state (or retrieved from Tendermint).

@fedekunze
Copy link
Collaborator

We need internal header introspection in SDK state (or retrieved from Tendermint).

I assume the introspection is related to retrieving past light client headers?

@cwgoes
Copy link
Contributor

cwgoes commented Nov 14, 2019

Past Tendermint headers (which the light client uses, yes) for the running chain.

@fedekunze
Copy link
Collaborator

ok, yeah this def needs an ADR. cc: @mossid @jackzampolin

@jackzampolin
Copy link
Member

@mossid do you have some bandwidth to write up this ADR?

@cwgoes cwgoes assigned cwgoes and unassigned mossid Nov 25, 2019
@cwgoes
Copy link
Contributor

cwgoes commented Nov 26, 2019

There is no way for the application to retrieve past headers from Tendermint through ABCI, right?

I am writing an ADR for the SDK right now and thinking that it might be easiest to just store duplicate data in the store (for the past n heights - validator set and commit hash, basically, the same data the light client keeps), but if there is some way for the application to query this from Tendermint (which stores it I presume) that would be more space-efficient

@cwgoes
Copy link
Contributor

cwgoes commented Nov 26, 2019

Suggestion by @zmanian - RPC queries to 26657 from the application

I am a bit concerned about the reliability of this approach.

@cwgoes
Copy link
Contributor

cwgoes commented Nov 26, 2019

Options

Retaining deliver-only direction (@warner):

  • App could instruct Tendermint to send last n headers with every BeginBlock
  • Could be more efficient (send n initially, then 1 each time)

Whenever the process is started, n headers need to be sent (app does not persist).

App could also instruct on return of BeginBlock to send more headers & retry.

@melekes
Copy link
Contributor

melekes commented Nov 27, 2019

Suggestion by @zmanian - RPC queries to 26657 from the application
I am a bit concerned about the reliability of this approach.

Some applications do that. Should be okay if you don't expose RPC to the outer world (i.e. i'd say reliability is same in such case).

@cwgoes
Copy link
Contributor

cwgoes commented Nov 27, 2019

Suggestion by @zmanian - RPC queries to 26657 from the application
I am a bit concerned about the reliability of this approach.

Some applications do that. Should be okay if you don't expose RPC to the outer world (i.e. i'd say reliability is same in such case).

We depend on this in consensus though - if anyone disabled RPC in the config, used any sort of authentication, or there were any sort of HTTP errors, the process would crash.

@cwgoes
Copy link
Contributor

cwgoes commented Dec 2, 2019

Outstanding question: validator sets vs. validator set hashes. We should only need to verify that the hashes match, not store / track the entire validator set.

@fedekunze fedekunze modified the milestones: IBC Implementation & Integration, IBC Implementation Dec 10, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants