Allow using Amaru as an upstream Cardano node#251
Conversation
WalkthroughAlright mate, here's the skinny: This update tweaks the CI workflow for the aarch64-unknown-linux-musl target, spruces up error logging and tracing in consensus stages, introduces handy macros for async message handling, and gives the block fetch protocol a proper minimal implementation. There's also a bit of tidying up in the docs and some extra trace logging sprinkled about for good measure. Changes
Sequence Diagram(s)sequenceDiagram
participant Client
participant Server
participant ChainStore
Client->>Server: BlockFetch(BlockRequest { lower, upper })
alt lower == upper
Server->>ChainStore: Load block at point
ChainStore-->>Server: Block
Server->>Client: Batch with single block
else lower != upper
Server->>Client: Error: CannotServeRange(lower, upper)
end
Client-->>Server: Terminate (optional)
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
jeluard
force-pushed
the
abailly/bootstrap-command
branch
from
096ab06 to
5ed19de
Compare
abailly
force-pushed
the
abailly/connect-2-nodes
branch
from
fc2da64 to
6acd9be
Compare
|
Still a draft? Also, does it have to be based on branch |
|
I can rebase it to main I think, let me try. |
|
Actually, I wanted to have some proper e2e test in place before undrafting it |
abailly
force-pushed
the
abailly/connect-2-nodes
branch
from
6acd9be to
58741b1
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
crates/amaru/src/stages/consensus/validate_header.rs (1)
46-55: Same wrapping remark as inreceive_headerSwapping the source error for
WorkerError::Recvdrops valuable context.
A thin wrapper keeps logs happy while allowing callers to inspect the root cause. Same diff pattern applies here.
🧹 Nitpick comments (3)
crates/amaru-consensus/src/consensus/select_chain.rs (1)
90-93: Nice trace hook, but include the peer for richer breadcrumbsLove the extra log, mate – helps when spelunking through forks like Nathan Drake.
Tiny nit: emit thepeerID so we know who twisted our arm.-trace!(target: EVENT_TARGET, rollback = %rollback_point, "switching to fork"); +trace!(target: EVENT_TARGET, rollback = %rollback_point, peer = %peer, "switching to fork");crates/amaru/src/stages/consensus/receive_header.rs (1)
35-38: Preserve the original error to ease post-mortemsWe log the error but then map it to a generic
WorkerError::Recv, losing the juicy stack for upstream handlers.
Consider wrapping instead of replacing so the caller can still pattern-match or bubble it up later.- .map_err(|e| { - error!("fail to handle chain sync {}", e); - WorkerError::Recv - })?; + .map_err(|e| { + error!("fail to handle chain sync: {e}"); + WorkerError::RecvWithCause(Box::new(e)) + })?;(Assumes you add
RecvWithCauseor similar toWorkerError.)
It’s the difference between a VHS and 4K when debugging.crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (1)
242-247: Helper converts slot/hash nicely – promote toamaru_kernel::Point::from?
from_pallas_pointis handy. Consider moving it to a shared util module and implementingFrom<Point>foramaru_kernel::Pointso the conversion becomes ergonomic throughout the codebase.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
examples/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (6)
.github/workflows/CI.yml(1 hunks)crates/amaru-consensus/src/consensus/select_chain.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/README.md(0 hunks)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs(4 hunks)crates/amaru/src/stages/consensus/receive_header.rs(2 hunks)crates/amaru/src/stages/consensus/validate_header.rs(3 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru/src/stages/consensus/forward_chain/README.md
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (1)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
hash(136-138)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Build on ubuntu-24.04 with target aarch64-unknown-linux-musl
- GitHub Check: Coverage
- GitHub Check: Snapshots (preprod, 10.1.4)
🔇 Additional comments (2)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
31-33: Good UX: explicit error for unsupported rangesThe new
CannotServeRangevariant is a solid shout. 👍
No action needed, just tipping the hat.
208-212: Hard-codingvariant: 6– confirm protocol version driftThe variant jumped from
1to6. Make sure the downstream Cardano nodes you plan to charm actually expect6, or you’ll be speaking Klingon while they listen for Elvish.
| - runner: ubuntu-24.04 | ||
| target: aarch64-unknown-linux-musl | ||
| command: test | ||
| setup: rustup target add aarch64-unknown-linux-musl | ||
| setup: rustup target add aarch64-unknown-linux-musl && sudo apt-get install -y pkg-config libssl-dev | ||
| cross-compile: true |
There was a problem hiding this comment.
💡 Verification agent
❓ Verification inconclusive
Add an apt-get update before installing packages
The new inline setup command installs pkg-config and libssl-dev, but skips an apt-get update.
Fresh runners occasionally have stale package indices which blows up with the classic “Unable to locate package” error and tanks the matrix job faster than a go-kart hitting a blue shell.
-setup: rustup target add aarch64-unknown-linux-musl && sudo apt-get install -y pkg-config libssl-dev
+setup: |
+ rustup target add aarch64-unknown-linux-musl
+ sudo apt-get update -y
+ sudo apt-get install -y pkg-config libssl-devAdd sudo apt-get update to the CI setup step
Hey team, on fresh Ubuntu 24.04 runners the apt cache can be a wee bit dusty—think of it like trying to pick up loot from a sandbox without refreshing the map in an RPG. Let’s stave off the classic “Unable to locate package” boss fight by updating before we install.
Locations to tweak:
.github/workflows/CI.yml, lines ~70–74
Suggested change:
- setup: rustup target add aarch64-unknown-linux-musl && sudo apt-get install -y pkg-config libssl-dev
+ setup: |
+ rustup target add aarch64-unknown-linux-musl
+ sudo apt-get update -y
+ sudo apt-get install -y pkg-config libssl-dev📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| - runner: ubuntu-24.04 | |
| target: aarch64-unknown-linux-musl | |
| command: test | |
| setup: rustup target add aarch64-unknown-linux-musl | |
| setup: rustup target add aarch64-unknown-linux-musl && sudo apt-get install -y pkg-config libssl-dev | |
| cross-compile: true | |
| - runner: ubuntu-24.04 | |
| target: aarch64-unknown-linux-musl | |
| command: test | |
| setup: | | |
| rustup target add aarch64-unknown-linux-musl | |
| sudo apt-get update -y | |
| sudo apt-get install -y pkg-config libssl-dev | |
| cross-compile: true |
🤖 Prompt for AI Agents
In .github/workflows/CI.yml around lines 70 to 74, the setup command installs
packages without running 'apt-get update' first, which can cause package not
found errors on fresh runners. Fix this by prepending 'sudo apt-get update &&'
to the setup command so the package index is refreshed before installing
pkg-config and libssl-dev.
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
crates/amaru/src/point.rs (1)
18-22: Avoid the full clone – match on the borrowed value insteadSolid move switching to a borrowed arg, mate, but the
clone()inside drags a fullVec<u8>copy each time we traverse this function. We can sidestep that overhead by pattern-matching on the reference and only cloning the hash when needed:-pub(crate) fn from_network_point(point: &pallas_network::miniprotocols::Point) -> Point { - match point.clone() { - pallas_network::miniprotocols::Point::Origin => Point::Origin, - pallas_network::miniprotocols::Point::Specific(slot, hash) => Point::Specific(slot, hash), - } +pub(crate) fn from_network_point(point: &pallas_network::miniprotocols::Point) -> Point { + match point { + pallas_network::miniprotocols::Point::Origin => Point::Origin, + pallas_network::miniprotocols::Point::Specific(slot, hash) => { + Point::Specific(*slot, hash.clone()) + } + } }Same semantics, leaner byte-shuffling. Unless we’re in “who cares, it’s once per epoch” land, this keeps things tidy.
crates/amaru/src/stages/pull.rs (1)
205-207: Borrowing looks grand, but watch the double-clone costPassing
&pointaligns nicely with the new signature – cheers. Just flagging thatfrom_network_pointstill clones internally, so we’re paying for the copy even though we still ownpointhere. If you tweakfrom_network_pointper the earlier suggestion, this call site stays the same and we dodge the surplus clone.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/amaru/src/point.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs(4 hunks)crates/amaru/src/stages/pull.rs(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-24.04 with target aarch64-unknown-linux-musl
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Coverage
- GitHub Check: Sanity
- GitHub Check: Snapshots (preprod, 10.1.4)
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
abailly
force-pushed
the
abailly/connect-2-nodes
branch
from
3a2bfea to
17bbff7
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
crates/amaru/src/stages/consensus/select_chain.rs (1)
47-55: 🛠️ Refactor suggestionMisleading error kind on chain-selection failure
WorkerError::Recvsuggests an I/O glitch, yet the failure comes from business logic insideselect_chain.handle_chain_sync. Consider mapping toWorkerError::Panicor a dedicatedWorkerError::Stageto avoid masking real bugs behind “recv” noise.
🧹 Nitpick comments (7)
crates/amaru/src/stages/common.rs (1)
19-27: 💌send!macro: ergonomics are ace, but consider a tiny polishThe macro already does the heavy lifting, yet every call site must remember to tack on
?to bubble-up theResult.
An alternative is to return()and handle the error conversion inside the macro, sparing callers from the extra sigil and keeping their intent crystal-clear:-$port.send($msg.into()).await.map_err(|e| { - tracing::error!(error=%e, "failed to send event"); - gasket::framework::WorkerError::Send -}) +if let Err(e) = $port.send($msg.into()).await { + tracing::error!(error = %e, "failed to send event"); + return Err(gasket::framework::WorkerError::Send); +} +Ok(())No functional bug here, just a UX upgrade for future devs.
crates/amaru/src/stages/consensus/forward_chain.rs (1)
342-343: Nit: stick to structured logging fieldsEverywhere else you’re using
error!(error=%e, ...). Here we switched to a free-text message. For log-query tooling it’s handier to keep theerror=%efield.-error!(target: EVENT_TARGET, "Unit::Peer was empty in execute"); +error!(target: EVENT_TARGET, reason = "empty Unit::Peer");crates/amaru/src/stages/consensus/select_chain.rs (1)
19-22: Unnecessary macro import clutters namespace
send!andschedule!are in the crate root thanks to#[macro_export], souse crate::{schedule, send};isn’t required and tripsunused_importswhenmacro_uselinting is tightened.-use crate::{schedule, send}; +// macros are in scope by defaultcrates/amaru/src/stages/consensus/store_header.rs (2)
15-19: Same import quirk as aboveMacros don’t need the
use crate::{schedule, send};import; drop it to dodge lints.
44-51: Structured error logging for the winThe new error message is free-text (
"fail to store header {}"). For grep-less observability stick with structured fields, e.g.:-error!("fail to store header {}", e); +error!(error = %e, "fail to store header");crates/amaru/src/stages/consensus/fetch_block.rs (2)
63-70: Now thatsend!exists let’s use it for symmetryYou manually map the send error instead of going through
send!; keeps two codepaths alive for the same job.- self.downstream - .send(ValidateBlockEvent::Validated { point, block, span }.into()) - .await - .map_err(|e| { - error!(error=%e, "failed to send event"); - WorkerError::Send - })? + send!(&mut self.downstream, + ValidateBlockEvent::Validated { point, block, span }.into() + )?;
94-114: Potential DOS: single peer fetch path
fetch_blockfails the whole stage if the chosen peer doesn’t have the block.
Given the comment// FIXME:you’re already aware, but flagging: a malicious or lagging peer can now stall sync. Consider falling back to any peer claiming height ≥ slot.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (16)
.github/workflows/CI.yml(1 hunks)crates/amaru-consensus/src/consensus/select_chain.rs(1 hunks)crates/amaru/src/point.rs(1 hunks)crates/amaru/src/stages/common.rs(1 hunks)crates/amaru/src/stages/consensus/fetch_block.rs(3 hunks)crates/amaru/src/stages/consensus/forward_chain.rs(2 hunks)crates/amaru/src/stages/consensus/forward_chain/README.md(0 hunks)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs(4 hunks)crates/amaru/src/stages/consensus/receive_header.rs(3 hunks)crates/amaru/src/stages/consensus/select_chain.rs(3 hunks)crates/amaru/src/stages/consensus/store_block.rs(3 hunks)crates/amaru/src/stages/consensus/store_header.rs(3 hunks)crates/amaru/src/stages/consensus/validate_header.rs(3 hunks)crates/amaru/src/stages/ledger.rs(3 hunks)crates/amaru/src/stages/mod.rs(1 hunks)crates/amaru/src/stages/pull.rs(3 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru/src/stages/consensus/forward_chain/README.md
✅ Files skipped from review due to trivial changes (3)
- crates/amaru/src/stages/mod.rs
- crates/amaru/src/stages/ledger.rs
- crates/amaru/src/stages/consensus/store_block.rs
🚧 Files skipped from review as they are similar to previous changes (7)
- crates/amaru-consensus/src/consensus/select_chain.rs
- crates/amaru/src/stages/pull.rs
- crates/amaru/src/point.rs
- crates/amaru/src/stages/consensus/validate_header.rs
- .github/workflows/CI.yml
- crates/amaru/src/stages/consensus/receive_header.rs
- crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs
🧰 Additional context used
🧬 Code Graph Analysis (3)
crates/amaru/src/stages/common.rs (7)
crates/amaru/src/stages/consensus/receive_header.rs (1)
schedule(54-59)crates/amaru/src/stages/consensus/select_chain.rs (1)
schedule(73-78)crates/amaru/src/stages/consensus/store_block.rs (1)
schedule(63-68)crates/amaru/src/stages/consensus/fetch_block.rs (1)
schedule(125-130)crates/amaru/src/stages/consensus/store_header.rs (1)
schedule(62-67)crates/amaru/src/stages/consensus/validate_header.rs (1)
schedule(70-75)crates/amaru/src/stages/ledger.rs (1)
schedule(158-163)
crates/amaru/src/stages/consensus/select_chain.rs (8)
crates/amaru/src/stages/consensus/receive_header.rs (1)
schedule(54-59)crates/amaru/src/stages/consensus/store_block.rs (1)
schedule(63-68)crates/amaru/src/stages/consensus/fetch_block.rs (1)
schedule(125-130)crates/amaru/src/stages/consensus/store_header.rs (1)
schedule(62-67)crates/amaru/src/stages/consensus/validate_header.rs (1)
schedule(70-75)crates/amaru/src/stages/ledger.rs (1)
schedule(158-163)crates/amaru/src/stages/consensus/forward_chain.rs (1)
schedule(232-245)crates/amaru/src/stages/pull.rs (1)
schedule(160-171)
crates/amaru/src/stages/consensus/fetch_block.rs (7)
crates/amaru/src/stages/consensus/receive_header.rs (1)
schedule(54-59)crates/amaru/src/stages/consensus/select_chain.rs (1)
schedule(73-78)crates/amaru/src/stages/consensus/store_block.rs (1)
schedule(63-68)crates/amaru/src/stages/consensus/store_header.rs (1)
schedule(62-67)crates/amaru/src/stages/consensus/validate_header.rs (1)
schedule(70-75)crates/amaru/src/stages/ledger.rs (1)
schedule(158-163)crates/amaru/src/stages/pull.rs (1)
schedule(160-171)
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Build on ubuntu-24.04 with target aarch64-unknown-linux-musl
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Coverage
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Snapshots (preprod, 10.1.4)
🔇 Additional comments (4)
crates/amaru/src/stages/common.rs (1)
34-44: 🤔schedule!macro may leak envelopes whenrecvyields an error
InputPort::recv()only errors on channel closure; otherwise it returns an envelope.
Mapping the envelope straight tounit.payloadmeans anyEnvelope::Ack/control message would be silently ignored instead of being acked, potentially causing upstream back-pressure to stall.Double-check the semantics of
Envelope; if non-data variants exist, we need to handle/ack them explicitly before turning them intoWorkSchedule::Unit.crates/amaru/src/stages/consensus/forward_chain.rs (1)
279-285: 🍻 Great visibility! Tiny trace tweakLove the new
"tip_changed"log – makes following chain growth a breeze.
You’re forwarding the originalspan(from the peer) as parent; if you meant to keep the internal_spanyou just opened a few lines above, swapparent: spanforparent: &_span. Otherwise all good.crates/amaru/src/stages/consensus/select_chain.rs (1)
57-59: Consistency: leverage your ownsend!macroGreat that you brought the macro in – but the loop still uses the explicit call.
- for event in events { - send!(&mut self.downstream, event)?; - } + for event in events { + send!(&mut self.downstream, event)?; + }(If you keep the loop minimalist, a
.try_for_eachcould shave a line, but that’s just bikeshedding.)crates/amaru/src/stages/consensus/fetch_block.rs (1)
56-63: Goodbye panics, hello logs – love it!Replacing the
or_panic()with proper error bubbling keeps the node alive in face of flaky peers. Nicely played 👌
| use crate::schedule; | ||
| use crate::stages::PeerSession; | ||
| use amaru_consensus::{consensus::ValidateHeaderEvent, peer::Peer, ConsensusError}; | ||
| use amaru_kernel::{block::ValidateBlockEvent, Point}; | ||
| use gasket::framework::*; | ||
| use tracing::{instrument, Span}; | ||
| use tracing::{error, instrument, Span}; | ||
| use tracing_opentelemetry::OpenTelemetrySpanExt; |
There was a problem hiding this comment.
Importing PeerSession but never using it
use crate::stages::PeerSession; vanished from the diff’s body – looks like deadwood sneaked in.
-use crate::stages::PeerSession;📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| use crate::schedule; | |
| use crate::stages::PeerSession; | |
| use amaru_consensus::{consensus::ValidateHeaderEvent, peer::Peer, ConsensusError}; | |
| use amaru_kernel::{block::ValidateBlockEvent, Point}; | |
| use gasket::framework::*; | |
| use tracing::{instrument, Span}; | |
| use tracing::{error, instrument, Span}; | |
| use tracing_opentelemetry::OpenTelemetrySpanExt; | |
| use crate::schedule; | |
| use amaru_consensus::{consensus::ValidateHeaderEvent, peer::Peer, ConsensusError}; | |
| use amaru_kernel::{block::ValidateBlockEvent, Point}; | |
| use gasket::framework::*; | |
| use tracing::{error, instrument, Span}; | |
| use tracing_opentelemetry::OpenTelemetrySpanExt; |
🤖 Prompt for AI Agents
In crates/amaru/src/stages/consensus/fetch_block.rs between lines 17 and 23, the
import statement for PeerSession is present but not used anywhere in the code.
Remove the line `use crate::stages::PeerSession;` to clean up unused imports and
avoid dead code.
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: jeluard <jeluard@users.noreply.github.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
This does the minimum to be able to serve blocks to a downstream Amaru node. Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
while I think the node selecting a new tip should be an 'info' and not a 'trace', this makes the CI timeout because of the amount of log entries it generates which are captured by GHA. Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
abailly
force-pushed
the
abailly/connect-2-nodes
branch
from
d2560a1 to
fa4621f
Compare
There was a problem hiding this comment.
Actionable comments posted: 0
🔭 Outside diff range comments (2)
crates/amaru-consensus/src/consensus/validate_header.rs (2)
146-154: Parameter bloat – consider foldingspaninto the event, not the API
handle_roll_forwardnow asks callers for an explicitSpan.
The method does nothing with it except shuttle it back inside the result.
Every caller already has the span (it was extracted from the incoming
DecodedChainSyncEvent). Passing it back-and-forth through every layer
just mushrooms the surface of the API and forces ripple edits across the
codebase.You could slim things down by:
- span: Span, + // no span parameterand inside the
Ok(...)block simply reuseSpan::current()(or attach
spanfetched from the incoming event, if that’s the desired parent).That keeps the public signature lean and avoids a param that carries no
new information.
229-236: Two nearly-identical code paths → maintenance gremlin
handle_chain_synccalls the “classic”handle_roll_forward, while
validate_headerroutes tohandle_roll_forward_new. Both functions
perform the same checks but diverge on howevolve_nonceis executed
(one holds the mutex directly, the other wraps it in anExternalEffect
for pure-stage).Duplicated validation logic will inevitably drift (the old “Left 4
Dead vs. Left 4 Dead 2” patch dilemma). Extract the common bits into a
private helper, and let each public function focus only on where the
nonce data comes from.Something like:
async fn process_roll_forward( &self, point: &Point, header: &Header, epoch_nonce: &Nonce, global_parameters: &GlobalParameters, ) -> Result<(), ConsensusError> { ... }Both callers can then fetch
epoch_noncetheir preferred way and dunk
it into this helper.
🧹 Nitpick comments (1)
crates/amaru-consensus/src/consensus/validate_header.rs (1)
172-177: Redundant data shuffling – span is already inside the eventHere the span is piped straight from the parameter into the output
struct unmodified. The code tells us nothing is done with the span in
between, so schlepping it through the function doesn’t add value and
just invites ordering mistakes down the road.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (18)
.github/workflows/CI.yml(1 hunks)crates/amaru-consensus/src/consensus/select_chain.rs(4 hunks)crates/amaru-consensus/src/consensus/validate_header.rs(3 hunks)crates/amaru-consensus/src/peer.rs(1 hunks)crates/amaru/src/point.rs(1 hunks)crates/amaru/src/stages/common.rs(1 hunks)crates/amaru/src/stages/consensus/fetch_block.rs(3 hunks)crates/amaru/src/stages/consensus/forward_chain.rs(5 hunks)crates/amaru/src/stages/consensus/forward_chain/README.md(0 hunks)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs(4 hunks)crates/amaru/src/stages/consensus/receive_header.rs(4 hunks)crates/amaru/src/stages/consensus/select_chain.rs(3 hunks)crates/amaru/src/stages/consensus/store_block.rs(3 hunks)crates/amaru/src/stages/consensus/store_header.rs(3 hunks)crates/amaru/src/stages/consensus/validate_header.rs(3 hunks)crates/amaru/src/stages/ledger.rs(4 hunks)crates/amaru/src/stages/mod.rs(1 hunks)crates/amaru/src/stages/pull.rs(4 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru/src/stages/consensus/forward_chain/README.md
✅ Files skipped from review due to trivial changes (1)
- crates/amaru-consensus/src/peer.rs
🚧 Files skipped from review as they are similar to previous changes (15)
- crates/amaru/src/stages/mod.rs
- .github/workflows/CI.yml
- crates/amaru/src/point.rs
- crates/amaru/src/stages/consensus/store_header.rs
- crates/amaru/src/stages/consensus/select_chain.rs
- crates/amaru/src/stages/pull.rs
- crates/amaru/src/stages/consensus/validate_header.rs
- crates/amaru/src/stages/consensus/fetch_block.rs
- crates/amaru/src/stages/consensus/receive_header.rs
- crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs
- crates/amaru-consensus/src/consensus/select_chain.rs
- crates/amaru/src/stages/common.rs
- crates/amaru/src/stages/consensus/forward_chain.rs
- crates/amaru/src/stages/consensus/store_block.rs
- crates/amaru/src/stages/ledger.rs
🧰 Additional context used
🧠 Learnings (1)
crates/amaru-consensus/src/consensus/validate_header.rs (1)
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.
⏰ Context from checks skipped due to timeout of 90000ms (6)
- GitHub Check: Snapshots (preprod, 1, 10.1.4)
- GitHub Check: Build on ubuntu-24.04 with target aarch64-unknown-linux-musl
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Coverage
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
3 participants
This PR adds a minimal ability to let other Cardano nodes connect to an Amaru upstream node and start syncing the chain from them:
This PR has only been tested with 2 Amaru nodes and usage documents are not updated because this new feature can be used simply by running an Amaru node and setting its
--peer-addressargument to the--listen-addressof the other Amaru node. We would want to check what happens if a cardano-node connects to the Amaru node and sync from them.Also, there's no automated tests which is annoying: I don't have a good mental model of how to write tests for these low-level network features.
Summary by CodeRabbit
New Features
Bug Fixes
Documentation
Chores
Refactor