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
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Add support for fetching & parsing the Tendermint version of a network that
Hermes is connected to. ([#2301](https://github.com/informalsystems/ibc-rs/issues/2301))
72 changes: 58 additions & 14 deletions relayer/src/chain/cosmos/version.rs
Original file line number Diff line number Diff line change
@@ -1,15 +1,15 @@
//! Utilities for extracting and parsing versioning information
//! of Cosmos-SDK chains. The extracted version specification
//! of Cosmos-SDK networks. The extracted version specification
//! is captured in a domain-type semver format in [`Specs`].

use flex_error::define_error;
use tracing::trace;

use ibc_proto::cosmos::base::tendermint::v1beta1::VersionInfo;

/// Specifies the SDK & IBC-go modules path, as it is expected
/// Specifies the SDK, IBC-go, and Tendermint modules path, as expected
/// to appear in the application version information of a
/// Cosmos chain.
/// Cosmos-SDK network.
///
/// The module identification is captured in a [`Module`]
/// with the following structure as an example:
Expand All @@ -22,17 +22,19 @@ use ibc_proto::cosmos::base::tendermint::v1beta1::VersionInfo;
/// ```
const SDK_MODULE_NAME: &str = "cosmos/cosmos-sdk";
const IBC_GO_MODULE_NAME: &str = "cosmos/ibc-go";
const TENDERMINT_MODULE_NAME: &str = "tendermint/tendermint";

/// Captures the version(s) specification of different
/// modules of a chain.
/// modules of a network.
///
/// Assumes that the chain runs on Cosmos SDK.
/// Assumes that the network runs on Cosmos SDK.
/// Stores both the SDK version as well as
/// the IBC-go module version (if existing).
#[derive(Debug)]
pub struct Specs {
pub sdk_version: semver::Version,
pub ibc_go_version: Option<semver::Version>,
pub tendermint_version: semver::Version,
}

define_error! {
Expand All @@ -42,7 +44,14 @@ define_error! {
address: String,
app: AppInfo,
}
|e| { format!("node at {} running chain {} not caught up", e.address, e.app) },
|e| { format!("failed to find the SDK module dependency ('{}') for application {}", e.address, e.app) },

TendermintModuleNotFound
{
address: String,
app: AppInfo,
}
|e| { format!("failed to find the Tendermint dependency ('{}') for application {}", e.address, e.app) },

VersionParsingFailed
{
Expand All @@ -51,7 +60,7 @@ define_error! {
cause: String,
app: AppInfo,
}
|e| { format!("failed parsing the SDK module ('{}') version number '{}' into a semver for application {}; cause: {}",
|e| { format!("failed parsing the module path ('{}') version number '{}' into a semver for application {}; cause: {}",
e.module_path, e.raw_version, e.app, e.cause) },
}
}
Expand All @@ -63,19 +72,22 @@ impl TryFrom<VersionInfo> for Specs {
// Get the Cosmos SDK version
let sdk_version = parse_sdk_version(&raw_version)?;
let ibc_go_version = parse_ibc_go_version(&raw_version)?;
let tendermint_version = parse_tendermint_version(&raw_version)?;

trace!(
"parsed version specification for {} {}@{} -> SDK={}; Ibc-Go status={:?}",
raw_version.app_name,
raw_version.version,
raw_version.git_commit,
sdk_version,
ibc_go_version
application = %raw_version.app_name,
version = %raw_version.version,
git_commit = %raw_version.git_commit,
sdk_version = %sdk_version,
ibc_go_status = ?ibc_go_version,
tendermint_version = %tendermint_version,
"parsed version specification"
);

Ok(Self {
sdk_version,
ibc_go_version,
tendermint_version,
})
}
}
Expand Down Expand Up @@ -117,7 +129,8 @@ fn parse_ibc_go_version(version_info: &VersionInfo) -> Result<Option<semver::Ver
.find(|&m| m.path.contains(IBC_GO_MODULE_NAME))
{
// If binary lacks the ibc-go dependency it is _not_ an error,
// we support chains without the standalone ibc-go module.
// we support networks without the standalone ibc-go module; typically these
// are SDK 0.42-based networks, which will eventually no longer be supported.
None => Ok(None),
Some(ibc_module) => {
// The raw version number has a leading 'v', trim it out;
Expand All @@ -142,6 +155,37 @@ fn parse_ibc_go_version(version_info: &VersionInfo) -> Result<Option<semver::Ver
}
}

fn parse_tendermint_version(version_info: &VersionInfo) -> Result<semver::Version, Error> {
let module = version_info
.build_deps
.iter()
.find(|&m| m.path.contains(TENDERMINT_MODULE_NAME))
adizere marked this conversation as resolved.
Show resolved Hide resolved
.ok_or_else(|| {
Error::tendermint_module_not_found(
TENDERMINT_MODULE_NAME.to_string(),
AppInfo::from(version_info),
)
})?;

// The raw version number has a leading 'v', trim it out;
let plain_version = module.version.trim_start_matches('v');

// Parse the module version
let mut version = semver::Version::parse(plain_version).map_err(|e| {
Error::version_parsing_failed(
module.path.clone(),
module.version.clone(),
e.to_string(),
AppInfo::from(version_info),
)
})?;

// Remove the pre-release version to ensure we don't give special treatment to pre-releases.
version.pre = semver::Prerelease::EMPTY;
Comment on lines +183 to +184
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.


Ok(version)
}

/// Helper struct to capture all the reported information of an
/// IBC application, e.g., `gaiad`.
#[derive(Clone, Debug)]
Expand Down