Export standalone commitment creators for PacketCommitment & `Ackno…

…wledgementCommitment` (#492)

* Export standalone commitment creators for Ack & Packet

* Remove compute mod + make compute functions private

* Fix clippy catch

.changelog/unreleased/breaking-changes/470-standalone-commitment-computation.md

@@ -0,0 +1,3 @@
+Refactor and privatize Packet/Ack commitment computations for improved security
+and modularity.
+([#470](https://github.com/cosmos/ibc-rs/issues/470))

crates/ibc/src/core/context.rs

@@ -41,10 +41,8 @@ use crate::core::ics03_connection::version::{
 use crate::core::ics04_channel::channel::ChannelEnd;
 use crate::core::ics04_channel::commitment::{AcknowledgementCommitment, PacketCommitment};
 use crate::core::ics04_channel::context::calculate_block_delay;
-use crate::core::ics04_channel::msgs::acknowledgement::Acknowledgement;
 use crate::core::ics04_channel::msgs::{ChannelMsg, PacketMsg};
 use crate::core::ics04_channel::packet::{Receipt, Sequence};
-use crate::core::ics04_channel::timeout::TimeoutHeight;
 use crate::core::ics05_port::error::PortError::UnknownPort;
 use crate::core::ics23_commitment::commitment::CommitmentPrefix;
 use crate::core::ics24_host::identifier::{ConnectionId, PortId};
@@ -346,38 +344,6 @@ pub trait ValidationContext: Router {
         ack_path: &AckPath,
     ) -> Result<AcknowledgementCommitment, ContextError>;
 
-    /// Compute the commitment for a packet.
-    /// Note that the absence of `timeout_height` is treated as
-    /// `{revision_number: 0, revision_height: 0}` to be consistent with ibc-go,
-    /// where this value is used to mean "no timeout height":
-    /// <https://github.com/cosmos/ibc-go/blob/04791984b3d6c83f704c4f058e6ca0038d155d91/modules/core/04-channel/keeper/packet.go#L206>
-    fn compute_packet_commitment(
-        &self,
-        packet_data: &[u8],
-        timeout_height: &TimeoutHeight,
-        timeout_timestamp: &Timestamp,
-    ) -> PacketCommitment {
-        let mut hash_input = timeout_timestamp.nanoseconds().to_be_bytes().to_vec();
-
-        let revision_number = timeout_height.commitment_revision_number().to_be_bytes();
-        hash_input.append(&mut revision_number.to_vec());
-
-        let revision_height = timeout_height.commitment_revision_height().to_be_bytes();
-        hash_input.append(&mut revision_height.to_vec());
-
-        let packet_data_hash = self.hash(packet_data);
-        hash_input.append(&mut packet_data_hash.to_vec());
-
-        self.hash(&hash_input).into()
-    }
-
-    fn ack_commitment(&self, ack: &Acknowledgement) -> AcknowledgementCommitment {
-        self.hash(ack.as_ref()).into()
-    }
-
-    /// A hashing function for packet commitments
-    fn hash(&self, value: &[u8]) -> Vec<u8>;
-
     /// 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,

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

@@ -120,6 +120,7 @@ mod tests {
     use crate::core::ics03_connection::connection::State as ConnectionState;
     use crate::core::ics04_channel::channel::Counterparty;
     use crate::core::ics04_channel::channel::State;
+    use crate::core::ics04_channel::commitment::compute_packet_commitment;
     use crate::core::ics04_channel::Version;
     use crate::core::ics24_host::identifier::ChannelId;
     use crate::core::ics24_host::identifier::PortId;
@@ -165,7 +166,7 @@ mod tests {
 
         let packet = msg.packet.clone();
 
-        let packet_commitment = ctx.compute_packet_commitment(
+        let packet_commitment = compute_packet_commitment(
             &msg.packet.data,
             &msg.packet.timeout_height_on_b,
             &msg.packet.timeout_timestamp_on_b,

crates/ibc/src/core/context/recv_packet.rs

@@ -2,6 +2,7 @@ use crate::{
     core::{
         ics04_channel::{
             channel::Order,
+            commitment::compute_ack_commitment,
             error::ChannelError,
             events::{ReceivePacket, WriteAcknowledgement},
             handler::recv_packet,
@@ -102,8 +103,10 @@ where
             msg.packet.sequence,
         );
         // `writeAcknowledgement` handler state changes
-        ctx_b
-            .store_packet_acknowledgement(&ack_path_on_b, ctx_b.ack_commitment(&acknowledgement))?;
+        ctx_b.store_packet_acknowledgement(
+            &ack_path_on_b,
+            compute_ack_commitment(&acknowledgement),
+        )?;
     }
 
     // emit events and logs

crates/ibc/src/core/context/timeout.rs

@@ -151,6 +151,7 @@ mod tests {
     use crate::core::ics03_connection::connection::Counterparty as ConnectionCounterparty;
     use crate::core::ics03_connection::connection::State as ConnectionState;
     use crate::core::ics03_connection::version::get_compatible_versions;
+    use crate::core::ics04_channel::commitment::compute_packet_commitment;
     use crate::core::ics04_channel::commitment::PacketCommitment;
     use crate::core::ics24_host::identifier::ChannelId;
     use crate::core::ics24_host::identifier::PortId;
@@ -200,7 +201,7 @@ mod tests {
 
         let packet = msg.packet.clone();
 
-        let packet_commitment = ctx.compute_packet_commitment(
+        let packet_commitment = compute_packet_commitment(
             &msg.packet.data,
             &msg.packet.timeout_height_on_b,
             &msg.packet.timeout_timestamp_on_b,

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

@@ -1,4 +1,7 @@
+use crate::core::ics04_channel::msgs::acknowledgement::Acknowledgement;
+use crate::core::ics04_channel::timeout::TimeoutHeight;
 use crate::prelude::*;
+use crate::timestamp::Timestamp;
 
 /// Packet commitment
 #[cfg_attr(
@@ -69,3 +72,43 @@ impl From<Vec<u8>> for AcknowledgementCommitment {
         Self(bytes)
     }
 }
+
+/// Compute the commitment for a packet.
+///
+/// Note that the absence of `timeout_height` is treated as
+/// `{revision_number: 0, revision_height: 0}` to be consistent with ibc-go,
+/// where this value is used to mean "no timeout height":
+/// <https://github.com/cosmos/ibc-go/blob/04791984b3d6c83f704c4f058e6ca0038d155d91/modules/core/04-channel/keeper/packet.go#L206>
+pub(crate) fn compute_packet_commitment(
+    packet_data: &[u8],
+    timeout_height: &TimeoutHeight,
+    timeout_timestamp: &Timestamp,
+) -> PacketCommitment {
+    let mut hash_input = timeout_timestamp.nanoseconds().to_be_bytes().to_vec();
+
+    let revision_number = timeout_height.commitment_revision_number().to_be_bytes();
+    hash_input.append(&mut revision_number.to_vec());
+
+    let revision_height = timeout_height.commitment_revision_height().to_be_bytes();
+    hash_input.append(&mut revision_height.to_vec());
+
+    let packet_data_hash = hash(packet_data);
+    hash_input.append(&mut packet_data_hash.to_vec());
+
+    hash(&hash_input).into()
+}
+
+/// Compute the commitment for an acknowledgement.
+pub(crate) fn compute_ack_commitment(ack: &Acknowledgement) -> AcknowledgementCommitment {
+    hash(ack.as_ref()).into()
+}
+
+/// Helper function to hash a byte slice using SHA256.
+///
+/// Note that computing commitments with anything other than SHA256 will
+/// break the Merkle proofs of the IBC provable store.
+fn hash(data: impl AsRef<[u8]>) -> Vec<u8> {
+    use sha2::Digest;
+
+    sha2::Sha256::digest(&data).to_vec()
+}

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

@@ -15,10 +15,8 @@ use crate::core::ics03_connection::connection::ConnectionEnd;
 use crate::core::ics04_channel::channel::ChannelEnd;
 use crate::core::ics04_channel::commitment::PacketCommitment;
 use crate::core::ics24_host::identifier::{ClientId, ConnectionId};
-use crate::timestamp::Timestamp;
 
 use super::packet::Sequence;
-use super::timeout::TimeoutHeight;
 
 pub trait SendPacketValidationContext {
     /// Returns the ChannelEnd for the given `port_id` and `chan_id`.
@@ -38,15 +36,6 @@ pub trait SendPacketValidationContext {
 
     fn get_next_sequence_send(&self, seq_send_path: &SeqSendPath)
         -> Result<Sequence, ContextError>;
-
-    fn hash(&self, value: &[u8]) -> Vec<u8>;
-
-    fn compute_packet_commitment(
-        &self,
-        packet_data: &[u8],
-        timeout_height: &TimeoutHeight,
-        timeout_timestamp: &Timestamp,
-    ) -> PacketCommitment;
 }
 
 impl<T> SendPacketValidationContext for T
@@ -78,19 +67,6 @@ where
     ) -> Result<Sequence, ContextError> {
         self.get_next_sequence_send(seq_send_path)
     }
-
-    fn hash(&self, value: &[u8]) -> Vec<u8> {
-        self.hash(value)
-    }
-
-    fn compute_packet_commitment(
-        &self,
-        packet_data: &[u8],
-        timeout_height: &TimeoutHeight,
-        timeout_timestamp: &Timestamp,
-    ) -> PacketCommitment {
-        self.compute_packet_commitment(packet_data, timeout_height, timeout_timestamp)
-    }
 }
 
 pub trait SendPacketExecutionContext: SendPacketValidationContext {

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

@@ -1,6 +1,7 @@
 use crate::core::ics03_connection::connection::State as ConnectionState;
 use crate::core::ics04_channel::channel::State;
 use crate::core::ics04_channel::channel::{Counterparty, Order};
+use crate::core::ics04_channel::commitment::{compute_ack_commitment, compute_packet_commitment};
 use crate::core::ics04_channel::error::ChannelError;
 use crate::core::ics04_channel::error::PacketError;
 use crate::core::ics04_channel::msgs::acknowledgement::MsgAcknowledgement;
@@ -61,7 +62,7 @@ where
     };
 
     if commitment_on_a
-        != ctx_a.compute_packet_commitment(
+        != compute_packet_commitment(
             &packet.data,
             &packet.timeout_height_on_b,
             &packet.timeout_timestamp_on_b,
@@ -100,7 +101,7 @@ where
         let client_cons_state_path_on_a =
             ClientConsensusStatePath::new(client_id_on_a, &msg.proof_height_on_b);
         let consensus_state = ctx_a.consensus_state(&client_cons_state_path_on_a)?;
-        let ack_commitment = ctx_a.ack_commitment(&msg.acknowledgement);
+        let ack_commitment = compute_ack_commitment(&msg.acknowledgement);
         let ack_path_on_b = AckPath::new(&packet.port_on_b, &packet.chan_on_b, packet.sequence);
         // Verify the proof for the packet against the chain store.
         client_state_on_a
@@ -125,10 +126,10 @@ where
 
 #[cfg(test)]
 mod tests {
+    use crate::core::ics04_channel::commitment::compute_packet_commitment;
     use crate::core::ics04_channel::handler::acknowledgement::validate;
     use crate::core::ics24_host::identifier::ChannelId;
     use crate::core::ics24_host::identifier::PortId;
-    use crate::core::ValidationContext;
     use crate::prelude::*;
     use rstest::*;
     use test_log::test;
@@ -168,7 +169,7 @@ mod tests {
         .unwrap();
         let packet = msg.packet.clone();
 
-        let packet_commitment = context.compute_packet_commitment(
+        let packet_commitment = compute_packet_commitment(
             &packet.data,
             &packet.timeout_height_on_b,
             &packet.timeout_timestamp_on_b,

crates/ibc/src/core/ics04_channel/handler/recv_packet.rs

@@ -1,5 +1,6 @@
 use crate::core::ics03_connection::connection::State as ConnectionState;
 use crate::core::ics04_channel::channel::{Counterparty, Order, State};
+use crate::core::ics04_channel::commitment::compute_packet_commitment;
 use crate::core::ics04_channel::error::ChannelError;
 use crate::core::ics04_channel::error::PacketError;
 use crate::core::ics04_channel::msgs::recv_packet::MsgRecvPacket;
@@ -79,7 +80,7 @@ where
             ClientConsensusStatePath::new(client_id_on_b, &msg.proof_height_on_a);
         let consensus_state_of_a_on_b = ctx_b.consensus_state(&client_cons_state_path_on_b)?;
 
-        let expected_commitment_on_a = ctx_b.compute_packet_commitment(
+        let expected_commitment_on_a = compute_packet_commitment(
             &msg.packet.data,
             &msg.packet.timeout_height_on_b,
             &msg.packet.timeout_timestamp_on_b,

crates/ibc/src/core/ics04_channel/handler/send_packet.rs

@@ -1,5 +1,6 @@
 use crate::core::ics04_channel::channel::Counterparty;
 use crate::core::ics04_channel::channel::State;
+use crate::core::ics04_channel::commitment::compute_packet_commitment;
 use crate::core::ics04_channel::context::SendPacketExecutionContext;
 use crate::core::ics04_channel::events::SendPacket;
 use crate::core::ics04_channel::{
@@ -109,7 +110,7 @@ pub fn send_packet_execute(
 
     ctx_a.store_packet_commitment(
         &CommitmentPath::new(&packet.port_on_a, &packet.chan_on_a, packet.sequence),
-        ctx_a.compute_packet_commitment(
+        compute_packet_commitment(
             &packet.data,
             &packet.timeout_height_on_b,
             &packet.timeout_timestamp_on_b,

crates/ibc/src/core/ics04_channel/handler/timeout.rs

@@ -1,5 +1,6 @@
 use crate::core::ics04_channel::channel::State;
 use crate::core::ics04_channel::channel::{Counterparty, Order};
+use crate::core::ics04_channel::commitment::compute_packet_commitment;
 use crate::core::ics04_channel::error::ChannelError;
 use crate::core::ics04_channel::error::PacketError;
 use crate::core::ics04_channel::msgs::timeout::MsgTimeout;
@@ -59,7 +60,7 @@ where
         Err(_) => return Ok(()),
     };
 
-    let expected_commitment_on_a = ctx_a.compute_packet_commitment(
+    let expected_commitment_on_a = compute_packet_commitment(
         &msg.packet.data,
         &msg.packet.timeout_height_on_b,
         &msg.packet.timeout_timestamp_on_b,
@@ -150,6 +151,7 @@ where
 
 #[cfg(test)]
 mod tests {
+    use crate::core::ics04_channel::commitment::compute_packet_commitment;
     use crate::prelude::*;
     use rstest::*;
 
@@ -165,7 +167,6 @@ mod tests {
     use crate::core::ics04_channel::msgs::timeout::MsgTimeout;
     use crate::core::ics04_channel::Version;
     use crate::core::ics24_host::identifier::{ChannelId, ClientId, ConnectionId, PortId};
-    use crate::core::ValidationContext;
     use crate::mock::context::MockContext;
     use crate::timestamp::ZERO_DURATION;
 
@@ -196,7 +197,7 @@ mod tests {
         .unwrap();
         let packet = msg.packet.clone();
 
-        let packet_commitment = context.compute_packet_commitment(
+        let packet_commitment = compute_packet_commitment(
             &msg.packet.data,
             &msg.packet.timeout_height_on_b,
             &msg.packet.timeout_timestamp_on_b,

crates/ibc/src/core/ics04_channel/handler/timeout_on_close.rs

@@ -1,5 +1,6 @@
 use crate::core::ics04_channel::channel::State;
 use crate::core::ics04_channel::channel::{ChannelEnd, Counterparty, Order};
+use crate::core::ics04_channel::commitment::compute_packet_commitment;
 use crate::core::ics04_channel::error::{ChannelError, PacketError};
 use crate::core::ics04_channel::msgs::timeout_on_close::MsgTimeoutOnClose;
 use crate::core::ics24_host::path::{
@@ -44,7 +45,7 @@ where
         Err(_) => return Ok(()),
     };
 
-    let expected_commitment_on_a = ctx_a.compute_packet_commitment(
+    let expected_commitment_on_a = compute_packet_commitment(
         &packet.data,
         &packet.timeout_height_on_b,
         &packet.timeout_timestamp_on_b,
@@ -161,9 +162,9 @@ where
 
 #[cfg(test)]
 mod tests {
+    use crate::core::ics04_channel::commitment::compute_packet_commitment;
     use crate::core::ics04_channel::commitment::PacketCommitment;
     use crate::core::ics04_channel::handler::timeout_on_close::validate;
-    use crate::core::ValidationContext;
     use crate::mock::context::MockContext;
     use crate::prelude::*;
     use crate::Height;
@@ -204,7 +205,7 @@ mod tests {
 
         let packet = msg.packet.clone();
 
-        let packet_commitment = context.compute_packet_commitment(
+        let packet_commitment = compute_packet_commitment(
             &msg.packet.data,
             &msg.packet.timeout_height_on_b,
             &msg.packet.timeout_timestamp_on_b,

crates/ibc/src/mock/context.rs

@@ -17,7 +17,6 @@ use core::time::Duration;
 use parking_lot::Mutex;
 
 use ibc_proto::google::protobuf::Any;
-use sha2::Digest;
 use tracing::debug;
 
 use crate::clients::ics07_tendermint::client_state::test_util::get_dummy_tendermint_client_state;
@@ -1042,10 +1041,6 @@ impl ValidationContext for MockContext {
         .map_err(ContextError::PacketError)
     }
 
-    fn hash(&self, value: &[u8]) -> Vec<u8> {
-        sha2::Sha256::digest(value).to_vec()
-    }
-
     fn client_update_time(
         &self,
         client_id: &ClientId,