test(sync): I5 protection and CIP §2.3 negotiation compliance tests#1954
Closed
test(sync): I5 protection and CIP §2.3 negotiation compliance tests#1954
Conversation
Introduces a deterministic simulation framework for testing the sync protocol and adds comprehensive test coverage for: 1. Snapshot Merge Protection (Invariant I5) - Verifies initialized nodes never receive Snapshot protocol - Tests protection across various entity counts and scenarios - Confirms fresh nodes can still bootstrap via Snapshot 2. CIP §2.3 Protocol Negotiation Compliance - Tests all 7 rules of the protocol selection decision table - Verifies rule priority ordering - Confirms deterministic protocol selection The simulation framework (`sync_sim`) provides: - Deterministic RNG for reproducible tests - SimNode with state tracking and handshake generation - Scenario builders for common test setups - Network simulation with partition and fault injection Relates to #1783, #1785
![meroreviewer[bot]](https://avatars.githubusercontent.com/in/2793509?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 3 agents | Quality score: 100% | Review time: 226.7s
🟡 1 warnings, 💡 1 suggestions. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-6f42797d
| Scenario::force_hash_high_divergence(), | ||
| ), | ||
| ("force_subtree_prefetch", Scenario::force_subtree_prefetch()), | ||
| ("force_bloom_filter", Scenario::force_bloom_filter()), |
There was a problem hiding this comment.
🟡 Logic error: nodes created from separate scenario generations
Calls n_nodes_diverged(2) twice, taking node 0 from the first call and node 1 from the second call, resulting in nodes from different scenario generations rather than a single diverged pair as the comment suggests.
Suggested fix:
Store the result of a single `n_nodes_diverged(2)` call and extract both nodes from it: `let mut nodes = Scenario::n_nodes_diverged(2).into_iter(); let a = rt.add_existing_node(nodes.next().unwrap()); let b = rt.add_existing_node(nodes.next().unwrap());`
|
|
||
| let hs_a = a.build_handshake(); | ||
| let hs_b = b.build_handshake(); | ||
|
|
There was a problem hiding this comment.
💡 DRY: Divergence calculation duplicated
The divergence calculation logic (max_count/min_count/division) is repeated in test_cip23_rule3_high_divergence_hash_comparison and test_cip23_rule4_deep_tree_subtree_prefetch; consider extracting to a helper function.
Suggested fix:
Add `fn calculate_divergence(hs_a: &SyncHandshake, hs_b: &SyncHandshake) -> f64` helper and reuse across tests.
xilosada
changed the title
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
| 1.0 - (min_count / max_count) | ||
| } else { | ||
| 0.0 | ||
| }; |
Contributor
There was a problem hiding this comment.
Test divergence formula differs from actual implementation
Low Severity
The test precondition checks compute divergence as 1.0 - (min_count / max_count), a symmetric formula. However, the actual calculate_divergence() in protocol.rs uses abs_diff / remote.entity_count.max(1), an asymmetric formula. These formulas agree only when local ≤ remote (as happens to be the case now), but differ when local > remote. For example, with local=150, remote=100: the test formula gives ~33% while the real function gives 50%. The precondition assertions would not catch a scenario misconfiguration if entity counts are ever reordered.
Additional Locations (1)
| seed, | ||
| selection.protocol | ||
| ); | ||
| } |
Contributor
There was a problem hiding this comment.
Random seeds test uses fully deterministic identical inputs
Medium Severity
test_protection_holds_across_random_seeds claims to test across 100 random seeds, but Scenario::n_nodes_diverged(2) is completely deterministic and takes no seed parameter — every call returns identical nodes. The SimRuntime::new(seed) creates a seeded RNG that is never consumed since the test only builds pre-configured nodes and calls select_protocol. This loop effectively runs the exact same assertion 100 times, providing a false sense of coverage. Using RandomScenario::two_nodes_random(seed) or similar would actually vary the test data per seed.
| 0 | ||
| } else { | ||
| (64 - entity_count.leading_zeros()).min(32) | ||
| }; |
Contributor
There was a problem hiding this comment.
Scenarios don't force named protocols via build_handshake
Medium Severity
build_handshake() estimates max_depth as 64 - entity_count.leading_zeros(), which yields ≥6 for any node with ≥36 entities. Since select_protocol Rule 4 fires when max_depth > 3, this means Scenario::force_bloom_filter() and Scenario::force_levelwise() both produce SubtreePrefetch instead of their namesake protocols when used with build_handshake(). The depth estimate overwhelms the intended shallow-tree structure these scenarios create. The compliance tests in negotiation.rs work around this by constructing handshakes directly with explicit depth values, but the named scenario helpers are broken for their stated purpose.
Additional Locations (2)
Contributor
|
Bugbot Autofix prepared fixes for 3 of the 3 bugs found in the latest run.
Or push these changes by commenting: Preview (6c9acb56cb)diff --git a/crates/node/tests/sync_compliance/negotiation.rs b/crates/node/tests/sync_compliance/negotiation.rs
--- a/crates/node/tests/sync_compliance/negotiation.rs
+++ b/crates/node/tests/sync_compliance/negotiation.rs
@@ -128,14 +128,12 @@
// Preconditions
assert!(hs_a.has_state);
assert!(hs_b.has_state);
- // Calculate divergence
- let max_count = hs_a.entity_count.max(hs_b.entity_count) as f64;
- let min_count = hs_a.entity_count.min(hs_b.entity_count) as f64;
- let divergence = if max_count > 0.0 {
- 1.0 - (min_count / max_count)
- } else {
- 0.0
- };
+ // Calculate divergence using the same formula as calculate_divergence():
+ // abs_diff / remote.entity_count.max(1)
+ // Here hs_b is treated as remote for symmetry with the select_protocol call.
+ let diff = hs_a.entity_count.abs_diff(hs_b.entity_count) as f64;
+ let denominator = hs_b.entity_count.max(1) as f64;
+ let divergence = diff / denominator;
assert!(
divergence > 0.5,
"Precondition: divergence > 50%, got {:.2}%",
@@ -189,14 +187,12 @@
"Precondition: max_depth > 3, got {}",
hs_b.max_depth
);
- // divergence < 20%
- let max_count = hs_a.entity_count.max(hs_b.entity_count) as f64;
- let min_count = hs_a.entity_count.min(hs_b.entity_count) as f64;
- let divergence = if max_count > 0.0 {
- 1.0 - (min_count / max_count)
- } else {
- 0.0
- };
+ // divergence < 20% using the same formula as calculate_divergence():
+ // abs_diff / remote.entity_count.max(1)
+ // Here hs_b is treated as remote for symmetry with the select_protocol call.
+ let diff = hs_a.entity_count.abs_diff(hs_b.entity_count) as f64;
+ let denominator = hs_b.entity_count.max(1) as f64;
+ let divergence = diff / denominator;
assert!(
divergence < 0.2,
"Precondition: divergence < 20%, got {:.2}%",
@@ -429,9 +425,10 @@
SyncProtocol::HashComparison { .. }
) {
// Only count as R3 if divergence > 50%
- let max = hs_a.entity_count.max(hs_b.entity_count) as f64;
- let min = hs_a.entity_count.min(hs_b.entity_count) as f64;
- let div = 1.0 - (min / max);
+ // Use the same formula as calculate_divergence(): abs_diff / remote.max(1)
+ let diff = hs_a.entity_count.abs_diff(hs_b.entity_count) as f64;
+ let denominator = hs_b.entity_count.max(1) as f64;
+ let div = diff / denominator;
if div > 0.5 {
rules_hit[2] = true;
}
diff --git a/crates/node/tests/sync_scenarios/snapshot_merge_protection.rs b/crates/node/tests/sync_scenarios/snapshot_merge_protection.rs
--- a/crates/node/tests/sync_scenarios/snapshot_merge_protection.rs
+++ b/crates/node/tests/sync_scenarios/snapshot_merge_protection.rs
@@ -250,21 +250,28 @@
}
/// Protection holds across 100 random seeds.
+///
+/// Uses `RandomScenario::two_nodes_random(seed)` to generate varied node
+/// configurations for each seed, ensuring actual coverage of different
+/// entity counts, tree depths, and divergence levels.
#[test]
fn test_protection_holds_across_random_seeds() {
for seed in 0..100 {
let mut rt = SimRuntime::new(seed);
- // Create two diverged nodes (both have state)
- let nodes = Scenario::n_nodes_diverged(2);
- let a = rt.add_existing_node(nodes.into_iter().next().unwrap());
- let b_node = Scenario::n_nodes_diverged(2).into_iter().nth(1).unwrap();
+ // Create two diverged nodes using seed-based random generation.
+ // Each seed produces different entity counts, data, and tree structure.
+ let mut nodes = RandomScenario::two_nodes_random(seed);
+ let b_node = nodes.pop().unwrap();
+ let a_node = nodes.pop().unwrap();
+
+ let a = rt.add_existing_node(a_node);
let b = rt.add_existing_node(b_node);
let hs_a = rt.node_mut(&a).unwrap().build_handshake();
let hs_b = rt.node_mut(&b).unwrap().build_handshake();
- // Both should have state
+ // Both should have state (RandomScenario default has fresh_node_probability=0.0)
if !hs_a.has_state || !hs_b.has_state {
continue;
}
diff --git a/crates/node/tests/sync_sim/node/state.rs b/crates/node/tests/sync_sim/node/state.rs
--- a/crates/node/tests/sync_sim/node/state.rs
+++ b/crates/node/tests/sync_sim/node/state.rs
@@ -100,6 +100,9 @@
pub has_state: bool,
/// Whether node is currently crashed (offline).
pub is_crashed: bool,
+ /// Optional override for max_depth in handshake.
+ /// Used by scenarios that need to control the depth independently of entity count.
+ pub handshake_depth_override: Option<u32>,
}
impl SimNode {
@@ -120,6 +123,7 @@
sender_sessions: HashMap::new(),
has_state: false,
is_crashed: false,
+ handshake_depth_override: None,
}
}
@@ -325,21 +329,35 @@
/// Build a SyncHandshake for this node.
///
/// Used for protocol negotiation testing.
+ ///
+ /// If `handshake_depth_override` is set, uses that value for max_depth.
+ /// Otherwise estimates max_depth from entity count (log2 approximation).
pub fn build_handshake(&mut self) -> SyncHandshake {
let root_hash = self.root_hash();
let entity_count = self.entity_count() as u64;
- // Estimate max_depth from entity count (log2-ish for balanced tree)
- let max_depth = if entity_count == 0 {
- 0
- } else {
- (64 - entity_count.leading_zeros()).min(32)
- };
+ // Use override if set, otherwise estimate max_depth from entity count
+ let max_depth = self.handshake_depth_override.unwrap_or_else(|| {
+ if entity_count == 0 {
+ 0
+ } else {
+ (64 - entity_count.leading_zeros()).min(32)
+ }
+ });
let dag_heads: Vec<[u8; 32]> = self.dag_heads.iter().map(|d| d.0).collect();
SyncHandshake::new(root_hash, entity_count, max_depth, dag_heads)
}
+
+ /// Set the max_depth override for handshake generation.
+ ///
+ /// This allows scenarios to control the reported tree depth independently
+ /// of the actual entity count, which is necessary for testing specific
+ /// protocol selection rules.
+ pub fn set_handshake_depth(&mut self, depth: u32) {
+ self.handshake_depth_override = Some(depth);
+ }
}
#[cfg(test)]
diff --git a/crates/node/tests/sync_sim/scenarios/deterministic.rs b/crates/node/tests/sync_sim/scenarios/deterministic.rs
--- a/crates/node/tests/sync_sim/scenarios/deterministic.rs
+++ b/crates/node/tests/sync_sim/scenarios/deterministic.rs
@@ -179,6 +179,9 @@
/// Rule 5: Large tree + small diff → BloomFilter.
///
/// Creates nodes with mostly shared state and small difference.
+ ///
+ /// Requirements for Rule 5: `entity_count > 50 AND divergence < 10% AND NOT (max_depth > 3)`
+ /// Since Rule 4 (SubtreePrefetch) fires when `max_depth > 3`, we must set depth ≤ 3.
pub fn force_bloom_filter() -> (SimNode, SimNode) {
let mut a = SimNode::new("a");
let mut b = SimNode::new("b");
@@ -195,6 +198,10 @@
b.insert_entity_with_metadata(id, data, metadata);
}
+ // Override depth to 3 to avoid triggering Rule 4 (SubtreePrefetch requires depth > 3)
+ a.set_handshake_depth(3);
+ b.set_handshake_depth(3);
+
assert!(b.entity_count() > 50);
(a, b)
}
@@ -202,6 +209,9 @@
/// Rule 6: Wide shallow tree → LevelWise.
///
/// Creates shallow tree structures.
+ ///
+ /// Requirements for Rule 6: `max_depth 1-2 AND avg_children/level > 10`
+ /// We must override depth to 2 to ensure Rule 4 (SubtreePrefetch) doesn't fire first.
pub fn force_levelwise() -> (SimNode, SimNode) {
let mut a = SimNode::new("a");
let mut b = SimNode::new("b");
@@ -214,6 +224,11 @@
b.insert_entity_with_metadata(id, data, metadata);
}
+ // Override depth to 2 to avoid triggering Rule 4 (SubtreePrefetch requires depth > 3)
+ // and to match Rule 6 requirements (max_depth 1-2)
+ a.set_handshake_depth(2);
+ b.set_handshake_depth(2);
+
(a, b)
} |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Adds comprehensive test coverage for sync protocol using the existing simulation framework:
build_handshake()helper toSimNodefor protocol negotiation testingWhat's Tested
1. Snapshot Merge Protection (
snapshot_merge_protection.rs)Tests for Invariant I5: No Silent Data Loss - verifies:
2. Protocol Negotiation Compliance (
negotiation.rs)Tests for CIP §2.3 protocol selection decision table:
Test Plan
cargo test --package calimero-node --test sync_sim sync_scenarios::(12 tests pass)cargo test --package calimero-node --test sync_sim sync_compliance::(16 tests pass)cargo fmt- no formatting issuesRelated Issues
Relates to #1783 (Snapshot Merge Protection)
Relates to #1785 (Compliance Test Suite)