From 289b6ba288df204f7407ed7192fd7310eb308cb4 Mon Sep 17 00:00:00 2001 From: Farhad Shabani Date: Mon, 6 Mar 2023 04:09:56 -0800 Subject: [PATCH] Export standalone commitment creators for `PacketCommitment` & `AcknowledgementCommitment` (#492) * Export standalone commitment creators for Ack & Packet * Remove compute mod + make compute functions private * Fix clippy catch --- .../470-standalone-commitment-computation.md | 3 ++ crates/ibc/src/core/context.rs | 34 --------------- .../ibc/src/core/context/acknowledgement.rs | 3 +- crates/ibc/src/core/context/recv_packet.rs | 7 ++- crates/ibc/src/core/context/timeout.rs | 3 +- .../ibc/src/core/ics04_channel/commitment.rs | 43 +++++++++++++++++++ crates/ibc/src/core/ics04_channel/context.rs | 24 ----------- .../ics04_channel/handler/acknowledgement.rs | 9 ++-- .../core/ics04_channel/handler/recv_packet.rs | 3 +- .../core/ics04_channel/handler/send_packet.rs | 3 +- .../src/core/ics04_channel/handler/timeout.rs | 7 +-- .../ics04_channel/handler/timeout_on_close.rs | 7 +-- crates/ibc/src/mock/context.rs | 5 --- crates/ibc/src/test_utils.rs | 26 ----------- 14 files changed, 72 insertions(+), 105 deletions(-) create mode 100644 .changelog/unreleased/breaking-changes/470-standalone-commitment-computation.md diff --git a/.changelog/unreleased/breaking-changes/470-standalone-commitment-computation.md b/.changelog/unreleased/breaking-changes/470-standalone-commitment-computation.md new file mode 100644 index 000000000..b8bbfbf67 --- /dev/null +++ b/.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)) \ No newline at end of file diff --git a/crates/ibc/src/core/context.rs b/crates/ibc/src/core/context.rs index 8615c132b..d7489a7c3 100644 --- a/crates/ibc/src/core/context.rs +++ b/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; - /// 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": - /// - 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; - /// 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, diff --git a/crates/ibc/src/core/context/acknowledgement.rs b/crates/ibc/src/core/context/acknowledgement.rs index d054a8368..7d696a061 100644 --- a/crates/ibc/src/core/context/acknowledgement.rs +++ b/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, diff --git a/crates/ibc/src/core/context/recv_packet.rs b/crates/ibc/src/core/context/recv_packet.rs index 469ebbbb5..4e9ce0311 100644 --- a/crates/ibc/src/core/context/recv_packet.rs +++ 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 diff --git a/crates/ibc/src/core/context/timeout.rs b/crates/ibc/src/core/context/timeout.rs index 1e07462ea..c0a1f77a0 100644 --- a/crates/ibc/src/core/context/timeout.rs +++ b/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, diff --git a/crates/ibc/src/core/ics04_channel/commitment.rs b/crates/ibc/src/core/ics04_channel/commitment.rs index ec3df059b..d29170a61 100644 --- a/crates/ibc/src/core/ics04_channel/commitment.rs +++ 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> 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": +/// +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 { + use sha2::Digest; + + sha2::Sha256::digest(&data).to_vec() +} diff --git a/crates/ibc/src/core/ics04_channel/context.rs b/crates/ibc/src/core/ics04_channel/context.rs index b94a13f0b..7bfc43b44 100644 --- a/crates/ibc/src/core/ics04_channel/context.rs +++ b/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; - - fn hash(&self, value: &[u8]) -> Vec; - - fn compute_packet_commitment( - &self, - packet_data: &[u8], - timeout_height: &TimeoutHeight, - timeout_timestamp: &Timestamp, - ) -> PacketCommitment; } impl SendPacketValidationContext for T @@ -78,19 +67,6 @@ where ) -> Result { self.get_next_sequence_send(seq_send_path) } - - fn hash(&self, value: &[u8]) -> Vec { - 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 { diff --git a/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs b/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs index 78689a346..8624428c0 100644 --- a/crates/ibc/src/core/ics04_channel/handler/acknowledgement.rs +++ b/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, diff --git a/crates/ibc/src/core/ics04_channel/handler/recv_packet.rs b/crates/ibc/src/core/ics04_channel/handler/recv_packet.rs index cfdce180c..bdf8e140a 100644 --- a/crates/ibc/src/core/ics04_channel/handler/recv_packet.rs +++ 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, diff --git a/crates/ibc/src/core/ics04_channel/handler/send_packet.rs b/crates/ibc/src/core/ics04_channel/handler/send_packet.rs index 5811d0097..57298cbe3 100644 --- a/crates/ibc/src/core/ics04_channel/handler/send_packet.rs +++ 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, diff --git a/crates/ibc/src/core/ics04_channel/handler/timeout.rs b/crates/ibc/src/core/ics04_channel/handler/timeout.rs index d7e39aac0..e2c1da735 100644 --- a/crates/ibc/src/core/ics04_channel/handler/timeout.rs +++ 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, diff --git a/crates/ibc/src/core/ics04_channel/handler/timeout_on_close.rs b/crates/ibc/src/core/ics04_channel/handler/timeout_on_close.rs index a9e15df60..c0ee297d2 100644 --- a/crates/ibc/src/core/ics04_channel/handler/timeout_on_close.rs +++ 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, diff --git a/crates/ibc/src/mock/context.rs b/crates/ibc/src/mock/context.rs index 5b30455d5..4d9d4bfeb 100644 --- a/crates/ibc/src/mock/context.rs +++ 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 { - sha2::Sha256::digest(value).to_vec() - } - fn client_update_time( &self, client_id: &ClientId, diff --git a/crates/ibc/src/test_utils.rs b/crates/ibc/src/test_utils.rs index 40576d5ac..a12cdc9af 100644 --- a/crates/ibc/src/test_utils.rs +++ b/crates/ibc/src/test_utils.rs @@ -325,32 +325,6 @@ impl SendPacketValidationContext for DummyTransferModule { .into()), } } - - fn hash(&self, value: &[u8]) -> Vec { - use sha2::Digest; - - sha2::Sha256::digest(value).to_vec() - } - - fn compute_packet_commitment( - &self, - packet_data: &[u8], - timeout_height: &crate::core::ics04_channel::timeout::TimeoutHeight, - timeout_timestamp: &crate::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() - } } impl SendPacketExecutionContext for DummyTransferModule {