diff --git a/e2e/e2e/client.py b/e2e/e2e/client.py index 05b7586068..89941d61bc 100644 --- a/e2e/e2e/client.py +++ b/e2e/e2e/client.py @@ -37,7 +37,7 @@ class ClientUpdated: @dataclass -@cmd("tx raw update-client") +@cmd("update client") class TxUpdateClient(Cmd[ClientUpdated]): dst_chain_id: ChainId dst_client_id: ClientId diff --git a/modules/src/ics02_client/height.rs b/modules/src/ics02_client/height.rs index 5f29755f90..c3dd1b542a 100644 --- a/modules/src/ics02_client/height.rs +++ b/modules/src/ics02_client/height.rs @@ -1,5 +1,6 @@ use std::cmp::Ordering; use std::convert::{Infallible, TryFrom}; +use std::fmt::{Debug, Display}; use std::str::FromStr; use serde_derive::{Deserialize, Serialize}; @@ -18,6 +19,46 @@ pub struct Height { pub revision_height: u64, } +// A hack to indicate that a height is non-zero. +// TODO: Make all height non-zero and use Option for +// optional height parameter: +// https://github.com/informalsystems/ibc-rs/issues/1009 +#[derive(Debug, Copy, Clone, PartialEq, Eq, Ord, PartialOrd, Hash, Serialize, Deserialize)] +pub struct NonZeroHeight(Height); + +impl NonZeroHeight { + pub fn new(height: Height) -> Option { + if height == Height::zero() { + None + } else { + Some(NonZeroHeight(height)) + } + } + + /// Unsafe creation of a NonZeroHeight that may be zero. + /// This unfortunate counterintuitive function is due to the code base + /// right now using zero height to indicate for both the absense of height + /// and also zero height. + /// + /// Use this function to create zero height that bypass all checks that + /// behave differently on zero height by treating them as absense of height. + /// + /// This function can also be used for unfailable creation of + /// non-zero heights without the use of panics. The invariants + /// then have to be guaranteed by the caller. + /// + /// This function is unsafe, because we may be accidentally be converting + /// zero heights that are meant to indicate absense on height to actual + /// zero height and cause unexpected behavior. + pub fn unsafe_new(height: Height) -> Self { + NonZeroHeight(height) + } + + pub fn height(self) -> Height { + self.0 + } +} + impl Height { pub fn new(revision_number: u64, revision_height: u64) -> Self { Self { @@ -123,7 +164,7 @@ impl From for RawHeight { } } -impl std::fmt::Debug for Height { +impl Debug for Height { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { f.debug_struct("Height") .field("revision", &self.revision_number) @@ -132,13 +173,18 @@ impl std::fmt::Debug for Height { } } -/// Custom debug output to omit the packet data -impl std::fmt::Display for Height { +impl Display for Height { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { write!(f, "{}-{}", self.revision_number, self.revision_height) } } +impl Display for NonZeroHeight { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> Result<(), std::fmt::Error> { + write!(f, "{}", self.0) + } +} + impl TryFrom<&str> for Height { type Error = Kind; diff --git a/modules/src/lib.rs b/modules/src/lib.rs index e1cfdc510c..a5046f1a3a 100644 --- a/modules/src/lib.rs +++ b/modules/src/lib.rs @@ -53,6 +53,7 @@ mod serializers; /// Re-export of ICS 002 Height domain type pub type Height = crate::ics02_client::height::Height; +pub type NonZeroHeight = crate::ics02_client::height::NonZeroHeight; #[cfg(test)] mod test; diff --git a/modules/src/mock/context.rs b/modules/src/mock/context.rs index 7ff4cd13dc..644c5dab33 100644 --- a/modules/src/mock/context.rs +++ b/modules/src/mock/context.rs @@ -421,14 +421,17 @@ impl MockContext { } pub fn consensus_states(&self, client_id: &ClientId) -> Vec { - self.clients[client_id] - .consensus_states - .iter() - .map(|(k, v)| AnyConsensusStateWithHeight { - height: *k, - consensus_state: v.clone(), - }) - .collect() + match self.clients.get(client_id) { + Some(client) => client + .consensus_states + .iter() + .map(|(k, v)| AnyConsensusStateWithHeight { + height: *k, + consensus_state: v.clone(), + }) + .collect(), + None => Vec::new(), + } } } diff --git a/relayer-cli/src/commands/tx/client.rs b/relayer-cli/src/commands/tx/client.rs index a3f82fdeb5..fd809da1c2 100644 --- a/relayer-cli/src/commands/tx/client.rs +++ b/relayer-cli/src/commands/tx/client.rs @@ -3,6 +3,7 @@ use abscissa_core::{Command, Options, Runnable}; use ibc::events::IbcEvent; use ibc::ics02_client::client_state::ClientState; use ibc::ics24_host::identifier::{ChainId, ClientId}; +use ibc::NonZeroHeight; use ibc_relayer::foreign_client::ForeignClient; use crate::application::app_config; @@ -60,59 +61,59 @@ pub struct TxUpdateClientCmd { )] dst_client_id: ClientId, - #[options(help = "the target height of the client update", short = "h")] - target_height: Option, + #[options(required, help = "the target height of the client update", short = "h")] + target_height: u64, - #[options(help = "the trusted height of the client update", short = "t")] - trusted_height: Option, + #[options( + required, + help = "the trusted height of the client update", + short = "t" + )] + trusted_height: u64, } -impl Runnable for TxUpdateClientCmd { - fn run(&self) { +impl TxUpdateClientCmd { + fn execute(&self) -> Result, String> { let config = app_config(); - let dst_chain = match spawn_chain_runtime(&config, &self.dst_chain_id) { - Ok(handle) => handle, - Err(e) => return Output::error(format!("{}", e)).exit(), - }; + let dst_chain = + spawn_chain_runtime(&config, &self.dst_chain_id).map_err(|e| format!("{}", e))?; - let src_chain_id = - match dst_chain.query_client_state(&self.dst_client_id, ibc::Height::zero()) { - Ok(cs) => cs.chain_id(), - Err(e) => { - return Output::error(format!( - "Query of client '{}' on chain '{}' failed with error: {}", - self.dst_client_id, self.dst_chain_id, e - )) - .exit() - } - }; + let src_chain_id = dst_chain + .query_client_state(&self.dst_client_id, ibc::Height::zero()) + .map_err(|e| { + format!( + "Query of client '{}' on chain '{}' failed with error: {}", + self.dst_client_id, self.dst_chain_id, e + ) + })? + .chain_id(); - let src_chain = match spawn_chain_runtime(&config, &src_chain_id) { - Ok(handle) => handle, - Err(e) => return Output::error(format!("{}", e)).exit(), - }; + let src_chain = + spawn_chain_runtime(&config, &src_chain_id).map_err(|e| format!("{}", e))?; - let height = match self.target_height { - Some(height) => ibc::Height::new(src_chain.id().version(), height), - None => ibc::Height::zero(), - }; - - let trusted_height = match self.trusted_height { - Some(height) => ibc::Height::new(src_chain.id().version(), height), - None => ibc::Height::zero(), - }; + let src_chain_id = src_chain.id().version(); let client = ForeignClient::find(src_chain, dst_chain, &self.dst_client_id) - .unwrap_or_else(exit_with_unrecoverable_error); + .map_err(|e| format!("failed to find foreign client: {}", e))?; - let res: Result = client - .build_update_client_and_send(height, trusted_height) - .map_err(|e| Kind::Tx.context(e).into()); + let target_height = + NonZeroHeight::unsafe_new(ibc::Height::new(src_chain_id, self.target_height)); - match res { + let trusted_height = + NonZeroHeight::unsafe_new(ibc::Height::new(src_chain_id, self.trusted_height)); + + client + .build_update_client_and_send(target_height, trusted_height) + .map_err(|e| e.to_string()) + } +} + +impl Runnable for TxUpdateClientCmd { + fn run(&self) { + match self.execute() { Ok(receipt) => Output::success(receipt).exit(), - Err(e) => Output::error(format!("{}", e)).exit(), + Err(e) => Output::error(e).exit(), } } } diff --git a/relayer-cli/src/commands/update.rs b/relayer-cli/src/commands/update.rs index 46d09b55d6..e0ca9e0012 100644 --- a/relayer-cli/src/commands/update.rs +++ b/relayer-cli/src/commands/update.rs @@ -1,8 +1,94 @@ //! `update` subcommand use abscissa_core::{Command, Help, Options, Runnable}; +use ibc::events::IbcEvent; +use ibc::ics02_client::client_state::ClientState; +use ibc::ics24_host::identifier::{ChainId, ClientId}; +use ibc::NonZeroHeight; +use ibc_relayer::foreign_client::ForeignClient; -use crate::commands::tx::client::TxUpdateClientCmd; +use crate::application::app_config; +use crate::cli_utils::spawn_chain_runtime; +use crate::conclude::Output; + +#[derive(Clone, Command, Debug, Options)] +pub struct UpdateClientCmd { + #[options(free, required, help = "identifier of the destination chain")] + dst_chain_id: ChainId, + + #[options( + free, + required, + help = "identifier of the client to be updated on destination chain" + )] + dst_client_id: ClientId, + + #[options(help = "the target height of the client update", short = "h")] + target_height: Option, +} + +impl UpdateClientCmd { + fn execute(&self) -> Result, String> { + let config = app_config(); + + let dst_chain = + spawn_chain_runtime(&config, &self.dst_chain_id).map_err(|e| format!("{}", e))?; + + let src_chain_id = dst_chain + .query_client_state(&self.dst_client_id, ibc::Height::zero()) + .map_err(|e| { + format!( + "Query of client '{}' on chain '{}' failed with error: {}", + self.dst_client_id, self.dst_chain_id, e + ) + })? + .chain_id(); + + let src_chain = + spawn_chain_runtime(&config, &src_chain_id).map_err(|e| format!("{}", e))?; + + let src_chain_id = src_chain.id().version(); + + let client = ForeignClient::find(src_chain, dst_chain, &self.dst_client_id) + .map_err(|e| format!("failed to find foreign client: {}", e))?; + + let m_target_height = self + .target_height + .and_then(|h| NonZeroHeight::new(ibc::Height::new(src_chain_id, h))); + + let target_height = match m_target_height { + Some(height) => { + client.wait_src_chain_reach_height(height).map_err(|e| { + format!( + "failed to wait for client to reach height {}: {}", + height, e + ) + })?; + height + } + None => client + .latest_src_chain_height() + .map_err(|e| e.to_string())?, + }; + + let trusted_height = client + .latest_consensus_state_height() + .map_err(|e| e.to_string())?; + + client + .build_update_client_and_send(target_height, trusted_height) + .map_err(|e| e.to_string()) + } +} + +impl Runnable for UpdateClientCmd { + fn run(&self) { + match self.execute() { + Ok(receipt) => Output::success(receipt).exit(), + Err(e) => Output::error(e).exit(), + } + } +} #[derive(Command, Debug, Options, Runnable)] pub enum UpdateCmds { @@ -12,5 +98,5 @@ pub enum UpdateCmds { /// Subcommand for updating a `client` #[options(help = "Update an IBC client")] - Client(TxUpdateClientCmd), + Client(UpdateClientCmd), } diff --git a/relayer/src/chain/cosmos.rs b/relayer/src/chain/cosmos.rs index a58145e830..edb9e399cb 100644 --- a/relayer/src/chain/cosmos.rs +++ b/relayer/src/chain/cosmos.rs @@ -18,6 +18,7 @@ use tendermint_rpc::query::Query; use tendermint_rpc::{endpoint::broadcast::tx_commit::Response, Client, HttpClient, Order}; use tokio::runtime::Runtime as TokioRuntime; use tonic::codegen::http::Uri; +use tracing::debug; use ibc::downcast; use ibc::events::{from_tx_response_event, IbcEvent}; @@ -1170,6 +1171,11 @@ impl Chain for CosmosSdkChain { let connection_end = ConnectionEnd::decode_vec(&res.value) .map_err(|e| Kind::Query("proven connection".into()).context(e))?; + debug!( + "got proven connection response for connection {} at height {}: {:#?}", + connection_id, height, res + ); + Ok(( connection_end, res.proof.ok_or_else(|| { diff --git a/relayer/src/channel.rs b/relayer/src/channel.rs index 481d8d9c8f..1636c5d938 100644 --- a/relayer/src/channel.rs +++ b/relayer/src/channel.rs @@ -16,7 +16,7 @@ use ibc::ics04_channel::msgs::chan_open_init::MsgChannelOpenInit; use ibc::ics04_channel::msgs::chan_open_try::MsgChannelOpenTry; use ibc::ics24_host::identifier::{ChainId, ChannelId, ClientId, ConnectionId, PortId}; use ibc::tx_msg::Msg; -use ibc::Height; +use ibc::{Height, NonZeroHeight}; use crate::chain::handle::ChainHandle; use crate::connection::Connection; @@ -515,7 +515,10 @@ impl Channel { self.step_state(state, index) } - pub fn build_update_client_on_dst(&self, height: Height) -> Result, ChannelError> { + pub fn build_update_client_on_dst( + &self, + height: NonZeroHeight, + ) -> Result, ChannelError> { let client = ForeignClient::restore( self.dst_client_id().clone(), self.dst_chain().clone(), @@ -716,8 +719,11 @@ impl Channel { .build_channel_proofs(self.src_port_id(), src_channel_id, query_height) .map_err(|e| ChannelError::Failed(format!("failed to build channel proofs: {}", e)))?; + let proof_height = NonZeroHeight::new(proofs.height()) + .ok_or_else(|| ChannelError::Failed("unexpected zero height".to_string()))?; + // Build message(s) to update client on destination - let mut msgs = self.build_update_client_on_dst(proofs.height())?; + let mut msgs = self.build_update_client_on_dst(proof_height)?; let counterparty = Counterparty::new(self.src_port_id().clone(), self.src_channel_id().cloned()); @@ -824,8 +830,11 @@ impl Channel { )) })?; + let proof_height = NonZeroHeight::new(proofs.height()) + .ok_or_else(|| ChannelError::Failed("unexpected zero height".to_string()))?; + // Build message(s) to update client on destination - let mut msgs = self.build_update_client_on_dst(proofs.height())?; + let mut msgs = self.build_update_client_on_dst(proof_height)?; // Get signer let signer = self.dst_chain().get_signer().map_err(|e| { @@ -910,8 +919,11 @@ impl Channel { .build_channel_proofs(self.src_port_id(), src_channel_id, query_height) .map_err(|e| ChannelError::Failed(format!("failed to build channel proofs: {}", e)))?; + let proof_height = NonZeroHeight::new(proofs.height()) + .ok_or_else(|| ChannelError::Failed("unexpected zero height".to_string()))?; + // Build message(s) to update client on destination - let mut msgs = self.build_update_client_on_dst(proofs.height())?; + let mut msgs = self.build_update_client_on_dst(proof_height)?; // Get signer let signer = self.dst_chain().get_signer().map_err(|e| { @@ -1052,8 +1064,11 @@ impl Channel { .build_channel_proofs(self.src_port_id(), src_channel_id, query_height) .map_err(|e| ChannelError::Failed(format!("failed to build channel proofs: {}", e)))?; + let proof_height = NonZeroHeight::new(proofs.height()) + .ok_or_else(|| ChannelError::Failed("unexpected zero height".to_string()))?; + // Build message(s) to update client on destination - let mut msgs = self.build_update_client_on_dst(proofs.height())?; + let mut msgs = self.build_update_client_on_dst(proof_height)?; // Get signer let signer = self.dst_chain().get_signer().map_err(|e| { diff --git a/relayer/src/connection.rs b/relayer/src/connection.rs index b9fb549418..2979609ca0 100644 --- a/relayer/src/connection.rs +++ b/relayer/src/connection.rs @@ -17,7 +17,7 @@ use ibc::ics03_connection::msgs::conn_open_try::MsgConnectionOpenTry; use ibc::ics24_host::identifier::{ChainId, ClientId, ConnectionId}; use ibc::timestamp::ZERO_DURATION; use ibc::tx_msg::Msg; -use ibc::Height as ICSHeight; +use ibc::{Height as ICSHeight, NonZeroHeight}; use crate::chain::handle::ChainHandle; use crate::error::Error; @@ -411,14 +411,20 @@ impl Connection { Ok(dst_expected_connection) } - pub fn build_update_client_on_src(&self, height: Height) -> Result, ConnectionError> { + pub fn build_update_client_on_src( + &self, + height: NonZeroHeight, + ) -> Result, ConnectionError> { let client = self.restore_src_client(); client.build_update_client(height).map_err(|e| { ConnectionError::ClientOperation(self.src_client_id().clone(), self.src_chain().id(), e) }) } - pub fn build_update_client_on_dst(&self, height: Height) -> Result, ConnectionError> { + pub fn build_update_client_on_dst( + &self, + height: NonZeroHeight, + ) -> Result, ConnectionError> { let client = self.restore_dst_client(); client.build_update_client(height).map_err(|e| { ConnectionError::ClientOperation(self.dst_client_id().clone(), self.dst_chain().id(), e) @@ -520,6 +526,10 @@ impl Connection { .dst_chain() .query_latest_height() .map_err(|e| ConnectionError::QueryError(self.dst_chain().id(), e))?; + + let src_client_target_height = NonZeroHeight::new(src_client_target_height) + .ok_or_else(|| ConnectionError::Failed("unexpected zero height".to_string()))?; + let client_msgs = self.build_update_client_on_src(src_client_target_height)?; self.src_chain() .send_msgs(client_msgs) @@ -541,8 +551,11 @@ impl Connection { ConnectionError::Failed(format!("failed to build connection proofs: {}", e)) })?; + let proof_height = NonZeroHeight::new(proofs.height()) + .ok_or_else(|| ConnectionError::Failed("unexpected zero height".to_string()))?; + // Build message(s) for updating client on destination - let mut msgs = self.build_update_client_on_dst(proofs.height())?; + let mut msgs = self.build_update_client_on_dst(proof_height)?; let counterparty_versions = if src_connection.versions().is_empty() { self.src_chain() @@ -637,7 +650,12 @@ impl Connection { .dst_chain() .query_latest_height() .map_err(|e| ConnectionError::QueryError(self.dst_chain().id(), e))?; + + let src_client_target_height = NonZeroHeight::new(src_client_target_height) + .ok_or_else(|| ConnectionError::Failed("unexpected zero height".to_string()))?; + let client_msgs = self.build_update_client_on_src(src_client_target_height)?; + self.src_chain() .send_msgs(client_msgs) .map_err(|e| ConnectionError::SubmitError(self.src_chain().id(), e))?; @@ -658,8 +676,11 @@ impl Connection { ConnectionError::Failed(format!("failed to build connection proofs: {}", e)) })?; + let proof_height = NonZeroHeight::new(proofs.height()) + .ok_or_else(|| ConnectionError::Failed("unexpected zero height".to_string()))?; + // Build message(s) for updating client on destination - let mut msgs = self.build_update_client_on_dst(proofs.height())?; + let mut msgs = self.build_update_client_on_dst(proof_height)?; // Get signer let signer = self.dst_chain().get_signer().map_err(|e| { @@ -748,8 +769,11 @@ impl Connection { ConnectionError::Failed(format!("failed to build connection proofs: {}", e)) })?; + let proof_height = NonZeroHeight::new(proofs.height()) + .ok_or_else(|| ConnectionError::Failed("unexpected zero height".to_string()))?; + // Build message(s) for updating client on destination - let mut msgs = self.build_update_client_on_dst(proofs.height())?; + let mut msgs = self.build_update_client_on_dst(proof_height)?; // Get signer let signer = self.dst_chain().get_signer().map_err(|e| { diff --git a/relayer/src/foreign_client.rs b/relayer/src/foreign_client.rs index dad8993dd5..0c1352e7b4 100644 --- a/relayer/src/foreign_client.rs +++ b/relayer/src/foreign_client.rs @@ -22,7 +22,7 @@ use ibc::ics24_host::identifier::{ChainId, ClientId}; use ibc::query::QueryTxRequest; use ibc::timestamp::Timestamp; use ibc::tx_msg::Msg; -use ibc::Height; +use ibc::{Height, NonZeroHeight}; use ibc_proto::ibc::core::client::v1::QueryConsensusStatesRequest; use crate::chain::handle::ChainHandle; @@ -188,7 +188,11 @@ impl ForeignClient { info!("[{}] upgrade Height: {}", self, src_height); - let mut msgs = self.build_update_client(src_height)?; + let target_height = NonZeroHeight::new(src_height).ok_or_else(|| { + ForeignClientError::ClientUpgrade(self.id.clone(), "unexpected zero height".to_string()) + })?; + + let mut msgs = self.build_update_client(target_height)?; // Query the host chain for the upgraded client state, consensus state & their proofs. let (client_state, proof_upgrade_client) = self @@ -387,7 +391,6 @@ impl ForeignClient { if elapsed > refresh_window { info!("[{}] client requires refresh", self); self.build_latest_update_client_and_send() - .map_or_else(Err, |ev| Ok(Some(ev))) } else { Ok(None) } @@ -395,34 +398,62 @@ impl ForeignClient { } } + /// Wait for the source chain to reach a certain target height. + /// This will loop forever until the target height is reached, + /// or when there is an error querying the height. + pub fn wait_src_chain_reach_height( + &self, + target_height: NonZeroHeight, + ) -> Result<(), ForeignClientError> { + loop { + debug!( + "waiting for chain {} to reach height {}", + self.src_chain().id(), + target_height + ); + let current_height = self.src_chain().query_latest_height().map_err(|e| { + ForeignClientError::ClientQuery( + self.id.clone(), + self.src_chain().id(), + format!("failed fetching src chain latest height with error: {}", e), + ) + })?; + + if current_height >= target_height.height() { + debug!( + "chain {} has reached height {}", + self.src_chain().id(), + current_height + ); + return Ok(()); + } else { + thread::sleep(Duration::from_millis(100)); + } + } + } + /// Wrapper for build_update_client_with_trusted. pub fn build_update_client( &self, - target_height: Height, + target_height: NonZeroHeight, ) -> Result, ForeignClientError> { - self.build_update_client_with_trusted(target_height, Height::zero()) + self.wait_src_chain_reach_height(target_height)?; + let trusted_height = self.latest_consensus_state_height()?; + self.build_update_client_with_trusted(target_height, trusted_height) } /// Returns a vector with a message for updating the client to height `target_height`. /// If the client already stores consensus states for this height, returns an empty vector. pub fn build_update_client_with_trusted( &self, - target_height: Height, - trusted_height: Height, + target_height: NonZeroHeight, + trusted_height: NonZeroHeight, ) -> Result, ForeignClientError> { - // Wait for source chain to reach `target_height` - while self.src_chain().query_latest_height().map_err(|e| { - ForeignClientError::ClientUpdate(format!( - "failed fetching src chain latest height with error: {}", - e - )) - })? < target_height - { - thread::sleep(Duration::from_millis(100)) - } + debug!("build_update_client_with_trusted with source chain: {}, dst_chain: {}, target_height: {}, trusted_height: {}", + self.src_chain().id(), self.dst_chain().id(), target_height, trusted_height); // Get the latest client state on destination. - let client_state = self + let destination_state = self .dst_chain() .query_client_state(&self.id, Height::default()) .map_err(|e| { @@ -432,34 +463,6 @@ impl ForeignClient { )) })?; - // If not specified, set trusted state to the highest height smaller than target height. - // Otherwise ensure that a consensus state at trusted height exists on-chain. - let cs_heights = self.consensus_state_heights()?; - let trusted_height = if trusted_height == Height::zero() { - // Get highest height smaller than target height - cs_heights - .into_iter() - .find(|h| h < &target_height) - .ok_or_else(|| { - ForeignClientError::ClientUpdate(format!( - "chain {} is missing trusted state smaller than target height {}", - self.dst_chain().id(), - target_height - )) - })? - } else { - cs_heights - .into_iter() - .find(|h| h == &trusted_height) - .ok_or_else(|| { - ForeignClientError::ClientUpdate(format!( - "chain {} is missing trusted state at height {}", - self.dst_chain().id(), - trusted_height - )) - })? - }; - if trusted_height >= target_height { warn!( "[{}] skipping update: trusted height ({}) >= chain target height ({})", @@ -470,7 +473,11 @@ impl ForeignClient { let header = self .src_chain() - .build_header(trusted_height, target_height, client_state) + .build_header( + trusted_height.height(), + target_height.height(), + destination_state, + ) .map_err(|e| { ForeignClientError::ClientUpdate(format!( "failed building header with error: {}", @@ -501,35 +508,34 @@ impl ForeignClient { Ok(vec![new_msg.to_any()]) } - pub fn build_latest_update_client_and_send(&self) -> Result { - self.build_update_client_and_send(Height::zero(), Height::zero()) + pub fn build_latest_update_client_and_send( + &self, + ) -> Result, ForeignClientError> { + let target_height = self.latest_src_chain_height()?; + let trusted_height = self.latest_consensus_state_height()?; + + debug!( + "build_latest_update_client_and_send with trusted_height: {}, target_height: {}", + trusted_height, target_height + ); + + self.build_update_client_and_send(target_height, trusted_height) } pub fn build_update_client_and_send( &self, - height: Height, - trusted_height: Height, - ) -> Result { - let h = if height == Height::zero() { - self.src_chain.query_latest_height().map_err(|e| { - ForeignClientError::ClientUpdate(format!( - "failed while querying src chain ({}) for latest height: {}", - self.src_chain.id(), - e - )) - })? - } else { - height - }; - - let new_msgs = self.build_update_client_with_trusted(h, trusted_height)?; + target_height: NonZeroHeight, + trusted_height: NonZeroHeight, + ) -> Result, ForeignClientError> { + let new_msgs = self.build_update_client_with_trusted(target_height, trusted_height)?; if new_msgs.is_empty() { - return Err(ForeignClientError::ClientUpdate(format!( + info!( "Client {} is already up-to-date with chain {}@{}", self.id, self.src_chain.id(), - h - ))); + target_height + ); + return Ok(None); } let mut events = self.dst_chain().send_msgs(new_msgs).map_err(|e| { @@ -540,9 +546,7 @@ impl ForeignClient { )) })?; - assert!(!events.is_empty()); - - Ok(events.pop().unwrap()) + Ok(events.pop()) } /// Attempts to update a client using header from the latest height of its source chain. @@ -672,6 +676,37 @@ impl ForeignClient { Ok(consensus_state_heights) } + /// Returns the latest consensus state height for a client + pub fn latest_consensus_state_height(&self) -> Result { + let height = self + .consensus_states()? + .iter() + .map(|cs| cs.height) + .max() + .ok_or_else(|| { + ForeignClientError::ClientUpdate(format!( + "chain {} is missing trusted state", + self.dst_chain().id(), + )) + })?; + + NonZeroHeight::new(height) + .ok_or_else(|| ForeignClientError::ClientUpdate("unexpected zero height".to_string())) + } + + pub fn latest_src_chain_height(&self) -> Result { + let height = self.src_chain.query_latest_height().map_err(|e| { + ForeignClientError::ClientUpdate(format!( + "failed while querying src chain ({}) for latest height: {}", + self.src_chain.id(), + e + )) + })?; + + NonZeroHeight::new(height) + .ok_or_else(|| ForeignClientError::ClientUpdate("unexpected zero height".to_string())) + } + /// Checks for evidence of misbehaviour. /// The check starts with and `update_event` emitted by chain B (`dst_chain`) for a client update /// with a header from chain A (`src_chain`). The algorithm goes backwards through the headers @@ -936,6 +971,7 @@ mod test { use std::sync::Arc; use tokio::runtime::Runtime as TokioRuntime; + use tracing::debug; use ibc::events::IbcEvent; use ibc::ics24_host::identifier::ClientId; @@ -999,22 +1035,18 @@ mod test { ForeignClient::restore(ClientId::default(), b_chain.clone(), a_chain.clone()); // This action should fail because no client exists (yet) - let res = a_client.build_latest_update_client_and_send(); - assert!( - res.is_err(), - "build_update_client_and_send was supposed to fail (no client existed)" - ); + a_client + .build_latest_update_client_and_send() + .expect_err("build_update_client_and_send was supposed to fail (no client existed)"); // Remember b's height. let b_height_start = b_chain.clone().query_latest_height().unwrap(); + debug!("b_height_start: {}", b_height_start); // Create a client on chain a - let res = a_client.create(); - assert!( - res.is_ok(), - "build_create_client_and_send failed (chain a) with error {:?}", - res - ); + a_client + .create() + .expect("build_create_client_and_send failed (chain a)"); // TODO: optionally add return events from `create` and assert on the event type, e.g.: // assert!(matches!(res.as_ref().unwrap(), IBCEvent::CreateClient(_))); @@ -1023,10 +1055,13 @@ mod test { // This should fail because the client on chain a already has the latest headers. Chain b, // the source chain for the client on a, is at the same height where it was when the client // was created, so an update should fail here. - let res = a_client.build_latest_update_client_and_send(); + let res = a_client + .build_latest_update_client_and_send() + .expect("failed to update client"); + assert!( - res.is_err(), - "build_update_client_and_send was supposed to fail", + res.is_none(), + "build_update_client_and_send was supposed to return no event", ); // Remember b's height. @@ -1049,15 +1084,17 @@ mod test { // Remember the current height of chain a let mut a_height_last = a_chain.query_latest_height().unwrap(); + debug!( + "a_height_last: {}, b_height_last: {}", + a_height_last, b_height_last + ); // Now we can update both clients -- a ping pong, similar to ICS18 `client_update_ping_pong` for _i in 1..num_iterations { - let res = a_client.build_latest_update_client_and_send(); - assert!( - res.is_ok(), - "build_update_client_and_send failed (chain a) with error: {:?}", - res - ); + let res = a_client + .build_latest_update_client_and_send() + .expect("build_update_client_and_send failed (chain a) with error"); + assert!(matches!(res.as_ref().unwrap(), IbcEvent::UpdateClient(_))); let a_height_current = a_chain.query_latest_height().unwrap(); @@ -1068,12 +1105,10 @@ mod test { ); // And also update the client on chain b. - let res = b_client.build_latest_update_client_and_send(); - assert!( - res.is_ok(), - "build_update_client_and_send failed (chain b) with error: {:?}", - res - ); + let res = b_client + .build_latest_update_client_and_send() + .expect("build_update_client_and_send failed (chain b) with error"); + assert!(matches!(res.as_ref().unwrap(), IbcEvent::UpdateClient(_))); let b_height_current = b_chain.query_latest_height().unwrap(); diff --git a/relayer/src/link.rs b/relayer/src/link.rs index 685451c7c1..3707fdd128 100644 --- a/relayer/src/link.rs +++ b/relayer/src/link.rs @@ -27,7 +27,7 @@ use ibc::{ signer::Signer, timestamp::ZERO_DURATION, tx_msg::Msg, - Height, + Height, NonZeroHeight, }; use ibc_proto::ibc::core::channel::v1::{ @@ -144,7 +144,8 @@ impl OperationalData { // For zero delay we prepend the client update msgs. if relay_path.zero_delay() { - let update_height = self.proofs_height.increment(); + let update_height = NonZeroHeight::new(self.proofs_height.increment()) + .ok_or_else(|| LinkError::Failed("unexpected zero height".to_string()))?; info!( "[{}] prepending {} client update @ height {}", @@ -314,15 +315,25 @@ impl RelayPath { self.channel.ordering == Order::Ordered } - pub fn build_update_client_on_dst(&self, height: Height) -> Result, LinkError> { + pub fn build_update_client_on_dst(&self, height: NonZeroHeight) -> Result, LinkError> { let client = self.restore_dst_client(); + + client + .wait_src_chain_reach_height(height) + .map_err(LinkError::ClientError)?; + client .build_update_client(height) .map_err(LinkError::ClientError) } - pub fn build_update_client_on_src(&self, height: Height) -> Result, LinkError> { + pub fn build_update_client_on_src(&self, height: NonZeroHeight) -> Result, LinkError> { let client = self.restore_src_client(); + + client + .wait_src_chain_reach_height(height) + .map_err(LinkError::ClientError)?; + client .build_update_client(height) .map_err(LinkError::ClientError) @@ -800,12 +811,16 @@ impl RelayPath { } /// Handles updating the client on the destination chain - fn update_client_dst(&self, src_chain_height: Height) -> Result<(), LinkError> { + fn update_client_dst(&self, src_chain_height: NonZeroHeight) -> Result<(), LinkError> { // Handle the update on the destination chain // Check if a consensus state at update_height exists on destination chain already if self .dst_chain() - .proven_client_consensus(self.dst_client_id(), src_chain_height, Height::zero()) + .proven_client_consensus( + self.dst_client_id(), + src_chain_height.height(), + Height::zero(), + ) .is_ok() { return Ok(()); @@ -844,10 +859,14 @@ impl RelayPath { } /// Handles updating the client on the source chain - fn update_client_src(&self, dst_chain_height: Height) -> Result<(), LinkError> { + fn update_client_src(&self, dst_chain_height: NonZeroHeight) -> Result<(), LinkError> { if self .src_chain() - .proven_client_consensus(self.src_client_id(), dst_chain_height, Height::zero()) + .proven_client_consensus( + self.src_client_id(), + dst_chain_height.height(), + Height::zero(), + ) .is_ok() { return Ok(()); @@ -1396,7 +1415,9 @@ impl RelayPath { // Update clients ahead of scheduling the operational data, if the delays are non-zero. if !self.zero_delay() { - let target_height = od.proofs_height.increment(); + let target_height = NonZeroHeight::new(od.proofs_height.increment()) + .ok_or_else(|| LinkError::Failed("unexpected zero height".to_string()))?; + match od.target { OperationalDataTarget::Source => self.update_client_src(target_height)?, OperationalDataTarget::Destination => self.update_client_dst(target_height)?,