Use custom options of the create client command

.changelog/unreleased/bug-fixes/1921-create-client-options.md

@@ -0,0 +1,2 @@
+- Apply client options specified with the `create client` command.
+  ([#1921](https://github.com/informalsystems/ibc-rs/issues/1921))

relayer-cli/src/commands/tx/client.rs

@@ -7,9 +7,11 @@ use ibc::core::ics02_client::client_state::ClientState;
 use ibc::core::ics24_host::identifier::{ChainId, ClientId};
 use ibc::events::IbcEvent;
 use ibc_proto::ibc::core::client::v1::QueryClientStatesRequest;
+use ibc_relayer::chain::client::ClientSettings;
+use ibc_relayer::chain::cosmos;
 use ibc_relayer::chain::handle::ChainHandle;
 use ibc_relayer::config::Config;
-use ibc_relayer::foreign_client::{CreateParams, ForeignClient};
+use ibc_relayer::foreign_client::ForeignClient;
 use tendermint_light_client_verifier::types::TrustThreshold;
 
 use crate::application::app_config;
@@ -25,13 +27,15 @@ pub struct TxCreateClientCmd {
     #[clap(required = true, help = "identifier of the source chain")]
     src_chain_id: ChainId,
 
-    /// Override the default clock drift specified in the configuration.
+    /// The maximum allowed clock drift for this client.
     ///
     /// The clock drift is a correction parameter. It helps deal with clocks
     /// that are only approximately synchronized between the source and destination chains
     /// of this client.
     /// The destination chain for this client uses the clock drift parameter when deciding
     /// to accept or reject a new header (originating from the source chain) for this client.
+    /// If this option is not specified, a suitable clock drift value is derived from the chain
+    /// configurations.
     #[clap(short = 'd', long)]
     clock_drift: Option<humantime::Duration>,
 
@@ -67,15 +71,15 @@ impl Runnable for TxCreateClientCmd {
 
         let client = ForeignClient::restore(ClientId::default(), chains.dst, chains.src);
 
-        let params = CreateParams {
-            clock_drift: self.clock_drift.map(Into::into),
+        let settings = ClientSettings::Cosmos(cosmos::client::Settings {
+            max_clock_drift: self.clock_drift.map(Into::into),
             trusting_period: self.trusting_period.map(Into::into),
-            trust_threshold: self.trust_threshold,
-        };
+            trust_threshold: self.trust_threshold.map(Into::into),
+        });
 
         // Trigger client creation via the "build" interface, so that we obtain the resulting event
         let res: Result<IbcEvent, Error> = client
-            .build_create_client_and_send(&params)
+            .build_create_client_and_send(settings)
             .map_err(Error::foreign_client);
 
         match res {

relayer/src/chain.rs

@@ -1,7 +1,6 @@
 use alloc::sync::Arc;
 use core::convert::TryFrom;
 
-use tendermint::block::Height;
 use tokio::runtime::Runtime as TokioRuntime;
 
 pub use cosmos::CosmosSdkChain;
@@ -36,6 +35,7 @@ use ibc_proto::ibc::core::commitment::v1::MerkleProof;
 use ibc_proto::ibc::core::connection::v1::{
     QueryClientConnectionsRequest, QueryConnectionsRequest,
 };
+use tendermint::block::Height;
 use tendermint_rpc::endpoint::broadcast::tx_sync::Response as TxResponse;
 
 use crate::connection::ConnectionMsgType;
@@ -45,8 +45,10 @@ use crate::keyring::{KeyEntry, KeyRing};
 use crate::light_client::LightClient;
 use crate::{config::ChainConfig, event::monitor::EventReceiver};
 
+use self::client::ClientSettings;
 use self::tx::TrackedMsgs;
 
+pub mod client;
 pub mod cosmos;
 pub mod counterparty;
 pub mod handle;
@@ -309,7 +311,7 @@ pub trait ChainEndpoint: Sized {
     fn build_client_state(
         &self,
         height: ICSHeight,
-        dst_config: ChainConfig,
+        settings: ClientSettings,
     ) -> Result<Self::ClientState, Error>;
 
     fn build_consensus_state(

relayer/src/chain/client.rs

@@ -0,0 +1,28 @@
+//! Data structures and logic to set up IBC client's parameters.
+
+use crate::chain::cosmos;
+use crate::config::ChainConfig;
+
+/// Client parameters for the `build_create_client` operation.
+///
+/// The parameters are specialized for each supported chain type.
+#[derive(Clone, Debug)]
+pub enum ClientSettings {
+    Cosmos(cosmos::client::Settings),
+}
+
+impl ClientSettings {
+    /// Fills in some settings if they have not been specified,
+    /// using the configuration of the source and the destination chain.
+    pub fn fill_in_from_chain_configs(
+        &mut self,
+        src_chain_config: &ChainConfig,
+        dst_chain_config: &ChainConfig,
+    ) {
+        use ClientSettings::*;
+
+        match self {
+            Cosmos(config) => config.fill_in_from_chain_configs(src_chain_config, dst_chain_config),
+        }
+    }
+}

relayer/src/chain/cosmos.rs

@@ -79,7 +79,6 @@ use ibc_proto::ibc::core::connection::v1::{
     QueryClientConnectionsRequest, QueryConnectionsRequest,
 };
 
-use crate::chain::{cosmos::account::query_account, QueryResponse, StatusResponse};
 use crate::config::types::Memo;
 use crate::config::{AddressType, ChainConfig, GasPrice};
 use crate::error::Error;
@@ -95,11 +94,13 @@ use ibc::core::ics24_host::path::{
     ConnectionsPath, ReceiptsPath, SeqRecvsPath,
 };
 
-use self::account::{Account, AccountNumber, AccountSequence};
-
-use super::{tx::TrackedMsgs, ChainEndpoint, HealthCheck};
+use self::account::{query_account, Account, AccountNumber, AccountSequence};
+use super::client::ClientSettings;
+use super::tx::TrackedMsgs;
+use super::{ChainEndpoint, HealthCheck, QueryResponse, StatusResponse};
 
 pub mod account;
+pub mod client;
 pub mod compatibility;
 pub mod version;
 
@@ -2077,17 +2078,25 @@ impl ChainEndpoint for CosmosSdkChain {
     fn build_client_state(
         &self,
         height: ICSHeight,
-        dst_config: ChainConfig,
+        settings: ClientSettings,
     ) -> Result<Self::ClientState, Error> {
+        let ClientSettings::Cosmos(settings) = settings;
+        let max_clock_drift = settings
+            .max_clock_drift
+            .expect("`fill_in_from_chain_configs` should have been called");
         let unbonding_period = self.unbonding_period()?;
-
-        let max_clock_drift = calculate_client_state_drift(self.config(), &dst_config);
+        let trusting_period = settings
+            .trusting_period
+            .unwrap_or_else(|| self.trusting_period(unbonding_period));
+        let trust_threshold = settings
+            .trust_threshold
+            .unwrap_or(self.config.trust_threshold.into());
 
         // Build the client state.
         ClientState::new(
             self.id().clone(),
-            self.config.trust_threshold.into(),
-            self.trusting_period(unbonding_period),
+            trust_threshold,
+            trusting_period,
             unbonding_period,
             max_clock_drift,
             height,
@@ -2550,20 +2559,6 @@ fn mul_ceil(a: u64, f: f64) -> BigInt {
     (f * a).ceil().to_integer()
 }
 
-/// Compute the `max_clock_drift` for a (new) client state
-/// as a function of the configuration of the source chain
-/// and the destination chain configuration.
-///
-/// The client state clock drift must account for destination
-/// chain block frequency and clock drift on source and dest.
-/// https://github.com/informalsystems/ibc-rs/issues/1445
-fn calculate_client_state_drift(
-    src_chain_config: &ChainConfig,
-    dst_chain_config: &ChainConfig,
-) -> Duration {
-    src_chain_config.clock_drift + dst_chain_config.clock_drift + dst_chain_config.max_block_time
-}
-
 #[cfg(test)]
 mod tests {
     use ibc::{

relayer/src/chain/cosmos/client.rs

@@ -0,0 +1,54 @@
+//! Cosmos-specific client settings.
+
+use core::time::Duration;
+
+use tracing::warn;
+
+use ibc::core::ics02_client::trust_threshold::TrustThreshold;
+
+use crate::config::ChainConfig;
+
+/// Cosmos-specific client parameters for the `build_create_client` operation.
+/// If set, the options override the defaults taken from the source chain configuration.
+#[derive(Clone, Debug, Default)]
+pub struct Settings {
+    pub max_clock_drift: Option<Duration>,
+    pub trusting_period: Option<Duration>,
+    pub trust_threshold: Option<TrustThreshold>,
+}
+
+impl Settings {
+    pub fn fill_in_from_chain_configs(
+        &mut self,
+        src_chain_config: &ChainConfig,
+        dst_chain_config: &ChainConfig,
+    ) {
+        match self.max_clock_drift {
+            None => {
+                self.max_clock_drift = Some(calculate_client_state_drift(
+                    src_chain_config,
+                    dst_chain_config,
+                ));
+            }
+            Some(user_value) => {
+                if user_value > dst_chain_config.max_block_time {
+                    warn!(
+                        "user specified max_clock_drift ({:?}) exceeds max_block_time \
+                        of the destination chain {}",
+                        user_value, dst_chain_config.id,
+                    );
+                }
+            }
+        }
+    }
+}
+
+/// The client state clock drift must account for destination
+/// chain block frequency and clock drift on source and dest.
+/// https://github.com/informalsystems/ibc-rs/issues/1445
+fn calculate_client_state_drift(
+    src_chain_config: &ChainConfig,
+    dst_chain_config: &ChainConfig,
+) -> Duration {
+    src_chain_config.clock_drift + dst_chain_config.clock_drift + dst_chain_config.max_block_time
+}

relayer/src/chain/handle.rs

@@ -51,7 +51,9 @@ use crate::{
     keyring::KeyEntry,
 };
 
-use super::{tx::TrackedMsgs, HealthCheck, StatusResponse};
+use super::client::ClientSettings;
+use super::tx::TrackedMsgs;
+use super::{HealthCheck, StatusResponse};
 
 mod base;
 mod cache;
@@ -165,7 +167,7 @@ pub enum ChainRequest {
 
     BuildClientState {
         height: Height,
-        dst_config: ChainConfig,
+        settings: ClientSettings,
         reply_to: ReplyTo<AnyClientState>,
     },
 
@@ -493,7 +495,7 @@ pub trait ChainHandle: Clone + Send + Sync + Serialize + Debug + 'static {
     fn build_client_state(
         &self,
         height: Height,
-        dst_config: ChainConfig,
+        settings: ClientSettings,
     ) -> Result<AnyClientState, Error>;
 
     /// Constructs a consensus state at the given height

relayer/src/chain/handle/base.rs

@@ -37,7 +37,7 @@ use ibc_proto::ibc::core::connection::v1::QueryClientConnectionsRequest;
 use ibc_proto::ibc::core::connection::v1::QueryConnectionsRequest;
 
 use crate::{
-    chain::{tx::TrackedMsgs, StatusResponse},
+    chain::{client::ClientSettings, tx::TrackedMsgs, StatusResponse},
     config::ChainConfig,
     connection::ConnectionMsgType,
     error::Error,
@@ -331,11 +331,11 @@ impl ChainHandle for BaseChainHandle {
     fn build_client_state(
         &self,
         height: Height,
-        dst_config: ChainConfig,
+        settings: ClientSettings,
     ) -> Result<AnyClientState, Error> {
         self.send(|reply_to| ChainRequest::BuildClientState {
             height,
-            dst_config,
+            settings,
             reply_to,
         })
     }

relayer/src/chain/handle/cache.rs

@@ -35,6 +35,7 @@ use ibc_proto::ibc::core::connection::v1::QueryConnectionsRequest;
 use serde::{Serialize, Serializer};
 
 use crate::cache::{Cache, CacheStatus};
+use crate::chain::client::ClientSettings;
 use crate::chain::handle::{ChainHandle, ChainRequest, Subscription};
 use crate::chain::tx::TrackedMsgs;
 use crate::chain::{HealthCheck, StatusResponse};
@@ -341,9 +342,9 @@ impl<Handle: ChainHandle> ChainHandle for CachingChainHandle<Handle> {
     fn build_client_state(
         &self,
         height: Height,
-        dst_config: ChainConfig,
+        settings: ClientSettings,
     ) -> Result<AnyClientState, Error> {
-        self.inner().build_client_state(height, dst_config)
+        self.inner().build_client_state(height, settings)
     }
 
     /// Constructs a consensus state at the given height

relayer/src/chain/handle/counting.rs

@@ -35,6 +35,7 @@ use std::collections::HashMap;
 use std::sync::{Arc, RwLock, RwLockReadGuard};
 use tracing::debug;
 
+use crate::chain::client::ClientSettings;
 use crate::chain::handle::{ChainHandle, ChainRequest, Subscription};
 use crate::chain::tx::TrackedMsgs;
 use crate::chain::{HealthCheck, StatusResponse};
@@ -337,10 +338,10 @@ impl<Handle: ChainHandle> ChainHandle for CountingChainHandle<Handle> {
     fn build_client_state(
         &self,
         height: Height,
-        dst_config: ChainConfig,
+        options: ClientSettings,
     ) -> Result<AnyClientState, Error> {
         self.inc_metric("build_client_state");
-        self.inner().build_client_state(height, dst_config)
+        self.inner().build_client_state(height, options)
     }
 
     /// Constructs a consensus state at the given height

relayer/src/chain/mock.rs

@@ -39,6 +39,7 @@ use ibc_proto::ibc::core::connection::v1::{
     QueryClientConnectionsRequest, QueryConnectionsRequest,
 };
 
+use crate::chain::client::ClientSettings;
 use crate::chain::{ChainEndpoint, StatusResponse};
 use crate::config::ChainConfig;
 use crate::error::Error;
@@ -350,15 +351,25 @@ impl ChainEndpoint for MockChain {
     fn build_client_state(
         &self,
         height: Height,
-        dst_config: ChainConfig,
+        settings: ClientSettings,
     ) -> Result<Self::ClientState, Error> {
+        let ClientSettings::Cosmos(settings) = settings;
+        let max_clock_drift = settings
+            .max_clock_drift
+            .expect("`fill_in_from_chain_configs` should have been called");
+        let trusting_period = settings
+            .trusting_period
+            .unwrap_or_else(|| self.trusting_period());
+        let trust_threshold = settings
+            .trust_threshold
+            .unwrap_or(self.config.trust_threshold.into());
+
         let client_state = TendermintClientState::new(
             self.id().clone(),
-            self.config.trust_threshold.into(),
-            self.trusting_period(),
+            trust_threshold,
+            trusting_period,
             self.trusting_period().add(Duration::from_secs(1000)),
-            // See `calculate_client_state_drift`
-            self.config.clock_drift + dst_config.clock_drift + dst_config.max_block_time,
+            max_clock_drift,
             height,
             ProofSpecs::default(),
             vec!["upgrade/upgradedClient".to_string()],