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

Add support for fetching & parsing the Tendermint version of a chain #2302

Merged
merged 5 commits into from
Jun 16, 2022

Conversation

adizere
Copy link
Member

@adizere adizere commented Jun 15, 2022

Closes: #2301

Description

Adds version detection support for the Tendermint dependency. In Hermes logs, this appears as:

2022-06-15T09:25:08.592954Z TRACE ThreadId(25) parsed version specification for gaiad v6.0.4@305668ab9d962431c79d718bb0ffdeec77a46439 -> SDK=0.44.6; Ibc-Go status=Some(Version { major: 2, minor: 0, patch: 3 }); Tendermint=Version { major: 0, minor: 34, patch: 15 }


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).

relayer/src/chain/cosmos/version.rs Outdated Show resolved Hide resolved
relayer/src/chain/cosmos/version.rs Show resolved Hide resolved
Comment on lines +183 to +184
// Remove the pre-release version to ensure we don't give special treatment to pre-releases.
version.pre = semver::Prerelease::EMPTY;
Copy link
Contributor

Choose a reason for hiding this comment

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

This could be problematic, since the -pre versions semantically precede the release without the pre suffix. So this is lossily bumping the version to a later one than what the module author intended.

Copy link
Member Author

Choose a reason for hiding this comment

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

We added this trimming of Prerelease version due to this issue: #1337 (comment).

The issue was that networks were using an RC instead of the stable release, e.g.

SDK module at version '0.42.0-rc0' does not meet compatibility requirements >=0.41.3, <=0.42.6

Which was fixed by trimming the Prerelease element. Not sure if you're hinting at a different issue than the one we noticed in the production signaled in #1337.

Copy link
Contributor

@mzabaluev mzabaluev Jun 16, 2022

Choose a reason for hiding this comment

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

The issue was that networks were using an RC instead of the stable release, e.g.

SDK module at version '0.42.0-rc0' does not meet compatibility requirements >=0.41.3, <=0.42.6

This semver check behavior is stricter than what I assumed. I expected that 0.42.0-rc0 does succeed 0.41.3 and precede 0.42.6, but would not meet this requirement: >= 0.42.0. Do pre-release suffixes make versions completely incomparable by the comparison logic?

Which was fixed by trimming the Prerelease element. Not sure if you're hinting at a different issue than the one we noticed in the production signaled in #1337.

Thanks for the exposition on this. A proper solution, I think, would be for the compatibility requirements to explicitly include any pre-releases they want to allow, but perhaps practicalities dictate otherwise.

Copy link
Member Author

Choose a reason for hiding this comment

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

Do pre-release suffixes make versions completely incomparable by the comparison logic?

We could give this a try in a small experiment. Not sure what you want to try exactly, but I managed to reproduce the original issue here: https://play.rust-lang.org/?version=stable&mode=debug&edition=2018&gist=c31232aaa272967f2979abf71e83f258

A proper solution, I think, would be for the compatibility requirements to explicitly include any pre-releases they want to allow, but perhaps practicalities dictate otherwise.

Indeed, it's not feasible for us to track all pre-releases and test with each of them individually. We assume that pre-releases have the same API as their corresponding release.

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for the playground; it appears that the semver crate does treat non-empty pre-release segment as not matching any comparisons with regular versions:

thread 'main' panicked at 'version 0.42.0-rc0 matching req >0.41.5', src/main.rs:14:5

The logic of VersionReq::match is poorly documented in semver docs, even though the Version type as such implements a total ordering that is documented to work in the way I described.
Then there is this:

Where the various tools differ in their interpretation or implementation of the spec, this crate follows the implementation choices made by Cargo. If you are operating on version numbers from some other package ecosystem, you will want to use a different semver library which is appropriate to that ecosystem.

Copy link
Contributor

@mzabaluev mzabaluev left a comment

Choose a reason for hiding this comment

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

Good to go, with reservations I have filed in #2309. These, however, apply to more than the code introduced in this PR, and can be fixed later.

@adizere
Copy link
Member Author

adizere commented Jun 16, 2022

Excellent, thanks for filing 2309 and the great feedback here Mikhail, appreciate the level of details we went into!

@adizere adizere merged commit 625bed4 into master Jun 16, 2022
@adizere adizere deleted the adi/2301_tm_version branch June 16, 2022 15:02
seunlanlege pushed a commit to ComposableFi/ibc-rs that referenced this pull request Jun 20, 2022
* Add codespace information in unknown SDK error (informalsystems#2268)

* ICS20 API improvements (informalsystems#2280)

* Remove `Debug` and `'static` requirements on Module trait

* Manually implement Debug for `MockRouter`

* Remove `Ics20Reader` supertrait `PortReader`

* Use primitive_types::U256 instead of uint::construct_uint!()

* Impl serde for Amount

* Add .changelog entries

* Fix relayer Dockerfile for M1 Mac (informalsystems#2286)

Otherwise library not found error will be encountered
Ref: nodejs/help#3239

* Bump uuid from 1.1.1 to 1.1.2 (informalsystems#2289)

Bumps [uuid](https://github.com/uuid-rs/uuid) from 1.1.1 to 1.1.2.
- [Release notes](https://github.com/uuid-rs/uuid/releases)
- [Commits](uuid-rs/uuid@1.1.1...1.1.2)

---
updated-dependencies:
- dependency-name: uuid
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump hdpath from 0.6.0 to 0.6.1 (informalsystems#2292)

Bumps [hdpath](https://github.com/emeraldpay/hdpath-rs) from 0.6.0 to 0.6.1.
- [Release notes](https://github.com/emeraldpay/hdpath-rs/releases)
- [Commits](https://github.com/emeraldpay/hdpath-rs/commits)

---
updated-dependencies:
- dependency-name: hdpath
  dependency-type: direct:production
  update-type: version-update:semver-patch
...

Signed-off-by: dependabot[bot] <support@github.com>

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* Bump clap from 3.1.18 to 3.2.1 (informalsystems#2291)

* Bump semver from 1.0.9 to 1.0.10 (informalsystems#2295)

* Bump http from 0.2.7 to 0.2.8 (informalsystems#2296)

* Bump tracing from 0.1.34 to 0.1.35 (informalsystems#2290)

* Hs/2210 - Fixed the variable TM to point to GAIAD_BINARY (informalsystems#2297)

* Fixed variable TM

* change log entry and lib-gm version updated to v0.1.3

Co-authored-by: Harveen Singh <harveen@informal.systems>

* Bump clap_complete from 3.1.4 to 3.2.1 (informalsystems#2288)

* Bump clap_complete from 3.1.4 to 3.2.1

Bumps [clap_complete](https://github.com/clap-rs/clap) from 3.1.4 to 3.2.1.
- [Release notes](https://github.com/clap-rs/clap/releases)
- [Changelog](https://github.com/clap-rs/clap/blob/master/CHANGELOG.md)
- [Commits](clap-rs/clap@clap_complete-v3.1.4...clap_complete-v3.2.1)

---
updated-dependencies:
- dependency-name: clap_complete
  dependency-type: direct:production
  update-type: version-update:semver-minor
...

Signed-off-by: dependabot[bot] <support@github.com>

* Bump clap dependency to 3.2.4

Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: Mikhail Zabaluev <mikhail@informal.systems>

* Fix recv packet handler incorrectly using dest port/chan to get receipt/next_seq_recv (informalsystems#2294)

* Use packet destination port/channel in recv_packet handler where appropriate

* Add .changelog entry

* Address review feedback

* Use enum for recv-packet results

* Improve RecvPacketResult

* Ignore acc seq mismath when expected < got (informalsystems#2298)

* Ignore acc seq mismath when expected < got

* Address review comments

* Changelog entry

* Add support for fetching & parsing the Tendermint version of a chain (informalsystems#2302)

* Fix for informalsystems#2301

* changelog

* changelog broken link

* KV pairs in log h/t Mikhail

* update dependencies to master branch

* update tendermint dependency

Co-authored-by: Soares Chen <soares.chen@maybevoid.com>
Co-authored-by: Shoaib Ahmed <sufialhussaini@gmail.com>
Co-authored-by: PikachuEXE <pikachuexe@gmail.com>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Co-authored-by: harveenSingh <harveen.s@hotmail.com>
Co-authored-by: Harveen Singh <harveen@informal.systems>
Co-authored-by: Mikhail Zabaluev <mikhail@informal.systems>
Co-authored-by: Anca Zamfir <ancazamfir@users.noreply.github.com>
Co-authored-by: Adi Seredinschi <adi@informal.systems>
hu55a1n1 pushed a commit to hu55a1n1/hermes that referenced this pull request Sep 13, 2022
…nformalsystems#2302)

* Fix for informalsystems#2301

* changelog

* changelog broken link

* KV pairs in log h/t Mikhail
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.

Hermes: Add support to fetch Tendermint version of a network
2 participants