common/internal-types/benches/bloom_filter.rs (2)

6-9: Consider pre-generating random data for initialization

Creating random bytes in a tight loop during initialization might affect system resources. Consider pre-generating a vector of random bytes before the benchmark.

-    for _ in 1..bloom.capacity() {
-        bloom.set(&hopr_crypto_random::random_bytes());
+    let random_data: Vec<_> = (0..bloom.capacity())
+        .map(|_| hopr_crypto_random::random_bytes())
+        .collect();
+    for tag in random_data {
+        bloom.set(&tag);

---

14-16: Consider adjusting benchmark parameters based on false positive rate

Given the PR mentions a false positive rate of 0.001%, the sample size of 100,000 might need adjustment to effectively capture this rate in the benchmarks.

Consider increasing the sample size or adding specific benchmarks to measure false positive occurrences.

common/internal-types/src/protocol.rs (2)

163-172: Consider adding size validation in from_bytes.

While the implementation is correct, consider adding validation for the deserialized size to ensure it matches the expected capacity.

 pub fn from_bytes(data: &[u8]) -> Result<Self> {
-    bincode::deserialize(data).map_err(|e| CoreTypesError::ParseError(e.to_string()))
+    let filter: Self = bincode::deserialize(data)
+        .map_err(|e| CoreTypesError::ParseError(e.to_string()))?;
+    if filter.count > filter.capacity {
+        return Err(CoreTypesError::ParseError("Invalid count exceeds capacity".into()));
+    }
+    Ok(filter)
 }

---

326-368: LGTM! Comprehensive test coverage with room for improvement.

The new tests thoroughly verify:

• Count management
• Capacity limits
• Wrap-around behavior
• Edge cases

Consider adding tests for:

• Concurrent access scenarios
• Serialization/deserialization with various capacities

Configuration used: .coderabbit.yaml
Review profile: CHILL

#!/bin/bash
# Search for capacity configuration in the codebase
rg -A 5 "TagBloomFilter.*new|TagBloomFilter.*default" 

#!/bin/bash
# Search for TagBloomFilter implementation
ast-grep --pattern 'struct TagBloomFilter {
  $$$
}'

# Also search for any constants or configurations related to capacity
rg -A 5 "const.*CAPACITY|BLOOM_FILTER.*CAPACITY"

#!/bin/bash
# Search for TagBloomFilter implementation
ast-grep --pattern 'impl TagBloomFilter {
  $$$
}'

# Also search for any constants or configurations related to TagBloomFilter capacity
rg "TagBloomFilter.*capacity|BLOOM_FILTER.*CAPACITY|BLOOM.*CAPACITY" -A 5

#!/bin/bash
# Description: Search for tests that verify edge cases in check_and_set behavior

# Look for test cases involving capacity and count management
rg -l "check_and_set.*capacity|count" --type rust

#!/bin/bash
# Search for tests related to the Bloom filter implementation
rg -l "bloom.*test|test.*bloom" --type rust

# Search for the actual Bloom filter implementation
rg -l "struct.*Bloom|impl.*Bloom" --type rust

#!/bin/bash
# Check the Bloom filter implementation and tests
cat transport/protocol/src/bloom.rs common/internal-types/src/protocol.rs vendor/cargo/bloomfilter-2.0.0/tests/bloom.rs

common/internal-types/benches/bloom_filter.rs (3)

18-20: Document benchmark configuration choices

Consider adding comments explaining why these specific values were chosen:

• Sample size of 100,000
• Throughput of 1 element

     let mut group = c.benchmark_group("tag_bloom_filter");
+    // Large sample size to ensure statistical significance
     group.sample_size(100_000);
+    // Each operation processes one tag
     group.throughput(Throughput::Elements(1));

---

22-26: Consider expanding benchmark coverage

The benchmark only measures check operations. Consider adding benchmarks for:

• set operations
• Multiple consecutive operations to measure any caching effects

     group.bench_function("check", |b| {
         b.iter(|| {
             bloom.check(black_box(&existing_tag));
         })
     });
+
+    group.bench_function("set", |b| {
+        b.iter(|| {
+            let tag = hopr_crypto_random::random_bytes();
+            bloom.set(black_box(&tag));
+        })
+    });

---

28-33: Use more realistic test data for non-existent tags

Using a zeroed array might not represent real-world scenarios well and could lead to optimizations that wouldn't occur in production.

-    let non_existent_tag = [0u8; PACKET_TAG_LENGTH];
+    // Generate a random tag and ensure it's not in the filter
+    let mut non_existent_tag = hopr_crypto_random::random_bytes();
+    while bloom.check(&non_existent_tag) {
+        non_existent_tag = hopr_crypto_random::random_bytes();
+    }

Configuration used: .coderabbit.yaml
Review profile: CHILL

     let mut bloom = TagBloomFilter::default();
+    assert_eq!(bloom.capacity(), 10_000_000, "Unexpected filter capacity");
     for _ in 1..bloom.capacity() {
-        let mut tag = hopr_crypto_random::random_bytes();
-        tag[0] = 0xaa;
+        let tag = hopr_crypto_random::random_bytes();
         bloom.set(&tag);
     }

common/internal-types/src/protocol.rs (1)

139-157: Consider improving readability with early returns.

The logic is correct but could be more readable. Consider restructuring with early returns:

 pub fn check_and_set(&mut self, tag: &PacketTag) -> bool {
-    if self.count == self.capacity {
-        let is_present = self.bloom.check(tag);
-        if !is_present {
-            warn!("maximum number of items in the Bloom filter reached!");
-            self.bloom.clear();
-            self.bloom.set(tag);
-            self.count = 1;
-        }
-        is_present
-    } else {
-        let was_present = self.bloom.check_and_set(tag);
-        if !was_present {
-            self.count += 1;
-        }
-        was_present
+    // Handle full capacity case
+    if self.count == self.capacity {
+        let is_present = self.bloom.check(tag);
+        if is_present {
+            return true;
+        }
+        
+        // Reset filter for new item
+        warn!("maximum number of items in the Bloom filter reached!");
+        self.bloom.clear();
+        self.bloom.set(tag);
+        self.count = 1;
+        return false;
     }
+    
+    // Handle normal case
+    let was_present = self.bloom.check_and_set(tag);
+    if !was_present {
+        self.count += 1;
+    }
+    was_present
 }

Configuration used: .coderabbit.yaml
Review profile: CHILL

 pub fn to_bytes(&self) -> Box<[u8]> {
-    bincode::serialize(&self)
-        .expect("serialization must not fail")
-        .into_boxed_slice()
+    bincode::serialize(&self)
+        .map_err(|e| CoreTypesError::SerializationError(e.to_string()))?
+        .into_boxed_slice()
 }