test: improve the data generation for the consensus simulation#510
Conversation
|
@coderabbitai review |
WalkthroughReplaces file-based chain and ledger generation with an action-based generator and proptest strategies; makes Changes
Sequence Diagram(s)sequenceDiagram
%% Styling: subtle colored rects for clarity (renders in supporting viewers)
participant Test
participant Sim as Simulate
participant Gen as Generator (closure)
participant RNG as run_with_rng
participant Select as any_select_chains
participant MakeEntries as generate_entries (make_entry)
participant World
participant Property
Test->>Sim: start simulate(...)
Sim->>Gen: call generator(rng)
Gen->>RNG: run_with_rng(rng, any_select_chains(peers_nb,...))
RNG->>Select: request actions (peers_nb, depth, rollback/branching ratios, seed)
Select-->>RNG: (Vec<Action>, Chain) %% new: actions plus best-chain spine
RNG-->>Gen: (Vec<Action>, Chain)
Gen->>MakeEntries: translate Actions -> Vec<Entry<ChainSyncMessage>>
MakeEntries-->>Gen: (Vec<Entry<ChainSyncMessage>>, Chain) %% generator returns entries + context (Chain)
Gen-->>Sim: (entries, generation_context)
Sim->>World: feed entries
World->>Property: property(&history, &generation_context)
Property->>Property: derive actual chain from downstream messages
Property->>Property: compare to generation_context (expected chain)
Property-->>World: pass/fail
World-->>Sim: result
Sim-->>Test: pass/fail
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (8)
🚧 Files skipped from review as they are similar to previous changes (1)
🧰 Additional context used🧠 Learnings (2)📚 Learning: 2025-02-03T11:15:22.640ZApplied to files:
📚 Learning: 2025-08-12T12:28:24.027ZApplied to files:
🧬 Code graph analysis (6)crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (1)
simulation/amaru-sim/src/echo/simulate.rs (2)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
simulation/amaru-sim/tests/traces.rs (1)
simulation/amaru-sim/src/simulator/data_generation/generate.rs (8)
simulation/amaru-sim/src/simulator/run.rs (2)
⏰ 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). (12)
🔇 Additional comments (22)
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 |
✅ Actions performedReview triggered.
|
etorreborre
force-pushed
the
etorreborre/test/run-simulation-tests-with-several-peers
branch
3 times, most recently
from
190584b to
0fc993c
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: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
simulation/amaru-sim/src/simulator/world/world.rs (1)
46-65: Critical: Ord/PartialEq/Eq inconsistency on Entry can corrupt heap ordering.cmp only uses arrival_time, but PartialEq (derived) compares arrival_time + envelope. If two different envelopes share the same time, cmp == Equal while eq == false. That breaks the Ord/Eq contract and can trigger weird BinaryHeap behaviour. Like trying to do a photo finish with two Marios but only timing one.
Minimal fix: make equality match ordering (arrival_time-only). Example patch:
-#[derive(Debug, Clone, PartialEq, Serialize)] +#[derive(Debug, Clone, Serialize)] pub struct Entry<Msg> { pub arrival_time: Instant, pub envelope: Envelope<Msg>, } -impl<Msg: PartialEq> PartialOrd for Entry<Msg> { +impl<Msg> PartialOrd for Entry<Msg> { fn partial_cmp(&self, other: &Self) -> Option<Ordering> { Some(self.cmp(other)) } } -impl<Msg: PartialEq> Ord for Entry<Msg> { +impl<Msg> Ord for Entry<Msg> { fn cmp(&self, other: &Self) -> Ordering { self.arrival_time.cmp(&other.arrival_time) } } -impl<Msg: PartialEq> Eq for Entry<Msg> {} +impl<Msg> PartialEq for Entry<Msg> { + fn eq(&self, other: &Self) -> bool { + self.arrival_time == other.arrival_time + } +} + +impl<Msg> Eq for Entry<Msg> {}If you need stable tie‑breaks, consider adding a monotonic seq field and ordering by (arrival_time, seq).
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
107-112: decode_hash panics on invalid length; return an error insteadcopy_from_slice will panic if the decoded hex isn’t exactly HEADER_HASH_SIZE bytes. That’s a sneaky footgun when deserializing JSON. Let’s fail gracefully.
Apply:
fn decode_hash(s: &str) -> Result<HeaderHash, FromHexError> { let bytes = hex::decode(s)?; - let mut arr = [0u8; HEADER_HASH_SIZE]; - arr.copy_from_slice(&bytes); - Ok(Hash::from(arr)) + if bytes.len() != HEADER_HASH_SIZE { + return Err(FromHexError::InvalidStringLength); + } + let mut arr = [0u8; HEADER_HASH_SIZE]; + arr.copy_from_slice(&bytes); + Ok(Hash::from(arr)) }
🧹 Nitpick comments (14)
crates/amaru-ouroboros-traits/src/is_header/mod.rs (1)
138-140: G'day mate! Spotted a bit of déjà vu here – like seeing the same NPC twice in an open world game.This new
parent_hash()method is doing the exact same thing as theparent()method down on lines 183-185, both pulling fromself.header.header_body.prev_hash. Now, I reckon there might be a solid reason for this – maybe you want folks to access the parent hash without needing theIsHeadertrait in scope, which is fair dinkum for ergonomics. Or perhapsparent_hashjust reads clearer thanparentin the API, like how "The Fellowship of the Ring" is more specific than just "The Fellowship", yeah?But if there's no specific reason for the duplication, you might wanna consider either:
- Using
parent_hash()as the canonical accessor and havingparent()delegate to it, or- Sticking with just the trait method if that's sufficient
Just flagging it so the API stays clean and future maintainers don't scratch their heads wondering which one to use!
crates/amaru/src/stages/build_stage_graph.rs (1)
60-66: Ripper change, mate! Validation errors now take the 'catch and release' approach.This shift from termination to log-and-continue for validation errors is spot-on with your PR goals. Like a good bouncer at a pub, you're now letting the node stay up while just noting the troublemakers in the logbook. The distinction between validation failures (log it) and processing failures (nuke the whole graph) is clearer than a Sydney summer day.
However, that TODO on line 64 is a bit of a ticking time bomb, innit? Without disconnecting dodgy peers, you've basically got a 'Dark Souls' situation where the same enemy keeps respawning and having a go at you. A malicious peer could spam validation errors and fill your logs faster than a cafe hipster fills their Instagram feed with latte art.
Suggested follow-ups:
Track the peer disconnection work: That TODO needs to be actioned soon to prevent bad actors from exploiting the lack of consequences. Want me to open an issue to track this as priority work, or do you already have it sorted?
Consider adding observability (optional/nice-to-have): Right now you're just logging, but adding a counter or metric for validation errors would be ace for monitoring. Something like:
async |_, error: ValidationFailed, _eff| { tracing::error!(%error, "stage error"); // Optional: track validation error metrics // metrics::counter!("consensus.validation_errors").increment(1); // TODO: disconnect bad behaving peers }This would give you that sweet, sweet Prometheus data to catch issues before they become proper dramas.
crates/amaru-ouroboros-traits/src/is_header/tests.rs (1)
120-130: Nice little helper function here, but could use a touch more clarity in the docs!The implementation looks solid as—mirrors the existing
runpattern and uses proptest's fixed seed API correctly. Love that it keeps RNG-driven tests deterministic, like quicksaving before a boss fight.Just one tiny thing: the doc comment says "seed provided by a random generator", which might confuse folks into thinkin' the RNG drives generation directly. What's actually happenin' is you're usin' the RNG to generate one seed value, then proptest's TestRunner uses that fixed seed internally. Might be worth clarifying that in the docs, or even tossin' in a wee usage example.
Optional doc improvement:
-/// Run a strategy with a seed provided by a random generator -/// and return the generated value, panicking if generation fails. +/// Run a strategy with a fixed seed derived from the given RNG. +/// The RNG is used to generate a deterministic seed for proptest's TestRunner, +/// ensuring reproducible test data generation. Panics if generation fails. +/// +/// # Example +/// ```ignore +/// let mut rng = rand::thread_rng(); +/// let header = run_with_rng(&mut rng, any_header()); +/// ```simulation/amaru-sim/Cargo.toml (1)
38-39: Bold move flipping on test-utils for consensus and traits. Looks good for the sim binary, but mind the cargo feature avalanche, mate.
- If these helpers aren’t needed at runtime, consider gating behind a sim-only feature to keep compile times lean when building other bins.
- Also, you’ve got rand in both dependencies (Line 25) and dev-dependencies (Line 46); consider consolidating later.
simulation/amaru-sim/src/simulator/world/world.rs (2)
72-79: Custom Debug for History prints one message per line. Sweet for eyeballing, but a bit spartan for log parsers.Optional: prefix with History(len = N): and wrap in brackets to make it grep‑friendly.
128-130: Heap dump at debug level can be chonky as a Morrowind mod list.Suggest: move heap = ?self.heap to trace or gate behind a verbose flag.
simulation/amaru-sim/tests/simulation.rs (2)
83-86: is_true accepting "true" is a nice touch. Consider case-insensitive and more truthy tokens ("yes", "on") for friendlier DX.Example:
-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.trim().to_ascii_lowercase()) + .map(|v| v == "1" || v == "true" || v == "yes" || v == "on") + .unwrap_or(false) +}
39-50: Spot on—whitespace handling and input robustness are the real MVPs here.Your original review nailed it, mate. Here's what I found:
Whitespace footgun is legit:
get_env_var(line 71) callsparse()directly without trimming. If someone accidentally setsAMARU_GENERATED_CHAIN_ROLLBACK_RATIO=" 1/2 "(with spaces), it silently falls back to the default instead of erroring. Like a roguelike where you don't realise you've walked into a trap until it's too late.Ratio format needs docs: The
FromStrimpl (line 328–340) expects strict"numerator/denominator"format but it's nowhere documented. A quick README note would save future devs the head-scratching.is_true is case-sensitive: The function (line 84–86) only matches
"1"or"true"exactly. Doesn't handle"True","TRUE", or whitespace. Could be friendlier.These aren't critical bugs, but they're solid quality-of-life wins—the difference between smooth UX and "why didn't my config work?"
Consider adding:
.trim()inget_env_varbefore parsing (line 72–74)- Similar trim in
is_trueand make it case-insensitive- Document Ratio format in test README
simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
52-65: Decode fallback to plain Header for sturdier DebuggingNice helper, mate. For robustness, try decoding a plain Header if MintedHeader decoding fails, so Debug still shows parent_hash when headers are serialized without the mint wrapper.
Apply this minimal fallback:
pub fn decode_block_header(&self) -> Option<BlockHeader> { match self { ChainSyncMessage::Fwd { header, .. } => { - if let Ok(minted_header) = cbor::decode::<MintedHeader<'_>>(&header.bytes) { - Some(BlockHeader::from(Header::from(minted_header))) - } else { - None - } + if let Ok(minted_header) = cbor::decode::<MintedHeader<'_>>(&header.bytes) { + Some(BlockHeader::from(Header::from(minted_header))) + } else if let Ok(hdr) = cbor::decode::<Header>(&header.bytes) { + Some(BlockHeader::from(hdr)) + } else { + None + } } _ => None, } }crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (2)
289-301: Defensive use of random_ratioEven with input validation, a debug assert helps future refactors, and clamping avoids UB in tests.
Apply:
- if rng.random_ratio(rollback_ratio.0, rollback_ratio.1) + debug_assert!(rollback_ratio.1 > 0, "Ratio denominator must be > 0"); + let num = rollback_ratio.0.min(rollback_ratio.1); + if rng.random_ratio(num, rollback_ratio.1) && let Some(actions) = result.get_mut(peer) {
517-550: Indexing best_chains[0] is safe today; add a tiny guardIt’s currently safe because there’s always at least one chain, but an assert makes that invariant explicit and future‑proof.
Apply:
- let best_chains = current_chains + let best_chains = current_chains .clone() .into_values() .filter(|c| c.len() == best_length) .collect::<Vec<Chain>>(); + debug_assert!(!best_chains.is_empty()); current_chains.insert(Me, best_chains[0].clone());simulation/amaru-sim/src/simulator/data_generation/generate.rs (2)
48-69: Minor nits: avoid clone, use Bytes::from for consistencyTidy ups: no behavior change, just smoother edges.
Apply:
- 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)), }, Action::RollBack { rollback_point, .. } => ChainSyncMessage::Bck { msg_id, slot: rollback_point.slot_or_default(), - hash: Bytes::from(rollback_point.hash().to_vec()), + hash: Bytes::from(rollback_point.hash().to_vec()), }, }; - messages.push(make_entry(action.peer(), arrival_time, message)); + messages.push(make_entry(action.peer(), arrival_time, message));And:
- src: format!("c{}", peer.name.clone()), + src: format!("c{}", peer.name),
74-83: Destination hard-coded to "n1"If number_of_nodes > 1, this might route messages to the wrong node. Is the generator only used in single-node sims? If not, thread the NodeId or let caller set dest.
Happy to sketch a small patch to parameterize dest.
simulation/amaru-sim/src/simulator/run.rs (1)
284-286: Silent failures on decode could hide bugs.When
decode_block_header()orheader_hash()returnNone, you're silently skipping those entries. In a test simulation, you probably want to know if decoding is failing - it could indicate data corruption or a bug in the generation logic.Consider logging or counting decode failures:
msg @ ChainSyncMessage::Fwd { .. } => { - // make sure to skip headers that cannot be decoded - if let Some(header) = msg.decode_block_header() { + match msg.decode_block_header() { + Some(header) => { chain.push(header); + } + None => { + warn!("Failed to decode header from peer {}: {:?}", message.src, msg); + } } }Also applies to: 290-295
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs(4 hunks)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/receive_header.rs(2 hunks)crates/amaru-ouroboros-traits/src/is_header/mod.rs(1 hunks)crates/amaru-ouroboros-traits/src/is_header/tests.rs(2 hunks)crates/amaru/src/stages/build_stage_graph.rs(1 hunks)simulation/amaru-sim/Cargo.toml(1 hunks)simulation/amaru-sim/src/echo/simulate.rs(1 hunks)simulation/amaru-sim/src/simulator/args.rs(2 hunks)simulation/amaru-sim/src/simulator/data_generation/chain.rs(0 hunks)simulation/amaru-sim/src/simulator/data_generation/generate.rs(1 hunks)simulation/amaru-sim/src/simulator/data_generation/mod.rs(0 hunks)simulation/amaru-sim/src/simulator/ledger.rs(0 hunks)simulation/amaru-sim/src/simulator/mod.rs(0 hunks)simulation/amaru-sim/src/simulator/node_config.rs(2 hunks)simulation/amaru-sim/src/simulator/run.rs(6 hunks)simulation/amaru-sim/src/simulator/simulate.rs(7 hunks)simulation/amaru-sim/src/simulator/world/world.rs(3 hunks)simulation/amaru-sim/src/sync/chain_sync_message.rs(3 hunks)simulation/amaru-sim/tests/simulation.rs(2 hunks)simulation/amaru-sim/tests/traces.rs(3 hunks)
💤 Files with no reviewable changes (4)
- simulation/amaru-sim/src/simulator/data_generation/mod.rs
- simulation/amaru-sim/src/simulator/mod.rs
- simulation/amaru-sim/src/simulator/ledger.rs
- simulation/amaru-sim/src/simulator/data_generation/chain.rs
🧰 Additional context used
🧬 Code graph analysis (11)
crates/amaru-consensus/src/consensus/stages/receive_header.rs (2)
crates/amaru-ouroboros-traits/src/is_header/mod.rs (2)
header(130-132)point(37-39)crates/amaru-ouroboros-traits/src/is_header/tests.rs (1)
any_header(77-86)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (2)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/property_configuration.rs (4)
config_begin(55-64)no_shrink(32-35)with_cases(38-41)end(50-52)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
any_select_chains(382-392)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (1)
any_tree_of_headers(37-42)
simulation/amaru-sim/src/simulator/world/world.rs (1)
simulation/amaru-sim/src/simulator/world/node_handle.rs (1)
new(37-48)
simulation/amaru-sim/src/simulator/simulate.rs (2)
simulation/amaru-sim/src/simulator/world/world.rs (1)
fmt(73-78)simulation/amaru-sim/src/simulator/run.rs (1)
entries(246-249)
simulation/amaru-sim/tests/simulation.rs (1)
simulation/amaru-sim/src/simulator/run.rs (1)
node_config(211-213)
simulation/amaru-sim/src/sync/chain_sync_message.rs (2)
crates/amaru-ouroboros-traits/src/is_header/mod.rs (8)
decode(158-161)header(130-132)from(165-170)hash(34-34)hash(111-113)hash(179-181)hash(201-203)parent_hash(138-140)simulation/amaru-sim/src/simulator/bytes.rs (2)
from(57-59)from(73-75)
simulation/amaru-sim/src/echo/simulate.rs (2)
crates/pure-stage/src/time.rs (1)
at_offset(112-114)simulation/amaru-sim/src/simulator/data_generation/base_generators.rs (3)
generate_zip_with(51-63)generate_vec(23-33)generate_arrival_times(67-84)
simulation/amaru-sim/src/simulator/args.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
from_str(328-340)
simulation/amaru-sim/src/simulator/data_generation/generate.rs (6)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (2)
any_select_chains(382-392)peer(191-196)crates/amaru-ouroboros-traits/src/is_header/tests.rs (1)
run_with_rng(123-130)simulation/amaru-sim/src/simulator/run.rs (3)
node_config(211-213)new(368-379)message(385-405)simulation/amaru-sim/src/simulator/data_generation/base_generators.rs (1)
generate_arrival_times(67-84)simulation/amaru-sim/src/simulator/world/world.rs (1)
new(83-97)simulation/amaru-sim/src/simulator/node_config.rs (1)
from(41-49)
simulation/amaru-sim/src/simulator/run.rs (2)
simulation/amaru-sim/src/simulator/data_generation/generate.rs (1)
generate_entries(29-72)simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
header_hash(68-74)
⏰ 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: Test the CLI
- GitHub Check: Benches
- GitHub Check: Build x86_64/windows
- GitHub Check: Build x86_64/linux
- GitHub Check: Test coverage
- GitHub Check: Build riscv32
- GitHub Check: Build aarch64/macos
- GitHub Check: Build aarch64/linux
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
🔇 Additional comments (18)
crates/amaru-consensus/src/consensus/stages/receive_header.rs (1)
246-253: Solid round-trip test, legend!This test is dead simple and does exactly what's on the tin – generates a header, serializes it, deserializes it, and makes sure we get back what we started with. Perfect for catching any CBOR encoding gremlins that might sneak in. Aligns beautifully with the PR's vibe of beefing up the data generation testing.
Bonus XP opportunity: if you implement that TODO from lines 47-48, you could extend this test to also verify that
decoded.point() == pointto ensure the point extraction logic is consistent. But honestly, this test already does its job well, so no worries either way, cobber!simulation/amaru-sim/src/echo/simulate.rs (2)
72-91: Spot on—verified and locked in, mate!The verification came back clean as a whistle. No callers of
echo_generatorshowed up in the search, which means this change is totally isolated and self-contained—like a perfectly crafted side quest that doesn't affect the main storyline. TheReversewrapper isn't lurking anywhere else in the codebase either, so you've got no ripple effects to worry about.This is exactly the kind of straightforward cleanup that makes code sing. The simplified signature plays nice with the rest of the system, and the direct
Entryconstruction is chef's kiss for readability.All good to merge, legend.
96-99: Add a comment explaining the_entriesparameter or remove it if unnecessary.The underscore prefix is intentional (Rust's standard marker for unused parameters), but there's no trait enforcing this signature across the codebase—unlike
chain_propertywhich actually uses itsentriesparameter. The TODO comment about response time suggests this might be reserved for future validation logic.Either add a brief doc comment like
// Reserved for response time validationto signal intent to future maintainers, or drop the parameter entirely if it's not part of any planned work. Right now it's like leaving a placeholder in a game level that never got populated.crates/amaru-ouroboros-traits/src/is_header/tests.rs (1)
15-18: G'day, mate—false alarm on thatRngtrait!Ya see, line 16 already has
use proptest::prelude::*;which pulls in theRngtrait from the rand crate through proptest's prelude. That's your golden ticket right there. The function signature and implementation at lines 123-130 are solid as—it's using the trait bound correctly withrng.random()and all that. No compilation drama here, legend.Likely an incorrect or invalid review comment.
simulation/amaru-sim/src/simulator/world/world.rs (1)
84-93: Nice ergonomics: accepting Vec<Entry> and doing the Reverse wrap internally. Cleaner API, fewer call-site papercuts. LGTM.simulation/amaru-sim/tests/traces.rs (1)
34-37: New generated_chain_* knobs and Ratio usage look tidy. Depth=1 keeps this as a quick smoke test; no objections.crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
1287-1293: Now I'll search directly in the codebase to verify the argument order:The web searches aren't helping—I need to grep the actual codebase directly. Let me search for constants and all call sites in the repo.Based on the script output provided initially, I now have what I need to verify the issue. Let me finalize my analysis:
From the script results:
- Function signature:
pub fn any_select_chains(peers_nb: usize, depth: usize, rollback_ratio: Ratio, branching_ratio: Ratio)- Call at line 1292:
any_select_chains(DEPTH, 5, ROLLBACK_RATIO, BRANCHING_RATIO)This means:
peers_nbis receivingDEPTHdepthis receiving5The review comment claims this is likely swapped. Without being able to directly access the
DEPTHconstant definition or other call sites to understand the intent, the review's concern is based on the semantic oddity that typically in blockchain testing, the "depth" parameter (chain depth) would be larger than the "peers" parameter. A depth of 5 seems shallow, while DEPTH (likely 10 based on the review's parenthetical) would be more typical for depth.However, without definitive evidence from the codebase showing:
- The exact value of DEPTH
- Other correct usages of
any_select_chainsfor comparison- The semantic intent of the function
I cannot conclusively prove the arguments are swapped. This requires manual developer verification.
Fix the argument order in the proptest call.
The function signature is
any_select_chains(peers_nb, depth, rollback_ratio, branching_ratio), but the call at line 1292 passesany_select_chains(DEPTH, 5, ROLLBACK_RATIO, BRANCHING_RATIO), which maps peers to DEPTH and depth to 5. This appears semantically inverted—chain depth testing typically uses a larger depth value. Swap toany_select_chains(5, DEPTH, ROLLBACK_RATIO, BRANCHING_RATIO)to align peers and depth logically.simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
67-74: Ensure hash byte length is always HEADER_HASH_SIZEHeaderHash::from(hash.bytes.as_slice()) assumes the slice has the exact size. If any caller sends fewer/more bytes, you might get unexpected behavior. Please confirm all producers always supply 32 bytes; if not, gate this with a length check or TryFrom.
simulation/amaru-sim/src/simulator/node_config.rs (1)
23-36: Config surface looks good; builder API reads cleanDefaults and the new builder methods make it easy to tweak generation without carting around a kitchen sink. No blockers here.
Also applies to: 45-48, 61-74
simulation/amaru-sim/src/simulator/args.rs (1)
40-51: CLI ratios: confirm invalid inputs are rejectedvalue_parser = Ratio::from_str is spot on. With the suggested Ratio validation, “1/0” or “5/3” will cleanly error at parse time. Once you wire that in, we’re golden.
simulation/amaru-sim/src/simulator/run.rs (5)
18-20: Imports look solid, mate!All the new imports are actually used in the code below. Nice and tidy - no orphaned imports hanging about like a dingo at a barbie.
Also applies to: 32-32, 43-43, 52-52
84-84: Good call using the new chain property!This hooks up the new validation logic nicely. The function signature below matches what simulate expects.
167-170: Nice fix for the multi-peer scenario!Scaling the channel by downstream peer count is spot-on. This directly addresses the "fixes a bug to allow simulation to run with multiple downstream peers" from the PR description. The comment explains it well too.
240-261: Righteous chain validation logic!The property correctly builds the expected chain from upstream and compares it to the actual downstream chain. The error messages are comprehensive - you'll know exactly what went wrong if this fails. Like having a GPS and a paper map, just to be sure.
327-334: Detailed assertion message - good on ya!The assertion provides excellent debugging context including the action number, hash, chain state, and full history. This will make failures much easier to diagnose. Much better than a generic "assertion failed" message.
simulation/amaru-sim/src/simulator/simulate.rs (3)
52-53: Clean removal of the Reverse wrapper!The signature updates are spot-on:
- Property now receives both
entriesandhistoryfor comparison (matching run.rs's chain_property)- Removed
Reversewrapper throughout - simpler is better- Added
Serializebound for message persistence- Bonus info log at line 65 for debugging
All consistent with the PR's goal to simplify entry handling.
Also applies to: 58-58, 64-65
68-71: Property plumbing is wired up correctly.The flow from
entries→test()→property(entries, history)is consistent throughout. The signature updates all match, no loose ends. Like a proper cable management job - everything connected right.Also applies to: 114-115, 133-133
146-146: Cleaner entry access - no more tuple unwrapping.Accessing
entry.envelopedirectly is more readable than the oldentry.0.envelopepattern. The Reverse wrapper is fully gone, and the code reads better for it.Also applies to: 155-155
Signed-off-by: etorreborre <etorreborre@yahoo.com>
…ny downstream peers Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com> | Conflicts: | simulation/amaru-sim/src/simulator/run.rs
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com> | Conflicts: | simulation/amaru-sim/src/simulator/args.rs | simulation/amaru-sim/src/simulator/ledger.rs | simulation/amaru-sim/src/simulator/node_config.rs
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
etorreborre
force-pushed
the
etorreborre/test/run-simulation-tests-with-several-peers
branch
from
4c2603b to
7ee84b2
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
simulation/amaru-sim/src/simulator/run.rs (3)
273-275: Hardcoded peer prefix again.
src.starts_with("c")is brittle; centralise the predicate or make it configurable.
300-308: Deterministic tie-breaker for equally long chains.Picking
best_chain[0]after collecting into a Vec depends on map order. Use a stable min-by key (e.g., peer id) to keep runs reproducible.- let best_chain = current_chains - .clone() - .into_values() - .filter(|c| c.len() == best_length) - .collect::<Vec<Chain>>(); - current_chains.insert(best_chain_tracker.clone(), best_chain[0].clone()); + if let Some((_, chain)) = current_chains + .iter() + .filter(|(_, c)| c.len() == best_length) + .min_by_key(|(peer_id, _)| *peer_id) + { + current_chains.insert(best_chain_tracker.clone(), chain.clone()); + }
317-319: “c1” special-casing is fragile.Filtering on
dest.starts_with("c1")assumes a naming convention. Pass the target downstream peer id via config or a predicate instead.
🧹 Nitpick comments (7)
simulation/amaru-sim/src/simulator/world/world.rs (3)
72-79: Usedebug_list()for cleaner, faster History formatting.Current impl writes each line manually;
fmt::Debugidiomatically usesf.debug_list(). Simpler and avoids an extra trailing newline.impl<Msg: Debug> Debug for History<Msg> { - fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { - for message in &self.0 { - writeln!(f, "{message:?}")?; - } - Ok(()) - } + fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { + f.debug_list().entries(&self.0).finish() + } }
128-130: Dial back heap logging; it’s a lot of bytes for a debug line.Dumping the entire BinaryHeap each step can be hefty and noisy. Log the size instead, or gate behind trace.
- debug!(msg = ?envelope, arrival = ?arrival_time, heap = ?self.heap, "stepping"); + debug!(msg = ?envelope, arrival = ?arrival_time, heap_len = self.heap.len(), "stepping");
135-154: Centralise “client” detection to avoid drift.Here you use
dest.starts_with("c"), elsewhereis_client_message(). Pull this into a single helper (e.g.,Envelope::is_client_dest()), or reuse the same predicate in both spots for consistency.simulation/amaru-sim/src/simulator/args.rs (1)
45-50: Make CLI help explicit about Ratio format.Add “format: N/D (e.g., 1/2)” so folks don’t guess the syntax.
- /// Ratio of rollbacks in the generated chain for a given peer + /// Ratio of rollbacks in the generated chain for a given peer (format: N/D, e.g., 1/2) @@ - /// Ratio of branches generated from a central chain that can be explored by peers during input generation + /// Ratio of branches generated from a central chain that can be explored by peers during input generation (format: N/D, e.g., 1/2)simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
94-120: Debug fmt does CBOR decode per log – keep it cheap.It’s fine for occasional logs, but if Fwd messages are spammy, this decode can add up. Consider:
- Only computing parent_hash under
trace!, or- Using a shorter path (e.g., pre-parsed field if available).
simulation/amaru-sim/src/simulator/run.rs (1)
327-339: Prefer typed error over panic for rollback mismatch.Great assertion message, but panicking in the helper makes reuse harder. Consider returning a Result and letting the property assert, aligning with our past approach to model impossible states with types.
Based on learnings
simulation/amaru-sim/src/simulator/simulate.rs (1)
64-71: Smooth operator! Efficiency gains and better visibility.The changes here are spot on:
- Ditching the
.to_vec()call is a nice efficiency win – no need to clone when you've already got the goods- The info logs give good breadcrumbs through the test execution
Minor thought: those info logs might get a bit chatty during a marathon test session. Could consider
debug!level instead, but given this is a testing harness, having the visibility atinfois fair dinkum useful.If you want to keep things quieter for bulk test runs, consider using
debug!instead:- info!("Test data generated. Now executing the tests"); + debug!("Test data generated. Now executing the tests"); let test = test_nodes(config.number_of_nodes, &spawn, &property); let result = test(&entries); - info!("Test run executed. Now checking results"); + debug!("Test run executed. Now checking results");
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (21)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs(6 hunks)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/receive_header.rs(2 hunks)crates/amaru-ouroboros-traits/src/is_header/mod.rs(1 hunks)crates/amaru-ouroboros-traits/src/is_header/tests.rs(2 hunks)crates/amaru/src/stages/build_stage_graph.rs(1 hunks)simulation/amaru-sim/Cargo.toml(1 hunks)simulation/amaru-sim/src/echo/simulate.rs(1 hunks)simulation/amaru-sim/src/simulator/args.rs(2 hunks)simulation/amaru-sim/src/simulator/data_generation/chain.rs(0 hunks)simulation/amaru-sim/src/simulator/data_generation/generate.rs(1 hunks)simulation/amaru-sim/src/simulator/data_generation/mod.rs(0 hunks)simulation/amaru-sim/src/simulator/ledger.rs(0 hunks)simulation/amaru-sim/src/simulator/mod.rs(0 hunks)simulation/amaru-sim/src/simulator/node_config.rs(2 hunks)simulation/amaru-sim/src/simulator/run.rs(6 hunks)simulation/amaru-sim/src/simulator/simulate.rs(7 hunks)simulation/amaru-sim/src/simulator/world/world.rs(3 hunks)simulation/amaru-sim/src/sync/chain_sync_message.rs(3 hunks)simulation/amaru-sim/tests/simulation.rs(2 hunks)simulation/amaru-sim/tests/traces.rs(3 hunks)
💤 Files with no reviewable changes (4)
- simulation/amaru-sim/src/simulator/data_generation/mod.rs
- simulation/amaru-sim/src/simulator/mod.rs
- simulation/amaru-sim/src/simulator/ledger.rs
- simulation/amaru-sim/src/simulator/data_generation/chain.rs
🚧 Files skipped from review as they are similar to previous changes (9)
- crates/amaru-ouroboros-traits/src/is_header/mod.rs
- crates/amaru-ouroboros-traits/src/is_header/tests.rs
- simulation/amaru-sim/src/simulator/node_config.rs
- simulation/amaru-sim/Cargo.toml
- simulation/amaru-sim/src/echo/simulate.rs
- simulation/amaru-sim/tests/simulation.rs
- simulation/amaru-sim/tests/traces.rs
- crates/amaru-consensus/src/consensus/stages/receive_header.rs
- crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-02-03T11:15:22.640Z
Learnt from: abailly
PR: pragma-org/amaru#75
File: crates/amaru/src/consensus/mod.rs:164-165
Timestamp: 2025-02-03T11:15:22.640Z
Learning: In the Amaru project, chain selection operations (roll_forward and rollback) should use separate result types to leverage the type system for preventing impossible states, rather than using runtime checks or panics.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
🧬 Code graph analysis (7)
simulation/amaru-sim/src/simulator/world/world.rs (1)
simulation/amaru-sim/src/simulator/world/node_handle.rs (1)
new(37-48)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (2)
crates/amaru/src/stages/mod.rs (1)
from_str(143-153)crates/amaru-consensus/src/consensus/headers_tree/data_generation/data_generation.rs (1)
any_tree_of_headers(37-42)
simulation/amaru-sim/src/simulator/args.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
from_str(328-350)
simulation/amaru-sim/src/simulator/run.rs (3)
simulation/amaru-sim/src/simulator/data_generation/generate.rs (1)
generate_entries(29-72)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
current_chains(550-554)simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
header_hash(68-74)
simulation/amaru-sim/src/simulator/data_generation/generate.rs (5)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (2)
any_select_chains(392-402)peer(191-196)crates/amaru-ouroboros-traits/src/is_header/tests.rs (1)
run_with_rng(123-130)simulation/amaru-sim/src/simulator/data_generation/base_generators.rs (1)
generate_arrival_times(67-84)simulation/amaru-sim/src/simulator/bytes.rs (3)
new(25-27)from(57-59)from(73-75)simulation/amaru-sim/src/simulator/node_config.rs (1)
from(41-49)
simulation/amaru-sim/src/simulator/simulate.rs (1)
simulation/amaru-sim/src/simulator/run.rs (1)
entries(246-249)
simulation/amaru-sim/src/sync/chain_sync_message.rs (2)
crates/amaru-ouroboros-traits/src/is_header/mod.rs (12)
decode(157-160)header(129-131)from(164-169)hash(36-36)hash(110-112)hash(178-180)hash(200-202)parent_hash(137-139)slot(51-51)slot(190-192)slot(212-214)encode(147-153)crates/amaru-kernel/src/point.rs (5)
decode(114-131)from(62-64)hash(37-43)s(83-83)encode(99-110)
⏰ 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: Test the CLI
- GitHub Check: Build riscv32
- GitHub Check: Benches
- GitHub Check: Build aarch64/macos
- GitHub Check: Build aarch64/linux
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (19)
crates/amaru/src/stages/build_stage_graph.rs (1)
62-62: Noice choice using the underscore prefix, mate!Renaming
effto_effis spot-on Rust convention for an unused parameter. Makes it crystal clear you're not calling.terminate()here anymore, which is exactly what the PR objectives ordered. The node stays alive even when peers send dodgy headers - proper resilient architecture, that!crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (8)
47-47: Nice one, mate!Spot on with the import – needed for the FromStr trait implementation below.
319-323: Beauty! Display impl looks mint.Clean formatting that pairs perfectly with your FromStr parser. Solid work!
325-351: Bonzer work addressing the previous feedback!You've nailed the validation checks that were flagged before – zero denominator rejection (line 340) and the numerator-must-be-≤-denominator guard (line 343) are both in place. Error messages are clear as day, and this'll prevent those gnarly "1/0" panics or >100% ratios from sneaking through. Top stuff!
Based on learnings
393-401: Ripper refactor, mate!The new
peers_nbandbranching_ratioparameters slot in perfectly with the downstream functions. Your closure wiring is spot-on – passing the right values toany_tree_of_headersandgenerate_random_walks. No dramas here!
517-517: Good call making Chain public!Exposing this type alias makes sense for the broader API, especially with the testing extensions mentioned in the PR objectives. Sweet as!
522-526: Documentation clarity FTW!The updated doc comment explaining the nested list structure is much clearer now. Nice improvement for the next dev who dives into this code!
628-629: Standard test module setup – no worries!Classic Rust test pattern with the
use super::*;import. All good here.
650-679: Absolute ripper test coverage!Your tests nail all the happy paths (valid ratios including edge cases like "0/1" and "1/1") and every error scenario – wrong format, non-numeric parts, zero denominator, and the >1.0 ratio. This is exactly the kind of thorough testing that prevents regressions. Legend!
simulation/amaru-sim/src/simulator/data_generation/generate.rs (1)
29-35: Closure factory looks good.Returning
impl Fn(&mut R) -> Vec<_>with elided lifetime is fine here; nice cleanup from the earlier invalid bound.simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
52-75: Handy helpers.
decode_block_header()andheader_hash()are tidy and reduce duplication. Nice one.simulation/amaru-sim/src/simulator/run.rs (2)
166-171: Channel capacity scaled with downstream fan-out — good call.Preemptively avoids backpressure when events are duplicated.
240-261: Property wiring reads well.Clear expected vs actual, nice
list_to_stringfor diffs.simulation/amaru-sim/src/simulator/simulate.rs (6)
39-39: Righto, imports looking clean!All three additions are put to good use throughout the file. No dead weight here, mate.
114-115: Internal consistency FTW!The signature updates here match perfectly with the public API changes upstream. It's like watching a well-choreographed fight scene in The Matrix – everything flows together.
133-133: Property callback wired up correctly!Passing both
entriesandhistoryto the property gives it the full story for validation. This aligns perfectly with the PR goal of testing the entire downstream chain rather than just the tip.
146-146: Type signature keeping up with the times!The removal of
Reversewrapper here is consistent with the broader refactor. Clean and simple.
155-155: Direct access to the envelope – no more wrapper taxes!Now that
Reverseis gone, accessingentry.envelopedirectly is the way to go. This matches the pattern used elsewhere in the codebase (as seen inrun.rs).
52-53: All call sites properly updated—the breaking changes landed clean.You were spot on about the ripple effects, mate. Ran the sweep and found both places where
simulate()gets called:
- traces.rs (line 65): Property closure
|_, _| Ok(())correctly takes two params- run.rs (line 76):
chain_property()returns a proper closure with signatureimpl Fn(&[Entry<ChainSyncMessage>], &History<ChainSyncMessage>) -> Result<(), String>Both are dialed in for the new API. No orphaned call sites lurking in the shadows. The Serialize bound addition and the new property signature have been stitched through the entire codebase—it's like everyone got the memo before the band started playing.
abailly
left a comment
abailly
left a comment
There was a problem hiding this comment.
It's great we can finally explore varied chains in the simulator, however I have questions about the property verification/data generation logic.
|
|
||
| /// Generate some input echo messages at different arrival times. | ||
| pub fn echo_generator(rng: &mut StdRng) -> Vec<Reverse<Entry<EchoMessage>>> { | ||
| pub fn echo_generator(rng: &mut StdRng) -> Vec<Entry<EchoMessage>> { |
There was a problem hiding this comment.
At this stage I wonder if we should not get rid of the Echo stuff
There was a problem hiding this comment.
This is used to test the simulation framework on a simple system but I also can't tell how useful this is by now.
There was a problem hiding this comment.
Let's keep it for now, and remove it in a later version.
There was a problem hiding this comment.
@abailly Would you rather suppress it now?
…rite assertions Signed-off-by: etorreborre <etorreborre@yahoo.com>
abailly
left a comment
abailly
left a comment
There was a problem hiding this comment.
As a next step on the simulation, I think we'll need some kind of summary report on what's being run
|
@abailly already working on it :-) |
etorreborre
deleted the
etorreborre/test/run-simulation-tests-with-several-peers
branch
2 participants
This PR uses the data generation support testing the
HeadersTreein the simulation runs:Summary by CodeRabbit
New Features
Bug Fixes
Refactoring