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

feat: add consumer genesis time query #2366

Merged
merged 13 commits into from
Nov 5, 2024
Merged

feat: add consumer genesis time query #2366

merged 13 commits into from
Nov 5, 2024

Conversation

sainoe
Copy link
Contributor

@sainoe sainoe commented Oct 18, 2024

Description

Closes: #2280


Author Checklist

All items are required. Please add a note to the item if the item is not applicable and
please add links to any relevant follow up issues.

I have...

  • Included the correct type prefix in the PR title
  • Added ! to the type prefix if the change is state-machine breaking
  • Confirmed this PR does not introduce changes requiring state migrations, OR migration code has been added to consumer and/or provider modules
  • Targeted the correct branch (see PR Targeting)
  • Provided a link to the relevant issue or specification
  • Followed the guidelines for building SDK modules
  • Included the necessary unit and integration tests
  • Added a changelog entry to CHANGELOG.md
  • Included comments for documenting Go code
  • Updated the relevant documentation or specification
  • Reviewed "Files changed" and left comments if necessary
  • Confirmed all CI checks have passed
  • If this PR is library API breaking, bump the go.mod version string of the repo, and follow through on a new major release

Reviewers Checklist

All items are required. Please add a note if the item is not applicable and please add
your handle next to the items reviewed if you only reviewed selected items.

I have...

  • confirmed the correct type prefix in the PR title
  • confirmed ! the type prefix if the change is state-machine breaking
  • confirmed this PR does not introduce changes requiring state migrations, OR confirmed migration code has been added to consumer and/or provider modules
  • confirmed all author checklist items have been addressed
  • reviewed state machine logic
  • reviewed API design and naming
  • reviewed documentation is accurate
  • reviewed tests and test coverage

@github-actions github-actions bot added C:x/provider Assigned automatically by the PR labeler C:Docs Assigned automatically by the PR labeler labels Oct 18, 2024
@sainoe sainoe marked this pull request as ready for review October 18, 2024 12:38
@sainoe sainoe requested a review from a team as a code owner October 18, 2024 12:38
@sainoe sainoe marked this pull request as draft October 18, 2024 12:51
@sainoe sainoe marked this pull request as ready for review October 18, 2024 15:10
cs, ok := k.clientKeeper.GetClientConsensusState(
ctx,
clientID,
params.InitialHeight,
Copy link
Contributor

Choose a reason for hiding this comment

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

Aren't old consensus states pruned? If yes, this query would stop working after some time.

Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure, needs checking.

However, we can make it so that the query runs for chains that haven't yet started (passed spawntime).

Copy link
Contributor

Choose a reason for hiding this comment

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

Those consumers will not have a client and thus no genesis time.

Copy link
Contributor

Choose a reason for hiding this comment

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

@insumity The reason for this query is starting the consumer chain -- creating the genesis file with a correct timestamp that will not result in expired consumer clients on the provider. The consensus state will not be pruned at least until a new client update is received by the provider (otherwise the client update will fail). By this time, the consumer chain is already running so the query is no longer needed.

Copy link
Contributor

@MSalopek MSalopek Nov 5, 2024

Choose a reason for hiding this comment

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

I thought he was saying that after starting a chain (making it run, with relaying enabled) the initial height consensus state would get pruned. Thus, state at block 1 would not be queryable from GetClientConsensusState after a while.

Copy link
Contributor

Choose a reason for hiding this comment

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

Consensus states are eventually pruned, but it takes a while. Otherwise client updates would not work. So for sure, the initial consensus state will remain on the provider for a while. The important part is that it will remain there for sure until either the consumer chain launches or the consumer light client on the provider expires.

Copy link
Contributor

Choose a reason for hiding this comment

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

I see. Thanks for the explanation.

Copy link
Contributor

@mpoke mpoke left a comment

Choose a reason for hiding this comment

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

Great work @sainoe.

@mpoke mpoke enabled auto-merge November 5, 2024 15:06
@mpoke mpoke added this pull request to the merge queue Nov 5, 2024
Merged via the queue into main with commit 00ba140 Nov 5, 2024
20 checks passed
@mpoke mpoke deleted the sainoe/genesis-time branch November 5, 2024 15:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C:Docs Assigned automatically by the PR labeler C:x/provider Assigned automatically by the PR labeler
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add query for the creation time of consumer clients
4 participants