test: display statistics for the generation of data for the simulation#517
Conversation
WalkthroughReplaces Ratio-based header-tree generation with GeneratedTree/GeneratedActions, threads them through benches and simulator generation, removes Ratio config fields, exposes the tree module, adds Display impls for several types, tightens Msg bounds, and updates simulate/run signatures and related tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor Bench/Test
participant DataGen as DataGeneration
participant GT as GeneratedTree
participant GA as GeneratedActions
participant Executor as Executor
Bench/Test->>DataGen: generate_tree_of_headers(depth, seed)
DataGen-->>GT: GeneratedTree (tree, depth, metadata)
Bench/Test->>DataGen: generate_random_walks(GT, peers_nb)
DataGen-->>GA: GeneratedActions (actions_per_peer, actions, generated_tree)
Bench/Test->>GA: call .actions() / .statistics()
Bench/Test->>Executor: execute_actions_on_tree(store, headers_tree, actions, flags)
Executor-->>Bench/Test: results / assertions
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
etorreborre
force-pushed
the
etorreborre/test/simulation-data-statistics
branch
2 times, most recently
from
8f76061 to
8c3b402
Compare
etorreborre
force-pushed
the
etorreborre/test/simulation-data-statistics
branch
2 times, most recently
from
a6807ac to
0737699
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (19)
simulation/amaru-sim/src/bin/amaru-sim/main.rs (1)
28-29: Nice inline guidance, but consider cross-referencing this in project docs.The comment's spot-on—it directly addresses the deep recursion path introduced by the new GeneratedTree generation flow. The exact env var and value (16MB) are super helpful. However, if a user hits a stack overflow at runtime, they might not stumble across this comment in the source. You might want to also mention this in a README or troubleshooting guide so folks can find it faster without needing to dig into the codebase.
crates/pure-stage/src/time.rs (1)
66-70: Consider simplifying the Display implementation, mate.Right now you're doing a two-step dance with
format!thenwrite_str. Could streamline this like a well-oiled espresso machine:impl Display for Instant { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - f.write_str(&format!("{:?}", self.saturating_since(*EPOCH))) + write!(f, "{:?}", self.saturating_since(*EPOCH)) } }This avoids the intermediate string allocation – not a biggie for Display, but every little bit helps when you're logging heaps of timestamps!
simulation/amaru-sim/src/simulator/world/world.rs (3)
82-99: Scope down the Display bound on WorldRequiring Display for all methods makes World harder to reuse. Only the logging path needs it. Split the impl or add a where bound on the specific method.
Apply this diff:
-impl<Msg: PartialEq + Clone + Debug + Display> World<Msg> { +impl<Msg: PartialEq + Clone + Debug> World<Msg> { @@ - fn step_world(&mut self) -> Next { + fn step_world(&mut self) -> Next + where + Msg: Display, + {
135-139: Include count and avoid building big strings unless neededNice touch logging only when non-empty. Consider adding a count to avoid huge lines, and keep the pretty list for debug level.
Example:
- if !outgoing.is_empty() { - let outgoing_to_string = format!("[{}]", outgoing.list_to_string(", ")); - info!(outgoing = %outgoing_to_string, "outgoing"); - } + if !outgoing.is_empty() { + info!(outgoing_count = outgoing.len(), "outgoing"); + debug!(outgoing = %format!("[{}]", outgoing.list_to_string(", ")), "outgoing"); + }
104-104: Tiny grammar nit in log“One step at a time.”
- info!("Run the simulation, one step at the time"); + info!("Run the simulation, one step at a time");simulation/amaru-sim/src/simulator/data_generation/generate.rs (1)
57-59: Pre-allocate and unify Bytes construction (minor polish)A couple of small wins for clarity and a smidge of perf.
- let mut messages = Vec::new(); + let mut messages = Vec::with_capacity(generated_actions.len()); @@ - Action::RollForward { header, .. } => ChainSyncMessage::Fwd { + Action::RollForward { header, .. } => ChainSyncMessage::Fwd { msg_id, slot: Slot::from(header.slot()), - hash: Bytes { - bytes: header.hash().to_vec(), - }, - header: Bytes { - bytes: to_cbor(&header), - }, + hash: Bytes::from(header.hash().to_vec()), + header: Bytes::from(to_cbor(&header)), },Optionally assert alignment between arrivals and actions during debug builds:
debug_assert_eq!(arrival_times.len(), generated_actions.len());Also applies to: 61-83
crates/amaru-consensus/benches/headers_tree.rs (2)
97-100: Pass a slice to avoid type coercionNot a bug, just clearer intent for the reader.
- let results = execute_actions_on_tree(store, &mut headers_tree, actions, false).unwrap(); + let results = execute_actions_on_tree(store, &mut headers_tree, &actions, false).unwrap();
128-129: Consider gating the benchmark threshold with an environment variable for CI environmentsThe 1000 microsecond assertion at line 128 is currently hardcoded, but right now you're running in CI (CI=true), which can be a right mare for benchmarks—what with RocksDB spins, CPU scaling, and all the build noise thrashing the machine. Like trying to maintain a perfect frame rate during a Cyberpunk raid, mate.
The review suggestion to gate this with an env var or relax the bound is spot on. No existing mechanism exists in the codebase to adjust thresholds for CI conditions, so consider wrapping the assertion with an environment variable check (e.g.,
AMARU_BENCH_CI_THRESHOLD) or relaxing the bound for CI runs to avoid spurious failures.simulation/amaru-sim/src/simulator/simulate.rs (4)
62-66: Relax Msg: Display bound (once World is narrowed)This bound exists to satisfy logging. After narrowing World’s bound, you can drop Display here to keep simulate generic.
See companion comment in world.rs.
85-112: Also print test stats on failuresPR goal is “stats after each test run”. On failure we return early without printing. Print before building the failure message.
match result { (history, Err(reason)) => { + // Emit per-test stats on failure too + display_test_stats(&generation_context); let failure_message = if simulate_config.disable_shrinking {
68-75: Trim the empty info! linesTiny signal-to-noise tweak.
- info!(""); info!( @@ - info!("");Also applies to: 115-120
166-172: Consider logging number_of_nodes tooIt’s handy context in the final summary.
fn display_test_configuration(simulate_config: &SimulateConfig, node_config: &NodeConfig) { info!("Number of tests: {}", simulate_config.number_of_tests); + info!("Number of nodes: {}", simulate_config.number_of_nodes); info!( "Number of upstream peers: {}", node_config.number_of_upstream_peers ); }crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (2)
36-44: Docs: fix type name and return descriptionSays “GeneraterTree” and suggests it returns the wrapper, but it returns Tree.
-/// Return a `proptest` Strategy producing a random `GeneraterTree` of a given depth. +/// Return a `proptest` Strategy producing a random `Tree<BlockHeader>` of a given depth. @@ -/// Return a `proptest` Strategy producing a random `GeneraterTree` of a given depth. +/// Return a `proptest` Strategy producing a random `GeneratedTree` of a given depth.
81-94: Remove the unused spine plumbingInserting root into spine doesn’t affect the built tree and the local spine isn’t used thereafter.
- let mut root_tree = Tree::make_leaf(&root); - let mut spine = generate_header_subtree(&mut rng, &mut root_tree, depth, depth - 1); - spine.insert(0, root); + let mut root_tree = Tree::make_leaf(&root); + generate_header_subtree(&mut rng, &mut root_tree, depth, depth - 1);simulation/amaru-sim/src/simulator/run.rs (2)
296-302: Add JSON toggle for stats output (env-driven).Handy logs; consider honoring AMARU_SIMULATION_LOG_AS_JSON here to emit one JSON blob, otherwise lines.
fn display_entries_statistics(generated_actions: &GeneratedActions) { - let statistics = generated_actions.statistics(); - for statistic in statistics.lines() { - info!("{}", statistic); - } + let statistics = generated_actions.statistics(); + if std::env::var_os("AMARU_SIMULATION_LOG_AS_JSON").is_some() { + let payload = serde_json::json!({ + "simulation": "entries_statistics", + "tree_depth": statistics.lines().get(0), + "number_of_nodes": statistics.lines().get(1), + "number_of_forks": statistics.lines().get(2), + }); + info!("{}", payload); + } else { + for statistic in statistics.lines() { + info!("{}", statistic); + } + } }You’ll need serde_json in amaru-sim Cargo.toml for this. Want me to wire it?
305-340: Returning Result instead of panicking: nice save.Turning the missing rollback position into an Err is the right call; less “game over” panics during sims. Two nits:
- Prefer explicit return Err(...) over Err(...)? to reduce reader head-scratching.
- Consider parameterising the chosen downstream peer ("c1") to avoid string coupling.
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (3)
272-279: Shuffle children instead of “random sort key”.sort_by_key(|_| rng.random_bool(0.5)) only yields two buckets and a biased/stable order; use proper Fisher–Yates via SliceRandom. Cleaner and truly shuffled, like re-rolling a loot table.
-use rand::prelude::SmallRng; +use rand::prelude::SmallRng; +use rand::seq::SliceRandom; @@ - let mut children: Vec<_> = tree.children.clone().into_iter().collect(); - children.sort_by_key(|_c| peer_rng.random_bool(0.5)); + let mut children: Vec<_> = tree.children.clone().into_iter().collect(); + children.shuffle(peer_rng);
318-326: Peer-duplication rule implemented cleanly.Cloning peer 1’s actions into peer 2 when peers_nb > 2 satisfies the “one peer duplicates another” property the PR promises. Consider pre-allocating duplicate_actions with capacity for a tiny perf win.
365-376: Return slices instead of &Vec to reduce coupling.Prefer &[Action] for actions() to avoid leaking the concrete collection type. Same for any similar getters elsewhere.
- pub fn actions(&self) -> &Vec<Action> { - &self.actions - } + pub fn actions(&self) -> &[Action] { + &self.actions + }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
crates/amaru-consensus/benches/headers_tree.rs(4 hunks)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs(9 hunks)crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs(4 hunks)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/tree.rs(4 hunks)crates/pure-stage/src/time.rs(2 hunks)simulation/amaru-sim/src/bin/amaru-sim/main.rs(1 hunks)simulation/amaru-sim/src/echo/message.rs(3 hunks)simulation/amaru-sim/src/simulator/args.rs(0 hunks)simulation/amaru-sim/src/simulator/data_generation/generate.rs(2 hunks)simulation/amaru-sim/src/simulator/node_config.rs(0 hunks)simulation/amaru-sim/src/simulator/run.rs(7 hunks)simulation/amaru-sim/src/simulator/simulate.rs(9 hunks)simulation/amaru-sim/src/simulator/world/node_handle.rs(2 hunks)simulation/amaru-sim/src/simulator/world/world.rs(4 hunks)simulation/amaru-sim/src/sync/chain_sync_message.rs(2 hunks)simulation/amaru-sim/tests/simulation.rs(1 hunks)simulation/amaru-sim/tests/traces.rs(1 hunks)
💤 Files with no reviewable changes (2)
- simulation/amaru-sim/src/simulator/args.rs
- simulation/amaru-sim/src/simulator/node_config.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-14T16:31:53.134Z
Learnt from: rkuhn
PR: pragma-org/amaru#263
File: simulation/amaru-sim/src/simulator/simulate.rs:298-300
Timestamp: 2025-06-14T16:31:53.134Z
Learning: StageRef in the pure-stage crate supports serde serialization and deserialization (derives serde::Serialize and serde::Deserialize), enabling it to be used in structs that also derive these traits for TraceBuffer and replay functionality.
Applied to files:
simulation/amaru-sim/src/simulator/simulate.rs
📚 Learning: 2025-05-12T14:21:27.470Z
Learnt from: stevana
PR: pragma-org/amaru#210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in `simulation/amaru-sim/src/simulator/simulate.rs` with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (`../../target/debug/echo`) and making tests more reliable.
Applied to files:
simulation/amaru-sim/src/simulator/simulate.rs
📚 Learning: 2025-08-23T15:44:44.318Z
Learnt from: rkuhn
PR: pragma-org/amaru#392
File: crates/ouroboros-traits/src/is_header/fake.rs:52-88
Timestamp: 2025-08-23T15:44:44.318Z
Learning: In the Amaru project, FakeHeader in crates/ouroboros-traits/src/is_header/fake.rs is specifically designed for testing purposes, and panic behavior (like copy_from_slice() panicking on malformed hex) is the preferred approach rather than graceful error handling, as it helps identify test data issues quickly during development.
Applied to files:
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs
🧬 Code graph analysis (14)
crates/amaru-consensus/src/consensus/headers_tree/mod.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (1)
tree(56-58)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (6)
any_select_chains(416-419)actions(365-367)actions(696-699)execute_actions(432-440)make_best_chains_from_results(590-619)make_best_chains_from_actions(549-582)
simulation/amaru-sim/src/simulator/world/world.rs (2)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (4)
fmt(117-122)fmt(126-128)fmt(132-134)format(139-172)simulation/amaru-sim/src/echo/message.rs (2)
fmt(27-29)fmt(65-98)
simulation/amaru-sim/src/simulator/world/node_handle.rs (3)
simulation/amaru-sim/src/echo/message.rs (2)
fmt(27-29)fmt(65-98)simulation/amaru-sim/src/simulator/world/world.rs (1)
fmt(74-79)simulation/amaru-sim/src/sync/chain_sync_message.rs (2)
fmt(79-132)fmt(136-172)
crates/amaru-consensus/benches/headers_tree.rs (2)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (4)
generate_tree_of_headers(83-94)depth(64-66)tree(56-58)nodes(68-70)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (7)
execute_actions_on_tree(449-494)generate_random_walks(302-346)generate_random_walks(675-712)generated_tree(357-359)len(369-371)actions(365-367)actions(696-699)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (3)
crates/amaru-consensus/src/consensus/headers_tree/tree.rs (5)
depth(138-140)nodes(148-157)fork_nodes(159-171)leaves(174-183)make_leaf(53-61)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (2)
generated_tree(357-359)best_chains(361-363)crates/amaru-ouroboros-traits/src/is_header/mod.rs (1)
header_body(133-135)
simulation/amaru-sim/src/echo/message.rs (2)
simulation/amaru-sim/src/sync/chain_sync_message.rs (2)
fmt(79-132)fmt(136-172)simulation/amaru-sim/src/echo/service.rs (1)
echo(73-88)
simulation/amaru-sim/src/simulator/data_generation/generate.rs (4)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (3)
any_select_chains_from_tree(422-427)peer(214-219)generated_tree(357-359)crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (1)
any_tree_of_headers(42-44)crates/amaru-ouroboros-traits/src/is_header/tests.rs (1)
run_with_rng(128-135)simulation/amaru-sim/src/simulator/run.rs (2)
generated_actions(264-268)new(367-378)
simulation/amaru-sim/src/simulator/run.rs (5)
simulation/amaru-sim/src/simulator/data_generation/generate.rs (1)
generate_entries(38-85)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (5)
peers(73-75)new(64-69)new(200-209)best_chains(401-416)best_chain(527-529)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (3)
generated_tree(357-359)best_chains(361-363)statistics(377-384)crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (2)
best_chains(60-62)tree(56-58)simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
header_hash(69-75)
simulation/amaru-sim/src/simulator/simulate.rs (4)
simulation/amaru-sim/src/simulator/run.rs (2)
node_config(210-212)new(367-378)simulation/amaru-sim/src/simulator/node_config.rs (1)
default(26-32)simulation/amaru-sim/src/simulator/simulate_config.rs (1)
default(28-35)simulation/amaru-sim/src/echo/simulate.rs (3)
spawn_echo_node(31-69)echo_generator(72-92)echo_property(97-123)
simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
simulation/amaru-sim/src/echo/message.rs (2)
fmt(27-29)fmt(65-98)
crates/amaru-consensus/src/consensus/headers_tree/tree.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (5)
fork_nodes(72-74)any_headers_tree(37-39)generate_headers_chain(152-155)generate_headers_tree(97-99)tree(56-58)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (3)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (5)
any_tree_of_headers(42-44)tree(56-58)best_chains(60-62)fork_nodes(72-74)depth(64-66)crates/amaru-ouroboros-traits/src/is_header/mod.rs (5)
hash(36-36)hash(110-112)hash(178-180)hash(200-202)header(129-131)crates/amaru-kernel/src/point.rs (1)
hash(37-43)
simulation/amaru-sim/tests/traces.rs (1)
simulation/amaru-sim/src/simulator/node_config.rs (1)
default(26-32)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build x86_64/windows
- GitHub Check: Build x86_64/linux
- GitHub Check: Build aarch64/linux
- GitHub Check: Examples (ledger-in-nodejs)
- GitHub Check: clippy
- GitHub Check: Benches
- GitHub Check: Build aarch64/macos
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Test coverage
🔇 Additional comments (23)
simulation/amaru-sim/src/simulator/world/node_handle.rs (3)
20-20: Righto, imports looking spot on!The addition of
Displayto thestd::fmtimports and switching todebugin the tracing imports are bang-on for the changes below. Perfectly aligned with the new trait bound and logging level, mate!Also applies to: 24-24
77-77: Just checking – debug level intentional, mate?You've downgraded the enqueuing log from
info!todebug!, which means it'll only show up when debug logging is cranked on. Given this is simulation code and could get pretty chatty with all those messages flying about, this might be a ripper way to reduce noise.But worth double-checking this aligns with your debugging needs – you won't see these in standard logs anymore, which could make tracking down enqueuing issues a bit trickier if they pop up.
68-74: Breaking change verified as safe – all message types already implement Display.The
Displaytrait bound added toMsginfrom_pure_stageis a breaking change in principle, but the codebase is already sorted. BothChainSyncMessage(used in main simulator and tests) andEchoMessage(used in echo simulation) already implementDisplay, so no compilation errors will occur at any call site.simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
135-173: Solid Display implementation, champion!Nice work keeping this concise and readable. The hex-encoded hash truncation matches the Debug impl pattern, and you've properly handled all variants including that explicit
header: _ignore on line 156. Clean as a whistle.simulation/amaru-sim/src/echo/message.rs (1)
26-30: Nice generic Display impl!Clean work making Envelope displayable for any T: Display. The format follows a sensible pattern.
crates/amaru-consensus/src/consensus/headers_tree/tree.rs (2)
159-171: Fork detection looks ace, cobber!The logic here checks children to see if they have multiple children (the actual fork points), then recursively descends. This makes sense for finding where the tree branches – you're identifying nodes that are parents of divergent paths. Solid recursive pattern.
185-203: Branches method is mint!Recursively building all root-to-leaf paths by prepending the current value and aggregating children's branches. Classic tree traversal pattern executed beautifully. For a leaf, returns just itself wrapped in a vec. Chef's kiss! 👌
crates/amaru-consensus/src/consensus/headers_tree/mod.rs (1)
21-21: Good move making tree public, legend!This exposes the tree module as part of your public API, which aligns beautifully with the new GeneratedTree/GeneratedActions workflow throughout the PR. Simple but important visibility change.
simulation/amaru-sim/tests/simulation.rs (1)
49-61: Conditional JSON logging setup looks proper!Nice addition of the
AMARU_SIMULATION_LOG_AS_JSONflag for toggling between human-readable and JSON log formats. The conditional init pattern is clean and straightforward – either go full JSON or keep it standard. This'll make automated log parsing much easier when needed.crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
257-295: Test migration to GeneratedActions looks bonzer!You've cleanly refactored the property test to use the new
GeneratedActionsAPI – removed the oldRatioconstants, renamed variables for clarity (generated_actions), and properly extracted actions via the.actions()accessor. The test flow stays intact while moving to the new abstraction. Smooth as Vegemite on toast!simulation/amaru-sim/tests/traces.rs (1)
29-37: Test config updates align perfectly!Removed the Ratio-based config fields and updated the simulate call to match the new GeneratedActions workflow. The simplified closure signatures (lines 65-66) reflect the cleaner API. Everything's consistent with the broader refactor across the codebase.
Also applies to: 62-66
simulation/amaru-sim/src/simulator/world/world.rs (1)
129-131: The review comment is based on incorrect assumptions—Display is already implemented.The
Envelope<T>struct has a Display implementation with the boundimpl<T: Display> Display for Envelope<T>, so the code at line 130 using%envelopewill compile without issue. The generic parameterMsgis constrained by the logging context itself.The
.to_string()call onarrival_timeis technically unnecessary—sinceInstanthas Display implemented—but it's harmless and doesn't prevent compilation. If you want to tidy it up, dropping.to_string()and relying on%arrival_timeis a valid micro-optimization, but it's not a blocker.The code as written is ship-shape, mate.
Likely an incorrect or invalid review comment.
simulation/amaru-sim/src/simulator/run.rs (3)
30-31: Imports expansion looks right, mates.Pulling in Chain, GeneratedActions, and the string-utils helpers lines up with the new flow and nicer diagnostics. No nitpicks here.
Also applies to: 41-41
72-76: All simulate() call sites verified and properly updated.The review concern checks out clean as a whistle, mate! 🎯
I've traced through all four
simulate()calls across the codebase:
- run.rs (lines 73-83): Passes all 8 parameters including
display_entries_statistics✓- traces.rs test (lines 60-68): Passes all 8 parameters correctly ✓
- simulate.rs tests (lines 286-294, 316-324): Both pass all 8 parameters correctly ✓
Every call site matches the new function signature perfectly—no stragglers left behind. The contract is locked in like a speedrun with no skips needed. The changes are solid.
252-294: Verify ifVec<BlockHeader>equality is actually problematic in practice.The review suggests switching from struct equality to hash-only comparison for defensive reasons. Here's what I found:
Chain is defined as
Vec<BlockHeader>, and BlockHeader derives(PartialEq, Eq, Clone). The concern about decode paths is plausible since the code callsdecode_block_header()to reconstruct headers—decoded structures could theoretically differ from generated ones.However, I'd note that the same codebase already trusts hash equality for rollback logic:
h.hash() == header_hash, which suggests the hash field is considered stable and reliable.The proposed refactor—comparing sequences of hashes instead of full BlockHeader structs—is solid defensive programming. But without seeing actual failures or the BlockHeader decode/encode implementations, I can't confirm if it's necessary or if the derived PartialEq is actually broken.
Bottom line: The suggestion is sensible but speculative. Before committing to this refactor, verify whether your test suite (or production) has actually encountered equality mismatches across decode paths. If you're seeing flaky chain comparisons, this fix is spot-on. If not, it might be premature optimization—though it's not a bad call to be cautious here, mate.
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (8)
56-80: New Action helpers are tidy.hash(), set_peer(), and pretty_print() round out the ergonomics and logging. The unwrap in pretty_print is fine for test tooling.
281-292: Rollback de-dup guard looks good.Comparing the last action’s hash to the candidate rollback avoids redundant backtracks without mutating semantics. Nice touch.
335-346: Interleaving across peers is clear; GeneratedActions struct looks sound.Transpose → flatten keeps all peers moving in lockstep. GeneratedActions captures both the per-peer map and the flattened list, which is handy for consumers.
377-385: Stats API is neat and readable.statistics() + lines()/Display provide both structured and human-friendly outputs. Plays nicely with the new logs.
Also applies to: 387-410
416-427: Strategies: thumbs up.any_select_chains and any_select_chains_from_tree compose nicely with proptest Strategy. Tidy API.
514-516: Diagnostics: pretty_print JSON blocks are dev-gold.That raw-string JSON trick is perfect for copy-paste repro. Rock solid.
39-41: Dependency versions confirmed—all good to go, mate!You've locked down rand at 0.9.2, which is the right call for Rng::random_bool. The migration from gen_bool to random_bool is solid in that version. Proptest 1.8.0 is also properly configured with workspace inheritance, so your imports of Just and Strategy are firing on all cylinders. It's like assembling the perfect speedrun route—every dependency is where it needs to be, and there's no version clash drama waiting to ambush you. The code is shipshape.
673-713: Test validates fork count and peer duplication—good guardrails to prevent regression.The assertions checking for 3–4 fork nodes and ensuring at least 2 peers share identical action streams (post peer normalization) are solid safeguards. Property-based tests can occasionally be flaky if the random generation doesn't hit the expected distribution bounds, but that's the nature of the beast. If flakiness does rear its head on CI, swapping in a fixed seed strategy for this test is totally doable—cheers for flagging that!
etorreborre
force-pushed
the
etorreborre/test/simulation-data-statistics
branch
from
0737699 to
e5f6211
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/amaru-consensus/benches/headers_tree.rs (1)
104-109: Fix invalidif letcondition in bench (won’t compile)You can’t combine
if letwith&&. Nest it or use a tuple match.Apply one of these:
- if profile && let Some(report) = guard.and_then(|g| g.report().build().ok()) { - let file = File::create("headers-tree-flamegraph.svg").unwrap(); - report - .flamegraph_with_options(file, &mut Options::default()) - .unwrap(); - } + if profile { + if let Some(report) = guard.and_then(|g| g.report().build().ok()) { + let file = File::create("headers-tree-flamegraph.svg").unwrap(); + report + .flamegraph_with_options(file, &mut Options::default()) + .unwrap(); + } + }
♻️ Duplicate comments (2)
simulation/amaru-sim/src/simulator/run.rs (1)
213-218: Clamp max_length and accept slice for peers (prevent underflow; idiomatic API)
generated_chain_depth as usize - 1can underflow for 0/1; also param should be&[Peer]. Same issue was flagged earlier.-fn make_chain_selector( - chain_store: Arc<dyn ChainStore<BlockHeader>>, - generated_chain_depth: u64, - peers: &Vec<Peer>, -) -> SelectChain { - // Set the maximum length for the best fragment based on the generated chain depth - // so we generated roll forwards that move the best chain anchor. - let max_length = if generated_chain_depth > DEFAULT_MAXIMUM_FRAGMENT_LENGTH as u64 { - DEFAULT_MAXIMUM_FRAGMENT_LENGTH - } else { - generated_chain_depth as usize - 1 - }; +fn make_chain_selector( + chain_store: Arc<dyn ChainStore<BlockHeader>>, + generated_chain_depth: u64, + peers: &[Peer], +) -> SelectChain { + // Keep fragment length < depth where possible; clamp to [1, DEFAULT_MAXIMUM_FRAGMENT_LENGTH] + let depth = generated_chain_depth as usize; + let max_length = depth.saturating_sub(1).min(DEFAULT_MAXIMUM_FRAGMENT_LENGTH).max(1); @@ - for peer in peers { + for peer in peers.iter() { tree_state .initialize_peer(chain_store.clone(), peer, &anchor) .expect("the root node is guaranteed to already be in the tree") }Also update the call site:
- let select_chain = make_chain_selector( - chain_store.clone(), - node_config.generated_chain_depth, - &peers, - ); + let select_chain = + make_chain_selector(chain_store.clone(), node_config.generated_chain_depth, &peers);Also applies to: 229-241
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (1)
132-143: Usegen_bool(rand 0.9) instead ofrandom_bool
random_boolisn’t in rand 0.9; swap togen_bool. While here, consider guardingtotal_depth / 3for small depths (≤4) to avoid branching every step.- // We decide to branch at 1/3rd and 2/3rds of the total depth - let must_branch = if total_depth >= 3 { - current_size % (total_depth / 3) == 0 - } else { - false - }; + // Branch roughly around 1/3 and 2/3 of the spine + let step = (total_depth / 3).max(1); + let must_branch = total_depth >= 3 && current_size % step == 0; if must_branch { generate_header_subtree(rng, current, total_depth, current_depth - current_size); - if rng.random_bool(0.2) { + if rng.gen_bool(0.2) { generate_header_subtree(rng, current, total_depth, current_depth - current_size); } }Based on learnings.
🧹 Nitpick comments (14)
crates/pure-stage/src/time.rs (1)
66-70: Minor efficiency tweak, if you're keen.Right now you're doing the ol' double-format dance with
format!("{:?}", ...)insidefmt, which allocates an intermediate String. SinceDurationalready implementsDisplay, you could skip that allocation entirely.Like this, yeah:
impl Display for Instant { fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - f.write_str(&format!("{:?}", self.saturating_since(*EPOCH))) + write!(f, "{:?}", self.saturating_since(*EPOCH)) } }That said, if you wanna stick with the Debug formatting (
{:?}) for consistency with your other output, the current impl works fine—it's just for logging stats anyway, not exactly your hot loop. No dramas either way! 🦘crates/amaru-consensus/src/consensus/headers_tree/tree.rs (3)
159-172: fork_nodes: clarify whether the root can be a fork tooCurrent logic only considers children (skips self), so a fork at the root won’t be returned. If “fork nodes” means “any node with >1 children,” consider including self.
pub fn fork_nodes(&self) -> Vec<H> where H: Clone, { - let mut result = vec![]; - for child in &self.children { + let mut result = Vec::new(); + if self.children.len() > 1 { + result.push(self.value.clone()); + } + for child in &self.children { if child.children.len() > 1 { result.push(child.value.clone()); } result.extend(child.fork_nodes()) } result }Does test intent expect excluding or including the root when it forks? Happy to align either way.
185-204: branches: reads well; can be made a tad more idiomaticLogic is sound. If you fancy a leaner style, this avoids the mutable temp:
pub fn branches(&self) -> Vec<Vec<H>> where H: Clone, { if self.children.is_empty() { vec![vec![self.value.clone()]] } else { - let mut result = vec![]; - for child in &self.children { - for mut branch in child.branches() { - let mut new_branch = vec![self.value.clone()]; - new_branch.append(&mut branch); - result.push(new_branch); - } - } - result + self.children + .iter() + .flat_map(|child| { + child.branches().into_iter().map(|mut b| { + let mut new_b = Vec::with_capacity(b.len() + 1); + new_b.push(self.value.clone()); + new_b.extend(b); + new_b + }) + }) + .collect() } }
290-299: Snapshot-y pretty_print expectation — stable seed, but brittleUsing generate_headers_tree(4, 42) should be deterministic, but minor RNG or formatting shifts will break this wall of text. Consider asserting shape (depth/branching) and a few anchor hashes, then compare the full string behind a feature flag or guard.
- Keep a compact “golden” (select lines) and/or
- Compare PrettyPrint lines with starts_with/contains for key nodes.
If you’d like, I can refactor this test to assert structure without hardcoding every hash.
crates/amaru-consensus/src/consensus/headers_tree/mod.rs (1)
21-21: Publicly exposingtreemodule — double good; add a re-export for ergonomicsMaking
treepublic unblocks external consumers. To reduce path verbosity (and churn), consider re-exportingTreeat this module level.pub mod headers_tree; pub mod tree; -pub use headers_tree::*; +pub use headers_tree::*; +pub use tree::Tree;Any semver promises here? If downstreams were depending on private paths via test-utils, this is a benign upgrade, but worth a heads-up in the changelog.
simulation/amaru-sim/tests/simulation.rs (1)
49-61: JSON logging toggle: nice touch; guard init to avoid “global subscriber already set” panicsConditional json/init looks neat. In test land, calling init multiple times across tests will panic. Prefer try_init and ignore the AlreadySet error.
- if amaru_logs_as_json { - formatter.json().init(); - } else { - formatter.init(); - } + let res = if amaru_logs_as_json { + formatter.json().try_init() + } else { + formatter.try_init() + }; + let _ = res; // ignore AlreadySet in multi-test runsAlso, consider case-insensitive truthiness for the env:
-fn is_true(var_name: &str) -> bool { - env::var(var_name).is_ok_and(|v| v == "1" || v == "true") -} +fn is_true(var_name: &str) -> bool { + env::var(var_name) + .map(|v| v == "1" || v.eq_ignore_ascii_case("true")) + .unwrap_or(false) +}Do other tests set tracing too? If so, the guard above will save you from a surprise stack trace.
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
1290-1295: Switch to GeneratedActions in property test: solid; add failure persistence for regressionsThe actions() flow and expected_chains via make_best_chains_from_actions line up. To keep flaky failures reproducible in CI, enable proptest failure persistence (or allow env override) instead of disabling it globally.
-#![proptest_config(config_begin().no_shrink().with_cases(TEST_CASES_NB).end())] +#![proptest_config( + config_begin() + .no_shrink() + .with_cases(TEST_CASES_NB) + // persist failures locally; CI can override via PROPTEST_REGRESSIONS + .with_failure_persistence("proptest-regressions") + .end() +)]Based on learnings.
simulation/amaru-sim/src/simulator/world/world.rs (3)
104-104: Tiny copy edit“Run the simulation, one step at the time” → “one step at a time.”
- info!("Run the simulation, one step at the time"); + info!("Run the simulation, one step at a time");
129-131: Avoid allocations in logsarrival_time already implements Display; no need for to_string(). Use %arrival_time.
- info!(msg = %envelope, arrival = %arrival_time.to_string(), "stepping"); + info!(msg = %envelope, arrival = %arrival_time, "stepping");
135-139: Outgoing logs: structured and scalableNice gating on empty. If the list can get chunky, consider logging count + debug instead of materialising a big string.
- if !outgoing.is_empty() { - let outgoing_to_string = - format!("[{}]", outgoing.list_to_string(", ")); - info!(outgoing = %outgoing_to_string, "outgoing"); - } + if !outgoing.is_empty() { + info!(outgoing_count = outgoing.len(), outgoing = ?outgoing, "outgoing"); + }crates/amaru-consensus/benches/headers_tree.rs (1)
61-67: Avoid cloning all nodes twice in the assert
tree.nodes()clones headers; here it’s called twice. Usesize()if available or cache the len.- assert!( - tree.nodes().len() > 5000, - "there are {} nodes", - tree.nodes().len() - ); + let node_count = tree.size(); + assert!(node_count > 5000, "there are {} nodes", node_count);simulation/amaru-sim/src/simulator/simulate.rs (2)
69-76: Don’t print per-test stats twiceYou log stats right after generation and again after success. Pick one (prefer after success/failure) to keep the signal clean.
- info!("Test data generated, now sending messages"); - display_test_stats(&generation_context); + info!("Test data generated, now sending messages"); @@ - (_history, Ok(())) => { - display_test_stats(&generation_context); + (_history, Ok(())) => { + display_test_stats(&generation_context);Alternatively, print on both branches of failure/success only.
Also applies to: 113-120
166-172: Enrich final configuration summaryHandy to include downstream peers and generated depth (and seed) in the final “Success!” block.
fn display_test_configuration(simulate_config: &SimulateConfig, node_config: &NodeConfig) { info!("Number of tests: {}", simulate_config.number_of_tests); - info!( - "Number of upstream peers: {}", - node_config.number_of_upstream_peers - ); + info!("Number of upstream peers: {}", node_config.number_of_upstream_peers); + info!("Number of downstream peers: {}", node_config.number_of_downstream_peers); + info!("Generated chain depth: {}", node_config.generated_chain_depth); + info!("Seed: {}", simulate_config.seed); }crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
377-384: Stats: avoid cloning the whole tree just to count nodes
self.tree.nodes().len()clones every header. ExposeGeneratedTree::size()and use it here for O(1) sizing.In
GeneratedTree(data_generation.rs):impl GeneratedTree { + pub fn size(&self) -> usize { + self.tree.size() + } }Then update:
- number_of_nodes: self.tree.nodes().len(), + number_of_nodes: self.tree.size(),
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
crates/amaru-consensus/benches/headers_tree.rs(4 hunks)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs(9 hunks)crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs(4 hunks)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/tree.rs(4 hunks)crates/pure-stage/src/time.rs(2 hunks)simulation/amaru-sim/src/bin/amaru-sim/main.rs(1 hunks)simulation/amaru-sim/src/echo/message.rs(3 hunks)simulation/amaru-sim/src/simulator/args.rs(0 hunks)simulation/amaru-sim/src/simulator/data_generation/generate.rs(2 hunks)simulation/amaru-sim/src/simulator/node_config.rs(0 hunks)simulation/amaru-sim/src/simulator/run.rs(7 hunks)simulation/amaru-sim/src/simulator/simulate.rs(9 hunks)simulation/amaru-sim/src/simulator/world/node_handle.rs(2 hunks)simulation/amaru-sim/src/simulator/world/world.rs(4 hunks)simulation/amaru-sim/src/sync/chain_sync_message.rs(2 hunks)simulation/amaru-sim/tests/simulation.rs(1 hunks)simulation/amaru-sim/tests/traces.rs(1 hunks)
💤 Files with no reviewable changes (2)
- simulation/amaru-sim/src/simulator/node_config.rs
- simulation/amaru-sim/src/simulator/args.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- simulation/amaru-sim/src/bin/amaru-sim/main.rs
- simulation/amaru-sim/tests/traces.rs
- simulation/amaru-sim/src/echo/message.rs
- simulation/amaru-sim/src/simulator/data_generation/generate.rs
- simulation/amaru-sim/src/sync/chain_sync_message.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-06-14T16:31:53.134Z
Learnt from: rkuhn
PR: pragma-org/amaru#263
File: simulation/amaru-sim/src/simulator/simulate.rs:298-300
Timestamp: 2025-06-14T16:31:53.134Z
Learning: StageRef in the pure-stage crate supports serde serialization and deserialization (derives serde::Serialize and serde::Deserialize), enabling it to be used in structs that also derive these traits for TraceBuffer and replay functionality.
Applied to files:
simulation/amaru-sim/src/simulator/simulate.rs
📚 Learning: 2025-05-12T14:21:27.470Z
Learnt from: stevana
PR: pragma-org/amaru#210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in `simulation/amaru-sim/src/simulator/simulate.rs` with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (`../../target/debug/echo`) and making tests more reliable.
Applied to files:
simulation/amaru-sim/src/simulator/simulate.rs
📚 Learning: 2025-08-23T15:44:44.318Z
Learnt from: rkuhn
PR: pragma-org/amaru#392
File: crates/ouroboros-traits/src/is_header/fake.rs:52-88
Timestamp: 2025-08-23T15:44:44.318Z
Learning: In the Amaru project, FakeHeader in crates/ouroboros-traits/src/is_header/fake.rs is specifically designed for testing purposes, and panic behavior (like copy_from_slice() panicking on malformed hex) is the preferred approach rather than graceful error handling, as it helps identify test data issues quickly during development.
Applied to files:
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs
🧬 Code graph analysis (10)
crates/amaru-consensus/benches/headers_tree.rs (3)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (4)
generate_tree_of_headers(83-94)depth(64-66)tree(56-58)nodes(68-70)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (7)
execute_actions_on_tree(449-494)generate_random_walks(302-346)generate_random_walks(675-712)generated_tree(357-359)len(369-371)actions(365-367)actions(696-699)crates/amaru-consensus/src/consensus/headers_tree/tree.rs (2)
depth(138-140)nodes(148-157)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (3)
crates/amaru-consensus/src/consensus/headers_tree/tree.rs (5)
depth(138-140)nodes(148-157)fork_nodes(159-171)leaves(174-183)make_leaf(53-61)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (2)
generated_tree(357-359)best_chains(361-363)crates/amaru-ouroboros-traits/src/is_header/mod.rs (1)
header_body(133-135)
crates/amaru-consensus/src/consensus/headers_tree/tree.rs (2)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (5)
fork_nodes(72-74)any_headers_tree(37-39)generate_headers_chain(152-155)generate_headers_tree(97-99)tree(56-58)crates/amaru-consensus/src/consensus/headers_tree/data_generation/property_configuration.rs (1)
config_begin(55-64)
simulation/amaru-sim/src/simulator/world/node_handle.rs (2)
simulation/amaru-sim/src/echo/message.rs (2)
fmt(27-29)fmt(65-98)simulation/amaru-sim/src/sync/chain_sync_message.rs (2)
fmt(79-132)fmt(136-172)
simulation/amaru-sim/src/simulator/simulate.rs (3)
simulation/amaru-sim/src/simulator/run.rs (2)
node_config(210-212)new(367-378)simulation/amaru-sim/src/simulator/node_config.rs (1)
default(26-32)simulation/amaru-sim/src/simulator/simulate_config.rs (1)
default(28-35)
simulation/amaru-sim/src/simulator/world/world.rs (3)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (4)
fmt(117-122)fmt(126-128)fmt(132-134)format(139-172)crates/pure-stage/src/time.rs (2)
fmt(59-63)fmt(67-69)simulation/amaru-sim/src/echo/message.rs (2)
fmt(27-29)fmt(65-98)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (2)
simulation/amaru-sim/src/simulator/run.rs (1)
generated_actions(264-268)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (5)
any_select_chains(416-419)actions(365-367)actions(696-699)execute_actions(432-440)make_best_chains_from_actions(549-582)
simulation/amaru-sim/src/simulator/run.rs (4)
simulation/amaru-sim/src/simulator/data_generation/generate.rs (1)
generate_entries(38-85)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (6)
peers(73-75)new(64-69)new(200-209)best_chains(401-416)format(139-172)best_chain(527-529)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (3)
generated_tree(357-359)best_chains(361-363)statistics(377-384)simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
header_hash(69-75)
crates/amaru-consensus/src/consensus/headers_tree/mod.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (1)
tree(56-58)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (4)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (5)
any_tree_of_headers(42-44)tree(56-58)best_chains(60-62)fork_nodes(72-74)depth(64-66)crates/amaru-ouroboros-traits/src/is_header/mod.rs (5)
hash(36-36)hash(110-112)hash(178-180)hash(200-202)header(129-131)crates/amaru-kernel/src/point.rs (1)
hash(37-43)crates/amaru-kernel/src/string_utils.rs (2)
list_to_string(21-21)list_to_string(29-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (8)
- GitHub Check: Analyze (javascript-typescript)
- GitHub Check: Build x86_64/windows
- GitHub Check: Build x86_64/linux
- GitHub Check: Build aarch64/linux
- GitHub Check: Build riscv32
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Build aarch64/macos
- GitHub Check: Benches
🔇 Additional comments (12)
crates/pure-stage/src/time.rs (1)
15-15: Good stuff, mate!The import's exactly what ya need for the Display impl below. Clean and purposeful.
simulation/amaru-sim/src/simulator/world/node_handle.rs (3)
20-20: Ripper work on the import cleanup, mate!The imports are spot-on consistent with the usage below. Display is needed for the new trait bound, and swapping info for debug at the tracing level is a beaut move for operational logs. No dramas here!
Also applies to: 24-24
77-77: Strewth, debug level is the right call here!Moving this enqueuing log from info to debug is proper log hygiene. Message enqueueing is operational detail that'd clutter up your info logs faster than a greyhound on Red Bull. This way, you can crank up verbosity when debugging without drowning in messages during normal runs.
68-74: Nice! The Display bound is already baked in across the board.Ran the verification gauntlet and here's the sitch: both
ChainSyncMessageandEchoMessage(used in all three call sites) already have Display implementations sorted. This is like finding out your multiplayer buddy already has the DLC installed – no compatibility drama ahead!The two real-world call sites in
run.rsandtraces.rsboth pull fromspawn_node(), which returnsChainSyncMessage, and that's got Display locked down atsimulation/amaru-sim/src/sync/chain_sync_message.rs:135. The echo simulator usesEchoMessagewith its own Display impl atsimulation/amaru-sim/src/echo/message.rs:64. You're all set!While this is technically a breaking API change on paper, all in-tree usage already meets the requirement. The refactor toward multi-line trait bounds reads clean as well.
crates/amaru-consensus/src/consensus/headers_tree/tree.rs (1)
257-258: Test generators swap: LGTMSwitch to any_headers_tree and generate_headers_tree fits the new data-gen surface. No beef here.
Also applies to: 264-264
simulation/amaru-sim/src/simulator/world/world.rs (1)
18-18: ---All message types implement Display—the concern is addressed.
Your
Displaybound onMsg(line 82) is solid as a rock. BothChainSyncMessageandEchoMessage—the message types actually used in the sim—have Display impls locked in. The constraint itself acts as a gatekeeper, so any future message types will need to implement Display to even compile withWorld. It's like having a bouncer at the club who checks IDs—only the qualified get through.crates/amaru-consensus/benches/headers_tree.rs (1)
15-15: Imports update looks goodNew public API wired cleanly for tree and actions. Ship it.
Also applies to: 37-39
simulation/amaru-sim/src/simulator/run.rs (1)
296-302: Stats hook LGTMNeat little post-run stats printer; pairs well with the per-test output above.
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (3)
514-516: Diagnostics output is spot-onPretty JSON lines with
pretty_printmakes reproducing failures a breeze.
416-419: Proptest strategies LGTMNice composability: from tree → actions in one go, or actions from a given tree.
Also applies to: 421-427
255-260: Rollback de-dup guard looks goodSkipping redundant rollback-to-same-hash avoids noise in traces.
Also applies to: 281-293
simulation/amaru-sim/src/simulator/simulate.rs (1)
51-58: Display bound is sound across all call sites—no downstream breakage detected.G'day! I've done a quick recon through the codebase, and mate, you're absolutely golden here. All four
simulate()call sites are passing message types that already implementDisplay—it's like a perfect speedrun, yeah?The breakdown:
- traces.rs and run.rs use
ChainSyncMessage(Display impl ✓ atchain_sync_message.rs:135)- simulate.rs tests use
EchoMessage(Display impl ✓ atecho/message.rs:64)No one's gonna be caught holding the idiot ball with a missing trait bound. The change is locked in tight.
etorreborre
force-pushed
the
etorreborre/test/simulation-data-statistics
branch
from
e5f6211 to
1b0baaf
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (2)
simulation/amaru-sim/src/simulator/run.rs (1)
229-241: Fix underflow inmax_lengthand takepeers: &[Peer](duplicate of prior concern).If
generated_chain_depth <= 1,as usize - 1underflows/lands on 0, risking invalid state. Also, prefer slice over&Vec.-fn make_chain_selector( - chain_store: Arc<dyn ChainStore<BlockHeader>>, - generated_chain_depth: u64, - peers: &Vec<Peer>, -) -> SelectChain { +fn make_chain_selector( + chain_store: Arc<dyn ChainStore<BlockHeader>>, + generated_chain_depth: u64, + peers: &[Peer], +) -> SelectChain { @@ - let max_length = if generated_chain_depth > DEFAULT_MAXIMUM_FRAGMENT_LENGTH as u64 { - DEFAULT_MAXIMUM_FRAGMENT_LENGTH - } else { - generated_chain_depth as usize - 1 - }; + let depth = generated_chain_depth as usize; + let max_length = depth + .saturating_sub(1) // keep < depth + .min(DEFAULT_MAXIMUM_FRAGMENT_LENGTH) + .max(2); // HeadersTree wants >= 2And the caller keeps working as-is:
- let select_chain = make_chain_selector( - chain_store.clone(), - node_config.generated_chain_depth, - &peers, - ); + let select_chain = make_chain_selector( + chain_store.clone(), + node_config.generated_chain_depth, + &peers, + );crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
272-279: Random order broken: useshuffleinstead ofsort_by_key, and fix the deprecated rand API.Two issues here that were flagged in the previous review, mate:
- Critical:
random_boolis deprecated in rand 0.9—needs to begen_bool.- Major:
sort_by_keywith a random boolean only partitions the children into two groups; it doesn't produce a true random ordering. Useshufflefor a proper random walk order.Based on learnings.
Apply this diff to fix both issues:
- // Process the children in a random order based on a peer-specific RNG + // Process the children in a random order based on a peer-specific RNG let mut children: Vec<_> = tree.children.clone().into_iter().collect(); - children.sort_by_key(|_c| peer_rng.random_bool(0.5)); + use rand::seq::SliceRandom; + children.shuffle(peer_rng);If you absolutely need to keep the
sort_by_keyapproach for some reason, at minimum fix the API:- children.sort_by_key(|_c| peer_rng.random_bool(0.5)); + children.sort_by_key(|_c| peer_rng.gen_bool(0.5));But seriously,
shuffleis what you want here—like picking a random movie from your collection instead of just flipping a coin on each one.
🧹 Nitpick comments (8)
simulation/amaru-sim/src/simulator/world/world.rs (1)
129-129: No worries, but there's a bit of redundancy here like a Mario level with too many coins!The
%format specifier in tracing macros already invokesDisplay::fmt, which does the string conversion. So calling.to_string()explicitly is like ordering a lager and then asking for it to be wet – it's already sorted, mate!- info!(msg = %envelope, arrival = %arrival_time.to_string(), "stepping"); + info!(msg = %envelope, arrival = %arrival_time, "stepping");crates/amaru-consensus/src/consensus/headers_tree/tree.rs (2)
159-171: These methods could use a quick doc comment, legend.Both
fork_nodesandbranchesare now part of your test-utils API, but they're flying without doc comments. A one-liner explaining what they return (especially the nuances, like whetherfork_nodesincludes the root) would help future devs – or yourself in 6 months when you've forgotten the details!Apply this diff to add doc comments:
+ /// Returns all nodes in the tree that have more than one child (fork points). + /// Note: only checks descendant nodes, not the root itself. pub fn fork_nodes(&self) -> Vec<H> where H: Clone, { // ... implementation } + /// Returns all root-to-leaf paths through the tree. + /// Each path is a vector starting from the root and ending at a leaf. pub fn branches(&self) -> Vec<Vec<H>> where H: Clone, { // ... implementation }Also applies to: 185-203
159-171: Righto, mate – I've had a proper look at the tree generation logic, and here's the good news: the root exclusion is actually spot-on, not a bug at all.Here's the sitch: the generated tree structure creates a single spine branching from the root. The root only ever gets 1 child (the spine entry point), so it doesn't qualify as a fork node by definition. The actual fork points are the spine nodes at roughly 1/3rd and 2/3rds depth, where 1–2 additional subtrees get grafted on (lines 139–142 in data_generation.rs). That gives you 3–4 internal fork nodes total, which is exactly what the test expects.
So your implementation is firing on all cylinders here – it's just that the lack of doc comments makes the intent a bit harder to suss out. Both
fork_nodesandbranchescould use a quick docstring explaining that fork nodes are internal branching points (not including the root), yeah?Add doc comments to
fork_nodesandbranchesmethods (lines 159 and 185 respectively) to clarify they only track internal fork points and leaf branches.crates/amaru-consensus/benches/headers_tree.rs (2)
61-67: Make the bench assertions less brittle (prefer sanity checks over magic thresholds).Hard-coding >5000 nodes and >10000 actions can flake when generation heuristics change, turning a perf bench into a logic test. Suggest asserting minimal shape and logging the rest.
Apply:
- assert!( - tree.nodes().len() > 5000, - "there are {} nodes", - tree.nodes().len() - ); + // Sanity: non-trivial tree (at least a chain of `depth`) + assert!( + tree.nodes().len() >= depth, + "expected at least {depth} nodes, got {}", + tree.nodes().len() + ); @@ - assert!(actions.len() > 10000); + assert!( + !actions.is_empty(), + "generated no actions; check peers/depth inputs" + );Also applies to: 69-72
128-128: Avoid CI flakiness: gate the SLA assert or downgrade to a warning.Hard fails on slow runners will feel like Dark Souls on a dance pad. Keep the signal, not the tears.
Use an opt‑in env flag:
- assert!(time_per_action.as_micros() < 1000); + if std::env::var("BENCH_FAIL_ON_SLOW").as_deref() == Ok("1") { + assert!(time_per_action.as_micros() < 1_000, "time_per_action = {:?}", time_per_action); + } else if time_per_action.as_micros() >= 1_000 { + eprintln!("warning: time_per_action = {:?} (not failing; set BENCH_FAIL_ON_SLOW=1 to enforce)", time_per_action); + }simulation/amaru-sim/tests/traces.rs (2)
62-67: Pass the same NodeConfig you used to spawn/generate, not a fresh default.Keeping configs in sync avoids spooky action at a distance later (looking at you, BioShock plasmids).
- &NodeConfig::default(), + &node_config, spawn, generate_one, - |_, _| Ok(()), - |_| (), + |_, _| Ok(()), + |_| (),
48-55: Variable naming nit:best_chain→generated_actions.The generator now returns
GeneratedActions. Rename for readability.- let (entries, best_chain) = generate_entries( + let (entries, generated_actions) = generate_entries( @@ - (entries.into_iter().take(1).collect(), best_chain) + (entries.into_iter().take(1).collect(), generated_actions)simulation/amaru-sim/src/simulator/data_generation/generate.rs (1)
61-63: Minor: derivemsg_idvia enumerate; avoid zipping an open range.Keeps it tidy and obvious—like choosing the right kart in Mario Kart.
- for (msg_id, (action, arrival_time)) in - (0_u64..).zip(generated_actions.actions().iter().zip(arrival_times.iter())) - { + for (idx, (action, arrival_time)) in + generated_actions.actions().iter().zip(arrival_times.iter()).enumerate() + { + let msg_id = idx as u64;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
crates/amaru-consensus/benches/headers_tree.rs(4 hunks)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs(9 hunks)crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs(4 hunks)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/tree.rs(4 hunks)crates/pure-stage/src/time.rs(2 hunks)simulation/amaru-sim/src/bin/amaru-sim/main.rs(1 hunks)simulation/amaru-sim/src/echo/message.rs(3 hunks)simulation/amaru-sim/src/simulator/args.rs(0 hunks)simulation/amaru-sim/src/simulator/data_generation/generate.rs(2 hunks)simulation/amaru-sim/src/simulator/node_config.rs(0 hunks)simulation/amaru-sim/src/simulator/run.rs(7 hunks)simulation/amaru-sim/src/simulator/simulate.rs(9 hunks)simulation/amaru-sim/src/simulator/world/node_handle.rs(2 hunks)simulation/amaru-sim/src/simulator/world/world.rs(4 hunks)simulation/amaru-sim/src/sync/chain_sync_message.rs(2 hunks)simulation/amaru-sim/tests/simulation.rs(1 hunks)simulation/amaru-sim/tests/traces.rs(1 hunks)
💤 Files with no reviewable changes (2)
- simulation/amaru-sim/src/simulator/args.rs
- simulation/amaru-sim/src/simulator/node_config.rs
🚧 Files skipped from review as they are similar to previous changes (8)
- crates/amaru-consensus/src/consensus/headers_tree/mod.rs
- crates/pure-stage/src/time.rs
- simulation/amaru-sim/tests/simulation.rs
- simulation/amaru-sim/src/simulator/world/node_handle.rs
- simulation/amaru-sim/src/bin/amaru-sim/main.rs
- simulation/amaru-sim/src/sync/chain_sync_message.rs
- crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs
- simulation/amaru-sim/src/echo/message.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-12T14:21:27.470Z
Learnt from: stevana
PR: pragma-org/amaru#210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in `simulation/amaru-sim/src/simulator/simulate.rs` with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (`../../target/debug/echo`) and making tests more reliable.
Applied to files:
simulation/amaru-sim/src/simulator/simulate.rs
📚 Learning: 2025-06-14T16:31:53.134Z
Learnt from: rkuhn
PR: pragma-org/amaru#263
File: simulation/amaru-sim/src/simulator/simulate.rs:298-300
Timestamp: 2025-06-14T16:31:53.134Z
Learning: StageRef in the pure-stage crate supports serde serialization and deserialization (derives serde::Serialize and serde::Deserialize), enabling it to be used in structs that also derive these traits for TraceBuffer and replay functionality.
Applied to files:
simulation/amaru-sim/src/simulator/simulate.rs
📚 Learning: 2025-08-23T15:44:44.318Z
Learnt from: rkuhn
PR: pragma-org/amaru#392
File: crates/ouroboros-traits/src/is_header/fake.rs:52-88
Timestamp: 2025-08-23T15:44:44.318Z
Learning: In the Amaru project, FakeHeader in crates/ouroboros-traits/src/is_header/fake.rs is specifically designed for testing purposes, and panic behavior (like copy_from_slice() panicking on malformed hex) is the preferred approach rather than graceful error handling, as it helps identify test data issues quickly during development.
Applied to files:
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs
🧬 Code graph analysis (9)
crates/amaru-consensus/benches/headers_tree.rs (2)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (4)
generate_tree_of_headers(83-94)depth(64-66)tree(56-58)nodes(68-70)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (7)
execute_actions_on_tree(449-494)generate_random_walks(302-346)generate_random_walks(675-712)generated_tree(357-359)len(369-371)actions(365-367)actions(696-699)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (2)
crates/amaru-consensus/src/consensus/headers_tree/tree.rs (5)
depth(138-140)nodes(148-157)fork_nodes(159-171)leaves(174-183)make_leaf(53-61)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (2)
generated_tree(357-359)best_chains(361-363)
simulation/amaru-sim/src/simulator/simulate.rs (3)
simulation/amaru-sim/src/simulator/run.rs (2)
node_config(210-212)new(367-378)simulation/amaru-sim/src/simulator/node_config.rs (1)
default(26-32)simulation/amaru-sim/src/simulator/simulate_config.rs (1)
default(28-35)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (4)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (5)
any_tree_of_headers(42-44)tree(56-58)best_chains(60-62)fork_nodes(72-74)depth(64-66)crates/amaru-kernel/src/point.rs (1)
hash(37-43)crates/amaru-consensus/src/consensus/headers_tree/tree.rs (3)
children(144-144)fork_nodes(159-171)depth(138-140)crates/amaru-kernel/src/string_utils.rs (2)
list_to_string(21-21)list_to_string(29-31)
simulation/amaru-sim/tests/traces.rs (1)
simulation/amaru-sim/src/simulator/node_config.rs (1)
default(26-32)
simulation/amaru-sim/src/simulator/data_generation/generate.rs (4)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (3)
any_select_chains_from_tree(422-427)peer(214-219)generated_tree(357-359)crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (1)
any_tree_of_headers(42-44)crates/amaru-ouroboros-traits/src/is_header/tests.rs (1)
run_with_rng(128-135)simulation/amaru-sim/src/simulator/run.rs (2)
generated_actions(264-268)new(367-378)
crates/amaru-consensus/src/consensus/headers_tree/tree.rs (2)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (5)
fork_nodes(72-74)any_headers_tree(37-39)generate_headers_chain(152-155)generate_headers_tree(97-99)tree(56-58)crates/amaru-consensus/src/consensus/headers_tree/data_generation/property_configuration.rs (1)
config_begin(55-64)
simulation/amaru-sim/src/simulator/run.rs (4)
simulation/amaru-sim/src/simulator/data_generation/generate.rs (1)
generate_entries(38-85)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (6)
peers(73-75)new(64-69)new(200-209)best_chains(401-416)format(139-172)best_chain(527-529)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (3)
generated_tree(357-359)best_chains(361-363)statistics(377-384)simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
header_hash(69-75)
simulation/amaru-sim/src/simulator/world/world.rs (2)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (4)
fmt(117-122)fmt(126-128)fmt(132-134)format(139-172)simulation/amaru-sim/src/echo/message.rs (2)
fmt(27-29)fmt(65-98)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Benches
- GitHub Check: Build riscv32
- GitHub Check: Build aarch64/macos
- GitHub Check: Build aarch64/linux
- GitHub Check: Test the CLI
- GitHub Check: Build x86_64/windows
- GitHub Check: Test coverage
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Analyze (rust)
🔇 Additional comments (14)
simulation/amaru-sim/src/simulator/world/world.rs (3)
18-18: Nice one, mate! Imports look spot on.Both the
ListToStringandDisplayimports are properly used downstream in the logging code. Clean as a whistle.Also applies to: 23-23
135-139: Sweet as! This conditional logging is like a well-placed checkpoint.Only logging when there's actually something to show is a ripper optimization – keeps the logs clean and reduces noise when the outgoing queue is empty. The
ListToStringformatting makes it readable too.This is like adding a HUD that only shows up when you need it. Top stuff!
82-82: All good on the Display front, mate – the ships have already sailed!Adding the
Displaytrait bound toWorld<Msg>is indeed a breaking API change, but here's the good news: the existing message types in your codebase already have the goods. BothChainSyncMessageandEchoMessagealready implementDisplay, so you're not gonna cop any compilation errors. It's like checking your toolbelt before heading to the site and realizing you've already got everything you need – no wasted trips!The bound is spot-on too – it's needed for that
%envelopeformatting happening downstream. This is a legit API improvement that happens to align perfectly with your current usage.crates/amaru-consensus/src/consensus/headers_tree/tree.rs (2)
257-257: Nice work updating the test scaffolding to match the new API!The import and function call updates look spot-on – you've cleanly migrated from the old
any_tree_of_headers+Ratioapproach to the simplerany_headers_tree, and the rename fromgenerate_header_treetogenerate_headers_treeis consistent. No dramas here, cobber.Also applies to: 264-264, 290-290
294-299: Pretty-print expectations updated correctly for the new tree structure.The expected output now reflects additional forked nodes, which aligns with the changes to data generation logic mentioned in the PR summary. The tree structure's more complex now (forks around 1/3 and 2/3 of the chain), and you've updated the golden output to match. Sweet as!
simulation/amaru-sim/src/simulator/run.rs (1)
296-302: Nice: stats logger + clearer property failure.Love the readable diagnostics and per-line stats—handy when hunting regressions. Looks ace.
Also applies to: 256-294
simulation/amaru-sim/src/simulator/simulate.rs (4)
50-64: Eight parameters is getting chonky, but fair enough with the allow attribute.The
#[allow(clippy::too_many_arguments)]acknowledges the hefty signature. If this function keeps growing in the future, consider bundling some of these into a dedicated config struct (maybe aSimulationCallbacksstruct for the function params?). For now though, it's all good—the parameters are clearly named and the intent is crystal clear.
62-62: Display bound added to Msg—spot on!Adding
Displayto theMsgtrait bounds makes perfect sense for the new statistics display functionality. Clean as a whistle.
166-172: Nice little helper for displaying config at the end.This
display_test_configurationfunction keeps the final summary tidy. Simple, focused, does one thing well—like a good indie game that nails its core mechanic.
279-298: Tests updated properly—no dramas here.The test cases have been adjusted to match the new signature with
simulate_configandnode_config. The|_| ()no-op callback fordisplay_test_statsis appropriate for these tests. All looking shipshape!crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (4)
56-80: Clean helper methods on Action—tidy as!The new
hash(),set_peer(), andpretty_print()methods are well-crafted and serve clear purposes:
hash()extracts the hash uniformly from both action variantsset_peer()enables reassigning peer ownership (used in the duplication logic)pretty_print()generates reproducible test case stringsAll three are concise and follow Rust idioms nicely. Top marks!
302-346: Peer duplication logic looks intentional—clever trick for simulating synchronized peers.The refactored
generate_random_walksdoes some interesting things:
- Lines 305-315: Each peer gets its own seeded RNG for reproducibility
- Lines 318-326: When
peers_nb > 2, peer 2 duplicates peer 1's actions—this simulates peers seeing the same chain, which is a realistic test scenario per the PR objectives- Lines 329-333: Truncating trailing rollbacks keeps the action list clean
- Lines 336-346: Transposing actions interleaves them to avoid tree pruning issues
The logic is sound and well-commented. The peer duplication is a ripper of an idea for testing edge cases!
349-410: GeneratedActions and statistics structs are well-designed—beaut API!The new
GeneratedActionstype bundles the tree, per-peer actions, and interleaved actions together with clean accessors. TheGeneratedActionsStatisticsprovides useful introspection into the generated data.The
Displayimpl for statistics and thelines()helper make it easy to log this info. The whole API feels cohesive and easy to use—like a well-designed game UI that just makes sense.
416-427: Updated signatures returning GeneratedActions—nice type evolution.The strategy functions now yield
GeneratedActionsinstead of raw action lists, which is a solid improvement. Users get the full context (tree + actions + stats) instead of just the actions. This is like upgrading from a basic health bar to a full stats screen—way more useful!
etorreborre
force-pushed
the
etorreborre/test/simulation-data-statistics
branch
from
1b0baaf to
dc5f64d
Compare
Signed-off-by: etorreborre <etorreborre@yahoo.com>
etorreborre
force-pushed
the
etorreborre/test/simulation-data-statistics
branch
from
dc5f64d to
579e88b
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
simulation/amaru-sim/src/simulator/run.rs (1)
229-249: Still got that underflow edge case lurking, mate.The previous review flagged the
depth - 1underflow risk whengenerated_chain_depth <= 1, and it's marked as "addressed in e5f6211"—but the code here still does the raw subtraction at line 239. Ifgenerated_chain_depthis 0 or 1, you'll get 0 or underflow the intent, andHeadersTreeState::new(0)will fail the assertion atheaders_tree.rs:201-204(requiresmax_length >= 2).Also, line 232 should take
peers: &[Peer]instead of&Vec<Peer>for idiomatic Rust—slices are more flexible and that's the Rust way.Apply this diff to properly clamp and use slices:
fn make_chain_selector( chain_store: Arc<dyn ChainStore<BlockHeader>>, generated_chain_depth: u64, - peers: &Vec<Peer>, + peers: &[Peer], ) -> SelectChain { // Set the maximum length for the best fragment based on the generated chain depth // so we generated roll forwards that move the best chain anchor. - let max_length = if generated_chain_depth > DEFAULT_MAXIMUM_FRAGMENT_LENGTH as u64 { - DEFAULT_MAXIMUM_FRAGMENT_LENGTH - } else { - generated_chain_depth as usize - 1 - }; + let depth_usize = generated_chain_depth as usize; + let max_length = depth_usize + .saturating_sub(1) + .clamp(2, DEFAULT_MAXIMUM_FRAGMENT_LENGTH); let mut tree_state = HeadersTreeState::new(max_length);
♻️ Duplicate comments (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (1)
83-94: Guard against depth underflow and ditch the unused spine, yeah?If
depth == 0, thendepth - 1wraps (usize underflow), and you'll spawn a massive chain—classic off-by-one nightmare. Also, lines 88-89 computespineand insert the root, but the result is never actually used. The previous review flagged this as critical, but it's still hanging around.Apply this diff:
pub fn generate_tree_of_headers(depth: usize, seed: u64) -> GeneratedTree { let mut rng = StdRng::seed_from_u64(seed); let root = generate_header(1, 1, None, &mut rng); let mut root_tree = Tree::make_leaf(&root); - let mut spine = generate_header_subtree(&mut rng, &mut root_tree, depth, depth - 1); - spine.insert(0, root); + assert!(depth >= 1, "generate_tree_of_headers: depth must be >= 1 (got {depth})"); + generate_header_subtree(&mut rng, &mut root_tree, depth, depth - 1); GeneratedTree { tree: root_tree, depth, } }
🧹 Nitpick comments (3)
simulation/amaru-sim/src/echo/message.rs (1)
64-99: Top-notch Display implementation, legends have it fixed!Your Display impl for EchoMessage is looking mint! Each variant formats nicely with clear labels and all the placeholders line up like a well-organized inventory screen.
I see that rogue closing paren at line 84 from the previous review has been sorted – nice work getting that cleaned up! The format string
"Echo ({}) {}"is now balanced and proper.One tiny optional thought, though – spotted that you're using
node_ids.list_to_string(", ")at line 77, while over inchain_sync_message.rs(line 145) they're using the standard library's.join(", ")for the same vibe. Both get the job done, but sticking with.join()keeps things a bit more vanilla and consistent across the codebase. Totally optional, mind you – just a little stylistic nudge if you're ever refactoring!Apply this diff if you fancy keeping it standard-library-flavoured:
} => { write!( f, "Init ({}) node_id={}, node_ids={}", msg_id, node_id, - node_ids.list_to_string(", ") + node_ids.join(", ") ) }crates/amaru-consensus/src/consensus/headers_tree/mod.rs (1)
21-21: Public API exposure: confirm intent and add doc-cfg for gated APIs.Making tree public is fine; just ensure test-only helpers inside remain cfg-gated (they are) and consider adding doc-cfg for discoverability. Update changelog/semver notes.
simulation/amaru-sim/tests/simulation.rs (1)
49-61: JSON log toggle LGTM; tiny ergonomics tweak optional.Current init looks good. If you fancy, accept TRUE/True as well: compare lowercased value.
- env::var(var_name).is_ok_and(|v| v == "1" || v == "true") + env::var(var_name).map(|v| v.eq_ignore_ascii_case("1") || v.eq_ignore_ascii_case("true")).unwrap_or(false)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (19)
crates/amaru-consensus/benches/headers_tree.rs(4 hunks)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs(9 hunks)crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs(4 hunks)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/tree.rs(4 hunks)crates/pure-stage/src/time.rs(2 hunks)simulation/amaru-sim/src/bin/amaru-sim/main.rs(1 hunks)simulation/amaru-sim/src/echo/message.rs(3 hunks)simulation/amaru-sim/src/simulator/args.rs(0 hunks)simulation/amaru-sim/src/simulator/data_generation/generate.rs(2 hunks)simulation/amaru-sim/src/simulator/node_config.rs(0 hunks)simulation/amaru-sim/src/simulator/run.rs(7 hunks)simulation/amaru-sim/src/simulator/simulate.rs(9 hunks)simulation/amaru-sim/src/simulator/world/node_handle.rs(2 hunks)simulation/amaru-sim/src/simulator/world/world.rs(4 hunks)simulation/amaru-sim/src/sync/chain_sync_message.rs(2 hunks)simulation/amaru-sim/tests/simulation.rs(1 hunks)simulation/amaru-sim/tests/traces.rs(1 hunks)
💤 Files with no reviewable changes (2)
- simulation/amaru-sim/src/simulator/args.rs
- simulation/amaru-sim/src/simulator/node_config.rs
🚧 Files skipped from review as they are similar to previous changes (4)
- crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs
- simulation/amaru-sim/src/bin/amaru-sim/main.rs
- crates/pure-stage/src/time.rs
- simulation/amaru-sim/src/simulator/world/node_handle.rs
🧰 Additional context used
🧠 Learnings (3)
📚 Learning: 2025-05-12T14:21:27.470Z
Learnt from: stevana
PR: pragma-org/amaru#210
File: simulation/amaru-sim/src/simulator/simulate.rs:264-277
Timestamp: 2025-05-12T14:21:27.470Z
Learning: The team plans to replace the out-of-process test in `simulation/amaru-sim/src/simulator/simulate.rs` with an in-process NodeHandle implementation in the future, eliminating the need for hard-coded binary paths (`../../target/debug/echo`) and making tests more reliable.
Applied to files:
simulation/amaru-sim/tests/traces.rssimulation/amaru-sim/src/simulator/simulate.rs
📚 Learning: 2025-06-14T16:31:53.134Z
Learnt from: rkuhn
PR: pragma-org/amaru#263
File: simulation/amaru-sim/src/simulator/simulate.rs:298-300
Timestamp: 2025-06-14T16:31:53.134Z
Learning: StageRef in the pure-stage crate supports serde serialization and deserialization (derives serde::Serialize and serde::Deserialize), enabling it to be used in structs that also derive these traits for TraceBuffer and replay functionality.
Applied to files:
simulation/amaru-sim/src/simulator/simulate.rs
📚 Learning: 2025-08-23T15:44:44.318Z
Learnt from: rkuhn
PR: pragma-org/amaru#392
File: crates/ouroboros-traits/src/is_header/fake.rs:52-88
Timestamp: 2025-08-23T15:44:44.318Z
Learning: In the Amaru project, FakeHeader in crates/ouroboros-traits/src/is_header/fake.rs is specifically designed for testing purposes, and panic behavior (like copy_from_slice() panicking on malformed hex) is the preferred approach rather than graceful error handling, as it helps identify test data issues quickly during development.
Applied to files:
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs
🧬 Code graph analysis (12)
simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
simulation/amaru-sim/src/echo/message.rs (2)
fmt(27-29)fmt(65-98)
simulation/amaru-sim/tests/traces.rs (1)
simulation/amaru-sim/src/simulator/node_config.rs (1)
default(26-32)
simulation/amaru-sim/src/echo/message.rs (3)
simulation/amaru-sim/src/sync/chain_sync_message.rs (2)
fmt(79-132)fmt(136-172)simulation/amaru-sim/src/sync/output_writer.rs (1)
write(37-42)simulation/amaru-sim/src/echo/service.rs (1)
echo(73-88)
crates/amaru-consensus/benches/headers_tree.rs (2)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (4)
generate_tree_of_headers(83-94)depth(64-66)tree(56-58)nodes(68-70)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (6)
generate_random_walks(302-346)generate_random_walks(675-711)generated_tree(357-359)len(369-371)actions(365-367)actions(696-699)
simulation/amaru-sim/src/simulator/simulate.rs (4)
simulation/amaru-sim/src/simulator/run.rs (2)
node_config(210-212)new(367-378)simulation/amaru-sim/src/simulator/node_config.rs (1)
default(26-32)simulation/amaru-sim/src/simulator/simulate_config.rs (1)
default(28-35)simulation/amaru-sim/src/echo/simulate.rs (3)
spawn_echo_node(31-69)echo_generator(72-92)echo_property(97-123)
crates/amaru-consensus/src/consensus/headers_tree/tree.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (5)
fork_nodes(72-74)any_headers_tree(37-39)generate_headers_chain(152-155)generate_headers_tree(97-99)tree(56-58)
simulation/amaru-sim/src/simulator/world/world.rs (3)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (4)
fmt(117-122)fmt(126-128)fmt(132-134)format(139-172)crates/pure-stage/src/time.rs (2)
fmt(59-63)fmt(67-69)simulation/amaru-sim/src/echo/message.rs (2)
fmt(27-29)fmt(65-98)
simulation/amaru-sim/src/simulator/data_generation/generate.rs (4)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (3)
any_select_chains_from_tree(422-427)peer(214-219)generated_tree(357-359)crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (1)
any_tree_of_headers(42-44)crates/amaru-ouroboros-traits/src/is_header/tests.rs (1)
run_with_rng(128-135)simulation/amaru-sim/src/simulator/data_generation/base_generators.rs (1)
generate_arrival_times(67-84)
simulation/amaru-sim/src/simulator/run.rs (5)
simulation/amaru-sim/src/simulator/data_generation/generate.rs (1)
generate_entries(38-85)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (5)
peers(73-75)new(64-69)new(200-209)best_chains(401-416)best_chain(527-529)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (3)
generated_tree(357-359)best_chains(361-363)statistics(377-384)crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (2)
best_chains(60-62)tree(56-58)simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
header_hash(69-75)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (3)
crates/amaru-consensus/src/consensus/headers_tree/tree.rs (5)
depth(138-140)nodes(148-157)fork_nodes(159-171)leaves(174-183)make_leaf(53-61)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (2)
generated_tree(357-359)best_chains(361-363)crates/amaru-ouroboros-traits/src/is_header/mod.rs (1)
header_body(133-135)
crates/amaru-consensus/src/consensus/headers_tree/mod.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (1)
tree(56-58)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (4)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (5)
any_tree_of_headers(42-44)tree(56-58)best_chains(60-62)fork_nodes(72-74)depth(64-66)crates/amaru-ouroboros-traits/src/is_header/mod.rs (5)
hash(36-36)hash(110-112)hash(178-180)hash(200-202)header(129-131)crates/amaru-kernel/src/point.rs (1)
hash(37-43)crates/amaru-kernel/src/string_utils.rs (2)
list_to_string(21-21)list_to_string(29-31)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Benches
- GitHub Check: Build aarch64/macos
- GitHub Check: Build aarch64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Build riscv32
- GitHub Check: Examples (ledger-in-nodejs)
- GitHub Check: Test coverage
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Analyze (rust)
🔇 Additional comments (21)
simulation/amaru-sim/src/sync/chain_sync_message.rs (2)
26-26: Solid import, mate!The Display and Formatter imports are spot-on for what you're adding below. Clean and tidy, like a freshly organized game library.
135-173: Ripper of an implementation, cobber!This Display trait is clean as a whistle. The formatting is concise and readable, perfect for logging or user-facing output. A few things that caught my eye (all good):
- The hash truncation to 3 bytes is consistent with your Debug impl—nice touch for keeping logs readable without drowning in hex soup.
- Explicitly ignoring the
headerfield in the Fwd variant (line 156) is the right call for Display; saves us from a wall of encoded bytes.- Using
node_ids.join(", ")is spot-on for Vec—no dramas there.- The slice truncation with
.min(3)makes it panic-proof even if someone sends an empty hash (though that'd be a bit of a plot twist).This pairs beautifully with your existing Debug impl for different verbosity levels. Like having both a cinematic cut and a director's commentary—you get to pick your poison!
simulation/amaru-sim/src/echo/message.rs (2)
15-17: Imports looking solid, mate!Both imports are spot-on and actually used in the Display implementations below. Clean as a whistle!
26-30: Beautiful Display impl for Envelope!The
src -> dest: bodyformat is clean and intuitive, like a well-crafted cutscene. TheT: Displaybound is bang-on correct – can't display an envelope if you can't display what's inside it, right?crates/amaru-consensus/benches/headers_tree.rs (1)
61-67: Bench flow migration looks solid.Generation via GeneratedTree/GeneratedActions and execution wiring are consistent. Nice one.
Also applies to: 69-72, 99-99
crates/amaru-consensus/src/consensus/headers_tree/tree.rs (1)
185-203: branches implementation LGTM.Returns all root-to-leaf paths as expected; cloning is fine for test-utils.
simulation/amaru-sim/src/simulator/data_generation/generate.rs (1)
31-38: Generator refactor to GeneratedActions is coherent; deterministic enough for repro.Good move returning GeneratedActions; arrival times and action zipping align. No blockers.
If you keep World: Display, confirm ChainSyncMessage implements Display to satisfy logging in world.rs; otherwise switch world logging to Debug per my other comment.
Also applies to: 42-48, 49-56, 57-63, 83-83
simulation/amaru-sim/src/simulator/world/world.rs (1)
82-82: The Display bound works for current code, but the design concern stands—refactor to avoid cascading constraints.Right, so here's the story: the Display bound IS genuinely needed for the logging at lines 129–139 as currently written. Envelope implements Display but requires T: Display, and the
list_to_string()call also demands Display on elements. EchoMessage and ChainSyncMessage both tick that box, so the code compiles.But—and this is the kicker—your review comment nails the architectural point. The Drop impl shows Display isn't universal, meaning you're forcing an unnecessary constraint on future Msg types that might not want or need it. Like forcing everyone to use a specific game engine when a simpler one would do.
The suggestions in your comment are spot on. Either:
- Switch to Debug formatting (use
?instead of%):-impl<Msg: PartialEq + Clone + Debug + Display> World<Msg> { +impl<Msg: PartialEq + Clone + Debug> World<Msg> {And update lines 129 and 135–139 to use
?formatting—cleaner and avoids the cascading constraint.
- Keep Display if you genuinely prefer the pretty output, but this locks all future Msg types into implementing it.
The code works now, but from a design perspective, you're right to question forcing Display across the board. The first option (Debug) is the safer, more flexible play.
simulation/amaru-sim/src/simulator/run.rs (3)
256-294: LGTM on the property refactor—nice error handling upgrade!The chain property now properly returns a
Result, usesGeneratedActionsto get the best chains directly, and provides detailed error messages with the tree and actions when things go sideways. The logic checks that the downstream-built chain matches one of the expected best chains from upstream—solid stuff.
297-302: Statistics display helper looks clean.Simple pass-through to log the statistics lines—does exactly what it says on the tin.
305-340: Improved error handling with Result return—nice one!Converting
make_best_chain_from_downstream_messagesto returnResult<Chain, String>instead of panicking on rollback mismatch is a solid improvement. The error message includes context (action number, hash, chain, history) for easier debugging.crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (1)
110-146: New branching logic looks solid—forks at 1/3 intervals, nice.The updated
generate_header_subtreecreates forks at regular intervals (every 1/3 of total depth) and occasionally adds an extra branch with 20% probability. The use ofrng.random_bool(0.2)is correct for rand 0.9 (as etorreborre confirmed in the past review thread). Logic is clear and aligns with the PR objectives.simulation/amaru-sim/src/simulator/simulate.rs (3)
51-132: Simulate refactor looks ace—clean separation of configs and stats display.The updated
simulatesignature now takes separatesimulate_configandnode_configparameters, adds thedisplay_test_statscallback for per-test statistics, and includesDisplayin theMsgbound to support richer formatting. The test loop correctly usessimulate_config.number_of_testsandsimulate_config.seed, and the success path callsdisplay_test_statsanddisplay_test_configurationat the right spots. The flow is clear and aligns perfectly with the PR's goal to display statistics after each test run and at the end.
166-172: Config display helper is straightforward.Logs the number of tests and upstream peers—does the job without fuss.
278-295: Test updates for new simulate signature look good.The test now constructs
SimulateConfigandNodeConfigseparately and passes both tosimulate, along with the no-op stats display closure. Aligns with the new API.crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (6)
56-80: Nice additions to Action—hash, set_peer, and pretty_print look solid.The new methods on
Actionare straightforward:
hash()extracts theHeaderHashfrom either variantset_peer()reassigns the peer while preserving the action typepretty_print()formats as a JSON string withr#"..."#wrapperThese enable the downstream statistics and diagnostics flow. Clean implementation.
255-293: random_walk refactor with per-peer RNG looks good.The updated
random_walknow takes a per-peerpeer_rngand uses it to order children viarandom_bool(0.5)at line 274. The use ofrandom_boolis correct for rand 0.9 (as you clarified in the past review thread). The logic correctly pushes RollForward, recurses into children in randomized order, and pushes RollBack to the parent when backtracking. Avoids duplicate rollbacks with the hash check at line 289.
302-346: generate_random_walks implementation is ace—peer duplication and transpose logic is spot on.The function generates per-peer random walks with dedicated RNGs (seeded from peer index), duplicates peer 1's actions to peer 2 when
peers_nb > 2(lines 318-326), truncates trailing rollbacks (lines 329-333), and transposes the actions for interleaving (lines 336-340). The peer duplication is a clever way to represent the "one peer duplicates another's actions" scenario mentioned in the PR objectives. The transpose ensures all peers get a chance to roll forward from the root before the tree grows beyond max_length. Solid design.
348-410: GeneratedActions and GeneratedActionsStatistics structs look clean.
GeneratedActionsencapsulates the tree, per-peer actions, and flattened actions list, with straightforward accessors (generated_tree,best_chains,actions,len,is_empty,statistics).GeneratedActionsStatisticscomputes tree depth, node count, and fork count, with aDisplayimpl that formats as multiple lines. Thelines()helper is useful for logging. All accessors delegate correctly.
416-427: Strategy functions updated to return GeneratedActions—perfect.
any_select_chainsgenerates a tree then wraps it ingenerate_random_walks, andany_select_chains_from_treeusesJustto wrap the result. Both returnGeneratedActionsas expected by the new API surface. Clean and idiomatic proptest usage.
674-711: Test exercises the new generate_random_walks thoroughly—nice assertions.The test generates random walks for 3 peers and checks:
- Fork count is between 3-4 (lines 686-690)
- Exactly 2 unique action patterns exist (peer duplication, lines 692-711)
The use of
BTreeSet<String>at line 703 is correct (you addressed the previous HashSet comment). The assertions validate the key properties of the new data generation logic. Solid test coverage.
1 participant
This PR displays simple statistics, after each test run (because the headers tree used to send message varies) and at the end of all the tests:
Additionally:
The data generation is simplified to only produce a few forks, basically at 1/3rd and 2/3rd of the best chain (then recursively on branches). This removes the need for configuration parameters beside the best chain length.
If the number of peers is > 2 we make sure that one peer duplicates the actions of an another one since this is a relevant situation.
A unit test has been added to check the 2 properties above.
GeneratedTreeandGeneratedActionsprovide additional information on top of the generated data in order to display statistics.A bug was fixed that was preventing to see the seed in case of a failure (because
assert!was used instead of returning a result in the best chain oracle).The
HeadersTreeused during the simulation has amax length < best chain lengthin order to make sure that some roll-forwards move the best chain anchor.The
AMARU_SIMULATION_LOG_AS_JSONvariable is introduced to generate the simulation logs as json if necessary.I ran the simulation with chain size of 2160 headers with 3 peers (among other trials). It takes 280s on my machine and succeeds.
Summary by CodeRabbit
New Features
Chores
Tests