Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use max_expected_time_per_block value for the max_block_time #3467

Merged
merged 4 commits into from
Jul 11, 2023
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -0,0 +1,3 @@
- Query the /genesis RPC endpoint to retrieve the value of
`max_expected_time_per_block` and use it for `max_block_time`.
([\#3211](https://github.com/informalsystems/hermes/issues/3211))
5 changes: 4 additions & 1 deletion config.toml
Original file line number Diff line number Diff line change
Expand Up @@ -284,9 +284,12 @@ max_tx_size = 2097152
# can drift into the future. Default: 5s
clock_drift = '5s'

# Specify the maximum time per block for this chain.
# Specify the fallback value for the maximum time per block for this chain.
# The block time together with the clock drift are added to the source drift to estimate
# the maximum clock drift when creating a client on this chain. Default: 30s
# When validating the configuration, Hermes will query the /genesis RPC endpoint in order
# to retrieve the `max_expected_time_per_block` and use it as the `max_block_time`.
# If the query fails, the configured `max_block_time` is used instead.
# For cosmos-SDK chains a good approximation is `timeout_propose` + `timeout_commit`
# Note: This MUST be the same as the `max_expected_time_per_block` genesis parameter for Tendermint chains.
max_block_time = '30s'
Expand Down
30 changes: 27 additions & 3 deletions crates/relayer/src/chain/cosmos.rs
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@ use std::{cmp::Ordering, thread};
use tokio::runtime::Runtime as TokioRuntime;
use tonic::codegen::http::Uri;
use tonic::metadata::AsciiMetadataValue;
use tracing::{error, instrument, trace, warn};
use tracing::{error, info, instrument, trace, warn};

use ibc_proto::cosmos::{
base::node::v1beta1::ConfigResponse, staking::v1beta1::Params as StakingParams,
Expand Down Expand Up @@ -107,6 +107,8 @@ use crate::util::pretty::{
PrettyIdentifiedChannel, PrettyIdentifiedClientState, PrettyIdentifiedConnection,
};

use self::types::app_state::GenesisAppState;

pub mod batch;
pub mod client;
pub mod compatibility;
Expand Down Expand Up @@ -190,7 +192,7 @@ impl CosmosSdkChain {
///
/// Emits a log warning in case any error is encountered and
/// exits early without doing subsequent validations.
pub fn validate_params(&self) -> Result<(), Error> {
pub fn validate_params(&mut self) -> Result<(), Error> {
let unbonding_period = self.unbonding_period()?;
let trusting_period = self.trusting_period(unbonding_period);

Expand Down Expand Up @@ -283,6 +285,28 @@ impl CosmosSdkChain {
));
}

// Query /genesis RPC endpoint to retrieve the `max_expected_time_per_block` value
// to use as `max_block_time`.
// If it is not found, keep the configured `max_block_time`.
match self.block_on(self.rpc_client.genesis::<GenesisAppState>()) {
Ok(genesis_reponse) => {
let old_max_block_time = self.config.max_block_time;
self.config.max_block_time =
Duration::from_nanos(genesis_reponse.app_state.max_expected_time_per_block());
info!(
"Updated `max_block_time` using /genesis endpoint. Old value: `{}s`, new value: `{}s`",
old_max_block_time.as_secs(),
self.config.max_block_time.as_secs()
);
}
Err(e) => {
warn!(
"Will use fallback value for max_block_time: `{}s`. Error: {e}",
self.config.max_block_time.as_secs()
);
}
}

Ok(())
}

Expand Down Expand Up @@ -931,7 +955,7 @@ impl ChainEndpoint for CosmosSdkChain {
/// Emits a log warning in case anything is amiss.
/// Exits early if any health check fails, without doing any
/// further checks.
fn health_check(&self) -> Result<HealthCheck, Error> {
fn health_check(&mut self) -> Result<HealthCheck, Error> {
if let Err(e) = do_health_check(self) {
warn!("Health checkup for chain '{}' failed", self.id());
warn!(" Reason: {}", e.detail());
Expand Down
47 changes: 47 additions & 0 deletions crates/relayer/src/chain/cosmos/types/app_state.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,47 @@
//! Structure used to parse queried Genesis state using
//! /genesis RPC endpoint.

use serde::Deserialize as SerdeDeserialize;
use serde_derive::Deserialize;
use serde_derive::Serialize;

#[derive(Debug, Deserialize, Serialize)]
pub struct GenesisAppState {
ibc: IbcConfig,
}

impl GenesisAppState {
pub fn max_expected_time_per_block(&self) -> u64 {
self.ibc
.connection_genesis
.params
.max_expected_time_per_block
}
}

#[derive(Debug, Deserialize, Serialize)]
struct IbcConfig {
connection_genesis: ConnectionGenesisConfig,
}

#[derive(Debug, Deserialize, Serialize)]
struct ConnectionGenesisConfig {
params: ConnectionGenesisParams,
}

#[derive(Debug, Deserialize, Serialize)]
struct ConnectionGenesisParams {
#[serde(deserialize_with = "deserialize_max_expected_per_block")]
max_expected_time_per_block: u64,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we do this manually rather than pulling in serde-with which also brings darling with it? We're already quite heavy on dependencies and should try to avoid introducing new ones for a single use.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point! I'll remove the serde-with dependency

}

fn deserialize_max_expected_per_block<'de, T, D>(de: D) -> Result<T, D::Error>
where
D: serde::Deserializer<'de>,
T: std::str::FromStr,
<T as std::str::FromStr>::Err: std::fmt::Display,
{
String::deserialize(de)?
.parse()
.map_err(serde::de::Error::custom)
}
1 change: 1 addition & 0 deletions crates/relayer/src/chain/cosmos/types/mod.rs
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
pub mod account;
pub mod app_state;
pub mod config;
pub mod events;
pub mod gas;
Expand Down
2 changes: 1 addition & 1 deletion crates/relayer/src/chain/endpoint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -98,7 +98,7 @@ pub trait ChainEndpoint: Sized {
fn shutdown(self) -> Result<(), Error>;

/// Perform a health check
fn health_check(&self) -> Result<HealthCheck, Error>;
fn health_check(&mut self) -> Result<HealthCheck, Error>;

// Events
fn subscribe(&mut self) -> Result<Subscription, Error>;
Expand Down