Addressed review comments

modules/src/ics07_tendermint/header.rs

@@ -41,15 +41,15 @@ impl Header {
 
         match self_header_height.cmp(&&ibc_client_height) {
             Ordering::Equal => {
-                // fork
+                // 1 - fork
                 self.signed_header.commit.block_id == other_header.signed_header.commit.block_id
             }
             Ordering::Greater => {
-                // BFT time violation
+                // 2 - BFT time violation
                 self.signed_header.header.time > other_header.signed_header.header.time
             }
             Ordering::Less => {
-                // BFT time violation
+                // 3 - BFT time violation
                 self.signed_header.header.time < other_header.signed_header.header.time
             }
         }

relayer-cli/src/commands/misbehaviour.rs

@@ -118,10 +118,10 @@ fn misbehaviour_handling(
             )
         })?;
 
-    if let Some(evidence_submission_result) = misbehaviour_detection_result {
+    if !misbehaviour_detection_result.is_empty() {
         info!(
-            "\nEvidence submission result {:?}",
-            evidence_submission_result
+            "evidence submission result {:?}",
+            misbehaviour_detection_result
         );
     }
 

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

@@ -134,10 +134,7 @@ impl Runnable for QueryClientConsensusCmd {
                 match res {
                     Ok(states) => {
                         if self.heights_only {
-                            let heights: Vec<Height> = states
-                                .iter()
-                                .filter_map(|cs| Option::from(cs.height))
-                                .collect();
+                            let heights: Vec<Height> = states.iter().map(|cs| cs.height).collect();
                             Output::success(heights).exit()
                         } else {
                             Output::success(states).exit()

relayer/src/chain/cosmos.rs

@@ -999,10 +999,7 @@ impl Chain for CosmosSdkChain {
                     return Ok(vec![]);
                 }
 
-                assert!(response.txs.len() == 1);
-
-                let events = update_client_from_tx_search_response(self.id(), &request, response)
-                    .map_or(vec![], |v| vec![v]);
+                let events = update_client_from_tx_search_response(self.id(), &request, response);
 
                 Ok(events)
             }
@@ -1310,26 +1307,22 @@ fn packet_from_tx_search_response(
     }
 }
 
-// Extract the update client event from the query_txs RPC response.
+// Extract all update client events for the requested client and height from the query_txs RPC response.
 fn update_client_from_tx_search_response(
     chain_id: &ChainId,
     request: &QueryClientEventRequest,
-    mut response: tendermint_rpc::endpoint::tx_search::Response,
-) -> Option<IbcEvent> {
+    response: tendermint_rpc::endpoint::tx_search::Response,
+) -> Vec<IbcEvent> {
     crate::time!("update_client_from_tx_search_response");
 
-    assert!(
-        response.txs.len() <= 1,
-        "update_client_from_tx_search_response: unexpected number of txs"
-    );
-    if let Some(r) = response.txs.pop() {
+    let mut matching = Vec::new();
+
+    for r in response.txs {
         let height = ICSHeight::new(chain_id.version(), u64::from(r.height));
         if request.height != ICSHeight::zero() && height > request.height {
-            return None;
+            return vec![];
         }
 
-        let mut matching = Vec::new();
-
         for e in r.tx_result.events {
             if e.type_str != request.event_id.as_str() {
                 continue;
@@ -1358,16 +1351,8 @@ fn update_client_from_tx_search_response(
 
             matching.push(event);
         }
-
-        assert_eq!(
-            matching.len(),
-            1,
-            "update_client_from_tx_search_response: unexpected number of matching packets"
-        );
-        matching.pop()
-    } else {
-        None
     }
+    matching
 }
 
 /// Perform a generic `abci_query`, and return the corresponding deserialized response data.

relayer/src/chain/handle.rs

@@ -50,6 +50,7 @@ pub fn reply_channel<T>() -> (ReplyTo<T>, Reply<T>) {
 
 /// Requests that a `ChainHandle` may send to a `ChainRuntime`.
 #[derive(Clone, Debug)]
+#[allow(clippy::large_enum_variant)]
 pub enum ChainRequest {
     Terminate {
         reply_to: ReplyTo<()>,
@@ -328,10 +329,10 @@ pub trait ChainHandle: DynClone + Send + Sync + Debug {
         client_state: AnyClientState,
     ) -> Result<AnyConsensusState, Error>;
 
-    fn build_misbehaviour(
+    fn check_misbehaviour(
         &self,
-        client_state: AnyClientState,
         update: UpdateClient,
+        client_state: AnyClientState,
     ) -> Result<Option<AnyMisbehaviour>, Error>;
 
     fn build_connection_proofs_and_client_state(

relayer/src/chain/handle/prod.rs

@@ -253,10 +253,10 @@ impl ChainHandle for ProdChainHandle {
         })
     }
 
-    fn build_misbehaviour(
+    fn check_misbehaviour(
         &self,
-        client_state: AnyClientState,
         update_event: UpdateClient,
+        client_state: AnyClientState,
     ) -> Result<Option<AnyMisbehaviour>, Error> {
         self.send(|reply_to| ChainRequest::BuildMisbehaviour {
             client_state,

relayer/src/chain/runtime.rs

@@ -199,7 +199,7 @@ impl<C: Chain + Send + 'static> ChainRuntime<C> {
                         }
 
                        Ok(ChainRequest::BuildMisbehaviour { client_state, update_event, reply_to }) => {
-                            self.build_misbehaviour(client_state, update_event, reply_to)?
+                            self.check_misbehaviour(update_event, client_state, reply_to)?
                         }
 
                         Ok(ChainRequest::BuildConnectionProofsAndClientState { message_type, connection_id, client_id, height, reply_to }) => {
@@ -439,15 +439,15 @@ impl<C: Chain + Send + 'static> ChainRuntime<C> {
     }
 
     /// Constructs AnyMisbehaviour for the update event
-    fn build_misbehaviour(
+    fn check_misbehaviour(
         &mut self,
-        client_state: AnyClientState,
         update_event: UpdateClient,
+        client_state: AnyClientState,
         reply_to: ReplyTo<Option<AnyMisbehaviour>>,
     ) -> Result<(), Error> {
         let misbehaviour = self
             .light_client
-            .build_misbehaviour(&client_state, update_event)?;
+            .check_misbehaviour(update_event, &client_state)?;
 
         reply_to
             .send(Ok(misbehaviour))

relayer/src/foreign_client.rs

@@ -24,6 +24,7 @@ use ibc::Height;
 use ibc_proto::ibc::core::client::v1::QueryConsensusStatesRequest;
 
 use crate::chain::handle::ChainHandle;
+use crate::relay::MAX_ITER;
 
 #[derive(Debug, Error)]
 pub enum ForeignClientError {
@@ -477,6 +478,10 @@ impl ForeignClient {
         Ok(())
     }
 
+    /// Retrieves the client update event that was emitted when a consensus state at the
+    /// specified height was created on chain.
+    /// It is possible that the event cannot be retrieved if the information is not yet available
+    /// on the full node. To handle this the query is retried a few times.
     pub fn update_client_event(
         &self,
         consensus_height: Height,
@@ -488,24 +493,38 @@ impl ForeignClient {
             consensus_height,
         };
 
-        // Tx query fails if we don't wait a bit here (block is not ready/ indexed?)
-        thread::sleep(Duration::from_millis(100));
-        let res = self
-            .dst_chain
-            .query_txs(QueryTxRequest::Client(request))
-            .map_err(|e| {
-                ForeignClientError::Misbehaviour(format!(
-                    "failed to query Tx-es for update client event {}",
-                    e
-                ))
-            })?;
+        let mut events = vec![];
+        for i in 0..MAX_ITER {
+            thread::sleep(Duration::from_millis(100));
+            let result = self
+                .dst_chain
+                .query_txs(QueryTxRequest::Client(request.clone()))
+                .map_err(|e| {
+                    ForeignClientError::Misbehaviour(format!(
+                        "failed to query Tx-es for update client event {}",
+                        e
+                    ))
+                });
+            match result {
+                Err(e) => {
+                    error!("query_tx with error {}, retry {}/{}", e, i + 1, MAX_ITER);
+                    continue;
+                }
+                Ok(result) => {
+                    events = result;
+                }
+            }
+        }
 
-        if res.is_empty() {
+        if events.is_empty() {
             return Ok(None);
-        };
+        }
 
-        assert_eq!(res.len(), 1);
-        let event = res[0].clone();
+        // It is possible in theory that `query_txs` returns multiple client update events for the
+        // same consensus height. This could happen when multiple client updates with same header
+        // were submitted to chain. However this is not what it's observed during testing.
+        // Regardless, just take the event from the first update.
+        let event = events[0].clone();
         let update = downcast!(event.clone() => IbcEvent::UpdateClient).ok_or_else(|| {
             ForeignClientError::Misbehaviour(format!(
                 "query Tx-es returned unexpected event {}",
@@ -515,7 +534,7 @@ impl ForeignClient {
         Ok(Some(update))
     }
 
-    /// Retrieves all consensus states for this client and sorts them in reverse height
+    /// Retrieves all consensus states for this client and sorts them in descending height
     /// order. If consensus states are not pruned on chain, then last consensus state is the one
     /// installed by the `CreateClient` operation.
     fn consensus_states(&self) -> Result<Vec<AnyConsensusStateWithHeight>, ForeignClientError> {
@@ -532,24 +551,23 @@ impl ForeignClient {
                     format!("{}", e),
                 )
             })?;
-        consensus_states.sort_by(|a, b| a.height.cmp(&b.height));
-        consensus_states.reverse();
+        consensus_states.sort_by_key(|a| std::cmp::Reverse(a.height));
         Ok(consensus_states)
     }
 
-    /// Retrieves all consensus heights for this client sorted in reverse
+    /// Retrieves all consensus heights for this client sorted in descending
     /// order.
     fn consensus_state_heights(&self) -> Result<Vec<Height>, ForeignClientError> {
         let consensus_state_heights: Vec<Height> = self
             .consensus_states()?
             .iter()
-            .filter_map(|cs| Option::from(cs.height))
+            .map(|cs| cs.height)
             .collect();
 
         Ok(consensus_state_heights)
     }
 
-    /// Check for misbehaviour and submit evidence.
+    /// Checks for misbehaviour and submits evidence.
     /// 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
     /// until it gets to the first misbehaviour.
@@ -592,15 +610,15 @@ impl ForeignClient {
         // Get the latest client state on destination.
         let client_state = self
             .dst_chain()
-            .query_client_state(&self.id, Height::default())
+            .query_client_state(&self.id, Height::zero())
             .map_err(|e| {
                 ForeignClientError::Misbehaviour(format!(
                     "failed querying client state on dst chain {} with error: {}",
                     self.id, e
                 ))
             })?;
 
-        // Get the list of consensus state heights in reverse order.
+        // Get the list of consensus state heights in descending order.
         // Note: If chain does not prune consensus states then the last consensus state is
         // the one installed by the `CreateClient` which does not include a header.
         // For chains that do support pruning, it is possible that the last consensus state
@@ -644,16 +662,17 @@ impl ForeignClient {
 
             // Check for misbehaviour according to the specific source chain type.
             // In case of Tendermint client, this will also check the BFT time violation if
-            // a header for the event height is not available.
+            // a header for the event height cannot be retrieved from the witness.
             let misbehavior = self
                 .src_chain
-                .build_misbehaviour(client_state.clone(), update_event.clone())
+                .check_misbehaviour(update_event, client_state.clone())
                 .map_err(|e| {
                     ForeignClientError::Misbehaviour(format!("failed to build misbehaviour {}", e))
                 })?;
 
             if misbehavior.is_some() {
-                // TODO - add updateClient messages if supporting headers are missing on-chain
+                // TODO - add updateClient messages if light blocks are returned from
+                //  `src_chain.check_misbehaviour` call above i.e. supporting headers are required
                 return Ok(misbehavior);
             }
 
@@ -667,15 +686,15 @@ impl ForeignClient {
     pub fn detect_misbehaviour_and_send_evidence(
         &self,
         update: Option<UpdateClient>,
-    ) -> Result<Option<IbcEvent>, ForeignClientError> {
+    ) -> Result<Vec<IbcEvent>, ForeignClientError> {
         match self.handle_misbehaviour(update)? {
-            None => Ok(None),
+            None => Ok(vec![]),
             Some(misbehaviour) => {
-                error!("MISBEHAVIOUR detected {}, sending evidence", misbehaviour);
+                error!("MISBEHAVIOUR DETECTED {}, sending evidence", misbehaviour);
 
                 let signer = self.dst_chain().get_signer().map_err(|e| {
                     ForeignClientError::Misbehaviour(format!(
-                        "failed getting signer for dst chain ({}) with error: {}",
+                        "failed getting signer for destination chain ({}), error: {}",
                         self.dst_chain.id(),
                         e
                     ))
@@ -692,13 +711,13 @@ impl ForeignClient {
                     .send_msgs(vec![msg.to_any()])
                     .map_err(|e| {
                         ForeignClientError::Misbehaviour(format!(
-                            "failed sending evidence to dst chain ({}) with err: {}",
+                            "failed sending evidence to destination chain ({}), error: {}",
                             self.dst_chain.id(),
                             e
                         ))
                     })?;
 
-                Ok(Some(events[0].clone()))
+                Ok(events)
             }
         }
     }

relayer/src/lib.rs

@@ -8,7 +8,6 @@
     unused_qualifications,
     rust_2018_idioms
 )]
-#![allow(clippy::large_enum_variant)]
 
 //! IBC Relayer implementation
 

relayer/src/light_client.rs

@@ -25,10 +25,10 @@ pub trait LightClient<C: Chain>: Send + Sync {
         client_state: &AnyClientState,
     ) -> Result<C::LightBlock, error::Error>;
 
-    fn build_misbehaviour(
+    fn check_misbehaviour(
         &mut self,
-        client_state: &AnyClientState,
         update: UpdateClient,
+        client_state: &AnyClientState,
     ) -> Result<Option<AnyMisbehaviour>, error::Error>;
     /// Fetch a header from the chain at the given height, without verifying it
     fn fetch(&mut self, height: ibc::Height) -> Result<C::LightBlock, error::Error>;

relayer/src/light_client/mock.rs

@@ -43,10 +43,10 @@ impl super::LightClient<MockChain> for LightClient {
         Ok(self.light_block(height))
     }
 
-    fn build_misbehaviour(
+    fn check_misbehaviour(
         &mut self,
-        _client_state: &AnyClientState,
         _update: UpdateClient,
+        _client_state: &AnyClientState,
     ) -> Result<Option<AnyMisbehaviour>, Error> {
         unimplemented!()
     }