From d0172d23df8b4c8be2a6135e10165a83df4ba10a Mon Sep 17 00:00:00 2001 From: Anca Zamfir Date: Thu, 28 Apr 2022 17:10:16 +0200 Subject: [PATCH] Use application latest height in `query_status` (#2021) * Test with abci_status * Fix status also * Remove redundant code * Move changes in chain query_application_status() * Cargo fmt * Added /blockchain-based implementation for query_application_status * Documenting impl of query_application_status. * changelog * Cleanup Co-authored-by: Adi Seredinschi --- .../relayer/1970-app-latest-height.md | 2 + relayer/src/chain.rs | 6 +- relayer/src/chain/cosmos.rs | 63 ++++++++++++++----- relayer/src/chain/handle.rs | 6 +- relayer/src/chain/handle/base.rs | 6 +- relayer/src/chain/handle/cache.rs | 6 +- relayer/src/chain/handle/counting.rs | 6 +- relayer/src/chain/mock.rs | 2 +- relayer/src/chain/runtime.rs | 8 +-- relayer/src/foreign_client.rs | 4 +- relayer/src/link/relay_path.rs | 4 +- relayer/src/transfer.rs | 6 +- relayer/src/upgrade_chain.rs | 2 +- tools/test-framework/src/relayer/chain.rs | 4 +- 14 files changed, 83 insertions(+), 42 deletions(-) create mode 100644 .changelog/unreleased/bug-fixes/relayer/1970-app-latest-height.md diff --git a/.changelog/unreleased/bug-fixes/relayer/1970-app-latest-height.md b/.changelog/unreleased/bug-fixes/relayer/1970-app-latest-height.md new file mode 100644 index 0000000000..167173977e --- /dev/null +++ b/.changelog/unreleased/bug-fixes/relayer/1970-app-latest-height.md @@ -0,0 +1,2 @@ +- Fixed query for application status when application lags blockchain state. + ([#1970](https://github.com/informalsystems/ibc-rs/issues/1970)) \ No newline at end of file diff --git a/relayer/src/chain.rs b/relayer/src/chain.rs index f94a45fa77..fc8db64234 100644 --- a/relayer/src/chain.rs +++ b/relayer/src/chain.rs @@ -65,7 +65,7 @@ pub enum HealthCheck { Unhealthy(Box), } -/// The result of a chain status query. +/// The result of the application status query. #[derive(Clone, Debug)] pub struct ChainStatus { pub height: ICSHeight, @@ -159,8 +159,8 @@ pub trait ChainEndpoint: Sized { Ok(get_compatible_versions()) } - /// Query the latest height and timestamp the chain is at - fn query_status(&self) -> Result; + /// Query the latest height and timestamp the application is at + fn query_application_status(&self) -> Result; /// Performs a query to retrieve the state of all clients that a chain hosts. fn query_clients( diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index 2e16815ff5..b59343a661 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -21,7 +21,7 @@ use tendermint_rpc::{ }; use tokio::runtime::Runtime as TokioRuntime; use tonic::codegen::http::Uri; -use tracing::{span, warn, Level}; +use tracing::{error, span, warn, Level}; use ibc::clients::ics07_tendermint::client_state::{AllowUpdate, ClientState}; use ibc::clients::ics07_tendermint::consensus_state::ConsensusState as TMConsensusState; @@ -166,7 +166,7 @@ impl CosmosSdkChain { } // Get the latest height and convert to tendermint Height - let latest_height = Height::try_from(self.query_latest_height()?.revision_height) + let latest_height = Height::try_from(self.query_chain_latest_height()?.revision_height) .map_err(Error::invalid_height)?; // Check on the configured max_tx_size against the consensus parameters at latest height @@ -356,7 +356,7 @@ impl CosmosSdkChain { /// /// Returns an error if the node is still syncing and has not caught up, /// ie. if `sync_info.catching_up` is `true`. - fn status(&self) -> Result { + fn chain_status(&self) -> Result { let status = self .block_on(self.rpc_client.status()) .map_err(|e| Error::rpc(self.config.rpc_addr.clone(), e))?; @@ -372,7 +372,7 @@ impl CosmosSdkChain { } /// Query the chain's latest height - pub fn query_latest_height(&self) -> Result { + pub fn query_chain_latest_height(&self) -> Result { crate::time!("query_latest_height"); crate::telemetry!(query, self.id(), "query_latest_height"); @@ -638,16 +638,44 @@ impl ChainEndpoint for CosmosSdkChain { .map_err(|_| Error::ics02(ClientError::empty_prefix())) } - /// Query the chain status - fn query_status(&self) -> Result { - crate::time!("query_status"); - crate::telemetry!(query, self.id(), "query_status"); + /// Query the application status + fn query_application_status(&self) -> Result { + crate::time!("query_application_status"); + crate::telemetry!(query, self.id(), "query_application_status"); - self.rt.block_on(query_status( - self.id(), - &self.rpc_client, - &self.config.rpc_addr, - )) + // We cannot rely on `/status` endpoint to provide details about the latest block. + // Instead, we need to pull block height via `/abci_info` and then fetch block + // metadata at the given height via `/blockchain` endpoint. + let abci_info = self + .block_on(self.rpc_client.abci_info()) + .map_err(|e| Error::rpc(self.config.rpc_addr.clone(), e))?; + + // Query `/blockchain` endpoint to pull the block metadata corresponding to + // the latest block that the application committed. + // TODO: Replace this query with `/header`, once it's available. + // https://github.com/informalsystems/tendermint-rs/pull/1101 + let blocks = self + .block_on( + self.rpc_client + .blockchain(abci_info.last_block_height, abci_info.last_block_height), + ) + .map_err(|e| Error::rpc(self.config.rpc_addr.clone(), e))? + .block_metas; + + return if let Some(latest_app_block) = blocks.first() { + let height = ICSHeight { + revision_number: ChainId::chain_version(latest_app_block.header.chain_id.as_str()), + revision_height: u64::from(abci_info.last_block_height), + }; + let timestamp = latest_app_block.header.time.into(); + + Ok(ChainStatus { height, timestamp }) + } else { + // The `/blockchain` query failed to return the header we wanted + Err(Error::query( + "/blockchain endpoint for latest app. block".to_owned(), + )) + }; } fn query_clients( @@ -1516,13 +1544,20 @@ fn do_health_check(chain: &CosmosSdkChain) -> Result<(), Error> { return Err(Error::no_historical_entries(chain_id.clone())); } - let status = chain.status()?; + let status = chain.chain_status()?; // Check that transaction indexing is enabled if status.node_info.other.tx_index != TxIndexStatus::On { return Err(Error::tx_indexing_disabled(chain_id.clone())); } + // Check that the chain identifier matches the network name + if !status.node_info.network.as_str().eq(chain_id.as_str()) { + // Log the error, continue optimistically + error!("/status endpoint from chain id '{}' reports network identifier to be '{}': this is usually a sign of misconfiguration, check your config.toml", + chain_id, status.node_info.network); + } + let version_specs = chain.block_on(fetch_version_specs(&chain.config.id, &chain.grpc_addr))?; // Checkup on the underlying SDK & IBC-go versions diff --git a/relayer/src/chain/handle.rs b/relayer/src/chain/handle.rs index 100a1c249c..39d121c81e 100644 --- a/relayer/src/chain/handle.rs +++ b/relayer/src/chain/handle.rs @@ -149,7 +149,7 @@ pub enum ChainRequest { reply_to: ReplyTo>, }, - QueryStatus { + QueryApplicationStatus { reply_to: ReplyTo, }, @@ -384,10 +384,10 @@ pub trait ChainHandle: Clone + Send + Sync + Serialize + Debug + 'static { /// Return the version of the IBC protocol that this chain is running, if known. fn ibc_version(&self) -> Result, Error>; - fn query_status(&self) -> Result; + fn query_application_status(&self) -> Result; fn query_latest_height(&self) -> Result { - Ok(self.query_status()?.height) + Ok(self.query_application_status()?.height) } fn query_clients( diff --git a/relayer/src/chain/handle/base.rs b/relayer/src/chain/handle/base.rs index 984144c69c..5d9d2d8de5 100644 --- a/relayer/src/chain/handle/base.rs +++ b/relayer/src/chain/handle/base.rs @@ -46,6 +46,8 @@ use crate::{ use super::{reply_channel, ChainHandle, ChainRequest, HealthCheck, ReplyTo, Subscription}; +/// A basic chain handle implementation. +/// For use in interactive CLIs, e.g., `query`, `tx raw`, etc. #[derive(Debug, Clone)] pub struct BaseChainHandle { /// Chain identifier @@ -142,8 +144,8 @@ impl ChainHandle for BaseChainHandle { self.send(|reply_to| ChainRequest::IbcVersion { reply_to }) } - fn query_status(&self) -> Result { - self.send(|reply_to| ChainRequest::QueryStatus { reply_to }) + fn query_application_status(&self) -> Result { + self.send(|reply_to| ChainRequest::QueryApplicationStatus { reply_to }) } fn query_clients( diff --git a/relayer/src/chain/handle/cache.rs b/relayer/src/chain/handle/cache.rs index d7e1b83654..e694b1818c 100644 --- a/relayer/src/chain/handle/cache.rs +++ b/relayer/src/chain/handle/cache.rs @@ -44,6 +44,8 @@ use crate::error::Error; use crate::telemetry; use crate::{connection::ConnectionMsgType, keyring::KeyEntry}; +/// A chain handle with support for caching. +/// To be used for the passive relaying mode (i.e., `start` CLI). #[derive(Debug, Clone)] pub struct CachingChainHandle { inner: Handle, @@ -127,8 +129,8 @@ impl ChainHandle for CachingChainHandle { self.inner().ibc_version() } - fn query_status(&self) -> Result { - self.inner().query_status() + fn query_application_status(&self) -> Result { + self.inner().query_application_status() } fn query_latest_height(&self) -> Result { diff --git a/relayer/src/chain/handle/counting.rs b/relayer/src/chain/handle/counting.rs index d094d04c84..c6d9c3a07f 100644 --- a/relayer/src/chain/handle/counting.rs +++ b/relayer/src/chain/handle/counting.rs @@ -155,9 +155,9 @@ impl ChainHandle for CountingChainHandle { self.inner().ibc_version() } - fn query_status(&self) -> Result { - self.inc_metric("query_status"); - self.inner().query_status() + fn query_application_status(&self) -> Result { + self.inc_metric("query_application_status"); + self.inner().query_application_status() } fn query_latest_height(&self) -> Result { diff --git a/relayer/src/chain/mock.rs b/relayer/src/chain/mock.rs index f26f60ce36..190da5e943 100644 --- a/relayer/src/chain/mock.rs +++ b/relayer/src/chain/mock.rs @@ -170,7 +170,7 @@ impl ChainEndpoint for MockChain { unimplemented!() } - fn query_status(&self) -> Result { + fn query_application_status(&self) -> Result { Ok(ChainStatus { height: self.context.host_height(), timestamp: self.context.host_timestamp(), diff --git a/relayer/src/chain/runtime.rs b/relayer/src/chain/runtime.rs index e4f74aba8c..4518efba54 100644 --- a/relayer/src/chain/runtime.rs +++ b/relayer/src/chain/runtime.rs @@ -303,8 +303,8 @@ where self.build_channel_proofs(port_id, channel_id, height, reply_to)? }, - Ok(ChainRequest::QueryStatus { reply_to }) => { - self.query_status(reply_to)? + Ok(ChainRequest::QueryApplicationStatus { reply_to }) => { + self.query_application_status(reply_to)? } Ok(ChainRequest::QueryClients { request, reply_to }) => { @@ -465,8 +465,8 @@ where reply_to.send(result).map_err(Error::send) } - fn query_status(&self, reply_to: ReplyTo) -> Result<(), Error> { - let latest_timestamp = self.chain.query_status(); + fn query_application_status(&self, reply_to: ReplyTo) -> Result<(), Error> { + let latest_timestamp = self.chain.query_application_status(); reply_to.send(latest_timestamp).map_err(Error::send) } diff --git a/relayer/src/foreign_client.rs b/relayer/src/foreign_client.rs index 8812714f06..88500ba8cf 100644 --- a/relayer/src/foreign_client.rs +++ b/relayer/src/foreign_client.rs @@ -777,7 +777,7 @@ impl ForeignClient Result<(), ForeignClientError> { // Get latest height and time on destination chain - let mut status = self.dst_chain().query_status().map_err(|e| { + let mut status = self.dst_chain().query_application_status().map_err(|e| { ForeignClientError::client_update( self.dst_chain.id(), "failed querying latest status of the destination chain".to_string(), @@ -803,7 +803,7 @@ impl ForeignClient RelayPath { let dst_latest_info = self .dst_chain() - .query_status() + .query_application_status() .map_err(|e| LinkError::query(self.src_chain().id(), e))?; let dst_latest_height = dst_latest_info.height; // Operational data targeting the source chain (e.g., Timeout packets) @@ -1460,7 +1460,7 @@ impl RelayPath { let dst_status = self .dst_chain() - .query_status() + .query_application_status() .map_err(|e| LinkError::query(self.src_chain().id(), e))?; let dst_current_height = dst_status.height; diff --git a/relayer/src/transfer.rs b/relayer/src/transfer.rs index e9d950e1e5..111f862ac9 100644 --- a/relayer/src/transfer.rs +++ b/relayer/src/transfer.rs @@ -141,14 +141,14 @@ pub fn build_and_send_transfer_messages Result { let upgrade_height = dst_chain - .query_latest_height() + .query_chain_latest_height() .map_err(UpgradeChainError::query)? .add(opts.height_offset); diff --git a/tools/test-framework/src/relayer/chain.rs b/tools/test-framework/src/relayer/chain.rs index 3d2c00be6b..c9d51931b4 100644 --- a/tools/test-framework/src/relayer/chain.rs +++ b/tools/test-framework/src/relayer/chain.rs @@ -128,8 +128,8 @@ where self.value().ibc_version() } - fn query_status(&self) -> Result { - self.value().query_status() + fn query_application_status(&self) -> Result { + self.value().query_application_status() } fn query_latest_height(&self) -> Result {