Fix client upgrade (informalsystems#2304)

* use appropriate query height in upgrade

* changelog

* use application height in build_update_client

* don't wait for target height

* better error message

* Update relayer/src/foreign_client.rs

Co-authored-by: Adi Seredinschi <adi@informal.systems>

* docstrings and var name change

* use ibc::Height::decrement

* add warning

* query_latest_height call once

Co-authored-by: Anca Zamfir <zamfiranca@gmail.com>
Co-authored-by: Adi Seredinschi <adi@informal.systems>

.changelog/unreleased/bug-fixes/ibc-relayer/2185-client-upgrade.md

@@ -0,0 +1,2 @@
+- Use appropriate height when querying for client upgrade state
+  ([#2185](https://github.com/informalsystems/ibc-rs/issues/2185))

relayer-cli/src/commands/tx/client.rs

@@ -6,13 +6,15 @@ use abscissa_core::{Command, Runnable};
 use ibc::core::ics02_client::client_state::ClientState;
 use ibc::core::ics24_host::identifier::{ChainId, ClientId};
 use ibc::events::IbcEvent;
+use ibc::Height;
 use ibc_relayer::chain::handle::ChainHandle;
 use ibc_relayer::chain::requests::{
     IncludeProof, PageRequest, QueryClientStateRequest, QueryClientStatesRequest,
 };
 use ibc_relayer::config::Config;
 use ibc_relayer::foreign_client::{CreateOptions, ForeignClient};
 use tendermint_light_client_verifier::types::TrustThreshold;
+use tracing::warn;
 
 use crate::application::app_config;
 use crate::cli_utils::{spawn_chain_runtime, spawn_chain_runtime_generic, ChainHandlePair};
@@ -208,7 +210,24 @@ impl Runnable for TxUpgradeClientCmd {
         let client = ForeignClient::find(src_chain, dst_chain, &self.client_id)
             .unwrap_or_else(exit_with_unrecoverable_error);
 
-        let outcome = client.upgrade();
+        // Assumption: this query is run while the chain is halted as a result of an upgrade
+        let src_upgrade_height = {
+            let src_application_height = match client.src_chain().query_latest_height() {
+                Ok(height) => height,
+                Err(e) => Output::error(format!("{}", e)).exit(),
+            };
+
+            // When the chain is halted, the application height reports a height
+            // 1 less than the halted height
+            src_application_height.increment()
+        };
+
+        warn!(
+            "Assuming that chain '{}' is currently halted for upgrade at height {}",
+            client.src_chain().id(),
+            src_upgrade_height
+        );
+        let outcome = client.upgrade(src_upgrade_height);
 
         match outcome {
             Ok(receipt) => Output::success(receipt).exit(),
@@ -234,12 +253,29 @@ impl Runnable for TxUpgradeClientsCmd {
             Err(e) => Output::error(format!("{}", e)).exit(),
         };
 
+        let src_upgrade_height = {
+            let src_application_height = match src_chain.query_latest_height() {
+                Ok(height) => height,
+                Err(e) => Output::error(format!("{}", e)).exit(),
+            };
+
+            // When the chain is halted, the application height reports a height
+            // 1 less than the halted height
+            src_application_height.increment()
+        };
+
         let results = config
             .chains
             .iter()
             .filter_map(|chain| {
-                (self.src_chain_id != chain.id)
-                    .then(|| self.upgrade_clients_for_chain(&config, src_chain.clone(), &chain.id))
+                (self.src_chain_id != chain.id).then(|| {
+                    self.upgrade_clients_for_chain(
+                        &config,
+                        src_chain.clone(),
+                        &chain.id,
+                        &src_upgrade_height,
+                    )
+                })
             })
             .collect();
 
@@ -257,6 +293,7 @@ impl TxUpgradeClientsCmd {
         config: &Config,
         src_chain: Chain,
         dst_chain_id: &ChainId,
+        src_upgrade_height: &Height,
     ) -> UpgradeClientsForChainResult {
         let dst_chain = spawn_chain_runtime_generic::<Chain>(config, dst_chain_id)?;
 
@@ -268,7 +305,14 @@ impl TxUpgradeClientsCmd {
             .map_err(Error::relayer)?
             .into_iter()
             .filter_map(|c| (self.src_chain_id == c.client_state.chain_id()).then(|| c.client_id))
-            .map(|id| TxUpgradeClientsCmd::upgrade_client(id, dst_chain.clone(), src_chain.clone()))
+            .map(|id| {
+                TxUpgradeClientsCmd::upgrade_client(
+                    id,
+                    dst_chain.clone(),
+                    src_chain.clone(),
+                    src_upgrade_height,
+                )
+            })
             .collect();
 
         Ok(outputs)
@@ -278,9 +322,13 @@ impl TxUpgradeClientsCmd {
         client_id: ClientId,
         dst_chain: Chain,
         src_chain: Chain,
+        src_upgrade_height: &Height,
     ) -> Result<Vec<IbcEvent>, Error> {
         let client = ForeignClient::restore(client_id, dst_chain, src_chain);
-        client.upgrade().map_err(Error::foreign_client)
+
+        client
+            .upgrade(*src_upgrade_height)
+            .map_err(Error::foreign_client)
     }
 }
 

relayer/src/chain/cosmos.rs

@@ -321,27 +321,36 @@ impl CosmosSdkChain {
     }
 
     /// Perform an ABCI query against the client upgrade sub-store.
-    /// Fetches both the target data, as well as the proof.
     ///
-    /// The data is returned in its raw format `Vec<u8>`, and is either
-    /// the client state (if the target path is [`UpgradedClientState`]),
-    /// or the client consensus state ([`UpgradedClientConsensusState`]).
+    /// The data is returned in its raw format `Vec<u8>`, and is either the
+    /// client state (if the target path is [`UpgradedClientState`]), or the
+    /// client consensus state ([`UpgradedClientConsensusState`]).
+    ///
+    /// Note: This is a special query in that it will only succeed if the chain
+    /// is halted after reaching the height proposed in a successful governance
+    /// proposal to upgrade the chain. In this scenario, let P be the height at
+    /// which the chain is planned to upgrade. We assume that the chain is
+    /// halted at height P. Tendermint will be at height P (as reported by the
+    /// /status RPC query), but the application will be at height P-1 (as
+    /// reported by the /abci_info RPC query).
+    ///
+    /// Therefore, `query_height` needs to be P-1. However, the path specified
+    /// in `query_data` needs to be constructed with height `P`, as this is how
+    /// the chain will have stored it in its upgrade sub-store.
     fn query_client_upgrade_state(
         &self,
-        data: ClientUpgradePath,
-        height: Height,
+        query_data: ClientUpgradePath,
+        query_height: ibc::Height,
     ) -> Result<(Vec<u8>, MerkleProof), Error> {
-        let prev_height = Height::try_from(height.value() - 1).map_err(Error::invalid_height)?;
-
         // SAFETY: Creating a Path from a constant; this should never fail
         let path = TendermintABCIPath::from_str(SDK_UPGRADE_QUERY_PATH)
             .expect("Turning SDK upgrade query path constant into a Tendermint ABCI path");
         let response: QueryResponse = self.block_on(abci_query(
             &self.rpc_client,
             &self.config.rpc_addr,
             path,
-            Path::Upgrade(data).to_string(),
-            prev_height,
+            Path::Upgrade(query_data).to_string(),
+            Height::try_from(query_height.revision_height).map_err(Error::invalid_height)?,
             true,
         ))?;
 
@@ -787,11 +796,14 @@ impl ChainEndpoint for CosmosSdkChain {
         crate::telemetry!(query, self.id(), "query_upgraded_client_state");
 
         // Query for the value and the proof.
-        let tm_height =
-            Height::try_from(request.height.revision_height).map_err(Error::invalid_height)?;
+        let upgrade_height = request.height;
+        let query_height = upgrade_height
+            .decrement()
+            .map_err(|_| Error::invalid_height_no_source())?;
+
         let (upgraded_client_state_raw, proof) = self.query_client_upgrade_state(
-            ClientUpgradePath::UpgradedClientState(request.height.revision_height),
-            tm_height,
+            ClientUpgradePath::UpgradedClientState(upgrade_height.revision_height),
+            query_height,
         )?;
 
         let client_state = AnyClientState::decode_vec(&upgraded_client_state_raw)
@@ -807,13 +819,15 @@ impl ChainEndpoint for CosmosSdkChain {
         crate::time!("query_upgraded_consensus_state");
         crate::telemetry!(query, self.id(), "query_upgraded_consensus_state");
 
-        let tm_height =
-            Height::try_from(request.height.revision_height).map_err(Error::invalid_height)?;
+        let upgrade_height = request.height;
+        let query_height = upgrade_height
+            .decrement()
+            .map_err(|_| Error::invalid_height_no_source())?;
 
         // Fetch the consensus state and its proof.
         let (upgraded_consensus_state_raw, proof) = self.query_client_upgrade_state(
-            ClientUpgradePath::UpgradedClientConsensusState(request.height.revision_height),
-            tm_height,
+            ClientUpgradePath::UpgradedClientConsensusState(upgrade_height.revision_height),
+            query_height,
         )?;
 
         let consensus_state = AnyConsensusState::decode_vec(&upgraded_consensus_state_raw)

relayer/src/channel.rs

@@ -739,7 +739,7 @@ impl<ChainA: ChainHandle, ChainB: ChainHandle> Channel<ChainA, ChainB> {
             self.src_chain().clone(),
         );
 
-        client.build_update_client(height).map_err(|e| {
+        client.wait_and_build_update_client(height).map_err(|e| {
             ChannelError::client_operation(self.dst_client_id().clone(), self.dst_chain().id(), e)
         })
     }

relayer/src/connection.rs

@@ -834,7 +834,7 @@ impl<ChainA: ChainHandle, ChainB: ChainHandle> Connection<ChainA, ChainB> {
 
     pub fn build_update_client_on_src(&self, height: Height) -> Result<Vec<Any>, ConnectionError> {
         let client = self.restore_src_client();
-        client.build_update_client(height).map_err(|e| {
+        client.wait_and_build_update_client(height).map_err(|e| {
             ConnectionError::client_operation(
                 self.src_client_id().clone(),
                 self.src_chain().id(),
@@ -845,7 +845,7 @@ impl<ChainA: ChainHandle, ChainB: ChainHandle> Connection<ChainA, ChainB> {
 
     pub fn build_update_client_on_dst(&self, height: Height) -> Result<Vec<Any>, ConnectionError> {
         let client = self.restore_dst_client();
-        client.build_update_client(height).map_err(|e| {
+        client.wait_and_build_update_client(height).map_err(|e| {
             ConnectionError::client_operation(
                 self.dst_client_id().clone(),
                 self.dst_chain().id(),

relayer/src/error.rs

@@ -150,6 +150,9 @@ define_error! {
             [ TendermintError ]
             |_| { "invalid height" },
 
+        InvalidHeightNoSource
+            |_| { "invalid height" },
+
         InvalidMetadata
             [ TraceError<InvalidMetadataValue> ]
             |_| { "invalid metadata" },

relayer/src/foreign_client.rs

@@ -179,6 +179,17 @@ define_error! {
                     e.client_id, e.chain_id, e.description, e.source)
             },
 
+        ClientUpgradeNoSource
+        {
+            client_id: ClientId,
+            chain_id: ChainId,
+            description: String,
+        }
+        |e| {
+            format_args!("failed while trying to upgrade client id {0} for chain {1}: {2}",
+                    e.client_id, e.chain_id, e.description)
+        },
+
         ClientEventQuery
             {
                 client_id: ClientId,
@@ -430,25 +441,31 @@ impl<DstChain: ChainHandle, SrcChain: ChainHandle> ForeignClient<DstChain, SrcCh
         }
     }
 
-    pub fn upgrade(&self) -> Result<Vec<IbcEvent>, ForeignClientError> {
-        // Fetch the latest height of the source chain.
-        let src_height = self.src_chain.query_latest_height().map_err(|e| {
-            ForeignClientError::client_upgrade(
-                self.id.clone(),
-                self.src_chain.id(),
-                "failed while querying src chain for latest height".to_string(),
-                e,
-            )
-        })?;
-
-        info!("[{}] upgrade Height: {}", self, src_height);
+    /// Create and send a transaction to perform a client upgrade.
+    /// src_upgrade_height: The height on the source chain at which the chain will halt for the upgrade.
+    pub fn upgrade(&self, src_upgrade_height: Height) -> Result<Vec<IbcEvent>, ForeignClientError> {
+        info!("[{}] upgrade Height: {}", self, src_upgrade_height);
 
-        let mut msgs = self.build_update_client(src_height)?;
+        let mut msgs = self
+            .build_update_client_with_trusted(src_upgrade_height, Height::zero())
+            .map_err(|_| {
+                ForeignClientError::client_upgrade_no_source(
+                    self.id.clone(),
+                    self.src_chain.id(),
+                    format!(
+                        "is chain {} halted at height {}?",
+                        self.src_chain().id(),
+                        src_upgrade_height
+                    ),
+                )
+            })?;
 
         // Query the host chain for the upgraded client state, consensus state & their proofs.
         let (client_state, proof_upgrade_client) = self
             .src_chain
-            .query_upgraded_client_state(QueryUpgradedClientStateRequest { height: src_height })
+            .query_upgraded_client_state(QueryUpgradedClientStateRequest {
+                height: src_upgrade_height,
+            })
             .map_err(|e| {
                 ForeignClientError::client_upgrade(
                     self.id.clone(),
@@ -463,7 +480,7 @@ impl<DstChain: ChainHandle, SrcChain: ChainHandle> ForeignClient<DstChain, SrcCh
         let (consensus_state, proof_upgrade_consensus_state) = self
             .src_chain
             .query_upgraded_consensus_state(QueryUpgradedConsensusStateRequest {
-                height: src_height,
+                height: src_upgrade_height,
             })
             .map_err(|e| {
                 ForeignClientError::client_upgrade(
@@ -785,11 +802,11 @@ impl<DstChain: ChainHandle, SrcChain: ChainHandle> ForeignClient<DstChain, SrcCh
     }
 
     /// Wrapper for build_update_client_with_trusted.
-    pub fn build_update_client(
+    pub fn wait_and_build_update_client(
         &self,
         target_height: Height,
     ) -> Result<Vec<Any>, ForeignClientError> {
-        self.build_update_client_with_trusted(target_height, Height::zero())
+        self.wait_and_build_update_client_with_trusted(target_height, Height::zero())
     }
 
     /// Returns a trusted height that is lower than the target height, so
@@ -940,14 +957,18 @@ impl<DstChain: ChainHandle, SrcChain: ChainHandle> ForeignClient<DstChain, SrcCh
         }
     }
 
-    /// Returns a vector with a message for updating the client to height `target_height`.
-    /// If the client already stores a consensus state for this height, returns an empty vector.
-    pub fn build_update_client_with_trusted(
+    /// Wait for the source chain application to reach height `target_height`
+    /// before building the update client messages.
+    ///
+    /// Returns a vector with a message for updating the client to height
+    /// `target_height`. If the client already stores a consensus state for this
+    /// height, returns an empty vector.
+    pub fn wait_and_build_update_client_with_trusted(
         &self,
         target_height: Height,
         trusted_height: Height,
     ) -> Result<Vec<Any>, ForeignClientError> {
-        let src_network_latest_height = || {
+        let src_application_latest_height = || {
             self.src_chain().query_latest_height().map_err(|e| {
                 ForeignClientError::client_create(
                     self.src_chain.id(),
@@ -958,10 +979,18 @@ impl<DstChain: ChainHandle, SrcChain: ChainHandle> ForeignClient<DstChain, SrcCh
         };
 
         // Wait for the source network to produce block(s) & reach `target_height`.
-        while src_network_latest_height()? < target_height {
+        while src_application_latest_height()? < target_height {
             thread::sleep(Duration::from_millis(100))
         }
 
+        self.build_update_client_with_trusted(target_height, trusted_height)
+    }
+
+    pub fn build_update_client_with_trusted(
+        &self,
+        target_height: Height,
+        trusted_height: Height,
+    ) -> Result<Vec<Any>, ForeignClientError> {
         // Get the latest client state on destination.
         let (client_state, _) = self.validated_client_state()?;
 
@@ -1084,7 +1113,7 @@ impl<DstChain: ChainHandle, SrcChain: ChainHandle> ForeignClient<DstChain, SrcCh
             height
         };
 
-        let new_msgs = self.build_update_client_with_trusted(h, trusted_height)?;
+        let new_msgs = self.wait_and_build_update_client_with_trusted(h, trusted_height)?;
         if new_msgs.is_empty() {
             return Err(ForeignClientError::client_already_up_to_date(
                 self.id.clone(),

relayer/src/link/relay_path.rs

@@ -317,14 +317,14 @@ impl<ChainA: ChainHandle, ChainB: ChainHandle> RelayPath<ChainA, ChainB> {
     pub fn build_update_client_on_dst(&self, height: Height) -> Result<Vec<Any>, LinkError> {
         let client = self.restore_dst_client();
         client
-            .build_update_client(height)
+            .wait_and_build_update_client(height)
             .map_err(LinkError::client)
     }
 
     pub fn build_update_client_on_src(&self, height: Height) -> Result<Vec<Any>, LinkError> {
         let client = self.restore_src_client();
         client
-            .build_update_client(height)
+            .wait_and_build_update_client(height)
             .map_err(LinkError::client)
     }