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

Fix 24/getConsensusState #187

Merged
merged 2 commits into from Aug 8, 2019

Conversation

@mossid
Copy link
Collaborator

commented Aug 8, 2019

Under getConsensusState,

getConsensusState MUST return the consensus state for the consensus algorithm of the host chain at the specified height, for all heights greater than zero and less than or equal to the current height.

For some cases getConsensusState will not be able to return the consensus state with height of 0 < _ < current. For efficiency, the chains might choose to prune the states and (if the pruning happens for past enough states) the impl will still work, so the restriction should be RECOMMENDED.

mossid added some commits Aug 8, 2019

Fix 24/getConsensusState
Under `getConsensusState`,

> `getConsensusState` MUST return the consensus state for the consensus algorithm of the host chain at the specified height, for all heights greater than zero and less than or equal to the current height.

For some cases `getConsensusState` will not be able to return the consensus state with height of `0 < _ < current`. For efficiency, the chains might choose to prune the states and (if the pruning happens for past enough states) the impl will still work, so the restriction should be RECOMMENDED.
@cwgoes

This comment has been minimized.

Copy link
Collaborator

commented Aug 8, 2019

For some cases getConsensusState will not be able to return the consensus state with height of 0 < _ < current. For efficiency, the chains might choose to prune the states and (if the pruning happens for past enough states) the impl will still work, so the restriction should be RECOMMENDED.

I agree; this will impose some synchrony constraints beyond what might already be imposed, though, so the chain will need to specify the number of past consensus states which it keeps accessible (some constant n).

@cwgoes

cwgoes approved these changes Aug 8, 2019

Copy link
Collaborator

left a comment

ACK, I will incorporate this into the arguments about safety / liveness elsewhere.

@cwgoes cwgoes merged commit 780b3c9 into master Aug 8, 2019

5 checks passed

ci/circleci: check_dependencies Your tests passed on CircleCI!
Details
ci/circleci: check_links Your tests passed on CircleCI!
Details
ci/circleci: check_proto Your tests passed on CircleCI!
Details
ci/circleci: check_sections Your tests passed on CircleCI!
Details
ci/circleci: check_syntax Your tests passed on CircleCI!
Details

@cwgoes cwgoes deleted the joon/getConsensusState-fix branch Aug 8, 2019

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.