Fix client upgrade

.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

@@ -208,7 +208,18 @@ 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()
+        };
+        let outcome = client.upgrade(src_upgrade_height);
 
         match outcome {
             Ok(receipt) => Output::success(receipt).exit(),
@@ -280,7 +291,20 @@ impl TxUpgradeClientsCmd {
         src_chain: Chain,
     ) -> Result<Vec<IbcEvent>, Error> {
         let client = ForeignClient::restore(client_id, dst_chain, src_chain);
-        client.upgrade().map_err(Error::foreign_client)
+
+        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()
+        };
+        client
+            .upgrade(src_upgrade_height)
+            .map_err(Error::foreign_client)
     }
 }
 

relayer/src/chain/cosmos.rs

@@ -319,27 +319,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: 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(),
+            query_height,
             true,
         ))?;
 
@@ -779,11 +788,19 @@ impl ChainEndpoint for CosmosSdkChain {
         crate::telemetry!(query, self.id(), "query_upgraded_client_state");
 
         // Query for the value and the proof.
-        let tm_height =
+        let upgrade_height =
             Height::try_from(request.height.revision_height).map_err(Error::invalid_height)?;
+        let query_height = Height::try_from(
+            upgrade_height
+                .value()
+                .checked_sub(1)
+                .expect("height overflow"),
+        )
+        .map_err(Error::invalid_height)?;
+
         let (upgraded_client_state_raw, proof) = self.query_client_upgrade_state(
-            ClientUpgradePath::UpgradedClientState(request.height.revision_height),
-            tm_height,
+            ClientUpgradePath::UpgradedClientState(upgrade_height.value()),
+            query_height,
         )?;
 
         let client_state = AnyClientState::decode_vec(&upgraded_client_state_raw)
@@ -799,13 +816,20 @@ impl ChainEndpoint for CosmosSdkChain {
         crate::time!("query_upgraded_consensus_state");
         crate::telemetry!(query, self.id(), "query_upgraded_consensus_state");
 
-        let tm_height =
+        let upgrade_height =
             Height::try_from(request.height.revision_height).map_err(Error::invalid_height)?;
+        let query_height = Height::try_from(
+            upgrade_height
+                .value()
+                .checked_sub(1)
+                .expect("height overflow"),
+        )
+        .map_err(Error::invalid_height)?;
 
         // 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.value()),
+            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/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 chain 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
@@ -942,7 +959,7 @@ 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(
+    pub fn wait_and_build_update_client_with_trusted(
         &self,
         target_height: Height,
         trusted_height: Height,
@@ -962,6 +979,14 @@ impl<DstChain: ChainHandle, SrcChain: ChainHandle> ForeignClient<DstChain, SrcCh
             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 +1109,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)
     }