From 3028f4e768dff5d7df40463155f68366735ecb56 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Wed, 3 Nov 2021 21:02:25 +0100 Subject: [PATCH 1/9] Rename CosmosSdk::estimate_gas to simulate_gas --- relayer/src/chain/cosmos.rs | 22 +++++++++++----------- 1 file changed, 11 insertions(+), 11 deletions(-) diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index 0151b29c4b..cf22185726 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -286,13 +286,13 @@ impl CosmosSdkChain { signatures: vec![signed_doc], }; - let estimated_gas = self.estimate_gas(simulate_tx)?; - let adjusted_fee = self.fee_with_gas(estimated_gas); + let simulated_gas = self.simulate_gas(simulate_tx)?; + let adjusted_fee = self.fee_with_gas(simulated_gas); debug!( "[{}] send_tx: using {} gas, fee {}", self.id(), - estimated_gas, + simulated_gas, PrettyFee(&adjusted_fee) ); @@ -340,10 +340,10 @@ impl CosmosSdkChain { /// [`MsgUpdateClient`, `MsgRecvPacket`, ..., `MsgRecvPacket`] /// 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 { - let estimated_gas = self.send_tx_simulate(tx).map(|sr| sr.gas_info); + fn simulate_gas(&self, tx: Tx) -> Result { + let simulated_gas = self.send_tx_simulate(tx).map(|sr| sr.gas_info); - if let Ok(ref gas) = estimated_gas { + if let Ok(ref gas) = simulated_gas { debug!( "[{}] send_tx: tx simulation successful, simulated gas: {:?}", self.id(), @@ -351,7 +351,7 @@ impl CosmosSdkChain { ); } - let estimated_gas = estimated_gas.map_or_else( + let simulated_gas = simulated_gas.map_or_else( |e| { error!( "[{}] send_tx: failed to estimate gas, falling back on default gas, error: {}", @@ -364,17 +364,17 @@ impl CosmosSdkChain { |gas_info| gas_info.map_or(self.default_gas(), |g| g.gas_used), ); - 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()); + if simulated_gas > self.max_gas() { + debug!(simulated = ?simulated_gas, max = ?self.max_gas(), "[{}] send_tx: simulated gas is higher than max gas", self.id()); return Err(Error::tx_simulate_gas_estimate_exceeded( self.id().clone(), - estimated_gas, + simulated_gas, self.max_gas(), )); } - Ok(estimated_gas) + Ok(simulated_gas) } /// The default amount of gas the relayer is willing to pay for a transaction, From 4d67858206fea72141ef9b5237d387382342e23b Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Wed, 3 Nov 2021 21:02:38 +0100 Subject: [PATCH 2/9] Rephrase a couple gRPC-related errors --- relayer/src/error.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/relayer/src/error.rs b/relayer/src/error.rs index ce97c0d4d2..7c1aeedf3c 100644 --- a/relayer/src/error.rs +++ b/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 ] - |_| { "error in underlying transport when making GRPC call" }, + |_| { "error in underlying transport when making gRPC call" }, GrpcResponseParam { param: String } From 77a4448fbdd9d7e337599f125fbc105979d70a94 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Thu, 4 Nov 2021 15:26:36 +0100 Subject: [PATCH 3/9] Recover from `account seq mismatch` error during tx simulation --- relayer/src/chain/cosmos.rs | 82 +++++++++++++++++++++++++------------ relayer/src/error.rs | 13 ++++++ 2 files changed, 68 insertions(+), 27 deletions(-) diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index cf22185726..a2b416dd22 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -336,45 +336,59 @@ 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 - /// [`MsgUpdateClient`, `MsgRecvPacket`, ..., `MsgRecvPacket`] - /// if the batch is split in two TX-es, the second one will fail the simulation in `deliverTx` check + /// 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. /// In this case we use the `default_gas` param. fn simulate_gas(&self, tx: Tx) -> Result { let simulated_gas = self.send_tx_simulate(tx).map(|sr| sr.gas_info); - if let Ok(ref gas) = simulated_gas { - debug!( - "[{}] send_tx: tx simulation successful, simulated gas: {:?}", - self.id(), - gas - ); - } + match simulated_gas { + Ok(Some(gas_info)) => { + debug!( + "[{}] simulate_gas: tx simulation successful, gas amount used: {:?}", + self.id(), + gas_info.gas_used + ); - let simulated_gas = simulated_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!( + "[{}] simulate_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!( + "[{}] simulate_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 simulated_gas > self.max_gas() { - debug!(simulated = ?simulated_gas, max = ?self.max_gas(), "[{}] send_tx: simulated gas is higher than max gas", self.id()); + Err(e) => { + error!( + "[{}] simulate_gas: failed to simulate tx with non-recoverable error: {}", + self.id(), + e.detail() + ); - return Err(Error::tx_simulate_gas_estimate_exceeded( - self.id().clone(), - simulated_gas, - self.max_gas(), - )); + Err(e) + } } - - Ok(simulated_gas) } /// The default amount of gas the relayer is willing to pay for a transaction, @@ -2266,6 +2280,20 @@ 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. +/// +/// At the moment, we have only seen this happen for +/// account sequence mismatch errors, for reasons yet to be determined. +fn can_recover_from_simulation_failure(e: &Error) -> bool { + use crate::error::ErrorDetail::*; + + match e.detail() { + GrpcStatus(detail) => detail.is_account_sequence_mismatch(), + _ => false, + } +} + struct PrettyFee<'a>(&'a Fee); impl fmt::Display for PrettyFee<'_> { diff --git a/relayer/src/error.rs b/relayer/src/error.rs index 7c1aeedf3c..1d7d8759cc 100644 --- a/relayer/src/error.rs +++ b/relayer/src/error.rs @@ -477,3 +477,16 @@ impl Error { Error::channel_send() } } + +impl GrpcStatusSubdetail { + 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") + } +} From da1e368e02d7beb4a7c3644a464876d79adb3fee Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Thu, 4 Nov 2021 15:26:49 +0100 Subject: [PATCH 4/9] Add changelog entry --- .../ibc-relayer/1479-abort-failed-simulated-txs.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/unreleased/improvements/ibc-relayer/1479-abort-failed-simulated-txs.md diff --git a/.changelog/unreleased/improvements/ibc-relayer/1479-abort-failed-simulated-txs.md b/.changelog/unreleased/improvements/ibc-relayer/1479-abort-failed-simulated-txs.md new file mode 100644 index 0000000000..fdf23c78d0 --- /dev/null +++ b/.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)) \ No newline at end of file From cef2614461baf8603a126cdc932b319fa6c15392 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Thu, 4 Nov 2021 15:45:24 +0100 Subject: [PATCH 5/9] Fix doc test --- relayer/src/chain/cosmos.rs | 3 +-- 1 file changed, 1 insertion(+), 2 deletions(-) diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index a2b416dd22..11cb7d536b 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -337,8 +337,7 @@ impl CosmosSdkChain { /// /// 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: - /// - /// [`MsgUpdateClient`, `MsgRecvPacket`, ..., `MsgRecvPacket`] + /// [`MsgUpdateClient`, `MsgRecvPacket`, ..., `MsgRecvPacket`] /// /// 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. From 67d872dd885b6047ee7ed214868a6c57b38046e3 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Fri, 5 Nov 2021 12:23:56 +0100 Subject: [PATCH 6/9] Undo naming change --- relayer/src/chain/cosmos.rs | 16 ++++++++-------- 1 file changed, 8 insertions(+), 8 deletions(-) diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index 11cb7d536b..0c2422863e 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -286,13 +286,13 @@ impl CosmosSdkChain { signatures: vec![signed_doc], }; - let simulated_gas = self.simulate_gas(simulate_tx)?; - let adjusted_fee = self.fee_with_gas(simulated_gas); + let estimated_gas = self.estimate_gas(simulate_tx)?; + let adjusted_fee = self.fee_with_gas(estimated_gas); debug!( "[{}] send_tx: using {} gas, fee {}", self.id(), - simulated_gas, + estimated_gas, PrettyFee(&adjusted_fee) ); @@ -341,13 +341,13 @@ impl CosmosSdkChain { /// /// 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 simulate_gas(&self, tx: Tx) -> Result { + fn estimate_gas(&self, tx: Tx) -> Result { let simulated_gas = self.send_tx_simulate(tx).map(|sr| sr.gas_info); match simulated_gas { Ok(Some(gas_info)) => { debug!( - "[{}] simulate_gas: tx simulation successful, gas amount used: {:?}", + "[{}] estimate_gas: tx simulation successful, gas amount used: {:?}", self.id(), gas_info.gas_used ); @@ -357,7 +357,7 @@ impl CosmosSdkChain { Ok(None) => { warn!( - "[{}] simulate_gas: tx simulation successful but no gas amount used was returned, falling back on default gas: {}", + "[{}] estimate_gas: tx simulation successful but no gas amount used was returned, falling back on default gas: {}", self.id(), self.default_gas() ); @@ -370,7 +370,7 @@ impl CosmosSdkChain { // See `can_recover_from_simulation_failure` for more info. Err(e) if can_recover_from_simulation_failure(&e) => { warn!( - "[{}] simulate_gas: failed to simulate tx, falling back on default gas because the error is potentially recoverable: {}", + "[{}] estimate_gas: failed to simulate tx, falling back on default gas because the error is potentially recoverable: {}", self.id(), e.detail() ); @@ -380,7 +380,7 @@ impl CosmosSdkChain { Err(e) => { error!( - "[{}] simulate_gas: failed to simulate tx with non-recoverable error: {}", + "[{}] estimate_gas: failed to simulate tx with non-recoverable error: {}", self.id(), e.detail() ); From 03df6506c139d7b05fbcd649187a6b5aa93036c8 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Fri, 5 Nov 2021 12:30:39 +0100 Subject: [PATCH 7/9] Continue when client state height too low rather than on account sequence mismatch --- relayer/src/chain/cosmos.rs | 5 +---- relayer/src/error.rs | 15 +++++++++++++++ 2 files changed, 16 insertions(+), 4 deletions(-) diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index 0c2422863e..16037aa0d8 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -2281,14 +2281,11 @@ async fn do_health_check(chain: &CosmosSdkChain) -> Result<(), Error> { /// Determine whether the given error yielded by `tx_simulate` /// can be recovered from by submitting the tx anyway. -/// -/// At the moment, we have only seen this happen for -/// account sequence mismatch errors, for reasons yet to be determined. fn can_recover_from_simulation_failure(e: &Error) -> bool { use crate::error::ErrorDetail::*; match e.detail() { - GrpcStatus(detail) => detail.is_account_sequence_mismatch(), + GrpcStatus(detail) => detail.is_client_state_height_too_low(), _ => false, } } diff --git a/relayer/src/error.rs b/relayer/src/error.rs index 1d7d8759cc..9f655612dd 100644 --- a/relayer/src/error.rs +++ b/relayer/src/error.rs @@ -479,6 +479,21 @@ impl Error { } 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; From fedd6c6af9c51f84c7d55075073a3024cecf77ae Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Thu, 11 Nov 2021 17:31:19 +0100 Subject: [PATCH 8/9] Check that the configured max gas is smaller than the max gas consensus param --- relayer/src/chain/cosmos.rs | 18 ++++++++++++++++++ relayer/src/error.rs | 13 ++++++++++++- 2 files changed, 30 insertions(+), 1 deletion(-) diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index 16037aa0d8..98f5b099d3 100644 --- a/relayer/src/chain/cosmos.rs +++ b/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, this check does not make sense. + 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(()) } diff --git a/relayer/src/error.rs b/relayer/src/error.rs index 9f655612dd..71ed29677d 100644 --- a/relayer/src/error.rs +++ b/relayer/src/error.rs @@ -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, From 8b0de2275b03c34e9ef921b2f6c13acc840dc948 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Thu, 11 Nov 2021 17:32:52 +0100 Subject: [PATCH 9/9] Re-introduce check that the estimated gas is smaller than the max gas --- relayer/src/chain/cosmos.rs | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index 98f5b099d3..b8618dd42d 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -200,7 +200,7 @@ 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, this check does not make sense. + // 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() @@ -305,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!(