fix: tmclient update header validation for v0.44.x (#917)

* test for tmclient update with different validator sets

* fix doc lints

* rm incorrect check

* add changelog entry

.changelog/unreleased/bug/911-fix-header-validation-during-tmclient-update.md

@@ -0,0 +1,2 @@
+- Remove an incorrect validation during tendermint client update
+  ([\#911](https://github.com/cosmos/ibc-rs/issues/911))

crates/ibc/src/clients/ics07_tendermint/header.rs

@@ -127,13 +127,6 @@ impl Header {
             });
         }
 
-        if self.trusted_next_validator_set.hash() != self.signed_header.header.next_validators_hash
-        {
-            return Err(Error::MismatchValidatorsHashes {
-                signed_header_validators_hash: self.signed_header.header.next_validators_hash,
-                validators_hash: self.trusted_next_validator_set.hash(),
-            });
-        }
         Ok(())
     }
 }

crates/ibc/src/core/ics02_client/handler/update_client.rs

@@ -162,6 +162,7 @@ mod tests {
     use crate::test_utils::get_dummy_account_id;
     use crate::Height;
     use ibc_proto::ibc::lightclients::tendermint::v1::{ClientState as RawTmClientState, Fraction};
+    use tendermint_testgen::Validator as TestgenValidator;
 
     #[test]
     fn test_update_client_ok() {
@@ -258,6 +259,71 @@ mod tests {
         assert_eq!(client_state.latest_height(), latest_header_height);
     }
 
+    #[test]
+    fn test_update_synthetic_tendermint_client_validator_change_ok() {
+        let client_id = ClientId::new(tm_client_type(), 0).unwrap();
+        let client_height = Height::new(1, 20).unwrap();
+        let update_height = Height::new(1, 21).unwrap();
+        let chain_id_b = ChainId::new("mockgaiaB", 1).unwrap();
+
+        let mut ctx = MockContext::new(
+            ChainId::new("mockgaiaA", 1).unwrap(),
+            HostType::Mock,
+            5,
+            Height::new(1, 1).unwrap(),
+        )
+        .with_client_parametrized_with_chain_id(
+            chain_id_b.clone(),
+            &client_id,
+            client_height,
+            Some(tm_client_type()), // The target host chain (B) is synthetic TM.
+            Some(client_height),
+        );
+
+        let ctx_b = MockContext::new_with_validator_history(
+            chain_id_b,
+            HostType::SyntheticTendermint,
+            &[
+                // Rano: Here I picked the default validator set which is
+                // used at host side client creation.
+                vec![
+                    TestgenValidator::new("1").voting_power(50),
+                    TestgenValidator::new("2").voting_power(50),
+                ],
+                vec![
+                    TestgenValidator::new("1").voting_power(60),
+                    TestgenValidator::new("2").voting_power(40),
+                ],
+            ],
+            update_height,
+        );
+
+        let signer = get_dummy_account_id();
+
+        let mut block = ctx_b.host_block(&update_height).unwrap().clone();
+        block.set_trusted_height(client_height);
+
+        let latest_header_height = block.height();
+        let msg = MsgUpdateClient {
+            client_id,
+            header: block.into(),
+            signer,
+        };
+
+        let res = validate(&ctx, MsgUpdateOrMisbehaviour::UpdateClient(msg.clone()));
+        assert!(res.is_ok());
+
+        let res = execute(&mut ctx, MsgUpdateOrMisbehaviour::UpdateClient(msg.clone()));
+        assert!(res.is_ok(), "result: {res:?}");
+
+        let client_state = ctx.client_state(&msg.client_id).unwrap();
+        assert!(client_state
+            .status(&ctx, &msg.client_id)
+            .unwrap()
+            .is_active());
+        assert_eq!(client_state.latest_height(), latest_header_height);
+    }
+
     #[test]
     fn test_update_synthetic_tendermint_client_non_adjacent_ok() {
         let client_id = ClientId::new(tm_client_type(), 0).unwrap();

crates/ibc/src/core/ics04_channel/acknowledgement.rs

@@ -51,8 +51,7 @@ impl TryFrom<Vec<u8>> for Acknowledgement {
 }
 
 /// Defines a convenience type for IBC applications to construct an
-/// [`Acknowledgement`](super::acknowledgement::Acknowledgement) based on the
-/// success or failure of processing a received packet.
+/// [`Acknowledgement`] based on the success or failure of processing a received packet.
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub enum AcknowledgementStatus {
@@ -67,8 +66,7 @@ pub enum AcknowledgementStatus {
 }
 
 /// A wrapper type that guards variants of
-/// [`AcknowledgementStatus`](crate::core::ics04_channel::acknowledgement::AcknowledgementStatus)
-/// against being constructed with an empty value.
+/// [`AcknowledgementStatus`] against being constructed with an empty value.
 #[cfg_attr(feature = "serde", derive(serde::Serialize, serde::Deserialize))]
 #[derive(Clone, Debug, PartialEq, Eq)]
 pub struct StatusValue(String);

crates/ibc/src/mock/context.rs

@@ -18,6 +18,7 @@ use core::time::Duration;
 use derive_more::{From, TryInto};
 use ibc_proto::protobuf::Protobuf;
 use parking_lot::Mutex;
+use tendermint_testgen::Validator as TestgenValidator;
 
 use ibc_proto::google::protobuf::Any;
 use tracing::debug;
@@ -316,6 +317,78 @@ impl MockContext {
         }
     }
 
+    /// Same as [Self::new] but with custom validator sets for each block.
+    /// Note: the validator history is used accordingly for current validator set and next validator set.
+    /// `validator_history[i]` and `validator_history[i+1]` is i'th block's current and next validator set.
+    /// The number of blocks will be `validator_history.len() - 1` due to the above.
+    pub fn new_with_validator_history(
+        host_id: ChainId,
+        host_type: HostType,
+        validator_history: &[Vec<TestgenValidator>],
+        latest_height: Height,
+    ) -> Self {
+        let max_history_size = validator_history.len() - 1;
+
+        assert_ne!(
+            max_history_size, 0,
+            "The chain must have a non-zero max_history_size"
+        );
+
+        assert_ne!(
+            latest_height.revision_height(),
+            0,
+            "The chain must have a non-zero revision_height"
+        );
+
+        assert!(
+            max_history_size as u64 <= latest_height.revision_height(),
+            "The number of blocks must be greater than the number of validator set histories"
+        );
+
+        assert_eq!(
+            host_id.revision_number(),
+            latest_height.revision_number(),
+            "The version in the chain identifier must match the version in the latest height"
+        );
+
+        let block_time = Duration::from_secs(DEFAULT_BLOCK_TIME_SECS);
+        let next_block_timestamp = Timestamp::now().add(block_time).expect("Never fails");
+
+        let history = (0..max_history_size)
+            .rev()
+            .map(|i| {
+                // generate blocks with timestamps -> N, N - BT, N - 2BT, ...
+                // where N = now(), BT = block_time
+                HostBlock::generate_block_with_validators(
+                    host_id.clone(),
+                    host_type,
+                    latest_height
+                        .sub(i as u64)
+                        .expect("Never fails")
+                        .revision_height(),
+                    next_block_timestamp
+                        .sub(Duration::from_secs(
+                            DEFAULT_BLOCK_TIME_SECS * (i as u64 + 1),
+                        ))
+                        .expect("Never fails"),
+                    &validator_history[i],
+                    &validator_history[i + 1],
+                )
+            })
+            .collect();
+
+        MockContext {
+            host_chain_type: host_type,
+            host_chain_id: host_id.clone(),
+            max_history_size,
+            history,
+            block_time,
+            ibc_store: Arc::new(Mutex::new(MockIbcStore::default())),
+            events: Vec::new(),
+            logs: Vec::new(),
+        }
+    }
+
     /// Associates a client record to this context.
     /// Given a client id and a height, registers a new client in the context and also associates
     /// to this client a mock client state and a mock consensus state for height `height`. The type

crates/ibc/src/mock/host.rs

@@ -8,7 +8,10 @@ use ibc_proto::protobuf::Protobuf as ErasedProtobuf;
 
 use tendermint::block::Header as TmHeader;
 use tendermint_testgen::light_block::TmLightBlock;
-use tendermint_testgen::{Generator, LightBlock as TestgenLightBlock};
+use tendermint_testgen::{
+    Commit as TestgenCommit, Generator, Header as TestgenHeader, LightBlock as TestgenLightBlock,
+    Validator as TestgenValidator,
+};
 
 use crate::clients::ics07_tendermint::consensus_state::ConsensusState as TmConsensusState;
 use crate::clients::ics07_tendermint::header::TENDERMINT_HEADER_TYPE_URL;
@@ -103,6 +106,40 @@ impl HostBlock {
         }
     }
 
+    /// Generates a new block at `height` for the given chain identifier, chain type and validator sets.
+    pub fn generate_block_with_validators(
+        chain_id: ChainId,
+        chain_type: HostType,
+        height: u64,
+        timestamp: Timestamp,
+        validators: &[TestgenValidator],
+        next_validators: &[TestgenValidator],
+    ) -> HostBlock {
+        match chain_type {
+            HostType::Mock => HostBlock::Mock(Box::new(MockHeader {
+                height: Height::new(chain_id.revision_number(), height).expect("Never fails"),
+                timestamp,
+            })),
+            HostType::SyntheticTendermint => {
+                let header = TestgenHeader::new(validators)
+                    .height(height)
+                    .chain_id(&chain_id.to_string())
+                    .next_validators(next_validators)
+                    .time(timestamp.into_tm_time().expect("Never fails"));
+
+                let commit = TestgenCommit::new(header.clone(), 1);
+
+                HostBlock::SyntheticTendermint(Box::new(SyntheticTmBlock {
+                    trusted_height: Height::new(chain_id.revision_number(), 1)
+                        .expect("Never fails"),
+                    light_block: TestgenLightBlock::new(header, commit)
+                        .generate()
+                        .expect("Never fails"),
+                }))
+            }
+        }
+    }
+
     pub fn generate_tm_block(
         chain_id: ChainId,
         height: u64,