Avoid submitting txs when simulation fails, except for one special case

.changelog/unreleased/improvements/ibc-relayer/1479-abort-failed-simulated-txs.md

@@ -0,0 +1,3 @@
+- The relayer will now avoid submitting a tx after the simulation failed
+  (in all but one special case) to avoid wasting fees unnecessarily
+  ([#1479](https://github.com/informalsystems/ibc-rs/issues/1479))

relayer/src/chain/cosmos.rs

@@ -197,6 +197,24 @@ impl CosmosSdkChain {
             ));
         }
 
+        // Check that the configured max gas is lower or equal to the consensus params max gas.
+        let consensus_max_gas = result.consensus_params.block.max_gas;
+
+        // If the consensus max gas is < 0, we don't need to perform the check.
+        if consensus_max_gas >= 0 {
+            let consensus_max_gas: u64 = consensus_max_gas
+                .try_into()
+                .expect("cannot over or underflow because it is positive");
+
+            if self.max_gas() > consensus_max_gas {
+                return Err(Error::config_validation_max_gas_too_high(
+                    self.id().clone(),
+                    self.max_gas(),
+                    result.consensus_params.block.max_gas,
+                ));
+            }
+        }
+
         Ok(())
     }
 
@@ -287,6 +305,17 @@ impl CosmosSdkChain {
         };
 
         let estimated_gas = self.estimate_gas(simulate_tx)?;
+
+        if estimated_gas > self.max_gas() {
+            debug!(estimated = ?estimated_gas, max = ?self.max_gas(), "[{}] send_tx: estimated gas is higher than max gas", self.id());
+
+            return Err(Error::tx_simulate_gas_estimate_exceeded(
+                self.id().clone(),
+                estimated_gas,
+                self.max_gas(),
+            ));
+        }
+
         let adjusted_fee = self.fee_with_gas(estimated_gas);
 
         debug!(
@@ -336,45 +365,58 @@ impl CosmosSdkChain {
     /// Try to simulate the given tx in order to estimate how much gas will be needed to submit it.
     ///
     /// It is possible that a batch of messages are fragmented by the caller (`send_msgs`) such that
-    /// they do not individually verify. For example for the following batch
+    /// they do not individually verify. For example for the following batch:
     /// [`MsgUpdateClient`, `MsgRecvPacket`, ..., `MsgRecvPacket`]
-    /// if the batch is split in two TX-es, the second one will fail the simulation in `deliverTx` check
+    ///
+    /// If the batch is split in two TX-es, the second one will fail the simulation in `deliverTx` check.
     /// In this case we use the `default_gas` param.
     fn estimate_gas(&self, tx: Tx) -> Result<u64, Error> {
-        let estimated_gas = self.send_tx_simulate(tx).map(|sr| sr.gas_info);
+        let simulated_gas = self.send_tx_simulate(tx).map(|sr| sr.gas_info);
 
-        if let Ok(ref gas) = estimated_gas {
-            debug!(
-                "[{}] send_tx: tx simulation successful, simulated gas: {:?}",
-                self.id(),
-                gas
-            );
-        }
+        match simulated_gas {
+            Ok(Some(gas_info)) => {
+                debug!(
+                    "[{}] estimate_gas: tx simulation successful, gas amount used: {:?}",
+                    self.id(),
+                    gas_info.gas_used
+                );
 
-        let estimated_gas = estimated_gas.map_or_else(
-            |e| {
-                error!(
-                    "[{}] send_tx: failed to estimate gas, falling back on default gas, error: {}",
+                Ok(gas_info.gas_used)
+            }
+
+            Ok(None) => {
+                warn!(
+                    "[{}] estimate_gas: tx simulation successful but no gas amount used was returned, falling back on default gas: {}",
+                    self.id(),
+                    self.default_gas()
+                );
+
+                Ok(self.default_gas())
+            }
+
+            // If there is a chance that the tx will be accepted once actually submitted, we fall
+            // back on the max gas and will attempt to send it anyway.
+            // See `can_recover_from_simulation_failure` for more info.
+            Err(e) if can_recover_from_simulation_failure(&e) => {
+                warn!(
+                    "[{}] estimate_gas: failed to simulate tx, falling back on default gas because the error is potentially recoverable: {}",
                     self.id(),
                     e.detail()
                 );
 
-                self.default_gas()
-            },
-            |gas_info| gas_info.map_or(self.default_gas(), |g| g.gas_used),
-        );
+                Ok(self.default_gas())
+            }
 
-        if estimated_gas > self.max_gas() {
-            debug!(estimated = ?estimated_gas, max = ?self.max_gas(), "[{}] send_tx: estimated gas is higher than max gas", self.id());
+            Err(e) => {
+                error!(
+                    "[{}] estimate_gas: failed to simulate tx with non-recoverable error: {}",
+                    self.id(),
+                    e.detail()
+                );
 
-            return Err(Error::tx_simulate_gas_estimate_exceeded(
-                self.id().clone(),
-                estimated_gas,
-                self.max_gas(),
-            ));
+                Err(e)
+            }
         }
-
-        Ok(estimated_gas)
     }
 
     /// The default amount of gas the relayer is willing to pay for a transaction,
@@ -2266,6 +2308,17 @@ async fn do_health_check(chain: &CosmosSdkChain) -> Result<(), Error> {
     Ok(())
 }
 
+/// Determine whether the given error yielded by `tx_simulate`
+/// can be recovered from by submitting the tx anyway.
+fn can_recover_from_simulation_failure(e: &Error) -> bool {
+    use crate::error::ErrorDetail::*;
+
+    match e.detail() {
+        GrpcStatus(detail) => detail.is_client_state_height_too_low(),
+        _ => false,
+    }
+}
+
 struct PrettyFee<'a>(&'a Fee);
 
 impl fmt::Display for PrettyFee<'_> {

relayer/src/error.rs

@@ -86,15 +86,15 @@ define_error! {
             |_| { "event monitor error" },
 
         Grpc
-            |_| { "GRPC error" },
+            |_| { "gRPC error" },
 
         GrpcStatus
             { status: GrpcStatus }
-            |e| { format!("GRPC call return error status {0}", e.status) },
+            |e| { format!("gRPC call failed with status: {0}", e.status) },
 
         GrpcTransport
             [ TraceError<TransportError> ]
-            |_| { "error in underlying transport when making GRPC call" },
+            |_| { "error in underlying transport when making gRPC call" },
 
         GrpcResponseParam
             { param: String }
@@ -410,10 +410,21 @@ define_error! {
                 genesis_bound: u64,
             }
             |e| {
-                format!("semantic config validation failed for option `max_tx_size` chain '{}', reason: `max_tx_size` = {} is greater than {}% of the genesis block param `max_size` = {}",
+                format!("semantic config validation failed for option `max_tx_size` for chain '{}', reason: `max_tx_size` = {} is greater than {}% of the consensus parameter `max_size` = {}",
                     e.chain_id, e.configured_bound, GENESIS_MAX_BYTES_MAX_FRACTION * 100.0, e.genesis_bound)
             },
 
+        ConfigValidationMaxGasTooHigh
+            {
+                chain_id: ChainId,
+                configured_max_gas: u64,
+                consensus_max_gas: i64,
+            }
+            |e| {
+                format!("semantic config validation failed for option `max_gas` for chain '{}', reason: `max_gas` = {} is greater than the consensus parameter `max_gas` = {}",
+                    e.chain_id, e.configured_max_gas, e.consensus_max_gas)
+            },
+
         ConfigValidationTrustingPeriodSmallerThanZero
             {
                 chain_id: ChainId,
@@ -477,3 +488,31 @@ impl Error {
         Error::channel_send()
     }
 }
+
+impl GrpcStatusSubdetail {
+    /// Check whether this gRPC error matches
+    /// - status: InvalidArgument
+    /// - message: verification failed: ... failed packet acknowledgement verification for client: client state height < proof height ...
+    pub fn is_client_state_height_too_low(&self) -> bool {
+        if self.status.code() != tonic::Code::InvalidArgument {
+            return false;
+        }
+
+        let msg = self.status.message();
+        msg.contains("verification failed") && msg.contains("client state height < proof height")
+    }
+
+    /// Check whether this gRPC error matches
+    /// - status: InvalidArgument
+    /// - message: account sequence mismatch ...
+    pub fn is_account_sequence_mismatch(&self) -> bool {
+        if self.status.code() != tonic::Code::InvalidArgument {
+            return false;
+        }
+
+        self.status
+            .message()
+            .trim_start()
+            .starts_with("account sequence mismatch")
+    }
+}