diff --git a/.changelog/unreleased/breaking-changes/973-update-meta.md b/.changelog/unreleased/breaking-changes/973-update-meta.md new file mode 100644 index 000000000..2aabe1231 --- /dev/null +++ b/.changelog/unreleased/breaking-changes/973-update-meta.md @@ -0,0 +1,6 @@ +- Merge client update time and height modification method pairs into + one, that is replace + a) client_update_{time,height} by update_meta, + b) store_update_{time,height} by store_update_meta and + c) delete_update_{time,height} by delete_update_meta. + ([\#972](https://github.com/cosmos/ibc-rs/pull/972)) diff --git a/docs/architecture/adr-005-handlers-redesign.md b/docs/architecture/adr-005-handlers-redesign.md index 6d4be433c..a4c25890a 100644 --- a/docs/architecture/adr-005-handlers-redesign.md +++ b/docs/architecture/adr-005-handlers-redesign.md @@ -40,14 +40,14 @@ function updateClient( clientState.VerifyClientMessage(clientMessage) // Validation END - + // Execution START foundMisbehaviour := clientState.CheckForMisbehaviour(clientMessage) if foundMisbehaviour { clientState.UpdateStateOnMisbehaviour(clientMessage) } - else { - clientState.UpdateState(clientMessage) + else { + clientState.UpdateState(clientMessage) } // Execution END } @@ -99,13 +99,13 @@ pub trait Host { /// An error type that can represent all host errors. type Error; - /// The Host's key-value store that must provide access to IBC paths. + /// The Host's key-value store that must provide access to IBC paths. type KvStore: IbcStore; /// An event logging facility. type EventLogger: EventLogger>; - /// Methods to access the store (ro & rw). + /// Methods to access the store (ro & rw). fn store(&self) -> &Self::KvStore; fn store_mut(&mut self) -> &mut Self::KvStore; @@ -373,11 +373,9 @@ trait ValidationContext { /// A hashing function for packet commitments fn hash(&self, value: Vec) -> Vec; - /// Returns the time when the client state for the given [`ClientId`] was updated with a header for the given [`Height`] - fn client_update_time(&self, client_id: &ClientId, height: Height) -> Result; - - /// Returns the height when the client state for the given [`ClientId`] was updated with a header for the given [`Height`] - fn client_update_height(&self, client_id: &ClientId, height: Height) -> Result; + /// Returns the time and height when the client state for the + /// given [`ClientId`] was updated with a header for the given [`Height`] + fn update_meta(&self, client_id: &ClientId, height: Height) -> Result<(Timestamp, Height), Error>; /// Returns a counter on the number of channel ids that have been created thus far. /// The value of this counter should increase only via method @@ -427,22 +425,14 @@ trait ExecutionContext { fn increase_client_counter(&mut self); /// Called upon successful client update. - /// Implementations are expected to use this to record the specified time as the time at which - /// this update (or header) was processed. - fn store_update_time( - &mut self, - client_id: ClientId, - height: Height, - timestamp: Timestamp, - ) -> Result<(), Error>; - - /// Called upon successful client update. - /// Implementations are expected to use this to record the specified height as the height at - /// at which this update (or header) was processed. - fn store_update_height( + /// + /// Implementations are expected to use this to record the specified time + /// and height as the time at which this update (or header) was processed. + fn store_update_meta( &mut self, client_id: ClientId, height: Height, + host_timestamp: Timestamp, host_height: Height, ) -> Result<(), Error>; @@ -550,16 +540,11 @@ pub trait Host { /// Methods currently in `ClientKeeper` fn increase_client_counter(&mut self); - fn store_update_time( - &mut self, - client_id: ClientId, - height: Height, - timestamp: Timestamp, - ) -> Result<(), Error>; - fn store_update_height( + fn store_update_meta( &mut self, client_id: ClientId, height: Height, + host_timestamp: Timestamp, host_height: Height, ) -> Result<(), Error>; @@ -574,8 +559,7 @@ pub trait Host { /// Methods currently in `ChannelReader` fn connection_channels(&self, cid: &ConnectionId) -> Result, Error>; fn hash(&self, value: Vec) -> Vec; - fn client_update_time(&self, client_id: &ClientId, height: Height) -> Result; - fn client_update_height(&self, client_id: &ClientId, height: Height) -> Result; + fn update_meta(&self, client_id: &ClientId, height: Height) -> Result<(Timestamp, Height), Error>; fn channel_counter(&self) -> Result; fn max_expected_time_per_block(&self) -> Duration; fn block_delay(&self, delay_period_time: Duration) -> u64; @@ -626,7 +610,7 @@ pub trait StoreSerde { /// Serialize to canonical binary representation fn serialize(self) -> Vec; - /// Deserialize from bytes + /// Deserialize from bytes fn deserialize(value: &[u8]) -> Self; } diff --git a/ibc-clients/ics07-tendermint/src/client_state.rs b/ibc-clients/ics07-tendermint/src/client_state.rs index f4e4390ba..1bffd8f99 100644 --- a/ibc-clients/ics07-tendermint/src/client_state.rs +++ b/ibc-clients/ics07-tendermint/src/client_state.rs @@ -336,8 +336,7 @@ where ), tm_consensus_state.into(), )?; - ctx.store_update_time(client_id.clone(), self.latest_height(), host_timestamp)?; - ctx.store_update_height(client_id.clone(), self.latest_height(), host_height)?; + ctx.store_update_meta(client_id, self.latest_height(), host_timestamp, host_height)?; Ok(()) } @@ -387,8 +386,7 @@ where ClientStatePath::new(client_id), ClientState::from(new_client_state).into(), )?; - ctx.store_update_time(client_id.clone(), header_height, host_timestamp)?; - ctx.store_update_height(client_id.clone(), header_height, host_height)?; + ctx.store_update_meta(client_id, header_height, host_timestamp, host_height)?; } Ok(vec![header_height]) @@ -483,8 +481,7 @@ where ), TmConsensusState::from(new_consensus_state).into(), )?; - ctx.store_update_time(client_id.clone(), latest_height, host_timestamp)?; - ctx.store_update_height(client_id.clone(), latest_height, host_height)?; + ctx.store_update_meta(client_id, latest_height, host_timestamp, host_height)?; Ok(latest_height) } diff --git a/ibc-clients/ics07-tendermint/src/client_state/update_client.rs b/ibc-clients/ics07-tendermint/src/client_state/update_client.rs index 89a88d520..599fc1ca5 100644 --- a/ibc-clients/ics07-tendermint/src/client_state/update_client.rs +++ b/ibc-clients/ics07-tendermint/src/client_state/update_client.rs @@ -218,13 +218,9 @@ impl ClientState { if tm_consensus_state_expiry > host_timestamp { break; - } else { - let client_id = client_id.clone(); - - ctx.delete_consensus_state(client_consensus_state_path)?; - ctx.delete_update_time(client_id.clone(), height)?; - ctx.delete_update_height(client_id, height)?; } + ctx.delete_consensus_state(client_consensus_state_path)?; + ctx.delete_update_meta(client_id, height)?; } Ok(()) diff --git a/ibc-clients/ics07-tendermint/types/src/error.rs b/ibc-clients/ics07-tendermint/types/src/error.rs index a9fac983e..556d5e6fe 100644 --- a/ibc-clients/ics07-tendermint/types/src/error.rs +++ b/ibc-clients/ics07-tendermint/types/src/error.rs @@ -74,10 +74,8 @@ pub enum Error { NotEnoughTrustedValsSigned { reason: VotingPowerTally }, /// verification failed: `{detail}` VerificationError { detail: LightClientErrorDetail }, - /// Processed time for the client `{client_id}` at height `{height}` not found - ProcessedTimeNotFound { client_id: ClientId, height: Height }, - /// Processed height for the client `{client_id}` at height `{height}` not found - ProcessedHeightNotFound { client_id: ClientId, height: Height }, + /// Processed time or height for the client `{client_id}` at height `{height}` not found + UpdateMetaDataNotFound { client_id: ClientId, height: Height }, /// The given hash of the validators does not matches the given hash in the signed header. Expected: `{signed_header_validators_hash}`, got: `{validators_hash}` MismatchValidatorsHashes { validators_hash: Hash, diff --git a/ibc-core/ics02-client/context/src/context.rs b/ibc-core/ics02-client/context/src/context.rs index 368e2c446..b181c388b 100644 --- a/ibc-core/ics02-client/context/src/context.rs +++ b/ibc-core/ics02-client/context/src/context.rs @@ -12,19 +12,13 @@ use super::consensus_state::ConsensusState; /// [crate::client_state::ClientStateValidation] must /// inherit from this trait. pub trait ClientValidationContext { - /// Returns the time when the client state for the given [`ClientId`] was updated with a header for the given [`Height`] - fn client_update_time( + /// Returns the time and height when the client state for the given + /// [`ClientId`] was updated with a header for the given [`Height`] + fn update_meta( &self, client_id: &ClientId, - height: &Height, - ) -> Result; - - /// Returns the height when the client state for the given [`ClientId`] was updated with a header for the given [`Height`] - fn client_update_height( - &self, - client_id: &ClientId, - height: &Height, - ) -> Result; + height: Height, + ) -> Result<(Timestamp, Height), ContextError>; } /// Defines the methods that all client `ExecutionContext`s (precisely the @@ -61,40 +55,27 @@ pub trait ClientExecutionContext: Sized { ) -> Result<(), ContextError>; /// Called upon successful client update. - /// Implementations are expected to use this to record the specified time as the time at which - /// this update (or header) was processed. - fn store_update_time( + /// + /// Implementations are expected to use this to record the specified time + /// and height as the time at which this update (or header) was processed. + fn store_update_meta( &mut self, - client_id: ClientId, + client_id: &ClientId, height: Height, host_timestamp: Timestamp, - ) -> Result<(), ContextError>; - - /// Called upon successful client update. - /// Implementations are expected to use this to record the specified height as the height at - /// at which this update (or header) was processed. - fn store_update_height( - &mut self, - client_id: ClientId, - height: Height, host_height: Height, ) -> Result<(), ContextError>; - /// Delete the update time associated with the client at the specified height. This update - /// time should be associated with a consensus state through the specified height. + /// Delete the update time and height associated with the client at the + /// specified height. + /// + /// This update time should be associated with a consensus state through the + /// specified height. /// /// Note that this timestamp is determined by the host. - fn delete_update_time( - &mut self, - client_id: ClientId, - height: Height, - ) -> Result<(), ContextError>; - - /// Delete the update height associated with the client at the specified height. This update - /// time should be associated with a consensus state through the specified height. - fn delete_update_height( + fn delete_update_meta( &mut self, - client_id: ClientId, + client_id: &ClientId, height: Height, ) -> Result<(), ContextError>; } diff --git a/ibc-core/ics02-client/types/src/error.rs b/ibc-core/ics02-client/types/src/error.rs index fbfcebcb1..985599dcc 100644 --- a/ibc-core/ics02-client/types/src/error.rs +++ b/ibc-core/ics02-client/types/src/error.rs @@ -26,10 +26,8 @@ pub enum ClientError { ClientStateAlreadyExists { client_id: ClientId }, /// consensus state not found at: `{client_id}` at height `{height}` ConsensusStateNotFound { client_id: ClientId, height: Height }, - /// Processed time for the client `{client_id}` at height `{height}` not found - ProcessedTimeNotFound { client_id: ClientId, height: Height }, - /// Processed height for the client `{client_id}` at height `{height}` not found - ProcessedHeightNotFound { client_id: ClientId, height: Height }, + /// Processed time or height for the client `{client_id}` at height `{height}` not found + UpdateMetaDataNotFound { client_id: ClientId, height: Height }, /// header verification failed with reason: `{reason}` HeaderVerificationFailure { reason: String }, /// failed to build trust threshold from fraction: `{numerator}`/`{denominator}` diff --git a/ibc-core/ics03-connection/src/delay.rs b/ibc-core/ics03-connection/src/delay.rs index d573a6eac..04fcc550f 100644 --- a/ibc-core/ics03-connection/src/delay.rs +++ b/ibc-core/ics03-connection/src/delay.rs @@ -19,19 +19,16 @@ where // Fetch the latest time and height that the counterparty client was updated on the host chain. let client_id = connection_end.client_id(); - let last_client_update_time = ctx + let last_client_update = ctx .get_client_validation_context() - .client_update_time(client_id, &packet_proof_height)?; - let last_client_update_height = ctx - .get_client_validation_context() - .client_update_height(client_id, &packet_proof_height)?; + .update_meta(client_id, packet_proof_height)?; // Fetch the connection delay time and height periods. let conn_delay_time_period = connection_end.delay_period(); let conn_delay_height_period = ctx.block_delay(&conn_delay_time_period); // Verify that the current host chain time is later than the last client update time - let earliest_valid_time = (last_client_update_time + conn_delay_time_period) + let earliest_valid_time = (last_client_update.0 + conn_delay_time_period) .map_err(ConnectionError::TimestampOverflow)?; if current_host_time < earliest_valid_time { return Err(ContextError::ConnectionError( @@ -43,7 +40,7 @@ where } // Verify that the current host chain height is later than the last client update height - let earliest_valid_height = last_client_update_height.add(conn_delay_height_period); + let earliest_valid_height = last_client_update.1.add(conn_delay_height_period); if current_host_height < earliest_valid_height { return Err(ContextError::ConnectionError( ConnectionError::NotEnoughBlocksElapsed { diff --git a/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs b/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs index 34a387a34..1d47277b6 100644 --- a/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs +++ b/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs @@ -350,8 +350,12 @@ where new_consensus_state.into(), )?; ctx.store_client_state(ClientStatePath::new(client_id), new_client_state.into())?; - ctx.store_update_time(client_id.clone(), header_height, ctx.host_timestamp()?)?; - ctx.store_update_height(client_id.clone(), header_height, ctx.host_height()?)?; + ctx.store_update_meta( + client_id, + header_height, + ctx.host_timestamp()?, + ctx.host_height()?, + )?; Ok(vec![header_height]) } @@ -395,8 +399,7 @@ where let host_timestamp = ctx.host_timestamp()?; let host_height = ctx.host_height()?; - ctx.store_update_time(client_id.clone(), latest_height, host_timestamp)?; - ctx.store_update_height(client_id.clone(), latest_height, host_height)?; + ctx.store_update_meta(client_id, latest_height, host_timestamp, host_height)?; Ok(latest_height) } diff --git a/ibc-testkit/src/testapp/ibc/core/client_ctx.rs b/ibc-testkit/src/testapp/ibc/core/client_ctx.rs index 667aabdf8..5280270f9 100644 --- a/ibc-testkit/src/testapp/ibc/core/client_ctx.rs +++ b/ibc-testkit/src/testapp/ibc/core/client_ctx.rs @@ -156,42 +156,23 @@ impl TmValidationContext for MockContext { } impl ClientValidationContext for MockContext { - fn client_update_time( + fn update_meta( &self, client_id: &ClientId, - height: &Height, - ) -> Result { - match self - .ibc_store - .lock() - .client_processed_times - .get(&(client_id.clone(), *height)) - { - Some(time) => Ok(*time), - None => Err(ClientError::ProcessedTimeNotFound { - client_id: client_id.clone(), - height: *height, - })?, - } - } - - fn client_update_height( - &self, - client_id: &ClientId, - height: &Height, - ) -> Result { - match self - .ibc_store - .lock() - .client_processed_heights - .get(&(client_id.clone(), *height)) - { - Some(height) => Ok(*height), - None => Err(ClientError::ProcessedHeightNotFound { - client_id: client_id.clone(), - height: *height, - })?, - } + height: Height, + ) -> Result<(Timestamp, Height), ContextError> { + let key = (client_id.clone(), height); + (|| { + let ibc_store = self.ibc_store.lock(); + let time = ibc_store.client_processed_times.get(&key)?; + let height = ibc_store.client_processed_heights.get(&key)?; + Some((*time, *height)) + })() + .ok_or(ClientError::UpdateMetaDataNotFound { + client_id: key.0, + height, + }) + .map_err(ContextError::from) } } @@ -273,61 +254,32 @@ impl ClientExecutionContext for MockContext { Ok(()) } - fn delete_update_height( + fn delete_update_meta( &mut self, - client_id: ClientId, - height: Height, - ) -> Result<(), ContextError> { - let _ = self - .ibc_store - .lock() - .client_processed_heights - .remove(&(client_id, height)); - - Ok(()) - } - - fn delete_update_time( - &mut self, - client_id: ClientId, - height: Height, - ) -> Result<(), ContextError> { - let _ = self - .ibc_store - .lock() - .client_processed_times - .remove(&(client_id, height)); - - Ok(()) - } - - fn store_update_time( - &mut self, - client_id: ClientId, + client_id: &ClientId, height: Height, - timestamp: Timestamp, ) -> Result<(), ContextError> { - let _ = self - .ibc_store - .lock() - .client_processed_times - .insert((client_id, height), timestamp); - + let key = (client_id.clone(), height); + let mut ibc_store = self.ibc_store.lock(); + ibc_store.client_processed_times.remove(&key); + ibc_store.client_processed_heights.remove(&key); Ok(()) } - fn store_update_height( + fn store_update_meta( &mut self, - client_id: ClientId, + client_id: &ClientId, height: Height, + host_timestamp: Timestamp, host_height: Height, ) -> Result<(), ContextError> { - let _ = self - .ibc_store - .lock() + let mut ibc_store = self.ibc_store.lock(); + ibc_store + .client_processed_times + .insert((client_id.clone(), height), host_timestamp); + ibc_store .client_processed_heights - .insert((client_id, height), host_height); - + .insert((client_id.clone(), height), host_height); Ok(()) } } diff --git a/ibc-testkit/tests/core/ics02_client/update_client.rs b/ibc-testkit/tests/core/ics02_client/update_client.rs index 8d9707566..287ea58ec 100644 --- a/ibc-testkit/tests/core/ics02_client/update_client.rs +++ b/ibc-testkit/tests/core/ics02_client/update_client.rs @@ -147,10 +147,7 @@ fn test_consensus_state_pruning() { expired_height.revision_number(), expired_height.revision_height(), ); - assert!(ctx - .client_update_height(&client_id, &expired_height) - .is_err()); - assert!(ctx.client_update_time(&client_id, &expired_height).is_err()); + assert!(ctx.update_meta(&client_id, expired_height).is_err()); assert!(ctx.consensus_state(&client_cons_state_path).is_err()); // Check that latest valid consensus state exists. @@ -161,14 +158,7 @@ fn test_consensus_state_pruning() { earliest_valid_height.revision_height(), ); - assert!(ctx - .client_update_height(&client_id, &earliest_valid_height) - .is_ok()); - - assert!(ctx - .client_update_time(&client_id, &earliest_valid_height) - .is_ok()); - + assert!(ctx.update_meta(&client_id, earliest_valid_height).is_ok()); assert!(ctx.consensus_state(&client_cons_state_path).is_ok()); let end_host_timestamp = ctx.host_timestamp().unwrap(); diff --git a/ibc-testkit/tests/core/ics04_channel/acknowledgement.rs b/ibc-testkit/tests/core/ics04_channel/acknowledgement.rs index 838f51efe..ead20652a 100644 --- a/ibc-testkit/tests/core/ics04_channel/acknowledgement.rs +++ b/ibc-testkit/tests/core/ics04_channel/acknowledgement.rs @@ -174,16 +174,10 @@ fn ack_success_happy_path(fixture: Fixture) { packet_commitment, ); ctx.get_client_execution_context() - .store_update_time( - ClientId::default(), + .store_update_meta( + &ClientId::default(), client_height, Timestamp::from_nanoseconds(1000).unwrap(), - ) - .unwrap(); - ctx.get_client_execution_context() - .store_update_height( - ClientId::default(), - client_height, Height::new(0, 4).unwrap(), ) .unwrap(); diff --git a/ibc-testkit/tests/core/ics04_channel/recv_packet.rs b/ibc-testkit/tests/core/ics04_channel/recv_packet.rs index 36dd40ff6..ee4844767 100644 --- a/ibc-testkit/tests/core/ics04_channel/recv_packet.rs +++ b/ibc-testkit/tests/core/ics04_channel/recv_packet.rs @@ -140,17 +140,10 @@ fn recv_packet_validate_happy_path(fixture: Fixture) { context .get_client_execution_context() - .store_update_time( - ClientId::default(), + .store_update_meta( + &ClientId::default(), client_height, Timestamp::from_nanoseconds(1000).unwrap(), - ) - .unwrap(); - context - .get_client_execution_context() - .store_update_height( - ClientId::default(), - client_height, Height::new(0, 5).unwrap(), ) .unwrap(); diff --git a/ibc-testkit/tests/core/ics04_channel/timeout.rs b/ibc-testkit/tests/core/ics04_channel/timeout.rs index ce5954abf..f3f7cd109 100644 --- a/ibc-testkit/tests/core/ics04_channel/timeout.rs +++ b/ibc-testkit/tests/core/ics04_channel/timeout.rs @@ -203,15 +203,10 @@ fn timeout_fail_proof_timeout_not_reached(fixture: Fixture) { packet_commitment, ); - ctx.store_update_time( - ClientId::default(), + ctx.store_update_meta( + &ClientId::default(), client_height, Timestamp::from_nanoseconds(5).unwrap(), - ) - .unwrap(); - ctx.store_update_height( - ClientId::default(), - client_height, Height::new(0, 4).unwrap(), ) .unwrap(); @@ -290,16 +285,10 @@ fn timeout_unordered_channel_validate(fixture: Fixture) { ); ctx.get_client_execution_context() - .store_update_time( - ClientId::default(), + .store_update_meta( + &ClientId::default(), client_height, Timestamp::from_nanoseconds(1000).unwrap(), - ) - .unwrap(); - ctx.get_client_execution_context() - .store_update_height( - ClientId::default(), - client_height, Height::new(0, 5).unwrap(), ) .unwrap(); @@ -345,15 +334,10 @@ fn timeout_ordered_channel_validate(fixture: Fixture) { packet_commitment, ); - ctx.store_update_time( - ClientId::default(), + ctx.store_update_meta( + &ClientId::default(), client_height, Timestamp::from_nanoseconds(1000).unwrap(), - ) - .unwrap(); - ctx.store_update_height( - ClientId::default(), - client_height, Height::new(0, 4).unwrap(), ) .unwrap(); diff --git a/ibc-testkit/tests/core/ics04_channel/timeout_on_close.rs b/ibc-testkit/tests/core/ics04_channel/timeout_on_close.rs index 3d6922ef8..23fcdf4e2 100644 --- a/ibc-testkit/tests/core/ics04_channel/timeout_on_close.rs +++ b/ibc-testkit/tests/core/ics04_channel/timeout_on_close.rs @@ -151,17 +151,10 @@ fn timeout_on_close_success_happy_path(fixture: Fixture) { context .get_client_execution_context() - .store_update_time( - ClientId::default(), + .store_update_meta( + &ClientId::default(), Height::new(0, 2).unwrap(), Timestamp::from_nanoseconds(5000).unwrap(), - ) - .unwrap(); - context - .get_client_execution_context() - .store_update_height( - ClientId::default(), - Height::new(0, 2).unwrap(), Height::new(0, 5).unwrap(), ) .unwrap();