Conversation
|
Important Review skippedDraft detected. Please check the settings in the CodeRabbit UI or the You can disable this status message by setting the Use the checkbox below for a quick retry:
WalkthroughThis PR performs a substantial architectural refactoring of the Amaru consensus codebase, replacing the TCP-based network server infrastructure with a resource-based manager pattern, removing internal network/store effects in favor of explicit resource access, introducing comprehensive node lifecycle management, and modernizing the test/simulator infrastructure with JSON-based tracing and deterministic test harnesses. Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Client Node
participant Manager as Manager Stage
participant Accept as Accept Stage
participant Network as Network (ConnectionProvider)
participant Peer as Peer Node
Client->>Manager: ManagerMessage::Listen(addr)
Manager->>Accept: StartAccept
Accept->>Network: listen(addr)
Network-->>Accept: SocketAddr
Peer->>Network: connect(addr)
Accept->>Network: accept()
Network-->>Accept: (Peer, ConnectionId)
Accept->>Manager: ManagerMessage::Accepted(peer, conn_id)
Client->>Manager: ManagerMessage::NewTip(tip)
Manager->>Peer: Forward tip via ConnectionProvider
sequenceDiagram
participant Test as Test Runner
participant NodeConfig as NodeTestConfig
participant SimGraph as SimulationBuilder
participant Manager as Manager Stage
participant Chain as Chain Selector
Test->>NodeConfig: Build test node config
NodeConfig-->>Test: NodeTestConfig with peers/depth
Test->>SimGraph: create_node(config)
SimGraph->>Manager: Wire manager stage
SimGraph->>Chain: Wire chain selector
SimGraph-->>Test: (StageRef<ManagerMessage>, StageRef<Action>)
Test->>Test: Run nodes for N steps
Test->>Test: Verify chain properties
Estimated code review effort🎯 4 (Complex) | ⏱️ ~75 minutes This PR introduces architectural changes across multiple subsystems (network abstraction, manager-based routing, stage graph restructuring), eliminates substantial legacy infrastructure (TCP server, actor model), and adds comprehensive new patterns (test node config, JSON tracing infrastructure). While individual changes are well-organized into cohorts, the breadth of affected modules, the removal of existing patterns requiring understanding of implications, and the introduction of new orchestration logic (build_node, run_tests) all demand careful review. Possibly related issues
Possibly related PRs
Suggested reviewers
🚥 Pre-merge checks | ✅ 3 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
and extend the test checking traces under simulation Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
etorreborre
force-pushed
the
etorreborre/feat/use-the-network-stack-downstream
branch
2 times, most recently
from
09f9cd1 to
ee2c8f4
Compare
|
@CodeRabbit review |
✅ 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: 13
Note
Due to the large number of review comments, Critical, Major severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/pure-stage/src/simulation/simulation_builder.rs (1)
15-28:⚠️ Potential issue | 🟡 MinorDuplicate license header — copy-paste artifact! 🎬
Ah, the ol' "copy-paste double-up" — like accidentally watching the opening credits of a film twice. The full Apache 2.0 license header already appears at lines 1–13, and it's repeated here at lines 16–28 right after the
#![expect(...)]attribute. One of them's gotta go, mate.🧹 Proposed fix — remove the duplicate header
#![expect(clippy::unwrap_used, clippy::panic, clippy::expect_used)] -// Copyright 2025 PRAGMA -// -// Licensed under the Apache License, Version 2.0 (the "License"); -// you may not use this file except in compliance with the License. -// You may obtain a copy of the License at -// -// http://www.apache.org/licenses/LICENSE-2.0 -// -// Unless required by applicable law or agreed to in writing, software -// distributed under the License is distributed on an "AS IS" BASIS, -// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. -// See the License for the specific language governing permissions and -// limitations under the License. - //! This module contains the [`SimulationBuilder`] and [`SimulationRunning`] types, which are
🤖 Fix all issues with AI agents
In `@crates/amaru-consensus/src/headers_tree/data_generation/actions.rs`:
- Around line 520-540: The implementation of Shrinkable::complement for
GeneratedActions has the parameter order reversed and builds a local complement
vector but never uses it; fix by changing the impl signature to match the trait
(complement(&self, to: usize, from: usize)) and then return a GeneratedActions
that actually uses the computed complement (instead of returning the original
actions_per_peer). Update GeneratedActions construction to accept the new
flattened action list (or set the appropriate override field) so that the
complement Vec<Action> produced from actions()[..to] and actions()[from..] is
stored/returned rather than discarded; keep tree cloning as before and ensure
len() and actions() remain consistent with the new stored actions.
In `@crates/amaru-consensus/src/headers_tree/data_generation/shrink.rs`:
- Around line 73-83: The trait Shrinkable has its complement parameter names
reversed compared to all implementations; change the signature in trait
Shrinkable's complement from complement(&self, to: usize, from: usize) -> Self
to complement(&self, from: usize, to: usize) -> Self (keeping the Self: Sized
bound and other method signatures intact) so the trait's parameter order matches
impls like the Vec<A> implementation and GeneratedActions::complement.
In `@crates/amaru-consensus/src/stages/fetch_block.rs`:
- Around line 118-133: The call to eff.base().call(...) currently uses
.await.unwrap_or_default(), which swallows timeouts/errors and produces an empty
Blocks leading to Ok(None); change this to handle the Result from .await
explicitly: capture the error from eff.base().call(...) (the call sending
ManagerMessage::FetchBlocks), log the error with context (including peer_clone
and point), and return Err(...) so callers can classify it as ProcessingFailed
(transient) instead of ValidationFailed; only treat an Ok(empty) Blocks as
Ok(None) after a successful call.
In `@crates/amaru-ouroboros-traits/src/connections/mod.rs`:
- Around line 21-22: The mock test scaffolding (the mod mock and its exported
MockConnectionProvider) is being compiled unconditionally; gate it behind the
test-utils feature by adding #[cfg(feature = "test-utils")] to the mod mock
declaration and also conditionally re-export it (e.g., add the same
#[cfg(feature = "test-utils")] to the pub use mock::* line) so
MockConnectionProvider and the mock module are only included when the test-utils
feature is enabled.
In `@crates/amaru-protocols/src/chainsync/responder.rs`:
- Around line 114-117: The Rollback handler (ResponderMessage::Rollback ->
ResponderAction::RollBackward) fails to update the responder internal state,
leaving self.pointer and self.upstream stale; modify the Rollback branch to set
self.upstream = tip and self.pointer = point (mirroring what RollForward does
with block_header.point() and tip) before returning
Ok((Some(ResponderAction::RollBackward(point, tip)), self)) so subsequent
next_header/NewTip logic uses the updated state.
In `@crates/amaru/Cargo.toml`:
- Line 74: The Cargo.toml entry adds the "test-utils" feature to the production
dependency amaru-ouroboros-traits, which exposes test-only code in non-dev
builds; remove the "features = [\"test-utils\"]" from the amaru-ouroboros-traits
dependency and instead enable that feature only for dev builds (e.g., move the
dependency to [dev-dependencies] with features = ["test-utils"] or keep it in
normal dependencies without the test-utils feature and enable test-utils via
per-package dev-dependencies or workspace overrides) so production builds don’t
pull in test-only functionality.
In `@crates/amaru/src/tests/configuration.rs`:
- Around line 225-232: The code sets both the anchor and the tip from
headers.first(); change it so the anchor remains headers.first() but the best
chain hash uses headers.last() instead—locate the block using headers.first(),
chain_store.set_anchor_hash and chain_store.set_best_chain_hash and replace the
argument to set_best_chain_hash to use the last header (headers.last()) so the
tip reflects the most recent header.
In `@simulation/amaru-sim/src/bin/amaru-sim/main.rs`:
- Around line 32-34: The current main swallows errors from run_tests by only
printing them, causing a zero exit code on failure; modify main to propagate the
error (e.g., change main() -> fn main() -> Result<(), Box<dyn Error>> and return
run_tests(args)?) or explicitly exit non‑zero on Err (call std::process::exit(1)
inside the Err branch) so failures in run_tests produce a non‑zero process exit;
update references to run_tests and main accordingly and ensure any error is
returned or causes process::exit(1).
In `@simulation/amaru-sim/src/simulator/args.rs`:
- Around line 100-101: The env-var logic for the struct field enable_shrinking
is inverted: it uses is_true_or("AMARU_DISABLE_SHRINKING",
run_config.enable_shrinking) which enables shrinking when the user intends to
disable it; update the assignment in args.rs to use the positive env var name by
calling is_true_or("AMARU_ENABLE_SHRINKING", run_config.enable_shrinking) (or
alternatively negate the current call with
!is_true_or("AMARU_DISABLE_SHRINKING", !run_config.enable_shrinking)) so the
field enable_shrinking and the #[arg] attribute semantics match; keep references
to the enable_shrinking field and the is_true_or helper consistent.
In `@simulation/amaru-sim/src/simulator/report.rs`:
- Line 22: The unconditional import of std::os::unix::fs::symlink in report.rs
causes Windows builds to fail because the code also references symlink_dir under
#[cfg(windows)]; fix this by consolidating platform-specific symlink behavior
into a small cross-platform helper (e.g., create_symlink or make_symlink) with
#[cfg(unix)] using std::os::unix::fs::symlink and #[cfg(windows)] using
std::os::windows::fs::symlink_dir, remove the unconditional use line, and update
all call sites that currently call symlink or symlink_dir to call the new helper
(so references to symlink and symlink_dir in report.rs are replaced by the
platform-agnostic helper).
In `@simulation/amaru-sim/src/simulator/run_config.rs`:
- Around line 79-82: The method disable_shrinking currently sets
self.enable_shrinking = true (opposite of its name); update the method so it
sets self.enable_shrinking = false to match its name, or if you prefer the
inverted semantics, rename disable_shrinking to enable_shrinking and keep the
assignment; adjust the method signature of disable_shrinking (or the renamed
method) in RunConfig so the behavior and name are consistent with the
enable_shrinking field and update any callers accordingly.
In `@simulation/amaru-sim/src/simulator/run_tests.rs`:
- Around line 38-43: The public function run_test currently creates nodes and
runs them but ignores chain-check results (unlike run_test_nb); update run_test
(or its signature) to surface failures by either (A) calling
check_chain_property(&nodes) after run_nodes and asserting or panicking on
failure, or (B) changing run_test to return a TestResult (or Result<..., Error>)
and propagate the outcome from check_chain_property so callers can inspect it;
locate run_test, create_nodes, run_nodes, and check_chain_property symbols to
implement the chosen approach and ensure failures are not silently swallowed.
- Around line 116-132: The bug is caused by shadowing the parameter test_result
inside shrink_test; call shrink returns a new TestResult but the local let (mut
test_result, ...) hides the &mut TestResult parameter so the caller is never
updated. Fix by renaming the local returned TestResult (e.g., shrunk_result) and
then assign it back into the caller's reference (e.g., *test_result =
shrunk_result) before calling test_result.set_test_failure(run_config,
test_number, number_of_shrinks); reference the functions/values shrink,
generated_actions(), shrink_test, and set_test_failure to locate the change.
🟡 Minor comments (15)
crates/pure-stage/src/simulation/running/mod.rs-488-498 (1)
488-498:⚠️ Potential issue | 🟡 Minor
Blocked::Terminatedand other blocked states are silently swallowed here, legend.Right, so here's the thing — like a sneaky plot twist in a Christopher Nolan film — if
run_until_sleeping_or_blocked()returnsBlocked::Terminated,Blocked::Idle,Blocked::Deadlock, orBlocked::Busy, this method just returnsfalseas if everything's grand.For
Blocked::Terminated, the caller won't know until the next call (whenis_terminated()picks it up via the watch channel). That one-iteration delay might be fine for your use case, but it's worth being explicit about.More importantly, if the simulation lands in
Blocked::IdleorBlocked::Deadlock, a caller looping onrun_or_terminatedwould spin forever without making progress — like being stuck in a Groundhog Day loop with no way out. Consider either:
- Returning the
Blockedreason so callers can react, or- At minimum, documenting these edge cases in the doc comment.
📝 Suggestion: enrich the return type or docs
Option A — return the blocked reason:
- /// Run until sleeping or blocked, and if sleeping, skip to the next wakeup. - /// Return true if the simulation is terminated, false otherwise. - pub fn run_or_terminated(&mut self) -> bool { + /// Run until sleeping or blocked, skip to the next wakeup if sleeping. + /// Returns `None` if there is more work to do, or `Some(blocked)` with the + /// reason the simulation cannot make further progress. + pub fn run_or_terminated(&mut self) -> Option<Blocked> { if self.is_terminated() { - return true; + return Some(Blocked::Idle); } - if let Blocked::Sleeping { .. } = self.run_until_sleeping_or_blocked() { - self.skip_to_next_wakeup(None); + match self.run_until_sleeping_or_blocked() { + Blocked::Sleeping { .. } => { + self.skip_to_next_wakeup(None); + None + } + blocked => Some(blocked), } - false }Option B — at least document the behavior:
/// Run until sleeping or blocked, and if sleeping, skip to the next wakeup. - /// Return true if the simulation is terminated, false otherwise. + /// Returns `true` if the simulation was already terminated before this call, + /// `false` otherwise. Note: if a stage terminates *during* this call, `false` + /// is still returned; the next call will detect it. Non-sleeping blocked states + /// (Idle, Deadlock, Busy) are silently ignored. pub fn run_or_terminated(&mut self) -> bool {crates/amaru-ouroboros-traits/Cargo.toml-23-23 (1)
23-23:⚠️ Potential issue | 🟡 MinorSpot on —
parking_lotshould be gated behindtest-utilsor moved to dev-dependencies.
parking_lotis used exclusively inconnections/mock.rsfor theMockConnectionProvider, which is pure test infrastructure. There's zero reason to drag it into every production build. Since you've already got atest-utilsfeature gate set up (like withrand), just throwparking_lotbehind that feature flag and you're golden.[features] default = [] test-utils = ["dep:rand", "dep:parking_lot"] [dependencies] parking_lot = { workspace = true, optional = true }It's like keeping a chainsaw in your kitchen when you only need it for the occasional campfire, yeah?
crates/amaru-protocols/Cargo.toml-29-29 (1)
29-29:⚠️ Potential issue | 🟡 MinorUnused test-utils dependency—consider moving to dev-dependencies or removing.
Enabling
test-utilsonamaru-ouroboros-traitsas a regular dependency pulls inrandas a build dependency. However,randisn't used anywhere in amaru-protocols' production code—only test code uses the test utilities (InMemConsensusStore, etc.), which are already properly guarded with#[cfg(test)]. This means you're compiling an unused dependency for every build. If test utilities are needed, consider gating them behind a feature flag in this crate'sCargo.tomlor moving the dependency to[dev-dependencies]instead.crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs-203-225 (1)
203-225:⚠️ Potential issue | 🟡 MinorTest utility
get_blockslooks solid.The
#[expect(clippy::expect_used)]annotation keeps clippy happy for test-utils, and the chain of.expect()calls is appropriate here — if anything's missing in a test, you want it to blow up fast, like a Creeper in Minecraft that wandered into your base. No subtlety needed.One small thought: the doc comment says "starting from the best chain tip down to the root" but
retrieve_best_chain()returns hashes from oldest to newest (see line 66 —best_chain.reverse()). So the actual iteration order is oldest-to-newest, which is fine, but the doc is slightly misleading.📝 Proposed doc fix
-/// Retrieve all blocks from the chain store starting from the best chain tip down to the root. +/// Retrieve all blocks from the chain store along the best chain, from oldest to newest.crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs-18-20 (1)
18-20:⚠️ Potential issue | 🟡 MinorUnconditional imports used only behind
#[cfg(feature = "test-utils")]will cause unused-import warnings.G'day mate! So here's the thing —
NetworkBlock,Block, andArcare only used inside theget_blocksandget_best_chain_block_headersfunctions (lines 207–240), which are gated behind#[cfg(feature = "test-utils")]. But these imports are unconditional, so when that feature isn't enabled, the compiler's gonna have a whinge about unused imports. Like bringing a surfboard to a LAN party — cool gear, wrong context.🛠️ Proposed fix — gate the test-utils-only imports
-use amaru_kernel::cardano::network_block::NetworkBlock; -use amaru_kernel::{Block, BlockHeader, HeaderHash, IsHeader, Point, RawBlock, Tip}; -use std::sync::Arc; +use amaru_kernel::{BlockHeader, HeaderHash, IsHeader, Point, RawBlock, Tip}; +#[cfg(feature = "test-utils")] +use amaru_kernel::cardano::network_block::NetworkBlock; +#[cfg(feature = "test-utils")] +use amaru_kernel::Block; +#[cfg(feature = "test-utils")] +use std::sync::Arc;crates/amaru-protocols/src/network_effects.rs-113-114 (1)
113-114:⚠️ Potential issue | 🟡 MinorExpect messages still reference the old type name
ConnectionResource.G'day mate! The
.get::<ConnectionsResource>()call is spot on with the rename, but the.expect()string on Line 114 still says "ConnectionResource" — same story on Lines 137, 163, 189, 215, and 240. It's like renaming your band but keeping the old name on the tour posters, y'know? Not a showstopper, but worth tidying up so devs debugging a panic don't go on a wild goose chase.🔧 Proposed fix (showing one instance; apply similarly to all six)
let resource = resources .get::<ConnectionsResource>() - .expect("ListenEffect requires a ConnectionResource") + .expect("ListenEffect requires a ConnectionsResource") .clone();simulation/amaru-sim/src/simulator/checks.rs-48-48 (1)
48-48:⚠️ Potential issue | 🟡 MinorTypo in tracing message: "dowstream" → "downstream".
Small one, but it'll show up in your logs and make you do a double-take like a typo in a game's loading screen. 😄
✏️ Fix
- tracing::info!(node_id = %node.config.listen_address, blocks_nb = %actual.len(), "retrieved the best dowstream block headers"); + tracing::info!(node_id = %node.config.listen_address, blocks_nb = %actual.len(), "retrieved the best downstream block headers");simulation/amaru-sim/src/simulator/data_generation/generate.rs-211-211 (1)
211-211:⚠️ Potential issue | 🟡 MinorWee typo alert — "arrivale" should be "arrival".
Like a misspelled quest item in an RPG, it won't break anything but it catches the eye! 😄
- // Generate arrivale times and make entries for each peer. + // Generate arrival times and make entries for each peer.simulation/amaru-sim/src/simulator/args.rs-85-85 (1)
85-85:⚠️ Potential issue | 🟡 MinorStale doc reference to
SimulateConfigandNodeConfig.The comment still says "defaults from SimulateConfig and NodeConfig" but these types have been replaced by
RunConfigandNodeTestConfig.-/// Create Args from environment variables, with defaults from SimulateConfig and NodeConfig. +/// Create Args from environment variables, with defaults from RunConfig and NodeTestConfig.simulation/amaru-sim/src/simulator/run_config.rs-94-104 (1)
94-104:⚠️ Potential issue | 🟡 MinorPeer address formatting will get funky for 10+ peers.
For
number_of_upstream_peers >= 10,format!("127.0.0.1:300{i}")produces"127.0.0.1:30010"(port 30010) instead of the expected 4-digit pattern. Functionally valid, but the intent seems to be 3001–3009 as a range. If more than 9 peers are ever used, this could be confusing during debugging. Just something to keep in mind!crates/amaru/src/stages/build_node.rs-81-86 (1)
81-86:⚠️ Potential issue | 🟡 MinorMinor grammar nit in the comment, and a silent fallback to origin.
Line 83 has a small stutter: "tip and anchors are is exactly aligned" → "tip and anchors are exactly aligned".
Also, line 86 silently falls back to
Tip::origin()if the chain store can't load the tip. If the ledger says the tip is X but the chain store doesn't have it (and it's notORIGIN_HASH, which is already checked at line 197), this could mask a misconfiguration. Might be worth a log or at least a debug trace here.simulation/amaru-sim/src/simulator/report.rs-119-137 (1)
119-137:⚠️ Potential issue | 🟡 Minor
canonicalize(target).unwrap()will panic iftargetdoesn't exist yet.
std::fs::canonicalizerequires the path to exist. If the target directory hasn't been created yet whencreate_symlink_diris called, this panics harder than a plot twist in Inception. Consider creating the directory first or propagating the error.crates/amaru/src/tests/setup.rs-99-99 (1)
99-99:⚠️ Potential issue | 🟡 MinorWee typo: "inititialize" → "initialize" 🔤
- // We inititialize the nodes by running a few steps to let them run the initialization phase + // We initialize the nodes by running a few steps to let them run the initialization phasecrates/amaru/src/tests/configuration.rs-246-249 (1)
246-249:⚠️ Potential issue | 🟡 MinorPotential underflow if
chain_lengthis ever 0 — a sneaky little gremlin.
self.chain_length - 1on ausizewill panic in debug or wrap in release ifchain_lengthis 0. Since this is test code and the default is 10, the risk is low, but a guard orsaturating_subwould be a nice safety net — like saving before a boss fight, yeah?🛡️ Optional guard
+ assert!(self.chain_length > 0, "chain_length must be at least 1"); let mut headers = run_strategy(any_headers_chain_with_root( self.chain_length - 1, // -1 since we already have the root header root_header.point(), ));crates/amaru/src/tests/setup.rs-81-84 (1)
81-84:⚠️ Potential issue | 🟡 MinorHardcoded
mailbox_sizeof 10000 ignoresconfig.mailbox_size📬Line 83 passes a hardcoded
10000towith_mailbox_size(), butNodeTestConfighas amailbox_sizefield (also defaulting to 10000) that could be customized by callers. If someone sets a different mailbox size via.with_mailbox_size(500), it'll be quietly ignored here — a bit like ordering a pint and getting served a schooner anyway.🐛 Proposed fix
let mut stage_graph = SimulationBuilder::default() .with_seed(config.seed) - .with_mailbox_size(10000) + .with_mailbox_size(config.mailbox_size) .with_trace_buffer(config.trace_buffer.clone());
🧹 Nitpick comments (31)
crates/amaru-protocols/src/tx_submission/responder.rs (1)
156-165: Consider returning&'static strinstead of&strfor consistency.All the match arms return string literals, which are inherently
'static. Some sibling implementations (e.g., inblockfetch/responder.rsandchainsync/responder.rs) already return&'static str, while others likehandshake/initiator.rsuse&str. It's a bit like a party where half the guests showed up in formal wear and the other half in boardies — works fine either way, but picking one look would be tidier.No functional impact, just a wee consistency nit.
🧹 Optional: tighten the return type
impl ResponderResult { - pub fn message_type(&self) -> &str { + pub fn message_type(&self) -> &'static str { match self {crates/pure-stage/src/simulation/simulation_builder.rs (1)
154-156: UseRandStdRng::from_seedinstead of manually constructing.You've just added that lovely
RandStdRng::from_seed(seed)convenience method over inrandom.rs— like crafting a legendary weapon and then not equipping it! This line manually doesRandStdRng(StdRng::seed_from_u64(seed)), which is exactly whatfrom_seedwraps. Using it would also let you drop theSeedableRngandStdRngimports on lines 58–59.♻️ Proposed refactor
pub fn with_seed(self, seed: u64) -> Self { - self.with_eval_strategy(RandStdRng(StdRng::seed_from_u64(seed))) + self.with_eval_strategy(RandStdRng::from_seed(seed)) }And clean up the now-unnecessary imports:
use crate::simulation::RandStdRng; use parking_lot::Mutex; -use rand::SeedableRng; -use rand::prelude::StdRng;crates/amaru-protocols/src/protocol_messages/version_table.rs (1)
112-123: Theentries.sort()is redundant and could misorder versions with different digit widths.G'day! So here's the thing, mate —
BTreeMapalready iterates inOrdorder of its keys, so by the time you.map()those entries into strings, they're already sorted byVersionNumber. The extraentries.sort()on line 120 re-sorts by lexicographic string order, which is a different game entirely — like sorting movie release years as text where "9" comes after "80". For the current V11–V14 range it's harmless, but it's a wee time bomb if versions ever span different digit counts.You can safely drop the sort:
♻️ Suggested simplification
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { let entries = self .values .iter() .map(|(version, data)| format!("{}: {}", version.as_u64(), data)) .collect::<Vec<String>>(); - entries.sort(); write!(f, "{{{}}}", entries.join(", ")) }simulation/amaru-sim/tests/replay_test.rs (1)
79-87:load_trace_entriesdiscardsInstantfrom each entry — worth a brief doc note.Just a small nit: line 84 deserializes
(Instant, TraceEntry)tuples but only keeps theTraceEntry. Might be worth a one-liner comment explaining that replay manages its own timing, so the original timestamps are intentionally dropped. Future-you will thank present-you, like reading your own save file notes. 📝simulation/amaru-sim/src/simulator/world/node_handle.rs (1)
94-117: No-op handle with commented-out code — consider cleaning up or gating.Hey legend, I see the TODO at line 94, which is fair — this is WIP, like that half-built base in Valheim you swear you'll finish "next weekend." But leaving ~15 lines of commented-out code in the tree is a bit of a code smell, even for a draft.
A couple of thoughts:
- The commented-out block (lines 97–112) would be better tracked in a git commit or an issue rather than inline. It makes the function harder to read.
- Since
from_pure_stagenow returns a no-op handle, any existing simulation tests that call this path will silently succeed without actually testing anything — a sneaky false-positive situation, like a Hitman mission where the target was already gone.Consider adding a
tracing::warn!ortodo!()inside the handle closure so it's visible when this code path is hit during tests, rather than quietly swallowing messages.Want me to open an issue to track the multi-node NodeHandle rework mentioned in the TODO?
♻️ Suggested cleanup
- // TODO: make different nodes for upstream peer, node under test and downstream peer - let handle = Box::new( - move |_msg: Option<Envelope<Msg>>| Ok(None), - // Some(msg) => { - // trace!(msg = %msg, "enqueuing"); - // running.enqueue_msg(&input, [msg]); - // match running.run_one_step(&rt) { - // Some(_blocked) => { - // let mut result = init_messages.drain().collect::<Vec<_>>(); - // result.extend(output.drain().collect::<Vec<_>>()); - // Ok(Some(result)) - // } - // None => Ok(None), - // } - // } - // None => match running.run_one_step(&rt) { - // Some(_) => Ok(Some(output.drain().collect::<Vec<_>>())), - // None => Ok(None), - // }, - ); + // TODO: make different nodes for upstream peer, node under test and downstream peer + let handle = Box::new(move |_msg: Option<Envelope<Msg>>| { + tracing::warn!("from_pure_stage NodeHandle is a no-op placeholder"); + Ok(None) + });crates/amaru/src/stages/ledger.rs (1)
25-101: NewLedgerabstraction — clean enum dispatch, minor duplication.Top shelf work here, mate. The
Ledgerenum is a crisp abstraction — like choosing between offline mode and online mode in a game, same interface, different backends. A few wee observations:
Repeated match arms: Every method (
get_tip,get_stake_distribution,get_block_validation) does the same thing in both arms. The PR's workspace deps includedelegate— if that's intended for this kind of thing, it could tidy things up. Not urgent with only 2 variants though.Minor nit on line 85/89:
anyhow!(format!("{e:?}"))can be simplified —anyhow!already supports format args directly.🧹 Simplify anyhow formatting
- let state = stage.state.lock().map_err(|e| anyhow!(format!("{e:?}")))?; + let state = stage.state.lock().map_err(|e| anyhow!("{e:?}"))?;Apply to both lines 85 and 89.
crates/amaru-ouroboros-traits/src/connections/to_socket_addrs.rs (1)
89-113: Newto_socket_addrsmethod — clean conversion logic.Looks solid, mate! Like a proper inventory screen — every variant gets its own clean path to the same result type. One thing worth calling out:
The
Stringvariant (line 95–100) usesstr::parse::<SocketAddr>(), which does not perform DNS resolution. So hostnames like"localhost:3000"will fail. If that's by design (IP-literal-only), grand. If not, you'd needstd::net::ToSocketAddrs(the stdlib trait) for hostname resolution. Just flagging it since the custom type name is quite similar to the stdlib trait.Also, a tiny nit:
anyhow!(format!("invalid address '{s}': {e}"))allocates an intermediateString. You could doanyhow!("invalid address '{s}': {e}")directly, which usesanyhow!'s built-in formatting.🧹 Skip intermediate format allocation
- s.parse() - .map_err(|e| anyhow!(format!("invalid address '{s}': {e}")))?, + s.parse() + .map_err(|e| anyhow!("invalid address '{s}': {e}"))?,crates/amaru/src/bin/ledger/cmd/mod.rs (1)
34-41:Config::default()used solely for arena parameters — consider extracting constants.Creating a full
Config::default()just to grabledger_vm_alloc_arena_countandledger_vm_alloc_arena_sizeis a bit like loading an entire RPG save file just to check your character's name. It works, but associated constants or a small defaults module would be more direct. Not a blocker though — she'll be right for now.crates/amaru/src/tests/in_memory_connection_provider.rs (3)
147-185:connect()allocates new channel pairs on everypoll_fninvocation — wasteful on retries.Oi, this is like buying a new set of cables every time you retry plugging in your console! When
poll_fnreturnsPending(no listener yet), the endpoints and Arc-wrapped VecDeques are allocated and then immediately dropped. On the next wake, they're recreated from scratch.Functionally correct since only the final-poll's channels get registered, but you could hoist the allocation out of
poll_fnby creating the channel pair once before theBox::pin(...)and moving them into the closure.
219-233:peer_conn_idis never set — everysend()broadcasts wake to all connections.This is like yelling across the whole pub instead of tapping your mate on the shoulder. 🍻 The
InMemoryEndpoint::peer_conn_iddefaults toNoneand I don't see any code path that populates it. So theelsebranch here (lines 222-232) fires on every send, waking all other connections' recv wakers.For a test-only provider this is tolerable, but it scales as O(n) per send where n is the number of active connections. If simulations grow to many nodes, this could become a bottleneck.
Consider setting
peer_conn_idduring theaccept()flow (when both sides are known) for targeted wakes.#!/bin/bash # Verify that peer_conn_id is indeed never set anywhere rg -n "peer_conn_id" --type rust -C2
239-278:recv()holdsconnectionslock for the entire poll closure body — could blocksend()andclose().Like hogging the bathroom at a house party, mate. The
connectionsMutex guard on line 246 is held while also lockingread_buffer(line 257) andread_queue(line 259). Any concurrentsend()orclose()call that needsconnections.lock()will be blocked until this poll returns.Since
parking_lot::Mutexis not async-aware and this is all withinpoll_fn, the guard is dropped whenPoll::Pendingis returned — so no deadlock across await points. But in tight send/recv loops, the contention window is wider than necessary.Consider extracting the endpoint's Arc'd fields (read_queue, read_buffer, recv_waker) under a brief lock and then operating on them outside the
connectionslock scope.crates/amaru-protocols/src/keepalive/messages.rs (1)
36-39:as_u16duplicates the existingFrom<Cookie> for u16impl — consider if both are needed.G'day mate! Just a heads-up — there's already a
From<Cookie> for u16impl at lines 48-52 that does the exact same thing. Callers can useu16::from(cookie)orcookie.into(). Having both isn't a game-breaker (like having two save slots in Dark Souls, why not), but it's a wee bit of redundant surface area.crates/amaru-protocols/src/tests/configuration.rs (1)
139-142: Pre-existing:chain_length - 1can underflow if called withchain_length == 0.This isn't from your changes, but since we're in the neighbourhood — if someone ever calls
with_best_chain_of_length(0), thatchain_length - 1on line 140 will panic on underflow in debug or wrap in release. Currently the constants are 4 and 10, so it's grand, but a wee guard or asaturating_subwould make this more robust against future misuse. No rush though — it's like a side quest you can pick up later. 🗺️🛡️ Optional defensive fix
fn initialize_chain_store( chain_length: usize, chain_store: Arc<dyn ChainStore<BlockHeader>>, ) -> anyhow::Result<()> { + anyhow::ensure!(chain_length > 0, "chain_length must be at least 1"); // Use the same root header for both initiator and respondersimulation/amaru-sim/tests/traces_test.rs (2)
1-1:recursion_limit = "4096"– that's a chonky macro expansion, fair dinkum.This is presumably needed for the deeply nested
json!macro on lines 62-168. Makes sense, but worth a wee comment explaining why it's so high so the next dev doesn't think it's a typo.
60-168: This hardcoded span tree is the Dark Souls of test assertions – precise but punishing. 🎮The exact JSON tree with ~60+ ordered span entries is an incredible level of trace verification, but it's also extremely brittle. Any change to protocol message ordering, span naming, instrumentation attributes, or even adding a new protocol step will break this test. That's a high maintenance tax.
A couple of options to consider down the track:
- Assert on a subset of critical spans rather than the full ordered sequence (e.g., verify that handshake → chainsync → blockfetch ordering is preserved, without asserting every intermediate tx_submission ping-pong).
- Use a structural pattern matcher that tolerates extra spans in between expected ones.
Not saying to change it now – for a WIP it's great to have the full picture locked down – but as the protocol evolves, this test will likely be the first thing that breaks on every PR. Just something to keep in the back pocket, like a save point before a boss fight.
crates/amaru/src/tests/assertions.rs (1)
75-80: Theget_tx_ids()call might be a no-op if no transactions were configured.
get_tx_ids()(from the relevant snippet inconfiguration.rs) creates transactions based onRESPONDER_TXS_NB. If this constant is 0, or if the test nodes weren't configured with transactions, both mempools will return empty results and the assertion trivially passes – like beating a boss that doesn't fight back.Might be worth adding an
assert!(!tx_ids.is_empty(), ...)guard similar to Line 54-57, or at least a comment clarifying when this check is meaningful.simulation/amaru-sim/tests/simulation_test.rs (1)
18-23: Quick nit:pubon a#[test]function in an integration test is unnecessary.Integration test functions don't need
pubvisibility since they can't be imported by other crates. Not a big deal – it's like putting a "Welcome" mat outside a room with no door – but trimming it keeps things tidy.🧹 Optional cleanup
-pub fn run_simulator() { +fn run_simulator() {simulation/amaru-sim/src/simulator/replay.rs (1)
91-93:load_blockalways returns the sameNETWORK_BLOCK– intentional but worth a comment.During replay, any
load_blockcall returns the predefinedNETWORK_BLOCKregardless of the requested hash. This is the deterministic replay trick – like using the same save file every time. It works beautifully for trace replay where block content isn't validated, but could cause subtle issues if validation logic that inspects block contents is ever added to the replay path. A quick doc comment here would save future-you a head-scratch.📝 Optional doc comment
+ /// Always returns the predefined NETWORK_BLOCK regardless of hash. + /// This ensures deterministic replay behavior where block content doesn't matter. fn load_block(&self, _hash: &HeaderHash) -> Result<Option<RawBlock>, StoreError> { Ok(Some(NETWORK_BLOCK.raw_block())) }crates/amaru-protocols/src/chainsync/initiator.rs (1)
66-76:ChainSyncInitiatorMsg::message_type()duplicatesInitiatorResult::message_type().Oi, this is like that scene in Groundhog Day — we're doing the same thing twice!
ChainSyncInitiatorMsg::message_type()manually matchesself.msgvariants, butInitiatorResult::message_type()(Lines 223-233) already does exactly that. You could just delegate to avoid the copy-paste.♻️ Proposed refactor
impl ChainSyncInitiatorMsg { pub fn message_type(&self) -> &str { - match self.msg { - InitiatorResult::Initialize => "Initialize", - InitiatorResult::IntersectFound(_, _) => "IntersectFound", - InitiatorResult::IntersectNotFound(_) => "IntersectNotFound", - InitiatorResult::RollForward(_, _) => "RollForward", - InitiatorResult::RollBackward(_, _) => "RollBackward", - } + self.msg.message_type() } }crates/amaru/src/tests/test_data.rs (1)
25-37: Consider reusingcreate_transactionsinsidecreate_transactions_in_mempool.This is a proper "nice-to-have" mate — you could lean on
create_transactionsand just loop over the results to add them to the mempool. Less duplication, like when a game reuses the same boss mechanics but with a different skin. 🎮♻️ Proposed refactor
#[expect(clippy::unwrap_used)] pub fn create_transactions_in_mempool( mempool: Arc<dyn Mempool<Transaction>>, number: usize, ) -> Vec<Transaction> { - let mut txs = vec![]; - for i in 0..number { - let tx = create_transaction(i); - txs.push(tx.clone()); - mempool.add(tx).unwrap(); + let txs = create_transactions(number); + for tx in &txs { + mempool.add(tx.clone()).unwrap(); } txs }crates/amaru-tracing-json/src/json_layer.rs (1)
134-136: Unnecessary.clone()—valueis already owned here.Mate, in this loop
for (key, value) in visitor.fields, thevisitor.fieldsmap is consumed (moved), sovalueis already an ownedValue. The.clone()is doing extra work for no reason — like paying for DLC you already own.♻️ Proposed fix
for (key, value) in visitor.fields { - event_json[key] = value.clone(); + event_json[key] = value; }crates/amaru-tracing-json/src/collect.rs (1)
99-108:is_spancan be simplified withmatches!.This is a pure nit, mate — just a tiny readability win. Like choosing the slightly faster dialogue option in an RPG.
♻️ Proposed refactor
pub fn is_span(item: &Value) -> bool { - if let Some(t) = item.get("type") - && t == "span" - { - true - } else { - false - } + item.get("type").is_some_and(|t| t == "span") }crates/amaru-tracing-json/src/json_trace_collector.rs (2)
30-43:flush()can avoid the clone by usingstd::mem::take.Right now you clone the entire
Vec<Value>and then clear it (lines 33-34). Since you hold a write lock, you can just swap the Vec out atomically withstd::mem::take, avoiding the allocation and copy. It's like fast-travelling instead of walking back to town — same result, less work. 🏃♻️ Suggested optimization
pub fn flush(&self) -> Vec<Value> { match self.0.write() { - Ok(mut traces) => { - let lines = traces.clone(); - traces.clear(); - lines - } + Ok(mut traces) => std::mem::take(&mut *traces), // The RwLock can only get poisoned should the thread panic while pushing a new line // onto the stack. In case this happen, we'll likely be missing traces which should be // caught by assertions down the line anyway. So it is fine here to simply return the // 'possibly corrupted' data. Err(err) => err.into_inner().clone(), } }
23-28: Silent data loss on poisoned lock ininsert()— worth a trace log?If the
RwLockis poisoned,insertsilently drops the trace value. Theflush()method has a nice comment about this scenario, butinsert()doesn't even log it. Consider adding atracing::warn!or at least a comment here for consistency with theflush()approach. It's like a stealth enemy killing your NPCs off-screen — you'd at least want to see it in the event log.💡 Optional improvement
pub(crate) fn insert(&self, value: Value) { - if let Ok(mut lines) = self.0.write() { - lines.push(value); + match self.0.write() { + Ok(mut lines) => lines.push(value), + Err(_) => { + // RwLock poisoned — trace data will be lost. See flush() comment. + } } }crates/amaru/src/stages/build_node.rs (1)
217-230: Prefer&[Peer]over&Vec<Peer>— the classic Rust slice wisdom! 🦀Like choosing the right weapon class in a souls-like,
&[Peer]is more versatile — it accepts bothVec<Peer>and slices, and is the idiomatic Rust way to pass borrowed sequences.♻️ Suggested tweak
fn make_chain_selector( chain_store: Arc<dyn ChainStore<BlockHeader>>, - peers: &Vec<Peer>, + peers: &[Peer], consensus_security_parameter: usize, ) -> Result<SelectChain, ConsensusError> {simulation/amaru-sim/src/simulator/args.rs (1)
86-109:make_args()duplicates env var names from#[arg]attributes — potential drift risk.The env var names in
make_args()(e.g.,"AMARU_NUMBER_OF_TESTS") are manually duplicated from the#[arg(env = "AMARU_NUMBER_OF_TESTS")]annotations onArgs. If one changes without the other, they'll silently diverge. Not a blocker for a WIP PR, but worth a wee TODO to consolidate these into constants so they stay in sync.simulation/amaru-sim/src/simulator/run_config.rs (1)
74-77: Rename builder method to match its field.The
with_number_of_nodesmethod is settingnumber_of_upstream_peers— bit of a disconnect there, yeah? Method name's stuck in the past like a Tarantino reference you've heard too many times. No callers in the codebase yet, so renaming is a clean play:♻️ Suggested rename
- pub fn with_number_of_nodes(mut self, n: u8) -> Self { + pub fn with_number_of_upstream_peers(mut self, n: u8) -> Self { self.number_of_upstream_peers = n; self }simulation/amaru-sim/src/simulator/report.rs (1)
53-66: Double-lock ontrace_buffer— the mutex is acquired twice in quick succession.Line 57 locks to check
is_empty(), drops the guard, then line 61 locks again to iterate. Withparking_lot::Mutexthis won't poison, but the buffer could theoretically change between the two locks. Since this runs post-simulation it's not a real race, but a single lock scope would be cleaner — like ordering your flat white and smashed avo in one go instead of two trips to the counter.♻️ Suggested single-lock approach
pub fn persist_traces_as_cbor( dir: &Path, trace_buffer: Arc<Mutex<TraceBuffer>>, ) -> Result<(), anyhow::Error> { - if trace_buffer.lock().is_empty() { - return Ok(()); - } - - let messages: Vec<Vec<u8>> = trace_buffer.lock().iter().map(|b| b.to_vec()).collect(); + let guard = trace_buffer.lock(); + if guard.is_empty() { + return Ok(()); + } + let messages: Vec<Vec<u8>> = guard.iter().map(|b| b.to_vec()).collect(); + drop(guard); let path = dir.join("traces.cbor"); let mut file = File::create(&path)?; cbor4ii::serde::to_writer(&mut file, &messages)?; Ok(()) }crates/amaru-consensus/src/headers_tree/data_generation/actions.rs (3)
413-448:Debuganddisplay_as_linesforGeneratedActions— helpful for debugging sims. 👌The line-by-line display with
GeneratedActionformatting (type, peer, slot, hash, parent) is a nice touch for reading simulation logs. Two separateimpl GeneratedActionsblocks (lines 413–449 and 451–473) could be merged into one, but it's purely cosmetic.
465-473:export_to_filepanics on failure while sibling functions returnResult.
File::create(path).unwrap()andfile.write_all(...).unwrap()at lines 469–471 will panic on I/O errors, while the caller inreport.rs(persist_generated_entries_as_json) wraps the call in a function returningResult. If the file write fails, the panic bubbles up instead of being caught. Since this is test/sim tooling it's not critical, but returning aResultwould be more consistent with the rest of the persistence API — like keeping the same art style across DLC packs, y'know?♻️ Suggested change
- pub fn export_to_file(&self, path: &str) { - use std::{fs::File, io::Write}; - - let mut file = File::create(path).unwrap(); - let content = self.as_json().to_string(); - file.write_all(content.as_bytes()).unwrap(); - } + pub fn export_to_file(&self, path: &str) -> std::io::Result<()> { + use std::{fs::File, io::Write}; + + let mut file = File::create(path)?; + let content = self.as_json().to_string(); + file.write_all(content.as_bytes())?; + Ok(()) + }
498-518:GeneratedActionand itsFrom<Action>— solid display formatting. 🍿The truncated 6-char hashes and compact
FWD/BCKlabels make logs scannable."n/a"for rollback parent hashes is appropriate. Minor style nit:unwrap_or("n/a".to_string())could useunwrap_or_else(|| "n/a".to_string())to avoid allocating the string on the happy path, but it's negligible here.
etorreborre
force-pushed
the
etorreborre/feat/use-the-network-stack-downstream
branch
3 times, most recently
from
fa5f710 to
abe6379
Compare
Signed-off-by: etorreborre <etorreborre@yahoo.com>
etorreborre
force-pushed
the
etorreborre/feat/use-the-network-stack-downstream
branch
from
abe6379 to
5439385
Compare
1 participant
WIP
Summary by CodeRabbit
New Features
Improvements
Bug Fixes