crypto/packet/benches/packet_crypto.rs (11)

28-28: Simplify reference in TicketBuilder method call

In line 28, the expression &(&chain_key).into() contains an unnecessary extra reference. You can simplify it to &chain_key.into(), which is more idiomatic and improves readability.

Apply this diff to simplify the code:

-let tb = TicketBuilder::zero_hop().direction(&(&chain_key).into(), &destination);
+let tb = TicketBuilder::zero_hop().direction(&chain_key.into(), &destination);

---

23-33: Move throughput setting outside the loop

Currently, group.throughput(Throughput::Elements(1)) is set inside the loop on line 24. Since the throughput remains constant for all iterations, consider moving it outside the loop to avoid redundant calls and enhance clarity.

Apply this diff to adjust the code:

 let mut group = c.benchmark_group("packet_sending");
 group.sample_size(SAMPLE_SIZE);
+group.throughput(Throughput::Elements(1));

 for hop in [0, 1, 2, 3].iter() {
-    group.throughput(Throughput::Elements(1));
     group.bench_with_input(BenchmarkId::from_parameter(format!("{hop} hop")), hop, |b, &hop| {
         b.iter(|| {

---

14-16: Avoid unnecessary cloning of public keys

In lines 15-16, when generating the path, you clone the public keys with .public().clone(). If public() returns a reference or a type that is Copy, cloning may be unnecessary. Eliminating redundant clones can improve performance, especially in benchmarks.

Modify the code as follows:

 let path = (0..=3)
-    .map(|_| OffchainKeypair::random().public().clone())
+    .map(|_| OffchainKeypair::random().public())
     .collect::<Vec<_>>();

Ensure that subsequent code works correctly without the clones.

---

44-44: Eliminate unnecessary clones in path initialization

Similarly, in line 44, you're cloning public keys when creating the path array. Avoiding these clones can reduce overhead.

Adjust the code as shown:

 let path = [
-    relayer.public().clone(),
-    recipient.public().clone()
+    relayer.public(),
+    recipient.public()
 ];

Verify that the rest of the code handles references appropriately.

---

50-50: Simplify reference in TicketBuilder method call

In line 50, the expression &(&chain_key).into() can be simplified to &chain_key.into() for consistency and better readability.

Apply this diff:

-let tb = TicketBuilder::zero_hop().direction(&(&chain_key).into(), &destination);
+let tb = TicketBuilder::zero_hop().direction(&chain_key.into(), &destination);

---

28-28: Consistency in TicketBuilder usage

The simplification of &(&chain_key).into() to &chain_key.into() in lines 28, 50, and 87 promotes consistency across your codebase and enhances readability.

Ensure that all instances are updated for consistency.

Also applies to: 50-50, 87-87

---

65-66: Review necessity of throughput setting

In lines 66, you're setting group.throughput(Throughput::Elements(1)) for the packet_forwarding_bench. Since this benchmark only processes single elements, consider whether setting the throughput provides additional insight. If not, you might omit it to simplify the code.

If you decide to keep it, no action is needed.

---

117-117: Use expect to provide meaningful error messages

In line 117, the use of .unwrap() could be replaced with .expect("descriptive message") to give more context in case of failure during the benchmark.

Modify the line as follows:

-ChainPacketComponents::from_incoming(&packet, &recipient, relayer.public().clone()).unwrap();
+ChainPacketComponents::from_incoming(&packet, &recipient, relayer.public().clone()).expect("Failed to process incoming packet");

---

53-61: Refactor duplicated packet assembly code

The code blocks in lines 53-61, 90-98, and 101-109 for assembling packets are similar. Consider refactoring this logic into a helper function to reduce code duplication and improve maintainability.

For example:

fn assemble_packet(packet_component: PacketType, ticket: TicketType) -> Box<[u8]> {
    let mut ret = Vec::with_capacity(ChainPacketComponents::SIZE);
    ret.extend_from_slice(packet_component.as_ref());
    ret.extend_from_slice(&ticket.into_encoded());
    ret.into_boxed_slice()
}

Then replace the duplicated code with calls to assemble_packet.

Also applies to: 90-98, 101-109

---

1-7: Organize imports according to Rust conventions

The import statements can be grouped and ordered to enhance readability. Place external crate imports first, followed by internal module imports, and sort them alphabetically within each group.

Consider reorganizing as follows:

use criterion::{criterion_group, criterion_main, BenchmarkId, Criterion, Throughput};
use hopr_crypto_packet::chain::ChainPacketComponents;
use hopr_crypto_types::prelude::{ChainKeypair, Keypair, OffchainKeypair};
use hopr_crypto_types::types::Hash;
use hopr_crypto_random::random_bytes;
use hopr_internal_types::prelude::{TicketBuilder, PAYLOAD_SIZE};
use hopr_primitive_types::prelude::{Address, BytesEncodable};

Confirm that no import is missing or unused after reordering.

---

23-23: Use inclusive range for loop over hops

In line 23, the loop iterates over [0, 1, 2, 3]. You can simplify this by using an inclusive range 0..=3.

Modify the loop as:

-for hop in [0, 1, 2, 3].iter() {
+for hop in 0..=3 {

This change makes the code more concise.

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Description: Verify the usage of cryptographic algorithms in packet operations

# Check for usage of these algorithms in the benchmark code
echo "Checking usage of cryptographic algorithms in benchmarks..."
rg -A 5 "ed25519|secp256k1|x25519" "crypto/packet/benches/"

# Check if these features are consistently enabled in other dependent crates
echo "Checking feature consistency across workspace..."
rg -l "hopr-crypto-sphinx.*features.*" -g "Cargo.toml"

#!/bin/bash
# Check the actual implementation files for usage of these cryptographic algorithms
echo "Checking implementation files for cryptographic algorithm usage..."
rg -A 5 "ed25519|secp256k1|x25519" "crypto/packet/src/"

# Check the feature definitions in hopr-crypto-sphinx
echo "Checking hopr-crypto-sphinx feature definitions..."
rg -A 5 "^\[features\]" $(fd "Cargo.toml" | grep "crypto-sphinx")

# Check for any tests using these cryptographic algorithms
echo "Checking test files for cryptographic algorithm usage..."
rg -A 5 "ed25519|secp256k1|x25519" "crypto/packet/tests/"

#!/bin/bash
# Description: Test various sample sizes to find the optimal value.

for SAMPLE_SIZE in 1000 10000 50000 100000; do
    echo "Testing SAMPLE_SIZE=$SAMPLE_SIZE"
    sed -i "s/const SAMPLE_SIZE: usize = [0-9_]*/const SAMPLE_SIZE: usize = $SAMPLE_SIZE;/" benches/packet_crypto.rs
    cargo bench -p hopr-crypto-packet
done

#!/bin/bash
# Let's first find the correct path to the benchmark file
fd "packet_crypto.rs" 

# Let's examine the benchmark file content to understand the context
rg -A 10 "SAMPLE_SIZE" 

# Let's also look for any other benchmark files in the project for comparison
fd "bench" --type f

# And check for any documentation about benchmarking
fd "README|CONTRIBUTING" --type f | xargs rg -i "benchmark"

#!/bin/bash
# Let's examine the benchmark file content to understand what it's benchmarking
rg -l "packet_crypto" | xargs rg -A 10 "packet_sending_bench|packet_receiving_bench"

# Let's also check if there are any similar benchmarks in the codebase for comparison
rg -l "bench" | xargs rg -A 5 "SAMPLE_SIZE.*=.*\d+"

# Let's look for any documentation about the benchmarking methodology
fd "README|CONTRIBUTING" --type f | xargs rg -i "crypto.*bench|packet.*bench"

#!/bin/bash
# Let's examine the full benchmark file to understand the context
rg -A 20 "packet_sending_bench" crypto/packet/benches/packet_crypto.rs

# Let's also check what's being measured in these benchmarks
rg -A 20 "group.bench" crypto/packet/benches/packet_crypto.rs

# Let's see if there are any related test files that might give context
fd "test.*packet" --type f

crypto/packet/benches/packet_crypto.rs (2)

8-8: Consider adjusting the sample size based on system constraints

The sample size of 100,000 iterations might lead to long benchmark execution times and high memory usage. Consider making this configurable or reducing it based on the system's capabilities.

-const SAMPLE_SIZE: usize = 100_000;
+// TODO: Make this configurable via environment variable
+const SAMPLE_SIZE: usize = std::env::var("BENCH_SAMPLE_SIZE")
+    .map(|v| v.parse().unwrap_or(10_000))
+    .unwrap_or(10_000);

---

27-28: Enhance documentation about ticket creation

The comment about hop count for ticket creation could be more detailed to explain why it doesn't affect the benchmark.

-                // The number of hops for ticket creation does not matter for benchmark purposes
+                // We use zero-hop tickets since we're only benchmarking packet creation,
+                // not the actual ticket verification or processing

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Description: Check for any remaining references to divan in the codebase

# Search for divan in all Cargo.toml files
echo "Checking Cargo.toml files for divan references:"
rg "divan" -g "Cargo.toml"

# Search for divan imports in Rust files
echo "Checking Rust files for divan imports:"
rg "use.*divan" -g "*.rs"

# Search for divan macros or attributes
echo "Checking Rust files for divan macros/attributes:"
rg "#\[.*divan" -g "*.rs"

#!/bin/bash
# Search for divan in all Rust files excluding vendor directory
echo "Checking project Rust files for divan references:"
rg "divan" -g "*.rs" --glob "!vendor/**"

# Search for divan in all Cargo.toml files excluding vendor directory
echo "Checking project Cargo.toml files for divan references:"
rg "divan" -g "Cargo.toml" --glob "!vendor/**"

# List all benches directories to check for benchmark files
echo "Checking for benchmark directories:"
fd -t d "benches$" --exclude vendor/

#!/bin/bash
# Check contents of benchmark files in the project
echo "Checking benchmark files in common/network-types/benches/:"
rg "divan" common/network-types/benches/ || echo "No divan references found"

echo -e "\nChecking benchmark files in crypto/packet/benches/:"
rg "divan" crypto/packet/benches/ || echo "No divan references found"

# List all files in these benchmark directories
echo -e "\nListing files in benchmark directories:"
find common/network-types/benches crypto/packet/benches -type f

#!/bin/bash
# Display contents of benchmark files
echo "Contents of common/network-types/benches/session.rs:"
cat common/network-types/benches/session.rs

echo -e "\nContents of crypto/packet/benches/packet_crypto.rs:"
cat crypto/packet/benches/packet_crypto.rs