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

refactor: check client status active when verify consensus state #1098

Draft
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

hoank101
Copy link

@hoank101 hoank101 commented Feb 22, 2024

Closes: #1088

Description


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests.
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).
  • Tagged one reviewer who will be the one responsible for shepherding this PR.

Reviewer checklist:

  • Reviewed Files changed in the GitHub PR explorer.
  • Manually tested (in case integration/unit/mock tests are absent).

@rnbguy
Copy link
Collaborator

rnbguy commented Feb 22, 2024

Thanks for your PR, @hoank101 ! But this issue is a bit nuanced, and my suggestion on the issue is was misleading.

is_active call on unstored client_id will return Frozen or Expired. Expired case happens because there is no consensus state present for that client_id - which is assumed to be pruned.

That's why you needed to insert a consensus state before executing MsgCreateClient. But that's not possible on live chain - and, this logic will always fail.

We need to check for the Expired status for the proved ConsensusState locally. To execute this nicely will require some careful changes in the traits.

@hoank101
Copy link
Author

Thanks for your PR, @hoank101 ! But this issue is a bit nuanced, and my suggestion on the issue is was misleading.

is_active call on unstored client_id will return Frozen or Expired. Expired case happens because there is no consensus state present for that client_id - which is assumed to be pruned.

That's why you needed to insert a consensus state before executing MsgCreateClient. But that's not possible on live chain - and, this logic will always fail.

We need to check for the Expired status for the proved ConsensusState locally. To execute this nicely will require some careful changes in the traits.

thank you for feedback, i will update logic

@rnbguy
Copy link
Collaborator

rnbguy commented Feb 22, 2024

Note how it's being handled in ibc-go here. The status is checked after the consensus state is stored.

But in ibc-rs, we separate validation and execution. If we followed ibc-go, we could have checked status.is_active() in execute() after the following.

client_state.initialise(
ctx.get_client_execution_context(),
&client_id,
consensus_state,
)?;

But this breaks the convention of our design.

@rnbguy rnbguy marked this pull request as draft February 22, 2024 18:34
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 this pull request may close these issues.

bug(core): client creation only checks if the client is frozen
2 participants