From ade47966a8888a87ac3b6e4ac3ef8f05c4fc81c8 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Tue, 6 Jun 2023 14:04:28 +0200 Subject: [PATCH 1/8] Add setting to configure the client refresh rate --- config.toml | 13 +++- crates/relayer-cli/src/chain_registry.rs | 1 + .../clients/ics07_tendermint/client_state.rs | 5 -- crates/relayer-types/src/mock/client_state.rs | 4 -- crates/relayer/src/client_state.rs | 18 ++--- crates/relayer/src/config.rs | 19 ++++- crates/relayer/src/config/refresh_rate.rs | 55 ++++++++++++++ crates/relayer/src/config/trust_threshold.rs | 72 +++++++++++++++++++ crates/relayer/src/foreign_client.rs | 26 ++++--- crates/relayer/src/object.rs | 13 ---- crates/relayer/src/worker/client.rs | 26 ++----- tools/test-framework/src/types/single/node.rs | 1 + 12 files changed, 192 insertions(+), 61 deletions(-) create mode 100644 crates/relayer/src/config/refresh_rate.rs create mode 100644 crates/relayer/src/config/trust_threshold.rs diff --git a/config.toml b/config.toml index 0331d6e677..cf0b7b3654 100644 --- a/config.toml +++ b/config.toml @@ -281,14 +281,23 @@ max_block_time = '30s' # Specify the amount of time to be used as the light client trusting period. # It should be significantly less than the unbonding period # (e.g. unbonding period = 3 weeks, trusting period = 2 weeks). +# # Default: 2/3 of the `unbonding period` for Cosmos SDK chains trusting_period = '14days' +# The rate at which to refresh the client referencing this chain, +# expressed as a fraction of the trusting period. +# +# Default: 1/3 (ie. three times per trusting period) +client_refresh_rate = '1/3' + # Specify the trust threshold for the light client, ie. the minimum fraction of validators # which must overlap across two blocks during light client verification. -# Default: { numerator = '2', denominator = '3' }, ie. 2/3. +# # Warning: This is an advanced feature! Modify with caution. -trust_threshold = { numerator = '2', denominator = '3' } +# +# Default: 2/3 +trust_threshold = '2/3' # Specify a string that Hermes will use as a memo for each transaction it submits # to this chain. The string is limited to 50 characters. Default: '' (empty). diff --git a/crates/relayer-cli/src/chain_registry.rs b/crates/relayer-cli/src/chain_registry.rs index fc3909127a..0763023b4e 100644 --- a/crates/relayer-cli/src/chain_registry.rs +++ b/crates/relayer-cli/src/chain_registry.rs @@ -141,6 +141,7 @@ where clock_drift: default::clock_drift(), max_block_time: default::max_block_time(), trusting_period: None, + client_refresh_rate: default::client_refresh_rate(), ccv_consumer_chain: false, memo_prefix: Memo::default(), proof_specs: Default::default(), diff --git a/crates/relayer-types/src/clients/ics07_tendermint/client_state.rs b/crates/relayer-types/src/clients/ics07_tendermint/client_state.rs index 0e0f800653..343d76af9d 100644 --- a/crates/relayer-types/src/clients/ics07_tendermint/client_state.rs +++ b/crates/relayer-types/src/clients/ics07_tendermint/client_state.rs @@ -138,11 +138,6 @@ impl ClientState { }) } - /// Get the refresh time to ensure the state does not expire - pub fn refresh_time(&self) -> Option { - Some(2 * self.trusting_period / 3) - } - /// Helper method to produce a [`Options`] struct for use in /// Tendermint-specific light client verification. pub fn as_light_client_options(&self) -> Options { diff --git a/crates/relayer-types/src/mock/client_state.rs b/crates/relayer-types/src/mock/client_state.rs index 320d15aafb..c4fb000f37 100644 --- a/crates/relayer-types/src/mock/client_state.rs +++ b/crates/relayer-types/src/mock/client_state.rs @@ -54,10 +54,6 @@ impl MockClientState { pub fn latest_height(&self) -> Height { self.header.height() } - - pub fn refresh_time(&self) -> Option { - None - } } impl Protobuf for MockClientState {} diff --git a/crates/relayer/src/client_state.rs b/crates/relayer/src/client_state.rs index 8c5eff288b..f5e8227449 100644 --- a/crates/relayer/src/client_state.rs +++ b/crates/relayer/src/client_state.rs @@ -85,30 +85,30 @@ impl AnyClientState { } } - pub fn max_clock_drift(&self) -> Duration { + pub fn trusting_period(&self) -> Duration { match self { - AnyClientState::Tendermint(state) => state.max_clock_drift, + AnyClientState::Tendermint(state) => state.trusting_period, #[cfg(test)] - AnyClientState::Mock(_) => Duration::new(0, 0), + AnyClientState::Mock(_) => Duration::from_secs(14 * 24 * 60 * 60), // 2 weeks } } - pub fn client_type(&self) -> ClientType { + pub fn max_clock_drift(&self) -> Duration { match self { - Self::Tendermint(state) => state.client_type(), + AnyClientState::Tendermint(state) => state.max_clock_drift, #[cfg(test)] - Self::Mock(state) => state.client_type(), + AnyClientState::Mock(_) => Duration::new(0, 0), } } - pub fn refresh_period(&self) -> Option { + pub fn client_type(&self) -> ClientType { match self { - AnyClientState::Tendermint(tm_state) => tm_state.refresh_time(), + Self::Tendermint(state) => state.client_type(), #[cfg(test)] - AnyClientState::Mock(mock_state) => mock_state.refresh_time(), + Self::Mock(state) => state.client_type(), } } } diff --git a/crates/relayer/src/config.rs b/crates/relayer/src/config.rs index f6754c34e5..80b4da8f5a 100644 --- a/crates/relayer/src/config.rs +++ b/crates/relayer/src/config.rs @@ -4,6 +4,8 @@ pub mod error; pub mod filter; pub mod gas_multiplier; pub mod proof_specs; +pub mod refresh_rate; +pub mod trust_threshold; pub mod types; use alloc::collections::BTreeMap; @@ -41,6 +43,7 @@ pub use crate::config::Error as ConfigError; pub use error::Error; pub use filter::PacketFilter; +pub use refresh_rate::RefreshRate; #[derive(Clone, Debug, PartialEq, Serialize, Deserialize)] pub struct GasPrice { @@ -204,6 +207,15 @@ pub mod default { pub fn max_grpc_decoding_size() -> Byte { Byte::from_bytes(33554432) } + + pub fn trust_threshold() -> TrustThreshold { + TrustThreshold::ONE_THIRD + } + + pub fn client_refresh_rate() -> RefreshRate { + // Refresh the client three times per trusting period + RefreshRate::new(1, 3) + } } #[derive(Clone, Debug, Default, Deserialize, Serialize)] @@ -546,6 +558,11 @@ pub struct ChainConfig { #[serde(default, with = "humantime_serde")] pub trusting_period: Option, + /// The rate at which to refresh the client referencing this chain, + /// expressed as a fraction of the trusting period. + #[serde(default = "default::client_refresh_rate")] + pub client_refresh_rate: RefreshRate, + /// CCV consumer chain #[serde(default = "default::ccv_consumer_chain")] pub ccv_consumer_chain: bool, @@ -573,7 +590,7 @@ pub struct ChainConfig { // These last few need to be last otherwise we run into `ValueAfterTable` error when serializing to TOML /// The trust threshold defines what fraction of the total voting power of a known /// and trusted validator set is sufficient for a commit to be accepted going forward. - #[serde(default)] + #[serde(default = "default::trust_threshold", with = "self::trust_threshold")] pub trust_threshold: TrustThreshold, pub gas_price: GasPrice, diff --git a/crates/relayer/src/config/refresh_rate.rs b/crates/relayer/src/config/refresh_rate.rs new file mode 100644 index 0000000000..c3d359f830 --- /dev/null +++ b/crates/relayer/src/config/refresh_rate.rs @@ -0,0 +1,55 @@ +use std::str::FromStr; + +use serde::{Deserialize, Deserializer, Serialize, Serializer}; + +#[derive(Copy, Clone, Debug, PartialEq, Eq)] +pub struct RefreshRate { + numerator: u64, + denominator: u64, +} + +impl RefreshRate { + pub fn new(numerator: u64, denominator: u64) -> Self { + Self { + numerator, + denominator, + } + } + + pub fn as_f64(self) -> f64 { + self.numerator as f64 / self.denominator as f64 + } +} + +impl FromStr for RefreshRate { + type Err = String; + + fn from_str(s: &str) -> Result { + let parts: Vec<&str> = s.split('/').collect(); + + if parts.len() != 2 { + return Err(format!("invalid refresh rate, must be a fraction: {s}")); + } + + let (num, denom) = (parts[0].parse(), parts[1].parse()); + + if let (Ok(num), Ok(denom)) = (num, denom) { + Ok(RefreshRate::new(num, denom)) + } else { + Err(format!("invalid refresh rate, must be a fraction: {s}",)) + } + } +} + +impl Serialize for RefreshRate { + fn serialize(&self, serializer: S) -> Result { + serializer.serialize_str(&format!("{}/{}", self.numerator, self.denominator)) + } +} + +impl<'de> Deserialize<'de> for RefreshRate { + fn deserialize>(deserializer: D) -> Result { + let s = String::deserialize(deserializer)?; + RefreshRate::from_str(&s).map_err(serde::de::Error::custom) + } +} diff --git a/crates/relayer/src/config/trust_threshold.rs b/crates/relayer/src/config/trust_threshold.rs new file mode 100644 index 0000000000..12dd151a86 --- /dev/null +++ b/crates/relayer/src/config/trust_threshold.rs @@ -0,0 +1,72 @@ +use std::{fmt, marker::PhantomData}; + +use serde::{ + de::{self, MapAccess, Visitor}, + Deserialize, Deserializer, Serialize, Serializer, +}; + +use tendermint_light_client::verifier::types::TrustThreshold; + +pub fn serialize( + trust_threshold: &TrustThreshold, + serializer: S, +) -> Result { + TrustThreshold::serialize(trust_threshold, serializer) +} + +pub fn deserialize<'de, D: Deserializer<'de>>(deserializer: D) -> Result { + deserializer.deserialize_any(StringOrStruct(PhantomData)) +} + +// This is a Visitor that forwards string types to T's `FromStr` impl and +// forwards map types to T's `Deserialize` impl. The `PhantomData` is to +// keep the compiler from complaining about T being an unused generic type +// parameter. We need T in order to know the Value type for the Visitor +// impl. +struct StringOrStruct(PhantomData T>); + +impl<'de> Visitor<'de> for StringOrStruct { + type Value = TrustThreshold; + + fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str("string (eg. '1/3') or { numerator = , denominator = }") + } + + fn visit_str(self, value: &str) -> Result + where + E: de::Error, + { + let parts: Vec<&str> = value.split('/').collect(); + + if parts.len() != 2 { + return Err(serde::de::Error::custom(format!( + "invalid trust threshold, must be a fraction: {value}", + ))); + } + + let (num, denom) = (parts[0].parse(), parts[1].parse()); + + match (num, denom) { + (Ok(num), Ok(denom)) => TrustThreshold::new(num, denom).map_err(|e| { + serde::de::Error::custom(format!( + "invalid trust threshold, must be a fraction: {e}" + )) + }), + + _ => Err(serde::de::Error::custom(format!( + "invalid trust threshold, must be a fraction: {value}", + ))), + } + } + + fn visit_map(self, map: M) -> Result + where + M: MapAccess<'de>, + { + // `MapAccessDeserializer` is a wrapper that turns a `MapAccess` + // into a `Deserializer`, allowing it to be used as the input to T's + // `Deserialize` implementation. T then deserializes itself using + // the entries from the map visitor. + TrustThreshold::deserialize(de::value::MapAccessDeserializer::new(map)) + } +} diff --git a/crates/relayer/src/foreign_client.rs b/crates/relayer/src/foreign_client.rs index 008737e052..8a04b21797 100644 --- a/crates/relayer/src/foreign_client.rs +++ b/crates/relayer/src/foreign_client.rs @@ -587,6 +587,7 @@ impl ForeignClient ForeignClient Result>, ForeignClientError> { let (client_state, elapsed) = self.validated_client_state()?; - // The refresh_window is the maximum duration - // we can backoff between subsequent client updates. - let refresh_window = client_state.refresh_period(); + let src_config = self.src_chain.config().map_err(|e| { + ForeignClientError::client_create( + self.src_chain.id(), + "failed while querying the source chain for configuration".to_string(), + e, + ) + })?; + + let refresh_rate = src_config.client_refresh_rate; + let refresh_period = client_state + .trusting_period() + .mul_f64(refresh_rate.as_f64()); - match (elapsed, refresh_window) { - (None, _) | (_, None) => Ok(None), - (Some(elapsed), Some(refresh_window)) => { - if elapsed > refresh_window { - info!(?elapsed, ?refresh_window, "client needs to be refreshed"); + match (elapsed, refresh_period) { + (None, _) => Ok(None), + (Some(elapsed), refresh_period) => { + if elapsed > refresh_period { + info!(?elapsed, ?refresh_period, "client needs to be refreshed"); self.build_latest_update_client_and_send() .map_or_else(Err, |ev| Ok(Some(ev))) diff --git a/crates/relayer/src/object.rs b/crates/relayer/src/object.rs index 0c3cea5ff0..6c269a553f 100644 --- a/crates/relayer/src/object.rs +++ b/crates/relayer/src/object.rs @@ -342,12 +342,6 @@ impl Object { ) .map_err(ObjectError::relayer)?; - if client_state.refresh_period().is_none() { - return Err(ObjectError::refresh_not_required( - e.client_id().clone(), - dst_chain.id(), - )); - } let src_chain_id = client_state.chain_id(); Ok(Client { @@ -371,13 +365,6 @@ impl Object { .map_err(ObjectError::supervisor)? .client; - if client.client_state.refresh_period().is_none() { - return Err(ObjectError::refresh_not_required( - client.client_id, - chain.id(), - )); - } - Ok(Client { dst_client_id: client.client_id.clone(), dst_chain_id: chain.id(), // The object's destination is the chain hosting the client diff --git a/crates/relayer/src/worker/client.rs b/crates/relayer/src/worker/client.rs index 71fe470355..117bddd1af 100644 --- a/crates/relayer/src/worker/client.rs +++ b/crates/relayer/src/worker/client.rs @@ -3,7 +3,6 @@ use core::time::Duration; use crossbeam_channel::Receiver; use retry::delay::Fibonacci; use retry::retry_with_index; -use std::time::Instant; use tracing::{debug, debug_span, error_span, trace, warn}; use ibc_relayer_types::core::ics02_client::events::UpdateClient; @@ -18,8 +17,8 @@ use crate::{ use super::WorkerCmd; -const REFRESH_INTERVAL: Duration = Duration::from_secs(2); // 2 seconds -const INITIAL_BACKOFF: Duration = Duration::from_secs(1); // 1 second +const REFRESH_CHECK_INTERVAL: Duration = Duration::from_secs(5); // 5 seconds +const INITIAL_BACKOFF: Duration = Duration::from_secs(5); // 5 seconds const MAX_REFRESH_DELAY: Duration = Duration::from_secs(60 * 60); // 1 hour const MAX_REFRESH_TOTAL_DELAY: Duration = Duration::from_secs(60 * 60 * 24); // 1 day @@ -35,9 +34,6 @@ pub fn spawn_refresh_client( return None; } - // Compute the refresh interval as a fraction of the client's trusting period - // If the trusting period or the client state is not retrieved, fallback to a default value. - let mut next_refresh = Instant::now() + REFRESH_INTERVAL; Some(spawn_background_task( error_span!( "worker.client.refresh", @@ -45,24 +41,16 @@ pub fn spawn_refresh_client( src_chain = %client.src_chain.id(), dst_chain = %client.dst_chain.id(), ), - Some(Duration::from_secs(1)), + Some(REFRESH_CHECK_INTERVAL), move || { - // This is used for integration tests until `spawn_background_task` - // uses async instead of threads - if Instant::now() < next_refresh { - return Ok(Next::Continue); - } - - // Use retry mechanism only if `client.refresh()` fails. + // Try to refresh the client, but only if the refresh window as expired. + // If the refresh fails, retry according to the given strategy. let res = retry_with_index(refresh_strategy(), |_| client.refresh()); match res { - // If `client.refresh()` was successful, update the `next_refresh` call. - Ok(_) => { - next_refresh = Instant::now() + REFRESH_INTERVAL; + // If `client.refresh()` was successful, continue + Ok(_) => Ok(Next::Continue), - Ok(Next::Continue) - } // If `client.refresh()` failed and the retry mechanism // exceeded the maximum delay, return a fatal error. Err(e) => Err(TaskError::Fatal(e)), diff --git a/tools/test-framework/src/types/single/node.rs b/tools/test-framework/src/types/single/node.rs index 5aeced1f4b..a7c9675521 100644 --- a/tools/test-framework/src/types/single/node.rs +++ b/tools/test-framework/src/types/single/node.rs @@ -162,6 +162,7 @@ impl FullNode { max_block_time: Duration::from_secs(30), clock_drift: Duration::from_secs(5), trusting_period: Some(Duration::from_secs(14 * 24 * 3600)), + client_refresh_rate: config::default::client_refresh_rate(), ccv_consumer_chain: false, trust_threshold: Default::default(), gas_price: config::GasPrice::new(0.003, "stake".to_string()), From 37e3e447485d4809593fc720e2e71537fff7d032 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi <106849+romac@users.noreply.github.com> Date: Fri, 25 Aug 2023 16:48:16 +0200 Subject: [PATCH 2/8] Add changelog entry --- .../unreleased/features/ibc-relayer-cli/3402-lc-refresh.md | 3 +++ 1 file changed, 3 insertions(+) create mode 100644 .changelog/unreleased/features/ibc-relayer-cli/3402-lc-refresh.md diff --git a/.changelog/unreleased/features/ibc-relayer-cli/3402-lc-refresh.md b/.changelog/unreleased/features/ibc-relayer-cli/3402-lc-refresh.md new file mode 100644 index 0000000000..2e6c95edae --- /dev/null +++ b/.changelog/unreleased/features/ibc-relayer-cli/3402-lc-refresh.md @@ -0,0 +1,3 @@ +- Add a `client_refresh_rate` setting to specify the rate at which to + refresh clients referencing this chain, relative to its trusting period. + ([\#3402](https://github.com/informalsystems/hermes/issues/3402)) \ No newline at end of file From b55ce20846dd0dcd80508dafb872130877d7ca86 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Wed, 3 Jan 2024 14:28:25 +0100 Subject: [PATCH 3/8] Allow deserializing a trust threshold as a string or a struct All these formats are allowed and equivalent: - trust_threshold = '2/3' - trust_threshold = { numerator = 2, denominator = 3 } - trust_threshold = { numerator = '2', denominator = '3' } (for backward compat) --- config.toml | 2 +- crates/relayer-cli/src/chain_registry.rs | 3 +- .../src/commands/config/validate.rs | 2 +- .../src/core/ics02_client/trust_threshold.rs | 122 +++++++++++++++--- crates/relayer/src/chain/cosmos/client.rs | 2 +- crates/relayer/src/chain/cosmos/config.rs | 21 +-- .../relayer/src/chain/cosmos/config/error.rs | 50 ++++--- crates/relayer/src/client_state.rs | 1 - crates/relayer/src/config.rs | 22 ++-- crates/relayer/src/config/types.rs | 2 + .../src/supervisor/client_state_filter.rs | 2 +- .../src/tests/client_refresh.rs | 5 +- .../src/tests/client_settings.rs | 5 +- 13 files changed, 160 insertions(+), 79 deletions(-) diff --git a/config.toml b/config.toml index 88801a955c..5bb0f6acb4 100644 --- a/config.toml +++ b/config.toml @@ -422,5 +422,5 @@ max_tx_size = 2097152 clock_drift = '5s' max_block_time = '30s' trusting_period = '14days' -trust_threshold = { numerator = '2', denominator = '3' } +trust_threshold = '2/3' address_type = { derivation = 'cosmos' } diff --git a/crates/relayer-cli/src/chain_registry.rs b/crates/relayer-cli/src/chain_registry.rs index f7952c6981..27c3b1bad0 100644 --- a/crates/relayer-cli/src/chain_registry.rs +++ b/crates/relayer-cli/src/chain_registry.rs @@ -19,11 +19,10 @@ use ibc_chain_registry::querier::*; use ibc_relayer::chain::cosmos::config::CosmosSdkConfig; use ibc_relayer::config::filter::{FilterPattern, PacketFilter}; use ibc_relayer::config::gas_multiplier::GasMultiplier; -use ibc_relayer::config::types::{MaxMsgNum, MaxTxSize, Memo}; +use ibc_relayer::config::types::{MaxMsgNum, MaxTxSize, Memo, TrustThreshold}; use ibc_relayer::config::{default, AddressType, ChainConfig, EventSourceMode, GasPrice}; use ibc_relayer::keyring::Store; -use tendermint_light_client_verifier::types::TrustThreshold; use tendermint_rpc::Url; const MAX_HEALTHY_QUERY_RETRIES: u8 = 5; diff --git a/crates/relayer-cli/src/commands/config/validate.rs b/crates/relayer-cli/src/commands/config/validate.rs index 4414bba2ab..03efaf018c 100644 --- a/crates/relayer-cli/src/commands/config/validate.rs +++ b/crates/relayer-cli/src/commands/config/validate.rs @@ -41,7 +41,7 @@ impl Runnable for ValidateCmd { // No need to output the underlying error, this is done already when the application boots. // See `application::CliApp::after_config`. match config.validate_config() { - Ok(_) => Output::success("configuration is valid").exit(), + Ok(_) => Output::success_msg("configuration is valid").exit(), Err(_) => Output::error("configuration is invalid").exit(), } } diff --git a/crates/relayer-types/src/core/ics02_client/trust_threshold.rs b/crates/relayer-types/src/core/ics02_client/trust_threshold.rs index b2183c6f98..158a05a9a9 100644 --- a/crates/relayer-types/src/core/ics02_client/trust_threshold.rs +++ b/crates/relayer-types/src/core/ics02_client/trust_threshold.rs @@ -4,6 +4,7 @@ use std::convert::TryFrom; use std::fmt::{Display, Error as FmtError, Formatter}; +use std::str::FromStr; use ibc_proto::Protobuf; use num_rational::Ratio; @@ -121,23 +122,32 @@ impl Display for TrustThreshold { } } +impl FromStr for TrustThreshold { + type Err = String; + + fn from_str(s: &str) -> Result { + let parts: Vec<&str> = s.split('/').collect(); + + if parts.len() != 2 { + return Err(format!("invalid trust threshold, must be a fraction: {s}")); + } + + let (num, denom) = (parts[0].parse(), parts[1].parse()); + + if let (Ok(num), Ok(denom)) = (num, denom) { + TrustThreshold::new(num, denom).map_err(|e| e.to_string()) + } else { + Err(format!("invalid trust threshold, must be a fraction: {s}",)) + } + } +} + impl Serialize for TrustThreshold { fn serialize(&self, serializer: S) -> Result where S: serde::Serializer, { - #[derive(Serialize)] - struct TrustThreshold { - numerator: u64, - denominator: u64, - } - - let tt = TrustThreshold { - numerator: self.numerator(), - denominator: self.denominator(), - }; - - tt.serialize(serializer) + serializer.serialize_str(&format!("{}/{}", self.numerator(), self.denominator())) } } @@ -146,13 +156,89 @@ impl<'de> Deserialize<'de> for TrustThreshold { where D: serde::Deserializer<'de>, { - #[derive(Deserialize)] - struct TrustThreshold { - numerator: u64, - denominator: u64, + use serde::de::{self, Visitor}; + use std::fmt; + + // This is a Visitor that forwards string types to T's `FromStr` impl and + // forwards map types to T's `Deserialize` impl. The `PhantomData` is to + // keep the compiler from complaining about T being an unused generic type + // parameter. We need T in order to know the Value type for the Visitor + // impl. + struct StringOrStruct; + + impl<'de> Visitor<'de> for StringOrStruct { + type Value = TrustThreshold; + + fn expecting(&self, formatter: &mut Formatter<'_>) -> fmt::Result { + formatter.write_str("string or map") + } + + fn visit_str(self, value: &str) -> Result + where + E: de::Error, + { + Ok(FromStr::from_str(value).unwrap()) + } + + fn visit_map(self, map: M) -> Result + where + M: de::MapAccess<'de>, + { + #[derive(Deserialize)] + struct TT { + #[serde(deserialize_with = "string_or_int")] + numerator: u64, + #[serde(deserialize_with = "string_or_int")] + denominator: u64, + } + + let tt = TT::deserialize(de::value::MapAccessDeserializer::new(map))?; + + TrustThreshold::new(tt.numerator, tt.denominator).map_err(de::Error::custom) + } + } + + deserializer.deserialize_any(StringOrStruct) + } +} + +fn string_or_int<'de, D>(deserializer: D) -> Result +where + D: serde::Deserializer<'de>, +{ + use serde::de::{self, Visitor}; + use std::fmt; + + struct StringOrInt; + + impl<'de> Visitor<'de> for StringOrInt { + type Value = u64; + + fn expecting(&self, formatter: &mut Formatter<'_>) -> fmt::Result { + formatter.write_str("string or int") } - let tt = TrustThreshold::deserialize(deserializer)?; - Self::new(tt.numerator, tt.denominator).map_err(serde::de::Error::custom) + fn visit_str(self, value: &str) -> Result + where + E: de::Error, + { + FromStr::from_str(value).map_err(de::Error::custom) + } + + fn visit_i64(self, value: i64) -> Result + where + E: de::Error, + { + Ok(value as u64) + } + + fn visit_u64(self, value: u64) -> Result + where + E: de::Error, + { + Ok(value) + } } + + deserializer.deserialize_any(StringOrInt) } diff --git a/crates/relayer/src/chain/cosmos/client.rs b/crates/relayer/src/chain/cosmos/client.rs index 0d3b69cdbc..cc8235c2e3 100644 --- a/crates/relayer/src/chain/cosmos/client.rs +++ b/crates/relayer/src/chain/cosmos/client.rs @@ -42,7 +42,7 @@ impl Settings { let trust_threshold = options .trust_threshold - .unwrap_or_else(|| src_chain_config.trust_threshold.into()); + .unwrap_or(src_chain_config.trust_threshold); Settings { max_clock_drift, diff --git a/crates/relayer/src/chain/cosmos/config.rs b/crates/relayer/src/chain/cosmos/config.rs index 0c73faef25..147f714229 100644 --- a/crates/relayer/src/chain/cosmos/config.rs +++ b/crates/relayer/src/chain/cosmos/config.rs @@ -1,20 +1,21 @@ +use core::time::Duration; +use std::path::PathBuf; + +use byte_unit::Byte; +use serde_derive::{Deserialize, Serialize}; +use tendermint_rpc::Url; + +use ibc_relayer_types::core::ics23_commitment::specs::ProofSpecs; +use ibc_relayer_types::core::ics24_host::identifier::ChainId; + use crate::chain::cosmos::config::error::Error as ConfigError; use crate::config::compat_mode::CompatMode; use crate::config::gas_multiplier::GasMultiplier; -use crate::config::types::{MaxMsgNum, MaxTxSize, Memo}; +use crate::config::types::{MaxMsgNum, MaxTxSize, Memo, TrustThreshold}; use crate::config::{ self, AddressType, EventSourceMode, ExtensionOption, GasPrice, GenesisRestart, PacketFilter, }; use crate::config::{default, RefreshRate}; -use byte_unit::Byte; -use core::time::Duration; -use ibc_relayer_types::core::ics23_commitment::specs::ProofSpecs; -use ibc_relayer_types::core::ics24_host::identifier::ChainId; -use serde_derive::{Deserialize, Serialize}; -use std::path::PathBuf; -use tendermint_light_client::verifier::types::TrustThreshold; -use tendermint_rpc::Url; - use crate::keyring::Store; pub mod error; diff --git a/crates/relayer/src/chain/cosmos/config/error.rs b/crates/relayer/src/chain/cosmos/config/error.rs index 41ee500266..5980ad9ac4 100644 --- a/crates/relayer/src/chain/cosmos/config/error.rs +++ b/crates/relayer/src/chain/cosmos/config/error.rs @@ -1,34 +1,32 @@ use flex_error::define_error; +use ibc_relayer_types::core::ics02_client::trust_threshold::TrustThreshold; use ibc_relayer_types::core::ics24_host::identifier::ChainId; -use tendermint_light_client_verifier::types::TrustThreshold; define_error! { - Error { - InvalidTrustThreshold - { - threshold: TrustThreshold, - chain_id: ChainId, - reason: String - } - |e| { - format!("config file specifies an invalid `trust_threshold` ({0}) for the chain '{1}', caused by: {2}", - e.threshold, e.chain_id, e.reason) - }, - -DeprecatedGasAdjustment - { - gas_adjustment: f64, - gas_multiplier: f64, - chain_id: ChainId, - } - |e| { - format!( - "config file specifies deprecated setting `gas_adjustment = {1}` for the chain '{0}'; \ - to get the same behavior, use `gas_multiplier = {2}", - e.chain_id, e.gas_adjustment, e.gas_multiplier - ) - }, + InvalidTrustThreshold + { + threshold: TrustThreshold, + chain_id: ChainId, + reason: String + } + |e| { + format!("config file specifies an invalid `trust_threshold` ({0}) for the chain '{1}', caused by: {2}", + e.threshold, e.chain_id, e.reason) + }, + DeprecatedGasAdjustment + { + gas_adjustment: f64, + gas_multiplier: f64, + chain_id: ChainId, + } + |e| { + format!( + "config file specifies deprecated setting `gas_adjustment = {1}` for the chain '{0}'; \ + to get the same behavior, use `gas_multiplier = {2}", + e.chain_id, e.gas_adjustment, e.gas_multiplier + ) + }, } } diff --git a/crates/relayer/src/client_state.rs b/crates/relayer/src/client_state.rs index 352106cf5d..814abddff6 100644 --- a/crates/relayer/src/client_state.rs +++ b/crates/relayer/src/client_state.rs @@ -14,7 +14,6 @@ use ibc_relayer_types::core::ics02_client::client_state::ClientState; use ibc_relayer_types::core::ics02_client::client_type::ClientType; use ibc_relayer_types::core::ics02_client::error::Error; use ibc_relayer_types::core::ics02_client::trust_threshold::TrustThreshold; - use ibc_relayer_types::core::ics24_host::error::ValidationError; use ibc_relayer_types::core::ics24_host::identifier::{ChainId, ClientId}; use ibc_relayer_types::Height; diff --git a/crates/relayer/src/config.rs b/crates/relayer/src/config.rs index d0e1d83ab9..a3e94e814f 100644 --- a/crates/relayer/src/config.rs +++ b/crates/relayer/src/config.rs @@ -10,15 +10,14 @@ pub mod trust_threshold; pub mod types; use alloc::collections::BTreeMap; +use core::cmp::Ordering; +use core::fmt::{Display, Error as FmtError, Formatter}; +use core::str::FromStr; +use core::time::Duration; +use std::{fs, fs::File, io::Write, ops::Range, path::Path}; + use byte_unit::Byte; -use core::{ - cmp::Ordering, - fmt::{Display, Error as FmtError, Formatter}, - str::FromStr, - time::Duration, -}; use serde_derive::{Deserialize, Serialize}; -use std::{fs, fs::File, io::Write, ops::Range, path::Path}; use tendermint::block::Height as BlockHeight; use tendermint_rpc::Url; use tendermint_rpc::WebSocketClientUrl; @@ -27,10 +26,11 @@ use ibc_proto::google::protobuf::Any; use ibc_relayer_types::core::ics24_host::identifier::{ChainId, ChannelId, PortId}; use ibc_relayer_types::timestamp::ZERO_DURATION; +use crate::chain::cosmos::config::CosmosSdkConfig; +use crate::config::types::TrustThreshold; +use crate::error::Error as RelayerError; use crate::extension_options::ExtensionOptionDynamicFeeTx; -use crate::keyring::Store; -use crate::keyring::{AnySigningKeyPair, KeyRing}; -use crate::{chain::cosmos::config::CosmosSdkConfig, error::Error as RelayerError}; +use crate::keyring::{AnySigningKeyPair, KeyRing, Store}; use crate::keyring; @@ -145,8 +145,6 @@ impl Display for ExtensionOption { /// Defaults for various fields pub mod default { - use ibc_relayer_types::core::ics02_client::trust_threshold::TrustThreshold; - use super::*; pub fn ccv_consumer_chain() -> bool { diff --git a/crates/relayer/src/config/types.rs b/crates/relayer/src/config/types.rs index def6765f0a..c79e0bd2ad 100644 --- a/crates/relayer/src/config/types.rs +++ b/crates/relayer/src/config/types.rs @@ -3,6 +3,8 @@ //! Implements defaults, as well as serializing and //! deserializing with upper-bound verification. +pub use ibc_relayer_types::core::ics02_client::trust_threshold::TrustThreshold; + pub use max_msg_num::MaxMsgNum; pub mod max_msg_num { diff --git a/crates/relayer/src/supervisor/client_state_filter.rs b/crates/relayer/src/supervisor/client_state_filter.rs index 87133e89b7..646d628710 100644 --- a/crates/relayer/src/supervisor/client_state_filter.rs +++ b/crates/relayer/src/supervisor/client_state_filter.rs @@ -1,9 +1,9 @@ use alloc::collections::BTreeMap as HashMap; use flex_error::define_error; -use ibc_relayer_types::core::ics02_client::trust_threshold::TrustThreshold; use tracing::{debug, trace}; +use ibc_relayer_types::core::ics02_client::trust_threshold::TrustThreshold; use ibc_relayer_types::core::ics03_connection::connection::ConnectionEnd; use ibc_relayer_types::core::ics04_channel::error::Error as ChannelError; use ibc_relayer_types::core::ics24_host::identifier::{ diff --git a/tools/integration-test/src/tests/client_refresh.rs b/tools/integration-test/src/tests/client_refresh.rs index 24df985574..8871cab801 100644 --- a/tools/integration-test/src/tests/client_refresh.rs +++ b/tools/integration-test/src/tests/client_refresh.rs @@ -1,10 +1,9 @@ use std::time::Duration; -use ibc_relayer::config::ChainConfig; -use ibc_relayer_types::core::ics02_client::trust_threshold::TrustThreshold; - use ibc_relayer::config::gas_multiplier::GasMultiplier; +use ibc_relayer::config::ChainConfig; use ibc_relayer::foreign_client::CreateOptions; +use ibc_relayer_types::core::ics02_client::trust_threshold::TrustThreshold; use ibc_test_framework::prelude::*; diff --git a/tools/integration-test/src/tests/client_settings.rs b/tools/integration-test/src/tests/client_settings.rs index bfd18ae66d..c5b5b4fa11 100644 --- a/tools/integration-test/src/tests/client_settings.rs +++ b/tools/integration-test/src/tests/client_settings.rs @@ -1,12 +1,11 @@ use std::time::Duration; -use ibc_relayer::config::ChainConfig; -use ibc_relayer_types::core::ics02_client::trust_threshold::TrustThreshold; - use ibc_relayer::chain::requests::{IncludeProof, QueryClientStateRequest, QueryHeight}; use ibc_relayer::client_state::AnyClientState; +use ibc_relayer::config::ChainConfig; use ibc_relayer::foreign_client::CreateOptions; use ibc_relayer_types::clients::ics07_tendermint::client_state::ClientState as TmClientState; +use ibc_relayer_types::core::ics02_client::trust_threshold::TrustThreshold; use ibc_test_framework::prelude::*; From 465291b44bd751a1502459a60852e8fad577dd6d Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Wed, 3 Jan 2024 14:40:54 +0100 Subject: [PATCH 4/8] Fix clippy warnings --- tools/integration-test/src/tests/client_settings.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/tools/integration-test/src/tests/client_settings.rs b/tools/integration-test/src/tests/client_settings.rs index c5b5b4fa11..cdb7465433 100644 --- a/tools/integration-test/src/tests/client_settings.rs +++ b/tools/integration-test/src/tests/client_settings.rs @@ -32,7 +32,7 @@ impl TestOverrides for ClientDefaultsTest { chain_config_a.clock_drift = Duration::from_secs(3); chain_config_a.max_block_time = Duration::from_secs(5); chain_config_a.trusting_period = Some(Duration::from_secs(120_000)); - chain_config_a.trust_threshold = TrustThreshold::new(13, 23).unwrap().into(); + chain_config_a.trust_threshold = TrustThreshold::new(13, 23).unwrap(); } } @@ -41,7 +41,7 @@ impl TestOverrides for ClientDefaultsTest { chain_config_b.clock_drift = Duration::from_secs(6); chain_config_b.max_block_time = Duration::from_secs(15); chain_config_b.trusting_period = Some(Duration::from_secs(340_000)); - chain_config_b.trust_threshold = TrustThreshold::TWO_THIRDS.into(); + chain_config_b.trust_threshold = TrustThreshold::TWO_THIRDS; } } } From bf42710b495809f2a88eaf3ff89984bdd599453e Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Wed, 3 Jan 2024 15:21:34 +0100 Subject: [PATCH 5/8] Keep serializing as a map --- .../src/core/ics02_client/trust_threshold.rs | 11 +++++++++-- 1 file changed, 9 insertions(+), 2 deletions(-) diff --git a/crates/relayer-types/src/core/ics02_client/trust_threshold.rs b/crates/relayer-types/src/core/ics02_client/trust_threshold.rs index 158a05a9a9..8f249b820f 100644 --- a/crates/relayer-types/src/core/ics02_client/trust_threshold.rs +++ b/crates/relayer-types/src/core/ics02_client/trust_threshold.rs @@ -147,7 +147,12 @@ impl Serialize for TrustThreshold { where S: serde::Serializer, { - serializer.serialize_str(&format!("{}/{}", self.numerator(), self.denominator())) + use serde::ser::SerializeStruct; + + let mut s = serializer.serialize_struct("TrustThreshold", 2)?; + s.serialize_field("numerator", &self.numerator())?; + s.serialize_field("denominator", &self.denominator())?; + s.end() } } @@ -170,7 +175,9 @@ impl<'de> Deserialize<'de> for TrustThreshold { type Value = TrustThreshold; fn expecting(&self, formatter: &mut Formatter<'_>) -> fmt::Result { - formatter.write_str("string or map") + formatter.write_str( + "string (eg. '1/3') or map `{ numerator = , denominator = }`", + ) } fn visit_str(self, value: &str) -> Result From df8025fbe59b2d116fe6ea50658813381d7bfc42 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Wed, 3 Jan 2024 15:21:43 +0100 Subject: [PATCH 6/8] Remove dead code --- crates/relayer/src/config.rs | 1 - crates/relayer/src/config/trust_threshold.rs | 72 -------------------- 2 files changed, 73 deletions(-) delete mode 100644 crates/relayer/src/config/trust_threshold.rs diff --git a/crates/relayer/src/config.rs b/crates/relayer/src/config.rs index a3e94e814f..f1bb4e7517 100644 --- a/crates/relayer/src/config.rs +++ b/crates/relayer/src/config.rs @@ -6,7 +6,6 @@ pub mod filter; pub mod gas_multiplier; pub mod proof_specs; pub mod refresh_rate; -pub mod trust_threshold; pub mod types; use alloc::collections::BTreeMap; diff --git a/crates/relayer/src/config/trust_threshold.rs b/crates/relayer/src/config/trust_threshold.rs deleted file mode 100644 index 12dd151a86..0000000000 --- a/crates/relayer/src/config/trust_threshold.rs +++ /dev/null @@ -1,72 +0,0 @@ -use std::{fmt, marker::PhantomData}; - -use serde::{ - de::{self, MapAccess, Visitor}, - Deserialize, Deserializer, Serialize, Serializer, -}; - -use tendermint_light_client::verifier::types::TrustThreshold; - -pub fn serialize( - trust_threshold: &TrustThreshold, - serializer: S, -) -> Result { - TrustThreshold::serialize(trust_threshold, serializer) -} - -pub fn deserialize<'de, D: Deserializer<'de>>(deserializer: D) -> Result { - deserializer.deserialize_any(StringOrStruct(PhantomData)) -} - -// This is a Visitor that forwards string types to T's `FromStr` impl and -// forwards map types to T's `Deserialize` impl. The `PhantomData` is to -// keep the compiler from complaining about T being an unused generic type -// parameter. We need T in order to know the Value type for the Visitor -// impl. -struct StringOrStruct(PhantomData T>); - -impl<'de> Visitor<'de> for StringOrStruct { - type Value = TrustThreshold; - - fn expecting(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { - formatter.write_str("string (eg. '1/3') or { numerator = , denominator = }") - } - - fn visit_str(self, value: &str) -> Result - where - E: de::Error, - { - let parts: Vec<&str> = value.split('/').collect(); - - if parts.len() != 2 { - return Err(serde::de::Error::custom(format!( - "invalid trust threshold, must be a fraction: {value}", - ))); - } - - let (num, denom) = (parts[0].parse(), parts[1].parse()); - - match (num, denom) { - (Ok(num), Ok(denom)) => TrustThreshold::new(num, denom).map_err(|e| { - serde::de::Error::custom(format!( - "invalid trust threshold, must be a fraction: {e}" - )) - }), - - _ => Err(serde::de::Error::custom(format!( - "invalid trust threshold, must be a fraction: {value}", - ))), - } - } - - fn visit_map(self, map: M) -> Result - where - M: MapAccess<'de>, - { - // `MapAccessDeserializer` is a wrapper that turns a `MapAccess` - // into a `Deserializer`, allowing it to be used as the input to T's - // `Deserialize` implementation. T then deserializes itself using - // the entries from the map visitor. - TrustThreshold::deserialize(de::value::MapAccessDeserializer::new(map)) - } -} From 35f12bb02a13ebdfba96b19087e6113e346ffcdd Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Fri, 5 Jan 2024 10:44:50 +0100 Subject: [PATCH 7/8] Fix typo Co-authored-by: Luca Joss <43531661+ljoss17@users.noreply.github.com> Signed-off-by: Romain Ruetschi --- crates/relayer/src/worker/client.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/relayer/src/worker/client.rs b/crates/relayer/src/worker/client.rs index 117bddd1af..b6147e4b25 100644 --- a/crates/relayer/src/worker/client.rs +++ b/crates/relayer/src/worker/client.rs @@ -43,7 +43,7 @@ pub fn spawn_refresh_client( ), Some(REFRESH_CHECK_INTERVAL), move || { - // Try to refresh the client, but only if the refresh window as expired. + // Try to refresh the client, but only if the refresh window has expired. // If the refresh fails, retry according to the given strategy. let res = retry_with_index(refresh_strategy(), |_| client.refresh()); From 977f1e0d4420f5282ee0b2e898c5b4aaaa212472 Mon Sep 17 00:00:00 2001 From: Romain Ruetschi Date: Fri, 5 Jan 2024 10:45:24 +0100 Subject: [PATCH 8/8] Fix default trust threshold to 2/3 Co-authored-by: Luca Joss <43531661+ljoss17@users.noreply.github.com> Signed-off-by: Romain Ruetschi --- crates/relayer/src/config.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/crates/relayer/src/config.rs b/crates/relayer/src/config.rs index f1bb4e7517..d7554e1b0d 100644 --- a/crates/relayer/src/config.rs +++ b/crates/relayer/src/config.rs @@ -199,7 +199,7 @@ pub mod default { } pub fn trust_threshold() -> TrustThreshold { - TrustThreshold::ONE_THIRD + TrustThreshold::TWO_THIRDS } pub fn client_refresh_rate() -> RefreshRate {