feat: replace store_update_{time,height} by metadata update

store_update_time and store_update_height methods are always called
together. In fact, they are also usually called together with
store_consensus_state.

Sadly, since they are separate calls, the implementations need to
treat them as separate pieces of data. For example, if an
implementation stores update times and heights in a map, it must
perform two separate lookups to update each value.

Change the interface such that store_consensus_state in addition takes
host timestamp and height. This makes it possible for implementations
to perform optimisations where timestamp and height are stored in
a single location. This reduces number of lookups and possibly also
improves memory footprint of the implementation.

In fact, they may be stared together with the consensus state making
the optimisation potential even stronger.

Alas, updating of the timestamp and height is at times called without
updating the consensus state. To accommodate that use case, introduce
store_consensus_metadata call which only takes the timestamp and
height arguments.

.changelog/unreleased/breaking-changes/973-consensus-metadata.md

@@ -0,0 +1,6 @@
+- Replace store_update_time and store_update_height with new
+  functionality of store_consensus_state and a new
+  store_consensus_metadata method.  Furthermore, get rid of
+  delete_update_time and delete_update_height; deletion is now done in
+  delete_consensus_state.
+  ([#973](https://github.com/cosmos/ibc-rs/issues))

crates/ibc-testkit/src/testapp/ibc/clients/mock/client_state.rs

@@ -322,6 +322,8 @@ where
         ctx.store_consensus_state(
             ClientConsensusStatePath::new(client_id, &self.latest_height()),
             mock_consensus_state.into(),
+            ctx.host_timestamp()?,
+            ctx.host_height()?,
         )?;
 
         Ok(())
@@ -342,10 +344,10 @@ where
         ctx.store_consensus_state(
             ClientConsensusStatePath::new(client_id, &new_client_state.latest_height()),
             new_consensus_state.into(),
+            ctx.host_timestamp()?,
+            ctx.host_height()?,
         )?;
         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()?)?;
 
         Ok(vec![header_height])
     }
@@ -379,15 +381,11 @@ where
         ctx.store_consensus_state(
             ClientConsensusStatePath::new(client_id, &latest_height),
             new_consensus_state.into(),
+            ctx.host_timestamp()?,
+            ctx.host_height()?,
         )?;
         ctx.store_client_state(ClientStatePath::new(client_id), new_client_state.into())?;
 
-        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)?;
-
         Ok(latest_height)
     }
 }

crates/ibc-testkit/src/testapp/ibc/core/client_ctx.rs

@@ -224,102 +224,79 @@ impl ClientExecutionContext for MockContext {
         &mut self,
         consensus_state_path: ClientConsensusStatePath,
         consensus_state: Self::AnyConsensusState,
+        host_timestamp: Timestamp,
+        host_height: Height,
     ) -> Result<(), ContextError> {
+        let ClientConsensusStatePath {
+            client_id,
+            epoch,
+            height,
+        } = consensus_state_path;
+        let height = Height::new(epoch, height)?;
+
         let mut ibc_store = self.ibc_store.lock();
 
-        let client_record = ibc_store
+        ibc_store
             .clients
-            .entry(consensus_state_path.client_id)
+            .entry(client_id.clone())
             .or_insert(MockClientRecord {
                 consensus_states: Default::default(),
                 client_state: Default::default(),
-            });
-
-        let height = Height::new(consensus_state_path.epoch, consensus_state_path.height)
-            .expect("Never fails");
-        client_record
+            })
             .consensus_states
             .insert(height, consensus_state);
 
-        Ok(())
-    }
-
-    fn delete_consensus_state(
-        &mut self,
-        consensus_state_path: ClientConsensusStatePath,
-    ) -> Result<(), ContextError> {
-        let mut ibc_store = self.ibc_store.lock();
-
-        let client_record = ibc_store
-            .clients
-            .entry(consensus_state_path.client_id)
-            .or_insert(MockClientRecord {
-                consensus_states: Default::default(),
-                client_state: Default::default(),
-            });
-
-        let height = Height::new(consensus_state_path.epoch, consensus_state_path.height)
-            .expect("Never fails");
-
-        client_record.consensus_states.remove(&height);
-
-        Ok(())
-    }
-
-    fn delete_update_height(
-        &mut self,
-        client_id: ClientId,
-        height: Height,
-    ) -> Result<(), ContextError> {
-        let _ = self
-            .ibc_store
-            .lock()
+        ibc_store
+            .client_processed_times
+            .insert((client_id.clone(), height), host_timestamp);
+        ibc_store
             .client_processed_heights
-            .remove(&(client_id, height));
+            .insert((client_id, height), host_height);
 
         Ok(())
     }
 
-    fn delete_update_time(
+    fn store_consensus_metadata(
         &mut self,
         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();
+
+        let key = (client_id, height);
+        ibc_store
             .client_processed_times
-            .remove(&(client_id, height));
+            .insert(key.clone(), host_timestamp);
+        ibc_store.client_processed_heights.insert(key, host_height);
 
         Ok(())
     }
 
-    fn store_update_time(
+    fn delete_consensus_state(
         &mut self,
-        client_id: ClientId,
-        height: Height,
-        timestamp: Timestamp,
+        consensus_state_path: ClientConsensusStatePath,
     ) -> Result<(), ContextError> {
-        let _ = self
-            .ibc_store
-            .lock()
-            .client_processed_times
-            .insert((client_id, height), timestamp);
+        let ClientConsensusStatePath {
+            client_id,
+            epoch,
+            height,
+        } = consensus_state_path;
+        let height = Height::new(epoch, height)?;
 
-        Ok(())
-    }
+        let mut ibc_store = self.ibc_store.lock();
 
-    fn store_update_height(
-        &mut self,
-        client_id: ClientId,
-        height: Height,
-        host_height: Height,
-    ) -> Result<(), ContextError> {
-        let _ = self
-            .ibc_store
-            .lock()
-            .client_processed_heights
-            .insert((client_id, height), host_height);
+        let client_record =
+            ibc_store
+                .clients
+                .entry(client_id.clone())
+                .or_insert(MockClientRecord {
+                    consensus_states: Default::default(),
+                    client_state: Default::default(),
+                });
+
+        client_record.consensus_states.remove(&height);
 
         Ok(())
     }

crates/ibc-testkit/tests/core/ics04_channel/acknowledgement.rs

@@ -161,17 +161,12 @@ fn ack_success_happy_path(fixture: Fixture) {
             msg.packet.seq_on_a,
             packet_commitment,
         );
+
     ctx.get_client_execution_context()
-        .store_update_time(
+        .store_consensus_metadata(
             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();

crates/ibc-testkit/tests/core/ics04_channel/recv_packet.rs

@@ -136,17 +136,10 @@ fn recv_packet_validate_happy_path(fixture: Fixture) {
 
     context
         .get_client_execution_context()
-        .store_update_time(
+        .store_consensus_metadata(
             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();

crates/ibc-testkit/tests/core/ics04_channel/timeout.rs

@@ -191,15 +191,10 @@ fn timeout_fail_proof_timeout_not_reached(fixture: Fixture) {
             packet_commitment,
         );
 
-    ctx.store_update_time(
+    ctx.store_consensus_metadata(
         ClientId::default(),
         client_height,
         Timestamp::from_nanoseconds(5).unwrap(),
-    )
-    .unwrap();
-    ctx.store_update_height(
-        ClientId::default(),
-        client_height,
         Height::new(0, 4).unwrap(),
     )
     .unwrap();
@@ -274,16 +269,10 @@ fn timeout_unordered_channel_validate(fixture: Fixture) {
         );
 
     ctx.get_client_execution_context()
-        .store_update_time(
+        .store_consensus_metadata(
             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();
@@ -325,15 +314,10 @@ fn timeout_ordered_channel_validate(fixture: Fixture) {
             packet_commitment,
         );
 
-    ctx.store_update_time(
+    ctx.store_consensus_metadata(
         ClientId::default(),
         client_height,
         Timestamp::from_nanoseconds(1000).unwrap(),
-    )
-    .unwrap();
-    ctx.store_update_height(
-        ClientId::default(),
-        client_height,
         Height::new(0, 4).unwrap(),
     )
     .unwrap();

crates/ibc-testkit/tests/core/ics04_channel/timeout_on_close.rs

@@ -147,17 +147,10 @@ fn timeout_on_close_success_happy_path(fixture: Fixture) {
 
     context
         .get_client_execution_context()
-        .store_update_time(
+        .store_consensus_metadata(
             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();

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

@@ -509,9 +509,9 @@ where
         ctx.store_consensus_state(
             ClientConsensusStatePath::new(client_id, &self.latest_height),
             tm_consensus_state.into(),
+            host_timestamp,
+            host_height,
         )?;
-        ctx.store_update_time(client_id.clone(), self.latest_height(), host_timestamp)?;
-        ctx.store_update_height(client_id.clone(), self.latest_height(), host_height)?;
 
         Ok(())
     }
@@ -548,10 +548,10 @@ where
             ctx.store_consensus_state(
                 ClientConsensusStatePath::new(client_id, &new_client_state.latest_height),
                 new_consensus_state.into(),
+                host_timestamp,
+                host_height,
             )?;
             ctx.store_client_state(ClientStatePath::new(client_id), 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)?;
         }
 
         Ok(vec![header_height])
@@ -628,9 +628,9 @@ where
         ctx.store_consensus_state(
             ClientConsensusStatePath::new(client_id, &latest_height),
             new_consensus_state.into(),
+            host_timestamp,
+            host_height,
         )?;
-        ctx.store_update_time(client_id.clone(), latest_height, host_timestamp)?;
-        ctx.store_update_height(client_id.clone(), latest_height, host_height)?;
 
         Ok(latest_height)
     }

crates/ibc/src/clients/ics07_tendermint/client_state/update_client.rs

@@ -197,13 +197,8 @@ 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)?;
         }
 
         Ok(())