Domain type & semantic validation of max_tx_size and max_num_msg …

…config options (#1246)

* Fix for #1245

* Split out params semantic validation from health check

* Fixup changelog entry

* Add .changelog entry

* Extract max fraction of genesis block max size into a consant

* Doc comments for constants

* Harmonize error messages

* Improve error message layout

* Comments & output consistent with parameters

* Final aestetic change

Co-authored-by: Romain Ruetschi <romain@informal.systems>

.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

.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

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<QueryResponse, Error> {
@@ -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;
 

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

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<u64>,
     pub gas_adjustment: Option<f64>,
-    pub max_msg_num: Option<usize>,
-    pub max_tx_size: Option<usize>,
+    #[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")]

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<D>(deserializer: D) -> Result<Self, D::Error>
+    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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        self.0.serialize(serializer)
+    }
+}
+
+impl From<MaxMsgNum> 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<D>(deserializer: D) -> Result<Self, D::Error>
+    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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: Serializer,
+    {
+        self.0.serialize(serializer)
+    }
+}
+
+impl From<MaxTxSize> for usize {
+    fn from(m: MaxTxSize) -> Self {
+        m.0
+    }
+}

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,31 +335,31 @@ define_error! {
             }
             [ DisplayOnly<tendermint_rpc::error::Error> ]
             |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,
                 endpoint: String,
             }
             [ DisplayOnly<tonic::transport::Error> ]
             |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,
                 endpoint: String,
                 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<tendermint_rpc::error::Error> ]
+            |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,