Use custom options of the create client command (informalsystems#2004)

* Rework of CreateParams -> ClientOptions

* Split CreateOptions and ClientSettings

ClientSettings is now realized from CreateOptions and chain
configurations.

* Update docs in relayer/src/chain/client.rs

Co-authored-by: Adi Seredinschi <adi@informal.systems>

* Improve help for --clock-drift

Explain that the value goes directly into the client state parameter.

* Rework ClientSettings

Make the settings specific to the chain type, in anticipation of
support for non-Cosmos chains.

* Builder pattern for foreign client tests

Change the test framework API to bootstrap ForeignClient and
ForeignClientPair to use the builder pattern.
The client_settings methods are used to override the default client
settings.

* Add client setting overrides to the test framework

* Warn when client clock drift exceeds max_block_time

* relayer: Sanitize imports in mod chain

.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

@@ -9,7 +9,7 @@ use ibc::events::IbcEvent;
 use ibc_proto::ibc::core::client::v1::QueryClientStatesRequest;
 use ibc_relayer::chain::handle::ChainHandle;
 use ibc_relayer::config::Config;
-use ibc_relayer::foreign_client::{CreateParams, ForeignClient};
+use ibc_relayer::foreign_client::{CreateOptions, ForeignClient};
 use tendermint_light_client_verifier::types::TrustThreshold;
 
 use crate::application::app_config;
@@ -25,13 +25,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 +69,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 options = CreateOptions {
+            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(options)
             .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,17 +35,20 @@ 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::config::ChainConfig;
 use crate::connection::ConnectionMsgType;
 use crate::error::Error;
-use crate::event::monitor::TxMonitorCmd;
+use crate::event::monitor::{EventReceiver, TxMonitorCmd};
 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,33 @@
+//! Data structures and logic to set up IBC client's parameters.
+
+use crate::chain::cosmos;
+use crate::config::ChainConfig;
+use crate::foreign_client::CreateOptions;
+
+/// Client parameters for the `build_create_client` operation.
+///
+/// The parameters are specialized for each supported chain type.
+#[derive(Clone, Debug)]
+pub enum ClientSettings {
+    Tendermint(cosmos::client::Settings),
+}
+
+impl ClientSettings {
+    /// Takes the settings from the user-supplied options if they have been specified,
+    /// falling back to defaults using the configuration of the source
+    /// and the destination chain.
+    pub fn for_create_command(
+        options: CreateOptions,
+        src_chain_config: &ChainConfig,
+        dst_chain_config: &ChainConfig,
+    ) -> Self {
+        // Currently, only Tendermint chain pairs are supported by
+        // ForeignClient::build_create_client_and_send. Support for
+        // heterogeneous chains is left for future revisions.
+        ClientSettings::Tendermint(cosmos::client::Settings::for_create_command(
+            options,
+            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,19 +2078,21 @@ impl ChainEndpoint for CosmosSdkChain {
     fn build_client_state(
         &self,
         height: ICSHeight,
-        dst_config: ChainConfig,
+        settings: ClientSettings,
     ) -> Result<Self::ClientState, Error> {
+        let ClientSettings::Tendermint(settings) = settings;
         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));
 
         // Build the client state.
         ClientState::new(
             self.id().clone(),
-            self.config.trust_threshold.into(),
-            self.trusting_period(unbonding_period),
+            settings.trust_threshold,
+            trusting_period,
             unbonding_period,
-            max_clock_drift,
+            settings.max_clock_drift,
             height,
             self.config.proof_specs.clone(),
             vec!["upgrade".to_string(), "upgradedIBCState".to_string()],
@@ -2550,20 +2553,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,58 @@
+//! Cosmos-specific client settings.
+
+use core::time::Duration;
+
+use tracing::warn;
+
+use ibc::core::ics02_client::trust_threshold::TrustThreshold;
+
+use crate::config::ChainConfig;
+use crate::foreign_client::CreateOptions;
+
+/// Cosmos-specific client parameters for the `build_client_state` operation.
+#[derive(Clone, Debug, Default)]
+pub struct Settings {
+    pub max_clock_drift: Duration,
+    pub trusting_period: Option<Duration>,
+    pub trust_threshold: TrustThreshold,
+}
+
+impl Settings {
+    pub fn for_create_command(
+        options: CreateOptions,
+        src_chain_config: &ChainConfig,
+        dst_chain_config: &ChainConfig,
+    ) -> Self {
+        let max_clock_drift = match options.max_clock_drift {
+            None => 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,
+                    );
+                }
+                user_value
+            }
+        };
+        let trust_threshold = options
+            .trust_threshold
+            .unwrap_or_else(|| src_chain_config.trust_threshold.into());
+        Settings {
+            max_clock_drift,
+            trusting_period: options.trusting_period,
+            trust_threshold,
+        }
+    }
+}
+
+/// 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,19 @@ impl ChainEndpoint for MockChain {
     fn build_client_state(
         &self,
         height: Height,
-        dst_config: ChainConfig,
+        settings: ClientSettings,
     ) -> Result<Self::ClientState, Error> {
+        let ClientSettings::Tendermint(settings) = settings;
+        let trusting_period = settings
+            .trusting_period
+            .unwrap_or_else(|| self.trusting_period());
+
         let client_state = TendermintClientState::new(
             self.id().clone(),
-            self.config.trust_threshold.into(),
-            self.trusting_period(),
+            settings.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,
+            settings.max_clock_drift,
             height,
             ProofSpecs::default(),
             vec!["upgrade/upgradedClient".to_string()],