Refactoring and improve update client semantics

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

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<Height> 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<Self> {
+        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<Height> 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;
 

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;

modules/src/mock/context.rs

@@ -421,14 +421,17 @@ impl MockContext {
     }
 
     pub fn consensus_states(&self, client_id: &ClientId) -> Vec<AnyConsensusStateWithHeight> {
-        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(),
+        }
     }
 }
 

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<u64>,
+    #[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<u64>,
+    #[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<Option<IbcEvent>, 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<IbcEvent, Error> = 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(),
         }
     }
 }

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<u64>,
+}
+
+impl UpdateClientCmd {
+    fn execute(&self) -> Result<Option<IbcEvent>, 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),
 }

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(|| {