From 886e3c601cb35cbd61fd64b379566aa31794f946 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi <106849+romac@users.noreply.github.com> Date: Fri, 2 Feb 2024 11:44:13 +0100 Subject: [PATCH 1/5] Use full module path to look for module in version parsing code --- crates/relayer/src/chain/cosmos/version.rs | 65 ++++++---------------- 1 file changed, 17 insertions(+), 48 deletions(-) diff --git a/crates/relayer/src/chain/cosmos/version.rs b/crates/relayer/src/chain/cosmos/version.rs index e6987eebb5..a75c4ed822 100644 --- a/crates/relayer/src/chain/cosmos/version.rs +++ b/crates/relayer/src/chain/cosmos/version.rs @@ -22,20 +22,15 @@ use ibc_proto::cosmos::base::tendermint::v1beta1::VersionInfo; /// sum: "h1:yaD4PyOx0LnyfiWasC5egg1U76lT83GRxjJjupPo7Gk=", /// }, /// ``` -const SDK_MODULE_NAME: &str = "cosmos/cosmos-sdk"; -const IBC_GO_MODULE_NAME: &str = "cosmos/ibc-go"; -const TENDERMINT_MODULE_NAME: &str = "tendermint/tendermint"; -const COMET_MODULE_NAME: &str = "cometbft/cometbft"; +const SDK_MODULE_NAME: &str = "github.com/cosmos/cosmos-sdk"; +const IBC_GO_MODULE_NAME: &str = "github.com/cosmos/ibc-go"; +const TENDERMINT_MODULE_NAME: &str = "github.com/tendermint/tendermint"; +const COMET_MODULE_NAME: &str = "github.com/cometbft/cometbft"; -/// Captures the version(s) specification of different -/// modules of a network. -/// -/// Assumes that the network runs on Cosmos SDK. -/// Stores both the SDK version as well as -/// the IBC-go module version (if existing). +/// Captures the version(s) specification of different modules of a network. #[derive(Debug)] pub struct Specs { - pub cosmos_sdk: semver::Version, + pub cosmos_sdk: Option, pub ibc_go: Option, pub tendermint: Option, pub comet: Option, @@ -43,6 +38,12 @@ pub struct Specs { impl Display for Specs { fn fmt(&self, f: &mut Formatter<'_>) -> Result<(), FmtError> { + let cosmos_sdk = self + .cosmos_sdk + .as_ref() + .map(|v| v.to_string()) + .unwrap_or_else(|| "UNKNOWN".to_string()); + let ibc_go = self .ibc_go .as_ref() @@ -64,20 +65,13 @@ impl Display for Specs { write!( f, "Cosmos SDK {}, IBC-Go {}, Tendermint {}, CometBFT {}", - self.cosmos_sdk, ibc_go, tendermint, comet + cosmos_sdk, ibc_go, tendermint, comet ) } } define_error! { Error { - SdkModuleNotFound - { - address: String, - app: AppInfo, - } - |e| { format!("failed to find the SDK module dependency ('{}') for application {}", e.address, e.app) }, - ConsensusModuleNotFound { tendermint: String, @@ -121,7 +115,7 @@ impl TryFrom for Specs { application = %raw_version.app_name, version = %raw_version.version, git_commit = %raw_version.git_commit, - sdk_version = %sdk_version, + sdk_version = ?sdk_version, ibc_go_status = ?ibc_go_version, tendermint_version = ?tendermint_version, comet_version = ?comet_version, @@ -137,33 +131,8 @@ impl TryFrom for Specs { } } -fn parse_sdk_version(version_info: &VersionInfo) -> Result { - let module = version_info - .build_deps - .iter() - .find(|&m| m.path.contains(SDK_MODULE_NAME)) - .ok_or_else(|| { - Error::sdk_module_not_found(SDK_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 treat pre-releases of the SDK - // as their normal version, eg. 0.42.0-rc2 should satisfy >=0.41.3, <= 0.42.6. - version.pre = semver::Prerelease::EMPTY; - - Ok(version) +fn parse_sdk_version(version_info: &VersionInfo) -> Result, Error> { + parse_optional_version(version_info, SDK_MODULE_NAME) } fn parse_ibc_go_version(version_info: &VersionInfo) -> Result, Error> { @@ -185,7 +154,7 @@ fn parse_optional_version( match version_info .build_deps .iter() - .find(|&m| m.path.contains(module_name)) + .find(|&m| m.path == module_name) { None => Ok(None), Some(module) => { From 6a0ff07f192d5bf172dcd61048c6e9f692eeeb5a Mon Sep 17 00:00:00 2001 From: Romain Ruetschi <106849+romac@users.noreply.github.com> Date: Fri, 2 Feb 2024 11:47:40 +0100 Subject: [PATCH 2/5] Better error reporting when compat check fails --- crates/relayer/src/chain/cosmos.rs | 2 +- .../relayer/src/chain/cosmos/compatibility.rs | 44 ++++++++++++------- crates/relayer/src/error.rs | 4 +- 3 files changed, 30 insertions(+), 20 deletions(-) diff --git a/crates/relayer/src/chain/cosmos.rs b/crates/relayer/src/chain/cosmos.rs index d5ccc13242..e62d07464d 100644 --- a/crates/relayer/src/chain/cosmos.rs +++ b/crates/relayer/src/chain/cosmos.rs @@ -2419,7 +2419,7 @@ fn do_health_check(chain: &CosmosSdkChain) -> Result<(), Error> { let version_specs = chain.block_on(fetch_version_specs(&chain.config.id, &chain.grpc_addr))?; if let Err(diagnostic) = compatibility::run_diagnostic(&version_specs) { - return Err(Error::sdk_module_version( + return Err(Error::compat_check_failed( chain_id.clone(), grpc_address, diagnostic.to_string(), diff --git a/crates/relayer/src/chain/cosmos/compatibility.rs b/crates/relayer/src/chain/cosmos/compatibility.rs index 990fc0b60a..f997081d92 100644 --- a/crates/relayer/src/chain/cosmos/compatibility.rs +++ b/crates/relayer/src/chain/cosmos/compatibility.rs @@ -24,12 +24,18 @@ const IBC_GO_MODULE_VERSION_REQ: &str = ">=4.1.1, <9"; #[derive(Error, Debug)] pub enum Diagnostic { + #[error("SDK module version not found, required {requirements}")] + MissingSdkModuleVersion { requirements: String }, + + #[error("IBC-Go module version not found, required {requirements}")] + MissingIbcGoModuleVersion { requirements: String }, + #[error( "SDK module at version '{found}' does not meet compatibility requirements {requirements}" )] MismatchingSdkModuleVersion { requirements: String, found: String }, - #[error("Ibc-Go module at version '{found}' does not meet compatibility requirements {requirements}")] + #[error("IBC-Go module at version '{found}' does not meet compatibility requirements {requirements}")] MismatchingIbcGoModuleVersion { requirements: String, found: String }, } @@ -44,40 +50,44 @@ pub enum Diagnostic { /// Sdk module by name, as well as the constants /// [`SDK_MODULE_VERSION_REQ`] and [`IBC_GO_MODULE_VERSION_REQ`] /// for establishing compatibility requirements. -pub(crate) fn run_diagnostic(v: &version::Specs) -> Result<(), Diagnostic> { - debug!("running diagnostic on version info {}", v); +pub(crate) fn run_diagnostic(specs: &version::Specs) -> Result<(), Diagnostic> { + debug!("running diagnostic on version specs: {specs}"); - sdk_diagnostic(&v.cosmos_sdk)?; - ibc_go_diagnostic(v.ibc_go.as_ref())?; + sdk_diagnostic(specs.cosmos_sdk.as_ref())?; + ibc_go_diagnostic(specs.ibc_go.as_ref())?; Ok(()) } -fn sdk_diagnostic(version: &semver::Version) -> Result<(), Diagnostic> { +fn sdk_diagnostic(version: Option<&semver::Version>) -> Result<(), Diagnostic> { // Parse the SDK requirements into a semver let sdk_reqs = semver::VersionReq::parse(SDK_MODULE_VERSION_REQ) .expect("parsing the SDK module requirements into semver"); - // Finally, check the version requirements - match sdk_reqs.matches(version) { - true => Ok(()), - false => Err(Diagnostic::MismatchingSdkModuleVersion { + match version { + None => Err(Diagnostic::MissingSdkModuleVersion { requirements: SDK_MODULE_VERSION_REQ.to_string(), - found: version.to_string(), }), + + Some(version) => match sdk_reqs.matches(version) { + true => Ok(()), + false => Err(Diagnostic::MismatchingSdkModuleVersion { + requirements: SDK_MODULE_VERSION_REQ.to_string(), + found: version.to_string(), + }), + }, } } -fn ibc_go_diagnostic(version_info: Option<&semver::Version>) -> Result<(), Diagnostic> { +fn ibc_go_diagnostic(version: Option<&semver::Version>) -> Result<(), Diagnostic> { // Parse the IBC-go module requirements into a semver let ibc_reqs = semver::VersionReq::parse(IBC_GO_MODULE_VERSION_REQ) .expect("parsing the IBC-Go module requirements into semver"); - // Find the Ibc-Go module - match version_info { - // If binary lacks the ibc-go dependency it is _not_ an error, - // we support chains without the standalone ibc-go module. - None => Ok(()), + match version { + None => Err(Diagnostic::MissingIbcGoModuleVersion { + requirements: IBC_GO_MODULE_VERSION_REQ.to_string(), + }), Some(version) => match ibc_reqs.matches(version) { true => Ok(()), false => Err(Diagnostic::MismatchingIbcGoModuleVersion { diff --git a/crates/relayer/src/error.rs b/crates/relayer/src/error.rs index 2f3a964547..301e344e6a 100644 --- a/crates/relayer/src/error.rs +++ b/crates/relayer/src/error.rs @@ -494,14 +494,14 @@ define_error! { format!("semantic config validation failed for option `gas_multiplier` of chain '{}', reason: gas multiplier ({}) is smaller than `1.1`, which could trigger gas fee errors in production", e.chain_id, e.gas_multiplier) }, - SdkModuleVersion + CompatCheckFailed { chain_id: ChainId, address: String, cause: String } |e| { - format!("Hermes health check failed while verifying the application compatibility for chain {0}:{1}; caused by: {2}", + format!("compatibility check failed for chain '{0}' at '{1}': {2}", e.chain_id, e.address, e.cause) }, From 6c66c96287429935aced21d161b9e36493156b4d Mon Sep 17 00:00:00 2001 From: Romain Ruetschi <106849+romac@users.noreply.github.com> Date: Fri, 2 Feb 2024 11:47:55 +0100 Subject: [PATCH 3/5] Adapt legacy upgrade proposal check --- crates/relayer/src/upgrade_chain.rs | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-) diff --git a/crates/relayer/src/upgrade_chain.rs b/crates/relayer/src/upgrade_chain.rs index 3c6b407f5c..81ec1bc579 100644 --- a/crates/relayer/src/upgrade_chain.rs +++ b/crates/relayer/src/upgrade_chain.rs @@ -17,6 +17,7 @@ use ibc_relayer_types::clients::ics07_tendermint::client_state::UpgradeOptions; use ibc_relayer_types::core::ics02_client::client_state::ClientState; use ibc_relayer_types::core::ics24_host::identifier::{ChainId, ClientId}; use ibc_relayer_types::{downcast, Height}; +use tracing::warn; use crate::chain::handle::ChainHandle; use crate::chain::requests::{IncludeProof, QueryClientStateRequest, QueryHeight}; @@ -96,22 +97,32 @@ pub fn build_and_send_ibc_upgrade_proposal( /// Looks at the ibc-go version to determine if the legacy `UpgradeProposal` message /// or if the newer `MsgIBCSoftwareUpdate` message should be used to upgrade the chain. /// If the ibc-go version returned isn't reliable, a deprecated version, then the version -/// of Cosmos SDK is used. +/// of Cosmos SDK is used, if any. If there is no SDK version, we assume that the legacy upgrade is required. pub fn requires_legacy_upgrade_proposal(dst_chain: impl ChainHandle) -> bool { - let version_specs = dst_chain.version_specs().unwrap(); + let Ok(version_specs) = dst_chain.version_specs() else { + warn!("failed to get version specs, assuming legacy upgrade proposal is required"); + return true; + }; + + let sdk_before_50 = version_specs + .cosmos_sdk + .as_ref() + .map(|s| s.minor < 50) + .unwrap_or(true); + match version_specs.ibc_go { + None => sdk_before_50, Some(ibc_version) => { // Some ibc-go simapps return unreliable ibc-go versions, such as simapp v8.0.0 // returns version v1.0.0. So if the ibc-go version matches which is not maintained // anymore, use the Cosmos SDK version to determine if the legacy upgrade proposal // has to be used if ibc_version.major < 4 { - version_specs.cosmos_sdk.minor < 50 + sdk_before_50 } else { ibc_version.major < 8 } } - None => version_specs.cosmos_sdk.minor < 50, } } From 132c8cd94aa6730adb3d8afcbdac2eb3acf11cca Mon Sep 17 00:00:00 2001 From: Romain Ruetschi <106849+romac@users.noreply.github.com> Date: Fri, 2 Feb 2024 11:48:14 +0100 Subject: [PATCH 4/5] Improve error message when health check fails --- crates/relayer/src/chain/cosmos.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/crates/relayer/src/chain/cosmos.rs b/crates/relayer/src/chain/cosmos.rs index e62d07464d..17fdf407b0 100644 --- a/crates/relayer/src/chain/cosmos.rs +++ b/crates/relayer/src/chain/cosmos.rs @@ -991,17 +991,17 @@ impl ChainEndpoint for CosmosSdkChain { /// further checks. fn health_check(&mut self) -> Result { if let Err(e) = do_health_check(self) { - warn!("Health checkup for chain '{}' failed", self.id()); - warn!(" Reason: {}", e.detail()); - warn!(" Some Hermes features may not work in this mode!"); + warn!("health check failed for chain '{}'", self.id()); + warn!("reason: {}", e.detail()); + warn!("some Hermes features may not work in this mode!"); return Ok(HealthCheck::Unhealthy(Box::new(e))); } if let Err(e) = self.validate_params() { - warn!("Hermes might be misconfigured for chain '{}'", self.id()); - warn!(" Reason: {}", e.detail()); - warn!(" Some Hermes features may not work in this mode!"); + warn!("found potential misconfiguration for chain '{}'", self.id()); + warn!("reason: {}", e.detail()); + warn!("some Hermes features may not work in this mode!"); return Ok(HealthCheck::Unhealthy(Box::new(e))); } @@ -2400,7 +2400,7 @@ fn do_health_check(chain: &CosmosSdkChain) -> Result<(), Error> { if !found_matching_denom { warn!( - "Chain '{}' has no minimum gas price of denomination '{}' \ + "chain '{}' has no minimum gas price of denomination '{}' \ that is strictly less than the `gas_price` specified for \ that chain in the Hermes configuration. \ This is usually a sign of misconfiguration, please check your chain and Hermes configurations", @@ -2409,7 +2409,7 @@ fn do_health_check(chain: &CosmosSdkChain) -> Result<(), Error> { } } else { warn!( - "Chain '{}' has no minimum gas price value configured for denomination '{}'. \ + "chain '{}' has no minimum gas price value configured for denomination '{}'. \ This is usually a sign of misconfiguration, please check your chain and \ relayer configurations", chain_id, relayer_gas_price.denom From 79f0c7d51533c4461dfb0e4d69a10ba435913e3f Mon Sep 17 00:00:00 2001 From: Romain Ruetschi <106849+romac@users.noreply.github.com> Date: Tue, 6 Feb 2024 14:20:15 +0100 Subject: [PATCH 5/5] Add changelog entry --- .../bug-fixes/ibc-relayer/3831-better-compat-check.md | 2 ++ 1 file changed, 2 insertions(+) create mode 100644 .changelog/unreleased/bug-fixes/ibc-relayer/3831-better-compat-check.md diff --git a/.changelog/unreleased/bug-fixes/ibc-relayer/3831-better-compat-check.md b/.changelog/unreleased/bug-fixes/ibc-relayer/3831-better-compat-check.md new file mode 100644 index 0000000000..b533e98fe6 --- /dev/null +++ b/.changelog/unreleased/bug-fixes/ibc-relayer/3831-better-compat-check.md @@ -0,0 +1,2 @@ +- Improve reliability of compatibility check and fix parsing of expected modules + versions ([\#3831](https://github.com/informalsystems/hermes/issues/3831)) \ No newline at end of file