Improve reliability of compatibility check

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

crates/relayer/src/chain/cosmos.rs

@@ -1064,17 +1064,17 @@ impl ChainEndpoint for CosmosSdkChain {
     /// further checks.
     fn health_check(&mut self) -> Result<HealthCheck, Error> {
         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)));
         }
@@ -2473,7 +2473,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",
@@ -2482,7 +2482,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
@@ -2492,7 +2492,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(),

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 {

crates/relayer/src/chain/cosmos/version.rs

@@ -22,27 +22,28 @@ 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<semver::Version>,
     pub ibc_go: Option<semver::Version>,
     pub tendermint: Option<semver::Version>,
     pub comet: Option<semver::Version>,
 }
 
 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<VersionInfo> 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<VersionInfo> for Specs {
     }
 }
 
-fn parse_sdk_version(version_info: &VersionInfo) -> Result<semver::Version, Error> {
-    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<Option<semver::Version>, Error> {
+    parse_optional_version(version_info, SDK_MODULE_NAME)
 }
 
 fn parse_ibc_go_version(version_info: &VersionInfo) -> Result<Option<semver::Version>, 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) => {

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)
             },
 

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,
     }
 }