Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

perf: avoid temporary Vec allocations when computing commitments #939

Merged
merged 8 commits into from
Nov 2, 2023
Merged
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions .changelog/unreleased/improvements/939-less-alloc.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,2 @@
- Reduce amount of temporary vector allocations.
mina86 marked this conversation as resolved.
Show resolved Hide resolved
([\#939](https://github.com/cosmos/ibc-rs/pull/939))
46 changes: 33 additions & 13 deletions crates/ibc/src/core/ics04_channel/commitment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -86,31 +86,51 @@ pub(crate) fn compute_packet_commitment(
timeout_height: &TimeoutHeight,
timeout_timestamp: &Timestamp,
) -> PacketCommitment {
let mut hash_input = timeout_timestamp.nanoseconds().to_be_bytes().to_vec();
let mut hash_input = [0; 3 * 8 + 32];
mina86 marked this conversation as resolved.
Show resolved Hide resolved

let revision_number = timeout_height.commitment_revision_number().to_be_bytes();
hash_input.append(&mut revision_number.to_vec());
hash_input[..8].copy_from_slice(&timeout_timestamp.nanoseconds().to_be_bytes());
hash_input[8..16].copy_from_slice(&timeout_height.commitment_revision_number().to_be_bytes());
hash_input[16..24].copy_from_slice(&timeout_height.commitment_revision_height().to_be_bytes());
hash_input[24..].copy_from_slice(&hash(packet_data));

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()
PacketCommitment(hash(&hash_input).into())
mina86 marked this conversation as resolved.
Show resolved Hide resolved
}

/// Compute the commitment for an acknowledgement.
pub(crate) fn compute_ack_commitment(ack: &Acknowledgement) -> AcknowledgementCommitment {
hash(ack.as_ref()).into()
AcknowledgementCommitment(hash(ack.as_ref()).into())
mina86 marked this conversation as resolved.
Show resolved Hide resolved
}

/// 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> {
fn hash(data: &[u8]) -> [u8; 32] {
use sha2::Digest;

sha2::Sha256::digest(&data).to_vec()
sha2::Sha256::digest(data).into()
}

#[test]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

As for the tests, I’d like to have @rnbguy's input!

Copy link
Contributor Author

@mina86 mina86 Nov 2, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The test here was purely to make sure that the output doesn’t change during refactoring. I can split this PR into two (one adding the test and the other doing the refactoring) if that’d help to demonstrate it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@mina86 can you please put the tests under

#[cfg(test)]
mod test { ... }

and, make two separate tests for compute_packet_commitment and compute_ack_commitment?

fn test_compute_commitment() {
// PacketCommitment
let want: [u8; 32] = [
169, 40, 181, 31, 98, 189, 84, 0, 145, 236, 69, 31, 78, 243, 69, 121, 79, 5, 158, 101, 145,
mina86 marked this conversation as resolved.
Show resolved Hide resolved
8, 22, 134, 97, 38, 220, 54, 79, 132, 204, 21,
];
let got = compute_packet_commitment(
mina86 marked this conversation as resolved.
Show resolved Hide resolved
"packet data".as_bytes(),
&TimeoutHeight::At(crate::Height::new(42, 24).unwrap()),
&Timestamp::from_nanoseconds(0x42).unwrap(),
);
assert_eq!(&want[..], got.as_ref());

// AcknowledgementCommitment
let want: [u8; 32] = [
5, 78, 222, 193, 208, 33, 31, 98, 79, 237, 12, 188, 169, 212, 249, 64, 11, 14, 73, 28, 67,
116, 42, 242, 197, 176, 171, 235, 240, 201, 144, 216,
];
let ack = Acknowledgement::try_from(vec![0u8, 1, 2, 3]).unwrap();
mina86 marked this conversation as resolved.
Show resolved Hide resolved
let got = compute_ack_commitment(&ack);
assert_eq!(&want[..], got.as_ref())
}
Loading