refactor: move forward chain to pure stage#464
Conversation
WalkthroughAdds a TCP-backed forward-chain broadcaster and resource-backed network effects; replaces in-process forward_chain with TcpForwardChainServer and ForwardEvent plumbing; migrates rollback payloads from Point → Header; introduces HeaderTip/AsHeaderTip; updates stage graph, tests, simulator, and pure-stage wiring. (47 words) Changes
Sequence Diagram(s)%%{init: {"themeVariables":{"actorBkg":"#F6FFF6","noteBkg":"#FFFBE6"}}}%%
sequenceDiagram
autonumber
actor Validator as "Validate Block Stage"
participant Fwd as "forward_chain stage"
participant Eff as "ForwardEventEffect"
participant Res as "ResourceForwardEventListener"
participant Server as "TcpForwardChainServer"
participant Peers as "Downstream Peers"
Validator->>Fwd: BlockValidationResult::{BlockValidated|RolledBackTo}
Fwd->>Eff: emit ForwardEventEffect(peer, {Forward|Backward})
Eff->>Res: resources.get::<ResourceForwardEventListener>()
alt resource present
Res-->>Server: send(ForwardEvent)
Server->>Peers: broadcast ClientOp::{Forward(Header)|Backward(HeaderTip)}
else resource missing
Note right of Eff: expect -> panic (fatal)
end
sequenceDiagram
autonumber
participant Store as "ChainStore"
participant App as "amaru::stages::mod"
participant Server as "TcpForwardChainServer"
App->>Store: load_header(our_tip_hash)
Store-->>App: Header
App->>Server: create(..., our_tip: HeaderTip)
Note over Server: accept peers → spawn client_protocols → hold tip
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
|
@coderabbitai review please |
|
@etorreborre G'day mate! I'll get right on reviewing this PR for ya. Let me dive into those changes and see what's cooking with this forward chain refactor! 🎬 ✅ Actions performedReview triggered.
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
crates/amaru/Cargo.toml (1)
37-43: Enable Tokio "time" and "macros" features (compile-breaker).rg confirms use of select! and tokio::time::sleep in crates/amaru/src/{stages/pure_stage_util.rs, exit.rs, bin/amaru/metrics.rs, bin/amaru/cmd/daemon.rs} — add the features to avoid compile errors.
tokio = { workspace = true, features = [ "fs", "net", + "macros", + "time", "rt", "rt-multi-thread", "signal", ] }crates/amaru-consensus/src/consensus/stages/validate_block.rs (1)
93-116: Rollback branch ignores inner effect errors and still emits success.
external(...).awaitcan returnOk(Err(_)). Current code treats that as success and sendsRolledBackTo, which is incorrect and can desync peers.Apply robust matching like the roll‑forward branch:
- } => { - if let Err(err) = eff - .external(RollbackBlockEffect::new(&peer, &rollback_header)) - .await - { - error!(?err, "Failed to rollback"); - eff.send(&processing_errors, err).await; - } else { - eff.send( - &downstream, - BlockValidationResult::RolledBackTo { - peer, - rollback_header, - span, - }, - ) - .await - } - } + } => { + match eff + .external(RollbackBlockEffect::new(&peer, &rollback_header)) + .await + { + Ok(Ok(())) => { + eff.send( + &downstream, + BlockValidationResult::RolledBackTo { peer, rollback_header, span }, + ) + .await + } + Ok(Err(err)) => { + error!(?err, "Rollback effect processing failed"); + eff.send(&processing_errors, err).await; + } + Err(err) => { + error!(?err, "Failed to execute rollback effect"); + eff.send(&processing_errors, err).await; + } + } + }crates/amaru-consensus/src/consensus/events.rs (1)
1-313: Unify rollback naming: fix mixedrollback_pointvsrollback_headerFound multiple stragglers — some places use
rollback_pointin logs/formatters while the underlying field/type is aHeader. Keeprollback_headerfor Header-typed fields androllback_pointonly for Point/Hash-typed fields.Key spots to fix:
- crates/amaru-consensus/src/consensus/events.rs — Debug impl for ValidateBlockEvent prints
"rollback_point"while the variant isrollback_header.- crates/amaru-consensus/src/consensus/stages/select_chain.rs — Fork formatting/write! uses
rollback_pointthoughForkdefinesrollback_header.- crates/amaru-consensus/src/consensus/stages/fetch_block.rs and validate_block.rs — pattern matches bind
rollback_headertorollback_pointlocals/labels; update bindings/labels.- Sweep remaining hits in simulation/amaru-sim/, crates/amaru-consensus/src/consensus/headers_tree/, data_generation/*, README.md and tests — some are valid Point uses, others are stale.
Action: rename the log/format field names or local bindings so they match the field types; run a targeted codemod (rg + sed) to replace only Header-typed cases. Tiny gremlins — kill ’em.
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
371-373: Potential over-rollback when switching after a peer rollback.
make_forkis called withold_tip = rollback_hash. That can push the intersection up the chain if the peer rolled back past the true LCA, causing unnecessary rollback and extra re-validation.Use the previous best tip (
peer_tip) asold_tip:- if let Some(best_peer) = self.best_peer() { - let fork = self.make_fork(&best_peer, rollback_hash, &self.best_chain()); + if let Some(best_peer) = self.best_peer() { + let fork = self.make_fork(&best_peer, &peer_tip, &self.best_chain()); Ok(RollbackChainSelection::SwitchToFork(fork)) } else {This keeps the rollback target at the true LCA between the previous best and the new best—less churn, fewer wasted cycles. Like swapping to the right save point instead of restarting the level, mate.
🧹 Nitpick comments (39)
crates/pure-stage/src/receiver.rs (1)
32-34: MakingReceiver::newpublic—API surface now exposes Tokio’s mpsc.All good for the refactor; just note you’re cementing
tokio::sync::mpsc::Receiverin your public API. If you want wiggle room later, consider a constructor that doesn’t leak the concrete channel type, or re-export it under your crate.simulation/amaru-sim/src/simulator/args.rs (1)
57-60: Clamp CLI to a sensible range.
number_of_downstream_peersasu8is grand, but let’s guard the input. Zero might be legit for some sims; if not, make it1..=255.Apply one of:
- #[arg(long, default_value = "1")] + #[arg(long, default_value = "1", value_parser = clap::value_parser!(u8).range(0..=255))] pub number_of_downstream_peers: u8,or (if zero is invalid):
- #[arg(long, default_value = "1")] + #[arg(long, default_value = "1", value_parser = clap::value_parser!(u8).range(1..=255))] pub number_of_downstream_peers: u8,crates/amaru/src/stages/consensus/forward_chain/README.md (1)
12-14: Fix MD039 (spaces inside link text) and wrap the link cleanly.You’ve got a stray space/newline inside the link text (
[ \acto`]`). Tighten it up.Apply this diff:
-The current design therefore splits up the state into non-concurrent pieces and places each piece within an [ -`acto`](https://docs.rs/acto) actor. +The current design therefore splits up the state into non-concurrent pieces and places each piece within an [`acto`](https://docs.rs/acto) actor.crates/amaru/src/stages/pure_stage_util.rs (1)
62-68: Tick interval: consider a shorter heartbeat for snappier shutdown/UI.A 1s sleep is fine, but 100–250ms often feels nicer in sims while still keeping CPU chill.
- _ = sleep(Duration::from_secs(1)) => Ok(WorkSchedule::Unit(())), + _ = sleep(Duration::from_millis(200)) => Ok(WorkSchedule::Unit(())),simulation/amaru-sim/src/simulator/world/node_handle.rs (2)
56-59: Docstring nit: pluralize and clarifyinit_messages.Change “init_message” → “init_messages”; consider briefly stating they're drained once before normal outputs.
- /// * `init_message` is a handle used to receive the chain sync initialization message. + /// * `init_messages` is a handle used to receive chain‑sync initialization messages.
70-77: Drain init channel only once; avoid per‑call overhead and ordering surprises.Currently we drain
init_messageson everyhandle_msgcall. That’s cheap after the first call but semantically odd and can mask late init signals. Drain once and then stick tooutput.- let handle = Box::new(move |msg: Envelope<Msg>| { + let mut init_drained = false; + let handle = Box::new(move |msg: Envelope<Msg>| { info!(msg = ?msg, "enqueuing"); running.enqueue_msg(&input, [msg]); running.run_until_blocked().assert_idle(); - let mut result = init_messages.drain().collect::<Vec<_>>(); + let mut result = if !init_drained { + init_drained = true; + init_messages.drain().collect::<Vec<_>>() + } else { + Vec::new() + }; result.extend(output.drain().collect::<Vec<_>>()); Ok(result) });If other call sites expect init messages on every call, flag it and I’ll adapt this logic behind a flag.
simulation/amaru-sim/src/echo/simulate.rs (2)
32-32: Unused parameter_node_idis fine, but consider documenting why.If node IDs will matter for init/broadcast soon, add a short comment to avoid “why unused?” churn.
62-68: Init channel is never written to; NodeHandle will always drain empty.You create
initbut the echo stage only sends tooutput. Either wire an init emission (e.g., a handshake) or drop the init path here to keep the sim lean.- let (output, rx) = network.output("output", 10); - let (_, init) = network.output("init", 10); + let (output, rx) = network.output("output", 10); @@ - NodeHandle::from_pure_stage(stage.without_state(), init, rx, running).unwrap() + NodeHandle::from_pure_stage(stage.without_state(), /* init */ rx.clone_empty(), rx, running).unwrap()Alternatively: keep
initbut add a one‑shot init send when the stage starts.Do you plan to emit an actual init message from echo soon? If yes, I’ll sketch it.
crates/amaru-consensus/src/consensus/stages/validate_block.rs (2)
184-187: Fix resourceexpectmessage for Rollback effect.The panic text names the wrong effect and trait; use the correct effect and trait to help debugging.
- .expect("ValidateBlockEffect requires a HasBlockValidation resource") + .expect("RollbackBlockEffect requires a CanValidateBlocks resource")
83-90: Minor: error message wording.“Failed to roll forward block” happens in the outer
Errbranch (effect execution failure), not a protocol validation issue. Consider clarifying to “Failed to execute validation effect”.crates/amaru/src/stages/mod.rs (1)
225-232: Network listener defaults: binding 0.0.0.0:3000 in dev can be risky.Good wiring overall. Consider defaulting to 127.0.0.1 or gating external bind behind a config flag/env for local runs.
- listen_address: "0.0.0.0:3000".to_string(), + listen_address: "127.0.0.1:3000".to_string(),If public bind is intentional, we should document auth/rate‑limit expectations for the listener.
crates/amaru-consensus/src/consensus/stages/forward_chain.rs (3)
61-76: Confirm ForwardEventEffect response handling; consider matching inner errors.You only handle outer
Err. IfForwardEventEffectreturnsOk(Err(_))(like other effects), we’ll silently treat delivery failures as success.If
ForwardEventEffect::Response = Result<(), ProcessingFailed>, prefer:- if let Err(e) = eff - .external(ForwardEventEffect::new( - &peer, - ForwardEvent::Forward(header.clone()), - )) - .await - { + if let Err(e) = eff + .external(ForwardEventEffect::new(&peer, ForwardEvent::Forward(header))) + .await + { error!(target: EVENT_TARGET, %e, "failed to send forward event"); eff.send(&processing_errors, ProcessingFailed::new(&peer, anyhow!(e))) .await + } + // else if let Ok(Err(e)) = ... { eff.send(&processing_errors, e).await }If the effect returns a plain
Result<()>, current code is fine—please confirm.
63-66: Tiny: avoid cloningheaderunnecessarily.You can move
headerinto the event after tip update.- ForwardEvent::Forward(header.clone()), + ForwardEvent::Forward(header),
88-103: Mirror the forward path error handling for backward events.Same inner/outer error nuance applies here.
Happy to send a patch once
ForwardEventEffect::Responseis confirmed.crates/amaru/src/stages/consensus/forward_chain/forward_chain.rs (5)
42-64: TCP listener binding might fail - consider more graceful error handlingThe
newconstructor usesexpect("failed to create AcTokio runtime")and.or_panic()for TCP binding. While this might be intentional for initialization failures, it could be harsh in production.Consider propagating errors instead of panicking:
- let runtime = - AcTokio::new("consensus.forward", 1).expect("failed to create AcTokio runtime"); + let runtime = AcTokio::new("consensus.forward", 1) + .map_err(|e| anyhow::anyhow!("Failed to create AcTokio runtime: {}", e))?; let tcp_listener = block_on(&runtime, async move { - TcpListener::bind(&listen_address).await.or_panic() + TcpListener::bind(&listen_address).await })?;
77-82: Mailbox size of 1 million - that's a chunky buffer!Setting the mailbox to 1,000,000 messages is like having a warehouse-sized letterbox! The FIXME comment acknowledges this is temporary, but this could consume significant memory under load.
This needs addressing before production. Would you like me to open an issue to track refactoring this to use pure-stage with proper backpressure handling?
123-132: block_on with 1-second timeout seems arbitraryThe helper function uses a hard-coded 1-second timeout. This could be too short for slow networks or too long for fast local connections.
Consider making the timeout configurable:
-fn block_on<F: Future>(runtime: &AcTokioRuntime, f: F) -> F::Output { +fn block_on<F: Future>(runtime: &AcTokioRuntime, f: F, timeout_secs: u64) -> F::Output { runtime .with_rt(|rt| { let _x = rt.enter(); - rt.block_on(timeout(Duration::from_secs(1), f)) + rt.block_on(timeout(Duration::from_secs(timeout_secs), f)) }) .unwrap() .unwrap() }
89-114: Infinite loop in peer acceptance taskThe peer acceptance loop runs forever without any shutdown mechanism. This could prevent clean shutdowns of the service.
Consider adding a shutdown signal:
+ let shutdown = Arc::new(AtomicBool::new(false)); + let shutdown_clone = shutdown.clone(); runtime.with_rt(|rt| { let our_tip_clone = our_tip.clone(); let clients_clone = clients.clone(); rt.spawn(async move { - loop { + while !shutdown_clone.load(Ordering::Relaxed) { + // Add timeout to accept to check shutdown periodically + let accept_future = PeerServer::accept(&tcp_listener, network_magic); + match timeout(Duration::from_secs(1), accept_future).await {
170-173: Max peers check silently drops connectionsWhen max peers is reached, new connections are silently dropped with just a warning log. The connecting peer won't know why they were rejected.
Consider sending a rejection message to the peer before dropping them, so they know to try again later or find another node.
crates/amaru/src/stages/consensus/forward_chain/client_state.rs (1)
165-173: Test note vs. spec — confirm intended precedence.The comment says “doesn't match documented behaviour (pick first in list)”. Your implementation picks the tallest common ancestor regardless of list order, which is what the spec effectively gives when the client orders points newest→oldest. If tests rely on unordered inputs, flag that expectation explicitly.
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
118-124: Avoid duplicating point→hash helpers across modules.
hash_pointkeeps popping up in different places. Consider centralising it (kernel or a small utils mod) to prevent drift.
307-329: Range fetch unimplemented — explicit error is fine for now.Returning
CannotServeRangeforlb_point != ub_pointis clear. Add a TODO with an issue link when you plan to support ranges.crates/amaru-consensus/src/consensus/events.rs (1)
195-202: Debug label drift: prints “rollback_point” but field isrollback_header.Tiny mismatch that makes logs confusing when you’re chasing forks at 3am. Rename the label.
Apply this diff:
- ValidateBlockEvent::Rollback { - peer, - rollback_header: rollback_point, - .. - } => f - .debug_struct("Rollback") - .field("peer", peer) - .field("rollback_point", rollback_point) - .finish(), + ValidateBlockEvent::Rollback { + peer, + rollback_header, + .. + } => f + .debug_struct("Rollback") + .field("peer", peer) + .field("rollback_header", rollback_header) + .finish(),crates/amaru-consensus/src/consensus/tip.rs (1)
153-178: HeaderTip is a tidy bridge; consider a couple of tiny conveniences.
- Optional:
impl From<Header> for HeaderTipand/orimpl From<&Header> for HeaderTipto mirrorAsHeaderTip.- Optional:
impl CopyifPointisCopyin your kernel; if not, ignore.If
pallas_crypto::hash::Hash<_>doesn’t implementDisplay,Display for HeaderTipwill fail to compile. Confirm or usehex::encode(self.0.hash()).crates/amaru-consensus/src/consensus/effects/network_effects.rs (5)
34-47: Unify point access to avoid clones (optional).
Backwardneeds a clone (tip.point().clone()),Forwarddoesn’t. If clone cost matters, consider returning Cow or adding apoint_ref()to avoid allocations on the hot path.
49-56: Make Display consistent.
Forwardprints the point;Backwardprints the whole tip. Prefer both to show a point for parity.Apply:
impl Display for ForwardEvent { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { match self { - ForwardEvent::Forward(header) => write!(f, "Forward({})", header.point()), - ForwardEvent::Backward(tip) => write!(f, "Backward({})", tip), + ForwardEvent::Forward(header) => write!(f, "Forward({})", header.point()), + ForwardEvent::Backward(tip) => write!(f, "Backward({})", tip.point()), } } }
58-63: Serde path nits.You import
serde::{Serialize, Deserialize}up top but derive with fully-qualifiedserde::...here. Pick one style for consistency.
85-93: Error text: say “event” (covers forward/backward).Tiny copy tweak so logs don’t read oddly when rolling back.
64-71: Builder ergonomics (optional).Consider
impl From<(Peer, ForwardEvent)> for ForwardEventEffectto reduce boilerplate at call sites.crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (7)
20-22: Unused import.
ForwardEventListenerisn’t used here; drop it to keep warnings quiet.-use amaru_consensus::consensus::effects::network_effects::{ForwardEvent, ForwardEventListener}; +use amaru_consensus::consensus::effects::network_effects::ForwardEvent;
99-114: Good: ephemeral bind + wiring. Magic numbers → constants (optional).Binding
127.0.0.1:0is ace. The42and1would read better as named constants.- let listener = TcpForwardEventListener::create( + let listener = TcpForwardEventListener::create( store.clone(), runtime.clone(), tcp_listener, - 42, - 1, + NETWORK_MAGIC, + PROTOCOL_VERSION, header.as_header_tip(), )?;Add near the top (outside this hunk):
const NETWORK_MAGIC: u64 = 42; const PROTOCOL_VERSION: u16 = 1;
127-135: Method doesn’t mutate; return Result instead of panicking.Make it
&selfand bubble errors instead ofunwrap(). Cleaner tests, fewer surprise crashes.- pub fn send_forward(&mut self, s: &str) { + pub fn send_forward(&self, s: &str) -> anyhow::Result<()> { let header = self.store.load_header(&hash(s)).unwrap(); tracing::info!("sending forward event"); - block_on( - self.runtime(), - self.listener.send(ForwardEvent::Forward(header)), - ) - .unwrap(); + block_on(self.runtime(), self.listener.send(ForwardEvent::Forward(header)))?; + Ok(()) }
137-146: Same treatment for rollback sender.Mirror the signature and error handling.
- pub fn send_backward(&mut self, s: &str) { + pub fn send_backward(&self, s: &str) -> anyhow::Result<()> { let rollback_header = self.store.load_header(&hash(s)).unwrap(); tracing::info!("sending rollback event"); - block_on( - self.runtime(), - self.listener - .send(ForwardEvent::Backward(rollback_header.as_header_tip())), - ) - .unwrap(); + block_on( + self.runtime(), + self.listener.send(ForwardEvent::Backward(rollback_header.as_header_tip())), + )?; + Ok(()) }
149-158: Connect path is tidy. Consider constants for42.Same “magic” note as above.
- PeerClient::connect(&format!("127.0.0.1:{}", self.port), 42), + PeerClient::connect(&format!("127.0.0.1:{}", self.port), NETWORK_MAGIC),
186-187: Tiny polish: log or count iterations when debugging flakes (optional).Loop is fine; if CI gets moody, a trace of iterations can help.
268-276: 1s timeout can be brittle on busy CI.Bump it a tad to reduce flaky reds.
- rt.block_on(timeout(Duration::from_secs(1), f)) + rt.block_on(timeout(Duration::from_secs(3), f))crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
377-378: Prefer slice over &Vec in API.Tighten signature to accept any slice.
-pub fn make_best_chains_from_actions(actions: &Vec<Action>) -> Vec<Vec<Chain>> { +pub fn make_best_chains_from_actions(actions: &[Action]) -> Vec<Vec<Chain>> {simulation/amaru-sim/src/simulator/simulate_config.rs (1)
25-25: Guard against zero downstream peers.Zero would be a head-scratcher in the sim. Clamp or enforce via type.
Minimal clamp (quick win):
impl SimulateConfig { pub fn from(args: Args) -> Self { let default = Self::default(); Self { number_of_tests: args.number_of_tests, seed: args.seed.unwrap_or(default.seed), number_of_nodes: args.number_of_nodes, number_of_upstream_peers: args.number_of_upstream_peers, - number_of_downstream_peers: args.number_of_downstream_peers, + number_of_downstream_peers: args.number_of_downstream_peers.max(1), disable_shrinking: args.disable_shrinking, } } @@ - pub fn with_number_of_downstream_peers(mut self, n: u8) -> Self { - self.number_of_downstream_peers = n; + pub fn with_number_of_downstream_peers(mut self, n: u8) -> Self { + self.number_of_downstream_peers = n.max(1); self }Stronger (follow-up): use
NonZeroU8for upstream/downstream in config/args to make “zero” unrepresentable.Also applies to: 36-36, 50-50, 74-77
crates/amaru-consensus/src/consensus/stages/fetch_block.rs (1)
68-77: Rename local for clarity: it’s a header, not a point.
rollback_header: rollback_pointis confusing after the type change.- ValidateHeaderEvent::Rollback { - peer, - rollback_header: rollback_point, - span, - .. - } => { + ValidateHeaderEvent::Rollback { + peer, + rollback_header, + span, + .. + } => { eff.send( &downstream, ValidateBlockEvent::Rollback { peer, - rollback_header: rollback_point, + rollback_header, span, }, ) .await }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (31)
crates/amaru-consensus/src/consensus/effects/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/network_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/events.rs(6 hunks)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs(3 hunks)crates/amaru-consensus/src/consensus/stages/fetch_block.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/forward_chain.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/select_chain.rs(6 hunks)crates/amaru-consensus/src/consensus/stages/validate_block.rs(5 hunks)crates/amaru-consensus/src/consensus/tip.rs(3 hunks)crates/amaru/Cargo.toml(1 hunks)crates/amaru/src/stages/build_stage_graph.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain.rs(0 hunks)crates/amaru/src/stages/consensus/forward_chain/README.md(1 hunks)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain/client_state.rs(5 hunks)crates/amaru/src/stages/consensus/forward_chain/forward_chain.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/mod.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs(7 hunks)crates/amaru/src/stages/consensus/forward_chain/tests.rs(4 hunks)crates/amaru/src/stages/mod.rs(4 hunks)crates/amaru/src/stages/pure_stage_util.rs(1 hunks)crates/pure-stage/src/receiver.rs(1 hunks)simulation/amaru-sim/src/echo/simulate.rs(2 hunks)simulation/amaru-sim/src/simulator/args.rs(1 hunks)simulation/amaru-sim/src/simulator/run.rs(7 hunks)simulation/amaru-sim/src/simulator/simulate.rs(3 hunks)simulation/amaru-sim/src/simulator/simulate_config.rs(4 hunks)simulation/amaru-sim/src/simulator/world/node_handle.rs(2 hunks)simulation/amaru-sim/tests/simulation.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru/src/stages/consensus/forward_chain.rs
🧰 Additional context used
🧠 Learnings (10)
📚 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:
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rscrates/amaru-consensus/src/consensus/stages/forward_chain.rscrates/amaru-consensus/src/consensus/stages/select_chain.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, ValidateHeader::handle_roll_forward returns a Result<PullEvent, ConsensusError>, not ValidateHeaderEvent as might be expected from the older code structure.
Applied to files:
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rscrates/amaru-consensus/src/consensus/stages/forward_chain.rscrates/amaru-consensus/src/consensus/headers_tree/headers_tree.rscrates/amaru-consensus/src/consensus/stages/validate_block.rscrates/amaru-consensus/src/consensus/stages/fetch_block.rscrates/amaru-consensus/src/consensus/stages/select_chain.rscrates/amaru/src/stages/build_stage_graph.rscrates/amaru-consensus/src/consensus/events.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/echo/simulate.rssimulation/amaru-sim/src/simulator/run.rssimulation/amaru-sim/src/simulator/simulate.rs
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
PR: pragma-org/amaru#228
File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128
Timestamp: 2025-05-21T18:58:48.631Z
Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront.
Applied to files:
crates/amaru/src/stages/mod.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, the ValidateHeader::handle_roll_forward method returns a PullEvent, not a ValidateHeaderEvent.
Applied to files:
crates/amaru-consensus/src/consensus/stages/validate_block.rscrates/amaru-consensus/src/consensus/stages/fetch_block.rscrates/amaru-consensus/src/consensus/stages/select_chain.rscrates/amaru-consensus/src/consensus/events.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
PR: pragma-org/amaru#295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In crates/amaru-consensus, flood prevention for duplicate invalid headers should be implemented in the validate_header stage or database layer, not in the store_header stage, since store_header runs before validation and cannot determine header validity.
Applied to files:
crates/amaru/src/stages/build_stage_graph.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
PR: pragma-org/amaru#295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In the amaru consensus pipeline, headers are now stored before validation (store_header stage runs before validate_header stage). The validity state of headers should be tracked in the database to avoid revalidating already-valid headers and to prevent flood attacks with duplicate invalid headers.
Applied to files:
crates/amaru/src/stages/build_stage_graph.rs
📚 Learning: 2025-08-08T14:43:20.218Z
Learnt from: KtorZ
PR: pragma-org/amaru#370
File: crates/amaru-kernel/src/point.rs:45-52
Timestamp: 2025-08-08T14:43:20.218Z
Learning: In crates/amaru-kernel/src/point.rs, Point::Specific stores a header hash that is always exactly 32 bytes by project invariant. Therefore, converting it with `impl From<&Point> for Hash<32>` without a runtime length check is acceptable; future reviews should not request an error-returning check inside this `From` impl. If validation is ever desired, it should be done via `TryFrom` or upstream constructors.
Applied to files:
crates/amaru-consensus/src/consensus/tip.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/tip.rssimulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
PR: pragma-org/amaru#387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
🪛 markdownlint-cli2 (0.17.2)
crates/amaru/src/stages/consensus/forward_chain/README.md
13-13: Spaces inside link text
(MD039, no-space-in-links)
⏰ 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: Test the CLI
- GitHub Check: Benches
- GitHub Check: Build riscv32
- GitHub Check: Build aarch64/linux
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/windows
- GitHub Check: Build x86_64/linux
- GitHub Check: Analyze (rust)
🔇 Additional comments (43)
crates/amaru/src/stages/pure_stage_util.rs (1)
76-86: Recv path removal — no lingering references found.
Repo-wide ripgrep for RecvAdapter and messaging::RecvAdapter returned no matches; removal looks clean. Good to merge, mate.crates/amaru/src/stages/mod.rs (4)
214-218: Tip derivation looks sound; nice Origin fallback.Loading the header then mapping to
HeaderTipwith an Origin fallback is clean.Sanity‑check: in Preprod with a fresh chain DB,
make_chain_storewill panic if tip != Origin and header missing. Tests useInMem(()), so we're safe; please confirm production bootstrap won’t hit this.
242-243: Passingour_tipinto the stage graph is the right move.Keeps tip semantics in one place and avoids coupling to output refs.
257-260: RegisteringResourceForwardEventListenercompletes the wiring.Looks consistent with the new network effects layer.
467-468: Test update to 2 stages acknowledged.Matches the removed in‑process forward stage.
simulation/amaru-sim/src/simulator/world/node_handle.rs (1)
70-77: API behavior change — init messages are now returned before output; verify callers/tests accept init-first orderingRepo scan found only the handler and a single consumer; confirm no callers or tests rely on the previous ordering.
Locations: simulation/amaru-sim/src/simulator/world/node_handle.rs:50-52 (handle_msg), simulation/amaru-sim/src/simulator/world/world.rs:117-119 (caller).simulation/amaru-sim/src/simulator/simulate.rs (3)
118-121: Node ID generation looks bonzer!The node ID generation using
format!("n{}", i)provides a clean, consistent naming scheme for nodes in the simulation. Nice touch passing it to both the map key and the spawn function.
271-271: The _node_id parameter preserves backward compatibility nicelyUsing
_node_idto ignore the parameter is a solid approach - like keeping the old arcade machine running while the new one's being installed. This maintains existing behavior while threading the identifier through.
59-59: Signature change verified — callers updatedAll good, mate — spawn now takes a String and callers were updated; verified at simulation/amaru-sim/src/echo/simulate.rs:32 and simulation/amaru-sim/src/simulator/simulate.rs:271. No remaining old-style spawn() usages found.
crates/amaru-consensus/src/consensus/stages/select_chain.rs (3)
79-86: Header-based rollback is a ripper improvement!Switching from
PointtoHeaderfor rollback operations gives you the full header context right where you need it. This aligns beautifully with the type system approach you've been pushing for - no more fetching headers after the fact!
230-231: Display formatting cleverly preserves the interfaceG'day! Love how you've aliased
rollback_headertorollback_pointin the Display implementation - like wearing vintage threads with modern comfort. This keeps the display output consistent while the internals evolve.Also applies to: 267-268
202-206: Verify downstream consumers: public API change to Fork structThe pub struct Fork<H: IsHeader> now uses rollback_header: H instead of rollback_point: Point (crates/amaru-consensus/src/consensus/stages/select_chain.rs:202). Repo search shows internal uses updated (select_chain.rs, headers_tree.rs, tests) and no .rollback_point matches. Confirm no external crates rely on Fork.rollback_point — if they do, bump the crate major version or add a compatibility shim (deprecated rollback_point accessor or conversion).
crates/amaru/src/stages/build_stage_graph.rs (3)
45-45: New forward_chain stage wired in nicelyThe forward_chain stage slots into the pipeline beautifully, sitting between chain selection and block validation. The wiring with
our_tipand error stages follows the established pattern perfectly.Also applies to: 70-77
31-37: Resolved — build_stage_graph signature change propagatedAll callers updated; found at crates/amaru/src/stages/mod.rs:238 and simulation/amaru-sim/src/simulator/run.rs:163. No remaining references to the old outputs parameter (outputs: StageRef) were found. All green.
78-84: validate_block_stage now consumes forward_chain output — verify the stage signaturesStage function definitions for forward_chain::stage and validate_block::stage were not found in the repo search; confirm forward_chain's output type matches validate_block's input signature by locating and inspecting the stage functions:
# find all `stage` function definitions in the crate rg -n --type rust '^\s*(pub\s+)?(async\s+)?fn\s+stage\s*\(' crates/amaru -S # find files mentioning the modules rg -n --hidden --glob 'crates/amaru/src/**' 'forward_chain|validate_block' -S # inspect the located files to check the signatures sed -n '1,240p' path/to/forward_chain.rs sed -n '1,240p' path/to/validate_block.rscrates/amaru/src/stages/consensus/forward_chain/tests.rs (4)
26-28: Destructuring intersection as (point, tip) - clean!Nice destructuring of the intersection result into
(point, tip). Much clearer than accessing tuple elements by index, like naming your game saves instead of just "Save 1", "Save 2"...
41-42: Consistent migration to Forward-oriented messagingThe switch from
send_validatedtosend_forwardthroughout the tests aligns perfectly with the new forward chain architecture. Like upgrading from dial-up to broadband - same internet, better delivery mechanism!Also applies to: 83-85
46-49: Great comment explaining the timing edge case!Top-notch comment explaining why the tip comes out as chain[6] - the Backward getting sent before the Forward updates the our_tip pointer. This kind of documentation is gold for future maintainers!
22-23: No change needed — .unwrap() is fine in this testSetup::new returns anyhow::Result (crates/amaru/src/stages/consensus/forward_chain/test_infra.rs:92). Project convention prefers explicit panics/unwrapping in tests, so keeping .unwrap() here is acceptable.
crates/amaru/src/stages/consensus/forward_chain/forward_chain.rs (1)
134-154: Clean implementation of ForwardEventListener trait!The implementation handles both Forward and Backward events cleanly, updating the tip atomically and broadcasting to all clients. The use of Mutex for our_tip ensures thread safety. Solid work!
crates/amaru/src/stages/consensus/forward_chain/client_state.rs (2)
47-48: Nice fix on backward matching with pallas points.Comparing
header2.pallas_point()totip.0is the right call and avoids the old tuple gotcha. Clean and tidy.
84-95: Catch-up sequence is correctly “post-intersection only”.Not including the matching ancestor header in
headers(only its children up tostart_header) is the correct Chainsync behaviour.Quick sanity check: do we ever pass
Point::Originasstart_pointwith stores that don't hold a genesis header? If yes,load_header(&hash_point(start_point))?would returnNoneand surface asNoIntersection. Confirm that's acceptable for your genesis bootstrap flow.crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
50-56: ClientOp shape looks good;tip()API makes the FSM simpler.This trims a lot of tuple noise and keeps state updates obvious.
198-213: State machine: good “await→wake” dance.The
waitingflip with immediate drain onnext_op()is solid.crates/amaru-consensus/src/consensus/events.rs (2)
141-155: Header-based rollback API shift looks consistent.Moving from
rollback_pointtorollback_headerhere aligns with the tip refactor elsewhere.
260-268: Handypeer()accessor.This helps flatten match arms all over the place.
simulation/amaru-sim/src/simulator/run.rs (3)
171-193: Wiring looks sound; dual outputs are clear.Using one pure-stage output for InitOk and a separate mpsc channel for forward events keeps test plumbing simple.
117-156: Receiver stage translation is neat.Fwd/Bck mapping to
ChainSyncEventwith consistentSpanis spot on.
66-71: Spawn signature ergonomics are nice.Returning
(input, init_messages, output)mirrors the network handshake flow cleanly.crates/amaru-consensus/src/consensus/tip.rs (1)
53-68: Extrablock_height()on Tip is handy.This makes a bunch of logging and metrics code less chatty.
crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
27-33: Nice abstraction for pluggable delivery.
ResourceForwardEventListenerand the async trait contract look clean and extensible.crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (6)
86-89: Private fields look right.Keeping
runtimeandlistenerprivate in test infra is tidy. No nits.
123-126: Accessor LGTM.Cloning the Arc keeps call sites neat.
172-182: Runtime plumb-through LGTM.API symmetry between Setup and Client is solid.
201-201: Unwrap in tests is fine.Given this is test infra, the panic is acceptable when
Nisn’t met.
214-216: Consistent runtime usage.Good consistency across client helpers.
80-90: Resolved — listener registered in wiring. ResourceForwardEventListener is inserted into Resources at crates/amaru/src/stages/mod.rs:259 and simulation/amaru-sim/src/simulator/run.rs:190; network_effects.rs's get() will find it.crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
429-435: Header-based rollback lookup is correct.Using
fork.rollback_header.hash()aligns with the newForksemantics. Assertion message updated consistently.crates/amaru-consensus/src/consensus/effects/mod.rs (1)
16-16: Good modular split.Re-exporting
network_effectskeeps the surface tidy.crates/amaru-consensus/src/consensus/stages/mod.rs (1)
16-16: Stage registered.
forward_chainis now part of the stage graph. No nits.crates/amaru/src/stages/consensus/forward_chain/mod.rs (1)
15-23: Solid module scaffolding.The split into
client_protocol,client_state, andforward_chainlooks clean; clippy exemption is appropriate for the inception module.If you intend external callers to use
stage/EVENT_TARGETdirectly, consider:+pub use forward_chain::{stage, EVENT_TARGET};simulation/amaru-sim/tests/simulation.rs (1)
38-41: Env-config passthrough looks good.Downstream peers now configurable via
AMARU_NUMBER_OF_DOWNSTREAM_PEERS. Nice and tidy.crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
476-478: Fork now carries a header: good move.Storing
rollback_headerinstead of aPointtightens invariants and simplifies downstream logic.
etorreborre
force-pushed
the
etorreborre/refactor/move-forward-chain-to-pure-stage
branch
4 times, most recently
from
8c5b9fb to
4557416
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (3)
crates/amaru/src/stages/pure_stage_util.rs (1)
62-68: Tokio feature gating forselect!andsleep.Echoing the earlier note: this path needs Tokio's "macros" and "time" features enabled for the crate, or the compiler will sulk like a stormtrooper who missed his shot.
simulation/amaru-sim/src/simulator/run.rs (1)
315-355: Reserve msg_id range once; avoid per-send locking and precompute payload.Prevents interleaving across concurrent sends and trims allocations. You’re close—just grab N ids up-front and build bodies from cached bytes.
- async fn send(&self, event: ForwardEvent) -> anyhow::Result<()> { - fn message(event: &ForwardEvent, msg_id: u64) -> ChainSyncMessage { - match event { - ForwardEvent::Forward(header) => ChainSyncMessage::Fwd { - msg_id, - slot: header.point().slot_or_default(), - hash: Bytes { - bytes: header.hash().as_slice().to_vec(), - }, - header: Bytes { - bytes: to_cbor(&header), - }, - }, - ForwardEvent::Backward(tip) => ChainSyncMessage::Bck { - msg_id, - slot: tip.point().slot_or_default(), - hash: Bytes { - bytes: tip.hash().as_slice().to_vec(), - }, - }, - } - } - - for i in 1..=self.number_of_downstream_peers { - let envelope = { - let mut msg_id = self.msg_id.lock().await; - let envelope = Envelope { - src: self.node_id.clone(), - dest: format!("c{}", i), - body: message(&event, *msg_id), - }; - *msg_id += 1; - envelope - }; - self.sender.send(envelope).await?; - } - Ok(()) - } + async fn send(&self, event: ForwardEvent) -> anyhow::Result<()> { + // Precompute payload pieces once. + let (slot, hash_bytes, header_bytes) = match &event { + ForwardEvent::Forward(header) => ( + header.point().slot_or_default(), + Bytes { bytes: header.hash().as_slice().to_vec() }, + Some(Bytes { bytes: to_cbor(header) }), + ), + ForwardEvent::Backward(tip) => ( + tip.point().slot_or_default(), + Bytes { bytes: tip.hash().as_slice().to_vec() }, + None, + ), + }; + + // Reserve a contiguous block of IDs. + let n = self.number_of_downstream_peers as u64; + let start_id = { + let mut guard = self.msg_id.lock().await; + let s = *guard; + *guard += n; + s + }; + + for i in 0..n { + let dest = format!("c{}", i + 1); + let body = match (&event, &header_bytes) { + (ForwardEvent::Forward(_), Some(hdr)) => ChainSyncMessage::Fwd { + msg_id: start_id + i, + slot, + hash: hash_bytes.clone(), + header: hdr.clone(), + }, + (ForwardEvent::Backward(_), _) => ChainSyncMessage::Bck { + msg_id: start_id + i, + slot, + hash: hash_bytes.clone(), + }, + }; + self.sender.send(Envelope { src: self.node_id.clone(), dest, body }).await?; + } + Ok(()) + }crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
75-98: G'day! That expect() could bring down the whole stage like a house of cardsUsing
.expect()here means if theForwardEventListenerresource isn't registered, the entire stage crashes harder than the stock market in '08. Better to handle this gracefully with a per-peer error instead.Apply this diff to handle the missing resource gracefully:
impl ExternalEffect for ForwardEventEffect { - #[expect(clippy::expect_used)] fn run( self: Box<Self>, resources: Resources, ) -> pure_stage::BoxFuture<'static, Box<dyn pure_stage::SendData>> { Box::pin(async move { - let listener = resources - .get::<ResourceForwardEventListener>() - .expect("ForwardEventEffect requires a ForwardEventListener") - .clone(); - let point = self.event.point(); + let listener = match resources.get::<ResourceForwardEventListener>() { + Some(l) => l.clone(), + None => { + let err = ProcessingFailed::new( + &self.peer, + anyhow!("ForwardEventListener resource missing - like trying to find a drop bear in Dublin"), + ); + return Box::new(Err(err)) as Box<dyn pure_stage::SendData>; + } + }; + let result: <Self as ExternalEffectAPI>::Response = listener.send(self.event).await.map_err(|e| { ProcessingFailed::new( &self.peer, anyhow!("Cannot send the forward event {}: {e}", &point), ) }); Box::new(result) as Box<dyn pure_stage::SendData> }) } }
🧹 Nitpick comments (16)
crates/amaru/src/stages/consensus/forward_chain/README.md (7)
12-13: Fix Markdown link split (MD039) — tighten the acto linkThe link is split across lines and has spacing inside the brackets. Collapse it to avoid markdownlint grief.
Apply this diff:
-The current design therefore splits up the state into non-concurrent pieces and places each piece within an [ -`acto`](https://docs.rs/acto) actor. +The current design therefore splits up the state into non-concurrent pieces and places each piece within an [`acto`](https://docs.rs/acto) actor.
17-21: Clarify the listener ownership sentenceThe phrasing “outside of the actor system supporting clients” is a tad cryptic. Tighten it so future you doesn’t squint at it like a Dark Souls tooltip.
Apply this diff:
-This is why the `TcpForwardChainServer` can be created with -a `TcpListener` initialized outside of the actor system supporting clients. +This is why the `TcpForwardChainServer` accepts a `TcpListener` constructed outside the client-actor system (the server takes ownership of the listener).
24-28: Name the concrete type once for readersSince “PeerServer” is Pallas-specific, drop one precise type path to ground the reader, then keep using the short name. Helps folks not go on a wiki-hunt mid‑read.
Apply this diff:
-The `PeerServer` API bundles miniprotocol instances for each of the supported protocols, but unfortunately, using these +The `PeerServer` (e.g. `pallas_network::miniprotocols::PeerServer`) bundles miniprotocol instances for each supported protocol, but using these
30-38: Note mailbox/backpressure expectationsNice split into
chain_syncandchain_sync_handler. Add one line on mailbox bounds/backpressure so readers know we won’t buffer ourselves into the Shadow Realm if validators are spamming updates.Proposed add after Line 37:
+Both actors use bounded mailboxes; when full, producers apply backpressure to avoid unbounded memory growth.
43-46: Assumption about a single intersect may not always holdChainSync clients can legally issue
FindIntersectmore than once (e.g., after rollbacks or reconnect patterns). Document whether subsequent intersects are supported or explicitly out of scope, and what happens if a cheeky client tries.Apply this wording tweak if multiple intersects are allowed:
-the intersect part of the protocol — which must happen first — within the `chain_sync` actor before handing off the header requests part to +the initial `FindIntersect` — which typically happens first — within the `chain_sync` actor before handing off subsequent `RequestNext` handling toAnd add after Line 46:
+If a client issues a new `FindIntersect` later in the session, the `chain_sync` actor handles it and resets its tracked state accordingly.
48-51: Minor wording + code font for actor groupIf
client_protocolsis a module/actor name, fence it; otherwise, rephrase. Keeps the vibe tidy like a well-sorted inventory.Apply this diff:
-Between the level of per-protocol actors for one client and the overall client supervisor there is an additional level -of per-client supervision in the `client_protocols` actors that serves to bundle the miniprotocols spoken over one +Between per‑protocol actors for a client and the overall client supervisor, there is an additional +per‑client supervisor in the `client_protocols` actors that bundles the miniprotocols spoken over one
1-9: Add a tiny “At a glance” sectionA quick TL;DR with the actor names and responsibilities would help folks onboard faster than a Mass Effect recap.
Proposed insert after Line 5:
+### At a glance +- `TcpForwardChainServer`: accepts clients, supervises per‑client actors +- `chain_sync`: tracks best chain vs. last sent state; processes validation updates +- `chain_sync_handler`: serves ChainSync requests (`RequestNext`, etc.) +- `keepalive_handler`: answers keepalive pingscrates/pure-stage/src/receiver.rs (1)
32-34: MakingReceiver::newpublic — LGTM, mate. Just confirm you’re happy exposingtokio::sync::mpscin the public API.Nice move for external wiring; just be sure the semver/CHANGES reflect the surface area bump and that this coupling to Tokio is intentional. Two tiny polish ideas:
- Add a brief doc comment to set expectations.
- Add a
From<mpsc::Receiver<T>>impl for ergonomic construction.Apply this doc tweak inline:
impl<T> Receiver<T> { - pub fn new(rx: mpsc::Receiver<T>) -> Self { + /// Construct from a Tokio `mpsc::Receiver`. + /// + /// Note: this makes `tokio::sync::mpsc` part of the public API surface. + pub fn new(rx: mpsc::Receiver<T>) -> Self { Self { rx } }And add this helper elsewhere in the file:
impl<T> From<mpsc::Receiver<T>> for Receiver<T> { fn from(rx: mpsc::Receiver<T>) -> Self { Self { rx } } }crates/amaru/src/stages/pure_stage_util.rs (1)
80-85: Preserve error context on failed sends (optional).Mapping all send failures to
Error::NotConnecteddrops useful diagnostics. Consider logging the original error (e.g., viatracing) or wrapping it ifgasket::error::Errorsupports sources. Helps future you when the gremlins show up at 2am.crates/amaru/src/stages/consensus/forward_chain/chain_forward.d2 (2)
51-52: Nit: label reads awkwardly (“accepts task”).Suggest “TcpListener accept loop” or “TcpListener accepts connections” for clarity.
- label: "TcpListener accepts task" + label: "TcpListener accept loop"
54-61: Diagram misses the external effect/resource hop.Add a “ResourceForwardEventListener” box and an arrow from TcpForwardChainServer to it to reflect ForwardEventEffect usage.
stage: { class: component label: "" t: |md ### TcpForwardChainServer tracks current `tip` | } + forward_listener: { + class: component + label: "ResourceForwardEventListener" + } ... - stage -> AcTokio.client_supervisor: "forwards block updates or received connections" + stage -> AcTokio.client_supervisor: "forwards block updates or received connections" + stage -> forward_listener: "sends ForwardEvent via effect"Also applies to: 122-124, 126-126
crates/amaru/src/stages/mod.rs (1)
467-468: Test: avoid brittle stage-count assertions.Hard-coding “2” may flap as we add metrics/aux stages. Prefer asserting on presence by name or “>= 2”.
- assert_eq!(2, stages.len()); + assert!(stages.len() >= 2, "expected at least core stages to start");crates/amaru-consensus/src/consensus/events.rs (1)
195-201: Debug field name mismatch (“rollback_point”).You renamed the field to rollback_header but kept “rollback_point” in Debug output. Either keep it intentionally (compat) with a comment or rename for consistency.
- .field("rollback_point", rollback_point) + .field("rollback_header", rollback_point)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (3)
1-1: Oi, the copyright year's doing time travel!Says 2024 but should be 2025 to match the other files. Unless you've got a DeLorean hidden somewhere?
-// Copyright 2024 PRAGMA +// Copyright 2025 PRAGMA
52-61: That expect() on runtime creation needs better error handlingWhile it's unlikely to fail, using
.expect()for runtime creation could panic the whole initialization. Consider propagating the error up instead - makes debugging easier when things go pear-shaped.- let runtime = - AcTokio::new("consensus.forward", 1).expect("failed to create AcTokio runtime"); + let runtime = AcTokio::new("consensus.forward", 1) + .map_err(|e| anyhow::anyhow!("Failed to create AcTokio runtime: {}", e))?;
86-93: Fair dinkum, that mailbox size workaround needs trackingThe 1,000,000 mailbox size is a band-aid solution that could mask flow control issues. The FIXME comment is good, but consider creating a tracking issue for the pure-stage refactor to ensure this doesn't become permanent technical debt.
Would you like me to create a GitHub issue to track the refactoring of this temporary mailbox size workaround? This'll ensure it doesn't become as permanent as "temporary" government buildings.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/amaru/src/stages/consensus/forward_chain/chain_forward.pngis excluded by!**/*.png
📒 Files selected for processing (27)
crates/amaru-consensus/src/consensus/effects/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/network_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/events.rs(6 hunks)crates/amaru-consensus/src/consensus/stages/forward_chain.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/tip.rs(3 hunks)crates/amaru/Cargo.toml(1 hunks)crates/amaru/src/stages/build_stage_graph.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain.rs(0 hunks)crates/amaru/src/stages/consensus/forward_chain/README.md(1 hunks)crates/amaru/src/stages/consensus/forward_chain/chain_forward.d2(3 hunks)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain/client_state.rs(5 hunks)crates/amaru/src/stages/consensus/forward_chain/mod.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs(7 hunks)crates/amaru/src/stages/consensus/forward_chain/tests.rs(4 hunks)crates/amaru/src/stages/mod.rs(4 hunks)crates/amaru/src/stages/pure_stage_util.rs(1 hunks)crates/pure-stage/src/receiver.rs(1 hunks)simulation/amaru-sim/src/echo/simulate.rs(2 hunks)simulation/amaru-sim/src/simulator/args.rs(1 hunks)simulation/amaru-sim/src/simulator/run.rs(7 hunks)simulation/amaru-sim/src/simulator/simulate.rs(3 hunks)simulation/amaru-sim/src/simulator/simulate_config.rs(4 hunks)simulation/amaru-sim/src/simulator/world/node_handle.rs(2 hunks)simulation/amaru-sim/tests/simulation.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru/src/stages/consensus/forward_chain.rs
🚧 Files skipped from review as they are similar to previous changes (14)
- crates/amaru-consensus/src/consensus/effects/mod.rs
- simulation/amaru-sim/src/simulator/simulate_config.rs
- simulation/amaru-sim/src/echo/simulate.rs
- crates/amaru-consensus/src/consensus/stages/mod.rs
- crates/amaru/Cargo.toml
- crates/amaru/src/stages/consensus/forward_chain/mod.rs
- simulation/amaru-sim/src/simulator/world/node_handle.rs
- simulation/amaru-sim/src/simulator/args.rs
- simulation/amaru-sim/src/simulator/simulate.rs
- crates/amaru/src/stages/consensus/forward_chain/tests.rs
- crates/amaru/src/stages/consensus/forward_chain/client_state.rs
- simulation/amaru-sim/tests/simulation.rs
- crates/amaru-consensus/src/consensus/stages/forward_chain.rs
- crates/amaru-consensus/src/consensus/tip.rs
🧰 Additional context used
🧠 Learnings (11)
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
PR: pragma-org/amaru#228
File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128
Timestamp: 2025-05-21T18:58:48.631Z
Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront.
Applied to files:
crates/amaru/src/stages/mod.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, ValidateHeader::handle_roll_forward returns a Result<PullEvent, ConsensusError>, not ValidateHeaderEvent as might be expected from the older code structure.
Applied to files:
crates/amaru/src/stages/build_stage_graph.rscrates/amaru-consensus/src/consensus/events.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
PR: pragma-org/amaru#295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In crates/amaru-consensus, flood prevention for duplicate invalid headers should be implemented in the validate_header stage or database layer, not in the store_header stage, since store_header runs before validation and cannot determine header validity.
Applied to files:
crates/amaru/src/stages/build_stage_graph.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
PR: pragma-org/amaru#295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In the amaru consensus pipeline, headers are now stored before validation (store_header stage runs before validate_header stage). The validity state of headers should be tracked in the database to avoid revalidating already-valid headers and to prevent flood attacks with duplicate invalid headers.
Applied to files:
crates/amaru/src/stages/build_stage_graph.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/run.rs
📚 Learning: 2025-04-20T17:56:39.223Z
Learnt from: rkuhn
PR: pragma-org/amaru#149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:0-0
Timestamp: 2025-04-20T17:56:39.223Z
Learning: For mpsc::channel in Tokio-based test code, use buffer sizes larger than 1 (e.g., 8) to avoid potential deadlocks when producers send multiple messages before consumers can process them.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-05-09T13:09:47.915Z
Learnt from: rkuhn
PR: pragma-org/amaru#206
File: crates/pure-stage/src/simulation/running.rs:240-242
Timestamp: 2025-05-09T13:09:47.915Z
Learning: Cloning messages in the pure-stage crate should be avoided for performance reasons. The current implementation in SimulationRunning deliberately avoids duplicating message data structures.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-09-05T17:30:55.869Z
Learnt from: etorreborre
PR: pragma-org/amaru#432
File: crates/amaru/src/stages/consensus/clients_block_fetcher.rs:0-0
Timestamp: 2025-09-05T17:30:55.869Z
Learning: In crates/amaru/src/stages/consensus/clients_block_fetcher.rs, the fetch method currently holds a mutex lock across an await operation and lacks timeout handling. The author etorreborre has acknowledged this should be improved later by releasing the lock before awaiting and adding a bounded timeout around the network fetch operation.
Applied to files:
simulation/amaru-sim/src/simulator/run.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:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
PR: pragma-org/amaru#387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, the ValidateHeader::handle_roll_forward method returns a PullEvent, not a ValidateHeaderEvent.
Applied to files:
crates/amaru-consensus/src/consensus/events.rs
🧬 Code graph analysis (8)
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (3)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
client_protocols(134-161)tip(93-98)crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
send(33-33)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (3)
runtime(123-125)runtime(172-174)block_on(268-276)
crates/amaru-consensus/src/consensus/effects/network_effects.rs (3)
crates/amaru-consensus/src/consensus/tip.rs (3)
point(103-108)point(171-173)new(163-165)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (2)
send(149-165)new(53-74)simulation/amaru-sim/src/simulator/run.rs (3)
send(317-354)new(301-312)run(63-88)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (3)
crates/amaru/src/stages/consensus/forward_chain/client_state.rs (1)
find_headers_between(71-104)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (4)
eq(256-266)fmt(236-252)point(76-78)hash(68-70)crates/amaru-kernel/src/point.rs (3)
slot_or_default(27-32)from(59-61)hash(34-40)
crates/amaru/src/stages/mod.rs (2)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
new(92-121)crates/amaru/src/stages/build_stage_graph.rs (1)
build_stage_graph(31-128)
crates/amaru/src/stages/build_stage_graph.rs (2)
crates/amaru-consensus/src/consensus/stages/forward_chain.rs (1)
stage(42-118)crates/amaru-consensus/src/consensus/stages/validate_block.rs (1)
stage(38-119)
simulation/amaru-sim/src/simulator/run.rs (7)
simulation/amaru-sim/src/simulator/simulate_config.rs (2)
default(30-39)from(43-53)crates/pure-stage/src/stagegraph.rs (3)
input(191-191)output(197-217)new(36-39)simulation/amaru-sim/src/simulator/world/node_handle.rs (4)
init_messages(74-74)output(75-75)from_pure_stage(61-82)new(37-48)crates/amaru/src/stages/build_stage_graph.rs (1)
build_stage_graph(31-128)crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
send(33-33)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
send(149-165)crates/pure-stage/src/serde.rs (1)
to_cbor(216-227)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (2)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (3)
tip(93-98)fmt(71-89)fmt(108-115)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (2)
block_on(135-143)create(78-131)
crates/amaru-consensus/src/consensus/events.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
peer(81-86)
🪛 markdownlint-cli2 (0.17.2)
crates/amaru/src/stages/consensus/forward_chain/README.md
13-13: Spaces inside link text
(MD039, no-space-in-links)
⏰ 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). (17)
- GitHub Check: Build aarch64/linux
- GitHub Check: Build riscv32
- GitHub Check: Benches
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/windows
- GitHub Check: Build x86_64/linux
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Test the CLI
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
- GitHub Check: Benches
- GitHub Check: Test the CLI
- GitHub Check: Build x86_64/windows
- GitHub Check: Build riscv32
- GitHub Check: Build aarch64/linux
- GitHub Check: Build x86_64/linux
- GitHub Check: Build aarch64/macos
🔇 Additional comments (16)
crates/amaru/src/stages/pure_stage_util.rs (1)
19-19: LGTM: clean pure_stage wiring.Nice tidy import set – pulling in TokioRunning and BoxFuture here makes the adapter stage read clean and self‑contained. Rock solid.
crates/amaru/src/stages/mod.rs (3)
214-218: Tip derivation fallback looks good.Origin fallback avoids panics when tip isn’t in the store. Nice one.
257-260: Resource wiring LGTM.Effect target registered; forward_chain stage should resolve it fine.
225-232: All good — TcpForwardChainServer::new creates/uses its own Acto runtime (no external tokio handle required).Constructor’s doc and implementation show it starts an Acto runtime and spawns the client supervisor (uses rt.spawn in crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs), so instantiating it before the main tokio Runtime is fine.
crates/amaru/src/stages/build_stage_graph.rs (2)
35-36: Wiring looks consistent with the new pure forward_chain.Passing HeaderTip into forward_chain and using it as downstream for validate_block matches the stage contracts.
Also applies to: 45-46, 70-77, 78-85
70-77: Signature sanity check — OK. forward_chain::State = (HeaderTip, StageRef, StageRef) and pub async fn stage(...) -> State destructures to (our_tip, validation_errors, processing_errors). All green, mate.crates/amaru-consensus/src/consensus/events.rs (2)
151-154: Header-based rollback is a solid move.Field rename to rollback_header aligns with dependent stages.
Also applies to: 168-173
260-268: Nice utility: BlockValidationResult::peer().Removes repetitive matches at call sites.
simulation/amaru-sim/src/simulator/run.rs (3)
66-71: Node spawn plumbing reads clean.Extra init channel makes the handshake explicit. Good stuff.
112-131: Init/InitOk handshake: tidy.Keeps init acks off the main downstream path.
162-169: Resource wiring and queue sizes look sane.Buffered channels (10) help avoid head-of-line blocking in tests.
Also applies to: 171-193
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (3)
261-266: Beauty! The forward header handling looks solidThe implementation properly extracts the header from the
ClientOp::Forwardvariant and sends it along with the tip. Like a well-oiled machine from Mad Max, this'll keep your chain syncing smooth as butter.
278-282: Spot on with the consistent header handlingGood to see the same pattern applied in the await-reply branch. Keeps the code DRY and predictable - no surprises here, just like ordering a flat white in Melbourne.
59-66: Crikey mate, those Tips need proper equality checks!The
PartialEqimplementation forClientOpis comparingTiptuples with pointer equality (&l0.0and&r0.0), which won't work as expected sincePointdoesn't implementCopy. This'll compare memory addresses rather than the actual values, making your equality checks as reliable as a chocolate teapot.Apply this diff to fix the equality comparison:
impl PartialEq for ClientOp { fn eq(&self, other: &Self) -> bool { match (self, other) { - (Self::Backward(l0), Self::Backward(r0)) => (&l0.0, l0.1) == (&r0.0, r0.1), + (Self::Backward(l0), Self::Backward(r0)) => l0.0 == r0.0 && l0.1 == r0.1, (Self::Forward(l0), Self::Forward(r0)) => l0 == r0, _ => false, } } }⛔ Skipped due to learnings
Learnt from: jeluard PR: pragma-org/amaru#69 File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117 Timestamp: 2025-01-21T15:32:17.911Z Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (2)
268-276: Nice work consolidating the block_on helper!The refactored
block_onfunction now takesArc<AcTokio>which aligns perfectly with the new architecture. Clean as a whistle, mate!
102-113: Bonzer TCP listener setup with ephemeral ports!Using port 0 for ephemeral port allocation is the way to go for tests - no more port conflicts! The setup with
TcpForwardChainServer::createandheader.as_header_tip()ties everything together nicely.
etorreborre
force-pushed
the
etorreborre/refactor/move-forward-chain-to-pure-stage
branch
2 times, most recently
from
6e8b4c5 to
166b7b8
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/amaru/src/stages/build_stage_graph.rs (1)
31-37: Update call sites to pass the new args: our_tip and a mutable StageGraphbuild_stage_graph's signature now requires our_tip and a mutable graph; current call sites still omit them — add the two args.
- simulation/amaru-sim/src/simulator/run.rs:163 — include the existing our_tip (declared at ~162) and pass a mutable graph (e.g. our_tip, &mut network).
- crates/amaru/src/stages/mod.rs:239 — include our_tip and &mut network (network is declared as
let mut network = TokioBuilder::default();at ~236).
♻️ Duplicate comments (2)
crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
75-98: Don’t panic if the ForwardEventListener resource is missing; return ProcessingFailed.We called this out earlier; still using expect() here would bring the whole show down if the resource isn’t registered. Bubble a per‑peer ProcessingFailed instead. Like swapping a red-shell for a well-aimed banana, yeah?
Apply:
impl ExternalEffect for ForwardEventEffect { - #[expect(clippy::expect_used)] fn run( self: Box<Self>, resources: Resources, ) -> pure_stage::BoxFuture<'static, Box<dyn pure_stage::SendData>> { Box::pin(async move { - let listener = resources - .get::<ResourceForwardEventListener>() - .expect("ForwardEventEffect requires a ForwardEventListener") - .clone(); - let point = self.event.point(); + let listener = match resources.get::<ResourceForwardEventListener>() { + Some(l) => l.clone(), + None => { + let err = ProcessingFailed::new( + &self.peer, + anyhow!("ForwardEventListener resource missing"), + ); + return Box::new(Err(err)) as Box<dyn pure_stage::SendData>; + } + }; let result: <Self as ExternalEffectAPI>::Response = listener.send(self.event).await.map_err(|e| { ProcessingFailed::new( &self.peer, anyhow!("Cannot send the forward event {}: {e}", &point), ) }); Box::new(result) as Box<dyn pure_stage::SendData> }) } }crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
117-124: Avoid .lock().unwrap() in accept loop; handle poison gracefully.One poisoned lock shouldn’t KO the whole server mid‑sync.
Apply:
- let our_tip = our_tip_clone.lock().unwrap().clone(); + let our_tip = our_tip_clone + .lock() + .map_err(|e| anyhow::anyhow!("Mutex poisoned: {}", e))? + .clone();Note: This block is inside a spawned task; you’ll need to return Result<()> from the task or map the error to a warn and continue. If you prefer to keep infallible, log and skip sending the initial tip on failure.
🧹 Nitpick comments (10)
crates/amaru/src/stages/consensus/forward_chain/README.md (1)
12-13: Fix MD039: no-space-in-links (and avoid split link text across lines).Tidy the acto link to silence markdownlint and improve readability.
Apply:
-The current design therefore splits up the state into non-concurrent pieces and places each piece within an [ -`acto`](https://docs.rs/acto) actor. +The current design therefore splits up the state into non-concurrent pieces and places each piece within an [`acto`](https://docs.rs/acto) actor.crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (3)
125-133: Throttle accept error loop to avoid log storms.On repeated transient failures this will spin and spam logs. Add a tiny backoff.
Apply:
Err(e) => { tracing::warn!( target: EVENT_TARGET, "error accepting peer: {e}", ); - continue; + tokio::time::sleep(std::time::Duration::from_millis(100)).await; + continue; }
50-75: Replace unwrap()/expect() during listener creation; propagate errors instead.Safer than face‑planting when the runtime/adapter isn’t available.
Apply:
- #[expect(clippy::expect_used)] - #[expect(clippy::unwrap_used)] pub fn new( ... - let runtime = - AcTokio::new("consensus.forward", 1).expect("failed to create AcTokio runtime"); + let runtime = AcTokio::new("consensus.forward", 1) + .map_err(|e| anyhow::anyhow!("failed to create AcTokio runtime: {e}"))?; ... - let tcp_listener = runtime - .with_rt(|rt| { - let _e = rt.enter(); // attach to that runtime - tokio::net::TcpListener::from_std(std_listener) - .expect("failed to wrap std listener") - }) - .unwrap(); + let tcp_listener = runtime.with_rt(|rt| { + let _e = rt.enter(); // attach to that runtime + tokio::net::TcpListener::from_std(std_listener) + .map_err(|e| anyhow::anyhow!("failed to wrap std listener: {e}")) + }).ok_or_else(|| anyhow::anyhow!("no runtime handle available"))??;
148-171: Optional: drop lock before broadcasting to clients.Tiny polish: release the mutex before send to minimise lock hold time (even though send is non‑awaiting).
Apply:
ForwardEvent::Forward(header) => { - let mut our_tip = self + let mut guard = self .our_tip .lock() .map_err(|e| anyhow::anyhow!("Mutex poisoned: {}", e))?; - *our_tip = header.as_header_tip(); + *guard = header.as_header_tip(); + drop(guard); self.clients.send(ClientMsg::Op(ClientOp::Forward(header))); Ok(()) } ForwardEvent::Backward(tip) => { - let mut our_tip = self + let mut guard = self .our_tip .lock() .map_err(|e| anyhow::anyhow!("Mutex poisoned: {}", e))?; - *our_tip = tip.clone(); + *guard = tip.clone(); + drop(guard); self.clients.send(ClientMsg::Op(ClientOp::Backward(Tip( tip.point().pallas_point(), tip.block_height(), )))); Ok(()) }simulation/amaru-sim/src/simulator/world/node_handle.rs (2)
57-59: Doc nit: parameter name mismatch ("init_message" vs "init_messages").Keep the docs in lockstep with the signature to avoid head‑scratching.
Apply this diff:
- /// * `init_message` is a handle used to receive the chain sync initialization message. + /// * `init_messages` is a handle used to receive the chain sync initialization message(s).
70-77: Ordering: always drain init before output (by design?)This drains any pending init messages on every call, then regular output. That’s fine if rx1 only ever carries Init/InitOk, but if other signal types sneak in later, this could reorder outputs unexpectedly. If that’s intentional for the handshake, a brief comment here would help future-you.
Apply this diff to document the intent:
running.run_until_blocked().assert_idle(); - let mut result = init_messages.drain().collect::<Vec<_>>(); + // Drain init-handshake messages first (rx1 is reserved for Init/InitOk), + // then normal outputs. This preserves the handshake-before-output contract. + let mut result = init_messages.drain().collect::<Vec<_>>();simulation/amaru-sim/src/simulator/run.rs (1)
315-355: Make msg_id assignment lock-free over await and precompute payload.You already avoid holding the mutex across .await (cheers). You can still reduce contention and allocations by reserving a block of IDs once and precomputing the payload bytes outside the loop.
Apply this diff:
#[async_trait] impl ForwardEventListener for MockForwardEventListener { async fn send(&self, event: ForwardEvent) -> anyhow::Result<()> { - fn message(event: &ForwardEvent, msg_id: u64) -> ChainSyncMessage { - match event { - ForwardEvent::Forward(header) => ChainSyncMessage::Fwd { - msg_id, - slot: header.point().slot_or_default(), - hash: Bytes { - bytes: header.hash().as_slice().to_vec(), - }, - header: Bytes { - bytes: to_cbor(&header), - }, - }, - ForwardEvent::Backward(tip) => ChainSyncMessage::Bck { - msg_id, - slot: tip.point().slot_or_default(), - hash: Bytes { - bytes: tip.hash().as_slice().to_vec(), - }, - }, - } - } - - for i in 1..=self.number_of_downstream_peers { - let envelope = { - let mut msg_id = self.msg_id.lock().await; - let envelope = Envelope { - src: self.node_id.clone(), - dest: format!("c{}", i), - body: message(&event, *msg_id), - }; - *msg_id += 1; - envelope - }; - self.sender.send(envelope).await?; - } + // Precompute payload pieces once. + let (slot, hash_bytes, header_bytes) = match &event { + ForwardEvent::Forward(header) => ( + header.point().slot_or_default(), + Bytes { bytes: header.hash().as_slice().to_vec() }, + Some(Bytes { bytes: to_cbor(header) }), + ), + ForwardEvent::Backward(tip) => ( + tip.point().slot_or_default(), + Bytes { bytes: tip.hash().as_slice().to_vec() }, + None, + ), + }; + + // Reserve a contiguous range of msg_ids. + let n = self.number_of_downstream_peers as u64; + let mut guard = self.msg_id.lock().await; + let start_id = *guard; + *guard = start_id + n; + drop(guard); + + // Send per-recipient messages without holding the lock. + for i in 0..n { + let dest = format!("c{}", i + 1); + let body = match &event { + ForwardEvent::Forward(_) => ChainSyncMessage::Fwd { + msg_id: start_id + i, + slot, + hash: hash_bytes.clone(), + header: header_bytes.clone().expect("header bytes present"), + }, + ForwardEvent::Backward(_) => ChainSyncMessage::Bck { + msg_id: start_id + i, + slot, + hash: hash_bytes.clone(), + }, + }; + self.sender.send(Envelope { src: self.node_id.clone(), dest, body }).await?; + } Ok(()) } }crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (1)
118-123: Use the canonical header hash size constant.Avoid hardcoding 32 — lean on the kernel’s HEADER_HASH_SIZE for resilience if it ever changes.
Apply this diff:
-use amaru_kernel::{Hash, Header, to_cbor}; +use amaru_kernel::{Hash, Header, to_cbor, HEADER_HASH_SIZE}; @@ -pub(crate) fn hash_point(point: &Point) -> Hash<32> { +pub(crate) fn hash_point(point: &Point) -> Hash<HEADER_HASH_SIZE> { match point { - Point::Origin => Hash::from([0; 32]), + Point::Origin => Hash::from([0; HEADER_HASH_SIZE]), Point::Specific(_slot, hash) => Hash::from(hash.as_slice()), } }crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (2)
268-276: Tests may flake with a 1s timeout; make it configurable or bump default.CI can be a bit hungover on Mondays — give it a few more seconds.
Apply this diff:
-fn block_on<F: Future>(runtime: Arc<AcTokio>, f: F) -> F::Output { +fn block_on<F: Future>(runtime: Arc<AcTokio>, f: F) -> F::Output { + let secs: u64 = std::env::var("AMARU_TEST_TIMEOUT_SECS") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(5); runtime .with_rt(|rt| { let _x = rt.enter(); - rt.block_on(timeout(Duration::from_secs(1), f)) + rt.block_on(timeout(Duration::from_secs(secs), f)) }) .unwrap() .unwrap() }
37-44: Minor naming nit: CHAIN_47 points to chain41.json.Not hurting anything, but the mismatch might trip a future refactor. Either rename the const or drop a comment explaining the provenance of that file.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/amaru/src/stages/consensus/forward_chain/chain_forward.pngis excluded by!**/*.png
📒 Files selected for processing (27)
crates/amaru-consensus/src/consensus/effects/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/network_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/events.rs(6 hunks)crates/amaru-consensus/src/consensus/stages/forward_chain.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/tip.rs(3 hunks)crates/amaru/Cargo.toml(1 hunks)crates/amaru/src/stages/build_stage_graph.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain.rs(0 hunks)crates/amaru/src/stages/consensus/forward_chain/README.md(1 hunks)crates/amaru/src/stages/consensus/forward_chain/chain_forward.d2(3 hunks)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain/client_state.rs(5 hunks)crates/amaru/src/stages/consensus/forward_chain/mod.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs(7 hunks)crates/amaru/src/stages/consensus/forward_chain/tests.rs(4 hunks)crates/amaru/src/stages/mod.rs(4 hunks)crates/amaru/src/stages/pure_stage_util.rs(1 hunks)crates/pure-stage/src/receiver.rs(1 hunks)simulation/amaru-sim/src/echo/simulate.rs(2 hunks)simulation/amaru-sim/src/simulator/args.rs(1 hunks)simulation/amaru-sim/src/simulator/run.rs(7 hunks)simulation/amaru-sim/src/simulator/simulate.rs(3 hunks)simulation/amaru-sim/src/simulator/simulate_config.rs(4 hunks)simulation/amaru-sim/src/simulator/world/node_handle.rs(2 hunks)simulation/amaru-sim/tests/simulation.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru/src/stages/consensus/forward_chain.rs
🚧 Files skipped from review as they are similar to previous changes (16)
- crates/amaru/src/stages/consensus/forward_chain/mod.rs
- crates/amaru-consensus/src/consensus/effects/mod.rs
- crates/amaru-consensus/src/consensus/stages/mod.rs
- simulation/amaru-sim/src/simulator/args.rs
- simulation/amaru-sim/src/echo/simulate.rs
- crates/amaru/src/stages/consensus/forward_chain/chain_forward.d2
- crates/amaru-consensus/src/consensus/stages/forward_chain.rs
- crates/pure-stage/src/receiver.rs
- simulation/amaru-sim/src/simulator/simulate_config.rs
- crates/amaru-consensus/src/consensus/events.rs
- crates/amaru/src/stages/pure_stage_util.rs
- simulation/amaru-sim/src/simulator/simulate.rs
- simulation/amaru-sim/tests/simulation.rs
- crates/amaru/Cargo.toml
- crates/amaru/src/stages/mod.rs
- crates/amaru/src/stages/consensus/forward_chain/client_state.rs
🧰 Additional context used
🧠 Learnings (7)
📚 Learning: 2025-09-05T17:30:55.869Z
Learnt from: etorreborre
PR: pragma-org/amaru#432
File: crates/amaru/src/stages/consensus/clients_block_fetcher.rs:0-0
Timestamp: 2025-09-05T17:30:55.869Z
Learning: In crates/amaru/src/stages/consensus/clients_block_fetcher.rs, the fetch method currently holds a mutex lock across an await operation and lacks timeout handling. The author etorreborre has acknowledged this should be improved later by releasing the lock before awaiting and adding a bounded timeout around the network fetch operation.
Applied to files:
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.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:
simulation/amaru-sim/src/simulator/run.rscrates/amaru-consensus/src/consensus/tip.rs
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
PR: pragma-org/amaru#387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-08-08T14:43:20.218Z
Learnt from: KtorZ
PR: pragma-org/amaru#370
File: crates/amaru-kernel/src/point.rs:45-52
Timestamp: 2025-08-08T14:43:20.218Z
Learning: In crates/amaru-kernel/src/point.rs, Point::Specific stores a header hash that is always exactly 32 bytes by project invariant. Therefore, converting it with `impl From<&Point> for Hash<32>` without a runtime length check is acceptable; future reviews should not request an error-returning check inside this `From` impl. If validation is ever desired, it should be done via `TryFrom` or upstream constructors.
Applied to files:
crates/amaru-consensus/src/consensus/tip.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, ValidateHeader::handle_roll_forward returns a Result<PullEvent, ConsensusError>, not ValidateHeaderEvent as might be expected from the older code structure.
Applied to files:
crates/amaru/src/stages/build_stage_graph.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
PR: pragma-org/amaru#295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In crates/amaru-consensus, flood prevention for duplicate invalid headers should be implemented in the validate_header stage or database layer, not in the store_header stage, since store_header runs before validation and cannot determine header validity.
Applied to files:
crates/amaru/src/stages/build_stage_graph.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
PR: pragma-org/amaru#295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In the amaru consensus pipeline, headers are now stored before validation (store_header stage runs before validate_header stage). The validity state of headers should be tracked in the database to avoid revalidating already-valid headers and to prevent flood attacks with duplicate invalid headers.
Applied to files:
crates/amaru/src/stages/build_stage_graph.rs
🧬 Code graph analysis (8)
crates/amaru-consensus/src/consensus/effects/network_effects.rs (3)
crates/amaru-consensus/src/consensus/tip.rs (5)
fmt(35-41)fmt(45-50)fmt(157-159)point(103-108)point(171-173)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
send(148-172)simulation/amaru-sim/src/simulator/run.rs (2)
send(317-354)run(63-88)
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (3)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
client_protocols(134-161)tip(93-98)crates/amaru-consensus/src/consensus/effects/network_effects.rs (2)
new(67-72)send(33-33)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (3)
new(92-121)runtime(123-125)runtime(172-174)
simulation/amaru-sim/src/simulator/run.rs (5)
crates/amaru/src/stages/mod.rs (8)
stages(265-268)stages(285-288)network(248-248)network(249-251)network(252-254)network(255-257)network(258-260)from(152-157)simulation/amaru-sim/src/simulator/world/node_handle.rs (4)
init_messages(74-74)output(75-75)from_pure_stage(61-82)new(37-48)crates/amaru/src/stages/build_stage_graph.rs (1)
build_stage_graph(31-128)crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
send(33-33)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
send(148-172)
crates/amaru-consensus/src/consensus/tip.rs (4)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/test_header.rs (4)
block_height(94-96)hash(47-49)hash(82-84)point(86-88)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (4)
fmt(236-252)hash(68-70)new(92-121)point(76-78)crates/amaru-kernel/src/point.rs (3)
fmt(44-49)fmt(53-55)hash(34-40)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
new(52-84)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (2)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (3)
tip(93-98)fmt(71-89)fmt(108-115)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
create(88-142)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
crates/amaru/src/stages/consensus/forward_chain/client_state.rs (1)
find_headers_between(71-104)crates/amaru-kernel/src/point.rs (4)
slot_or_default(27-32)encode(96-107)from(59-61)hash(34-40)
crates/amaru/src/stages/consensus/forward_chain/tests.rs (2)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (3)
hash(68-70)new(92-121)point(76-78)crates/amaru/src/stages/mod.rs (5)
stages(265-268)stages(285-288)pallas_point(404-404)pallas_point(408-410)pallas_point(414-416)
crates/amaru/src/stages/build_stage_graph.rs (2)
crates/amaru-consensus/src/consensus/stages/forward_chain.rs (1)
stage(42-118)crates/amaru-consensus/src/consensus/stages/validate_block.rs (1)
stage(38-119)
🪛 markdownlint-cli2 (0.17.2)
crates/amaru/src/stages/consensus/forward_chain/README.md
13-13: Spaces inside link text
(MD039, no-space-in-links)
⏰ 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). (7)
- GitHub Check: Benches
- GitHub Check: Build x86_64/windows
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Test the CLI
- GitHub Check: Build riscv32
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (8)
crates/amaru-consensus/src/consensus/tip.rs (2)
53-68: Tip::block_height convenience method looks good.Handy sugar over the IsHeader impl; no correctness concerns spotted. Rock‑solid.
148-188: HeaderTip/AsHeaderTip API reads clean and avoids pallas dep.Nice separation and Display format. This will keep the wire surface leaner. No blockers.
crates/amaru/src/stages/build_stage_graph.rs (1)
70-77: Forward-chain wired before validate_block: LGTM.The stage ordering matches the new ForwardEvent flow; inputs/outputs line up.
crates/amaru/src/stages/consensus/forward_chain/tests.rs (3)
22-33: LGTM: intersection and initial tip checks are spot on.The unwrap on Setup::new() is fine for tests; the lost-tip assertions match the new Forward/Tips flow.
40-59: LGTM: backward then batched forwards, with final-tip semantics.Nice catch on the “await” nuance; asserting the first forward carries the final batch tip reflects the queue semantics accurately.
70-99: LGTM: rollback optimisation test reads well.Assertions align with the client state pushing the latest tip through the batch.
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (1)
50-56: API shape fits the new Forward/Backward model.ClientOp {Backward(Tip), Forward(Header)} plus tip() keeps the state machine tidy. Debug output is helpful.
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
123-135: Nice: Forward/Backward helpers keep tests readable.The server drive via ForwardEvent mirrors production flow neatly.
rkuhn
left a comment
rkuhn
left a comment
There was a problem hiding this comment.
it looked a bit large at first, but is mostly straight-forward — looks good apart from some nitpicks
etorreborre
force-pushed
the
etorreborre/refactor/move-forward-chain-to-pure-stage
branch
from
166b7b8 to
4a2889e
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
371-373: Bug: wrong “old_tip” when building Fork on rollback.You pass rollback_hash as the old tip. The old tip should be the peer’s previous tip (peer_tip), otherwise the intersection (and thus rollback_header) can be wrong when the peer rolls back multiple blocks.
Apply:
- if let Some(best_peer) = self.best_peer() { - let fork = self.make_fork(&best_peer, rollback_hash, &self.best_chain()); + if let Some(best_peer) = self.best_peer() { + let fork = self.make_fork(&best_peer, &peer_tip, &self.best_chain()); Ok(RollbackChainSelection::SwitchToFork(fork)) } else {Consider adding a test that exercises select_rollback producing SwitchToFork and asserts the rollback_header equals the true intersection.
crates/amaru/src/stages/mod.rs (1)
451-472: Test: bind to an ephemeral localhost port to avoid conflicts.Config::default() sets listen_address = "0.0.0.0:3000" and bootstrap starts a TcpListener on that address — override it in the test to prevent port clashes.
Apply:
- let config = Config { + let config = Config { ledger_store: InMem(ledger_store), chain_store: InMem(()), network, + listen_address: "127.0.0.1:0".to_string(), ..Config::default() };If you want to keep the default for production, only override it in tests as shown.
🧹 Nitpick comments (14)
crates/amaru/src/stages/pure_stage_util.rs (1)
79-83: Optional: add a trace on send failure for easier spelunking.You map all failures to
NotConnected, which is fine API-wise. If you want a breadcrumb trail when a sender drops mid‑run, consider a low‑volume trace before mapping the error. Think “debug HUD” rather than noisy logs.- self.0 - .send(msg.payload) - .await - .map_err(|_| gasket::error::Error::NotConnected) + self.0 + .send(msg.payload) + .await + .map_err(|e| { + // Keep it quiet; flip to warn! if you need louder signals in sim runs. + tracing::trace!("pure-stage send failed: {:?}", e); + gasket::error::Error::NotConnected + })simulation/amaru-sim/src/simulator/world/node_handle.rs (1)
54-62: Doc param/name nits.Two tiny polish items:
- “n async function” → “an async function”.
- Param name in docs: use “init_messages” (plural) to match the arg.
Apply:
-/// Additionally, it provides n async function to shutdown the node gracefully. +/// Additionally, it provides an async function to shutdown the node gracefully. @@ - /// * `init_message` is a handle used to receive the chain sync initialization message. + /// * `init_messages` is a handle used to receive the chain sync initialization message(s).simulation/amaru-sim/src/echo/simulate.rs (1)
63-65: Init channel wiring: consistent with NodeHandle.Creating an init output and passing it to NodeHandle matches the new handshake model. All good.
You could rename the local to init_rx for clarity:
- let (_, init) = network.output("init", 10); + let (_, init_rx) = network.output("init", 10); @@ - NodeHandle::from_pure_stage(stage.without_state(), init, rx, running).unwrap() + NodeHandle::from_pure_stage(stage.without_state(), init_rx, rx, running).unwrap()Also applies to: 69-69
crates/amaru/src/stages/consensus/forward_chain/README.md (1)
12-13: Minor Markdown lint and tiny grammar tweaks.Fix the MD039 “spaces inside link text” and a couple of wording nits.
Apply:
-The current design therefore splits up the state into non-concurrent pieces and places each piece within an [ -`acto`](https://docs.rs/acto) actor. +The current design therefore splits up the state into non-concurrent pieces and places each piece within an +[`acto`](https://docs.rs/acto) actor. @@ -For `keepalive` this is trivial because no further information is needed to answer a request. +For `keepalive`, this is trivial because no further information is needed to answer a request. @@ -— which must happen first — within the `chain_sync` actor before handing off the header requests part to +— which must happen first — within the `chain_sync` actor before handing off the header-requests part toAlso applies to: 17-21, 24-32, 35-46, 48-52
crates/amaru-consensus/src/consensus/stages/forward_chain.rs (1)
108-115: The BlockValidationFailed handler is a bit of a wallflower.You're just logging the error but not doing anything with it - no state updates, no downstream notifications. Like watching "The Room" without Tommy Wiseau's commentary - you're missing the full experience! Consider if you need to propagate this failure or update any state.
crates/amaru-consensus/src/consensus/tip.rs (2)
62-67: Redundant API with IsHeader::block_height().This duplicates the method already exposed via the
IsHeaderimpl below. Consider dropping it or delegating to avoid drift.pub fn block_height(&self) -> u64 { - match self { - Tip::Genesis => 0, - Tip::Hdr(h) => h.block_height(), - } + IsHeader::block_height(self) }
180-188: Generalise AsHeaderTip to all IsHeader implementors.This lets you call
as_header_tip()on any header (pallas or kernel) and removes ad‑hoc conversions elsewhere.-pub trait AsHeaderTip { +pub trait AsHeaderTip { fn as_header_tip(&self) -> HeaderTip; } -impl AsHeaderTip for Header { +impl<T: IsHeader> AsHeaderTip for T { fn as_header_tip(&self) -> HeaderTip { - HeaderTip::new(self.point(), self.block_height()) + HeaderTip::new(self.point(), self.block_height()) } }simulation/amaru-sim/src/simulator/run.rs (1)
162-169: Seed our_tip from the store’s anchor, not Origin.Starting at
Point::Origincan cause unnecessary catch‑up. Initialise from the current anchor header instead.- let our_tip = HeaderTip::new(Point::Origin, 0); + let anchor = chain_ref + .load_header(&chain_ref.get_anchor_hash()) + .expect("anchor header present"); + let our_tip = HeaderTip::new(anchor.point(), anchor.block_height());crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
118-123: hash_point duplicates kernel’s Point::hash logic.Optional: convert to kernel
Pointthen call.hash()to keep one source of truth.pub(crate) fn hash_point(point: &Point) -> Hash<32> { - match point { - Point::Origin => Hash::from([0; 32]), - Point::Specific(_slot, hash) => Hash::from(hash.as_slice()), - } + use crate::point::from_network_point; + amaru_kernel::Point::from(from_network_point(point)).hash() }
297-303: variant = 6 is a magic number; consider a named constant.Minor readability win and avoids future head‑scratching.
-pub(super) fn to_header_content(header: Header) -> HeaderContent { +const BABBAGE_VARIANT: u8 = 6; +pub(super) fn to_header_content(header: Header) -> HeaderContent { HeaderContent { - variant: 6, + variant: BABBAGE_VARIANT, byron_prefix: None, cbor: to_cbor(&header), } }crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (4)
35-40: Name/path mismatch: CHAIN_47 points to chain41.json.Tiny papercut — either rename the const or the file to match.
116-123: No need for &mut on send_forward.These methods don’t mutate
Setup. Make them take&self.- pub async fn send_forward(&mut self, s: &str) { + pub async fn send_forward(&self, s: &str) {
125-132: Same here: &self is enough for send_backward.Keeps callers simpler.
- pub async fn send_backward(&mut self, s: &str) { + pub async fn send_backward(&self, s: &str) {
174-187: recv_n panics if fewer than N — OK for tests, but add a note.If you want clearer failures, assert length before
try_into().- ops.try_into().unwrap() + assert_eq!(ops.len(), N, "expected {N} messages, got {}", ops.len()); + ops.try_into().unwrap()
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/amaru/src/stages/consensus/forward_chain/chain_forward.pngis excluded by!**/*.png
📒 Files selected for processing (33)
crates/amaru-consensus/src/consensus/effects/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/network_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/events.rs(6 hunks)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs(3 hunks)crates/amaru-consensus/src/consensus/stages/fetch_block.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/forward_chain.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/select_chain.rs(6 hunks)crates/amaru-consensus/src/consensus/stages/validate_block.rs(5 hunks)crates/amaru-consensus/src/consensus/tip.rs(3 hunks)crates/amaru/Cargo.toml(1 hunks)crates/amaru/src/bin/amaru/cmd/daemon.rs(3 hunks)crates/amaru/src/stages/build_stage_graph.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain.rs(0 hunks)crates/amaru/src/stages/consensus/forward_chain/README.md(1 hunks)crates/amaru/src/stages/consensus/forward_chain/chain_forward.d2(3 hunks)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain/client_state.rs(5 hunks)crates/amaru/src/stages/consensus/forward_chain/mod.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs(6 hunks)crates/amaru/src/stages/consensus/forward_chain/tests.rs(2 hunks)crates/amaru/src/stages/mod.rs(7 hunks)crates/amaru/src/stages/pure_stage_util.rs(1 hunks)crates/pure-stage/src/receiver.rs(1 hunks)simulation/amaru-sim/src/echo/simulate.rs(2 hunks)simulation/amaru-sim/src/simulator/args.rs(1 hunks)simulation/amaru-sim/src/simulator/run.rs(7 hunks)simulation/amaru-sim/src/simulator/simulate.rs(3 hunks)simulation/amaru-sim/src/simulator/simulate_config.rs(4 hunks)simulation/amaru-sim/src/simulator/world/node_handle.rs(2 hunks)simulation/amaru-sim/tests/simulation.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru/src/stages/consensus/forward_chain.rs
🚧 Files skipped from review as they are similar to previous changes (12)
- crates/pure-stage/src/receiver.rs
- simulation/amaru-sim/src/simulator/args.rs
- crates/amaru-consensus/src/consensus/stages/fetch_block.rs
- crates/amaru/src/stages/consensus/forward_chain/mod.rs
- crates/amaru-consensus/src/consensus/effects/mod.rs
- crates/amaru-consensus/src/consensus/effects/network_effects.rs
- crates/amaru-consensus/src/consensus/stages/select_chain.rs
- simulation/amaru-sim/src/simulator/simulate_config.rs
- crates/amaru/src/stages/consensus/forward_chain/chain_forward.d2
- simulation/amaru-sim/tests/simulation.rs
- crates/amaru-consensus/src/consensus/stages/mod.rs
- crates/amaru/Cargo.toml
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, ValidateHeader::handle_roll_forward returns a Result<PullEvent, ConsensusError>, not ValidateHeaderEvent as might be expected from the older code structure.
Applied to files:
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rscrates/amaru-consensus/src/consensus/stages/forward_chain.rscrates/amaru-consensus/src/consensus/events.rscrates/amaru-consensus/src/consensus/stages/validate_block.rscrates/amaru/src/stages/build_stage_graph.rscrates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs
📚 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:
crates/amaru-consensus/src/consensus/stages/forward_chain.rscrates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
PR: pragma-org/amaru#295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In crates/amaru-consensus, flood prevention for duplicate invalid headers should be implemented in the validate_header stage or database layer, not in the store_header stage, since store_header runs before validation and cannot determine header validity.
Applied to files:
crates/amaru-consensus/src/consensus/stages/forward_chain.rscrates/amaru-consensus/src/consensus/stages/validate_block.rscrates/amaru/src/stages/build_stage_graph.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/stages/forward_chain.rssimulation/amaru-sim/src/simulator/run.rscrates/amaru-consensus/src/consensus/tip.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/run.rssimulation/amaru-sim/src/echo/simulate.rssimulation/amaru-sim/src/simulator/simulate.rs
📚 Learning: 2025-08-08T14:34:06.105Z
Learnt from: KtorZ
PR: pragma-org/amaru#370
File: crates/minicbor-extra/src/lib.rs:50-55
Timestamp: 2025-08-08T14:34:06.105Z
Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.
Applied to files:
simulation/amaru-sim/src/simulator/run.rscrates/amaru/src/stages/pure_stage_util.rs
📚 Learning: 2025-08-20T20:18:50.214Z
Learnt from: rkuhn
PR: pragma-org/amaru#384
File: crates/pure-stage/tests/simulation.rs:459-469
Timestamp: 2025-08-20T20:18:50.214Z
Learning: Rust 1.85 (released February 20, 2025) stabilized the std::task::Waker::noop() API. This is now the preferred and correct way to create a no-op waker for testing scenarios, replacing the need for futures_task::noop_waker_ref(). The signature is pub const fn noop() -> &'static Waker and it's specifically designed for use cases like tests where wake notifications aren't needed.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
PR: pragma-org/amaru#387
File: crates/amaru-stores/src/lib.rs:40-40
Timestamp: 2025-08-20T13:02:25.763Z
Learning: In the amaru-stores crate, amaru_slot_arithmetic types like Epoch and EraHistory are used throughout the main crate code in modules like in_memory/mod.rs, rocksdb/consensus.rs, and rocksdb/ledger/columns/, not just in tests. This means amaru-slot-arithmetic should be a regular dependency, not a dev-dependency.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-05-09T13:09:47.915Z
Learnt from: rkuhn
PR: pragma-org/amaru#206
File: crates/pure-stage/src/simulation/running.rs:240-242
Timestamp: 2025-05-09T13:09:47.915Z
Learning: Cloning messages in the pure-stage crate should be avoided for performance reasons. The current implementation in SimulationRunning deliberately avoids duplicating message data structures.
Applied to files:
crates/amaru/src/stages/pure_stage_util.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, the ValidateHeader::handle_roll_forward method returns a PullEvent, not a ValidateHeaderEvent.
Applied to files:
crates/amaru-consensus/src/consensus/events.rscrates/amaru-consensus/src/consensus/stages/validate_block.rs
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
PR: pragma-org/amaru#228
File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128
Timestamp: 2025-05-21T18:58:48.631Z
Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront.
Applied to files:
crates/amaru/src/stages/mod.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
PR: pragma-org/amaru#295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In the amaru consensus pipeline, headers are now stored before validation (store_header stage runs before validate_header stage). The validity state of headers should be tracked in the database to avoid revalidating already-valid headers and to prevent flood attacks with duplicate invalid headers.
Applied to files:
crates/amaru/src/stages/build_stage_graph.rs
📚 Learning: 2025-08-08T14:43:20.218Z
Learnt from: KtorZ
PR: pragma-org/amaru#370
File: crates/amaru-kernel/src/point.rs:45-52
Timestamp: 2025-08-08T14:43:20.218Z
Learning: In crates/amaru-kernel/src/point.rs, Point::Specific stores a header hash that is always exactly 32 bytes by project invariant. Therefore, converting it with `impl From<&Point> for Hash<32>` without a runtime length check is acceptable; future reviews should not request an error-returning check inside this `From` impl. If validation is ever desired, it should be done via `TryFrom` or upstream constructors.
Applied to files:
crates/amaru-consensus/src/consensus/tip.rs
📚 Learning: 2025-09-05T17:30:55.869Z
Learnt from: etorreborre
PR: pragma-org/amaru#432
File: crates/amaru/src/stages/consensus/clients_block_fetcher.rs:0-0
Timestamp: 2025-09-05T17:30:55.869Z
Learning: In crates/amaru/src/stages/consensus/clients_block_fetcher.rs, the fetch method currently holds a mutex lock across an await operation and lacks timeout handling. The author etorreborre has acknowledged this should be improved later by releasing the lock before awaiting and adding a bounded timeout around the network fetch operation.
Applied to files:
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs
🧬 Code graph analysis (16)
crates/amaru-consensus/src/consensus/stages/forward_chain.rs (4)
crates/amaru-consensus/src/consensus/tip.rs (9)
block_height(62-67)block_height(117-122)block_height(175-177)hash(96-101)hash(167-169)new(163-165)point(103-108)point(171-173)slot(124-129)crates/amaru-consensus/src/consensus/effects/network_effects.rs (2)
new(67-72)point(43-48)crates/amaru-consensus/src/consensus/events.rs (2)
peer(101-107)peer(261-267)crates/amaru-kernel/src/point.rs (1)
slot_or_default(27-32)
simulation/amaru-sim/src/simulator/run.rs (6)
simulation/amaru-sim/src/simulator/simulate_config.rs (2)
default(30-39)from(43-53)crates/pure-stage/src/stagegraph.rs (3)
input(191-191)output(197-217)new(36-39)simulation/amaru-sim/src/simulator/world/node_handle.rs (4)
init_messages(74-74)output(75-75)from_pure_stage(61-82)new(37-48)crates/amaru/src/stages/build_stage_graph.rs (1)
build_stage_graph(31-128)crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
send(33-33)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
send(123-147)
simulation/amaru-sim/src/echo/simulate.rs (2)
crates/pure-stage/src/stagegraph.rs (3)
stage(161-170)output(197-217)new(36-39)simulation/amaru-sim/src/simulator/world/node_handle.rs (3)
output(75-75)new(37-48)from_pure_stage(61-82)
crates/amaru/src/bin/amaru/cmd/daemon.rs (1)
crates/amaru/src/stages/mod.rs (1)
bootstrap(160-298)
crates/amaru/src/stages/pure_stage_util.rs (3)
crates/pure-stage/src/tokio.rs (2)
stage(111-132)new(62-70)crates/pure-stage/src/receiver.rs (1)
new(32-34)crates/pure-stage/src/simulation/running.rs (1)
new(78-106)
crates/amaru-consensus/src/consensus/events.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
peer(81-86)
crates/amaru/src/stages/mod.rs (3)
simulation/amaru-sim/src/simulator/run.rs (7)
fetch_block(283-285)new(301-312)network(178-178)network(179-181)network(182-184)network(185-187)network(188-190)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
new(89-114)crates/amaru/src/stages/build_stage_graph.rs (1)
build_stage_graph(31-128)
crates/amaru-consensus/src/consensus/stages/validate_block.rs (3)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (3)
new(36-41)new(79-85)new(123-128)crates/amaru-consensus/src/consensus/effects/block_effects.rs (1)
new(39-41)crates/amaru-consensus/src/consensus/events.rs (2)
peer(101-107)peer(261-267)
crates/amaru/src/stages/build_stage_graph.rs (3)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (1)
tip(93-98)crates/amaru-consensus/src/consensus/stages/forward_chain.rs (1)
stage(42-118)crates/amaru-consensus/src/consensus/stages/validate_block.rs (1)
stage(38-119)
crates/amaru/src/stages/consensus/forward_chain/client_state.rs (3)
crates/amaru/src/stages/mod.rs (5)
stages(267-270)stages(287-290)pallas_point(406-406)pallas_point(410-412)pallas_point(416-418)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
hash_point(118-123)tip(93-98)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (3)
hash(66-68)mk_store(41-64)new(89-114)
crates/amaru-consensus/src/consensus/tip.rs (3)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/test_header.rs (4)
block_height(94-96)hash(47-49)hash(82-84)point(86-88)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (4)
fmt(213-229)hash(66-68)new(89-114)point(74-76)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
new(50-59)
crates/amaru/src/stages/consensus/forward_chain/tests.rs (3)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (3)
hash(66-68)new(89-114)point(74-76)crates/amaru/src/stages/mod.rs (7)
stages(267-270)stages(287-290)pallas_point(406-406)pallas_point(410-412)pallas_point(416-418)as_tip(431-431)as_tip(435-437)crates/amaru/src/stages/consensus/forward_chain/client_state.rs (3)
new(32-34)get_point(209-209)get_point(244-247)
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (3)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
client_protocols(134-161)tip(93-98)crates/amaru-consensus/src/consensus/effects/network_effects.rs (2)
new(67-72)send(33-33)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
new(89-114)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
crates/amaru/src/stages/consensus/forward_chain/client_state.rs (1)
find_headers_between(71-104)crates/amaru-kernel/src/point.rs (3)
slot_or_default(27-32)from(59-61)hash(34-40)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (2)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (3)
tip(93-98)fmt(71-89)fmt(108-115)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
create(63-117)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (3)
crates/amaru-consensus/src/consensus/tip.rs (2)
hash(96-101)hash(167-169)crates/amaru-kernel/src/point.rs (1)
hash(34-40)crates/amaru-consensus/src/consensus/headers_tree/data_generation/test_header.rs (2)
hash(47-49)hash(82-84)
🪛 markdownlint-cli2 (0.18.1)
crates/amaru/src/stages/consensus/forward_chain/README.md
13-13: Spaces inside link text
(MD039, no-space-in-links)
⏰ 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)
- GitHub Check: Build x86_64/windows
- GitHub Check: Benches
- GitHub Check: Build wasm32
- GitHub Check: Build aarch64/macos
- GitHub Check: Build riscv32
- GitHub Check: Test the CLI
- GitHub Check: Build aarch64/linux
- GitHub Check: Build x86_64/linux
- GitHub Check: Test coverage
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: clippy
- GitHub Check: Analyze (rust)
🔇 Additional comments (48)
crates/amaru/src/stages/pure_stage_util.rs (3)
19-19: Nice pivot to TokioRunning; keeps the runtime alive with minimal surface.Importing
tokio::TokioRunningfrompure_stageand holding it on the struct is a clean way to keep tasks ticking without dragging in a fullRuntimehandle. Also, field order means_tokio_runningdrops aftertermination, which is what you want. Lovely.
21-21: Tokio feature gating still needed forselect!andsleep.Using
tokio::select!andtokio::time::sleeprequires enabling Tokio featuresmacrosandtimefor this crate. Please confirm they’re enabled (workspace or crate-level).#!/bin/bash set -euo pipefail # Show Tokio dependency declarations for the amaru crate and workspace root. for f in crates/amaru/Cargo.toml Cargo.toml; do if [[ -f "$f" ]]; then echo "---- $f" # Print the tokio dep line(s) with nearby context rg -nC2 -e '^\s*tokio\s*=' -e 'tokio\s*=\s*\{[^}]*\}' "$f" || true # Heuristic check for macros/time features on that dep rg -nP -C0 'tokio\s*=\s*\{[^}]*features\s*=\s*\[[^]]*(macros|time)[^]]*\]' "$f" || echo "⚠️ Could not find both macros/time features in $f" fi done
33-39: Constructor simplification looks spot on.
new(tokio_running, exit)withtermination = tokio_running.termination()wires shutdown cleanly, and the stored guard_tokio_runningensures liveness. This reads tidy and robust—no extra moving parts.crates/amaru/src/bin/amaru/cmd/daemon.rs (3)
34-35: Clap attr reflow looks good.Pure formatting; no change in behaviour. Keep this multiline style consistent across all Args fields for readability.
59-61: Consistent multiline attr formatting.Same here—tidier, no semantic shift. Ship it.
102-102: Correctly awaiting the now-asyncbootstrap— re-run check (rg PCRE2 failed)Nice one — this ensures stages are ready and start-up errors surface early. Your rg run hit a PCRE2 error; run the safer script below from the repo root to list any
bootstrap(...)calls not immediately followed by.await.#!/bin/bash set -euo pipefail # From repo root: find bootstrap(...) occurrences and report ones that don't have `.await` before the statement semicolon rg -n --type=rust 'bootstrap\(' -g '!target/**' > /tmp/bootstrap_matches || true if [ ! -s /tmp/bootstrap_matches ]; then echo "No bootstrap(...) occurrences found." exit 0 fi python3 - <<'PY' import re, sys from pathlib import Path with open('/tmp/bootstrap_matches', 'r') as f: hits = [l.rstrip('\n') for l in f if l.strip()] results = [] for line in hits: m = re.match(r'([^:]+):(\d+):', line) if not m: continue path, lineno = m.group(1), int(m.group(2)) txt = Path(path).read_text(encoding='utf-8') file_lines = txt.splitlines(True) start_idx = sum(len(l) for l in file_lines[:lineno-1]) line_text = file_lines[lineno-1] for mo in re.finditer(r'bootstrap\s*\(', line_text): pos_in_line = mo.start() pos = start_idx + pos_in_line i = txt.find('(', pos) if i == -1: continue depth = 0 n = len(txt) while i < n: c = txt[i] if c == '(': depth += 1 elif c == ')': depth -= 1 if depth == 0: j = i + 1 next_semi = txt.find(';', j) segment = txt[j:next_semi] if next_semi != -1 else txt[j:j+200] if '.await' not in segment: start_line = max(0, lineno-2) end_line = min(len(file_lines), lineno+3) snippet = ''.join(file_lines[start_line:end_line]) results.append((path, lineno, snippet)) break i += 1 if not results: print("All bootstrap call sites appear awaited ✅") else: for path, lineno, snippet in results: print(f"{path}:{lineno}\n{snippet}\n---\n") PYcrates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
429-435: Header-based rollback lookup: nice, aligns with Fork.rollback_header.Switching from point.hash() to rollback_header.hash() is correct and consistent with the new API.
simulation/amaru-sim/src/simulator/world/node_handle.rs (1)
70-77: Drain init then output: behaviour change acknowledged.Sequencing init_messages before output is sensible for the handshake flow. No functional concerns here.
If any stage emits multiple init bursts, confirm callers tolerate repeated empty drains (they will just get an empty vec). No action needed if that’s intended.
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
474-479: Fork now carries rollback_header: good upgrade.The change to store the intersection header (not point) in Fork improves type safety and matches the rest of the refactor.
simulation/amaru-sim/src/echo/simulate.rs (1)
32-32: API bump to accept node_id: SGTM.Signature change to take a node id threads through the simulator cleanly. No issues.
simulation/amaru-sim/src/simulator/simulate.rs (2)
59-60: Propagating Fn(String) for spawners: yes.Per-node identifiers will help with logging and addressing. Looks tidy.
118-122: Tests updated to new spawn signature: tidy.The test harness passes node ids and adapts the blackbox spawner. No further comments.
Also applies to: 271-274
crates/amaru/src/stages/build_stage_graph.rs (2)
17-23: Graph API changes and imports: aligned.Moving to a tip-aware graph builder and bringing in forward_chain aligns with the new architecture.
Also applies to: 35-37
70-77: Forward-chain wiring: correct ordering and state.validate_block now emits into forward_chain, with our_tip and error sinks provided. This matches forward_chain::stage’s State and Msg types.
Double-check initial our_tip for the genesis case: forward_chain::stage asserts parent/height continuity. With our_tip at Origin/0, the first BlockValidated must be the genesis successor.
Also applies to: 81-84
crates/amaru/src/stages/mod.rs (1)
241-247: Pure-stage network and resource wiring: looks solid.
- Passing our_tip into the graph.
- Registering ResourceForwardEventListener.
- Running network with current Handle.
All reads clean.Also applies to: 250-263, 264-266
crates/amaru-consensus/src/consensus/stages/validate_block.rs (2)
58-64: Looking good, mate! Header-based approach is spot on.The simplified
BlockValidatedevent structure with just peer, header, and span is cleaner. Nice work removing the redundant block data since the header carries all the essential info anyway.
169-173: Sweet refactor to use Header instead of Point!This change aligns nicely with the broader shift to header-based rollback targets across the codebase. The header gives you more context than just the point, which is ace.
crates/amaru/src/stages/consensus/forward_chain/client_state.rs (3)
47-47: Nice update to the Forward variant matching!The simplified
Forward(header)pattern aligns perfectly with the new single-header storage approach. Much cleaner than the previous dual-field setup.
84-94: Solid refactor of the headers collection, cobber!Using
ClientOp::Forward(header.clone())directly is much more intuitive than the old approach. The logic flows like a well-crafted Wes Anderson sequence - symmetrical and purposeful.
106-254: Ripper test suite addition!Love seeing comprehensive test coverage here. The
ChainStoreExttrait is particularly clever - gives you all those handy helper methods without cluttering the main implementation. The test scenarios cover the key paths nicely: tip-to-tip, branching, and the "lost" scenario. This is like the Director's Cut with all the deleted scenes restored!crates/amaru-consensus/src/consensus/events.rs (2)
151-151: Header-based rollbacks are the way forward!Brilliant move replacing
rollback_point: Pointwithrollback_header: Headeracross all these event types. You get way more context with the full header - it's like upgrading from VHS to 4K Blu-ray!Also applies to: 169-169, 254-254
260-268: Handy peer accessor method!This
peer()method is a nice touch - saves everyone from writing the same match statement over and over. DRY principle at its finest!crates/amaru/src/stages/consensus/forward_chain/tests.rs (2)
20-30: Async test migration looking schmick!The move to
#[tokio::test]and async/await patterns is spot on. The setup flow reads much better now withSetup::new(LOST_47).await.unwrap(). Just like upgrading from dial-up to fiber - everything's smoother!
44-47: That comment about the Backward/Forward race is gold!Good on ya for documenting that timing quirk where the Backward gets sent before the Forward updates
our_tip. These kinds of edge cases are like Easter eggs in video games - easy to miss but important to know about!crates/amaru-consensus/src/consensus/stages/forward_chain.rs (1)
51-56: Crikey! These asserts are like landmines in your consensus path.G'day mate, those
assert_eq!calls are gonna panic your node faster than you can say "Donnie Darko". In a consensus-critical path, we need graceful handling, not crashes. Consider validating and logging instead:- // assert that the new tip is a direct successor of the old tip - assert_eq!(header.block_height(), our_tip.block_height() + 1); - match header.parent() { - Some(parent) => assert_eq!(parent, our_tip.hash()), - None => assert_eq!(our_tip, HeaderTip::new(Point::Origin, 0)), - } + // validate that the new tip is a direct successor of the old tip + let expected_height = our_tip.block_height() + 1; + let parent_valid = match header.parent() { + Some(parent) => parent == our_tip.hash(), + None => our_tip == HeaderTip::new(Point::Origin, 0), + }; + + if header.block_height() != expected_height || !parent_valid { + error!( + target: EVENT_TARGET, + "Non-adjacent header received: height={}, expected={}, parent_valid={}", + header.block_height(), + expected_height, + parent_valid + ); + eff.send(&validation_errors, ValidationFailed::new(&peer, + ConsensusError::InvalidBlock { + peer: peer.clone(), + point: header.point(), + })) + .await; + return (our_tip, validation_errors, processing_errors); + }crates/amaru-consensus/src/consensus/tip.rs (4)
53-61: Nice convenience accessor.
to_header()is tidy and does what it says on the tin.
148-155: HeaderTip: good decoupling from pallas types.The tuple layout matches chainsync’s
(Point, height)expectation.
156-160: Ensure hex formatting is stable.
Displayrelies onHash’sDisplayimpl. If you need guaranteed hex (no surprises), encode explicitly.- write!(f, "{}.{}", self.1, self.0.hash()) + write!(f, "{}.{}", self.1, hex::encode(self.0.hash()))
162-178: Accessors look right.Hash/point/height wiring is correct and cheap.
simulation/amaru-sim/src/simulator/run.rs (5)
66-71: spawn closure wiring is clean.Returning
(input, init_messages, output)then wrapping withNodeHandle::from_pure_stagereads well.
111-160: Init/InitOk handshake: solid.Fast pathing Init→InitOk and routing Fwd/Bck to ChainSyncEvent is correct.
171-193: Resource injection looks right.ForwardEventListener is plumbed via resources and outputs are split as intended.
288-312: Use AtomicU64 for msg_id; drop the async mutex.Cheaper, contention‑free, and avoids lock hop in hot path.
-#[derive(Clone, Debug)] +#[derive(Clone, Debug)] pub struct MockForwardEventListener { node_id: String, number_of_downstream_peers: u8, sender: mpsc::Sender<Envelope<ChainSyncMessage>>, - msg_id: Arc<Mutex<u64>>, + msg_id: Arc<std::sync::atomic::AtomicU64>, } impl MockForwardEventListener { pub fn new( node_id: String, number_of_downstream_peers: u8, sender: mpsc::Sender<Envelope<ChainSyncMessage>>, ) -> Self { Self { node_id, number_of_downstream_peers, - msg_id: Arc::new(Mutex::new(0)), + msg_id: Arc::new(std::sync::atomic::AtomicU64::new(0)), sender, } } }
315-354: Precompute payloads and reserve msg_id block once.Avoid per‑recipient CBOR/hash allocations and lock hops; ensure unique, monotonic ids.
#[async_trait] impl ForwardEventListener for MockForwardEventListener { async fn send(&self, event: ForwardEvent) -> anyhow::Result<()> { - fn message(event: &ForwardEvent, msg_id: u64) -> ChainSyncMessage { - match event { - ForwardEvent::Forward(header) => ChainSyncMessage::Fwd { - msg_id, - slot: header.point().slot_or_default(), - hash: Bytes { - bytes: header.hash().as_slice().to_vec(), - }, - header: Bytes { - bytes: to_cbor(&header), - }, - }, - ForwardEvent::Backward(tip) => ChainSyncMessage::Bck { - msg_id, - slot: tip.point().slot_or_default(), - hash: Bytes { - bytes: tip.hash().as_slice().to_vec(), - }, - }, - } - } - - for i in 1..=self.number_of_downstream_peers { - let envelope = { - let mut msg_id = self.msg_id.lock().await; - let envelope = Envelope { - src: self.node_id.clone(), - dest: format!("c{}", i), - body: message(&event, *msg_id), - }; - *msg_id += 1; - envelope - }; - self.sender.send(envelope).await?; - } + use std::sync::atomic::Ordering::Relaxed; + let (slot, hash_bytes, header_bytes) = match &event { + ForwardEvent::Forward(h) => ( + h.point().slot_or_default(), + Bytes { bytes: h.hash().as_slice().to_vec() }, + Some(Bytes { bytes: to_cbor(h) }), + ), + ForwardEvent::Backward(t) => ( + t.point().slot_or_default(), + Bytes { bytes: t.hash().as_slice().to_vec() }, + None, + ), + }; + let n = self.number_of_downstream_peers as u64; + let start = self.msg_id.fetch_add(n, Relaxed); + for i in 0..n { + let body = match (&event, &header_bytes) { + (ForwardEvent::Forward(_), Some(hdr)) => ChainSyncMessage::Fwd { + msg_id: start + i, slot, hash: hash_bytes.clone(), header: hdr.clone(), + }, + (ForwardEvent::Backward(_), _) => ChainSyncMessage::Bck { + msg_id: start + i, slot, hash: hash_bytes.clone(), + }, + _ => unreachable!(), + }; + self.sender + .send(Envelope { src: self.node_id.clone(), dest: format!("c{}", i + 1), body }) + .await?; + } Ok(()) } }crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (6)
50-66: ClientOp shape is sensible.Backwards carries the tip; forward carries the header. Eq impl matches semantics.
70-99: Clear Debug and tip() helper.Handy for tracing; tuple accessors are correct.
105-116: PrettyPoint: neat formatting.Slot + hex hash is exactly what you want in logs.
134-161: Protocol fan‑out is tidy.Spawning per‑miniprotocol actors with a shared store/our_tip reads clean.
170-197: chainsync: intersection handling is correct.Early‑exit on missing intersection and reporting via server is spot on.
243-295: ReqNext handler: good await semantics.The None→Await path and late delivery handling look robust.
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (4)
120-147: Forward/Backward: state update + broadcast is correct.Mutex not held across awaits and broadcast goes via supervisor — good.
150-186: Client supervisor loop is fine; shutdown path might never trigger.
has_senders()may stay true while the listener holds anActoRef. Add an explicit shutdownClientMsgif you need graceful stop.
72-83: Mailbox 1_000_000: big memory footgun — make it configurable or coalesce.Great for demos, risky IRL. Bound sensibly and add simple coalescing/backpressure.
- .with_mailbox_size(1_000_000) + .with_mailbox_size(std::env::var("AMARU_FWD_MAILBOX") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(4096))
85-100: Poison‑safe lock in accept loop.This
.unwrap()can still panic on a poisoned mutex; match the safer handling used below.- let our_tip = our_tip_clone.lock().unwrap().clone(); + let our_tip = our_tip_clone + .lock() + .map_err(|e| anyhow::anyhow!("Mutex poisoned: {}", e)) + .expect("our_tip lock") // crash with context only if truly unrecoverable + .clone();crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (4)
82-86: Setup struct looks good.Holds just what the tests need.
89-114: Async Setup::new is clean.Listener creation + server bootstrap are tidy.
134-139: connect() is sweet and simple.Happy path for tests.
212-230: Debug impls are great for tracing.The PrettyPoint formatting shines here.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
simulation/amaru-sim/src/simulator/run.rs (1)
58-58: Use tracing instead of println for msg_id debug.Add trace to the import and keep logs consistent with the rest of the sim.
- use tracing::{Span, info}; + use tracing::{Span, info, trace};
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs(1 hunks)simulation/amaru-sim/src/lib.rs(1 hunks)simulation/amaru-sim/src/simulator/run.rs(7 hunks)
✅ Files skipped from review due to trivial changes (1)
- simulation/amaru-sim/src/lib.rs
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-05T17:30:55.869Z
Learnt from: etorreborre
PR: pragma-org/amaru#432
File: crates/amaru/src/stages/consensus/clients_block_fetcher.rs:0-0
Timestamp: 2025-09-05T17:30:55.869Z
Learning: In crates/amaru/src/stages/consensus/clients_block_fetcher.rs, the fetch method currently holds a mutex lock across an await operation and lacks timeout handling. The author etorreborre has acknowledged this should be improved later by releasing the lock before awaiting and adding a bounded timeout around the network fetch operation.
Applied to files:
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.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/run.rs
📚 Learning: 2025-08-08T14:34:06.105Z
Learnt from: KtorZ
PR: pragma-org/amaru#370
File: crates/minicbor-extra/src/lib.rs:50-55
Timestamp: 2025-08-08T14:34:06.105Z
Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-08-20T20:18:50.214Z
Learnt from: rkuhn
PR: pragma-org/amaru#384
File: crates/pure-stage/tests/simulation.rs:459-469
Timestamp: 2025-08-20T20:18:50.214Z
Learning: Rust 1.85 (released February 20, 2025) stabilized the std::task::Waker::noop() API. This is now the preferred and correct way to create a no-op waker for testing scenarios, replacing the need for futures_task::noop_waker_ref(). The signature is pub const fn noop() -> &'static Waker and it's specifically designed for use cases like tests where wake notifications aren't needed.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
🧬 Code graph analysis (2)
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (3)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
client_protocols(134-161)tip(93-98)crates/amaru-consensus/src/consensus/effects/network_effects.rs (2)
new(67-72)send(33-33)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
new(89-114)
simulation/amaru-sim/src/simulator/run.rs (6)
simulation/amaru-sim/src/simulator/simulate_config.rs (2)
default(30-39)from(43-53)crates/pure-stage/src/stagegraph.rs (3)
input(191-191)output(197-217)new(36-39)simulation/amaru-sim/src/simulator/world/node_handle.rs (4)
init_messages(74-74)output(75-75)from_pure_stage(61-82)new(37-48)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (2)
new(50-59)send(126-153)crates/amaru/src/stages/build_stage_graph.rs (1)
build_stage_graph(31-128)crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
send(33-33)
⏰ 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)
- GitHub Check: Benches
- GitHub Check: Test the CLI
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Build aarch64/macos
- GitHub Check: Build riscv32
- GitHub Check: Build wasm32
- GitHub Check: clippy
- GitHub Check: Build x86_64/windows
- GitHub Check: Build x86_64/linux
- GitHub Check: Build aarch64/linux
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (15)
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (7)
32-32: G'day mate! Love the event target constant - proper Aussie organisation that is!Having a dedicated constant for event targeting is brilliant practice, like having your own bar at the cricket. Makes debugging and log filtering a breeze!
34-42: TcpForwardChainServer structure looks solid, fair dinkum!Clean separation of concerns here - the Arc-wrapped tip for shared state, the ActoRef for client management, and the runtime handle. Like a well-organized toolshed, everything has its place.
44-59: Constructors are well thought out - love the builder pattern vibes!The async
newmethod handling the TCP binding internally whilecreateaccepts an external listener is a nice touch. Gives users flexibility like choosing between VB and Carlton - both get the job done!
72-83: That 1M mailbox size comment tells the whole story - but production might not be as forgiving as a demo crowd!The comment acknowledges this is a temporary solution, which is fair dinkum. But that million-message mailbox is like having a massive esky at a house party - great until someone has to clean up the mess!
As mentioned in previous reviews, this massive mailbox poses memory risks in production. The comment explains it's temporary, but consider making it configurable sooner rather than later.
86-113: Connection handling looks mint, but that spawn pattern is interesting!The tokio::spawn for accepting connections makes sense since you can't poll TcpListener::accept within the schedule method. It's like having a bouncer at the pub - someone's gotta be watching the door while the party's happening inside.
The mutex handling for our_tip looks good with the proper
.expect()usage as suggested in previous reviews.
126-154: Forward event handling is clean as a whistle!Both Forward and Backward event handling look solid. The mutex is properly handled with scoped blocks to avoid holding locks longer than needed - like not hogging the remote during ad breaks! The error handling for poisoned mutexes is spot on.
156-192: Client supervisor is well-architected - proper actor pattern implementation!The max_peers check prevents overload (like a venue capacity limit), and the supervision handling with cleanup on termination is textbook stuff. The message routing to all connected clients is efficient and clean.
One small observation: Line 162's
has_senders()check means this loop continues forever since the ActoRef in the listener task keeps senders alive, but that's by design for a server component.simulation/amaru-sim/src/simulator/run.rs (8)
24-26: New network-effects wiring looks sound.Imports for ForwardEvent{,Listener}, ResourceForwardEventListener, ChainSyncEvent, and HeaderTip align with the PR’s architectural shift. No nits here.
Also applies to: 31-31, 38-38
67-73: Spawn closure refactor reads well.Switch to passing node_id and threading init_messages into NodeHandle matches the updated handle API. Nice and tidy, mate.
71-72: Double-check NodeHandle tuple order.Confirm NodeHandle::from_pure_stage expects (input, init_messages, output, running) in that exact order. Looks consistent with world/node_handle.rs, but worth a quick sanity check.
99-106: spawn_node signature change — OK.Returning (input, init_messages, output) is clear and matches the new wiring.
112-161: Init handshake and event mapping are on point.
- Replying InitOk with in_reply_to is correct.
- Fwd/Bck → RollForward/Rollback mapping preserves peer and point info properly.
Ship it.
163-169: Initial tip set to Origin(0).Sane default for sim. If tests ever start mid-chain, just ensure this seed tip is overridden accordingly.
172-176: Channel plumbing looks good.output/rx1 for InitOk and a separate mpsc channel for downstream broadcasts keeps concerns clean. Nice separation.
189-191: Injecting ResourceForwardEventListener — LGTM.Resources wiring aligns with the new pure-stage external effect.
…esult::BlockValidated Signed-off-by: etorreborre <etorreborre@yahoo.com> | Conflicts: | crates/amaru/src/stages/consensus/forward_chain.rs
…vent Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
…it again 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>
Signed-off-by: etorreborre <etorreborre@yahoo.com> | Conflicts: | crates/amaru/src/bin/amaru/cmd/daemon.rs
etorreborre
force-pushed
the
etorreborre/refactor/move-forward-chain-to-pure-stage
branch
from
60874ac to
177a573
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
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/echo/simulate.rs (1)
40-44: Avoidis_multiple_of— use modulo for portability.
is_multiple_ofisn’t a stable inherent method on integers in std and will likely fail without an external trait in scope. Modulo is clear and bulletproof.Apply this diff:
- let echo_response = if state.0.is_multiple_of(5) { + let echo_response = if state.0 % 5 == 0 { echo.to_string().to_uppercase() } else { echo.to_string() };
🧹 Nitpick comments (20)
simulation/amaru-sim/src/simulator/run.rs (1)
163-171: Initial tip set to Origin — consider deriving from storeIf the store is already anchored beyond Origin, seed our_tip from it to avoid sending a stale tip to freshly connected peers.
Happy to sketch the snippet if the store API exposes current tip/height.
crates/amaru/src/stages/consensus/forward_chain/README.md (1)
12-13: Fix the broken Markdown link (MD039).The link label splits across lines and triggers markdownlint. Collapse to a single inline link.
Apply this diff:
-The current design therefore splits up the state into non-concurrent pieces and places each piece within an [ -`acto`](https://docs.rs/acto) actor. +The current design therefore splits up the state into non-concurrent pieces and places each piece within an [`acto`](https://docs.rs/acto) actor.crates/amaru/src/stages/pure_stage_util.rs (1)
60-66: Minor: prefer interval over repeated sleep to avoid drift.Functionally fine, but
tokio::time::intervalwould avoid tick drift and needless re-allocation of sleeps.Example:
- _ = sleep(Duration::from_secs(1)) => Ok(WorkSchedule::Unit(())), + _ = interval.tick() => Ok(WorkSchedule::Unit(())),(Initialize
let mut interval = tokio::time::interval(Duration::from_secs(1));before the loop.)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
474-478: Switch to rollback_header is consistent; document the semantics.The fork now carries the LCA header (intersection) as
rollback_header. Code and tests look consistent. Consider clarifying the make_fork doc thatrollback_headeris the intersection between old and new tips (the actual rollback target to reach before forwarding).Also applies to: 956-959, 983-986
crates/amaru-consensus/src/consensus/stages/select_chain.rs (2)
230-239: Rename Display label to match the type (nit).Display prints
rollback_point:while the field isrollback_header. Slightly confusing during debugging.Apply in both Display impls:
- "SwitchToFork[\n peer: {},\n rollback_point: {},\n fork:\n {}]", + "SwitchToFork[\n peer: {},\n rollback_header: {},\n fork:\n {}]",
266-275: Same label nit in RollbackChainSelection Display.Keep the wording consistent with the new data type.
Apply the same
rollback_headerlabel rename here as well.crates/amaru/src/stages/consensus/forward_chain/tests.rs (1)
60-101: Optional: wrap awaits with timeouts to avoid CI hangs.If a listener flaps or a future never resolves, the test could hang. A small
tokio::time::timeoutguard keeps the CI from turning into a Soulsborne boss fight.Sketch:
use tokio::time::{timeout, Duration}; let (_, tip) = timeout(Duration::from_secs(5), client.find_intersect(vec![])).await??; // ... same for recv_* callscrates/amaru-consensus/src/consensus/stages/validate_block.rs (3)
54-66: Drop unnecessary clones (RawBlock, Span).You already own
blockandspanin this match arm; cloning allocates needlessly.Apply:
- match eff - .external(ValidateBlockEffect::new(&peer, &point, block.clone())) + match eff + .external(ValidateBlockEffect::new(&peer, &point, block)) .await { Ok(Ok(_)) => { eff.send( &downstream, BlockValidationResult::BlockValidated { peer, header, - span: span.clone(), + span, }, ) .await }
185-187: Fix misleading expect() message in RollbackBlockEffect.The expect string mentions "ValidateBlockEffect" and "HasBlockValidation". It should reference
RollbackBlockEffectandCanValidateBlocksto aid debugging.Apply:
- .expect("ValidateBlockEffect requires a HasBlockValidation resource") + .expect("RollbackBlockEffect requires a CanValidateBlocks resource")
131-137: Consider passing Point by value to avoid double-clone.
header.point()creates a freshPointwhich you then clone innew(&Point). Takingpoint: PointinValidateBlockEffect::new(and storing it by value) removes an extra clone across the hot path.Happy to draft the small API tweak if you want to ripple it through call sites.
crates/amaru/src/stages/mod.rs (1)
214-217: our_tip fallback is fine; minor nit only.The
unwrap_or(HeaderTip::new(Point::Origin, 0))is correct givenmake_chain_storepre-checks. If you fancy,unwrap_or_elseavoids constructing the default eagerly.crates/amaru-consensus/src/consensus/stages/forward_chain.rs (1)
108-115: Log level choice for validation failures.These are expected protocol outcomes; consider
warn!to avoid noisy error logs under normal churn.crates/amaru-consensus/src/consensus/events.rs (1)
195-201: Debug label mismatch (“rollback_point” vs header).The Debug impl for
ValidateBlockEvent::Rollbackstill prints the field asrollback_pointwhile it now carries arollback_header. Tweak the label for clarity.Apply:
- .field("rollback_point", rollback_point) + .field("rollback_header", rollback_point)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (1)
20-23: Unify ChainStore import for consistency.Other modules use
amaru_ouroboros_traits::ChainStore; here it’samaru_consensus::ChainStore. Prefer one path across the crate to reduce “where’s that trait from?” head-scratching.crates/amaru-consensus/src/consensus/tip.rs (1)
148-188: HeaderTip is a tidy decoupling from pallas Tip.Public API surface is small and does the job. The Display impl is handy for logs. As a nicety, consider
impl From<Header> for HeaderTipalongsideAsHeaderTip.crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
165-169: Avoid empty actor names; provide a stable fallbackIf accepted_address is None, you’ll spawn a client named "". That’s a bit like an unnamed Final Fantasy protagonist—confusing in logs. Fall back to a generated name.
- let addr = peer - .accepted_address() - .map(|a| a.to_string()) - .unwrap_or_default(); + let addr = peer + .accepted_address() + .map(|a| a.to_string()) + .unwrap_or_else(|| format!("peer-{}", clients.len() + 1));Also applies to: 175-178
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (2)
174-187: recv_n breaks early on Await and then panics; loop until N insteadIf the server returns Await mid‑stream, you’ll bail early and then unwrap() a Vec of the wrong length. Keep pulling until you collect N ops.
- pub async fn recv_n<const N: usize>(&mut self) -> [ClientMsg; N] { - let mut ops = Vec::new(); - for _ in 0..N { - let msg = self.client.chainsync().request_next().await.unwrap(); - match msg { - NextResponse::RollForward(header, tip) => { - ops.push(ClientMsg::Forward(from_cbor(&header.cbor).unwrap(), tip)) - } - NextResponse::RollBackward(point, tip) => ops.push(ClientMsg::Backward(point, tip)), - NextResponse::Await => break, - } - } - ops.try_into().unwrap() - } + pub async fn recv_n<const N: usize>(&mut self) -> [ClientMsg; N] { + let mut ops = Vec::new(); + while ops.len() < N { + let msg = self.client.chainsync().request_next().await.unwrap(); + match msg { + NextResponse::RollForward(header, tip) => { + ops.push(ClientMsg::Forward(from_cbor(&header.cbor).unwrap(), tip)) + } + NextResponse::RollBackward(point, tip) => ops.push(ClientMsg::Backward(point, tip)), + NextResponse::Await => continue, + } + } + ops.try_into().unwrap() + }
134-139: Optional: add a short connect timeout to avoid hanging testsWrapping connect in a small timeout (e.g., 2–5s) keeps flaky CI from stalling forever if a port gets borked.
simulation/amaru-sim/src/echo/simulate.rs (2)
32-36: Make node_id earn its keep (or drop it).You’re accepting a node_id but not using it. Either remove it or use it to disambiguate the stage name for nicer logs/metrics when multiple nodes are running.
Apply this diff:
-pub fn spawn_echo_node(_node_id: String) -> NodeHandle<EchoMessage> { +pub fn spawn_echo_node(node_id: String) -> NodeHandle<EchoMessage> { let mut network = SimulationBuilder::default(); - let stage = network.stage( - "echo", + let stage = network.stage( + format!("echo-{node_id}"), async |mut state: State, msg: Envelope<EchoMessage>, eff| {
63-65: “init” output looks like a shim — consider clarifying or tightening.This channel exists only to satisfy the new NodeHandle init stream. That’s fine, but the comment is slightly misleading (it’s not “to make the stage”). Maybe clarify intent or, longer-term, let
from_pure_stageacceptOption<Receiver>to avoid dead outputs.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
crates/amaru/src/stages/consensus/forward_chain/chain_forward.pngis excluded by!**/*.png
📒 Files selected for processing (34)
crates/amaru-consensus/src/consensus/effects/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/network_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/events.rs(6 hunks)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs(3 hunks)crates/amaru-consensus/src/consensus/stages/fetch_block.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/forward_chain.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/select_chain.rs(6 hunks)crates/amaru-consensus/src/consensus/stages/validate_block.rs(5 hunks)crates/amaru-consensus/src/consensus/tip.rs(3 hunks)crates/amaru/Cargo.toml(1 hunks)crates/amaru/src/bin/amaru/cmd/daemon.rs(2 hunks)crates/amaru/src/stages/build_stage_graph.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain.rs(0 hunks)crates/amaru/src/stages/consensus/forward_chain/README.md(1 hunks)crates/amaru/src/stages/consensus/forward_chain/chain_forward.d2(3 hunks)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain/client_state.rs(5 hunks)crates/amaru/src/stages/consensus/forward_chain/mod.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs(6 hunks)crates/amaru/src/stages/consensus/forward_chain/tests.rs(2 hunks)crates/amaru/src/stages/mod.rs(7 hunks)crates/amaru/src/stages/pure_stage_util.rs(1 hunks)crates/pure-stage/src/receiver.rs(1 hunks)simulation/amaru-sim/src/echo/simulate.rs(2 hunks)simulation/amaru-sim/src/lib.rs(1 hunks)simulation/amaru-sim/src/simulator/args.rs(1 hunks)simulation/amaru-sim/src/simulator/run.rs(7 hunks)simulation/amaru-sim/src/simulator/simulate.rs(3 hunks)simulation/amaru-sim/src/simulator/simulate_config.rs(4 hunks)simulation/amaru-sim/src/simulator/world/node_handle.rs(2 hunks)simulation/amaru-sim/tests/simulation.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru/src/stages/consensus/forward_chain.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- crates/amaru/src/bin/amaru/cmd/daemon.rs
- simulation/amaru-sim/src/simulator/args.rs
- crates/amaru/src/stages/consensus/forward_chain/mod.rs
- crates/amaru-consensus/src/consensus/stages/mod.rs
- simulation/amaru-sim/src/simulator/world/node_handle.rs
- crates/amaru/src/stages/consensus/forward_chain/chain_forward.d2
- crates/amaru-consensus/src/consensus/effects/mod.rs
- simulation/amaru-sim/src/simulator/simulate.rs
- crates/amaru-consensus/src/consensus/effects/network_effects.rs
- crates/amaru/Cargo.toml
- crates/pure-stage/src/receiver.rs
- crates/amaru-consensus/src/consensus/stages/fetch_block.rs
- simulation/amaru-sim/src/lib.rs
- simulation/amaru-sim/tests/simulation.rs
- simulation/amaru-sim/src/simulator/simulate_config.rs
🧰 Additional context used
🧠 Learnings (14)
📚 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/echo/simulate.rssimulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-09-05T17:30:55.869Z
Learnt from: etorreborre
PR: pragma-org/amaru#432
File: crates/amaru/src/stages/consensus/clients_block_fetcher.rs:0-0
Timestamp: 2025-09-05T17:30:55.869Z
Learning: In crates/amaru/src/stages/consensus/clients_block_fetcher.rs, the fetch method currently holds a mutex lock across an await operation and lacks timeout handling. The author etorreborre has acknowledged this should be improved later by releasing the lock before awaiting and adding a bounded timeout around the network fetch operation.
Applied to files:
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs
📚 Learning: 2025-08-08T14:34:06.105Z
Learnt from: KtorZ
PR: pragma-org/amaru#370
File: crates/minicbor-extra/src/lib.rs:50-55
Timestamp: 2025-08-08T14:34:06.105Z
Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-08-20T20:18:50.214Z
Learnt from: rkuhn
PR: pragma-org/amaru#384
File: crates/pure-stage/tests/simulation.rs:459-469
Timestamp: 2025-08-20T20:18:50.214Z
Learning: Rust 1.85 (released February 20, 2025) stabilized the std::task::Waker::noop() API. This is now the preferred and correct way to create a no-op waker for testing scenarios, replacing the need for futures_task::noop_waker_ref(). The signature is pub const fn noop() -> &'static Waker and it's specifically designed for use cases like tests where wake notifications aren't needed.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-08-20T20:18:50.214Z
Learnt from: rkuhn
PR: pragma-org/amaru#384
File: crates/pure-stage/tests/simulation.rs:459-469
Timestamp: 2025-08-20T20:18:50.214Z
Learning: Rust 1.85 stabilized the Waker::noop() API, making it the preferred way to create a no-op waker instead of using futures_task::noop_waker_ref(). Code using Waker::noop() in modern Rust codebases is correct and doesn't need to be changed to use the futures_task alternative.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-05-09T13:09:47.915Z
Learnt from: rkuhn
PR: pragma-org/amaru#206
File: crates/pure-stage/src/simulation/running.rs:240-242
Timestamp: 2025-05-09T13:09:47.915Z
Learning: Cloning messages in the pure-stage crate should be avoided for performance reasons. The current implementation in SimulationRunning deliberately avoids duplicating message data structures.
Applied to files:
simulation/amaru-sim/src/simulator/run.rscrates/amaru/src/stages/pure_stage_util.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, ValidateHeader::handle_roll_forward returns a Result<PullEvent, ConsensusError>, not ValidateHeaderEvent as might be expected from the older code structure.
Applied to files:
crates/amaru-consensus/src/consensus/events.rscrates/amaru-consensus/src/consensus/stages/select_chain.rscrates/amaru/src/stages/build_stage_graph.rscrates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rscrates/amaru-consensus/src/consensus/stages/validate_block.rscrates/amaru-consensus/src/consensus/headers_tree/headers_tree.rscrates/amaru-consensus/src/consensus/stages/forward_chain.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, the ValidateHeader::handle_roll_forward method returns a PullEvent, not a ValidateHeaderEvent.
Applied to files:
crates/amaru-consensus/src/consensus/events.rscrates/amaru-consensus/src/consensus/stages/select_chain.rscrates/amaru-consensus/src/consensus/stages/validate_block.rs
📚 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:
crates/amaru-consensus/src/consensus/stages/select_chain.rscrates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rscrates/amaru-consensus/src/consensus/stages/forward_chain.rs
📚 Learning: 2025-08-08T14:43:20.218Z
Learnt from: KtorZ
PR: pragma-org/amaru#370
File: crates/amaru-kernel/src/point.rs:45-52
Timestamp: 2025-08-08T14:43:20.218Z
Learning: In crates/amaru-kernel/src/point.rs, Point::Specific stores a header hash that is always exactly 32 bytes by project invariant. Therefore, converting it with `impl From<&Point> for Hash<32>` without a runtime length check is acceptable; future reviews should not request an error-returning check inside this `From` impl. If validation is ever desired, it should be done via `TryFrom` or upstream constructors.
Applied to files:
crates/amaru-consensus/src/consensus/tip.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/tip.rscrates/amaru-consensus/src/consensus/stages/forward_chain.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
PR: pragma-org/amaru#295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In crates/amaru-consensus, flood prevention for duplicate invalid headers should be implemented in the validate_header stage or database layer, not in the store_header stage, since store_header runs before validation and cannot determine header validity.
Applied to files:
crates/amaru/src/stages/build_stage_graph.rscrates/amaru-consensus/src/consensus/stages/validate_block.rscrates/amaru-consensus/src/consensus/stages/forward_chain.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
PR: pragma-org/amaru#295
File: crates/amaru-consensus/src/consensus/store_header.rs:52-53
Timestamp: 2025-06-24T06:36:05.931Z
Learning: In the amaru consensus pipeline, headers are now stored before validation (store_header stage runs before validate_header stage). The validity state of headers should be tracked in the database to avoid revalidating already-valid headers and to prevent flood attacks with duplicate invalid headers.
Applied to files:
crates/amaru/src/stages/build_stage_graph.rs
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
PR: pragma-org/amaru#228
File: crates/amaru-stores/src/rocksdb/consensus.rs:89-128
Timestamp: 2025-05-21T18:58:48.631Z
Learning: The InMemConsensusStore implementation in crates/amaru-stores/src/rocksdb/consensus.rs will be fleshed out incrementally on a by-need basis, driven by test requirements rather than implementing all functionality upfront.
Applied to files:
crates/amaru/src/stages/mod.rs
🧬 Code graph analysis (16)
simulation/amaru-sim/src/echo/simulate.rs (2)
crates/pure-stage/src/stagegraph.rs (3)
stage(161-170)output(197-217)new(36-39)simulation/amaru-sim/src/simulator/world/node_handle.rs (3)
output(75-75)new(37-48)from_pure_stage(61-82)
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (4)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
client_protocols(134-161)tip(93-98)crates/amaru-consensus/src/consensus/effects/network_effects.rs (2)
new(67-72)send(33-33)crates/amaru-consensus/src/consensus/tip.rs (1)
new(163-165)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
new(89-114)
simulation/amaru-sim/src/simulator/run.rs (6)
simulation/amaru-sim/src/simulator/simulate_config.rs (2)
default(30-39)from(43-53)simulation/amaru-sim/src/simulator/world/node_handle.rs (4)
init_messages(74-74)output(75-75)from_pure_stage(61-82)new(37-48)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (2)
new(50-59)send(126-153)crates/amaru/src/stages/build_stage_graph.rs (1)
build_stage_graph(31-128)crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
send(33-33)crates/pure-stage/src/serde.rs (1)
to_cbor(216-227)
crates/amaru-consensus/src/consensus/events.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
peer(81-86)
crates/amaru-consensus/src/consensus/stages/select_chain.rs (3)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
point(74-76)crates/amaru-consensus/src/consensus/events.rs (2)
peer(101-107)peer(261-267)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
peer(81-86)
crates/amaru-consensus/src/consensus/tip.rs (4)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/test_header.rs (4)
block_height(94-96)hash(47-49)hash(82-84)point(86-88)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (4)
fmt(213-229)hash(66-68)new(89-114)point(74-76)crates/amaru-kernel/src/point.rs (3)
fmt(44-49)fmt(53-55)hash(34-40)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
new(50-59)
crates/amaru/src/stages/pure_stage_util.rs (2)
crates/pure-stage/src/tokio.rs (2)
stage(111-132)new(62-70)crates/pure-stage/src/receiver.rs (1)
new(32-34)
crates/amaru/src/stages/build_stage_graph.rs (3)
crates/amaru/src/stages/mod.rs (7)
stages(267-270)stages(287-290)network(250-250)network(251-253)network(254-256)network(257-259)network(260-262)crates/amaru-consensus/src/consensus/stages/forward_chain.rs (1)
stage(42-118)crates/amaru-consensus/src/consensus/stages/validate_block.rs (1)
stage(38-119)
crates/amaru/src/stages/mod.rs (3)
simulation/amaru-sim/src/simulator/run.rs (7)
fetch_block(284-286)new(302-313)network(179-179)network(180-182)network(183-185)network(186-188)network(189-191)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
new(89-114)crates/amaru/src/stages/build_stage_graph.rs (1)
build_stage_graph(31-128)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (3)
crates/amaru-consensus/src/consensus/tip.rs (2)
hash(96-101)hash(167-169)crates/amaru-kernel/src/point.rs (1)
hash(34-40)crates/amaru-consensus/src/consensus/headers_tree/data_generation/test_header.rs (2)
hash(47-49)hash(82-84)
crates/amaru-consensus/src/consensus/stages/validate_block.rs (3)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (3)
new(36-41)new(79-85)new(123-128)crates/amaru-consensus/src/consensus/effects/block_effects.rs (1)
new(39-41)crates/amaru-consensus/src/consensus/events.rs (2)
peer(101-107)peer(261-267)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (2)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (3)
tip(93-98)fmt(71-89)fmt(108-115)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
create(63-120)
crates/amaru/src/stages/consensus/forward_chain/tests.rs (3)
crates/amaru-consensus/src/consensus/tip.rs (8)
hash(96-101)hash(167-169)new(163-165)point(103-108)point(171-173)block_height(62-67)block_height(117-122)block_height(175-177)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (3)
hash(66-68)new(89-114)point(74-76)crates/amaru/src/stages/mod.rs (7)
stages(267-270)stages(287-290)pallas_point(406-406)pallas_point(410-412)pallas_point(416-418)as_tip(431-431)as_tip(435-437)
crates/amaru-consensus/src/consensus/stages/forward_chain.rs (4)
crates/amaru-consensus/src/consensus/tip.rs (9)
block_height(62-67)block_height(117-122)block_height(175-177)hash(96-101)hash(167-169)new(163-165)point(103-108)point(171-173)slot(124-129)crates/amaru-consensus/src/consensus/effects/network_effects.rs (2)
new(67-72)point(43-48)crates/amaru-consensus/src/consensus/events.rs (2)
peer(101-107)peer(261-267)crates/amaru-kernel/src/point.rs (1)
slot_or_default(27-32)
crates/amaru/src/stages/consensus/forward_chain/client_state.rs (3)
crates/amaru/src/stages/mod.rs (5)
stages(267-270)stages(287-290)pallas_point(406-406)pallas_point(410-412)pallas_point(416-418)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
hash_point(118-123)tip(93-98)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (3)
hash(66-68)mk_store(41-64)new(89-114)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (3)
crates/amaru/src/stages/consensus/forward_chain/client_state.rs (1)
find_headers_between(71-104)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (4)
eq(233-243)fmt(213-229)point(74-76)hash(66-68)crates/amaru-kernel/src/point.rs (3)
slot_or_default(27-32)from(59-61)hash(34-40)
🪛 markdownlint-cli2 (0.18.1)
crates/amaru/src/stages/consensus/forward_chain/README.md
13-13: Spaces inside link text
(MD039, no-space-in-links)
⏰ 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)
- GitHub Check: Benches
- GitHub Check: Build riscv32
- GitHub Check: Build aarch64/linux
- GitHub Check: Test the CLI
- GitHub Check: Build aarch64/macos
- GitHub Check: Build wasm32
- GitHub Check: Build x86_64/linux
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Build x86_64/windows
- GitHub Check: Test coverage
- GitHub Check: clippy
- GitHub Check: Analyze (rust)
🔇 Additional comments (35)
simulation/amaru-sim/src/simulator/run.rs (15)
24-26: Good move: wire in forward network effectsImports for ForwardEvent plumbing are spot on and consistent with the PR’s intent.
31-31: Import looks rightBringing ChainSyncEvent into scope aligns with the receiver stage wiring.
38-38: HeaderTip import matches new tip modelMatches the shift away from pallas::network::Tip.
54-54: Atomic counter import is appropriateUsing AtomicU64 for msg ids is the right call here.
57-57: tokio mpsc importNeeded for the new downstream channel; all good.
67-72: Spawn closure changes LGTMNew node_id param and init_messages/output tuple are wired correctly into NodeHandle::from_pure_stage.
95-97: Docs updatedReturn tuple doc now reflects init vs output channels — clearer for readers.
99-106: spawn_node signature evolutionAccepts node_id and returns the extra init_messages receiver — consistent with the rest of the file.
108-108: Config extractionUsing SimulateConfig here to derive downstream peer count is reasonable.
112-161: Init → InitOk handshake is tidyReceiver replies with InitOk and forwards Fwd/Bck as ChainSyncEvents. Clean separation, no footguns.
172-176: Downstream wiring: output + listenerTwo-channel split (init vs forward events) and listener construction look correct.
189-193: Resource injection for forward eventsPutting ResourceForwardEventListener in resources is exactly what forward_chain::stage expects.
289-299: Struct design: good choice with AtomicU64Arc with Relaxed is fine for a monotonic, uniqueness-only message id.
301-314: Constructor is straightforwardFields initialized sanely; counter starts at 0.
316-360: Fix msg_id off‑by‑one, avoid per‑recipient re‑serialization, and drop println!
- Off‑by‑one: fetch_add(n) reserves [base..base+n), but you use base+1..=base+n, leaving gaps. Use base+i with i in 0..n.
- Perf: precompute slot/hash/header bytes once and clone the small Bytes per recipient.
- Logging: println! will spam stdout in sims; prefer tracing or drop it.
Apply this patch:
#[async_trait] impl ForwardEventListener for MockForwardEventListener { async fn send(&self, event: ForwardEvent) -> anyhow::Result<()> { - fn message(event: &ForwardEvent, msg_id: u64) -> ChainSyncMessage { - match event { - ForwardEvent::Forward(header) => ChainSyncMessage::Fwd { - msg_id, - slot: header.point().slot_or_default(), - hash: Bytes { - bytes: header.hash().as_slice().to_vec(), - }, - header: Bytes { - bytes: to_cbor(&header), - }, - }, - ForwardEvent::Backward(tip) => ChainSyncMessage::Bck { - msg_id, - slot: tip.point().slot_or_default(), - hash: Bytes { - bytes: tip.hash().as_slice().to_vec(), - }, - }, - } - } - - // This allocates a range of message ids from - // self.msg_id to self.msg_id + number_of_downstream_peers - let base_msg_id = self - .msg_id - .fetch_add(self.number_of_downstream_peers as u64, Ordering::Relaxed); - - for i in 1..=self.number_of_downstream_peers { - let dest = format!("c{}", i); - let msg_id = base_msg_id + i as u64; - println!("msg id {}", msg_id); - let envelope = Envelope { - src: self.node_id.clone(), - dest, - body: message(&event, msg_id), - }; - self.sender.send(envelope).await?; - } + let n = self.number_of_downstream_peers as u64; + + // Precompute payload pieces once. + let (slot, hash_bytes, header_bytes) = match &event { + ForwardEvent::Forward(header) => ( + header.point().slot_or_default(), + Bytes { bytes: header.hash().as_slice().to_vec() }, + Some(Bytes { bytes: to_cbor(header) }), + ), + ForwardEvent::Backward(tip) => ( + tip.point().slot_or_default(), + Bytes { bytes: tip.hash().as_slice().to_vec() }, + None, + ), + }; + + // Reserve a contiguous id block and use base + i, i in [0, n). + let base = self.msg_id.fetch_add(n, Ordering::Relaxed); + for i in 0..n { + let dest = format!("c{}", i + 1); + let msg_id = base + i; + let body = match &event { + ForwardEvent::Forward(_) => ChainSyncMessage::Fwd { + msg_id, + slot, + hash: hash_bytes.clone(), + header: header_bytes.clone().expect("header bytes present"), + }, + ForwardEvent::Backward(_) => ChainSyncMessage::Bck { + msg_id, + slot, + hash: hash_bytes.clone(), + }, + }; + self.sender + .send(Envelope { src: self.node_id.clone(), dest, body }) + .await?; + } Ok(()) } }If you want logs, prefer tracing over println (will need the import tweak):
use tracing::{Span, info, trace};crates/amaru/src/stages/pure_stage_util.rs (1)
33-39: Constructor simplification looks good.Dropping the Runtime and keeping TokioRunning to hold lifetimes is tidy and avoids extra plumbing. No blockers.
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
429-435: Header-based rollback comparison is correct.Matching on
fork.rollback_header.hash()aligns with the new Fork payload and the rest of the PR. The assert message update is consistent.crates/amaru-consensus/src/consensus/stages/select_chain.rs (3)
75-99: Fork → sequence of Rollback + Forward is well-shaped.Emitting a single
Rollback { rollback_header }followed by forwards keeps the narrative clean for downstream. Nice and readable, mate.
120-126: Logs show the actual rollback header point.Using
rollback_header.point()in logs aligns with the payload change and will save a few puzzled stares in Grafana.
20-21: Import OK — amaru_ouroboros re-exports IsHeadercrates/amaru-ouroboros/src/lib.rs does pub use amaru_ouroboros_traits::*;, so amaru_ouroboros::IsHeader resolves to amaru_ouroboros_traits::IsHeader — no change needed.
crates/amaru/src/stages/consensus/forward_chain/tests.rs (1)
20-58: Async test flow reads clean and matches the new server semantics.Good coverage of intersect → initial forward → backward+forward sequence. Assertions use Pallas points and AsTip consistently.
crates/amaru/src/stages/mod.rs (1)
225-234: Bind listener to loopback/ephemeral in tests to avoid port clashes.Creating
TcpForwardChainServerwithconfig.listen_address(defaults to0.0.0.0:3000) will cause flaky CI when the port is busy.In the test below, override
listen_addressto127.0.0.1:0:let config = Config { ledger_store: InMem(ledger_store), chain_store: InMem(()), network, + listen_address: "127.0.0.1:0".to_string(), ..Config::default() };Picks a free ephemeral port on loopback — tidy as a pub on Friday arvo.
crates/amaru-consensus/src/consensus/events.rs (1)
260-268: Nice touch adding BlockValidationResult::peer().This simplifies routing without exposing internals. Looks good.
crates/amaru/src/stages/consensus/forward_chain/client_state.rs (2)
41-63: Rollback compaction logic is solid.Truncating to the last matching forward op or replacing with a fresh backward op keeps state lean and correct.
72-104: find_headers_between: correct intersection semantics.You exclude the intersection header from the forward list and return its tip — matches ChainSync expectations. Tests back it up. Nicely done.
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
50-99: ClientOp simplification to Forward(Header) is cleaner.The
tip()accessor deriving from the header keeps state aligned with messages. Debug output is helpful for tracing.
297-304: HeaderContent builder looks right.Variant and CBOR wiring are consistent with Pallas expectations.
crates/amaru-consensus/src/consensus/tip.rs (1)
62-67: Duplicate block_height provider is fine.Having both inherent
Tip::block_height()and traitIsHeader::block_height()is benign and ergonomic.crates/amaru/src/stages/build_stage_graph.rs (2)
70-77: Forward-chain stage wiring looks correct.Injecting
our_tipand error sinks upfront before validate_block keeps the flow clean.
78-85: validate_block downstream to store_block: LGTM.The pipeline order remains canonical after introducing the forward stage.
crates/amaru-consensus/src/consensus/stages/forward_chain.rs (1)
51-56: Don’t assert in a hot consensus stage; validate and continue.
assert_eq!will crash the node on bad input and turn a rogue peer into a boss-level crash bug. Guard, log, and keep the old tip instead.Apply:
- // assert that the new tip is a direct successor of the old tip - assert_eq!(header.block_height(), our_tip.block_height() + 1); - match header.parent() { - Some(parent) => assert_eq!(parent, our_tip.hash()), - None => assert_eq!(our_tip, HeaderTip::new(Point::Origin, 0)), - } + // sanity-check: header must be a direct successor of our_tip + let expected = our_tip.block_height() + 1; + let parent_ok = match header.parent() { + Some(parent) => parent == our_tip.hash(), + None => our_tip == HeaderTip::new(Point::Origin, 0), + }; + if header.block_height() != expected || !parent_ok { + error!( + target: EVENT_TARGET, + "non-adjacent header: got height={}, expected={}, parent_ok={}", + header.block_height(), + expected, + parent_ok + ); + return (our_tip, validation_errors, processing_errors); + }crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (3)
72-83: Mailbox set to 1_000_000: make it configurable and add backpressure/coalescingThat queue is bigger than a chocobo stable, mate. Great for demos, risky IRL. Gate it via config and consider coalescing ops or backpressure (acto’s send_wait exists).
Suggested tweak to make it env-configurable with a sane default:
- .with_mailbox_size(1_000_000) + .with_mailbox_size( + std::env::var("AMARU_FORWARD_MAILBOX_SIZE") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(8_192), + )Also consider:
- Using send_wait for bounded backpressure.
- Coalescing forward/backward ops (keep latest per client when queue swells).
- Emitting a metric and warn when queue length crosses thresholds.
126-152: Nice: lock scope kept tight; no await under lockThis neatly avoids the “hold mutex across await” footgun we’ve chatted about before. Rock solid.
156-192: Broadcast path can flood/clobber; add shutdown semantics and reduce cloning
- Shutdown: the while let ... has_senders() loop won’t end while the listener holds an ActoRef; add a dedicated Shutdown message to stop gracefully.
- Flood/clone pressure: cloning a full header per client every broadcast can be pricey. Consider:
- Coalescing ops (keep latest per client when lagging).
- Wrapping payloads in Arc to reduce copy churn across N clients.
Happy to sketch a Shutdown message wiring and an Arc refactor if you want it.
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
88-114: Async test setup LGTMClean switch to TCP-based infra with a proper ephemeral port. Nice one.
Signed-off-by: etorreborre <etorreborre@yahoo.com>
etorreborre
force-pushed
the
etorreborre/refactor/move-forward-chain-to-pure-stage
branch
from
177a573 to
97125bb
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (2)
simulation/amaru-sim/src/simulator/run.rs (1)
107-108: Log the node id for easier tracing.A tiny QoL tweak: include node_id in the spawn log so multi-node runs aren’t a Where’s Wally moment.
- info!("Spawning node!"); + info!(%node_id, "Spawning node");crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
38-42: Optional: swap Mutex for RwLock to reduce contentionRead-heavy path (accept loop) + occasional writes (forward/backward) → RwLock fits better and trims lock contention under load.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs(1 hunks)simulation/amaru-sim/src/lib.rs(1 hunks)simulation/amaru-sim/src/simulator/run.rs(7 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- simulation/amaru-sim/src/lib.rs
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-08T14:34:06.105Z
Learnt from: KtorZ
PR: pragma-org/amaru#370
File: crates/minicbor-extra/src/lib.rs:50-55
Timestamp: 2025-08-08T14:34:06.105Z
Learning: Project uses Rust 1.88 stable; using Result::is_err_and is acceptable and should not be flagged as unstable. In particular, crates/minicbor-extra/src/lib.rs relies on is_err_and when checking end_of_input; future reviews should not suggest replacing it for stability reasons.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-08-20T20:18:50.214Z
Learnt from: rkuhn
PR: pragma-org/amaru#384
File: crates/pure-stage/tests/simulation.rs:459-469
Timestamp: 2025-08-20T20:18:50.214Z
Learning: Rust 1.85 (released February 20, 2025) stabilized the std::task::Waker::noop() API. This is now the preferred and correct way to create a no-op waker for testing scenarios, replacing the need for futures_task::noop_waker_ref(). The signature is pub const fn noop() -> &'static Waker and it's specifically designed for use cases like tests where wake notifications aren't needed.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-08-20T20:18:50.214Z
Learnt from: rkuhn
PR: pragma-org/amaru#384
File: crates/pure-stage/tests/simulation.rs:459-469
Timestamp: 2025-08-20T20:18:50.214Z
Learning: Rust 1.85 stabilized the Waker::noop() API, making it the preferred way to create a no-op waker instead of using futures_task::noop_waker_ref(). Code using Waker::noop() in modern Rust codebases is correct and doesn't need to be changed to use the futures_task alternative.
Applied to files:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-05-09T13:09:47.915Z
Learnt from: rkuhn
PR: pragma-org/amaru#206
File: crates/pure-stage/src/simulation/running.rs:240-242
Timestamp: 2025-05-09T13:09:47.915Z
Learning: Cloning messages in the pure-stage crate should be avoided for performance reasons. The current implementation in SimulationRunning deliberately avoids duplicating message data structures.
Applied to files:
simulation/amaru-sim/src/simulator/run.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/run.rs
📚 Learning: 2025-09-05T17:30:55.869Z
Learnt from: etorreborre
PR: pragma-org/amaru#432
File: crates/amaru/src/stages/consensus/clients_block_fetcher.rs:0-0
Timestamp: 2025-09-05T17:30:55.869Z
Learning: In crates/amaru/src/stages/consensus/clients_block_fetcher.rs, the fetch method currently holds a mutex lock across an await operation and lacks timeout handling. The author etorreborre has acknowledged this should be improved later by releasing the lock before awaiting and adding a bounded timeout around the network fetch operation.
Applied to files:
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs
🧬 Code graph analysis (2)
simulation/amaru-sim/src/simulator/run.rs (6)
simulation/amaru-sim/src/simulator/simulate_config.rs (2)
default(30-39)from(43-53)crates/pure-stage/src/stagegraph.rs (3)
input(191-191)output(197-217)new(36-39)simulation/amaru-sim/src/simulator/world/node_handle.rs (4)
init_messages(74-74)output(75-75)from_pure_stage(61-82)new(37-48)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (2)
new(50-59)send(126-153)crates/amaru/src/stages/build_stage_graph.rs (1)
build_stage_graph(31-128)crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
send(33-33)
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (4)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
client_protocols(134-161)tip(93-98)simulation/amaru-sim/src/simulator/run.rs (2)
new(302-313)send(318-359)crates/amaru-consensus/src/consensus/effects/network_effects.rs (2)
new(67-72)send(33-33)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
new(89-114)
⏰ 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: license-headers
- GitHub Check: Test the CLI
- GitHub Check: Build riscv32
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/linux
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Build x86_64/linux
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (14)
simulation/amaru-sim/src/simulator/run.rs (11)
24-26: Good decoupling of forward effects.Nice move to ForwardEvent/ForwardEventListener; cleans up coupling to pallas tip land.
31-31: Import aligns with the new stage wiring.Using ChainSyncEvent here fits the new pure-stage graph.
38-38: HeaderTip import looks spot on.Matches the PR’s “ditch external Tip” goal.
54-54: Right choice: Arc with Relaxed.Perfect for a monotonic counter; no ordering requirement beyond uniqueness.
67-72: Spawn closure wiring LGTM.Pure-stage handle returned cleanly; suits the sim harness.
112-131: Init → InitOk handshake is tidy.Reply path via output channel is clear and side-effect free.
133-158: ChainSyncMessage → ChainSyncEvent mapping looks correct.RollForward/Rollback payloads are correctly shaped; Peer construction is consistent.
163-171: Confirm initial tip source (Origin vs store anchor).Starting from HeaderTip::new(Point::Origin, 0) is fine for “start from scratch,” but if the store is pre-populated, seeding from the chain store’s anchor tip could save a few hops. Was Origin intentional here?
172-176: Output split + listener wiring looks good.Dedicated channel for downstream broadcasts keeps concerns nicely separated.
189-193: Resource registration is correct.Putting ResourceForwardEventListener into the graph’s resources matches the new forward_chain stage contract.
316-360: Fix off-by-one msg_id, avoid per-recipient re-serialization, and drop println.Same gremlin as earlier, seems to have respawned like a Dark Souls boss:
- Off-by-one: fetch_add(n) reserves [base..base+n-1], but you use base+1..=base+n.
- Perf: header/hash re-serialized per recipient inside message().
- Logging: println! is noisy; use tracing at trace/debug.
Apply this diff to the send() body:
async fn send(&self, event: ForwardEvent) -> anyhow::Result<()> { - fn message(event: &ForwardEvent, msg_id: u64) -> ChainSyncMessage { - match event { - ForwardEvent::Forward(header) => ChainSyncMessage::Fwd { - msg_id, - slot: header.point().slot_or_default(), - hash: Bytes { - bytes: header.hash().as_slice().to_vec(), - }, - header: Bytes { - bytes: to_cbor(&header), - }, - }, - ForwardEvent::Backward(tip) => ChainSyncMessage::Bck { - msg_id, - slot: tip.point().slot_or_default(), - hash: Bytes { - bytes: tip.hash().as_slice().to_vec(), - }, - }, - } - } - - // This allocates a range of message ids from - // self.msg_id to self.msg_id + number_of_downstream_peers - let base_msg_id = self - .msg_id - .fetch_add(self.number_of_downstream_peers as u64, Ordering::Relaxed); - - for i in 1..=self.number_of_downstream_peers { - let dest = format!("c{}", i); - let msg_id = base_msg_id + i as u64; - println!("msg id {}", msg_id); - let envelope = Envelope { - src: self.node_id.clone(), - dest, - body: message(&event, msg_id), - }; - self.sender.send(envelope).await?; - } + let n = self.number_of_downstream_peers as u64; + + // Precompute payload pieces once. + let (slot, hash_bytes, header_bytes) = match &event { + ForwardEvent::Forward(header) => ( + header.point().slot_or_default(), + Bytes { bytes: header.hash().as_slice().to_vec() }, + Some(Bytes { bytes: to_cbor(header) }), + ), + ForwardEvent::Backward(tip) => ( + tip.point().slot_or_default(), + Bytes { bytes: tip.hash().as_slice().to_vec() }, + None, + ), + }; + + // Reserve a contiguous id block and use base + i, i in [0, n). + let base = self.msg_id.fetch_add(n, Ordering::Relaxed); + for i in 0..n { + let dest = format!("c{}", i + 1); + let msg_id = base + i; + let body = match &event { + ForwardEvent::Forward(_) => ChainSyncMessage::Fwd { + msg_id, + slot, + hash: hash_bytes.clone(), + header: header_bytes.clone().expect("header bytes present"), + }, + ForwardEvent::Backward(_) => ChainSyncMessage::Bck { + msg_id, + slot, + hash: hash_bytes.clone(), + }, + }; + // tracing::trace!(msg_id, dest = %dest, "broadcasting"); + self.sender.send(Envelope { src: self.node_id.clone(), dest, body }).await?; + } Ok(()) }Bonus: switches to_cbor(header) (no extra ref).
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (3)
72-84: Mailbox size 1_000_000 is a hungry kaiju — make it configurable and boundedGreat for demos, risky for prod. Let ops tune this and start with a safer default. Adds backpressure options later without code churn.
Apply:
- let clients = runtime + let mailbox_size: usize = std::env::var("AMARU_FORWARD_MAILBOX") + .ok() + .and_then(|s| s.parse().ok()) + .unwrap_or(100_000); + + let clients = runtime // FIXME: This is a temporary stop gap solution while we wait // to refactor to use pure-stage. Acto library has as a capped // size for the mailbox and drops incoming messages when it's // full. This should not be a problem in real life, but while // we are syncing _and_ forwarding at the same time for demo // purpose, this is problematic. - .with_mailbox_size(1_000_000) + .with_mailbox_size(mailbox_size) .spawn_actor("chain_forward", |cell| { client_supervisor(cell, store.clone(), max_peers) }) .me;
126-152: LGTM: poisoning handled and no await under lock in send()Clean lock scope, error mapped, then emit. Chef’s kiss.
156-192: Shutdown semantics: supervisor loop may never endBecause the listener task holds an ActoRef,
has_senders()can stay true forever. Add a dedicated Shutdown message or a drop path to let this actor wind down gracefully.
rkuhn
deleted the
etorreborre/refactor/move-forward-chain-to-pure-stage
branch
2 participants
This PR moves the last consensus stage,
forward_chainto apure-stageimplementation:BlockValidationResulthas been simplified to include a header and derive the rest of the information (point, tip) from it.HeaderTipstruct has been introduced to be the equivalent ofpallas::network'sTipwithout the dependency on that crate.forward_chain::stagefunction uses a newnetwork_effects::ForwardEventEffectto send forward events to downstream peers.forward_chain::stagestage doesn't do much actually now. It just does a sanity check on the current tip and knows which pure-stage external effect to use to send down forward events.test_infrahave been split into pure tests for finding an intersection and tests using tokio to test the clients connection.node_idto each spawned node.downstream_peer_nbargument to control the number of peers receiving forward events.Note: a major change in this PR is the handling of downstream peers via the
TcpForwardChainServerimplementation which takes care of:TcpListenerto allow peers to connect.ForwardEventevents from the consensus to send them to downstream peers.Summary by CodeRabbit
New Features
Changes
Documentation