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

CLIs for upgrades regression following update to tm-rs 0.21 #1229

Closed
6 tasks
adizere opened this issue Jul 22, 2021 · 0 comments · Fixed by #1254
Closed
6 tasks

CLIs for upgrades regression following update to tm-rs 0.21 #1229

adizere opened this issue Jul 22, 2021 · 0 comments · Fixed by #1254
Assignees
Labels
A: bug Admin: something isn't working
Milestone

Comments

@adizere
Copy link
Member

adizere commented Jul 22, 2021

Crate

ibc-relayer

Summary of Bug

Following the update to tm-rs 0.21 (#1223), in particular this change mentioned here, the upgrade-chain functionality fails. The zero_custom_fields method can no longer enforce the trust threshold to be 0/0, because that is not a valid domain type construction of the TrustThresholdFraction type.

There is a possible workaround to make the upgrade-chain command work successfully, documented in this commit
27c1a7e
This workaround achieves the following:

  • construct a prost_types::Any directly from a raw ClientState
  • the raw ClientState has not type checks, so we can enforce zero trust_level
  • enables the upgrade-chain tx to succeed

The drawback of this solution is that it's partial: the upgrade client(s) CLIs still don't work, because they required reading and decoding the client state from the upgraded chain, and this state includes the zero value, which cannot be decoded.

hermes upgrade client ibc-1 07-tendermint-0
Jul 22 16:50:48.047 INFO [ibc-0 -> ibc-1:07-tendermint-0] upgrade Height: 0-2401
Jul 22 16:50:48.072 DEBUG [ibc-0 -> ibc-1:07-tendermint-0] MsgUpdateAnyClient from trusted height 0-231 to target height 0-2401
Error: failed while trying to upgrade client id 07-tendermint-0 hosted on chain id ibc-1 with error: failed while fetching from chain ibc-0 the upgraded client state: GRPC error: invalid raw client state: error converting message type into domain type: invalid client state trust threshold: trust threshold is too large (must be <= 1)

Two possible long-term solution:

  • modify the tm-rs type TrustThresholdFraction to permit construction with zero field
  • replicate the domain type TrustThresholdFraction in the relayer crate and handle its validation there, permitting construction with zero fields

As a short term solution, for 0.6.1, we are explicitly disabling the upgrade-chain and upgrade client functionality.

More details on the underlying problem

A successful governance proposal would have a zero trust level:

gaiad query gov proposal 1 --home data/ibc-0/
...
trust_level:
denominator: "0"
numerator: "0"

Using the changes from a0d9d4b, the chain accepts the upgrade proposal, but the upgraded trust level is not zero:

gaiad query gov proposal 1 --node http://127.0.0.1:26557
...
trust_level:
denominator: "3"
numerator: "1"

The trust_level field, like all the other fields specified in the zero_custom_fields method, are custom and differ from client to client (these are chosen by the relayer when that client was created). Since these are client-specific field, the upgrading chain must set them to zero, and must only change those fields that are chain-specific (chain id, unbonding period).

After discussing with @colin-axner, here are further details:

  • the x/gov module stores the governance proposal, so when we query for it, we see the 1/3 trust level details
  • the client state gets zeroed out when the chain stores it to the ibc store keeper, so effectively the stored trust level (and its proof) are for 0/0 trust level
  • it seems that the on-chain handler for MsgUpgradeClient does not zero out the submitted client state, so if a relayer submits a client state with 1/3, the proof won't pass
  • the upgrade client CLI subsequently fails with an error specifying that proof verification failed, log here.

Version

3c383cf

Acceptance Criteria

  • re-enable upgrade-chain and upgrade client CLIs

For Admin Use

  • Not duplicate issue
  • Appropriate labels applied
  • Appropriate milestone (priority) applied
  • Appropriate contributors tagged
  • Contributor assigned/self-assigned
@adizere adizere added the A: bug Admin: something isn't working label Jul 22, 2021
@adizere adizere added this to the 07.2021 milestone Jul 22, 2021
@romac romac mentioned this issue Jul 22, 2021
2 tasks
romac added a commit that referenced this issue Jul 22, 2021
romac added a commit that referenced this issue Jul 22, 2021
romac added a commit that referenced this issue Jul 22, 2021
romac added a commit that referenced this issue Jul 22, 2021
* Bump crates and guide to 0.6.1

* Fix `hdpath` version

* Update changelog

* Bump proto crate version.

* Fixed typo

* Fix for query tx events unwrap

* Add warning on cancelled subscription in supervisor

* Bump ibc-proto to 0.9.0

* Update `hdpath`'s dep on `bitcoin`

* Disable `upgrade client` command due to a regression

See #1229 for more info

* Disable `tx raw upgrade-chain` due to a regression

See #1229 for more info

* Remove mention of `upgrade clients` from the changelog

* Add warninga about newly disabled commands

* Disable `tx raw upgrade-clients` due to a regression

See #1229 for more info

* Update changelog

Co-authored-by: Adi Seredinschi <adi@informal.systems>
adizere added a commit that referenced this issue Aug 2, 2021
* Fix for regression #1229 using new type ics02::TrustThreshold

* Cleanup

* changelog

* Apply suggestions from code review

Co-authored-by: Romain Ruetschi <romain@informal.systems>

* Add CI job for finding invalid links in markdown files (#1244)

* Added markdown-link-check

* Update changelog

* Added config file

* Added ignore pattern for crates.io, simplified output

* Added ignore pattern for localhost

* Fix ignore patterns in markdown-link-check config

* Better Github relative link for directory

* Fix Github relative links within the project

* Fixed two broken links

* Fixed last broken link

* Reverted changelog

* Removed pending changelog file

Co-authored-by: Adi Seredinschi <adi@informal.systems>

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Co-authored-by: Ranadeep Biswas <ranadeep@informal.systems>
@adizere adizere mentioned this issue Aug 2, 2021
5 tasks
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this issue Sep 13, 2022
* Bump crates and guide to 0.6.1

* Fix `hdpath` version

* Update changelog

* Bump proto crate version.

* Fixed typo

* Fix for query tx events unwrap

* Add warning on cancelled subscription in supervisor

* Bump ibc-proto to 0.9.0

* Update `hdpath`'s dep on `bitcoin`

* Disable `upgrade client` command due to a regression

See informalsystems#1229 for more info

* Disable `tx raw upgrade-chain` due to a regression

See informalsystems#1229 for more info

* Remove mention of `upgrade clients` from the changelog

* Add warninga about newly disabled commands

* Disable `tx raw upgrade-clients` due to a regression

See informalsystems#1229 for more info

* Update changelog

Co-authored-by: Adi Seredinschi <adi@informal.systems>
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this issue Sep 13, 2022
…reshold (informalsystems#1254)

* Fix for regression informalsystems#1229 using new type ics02::TrustThreshold

* Cleanup

* changelog

* Apply suggestions from code review

Co-authored-by: Romain Ruetschi <romain@informal.systems>

* Add CI job for finding invalid links in markdown files (informalsystems#1244)

* Added markdown-link-check

* Update changelog

* Added config file

* Added ignore pattern for crates.io, simplified output

* Added ignore pattern for localhost

* Fix ignore patterns in markdown-link-check config

* Better Github relative link for directory

* Fix Github relative links within the project

* Fixed two broken links

* Fixed last broken link

* Reverted changelog

* Removed pending changelog file

Co-authored-by: Adi Seredinschi <adi@informal.systems>

Co-authored-by: Romain Ruetschi <romain@informal.systems>
Co-authored-by: Ranadeep Biswas <ranadeep@informal.systems>
hu55a1n1 pushed a commit to cosmos/ibc-rs that referenced this issue Sep 29, 2022
* Bump crates and guide to 0.6.1

* Fix `hdpath` version

* Update changelog

* Bump proto crate version.

* Fixed typo

* Fix for query tx events unwrap

* Add warning on cancelled subscription in supervisor

* Bump ibc-proto to 0.9.0

* Update `hdpath`'s dep on `bitcoin`

* Disable `upgrade client` command due to a regression

See informalsystems/hermes#1229 for more info

* Disable `tx raw upgrade-chain` due to a regression

See informalsystems/hermes#1229 for more info

* Remove mention of `upgrade clients` from the changelog

* Add warninga about newly disabled commands

* Disable `tx raw upgrade-clients` due to a regression

See informalsystems/hermes#1229 for more info

* Update changelog

Co-authored-by: Adi Seredinschi <adi@informal.systems>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A: bug Admin: something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants