Handle some gas corner-cases

.changelog/unreleased/improvements/ibc-relayer/2487-gas-estimation-corner-cases.md

@@ -0,0 +1,3 @@
+- Added health-check to warn user if 'gas_multiplier' is smaller than 1.1.
+  Improved client refresh frequency to depend on 'trusting_period'.
+  ([#2487](https://github.com/informalsystems/ibc-rs/issues/2487))

modules/src/events.rs

@@ -128,7 +128,7 @@ const TIMEOUT_EVENT: &str = "timeout_packet";
 const TIMEOUT_ON_CLOSE_EVENT: &str = "timeout_packet_on_close";
 
 /// Events types
-#[derive(Debug, Clone, Deserialize, Serialize)]
+#[derive(Debug, Clone, Deserialize, Serialize, PartialEq, Eq)]
 pub enum IbcEventType {
     NewBlock,
     CreateClient,

relayer-cli/src/config.rs

@@ -170,16 +170,6 @@ fn validate_trust_threshold(
 }
 
 fn validate_gas_settings(id: &ChainId, config: &ChainConfig) -> Result<(), Diagnostic<Error>> {
-    // Check that the gas_multiplier is greater than or equal to 1.0
-    if let Some(gas_multiplier) = config.gas_multiplier {
-        if gas_multiplier < 1.0 {
-            return Err(Diagnostic::Error(Error::invalid_gas_multiplier(
-                gas_multiplier,
-                id.clone(),
-            )));
-        }
-    }
-
     // Check that the gas_adjustment option is not set
     if let Some(gas_adjustment) = config.gas_adjustment {
         let gas_multiplier = gas_adjustment + 1.0;

relayer/src/chain/cosmos.rs

@@ -65,7 +65,9 @@ use crate::chain::cosmos::query::tx::query_txs;
 use crate::chain::cosmos::query::{abci_query, fetch_version_specs, packet_query, QueryResponse};
 use crate::chain::cosmos::types::account::Account;
 use crate::chain::cosmos::types::config::TxConfig;
-use crate::chain::cosmos::types::gas::{default_gas_from_config, max_gas_from_config};
+use crate::chain::cosmos::types::gas::{
+    default_gas_from_config, gas_multiplier_from_config, max_gas_from_config,
+};
 use crate::chain::endpoint::{ChainEndpoint, ChainStatus, HealthCheck};
 use crate::chain::tracking::TrackedMsgs;
 use crate::config::ChainConfig;
@@ -223,6 +225,15 @@ impl CosmosSdkChain {
             }
         }
 
+        let gas_multiplier = gas_multiplier_from_config(&self.config);
+
+        if gas_multiplier < 1.1 {
+            return Err(Error::config_validation_gas_multiplier_low(
+                self.id().clone(),
+                gas_multiplier,
+            ));
+        }
+
         Ok(())
     }
 

relayer/src/chain/cosmos/gas.rs

@@ -71,8 +71,6 @@ fn adjust_estimated_gas(
         gas_amount,
     }: AdjustGas,
 ) -> u64 {
-    assert!(gas_multiplier >= 1.0);
-
     // No need to compute anything if the gas amount is zero
     if gas_amount == 0 {
         return 0;

relayer/src/chain/cosmos/types/gas.rs

@@ -6,9 +6,6 @@ use crate::config::{ChainConfig, GasPrice};
 /// Default gas limit when submitting a transaction.
 const DEFAULT_MAX_GAS: u64 = 400_000;
 
-/// By default, we do not increase the estimated gas to compute the fee.
-const DEFAULT_GAS_MULTIPLIER: f64 = 1.1;
-
 const DEFAULT_FEE_GRANTER: &str = "";
 
 #[derive(Debug, Clone)]
@@ -48,8 +45,8 @@ pub fn max_gas_from_config(config: &ChainConfig) -> u64 {
 }
 
 /// The gas multiplier
-fn gas_multiplier_from_config(config: &ChainConfig) -> f64 {
-    config.gas_multiplier.unwrap_or(DEFAULT_GAS_MULTIPLIER)
+pub fn gas_multiplier_from_config(config: &ChainConfig) -> f64 {
+    config.gas_multiplier.unwrap_or_default().to_f64()
 }
 
 /// Get the fee granter address

relayer/src/config.rs

@@ -17,7 +17,7 @@ use ibc::core::ics24_host::identifier::{ChainId, ChannelId, PortId};
 use ibc::timestamp::ZERO_DURATION;
 
 use crate::chain::ChainType;
-use crate::config::types::{MaxMsgNum, MaxTxSize, Memo};
+use crate::config::types::{GasMultiplier, MaxMsgNum, MaxTxSize, Memo};
 use crate::keyring::Store;
 
 pub use error::Error;
@@ -335,7 +335,7 @@ pub struct ChainConfig {
 
     // This field is deprecated, use `gas_multiplier` instead
     pub gas_adjustment: Option<f64>,
-    pub gas_multiplier: Option<f64>,
+    pub gas_multiplier: Option<GasMultiplier>,
 
     pub fee_granter: Option<String>,
     #[serde(default)]

relayer/src/config/types.rs

@@ -258,6 +258,86 @@ pub mod memo {
     }
 }
 
+pub use gas_multiplier::GasMultiplier;
+
+pub mod gas_multiplier {
+
+    flex_error::define_error! {
+        Error {
+            TooSmall
+                { value: f64 }
+                |e| {
+                    format_args!("`gas_multiplier` must be greater than or equal to {}, found {}",
+                        GasMultiplier::MIN_BOUND, e.value)
+                },
+        }
+    }
+
+    #[derive(Debug, Clone, Copy)]
+    pub struct GasMultiplier(f64);
+
+    impl GasMultiplier {
+        const DEFAULT: f64 = 1.1;
+        const MIN_BOUND: f64 = 1.0;
+
+        pub fn new(value: f64) -> Result<Self, Error> {
+            if value < Self::MIN_BOUND {
+                return Err(Error::too_small(value));
+            }
+            Ok(Self(value))
+        }
+
+        // Unsafe GasMultiplier used for test cases only.
+        pub fn unsafe_new(value: f64) -> Self {
+            Self(value)
+        }
+
+        pub fn to_f64(self) -> f64 {
+            self.0
+        }
+    }
+
+    impl Default for GasMultiplier {
+        fn default() -> Self {
+            Self(Self::DEFAULT)
+        }
+    }
+
+    use serde::de::Unexpected;
+    use serde::{de::Error as _, Deserialize, Deserializer, Serialize, Serializer};
+
+    impl<'de> Deserialize<'de> for GasMultiplier {
+        fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+        where
+            D: Deserializer<'de>,
+        {
+            let value = f64::deserialize(deserializer)?;
+
+            GasMultiplier::new(value).map_err(|e| match e.detail() {
+                ErrorDetail::TooSmall(_) => D::Error::invalid_value(
+                    Unexpected::Float(value),
+                    &format!("a f64 less than {}", Self::MIN_BOUND).as_str(),
+                ),
+            })
+        }
+    }
+
+    impl Serialize for GasMultiplier {
+        fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+        where
+            S: Serializer,
+        {
+            self.0.serialize(serializer)
+        }
+    }
+
+    impl From<GasMultiplier> for f64 {
+        fn from(m: GasMultiplier) -> Self {
+            m.0
+        }
+    }
+}
+
 #[cfg(test)]
 #[allow(dead_code)] // the fields of the structs defined below are never accessed
 mod tests {
@@ -323,4 +403,34 @@ mod tests {
 
         assert!(err.contains("a string length of at most"));
     }
+
+    #[test]
+    fn parse_invalid_gas_multiplier() {
+        #[derive(Debug, Deserialize)]
+        struct DummyConfig {
+            gas_multiplier: GasMultiplier,
+        }
+
+        let err = toml::from_str::<DummyConfig>("gas_multiplier = 0.9")
+            .unwrap_err()
+            .to_string();
+
+        assert!(err.contains("expected a f64 less than"));
+    }
+
+    #[test]
+    fn safe_gas_multiplier() {
+        let gas_multiplier = GasMultiplier::new(0.6);
+        assert!(
+            gas_multiplier.is_err(),
+            "Gas multiplier should be an error if value is lower than 1.0: {:?}",
+            gas_multiplier
+        );
+    }
+
+    #[test]
+    fn unsafe_gas_multiplier() {
+        let gas_multiplier = GasMultiplier::unsafe_new(0.6);
+        assert_eq!(gas_multiplier.to_f64(), 0.6);
+    }
 }

relayer/src/error.rs

@@ -459,6 +459,15 @@ define_error! {
                     e.chain_id, e.default_gas, e.max_gas)
             },
 
+        ConfigValidationGasMultiplierLow
+            {
+                chain_id: ChainId,
+                gas_multiplier: f64,
+            }
+            |e| {
+                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
             {
                 chain_id: ChainId,

relayer/src/foreign_client.rs

@@ -29,7 +29,7 @@ use ibc::core::ics02_client::msgs::upgrade_client::MsgUpgradeAnyClient;
 use ibc::core::ics02_client::trust_threshold::TrustThreshold;
 use ibc::core::ics24_host::identifier::{ChainId, ClientId};
 use ibc::downcast;
-use ibc::events::{IbcEvent, WithBlockDataType};
+use ibc::events::{IbcEvent, IbcEventType, WithBlockDataType};
 use ibc::timestamp::{Timestamp, TimestampOverflowError};
 use ibc::tx_msg::Msg;
 use ibc::Height;
@@ -783,6 +783,36 @@ impl<DstChain: ChainHandle, SrcChain: ChainHandle> ForeignClient<DstChain, SrcCh
     }
 
     pub fn refresh(&mut self) -> Result<Option<Vec<IbcEvent>>, ForeignClientError> {
+        match self.try_refresh() {
+            Ok(res) => {
+                // If elapsed < refresh_window for the client, `try_refresh()` will
+                // be successful with an empty vector.
+                if let Some(ibc_events) = res.clone() {
+                    // The assumption is that only one IbcEventType::ChainError will be
+                    // in the resulting Vec<IbcEvent> if an error occurred.
+                    match ibc_events
+                        .into_iter()
+                        .find(|e| e.event_type() == IbcEventType::ChainError)
+                    {
+                        Some(ev) => {
+                            return Err(ForeignClientError::chain_error_event(
+                                self.dst_chain().id(),
+                                ev,
+                            ));
+                        }
+                        None => {
+                            return Ok(res);
+                        }
+                    }
+                }
+                // Return the successful empty vector.
+                Ok(res)
+            }
+            Err(e) => Err(e),
+        }
+    }
+
+    fn try_refresh(&mut self) -> Result<Option<Vec<IbcEvent>>, ForeignClientError> {
         let (client_state, elapsed) = self.validated_client_state()?;
 
         // The refresh_window is the maximum duration

relayer/src/link/relay_path.rs

@@ -629,7 +629,7 @@ impl<ChainA: ChainHandle, ChainB: ChainHandle> RelayPath<ChainA, ChainB> {
             match self.send_from_operational_data::<S>(&odata) {
                 Ok(reply) => {
                     // Done with this op. data
-                    info!("success");
+                    info!("submitted");
                     telemetry!({
                         let (chain, counterparty, channel_id, port_id) =
                             self.target_info(odata.target);

relayer/src/link/relay_sender.rs

@@ -99,7 +99,11 @@ impl Submit for AsyncSender {
             .send_messages_and_wait_check_tx(msgs)
             .map_err(LinkError::relayer)?;
         let reply = AsyncReply { responses: a };
-        info!("[Async~>{}] {}\n", target.id(), reply);
+
+        // Note: There may be errors in the reply, for example:
+        // `Response { code: Err(11), data: Data([]), log: Log("Too much gas wanted: 35000000, maximum is 25000000: out of gas")`
+        // The runtime deliberately did not catch or retry on such errors.
+        info!(target_chain = %target.id(), "{}", reply);
 
         Ok(reply)
     }

relayer/src/worker/client.rs

@@ -1,18 +1,27 @@
 use core::convert::Infallible;
 use core::time::Duration;
 use crossbeam_channel::Receiver;
+use std::time::Instant;
 use tracing::{debug, span, trace, warn};
 
 use ibc::events::IbcEvent;
+use retry::delay::Fibonacci;
+use retry::retry_with_index;
 
+use crate::util::retry::clamp_total;
 use crate::util::task::{spawn_background_task, Next, TaskError, TaskHandle};
 use crate::{
     chain::handle::ChainHandle,
-    foreign_client::{ForeignClient, HasExpiredOrFrozenError, MisbehaviourResults},
+    foreign_client::{ForeignClient, MisbehaviourResults},
 };
 
 use super::WorkerCmd;
 
+const REFRESH_INTERVAL_SECONDS: u64 = 2;
+const INITIAL_FIBONACCI_VALUE: u64 = 1;
+const MAX_REFRESH_DELAY_SECONDS: u64 = 60 * 60; // 1 hour
+const MAX_REFRESH_TOTAL_DELAY_SECONDS: u64 = 60 * 60 * 24; // 1 day
+
 pub fn spawn_refresh_client<ChainA: ChainHandle, ChainB: ChainHandle>(
     mut client: ForeignClient<ChainA, ChainB>,
 ) -> Option<TaskHandle> {
@@ -23,6 +32,9 @@ pub fn spawn_refresh_client<ChainA: ChainHandle, ChainB: ChainHandle>(
         );
         None
     } else {
+        // Compute the refresh interval as a fraction of the client's trusting period
+        // If the trusting period or the client state is not retrieved, fallback to a default value.
+        let mut next_refresh = Instant::now() + Duration::from_secs(REFRESH_INTERVAL_SECONDS);
         Some(spawn_background_task(
             span!(
                 tracing::Level::ERROR,
@@ -33,15 +45,33 @@ pub fn spawn_refresh_client<ChainA: ChainHandle, ChainB: ChainHandle>(
             ),
             Some(Duration::from_secs(1)),
             move || {
-                let _res = client.refresh().map_err(|e| {
-                    if e.is_expired_or_frozen_error() {
-                        TaskError::Fatal(e)
-                    } else {
-                        TaskError::Ignore(e)
-                    }
-                })?;
+                // This is used for integration tests until `spawn_background_task`
+                // uses async instead of threads
+                if Instant::now() < next_refresh {
+                    return Ok(Next::Continue);
+                }
 
-                Ok(Next::Continue)
+                // Use retry mechanism only if `client.refresh()` fails.
+                let res = retry_with_index(
+                    clamp_total(
+                        Fibonacci::from(Duration::from_secs(INITIAL_FIBONACCI_VALUE)),
+                        Duration::from_secs(MAX_REFRESH_DELAY_SECONDS),
+                        Duration::from_secs(MAX_REFRESH_TOTAL_DELAY_SECONDS),
+                    ),
+                    |_| client.refresh(),
+                );
+
+                match res {
+                    // If `client.refresh()` was successful, update the `next_refresh` call.
+                    Ok(_) => {
+                        next_refresh =
+                            Instant::now() + Duration::from_secs(REFRESH_INTERVAL_SECONDS);
+                        Ok(Next::Continue)
+                    }
+                    // If `client.refresh()` failed and the retry mechanism
+                    // exceeded the maximum delay, return a fatal error.
+                    Err(e) => Err(TaskError::Fatal(e)),
+                }
             },
         ))
     }