diff --git a/.changelog/unreleased/features/988-flex-error.md b/.changelog/unreleased/features/988-flex-error.md index 402a1a4e03..d89a1b6525 100644 --- a/.changelog/unreleased/features/988-flex-error.md +++ b/.changelog/unreleased/features/988-flex-error.md @@ -1,2 +1,4 @@ -Use the [`flex-error`](https://docs.rs/flex-error/) crate to define and -handle errors. +- Use the [`flex-error`](https://docs.rs/flex-error/) crate to define and +handle errors ([#1158]) + +[#1158]: https://github.com/informalsystems/ibc-rs/issues/1158 diff --git a/.changelog/unreleased/improvements/1245-max-params-validation.md b/.changelog/unreleased/improvements/1245-max-params-validation.md new file mode 100644 index 0000000000..f0205f6b39 --- /dev/null +++ b/.changelog/unreleased/improvements/1245-max-params-validation.md @@ -0,0 +1,3 @@ +- Add semantic validation of of `max_tx_size` and `max_num_msg` config options ([#1245]) + +[#1245]: https://github.com/informalsystems/ibc-rs/issues/1245 diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index b4cb7ddd1d..b61eb8979c 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -86,11 +86,15 @@ use super::Chain; mod compatibility; +/// Default gas limit when submitting a transaction. const DEFAULT_MAX_GAS: u64 = 300_000; + +/// Fraction of the estimated gas to add to the gas limit when submitting a transaction. const DEFAULT_GAS_PRICE_ADJUSTMENT: f64 = 0.1; -const DEFAULT_MAX_MSG_NUM: usize = 30; -const DEFAULT_MAX_TX_SIZE: usize = 2 * 1048576; // 2 MBytes +/// Upper limit on the size of transactions submitted by Hermes, expressed as a +/// fraction of the maximum block size defined in the Tendermint core consensus parameters. +pub const GENESIS_MAX_BYTES_MAX_FRACTION: f64 = 0.9; mod retry_strategy { use crate::util::retry::Fixed; @@ -115,12 +119,12 @@ pub struct CosmosSdkChain { impl CosmosSdkChain { /// Does multiple RPC calls to the full node, to check for - /// reachability and that some basic APIs are available. + /// reachability and some basic APIs are available. /// /// Currently this checks that: /// - the node responds OK to `/health` RPC call; /// - the node has transaction indexing enabled; - /// - the SDK version is supported. + /// - the SDK version is supported; /// /// Emits a log warning in case anything is amiss. /// Exits early if any health check fails, without doing any @@ -161,10 +165,11 @@ impl CosmosSdkChain { ) })?; + // Construct a grpc client let mut client = ServiceClient::connect(chain.grpc_addr.clone()) .await .map_err(|e| { - Error::health_check_json_grpc_transport( + Error::health_check_grpc_transport( chain_id.clone(), rpc_address.clone(), "tendermint::ServiceClient".to_string(), @@ -175,7 +180,7 @@ impl CosmosSdkChain { let request = tonic::Request::new(GetNodeInfoRequest {}); let response = client.get_node_info(request).await.map_err(|e| { - Error::health_check_json_grpc_status( + Error::health_check_grpc_status( chain_id.clone(), rpc_address.clone(), "tendermint::ServiceClient".to_string(), @@ -204,8 +209,50 @@ impl CosmosSdkChain { } if let Err(e) = self.block_on(do_health_checkup(self)) { - warn!("{}", e); - warn!("some Hermes features may not work in this mode!"); + warn!("Health checkup for chain '{}' failed", self.id()); + warn!(" Reason: {}", e); + warn!(" Some Hermes features may not work in this mode!"); + } + } + + /// Performs validation of chain-specific configuration + /// parameters against the chain's genesis configuration. + /// + /// Currently, validates the following: + /// - the configured `max_tx_size` is appropriate. + /// + /// Emits a log warning in case any error is encountered and + /// exits early without doing subsequent validations. + pub fn validate_params(&self) { + fn do_validate_params(chain: &CosmosSdkChain) -> Result<(), Error> { + // Check on the configured max_tx_size against genesis block max_bytes parameter + let genesis = chain.block_on(chain.rpc_client.genesis()).map_err(|e| { + Error::config_validation_json_rpc( + chain.id().clone(), + chain.config.rpc_addr.to_string(), + "/genesis".to_string(), + e, + ) + })?; + + let genesis_max_bound = genesis.consensus_params.block.max_bytes; + let max_allowed = mul_ceil(genesis_max_bound, GENESIS_MAX_BYTES_MAX_FRACTION) as usize; + + if chain.max_tx_size() > max_allowed { + return Err(Error::config_validation_tx_size_out_of_bounds( + chain.id().clone(), + chain.max_tx_size(), + genesis_max_bound, + )); + } + + Ok(()) + } + + if let Err(e) = do_validate_params(self) { + warn!("Hermes might be misconfigured for chain '{}'", self.id()); + warn!(" Reason: {}", e); + warn!(" Some Hermes features may not work in this mode!"); } } @@ -378,12 +425,12 @@ impl CosmosSdkChain { /// The maximum number of messages included in a transaction fn max_msg_num(&self) -> usize { - self.config.max_msg_num.unwrap_or(DEFAULT_MAX_MSG_NUM) + self.config.max_msg_num.into() } /// The maximum size of any transaction sent by the relayer to this chain fn max_tx_size(&self) -> usize { - self.config.max_tx_size.unwrap_or(DEFAULT_MAX_TX_SIZE) + self.config.max_tx_size.into() } fn query(&self, data: Path, height: ICSHeight, prove: bool) -> Result { @@ -677,6 +724,7 @@ impl Chain for CosmosSdkChain { }; chain.health_checkup(); + chain.validate_params(); Ok(chain) } @@ -1964,6 +2012,7 @@ fn calculate_fee(adjusted_gas_amount: u64, gas_price: &GasPrice) -> Coin { } } +/// Multiply `a` with `f` and round to result up to the nearest integer. fn mul_ceil(a: u64, f: f64) -> u64 { use fraction::Fraction as F; diff --git a/relayer/src/chain/mock.rs b/relayer/src/chain/mock.rs index f88659b952..1e8c2b8c9b 100644 --- a/relayer/src/chain/mock.rs +++ b/relayer/src/chain/mock.rs @@ -406,8 +406,8 @@ pub mod test_utils { max_gas: None, gas_price: GasPrice::new(0.001, "uatom".to_string()), gas_adjustment: None, - max_msg_num: None, - max_tx_size: None, + max_msg_num: Default::default(), + max_tx_size: Default::default(), clock_drift: Duration::from_secs(5), trusting_period: Duration::from_secs(14 * 24 * 60 * 60), // 14 days trust_threshold: Default::default(), diff --git a/relayer/src/config.rs b/relayer/src/config.rs index 93d0adbcbb..cce08b279e 100644 --- a/relayer/src/config.rs +++ b/relayer/src/config.rs @@ -1,6 +1,7 @@ //! Relayer configuration pub mod reload; +pub mod types; use std::collections::{HashMap, HashSet}; use std::{fmt, fs, fs::File, io::Write, path::Path, time::Duration}; @@ -11,6 +12,7 @@ use tendermint_light_client::types::TrustThreshold; use ibc::ics24_host::identifier::{ChainId, ChannelId, PortId}; use ibc::timestamp::ZERO_DURATION; +use crate::config::types::{MaxMsgNum, MaxTxSize}; use crate::error::Error; #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] @@ -254,8 +256,10 @@ pub struct ChainConfig { pub store_prefix: String, pub max_gas: Option, pub gas_adjustment: Option, - pub max_msg_num: Option, - pub max_tx_size: Option, + #[serde(default)] + pub max_msg_num: MaxMsgNum, + #[serde(default)] + pub max_tx_size: MaxTxSize, #[serde(default = "default::clock_drift", with = "humantime_serde")] pub clock_drift: Duration, #[serde(default = "default::trusting_period", with = "humantime_serde")] diff --git a/relayer/src/config/types.rs b/relayer/src/config/types.rs new file mode 100644 index 0000000000..0f2b58c6cd --- /dev/null +++ b/relayer/src/config/types.rs @@ -0,0 +1,101 @@ +//! Configuration-related types. +//! +//! Implements defaults, as well as serializing and +//! deserializing with upper-bound verification. + +use serde::de::Unexpected; +use serde::{de::Error, Deserialize, Deserializer, Serialize, Serializer}; + +#[derive(Debug, Clone, Copy)] +pub struct MaxMsgNum(usize); + +impl MaxMsgNum { + const DEFAULT: usize = 30; + const MAX_BOUND: usize = 100; +} + +impl Default for MaxMsgNum { + fn default() -> Self { + Self(Self::DEFAULT) + } +} + +impl<'de> Deserialize<'de> for MaxMsgNum { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let u = usize::deserialize(deserializer)?; + + if u > Self::MAX_BOUND { + return Err(D::Error::invalid_value( + Unexpected::Unsigned(u as u64), + &format!("a usize less than {}", Self::MAX_BOUND).as_str(), + )); + } + + Ok(MaxMsgNum(u)) + } +} + +impl Serialize for MaxMsgNum { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + self.0.serialize(serializer) + } +} + +impl From for usize { + fn from(m: MaxMsgNum) -> Self { + m.0 + } +} + +#[derive(Debug, Clone, Copy)] +pub struct MaxTxSize(usize); + +impl MaxTxSize { + const DEFAULT: usize = 2 * 1048576; // 2 MBytes + const MAX_BOUND: usize = 8 * 1048576; // 8 MBytes +} + +impl Default for MaxTxSize { + fn default() -> Self { + Self(Self::DEFAULT) + } +} + +impl<'de> Deserialize<'de> for MaxTxSize { + fn deserialize(deserializer: D) -> Result + where + D: Deserializer<'de>, + { + let u = usize::deserialize(deserializer)?; + + if u > Self::MAX_BOUND { + return Err(D::Error::invalid_value( + Unexpected::Unsigned(u as u64), + &format!("a usize less than {}", Self::MAX_BOUND).as_str(), + )); + } + + Ok(MaxTxSize(u)) + } +} + +impl Serialize for MaxTxSize { + fn serialize(&self, serializer: S) -> Result + where + S: Serializer, + { + self.0.serialize(serializer) + } +} + +impl From for usize { + fn from(m: MaxTxSize) -> Self { + m.0 + } +} diff --git a/relayer/src/error.rs b/relayer/src/error.rs index b3cd9f61a3..07c7686116 100644 --- a/relayer/src/error.rs +++ b/relayer/src/error.rs @@ -27,6 +27,7 @@ use ibc::{ proofs::ProofError, }; +use crate::chain::cosmos::GENESIS_MAX_BYTES_MAX_FRACTION; use crate::event::monitor; define_error! { @@ -334,11 +335,11 @@ define_error! { } [ DisplayOnly ] |e| { - format!("Hermes health check failed for endpoint {0} on the Json RPC interface of chain {1}:{2}", + format!("health check failed for endpoint {0} on the JSON-RPC interface of chain {1}:{2}", e.endpoint, e.chain_id, e.address) }, - HealthCheckJsonGrpcTransport + HealthCheckGrpcTransport { chain_id: ChainId, address: String, @@ -346,11 +347,11 @@ define_error! { } [ DisplayOnly ] |e| { - format!("Hermes health check failed for endpoint {0} on the Json RPC interface of chain {1}:{2}", + format!("health check failed for endpoint {0} on the gRPC interface of chain {1}:{2}", e.endpoint, e.chain_id, e.address) }, - HealthCheckJsonGrpcStatus + HealthCheckGrpcStatus { chain_id: ChainId, address: String, @@ -358,7 +359,7 @@ define_error! { status: tonic::Status } |e| { - format!("Hermes health check failed for endpoint {0} on the Json RPC interface of chain {1}:{2}; caused by: {3}", + format!("health check failed for endpoint {0} on the gRPC interface of chain {1}:{2}; caused by: {3}", e.endpoint, e.chain_id, e.address, e.status) }, @@ -369,10 +370,33 @@ define_error! { endpoint: String, } |e| { - format!("Hermes health check failed for endpoint {0} on the Json RPC interface of chain {1}:{2}; the gRPC response contains no application version information", + format!("health check failed for endpoint {0} on the Json RPC interface of chain {1}:{2}; the gRPC response contains no application version information", e.endpoint, e.chain_id, e.address) }, + ConfigValidationJsonRpc + { + chain_id: ChainId, + address: String, + endpoint: String, + } + [ DisplayOnly ] + |e| { + format!("semantic config validation: failed to reach endpoint {0} on the JSON-RPC interface of chain {1}:{2}", + e.endpoint, e.chain_id, e.address) + }, + + ConfigValidationTxSizeOutOfBounds + { + chain_id: ChainId, + configured_bound: usize, + 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` = {}", + e.chain_id, e.configured_bound, GENESIS_MAX_BYTES_MAX_FRACTION * 100.0, e.genesis_bound) + }, + SdkModuleVersion { chain_id: ChainId,