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

Convert frozen height to None when raw frozen height is zero #1640

Merged
merged 2 commits into from
Dec 6, 2021

Conversation

soareschen
Copy link
Contributor

@soareschen soareschen commented Dec 3, 2021

Fixes #1619

Description

This fixes a conversion error in #1619 which converts a raw zero frozen height to Some(height). This causes the client refresh code to always fail and produce errors in the log that looks like:

2021-12-03T15:15:23.859171Z  WARN ibc_relayer::worker::client: failed to refresh client 'ibc-beta-db1b0834 -> ibc-alpha-d6aefd25:07-tendermint-3':
   0: client 07-tendermint-3 on chain id ibc-alpha-d6aefd25 is expired or frozen

Location:
   /home/ubuntu/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/flex-error-0.4.4/src/tracer_impl/eyre.rs:10

  ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ BACKTRACE ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━
                                ⋮ 5 frames hidden ⋮
   6: flex_error::tracer_impl::eyre::<impl flex_error::tracer::ErrorMessageTracer for eyre::Report>::new_message::h840ab510e0dd20df
      at /home/ubuntu/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/flex-error-0.4.4/src/tracer_impl/eyre.rs:10
   7: ibc_relayer::foreign_client::ForeignClientError::expired_or_frozen::h7431f07b814e8973
      at /home/ubuntu/.cargo/registry/src/git.luolix.top-1ecc6299db9ec823/flex-error-0.4.4/src/macros.rs:823
   8: ibc_relayer::foreign_client::ForeignClient<DstChain,SrcChain>::refresh::h0c2eb9dc8d22c66b
      at /development/relayer/src/foreign_client.rs:575
   9: ibc_relayer::worker::client::ClientWorker<ChainA,ChainB>::run::h7a6eaa440ab40f79
      at /development/relayer/src/worker/client.rs:70
  10: ibc_relayer::worker::Worker<ChainA,ChainB>::run::hf27efb7db6ebec06
      at /development/relayer/src/worker.rs:122
  11: ibc_relayer::worker::Worker<ChainA,ChainB>::spawn::{{closure}}::hc3e19b432a406c12
      at /development/relayer/src/worker.rs:111
                                ⋮ 14 frames hidden ⋮

The error was not picked up by the CI because it is an error on the client worker, and therefore does not cause tests to crash.


PR author checklist:

  • Added changelog entry, using unclog.
  • Added tests: integration (for Hermes) or unit/mock tests (for modules).
  • Linked to GitHub issue.
  • Updated code comments and documentation (e.g., docs/).

Reviewer checklist:

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

@@ -200,6 +200,15 @@ impl TryFrom<RawClientState> for ClientState {
.clone()
.ok_or_else(Error::missing_trusting_period)?;

let frozen_height = raw.frozen_height.and_then(|raw_height| {
Copy link
Member

Choose a reason for hiding this comment

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

Raw frozen height is defined as an Option here ->

/// Block height when the client was frozen due to a misbehaviour
#[prost(message, optional, tag = "6")]
pub frozen_height: ::core::option::Option<super::super::super::core::client::v1::Height>,

I would've expected gaiad to respond with a None instead of RawHeight::zero() to indicate a client that isn't frozen. Seems to me like a bug in ibc-go(?), although I agree we need this fix for now anyway.

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

It seem like we need to update the E2E python script. I suspect the CI fails because the frozen_height has to be marked Optional[Height] instead of Height:

https://github.com/informalsystems/ibc-rs/blob/c2f6d2e5282581908f5fae99dda27028888de6fd/e2e/e2e/client.py#L63

Copy link
Member

@adizere adizere left a comment

Choose a reason for hiding this comment

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

Thanks Soares!

@adizere adizere merged commit d18cc90 into master Dec 6, 2021
@adizere adizere deleted the soares/fix-frozen-height branch December 6, 2021 17:57
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…lsystems#1640)

* Convert frozen height to None when raw frozen height is zero

* Fix Python E2E test
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.

3 participants