add keepalive protocol and start connection stage #608
Conversation
Signed-off-by: Roland Kuhn <[email protected]>
Signed-off-by: Roland Kuhn <[email protected]>
Signed-off-by: Roland Kuhn <[email protected]>
WalkthroughThis PR adds a Connection state machine, keep‑alive and tx‑submission protocols, a MemoryPool effects bridge and in‑memory mempool implementations, extends bytes deserialization, and introduces a Contramap effect path across pure‑stage (effect, resume, running, tokio, trace), plus tests and Cargo/workspace updates. Changes
Sequence Diagram(s)sequenceDiagram
participant App as Application
participant Conn as Connection
participant Mux as Muxer
participant HS as Handshake
participant KA as KeepAlive
participant Stage as PureStage
App->>Conn: new(conn_id, role, magic)
App->>Conn: send Initialize
Conn->>Mux: wire_up muxer & register handlers
Conn->>HS: register handshake handler
HS->>Conn: HandshakeResult (Accepted / Refused)
alt Accepted
Conn->>Stage: spawn tx_submission stage via register_tx_submission
Conn->>KA: register_keepalive(role, muxer)
KA->>Mux: exchange KeepAlive ⇄ ResponseKeepAlive
Stage->>Mux: send/receive tx_submission messages (RequestTxIds/Txs)
else Refused
Conn->>Conn: terminate connection
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~50 minutes
Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (6)
crates/amaru-network/src/handshake/tests.rs (1)
94-104: Consider usinghandler_transformhere too for consistency.The
test_against_node_with_tokiotest (line 198) now uses the shiny newhandshake::handler_transformhelper, but this inline closure does the exact same thing. Might be worth keeping things consistent across both tests, mate – like using the same control scheme in multiplayer so everyone's on the same page.let handshake_bytes = - network.contramap( - handshake, - "handshake_bytes", - |msg: HandlerMessage| match msg { - HandlerMessage::FromNetwork(bytes) => { - handshake::HandshakeMessage::FromNetwork(bytes) - } - HandlerMessage::Registered(_) => handshake::HandshakeMessage::Registered, - }, - ); + network.contramap(handshake, "handshake_bytes", handshake::handler_transform);crates/amaru-network/src/keepalive/mod.rs (1)
42-83: Initiator logic is solid overall.The flow makes sense: receive a response, validate the cookie (like checking if the right secret handshake was returned), cycle to the next cookie, wait a beat, then send the next ping. Good use of
or_terminatefor error handling.One small observation – the log on line 81 happens after the
eff.call()completes, so you're logging "sending" when it's already sent. Might want to flip the order for more accurate tracing during debugging.+ tracing::debug!(?state.cookie, "sending KeepAlive message"); eff.call(&state.muxer, NETWORK_SEND_TIMEOUT, |cr| { let msg = NonEmptyBytes::encode(&Message::KeepAlive(state.cookie)); MuxMessage::Send(PROTO_N2N_KEEP_ALIVE.erase(), msg, cr) }) .await; - tracing::debug!(?state.cookie, "sending KeepAlive message"); statecrates/amaru-network/src/connection.rs (4)
54-68: Consider renamingState::Initiatorto something role-agnostic.Right now, both initiators and responders end up in the
Initiatorstate after handshake (seedo_handshakeat line 180). It's like calling both players in a co-op game "Player 1" – technically works but might confuse future-you when debugging. PerhapsActiveorConnectedwould be clearer?I see the "LATER: make full duplex" comment, so if this is a placeholder that'll get reworked, no worries – just flagging for when you get there.
82-88: Theunimplemented!panic could be a bit rough in production.This is like having a "game over" screen with no continue option – if an unexpected state/message combo happens, the whole thing goes kaboom. In a staged system with controlled message flow, it might be fine, but consider whether logging and terminating gracefully would be safer:
- x => unimplemented!("{x:?}"), + (state, msg) => { + tracing::error!(?state, ?msg, "unexpected state/message combination"); + return eff.terminate().await; + }That said, if you're confident the message flow is deterministic and this truly should never happen (famous last words, I know), the
unimplemented!at least gives you a nice debug message.
100-106: Magic number5760could use a named constant.You've got 5760 appearing here and again on line 135 for the handshake buffer. It's like using the same cheat code twice – works, but if you need to change it later, you've gotta hunt it down in multiple spots.
const HANDSHAKE_MAX_BUFFER: usize = 5760;Then use it in both places. Makes the code self-documenting too.
158-178: Another magic number spotted:65535for keepalive buffer.Same deal as before – might be worth extracting to a constant like
KEEPALIVE_MAX_BUFFERfor clarity and maintainability.+const KEEPALIVE_MAX_BUFFER: usize = 65535; + // ... in do_handshake: eff.send( &muxer, MuxMessage::Register { protocol: PROTO_N2N_KEEP_ALIVE.erase(), frame: mux::Frame::OneCborItem, handler: keepalive.clone(), - max_buffer: 65535, + max_buffer: KEEPALIVE_MAX_BUFFER, }, )
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
crates/amaru-network/Cargo.toml(1 hunks)crates/amaru-network/src/bytes.rs(2 hunks)crates/amaru-network/src/connection.rs(1 hunks)crates/amaru-network/src/handshake/mod.rs(3 hunks)crates/amaru-network/src/handshake/tests.rs(1 hunks)crates/amaru-network/src/keepalive/messages.rs(1 hunks)crates/amaru-network/src/keepalive/mod.rs(1 hunks)crates/amaru-network/src/keepalive/tests.rs(1 hunks)crates/amaru-network/src/lib.rs(1 hunks)crates/amaru-network/src/mux.rs(5 hunks)crates/pure-stage/src/effect.rs(13 hunks)crates/pure-stage/src/simulation/resume.rs(1 hunks)crates/pure-stage/src/simulation/running.rs(4 hunks)crates/pure-stage/src/tokio.rs(3 hunks)crates/pure-stage/src/trace_buffer.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (16)
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-network/src/lib.rscrates/pure-stage/src/trace_buffer.rscrates/amaru-network/src/keepalive/tests.rscrates/amaru-network/src/connection.rscrates/amaru-network/src/handshake/tests.rscrates/pure-stage/src/tokio.rscrates/pure-stage/src/simulation/resume.rscrates/amaru-network/src/handshake/mod.rscrates/amaru-network/src/bytes.rscrates/amaru-network/src/keepalive/mod.rscrates/amaru-network/src/keepalive/messages.rscrates/pure-stage/src/simulation/running.rscrates/amaru-network/src/mux.rscrates/pure-stage/src/effect.rs
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 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:
crates/amaru-network/Cargo.toml
📚 Learning: 2025-06-14T16:31:53.134Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 263
File: simulation/amaru-sim/src/simulator/simulate.rs:298-300
Timestamp: 2025-06-14T16:31:53.134Z
Learning: StageRef in the pure-stage crate supports serde serialization and deserialization (derives serde::Serialize and serde::Deserialize), enabling it to be used in structs that also derive these traits for TraceBuffer and replay functionality.
Applied to files:
crates/pure-stage/src/trace_buffer.rscrates/pure-stage/src/tokio.rscrates/pure-stage/src/effect.rs
📚 Learning: 2025-05-12T14:21:27.470Z
Learnt from: stevana
Repo: pragma-org/amaru PR: 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:
crates/amaru-network/src/keepalive/tests.rscrates/amaru-network/src/handshake/tests.rs
📚 Learning: 2025-04-20T17:57:23.233Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285
Timestamp: 2025-04-20T17:57:23.233Z
Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.
Applied to files:
crates/amaru-network/src/keepalive/tests.rs
📚 Learning: 2025-08-12T12:28:24.027Z
Learnt from: etorreborre
Repo: pragma-org/amaru PR: 372
File: simulation/amaru-sim/src/simulator/mod.rs:410-412
Timestamp: 2025-08-12T12:28:24.027Z
Learning: In the Amaru project, panic statements are acceptable in simulation/test code (like amaru-sim crate) as they help identify configuration issues quickly during development, rather than needing proper error handling like production code.
Applied to files:
crates/amaru-network/src/keepalive/tests.rs
📚 Learning: 2025-09-29T16:44:14.807Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/protocol.rs:94-106
Timestamp: 2025-09-29T16:44:14.807Z
Learning: In the amaru-network crate protocol.rs file, the correct Cardano mini-protocol ID assignments are: PROTO_N2N_KEEP_ALIVE = 8 and PROTO_N2N_PEER_SHARE = 10, as verified against the network specification by the maintainer.
Applied to files:
crates/amaru-network/src/keepalive/tests.rscrates/amaru-network/src/keepalive/mod.rscrates/amaru-network/src/keepalive/messages.rs
📚 Learning: 2025-06-14T16:41:13.061Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 263
File: crates/pure-stage/src/simulation/running.rs:868-875
Timestamp: 2025-06-14T16:41:13.061Z
Learning: In the pure-stage simulation framework, the effect air-lock protocol is designed so that when a stage is polled, the stage implementation consumes/takes the value from the effect lock during polling. There's no need to manually clear the effect lock after Poll::Ready because "the other side will have taken the value out" - this is by design, not a bug.
Applied to files:
crates/pure-stage/src/tokio.rscrates/pure-stage/src/effect.rs
📚 Learning: 2025-09-29T16:39:24.001Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/mux.rs:317-325
Timestamp: 2025-09-29T16:39:24.001Z
Learning: In crates/amaru-network/src/mux.rs, the outgoing() method intentionally uses unwrap() after get_mut(&proto_id) as a fail-fast mechanism. This panic is designed to catch programming errors where an actor tries to send on an unregistered protocol, and should not be changed to return a Result since it represents internal code bugs that should terminate the process, not external input that should be handled gracefully.
Applied to files:
crates/amaru-network/src/handshake/mod.rscrates/amaru-network/src/mux.rs
📚 Learning: 2025-09-29T16:38:59.323Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/mux.rs:216-233
Timestamp: 2025-09-29T16:38:59.323Z
Learning: In crates/amaru-network/src/mux.rs, the segment length field uses u16 type which naturally limits values to 65535, matching MAX_SEGMENT_SIZE constant exactly. This provides type-level safety against oversized allocations without needing runtime bounds checking.
Applied to files:
crates/amaru-network/src/handshake/mod.rscrates/amaru-network/src/mux.rs
📚 Learning: 2025-08-08T14:35:35.562Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/transaction_pointer.rs:36-44
Timestamp: 2025-08-08T14:35:35.562Z
Learning: In the amaru project, when decoding CBOR arrays, prefer using minicbor_extra::heterogenous_array with the expected length to validate definite-length arrays and correctly handle indefinite-length arrays. Example: crates/amaru-kernel/src/transaction_pointer.rs Decode should use heterogenous_array(d, 2, …) instead of ignoring the length from d.array().
Applied to files:
crates/amaru-network/src/bytes.rs
📚 Learning: 2025-06-03T06:31:57.736Z
Learnt from: stevana
Repo: pragma-org/amaru PR: 236
File: simulation/amaru-sim/src/simulator/generate.rs:141-145
Timestamp: 2025-06-03T06:31:57.736Z
Learning: In the amaru project, the team prefers to use as_bytes() instead of hex::decode() for converting hash and header strings to bytes in simulation/amaru-sim/src/simulator/generate.rs, even though they appear to be hex-encoded strings.
Applied to files:
crates/amaru-network/src/bytes.rs
📚 Learning: 2025-08-08T14:43:20.218Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 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-network/src/bytes.rs
📚 Learning: 2025-06-14T16:36:04.502Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 263
File: crates/pure-stage/src/simulation/state.rs:33-36
Timestamp: 2025-06-14T16:36:04.502Z
Learning: In simulation and replay systems that require cloneable and serializable states, error types must often be converted to String rather than stored as trait objects (like Box<dyn Error> or anyhow::Error) because trait objects cannot be cloned, which breaks the snapshotting and replay functionality needed for deterministic simulation.
Applied to files:
crates/amaru-network/src/bytes.rs
📚 Learning: 2025-05-09T13:09:47.915Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 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/pure-stage/src/simulation/running.rs
📚 Learning: 2025-12-16T21:50:39.756Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/pure-stage/src/adapter.rs:67-100
Timestamp: 2025-12-16T21:50:39.756Z
Learning: In the pure-stage crate's adapter system (crates/pure-stage/src/adapter.rs), adapters cannot form cycles because an existing adapter cannot be repointed after creation. The Adapter's target field is immutable, preventing the formation of loops in the adapter chain.
Applied to files:
crates/pure-stage/src/simulation/running.rs
🧬 Code graph analysis (7)
crates/amaru-network/src/keepalive/tests.rs (4)
crates/amaru-network/src/connection.rs (2)
new(35-44)stage(77-90)crates/amaru-network/src/handshake/mod.rs (2)
new(48-60)stage(63-114)crates/amaru-network/src/keepalive/messages.rs (1)
new(21-23)crates/amaru-network/src/keepalive/mod.rs (1)
new(37-39)
crates/amaru-network/src/connection.rs (6)
crates/amaru-network/src/handshake/mod.rs (2)
new(48-60)stage(63-114)crates/amaru-network/src/keepalive/mod.rs (3)
new(37-39)initiator(42-83)responder(85-115)crates/amaru-network/src/mux.rs (4)
new(144-155)new(373-375)new(532-543)stage(200-219)crates/pure-stage/src/effect.rs (5)
new(79-93)new(496-498)stage(282-323)msg(310-311)state(309-309)crates/amaru-network/src/protocol.rs (1)
role(167-175)crates/amaru-network/src/handshake/messages.rs (1)
v11_and_above(39-85)
crates/amaru-network/src/handshake/tests.rs (1)
crates/amaru-network/src/handshake/mod.rs (1)
handler_transform(191-196)
crates/pure-stage/src/tokio.rs (3)
crates/pure-stage/src/simulation/blocked.rs (1)
name(140-140)crates/pure-stage/src/stage_ref.rs (1)
name(94-96)crates/pure-stage/src/stagegraph.rs (1)
stage_name(273-276)
crates/amaru-network/src/keepalive/messages.rs (1)
crates/amaru-network/src/keepalive/mod.rs (1)
new(37-39)
crates/pure-stage/src/simulation/running.rs (3)
crates/pure-stage/src/adapter.rs (1)
find_recipient(68-100)crates/pure-stage/src/simulation/resume.rs (1)
resume_contramap_internal(283-310)crates/pure-stage/src/stagegraph.rs (1)
stage_name(273-276)
crates/amaru-network/src/mux.rs (1)
crates/amaru-network/src/bytes.rs (1)
len(72-75)
🪛 GitHub Actions: Coding Practices
crates/amaru-network/src/keepalive/messages.rs
[error] 21-23: Clippy: 'new' without 'Default' for Cookie. Consider implementing Default for Cookie (e.g., implement Default by returning Self::new()). See: new_without_default lint suggestion at crates/amaru-network/src/keepalive/messages.rs:21-23.
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Build riscv32
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/linux
- GitHub Check: Build x86_64/linux
- GitHub Check: Build aarch64/macos
- GitHub Check: Test the CLI
- GitHub Check: Benches
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (46)
crates/amaru-network/Cargo.toml (1)
37-37: Good stuff, mate!Adding
rt-multi-threadto the dev-dependencies makes total sense here – like getting the right controller for the right console, yeah? The new keepalive and connection tests spin up a fullRuntime::new(), which needs that multi-threaded goodness to work properly.crates/amaru-network/src/mux.rs (5)
377-377: Tracing instrumentation updated – ripper!Skipping
handlerandefffrom the trace makes sense since they're not easily serializable. Like when you're streaming a game and don't want all the dev tools in shot, yeah?
437-437: Nice touch capturing the byte length in the trace fields.Good call using
skip_alland explicitly addingbytes = bytes.len()– you get the useful info without serializing the whole payload. It's like showing the loading bar percentage instead of every individual byte, mate.
448-448: Clean and consistent.Switching to
skip_allhere keeps things uniform across the muxer methods.
472-472: Solid instrumentation pattern.Skipping
bytesandeffwhile still capturing the byte length in fields – that's the sweet spot for debugging without log bloat.
487-487: Consistency FTW.Skipping
effaligns with the pattern established in the other methods.crates/amaru-network/src/lib.rs (1)
18-21: New modules on the block!Adding
connectionandkeepaliveto the crate's public API – like unlocking new levels in the networking game. The alphabetical ordering is maintained which keeps things tidy. Good on ya!crates/amaru-network/src/handshake/tests.rs (1)
197-198: Sweet use of the new helper function!Using
handshake::handler_transformhere is clean and DRY – like using a proper game engine instead of coding physics from scratch.crates/amaru-network/src/bytes.rs (3)
19-19: Derive updates look solid.Adding
Hashis handy for when you needNonEmptyBytesas a key in collections. Removing the derive forDeserializein favor of the manual impl below is the right call to maintain the non-empty invariant.
23-31: Proper validation on deserialize – nice one!This custom
Deserializeimpl ensures the non-empty invariant is enforced even when coming from serialized data. Like having a bouncer at the door who actually checks IDs, ya know? Usingserde::de::Error::customto surface theEmptyBytesErroris clean.
58-65: Encoding path now validates properly.Using
Self::new()instead of direct construction ensures the invariant is checked. The panic message "CBOR item should not be empty" is clear and accurate for debugging if something goes sideways.crates/amaru-network/src/keepalive/tests.rs (1)
71-73: Test structure looks good overall.Running the connection stage with a 10-second timeout is reasonable for an integration test. The
#[ignore]attribute is spot-on since this requires an external node to be running. It's like having a boss fight that only unlocks in co-op mode!crates/amaru-network/src/handshake/mod.rs (3)
17-18: Import reorganization looks good.Bringing
HandlerMessageandMuxMessagefrom the mux module directly keeps the imports clean and explicit.
31-32: Handy re-export!Making
VersionTablepublicly available from the handshake module means external callers don't have to dig intohandshake::messages– like having a fast-travel point right at the dungeon entrance.
190-196: Clean transformation helper.This
handler_transformfunction provides a proper public API for mappingHandlerMessagetoHandshakeMessage. The pattern matching is straightforward, and intentionally discarding the protocol ID fromRegistered(_)makes sense sinceHandshakeMessage::Registeredis just a unit-like variant signaling registration complete.crates/amaru-network/src/keepalive/mod.rs (2)
30-40: KeepAlive struct looks clean and minimal.Storing just the muxer reference and cookie is all you need for the keep-alive dance. Like a rhythm game – you just need to track the beat and the score!
85-115: [Rewritten review comment]
[Classification tag]crates/amaru-network/src/connection.rs (3)
1-26: All good here, mate!License header's proper and imports are nicely organized. Like a well-curated vinyl collection – everything's where it should be.
143-156: Solid handshake handling!Nice one – the
Accepted/Refusedmatch is clean, and bailing out with a log on refusal is the right call. Like a proper bouncer at a Dublin pub – polite refusal with a note for the records.
180-187: State assembly looks good!All the pieces are wired up properly. Keeping the
handshakeref around might be for future needs – no harm, no foul.crates/amaru-network/src/keepalive/messages.rs (3)
42-57: CBOR encoding for Cookie is spot on!Simple and clean – just encodes the u16 directly. No fuss, no muss. Like a well-executed speed run strat.
59-88: Message enum and encoding implementation looks solid!The CBOR structure with
[tag, cookie]for the keep-alive messages and[tag]for Done is clean and efficient. Good use of the array format with consistent tags. This'll serialize nice and compact over the wire.
90-115: Decoding logic is correct, though the validation could be cleaner.The length checks work, but the
label < 2condition is a bit like reading a puzzle box – takes a second to grok. It's not wrong, just not immediately obvious. If you fancy a slight refactor later:match label { 0 | 1 if len != 2 => return Err(decode::Error::message("expected array of length 2")), 2 if len != 1 => return Err(decode::Error::message("expected array of length 1")), 0 => Ok(Message::KeepAlive(d.decode()?)), 1 => Ok(Message::ResponseKeepAlive(d.decode()?)), 2 => Ok(Message::Done), _ => Err(decode::Error::message("can't decode Message")), }But honestly, what you've got works fine – just a style preference. Ship it!
crates/pure-stage/src/trace_buffer.rs (1)
142-143: LGTM! Nice one, mate!The new
ContramapResponseandAddStageResponsevariants are tucked into the Display formatting like a well-placed easter egg in a Zelda game. Keeps the output consistent with the rest of the response types - proper job!crates/pure-stage/src/tokio.rs (3)
248-248: LGTM!Good call adding the debug trace here - like having a minimap in an RPG, helps you know where you are in the game loop.
290-290: LGTM!Matching the debug trace in
run_stage_boxedtoo - consistency is key, like making sure both Mario and Luigi can double-jump.
442-458: LGTM! The Contramap handling looks solid.G'day, this interpreter branch follows the established pattern nicely - grab the transform, wrap it in an Adapter, pop it into the senders map, and respond with the new name. It's like setting up a pipe in a factory sim game: input goes in one end, transformation happens, output goes to the right destination.
The
stage_namecall ensures uniqueness (just like theAddStagehandling above), and thetransform.into_inner()properly unwraps theNoDebugwrapper. Fair dinkum work!crates/pure-stage/src/simulation/resume.rs (1)
282-309: LGTM! Follows the resume pattern to a T.Crikey, this function slots right into the existing resume_*_internal family like it was always meant to be there! The validation dance is spot-on:
- Check the stage is waiting for something
- Confirm it's the right Contramap effect with matching
originalandnew_name- Extract the transform only after all the checks pass
- Signal completion with the
ContramapResponseIt's like completing a quest chain in an MMO - you don't get the reward until all the conditions are met. Returning the boxed transform for the caller to wire up the Adapter is a clean separation of concerns.
crates/pure-stage/src/simulation/running.rs (4)
27-27: LGTM!Import updated to bring in
Adapter- essential for the contramap wiring below.
36-38: LGTM!The
resume_contramap_internalimport joins the squad alongside its resume function mates. Like assembling the party before the final dungeon!
756-777: LGTM! The handle_effect case for Contramap is bonzer.This follows the established pattern like a speedrunner following an optimal route:
- Generate a unique stage name using
stage_name- Get the transform from
resume_contramap_internal- Insert the Adapter into the stages map
The structure mirrors the
WireStagehandling above, which keeps the codebase predictable. TheAdaptergets the generatedname, theoriginalas target, and the extractedtransform. Clean as a whistle!
1115-1144: LGTM! Public API follows the established pattern.The
resume_contramapmethod is like the manual override in a racing game - lets you take direct control when needed. It mirrors the automatic handling inhandle_effectbut exposes it for fine-grained test control.Structure parallels
resume_wire_stagenicely: get data, extract transform via internal function, insert adapter. Consistency across the codebase is ace!crates/pure-stage/src/effect.rs (14)
281-281: LGTM!Switching from
#[allow(...)]to#[expect(...)]is the right call - it's like setting your game difficulty to "challenge mode" where the compiler will ping you if the lint no longer applies.
325-354: LGTM! The contramap method is well-crafted.This is the business end of the contramap feature, mate! Like a functor but going the other way (contravariant style), it lets you create a mapped stage reference that transforms messages before delivery.
The implementation is clever:
- Wraps the user's
transform: Fn(Mapped) -> Originalin a boxed closure that handles theBox<dyn SendData>dance- Uses the airlock pattern to emit the effect and await the response
- Returns a properly typed
StageRef<Mapped>so the type system keeps everyone honestThe
NoDebugwrapper on the transform is consistent with howWireStagehandles its transition factory.
356-356: LGTM!Another
#[expect(...)]conversion - keeping the linting consistent across async methods.
572-576: LGTM!The
StageEffect::Contramapvariant captures everything needed for the effect lifecycle:
original: the target stage we're contramapping fromnew_name: the desired name prefix for the new adaptertransform: the boxed transformation function (wrapped inNoDebugsince closures don't impl Debug)Clean struct-like enum variant - much more readable than a tuple variant would be for three fields.
591-591: LGTM!
ContramapResponse(Name)follows the same pattern asAddStageResponse(Name)- returns the generated name so the caller knows how to reference the new adapter. Symmetry is satisfying!
619-622: LGTM!JSON serialization for the response - straightforward and matches the pattern of
AddStageResponse.
641-641: LGTM!Display impl for
ContramapResponse- clean and informative.
714-729: LGTM! The split() handling is spot-on.This is crucial - when splitting a
StageEffect::Contramap:
- The
StageEffectkeeps thetransform(needed when resuming the effect)- The
Effectonly gets the serializable bits (at_stage,original,new_name)This separation ensures the non-serializable closure stays in memory while only the metadata gets exposed. It's like keeping your save game data separate from the game engine internals. Proper design!
779-783: LGTM!The
Effect::Contramapvariant is the externally-visible form - no transform, just names. Perfect for tracing, serialization, and debugging without leaking implementation details.
869-878: LGTM!JSON output includes all the identifying info -
at_stage,original, andnew_name. Useful for debugging and trace analysis.
919-925: LGTM!Display format
"contramap {at_stage} {original} -> {new_name}"tells the story nicely - shows where the contramap was created and what it's mapping from/to. Like reading a quest log!
1011-1011: LGTM!The
at_stage()accessor correctly returns the stage that initiated the contramap effect. Consistent with all other effect variants.
1337-1352: LGTM!
PartialEqimplementation forEffect::Contramapcompares all three name fields. This enables effect assertions in tests - comparing effects by their observable properties rather than identity.
149-149: The'staticremoval is sound—no compilation issues expected.The closure is immediately invoked and consumed; it needn't outlive the function. Removing
'staticwhile keepingSendis correct. This change is backward-compatible since it's a parameter bound (not a return constraint), so callers passing'staticclosures will still work. No downstream code relying on the bound should break.
Signed-off-by: Roland Kuhn <[email protected]>
Signed-off-by: etorreborre <[email protected]>
…n-protocol-with-pure-stage feat: add the tx submission protocol to the pure-stage network stack
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Nitpick comments (27)
crates/amaru-network/Cargo.toml (1)
41-41: Tokio runtime upgrade to multi-threaded - worth confirming this is needed.You've switched from the basic
rtfeature tort-multi-threadfor tokio in dev-dependencies. This gives you the full concurrent executor which is heavier but more powerful. Makes sense if your tests are spawning multiple tasks that need true parallelism, but if they're just simple async tests, the lighterrtmight be sufficient.Just a friendly heads-up to confirm this upgrade is intentional - like choosing between a bicycle and a motorcycle for a short trip. Both get you there, but one's got more overhead!
crates/amaru-network/src/socket.rs (1)
168-184: Handy test utility, but those unwraps are a bit cheeky!This test helper is a ripper - it consolidates the connection setup logic for tests, reads config from the
PEERenv var, and wraps everything in a 5-second timeout. Good stuff!However, lines 179-180 use
.unwrap()calls that will panic if address resolution or connection fails. In test code this is generally acceptable (fail fast, fail loud), but since you're already wrapping the whole thing in a timeout that returnsanyhow::Result, you might want to propagate errors properly with?instead. That way you get better error messages when things go sideways.// Consider: let addr = crate::socket_addr::ToSocketAddrs::String( std::env::var("PEER").unwrap_or_else(|_| "127.0.0.1:3000".to_string()), ) .resolve() .await?; conn.connect(addr).await?This would bubble up errors through the
anyhow::Resultreturn type rather than panicking. But hey, if the "panic on test failure" approach is the team's preference, no worries!crates/amaru-network/src/tx_submission/tests/test_cases.rs (1)
66-84: Solid concurrency test pattern, though there's a wee redundant clone.The concurrent mempool filling approach is a bonzer way to test race conditions between the initiator filling and responder fetching. Using
tokio::join!to await both tasks is proper form.One small thing: Line 71's
initiator_mempool.clone()is redundant sinceArc::clonewas already done on line 55, andArcisSend + Sync. You could just moveinitiator_mempooldirectly intotask2since it's not used after this point.🔎 Optional simplification
let txs_clone = txs.clone(); - let initiator_mempool = initiator_mempool.clone(); let task2 = tokio::spawn(async move { for tx in txs_clone { initiator_mempool.add(tx.clone()).unwrap(); } });crates/amaru-mempool/Cargo.toml (1)
18-18: Minor style inconsistency – quotes aroundworkspacekey.Fair dinkum, this works, but the quoted
"workspace"key is inconsistent with the other dependencies in this file and across the repo. Keeping it consistent makes the manifest easier to scan.🔎 Suggested consistency fix
-tokio = { "workspace" = true, features = ["time", "macros", "rt"] } +tokio = { workspace = true, features = ["time", "macros", "rt"] }crates/amaru-network/src/handshake/tests.rs (2)
83-93: Consider usinghandler_transformhere for consistency with the other test.The second test (
test_against_node_with_tokio) at line 174 useshandshake::handler_transformdirectly, which is cleaner. This test manually inlines the same logic. It works, but using the shared function would reduce duplication – like reusing assets instead of copy-pasting sprites.🔎 Optional: use handler_transform function
let handshake_bytes = - network.contramap( - handshake, - "handshake_bytes", - |msg: HandlerMessage| match msg { - HandlerMessage::FromNetwork(bytes) => { - handshake::HandshakeMessage::FromNetwork(bytes) - } - HandlerMessage::Registered(_) => handshake::HandshakeMessage::Registered, - }, - ); + network.contramap(handshake, "handshake_bytes", handshake::handler_transform);
169-169: Minor inconsistency:test_against_nodeuses a localnetwork_magicvariable, but this test usesNetworkMagic::MAINNETdirectly.Not a blocker, but for consistency you could extract it to a local variable here too, like the other test does. Makes it easier to change if you ever want to test against a different network.
🔎 Optional: extract network_magic variable
+ let network_magic = NetworkMagic::MAINNET; + let handshake = network.stage("handshake", handshake::stage); let handshake = network.wire_up( handshake, handshake::Handshake::new( mux.clone().without_state(), output, Role::Initiator, - VersionTable::v11_and_above(NetworkMagic::MAINNET, true), + VersionTable::v11_and_above(network_magic, true), ), );And update line 195 similarly.
crates/amaru-network/src/tx_submission/tests/assertions.rs (2)
31-36: Dead initialization here – like an unused save fileThe
let mut actual = vec![];on line 31 is immediately overwritten on line 33. It's like loading a game just to start a new save – pointless, mate. You can combine these into a single declaration.🔎 Proposed fix
- let mut actual = vec![]; - - actual = tx_ids + let actual: Vec<_> = tx_ids .iter() .filter(|tx_id| mempool.contains(tx_id)) .collect();
21-25: Doc comment says "eventually" but there's no waiting logicThe doc says "eventually present" which sounds like it'll wait and retry, but this function is more of a "right now or bust" check. Might want to update the doc to match the actual behaviour, or implement actual retry logic if that's the intent. Just a heads up so future-you doesn't get confused!
crates/amaru-network/src/tx_submission/responder_params.rs (2)
68-76: Optional: Could simplify the match with an if expressionNot a biggie at all, just a style thing – like choosing between keyboard and controller. The explicit match is perfectly readable, but if you fancy a shorter version:
🔎 Alternative implementation
impl<'b> Decode<'b, ()> for Blocking { fn decode(d: &mut Decoder<'b>, ctx: &mut ()) -> Result<Self, Error> { let value = bool::decode(d, ctx)?; - match value { - true => Ok(Blocking::Yes), - false => Ok(Blocking::No), - } + Ok(if value { Blocking::Yes } else { Blocking::No }) } }
78-85: Consider adding the reverse conversion for symmetryYou've got
From<Blocking> for bool, which is grand. For completeness, you might wantFrom<bool> for Blockingas well – like having fast travel work both ways. Not blocking (pun intended), just a thought!🔎 Proposed addition
impl From<bool> for Blocking { fn from(value: bool) -> Self { if value { Blocking::Yes } else { Blocking::No } } }crates/amaru/src/stages/mod.rs (1)
301-327: Solid temporary connection helper with proper error handlingThe "Temporary function" comment is appreciated – like a "work in progress" sign at a construction site. The 5-second timeout is sensible, and the error messages are descriptive. The double
?on line 321 correctly unwraps both the timeout result and the connection result.One small thing: if the timeout fires, users will see a generic timeout error. You might want to wrap it with more context:
🔎 Optional: Add context to timeout error
timeout(Duration::from_secs(5), async { match ToSocketAddrs::String(env::var("PEER").unwrap_or(peer.to_string())) .resolve() .await { Ok(addr) => conn.connect(addr).await.map_err(anyhow::Error::from), Err(e) => Err(anyhow::anyhow!( "Failed to resolve address for upstream peer {}: {}", peer, e )), } }) - .await? + .await + .context(format!("Timed out connecting to upstream peer {}", peer))?crates/amaru-network/src/tx_submission/outcome.rs (1)
106-115: Consider using directProtocolErrorcomparison instead of string formatting.G'day mate! This
PartialEqimpl is doing a bit of extra work, like taking the scenic route through Melbourne when you could just hop on the tram. SinceProtocolErroralready derivesPartialEq, you can compare errors directly instead of going throughformat!("{}", ...). It's like in Dark Souls - why roll twice when one roll does the job?🔎 Proposed fix
impl PartialEq for Outcome { fn eq(&self, other: &Self) -> bool { match (self, other) { (Outcome::Done, Outcome::Done) => true, - (Outcome::Error(e1), Outcome::Error(e2)) => format!("{}", e1) == format!("{}", e2), + (Outcome::Error(e1), Outcome::Error(e2)) => e1 == e2, (Outcome::Send(msg1), Outcome::Send(msg2)) => msg1 == msg2, _ => false, } } }crates/amaru-network/src/tx_submission/messages.rs (2)
84-89: Consider documenting the skipped message label 5.Oi! I notice the message labels go 0, 1, 2, 3, 4, then jump to 6 - like skipping level 5 in an old-school cheat code. This is probably intentional for protocol compatibility, but a quick comment explaining why label 5 is reserved/unused would be helpful for future devs who stumble upon this. Nobody wants to be scratching their head like trying to understand the Metal Gear Solid timeline!
🔎 Suggested comment
Message::Done => { e.array(1)?.u16(4)?; } + // Note: Label 5 is reserved/unused in the txsubmission protocol Message::Init => { e.array(1)?.u16(6)?; }
234-238: Potential length mismatch in test helper when zipping vectors.Heads up, mate! When you
ziptwo independently generated vectors with different size ranges (0..20each), the shorter one wins and the extra elements from the longer one get dropped like loot you can't carry. This is probably fine for property tests (you're still getting valid test cases), but if you wanted exact pairings, you could generate(TxId, u32)tuples directly.No drama if this is intentional - just making sure it's not a sneaky bug like a mimic chest in Dark Souls!
🔎 Alternative approach for exact pairing
prop_compose! { - fn any_tx_id_and_sizes_vec()(ids in vec(any_tx_id(), 0..20), sizes in vec(any::<u32>(), 0..20)) -> Vec<(TxId, u32)> { - ids.iter().zip(sizes).map(|(id, size)| (*id, size)).collect() + fn any_tx_id_and_sizes_vec()(pairs in vec((any_tx_id(), any::<u32>()), 0..20)) -> Vec<(TxId, u32)> { + pairs } }crates/amaru-network/src/tx_submission/initiator_state.rs (1)
29-29: Consider making the protocol limit configurable.The
MAX_REQUESTED_TX_IDS = 10is hardcoded here. While this is probably fine for now, you might want to consider making it configurable viaTxSubmissionInitiatorState::new()or a config struct in the future - especially if different network configurations need different limits. Just a thought for the backlog, like a side quest you can pick up later!crates/amaru-network/src/tx_submission/tests/nodes.rs (1)
59-65: Consider handling the unwrap for better test diagnostics.The
.unwrap()here will panic if insertion fails, which is probably fine for happy-path tests. But like getting a cryptic "You Died" screen in Dark Souls, a bare panic won't tell you why it failed. Consider using.expect("descriptive message")to make test failures more informative.🔎 Proposed improvement
pub fn insert_client_transactions(&self, txs: &[Tx]) { for tx in txs.iter() { self.initiator_mempool .insert(tx.clone(), amaru_ouroboros_traits::TxOrigin::Remote) - .unwrap(); + .expect("failed to insert transaction into initiator mempool"); } }crates/amaru-network/src/keepalive/mod.rs (1)
79-86: Timing and logging order could use a tweak.Two small things here, mate:
The
eff.wait(Duration::from_secs(1))at line 79 runs on every message, includingRegistered. So the first keepalive is delayed by 1 second even though there's no prior response to wait for. Might be intentional, but worth a double-check.The debug log at line 85 says "sending" but it's logged after the
call()completes. Like announcing "I'm about to attack!" after you've already swung—bit confusing for debugging. Consider moving it before thecall().🔎 Proposed reordering
eff.wait(Duration::from_secs(1)).await; + tracing::debug!(?state.cookie, "sending KeepAlive message"); eff.call(&state.muxer, NETWORK_SEND_TIMEOUT, |cr| { let msg = NonEmptyBytes::encode(&Message::KeepAlive(state.cookie)); MuxMessage::Send(PROTO_N2N_KEEP_ALIVE.erase(), msg, cr) }) .await; - tracing::debug!(?state.cookie, "sending KeepAlive message"); statecrates/amaru-network/src/tx_submission/responder_state.rs (1)
192-206: Unnecessary generic type parameter onreceived_tx_ids.This function has a generic
<Tx: Send + Sync + 'static>but the struct itself is already bound to a specificTxtype through the mempool trait. The generic shadows the importedTxtype, which could cause confusion. Since you're only callingmempool.contains()which doesn't need the generic, you could simplify this.🔎 Proposed simplification
- fn received_tx_ids<Tx: Send + Sync + 'static>( + fn received_tx_ids( &mut self, - mempool: &dyn TxSubmissionMempool<Tx>, + mempool: &dyn TxSubmissionMempool<amaru_kernel::Tx>, tx_ids: Vec<(TxId, u32)>, ) {Or just remove the generic entirely since
TxSubmissionMempool::containsonly needs&TxId:- fn received_tx_ids<Tx: Send + Sync + 'static>( - &mut self, - mempool: &dyn TxSubmissionMempool<Tx>, - tx_ids: Vec<(TxId, u32)>, - ) { + fn received_tx_ids(&mut self, mempool: &dyn TxSubmissionMempool<Tx>, tx_ids: Vec<(TxId, u32)>) {crates/amaru-ouroboros-traits/src/can_validate_transactions/mod.rs (2)
60-86: Serde and PartialEq impls are pragmatic but worth documenting in the type docs.The serialization is intentionally lossy (error message only), which is fine and documented in the deserialize impl comment. However, the
PartialEqbased on string comparison has a subtle implication: two errors with the same message but different underlying types will compare equal. This is probably fine for your use case, but might surprise someone down the track.Consider adding a note to the struct's doc comment about this behavior, like a proper "known issues" section in release notes.
🔎 Proposed doc addition
+/// A wrapper for transaction validation errors. +/// +/// # Serialization +/// This type serializes to the error message string only. The original error type +/// is lost during serialization/deserialization. +/// +/// # Equality +/// Two errors are considered equal if their string representations match, +/// regardless of the underlying error type. #[derive(Debug)] pub struct TransactionValidationError(anyhow::Error);
82-86: Consider implementingstd::error::Errorfor broader interop.The error type has
DebugandDisplay, but doesn't implementstd::error::Error. This might limit interop with other error handling libraries (likeeyre) or frameworks that expect the trait. Not a blocker, but something to consider for the future.🔎 Proposed addition
+impl std::error::Error for TransactionValidationError { + fn source(&self) -> Option<&(dyn std::error::Error + 'static)> { + self.0.source() + } +}crates/amaru-network/src/tx_submission/tests/test_data.rs (2)
39-45: Consider using iterator collect for a more idiomatic approach.This loop is giving me Groundhog Day vibes – it works, but Rust's iterators could make it a bit slicker:
🔎 Optional refactor
pub fn create_transactions(number: u64) -> Vec<Tx> { - let mut txs = vec![]; - for i in 0..number { - txs.push(create_transaction(i)); - } - txs + (0..number).map(create_transaction).collect() }
47-55: Same iterator pattern could apply here.Like the previous function, this one could be slightly more concise. Though I get that the imperative style makes the side effect (adding to mempool) more explicit, which is fair dinkum for readability.
crates/amaru-network/src/tx_submission/tests/nodes_options.rs (1)
73-78: Hardcoded capacity of 4 inwith_responder_tx_validator.Hey mate, just a heads up – this method hardcodes the capacity to 4. If someone's trying to test with a different capacity but custom validator, they might be scratching their head wondering why their capacity setting got ignored. It's like ordering a custom pizza but always getting the same crust size!
Maybe worth a comment, or consider adding a parameter for capacity:
🔎 Optional enhancement
pub fn with_responder_tx_validator( self, + capacity: u64, tx_validator: Arc<dyn CanValidateTransactions<Tx>>, ) -> Self { - self.with_responder_mempool(Arc::new(SizedMempool::with_tx_validator(4, tx_validator))) + self.with_responder_mempool(Arc::new(SizedMempool::with_tx_validator(capacity, tx_validator))) }crates/amaru-network/src/mempool_effects.rs (1)
299-401: Test module with ConstantMempool is comprehensive.The
ConstantMempoolis a decent test double – returns constant/predictable values. MissingFutureimport here too (line 384), but same fix applies.One nitpick – there's
#[allow(dead_code)]onConstantMempoolandnew(). If they're used in tests, they shouldn't need this. If they're not used, maybe yeet them?🔎 Proposed fix for test imports
use pallas_primitives::conway::{PseudoTransactionBody, PseudoTx, WitnessSet}; +use std::future::Future; use std::pin::Pin;crates/amaru-mempool/src/strategies/in_memory_mempool.rs (3)
134-151: Redundant sort detected – BTreeMap's already got your back.
BTreeMap::range()returns entries in sorted key order, so the.sort_by_key()at line 149 is doing nothing but burning CPU cycles. It's like sorting your alphabetically-organized vinyl collection again – unnecessary work!🔎 Proposed fix
let mut result: Vec<(TxId, u32, MempoolSeqNo)> = self .entries_by_seq .range(from_seq..) .take(limit as usize) .map(|(seq, tx_id)| { let Some(entry) = self.entries_by_id.get(tx_id) else { panic!( "Inconsistent mempool state: entry missing for tx_id {:?}", tx_id ) }; (*tx_id, entry.tx_size, *seq) }) .collect(); - result.sort_by_key(|(_, _, seq_no)| *seq_no); result
153-166: O(n×m) lookup – consider using a HashSet for the ids.Currently,
ids.contains(*key)is O(m) for each of the n entries, giving O(n×m) complexity. For larger mempools or id lists, this could become a bottleneck – like trying to find Wally in a crowd by checking every person one by one!Using a
HashSetfor the ids would drop this to O(n), which is a nice win for scalability.🔎 Proposed fix
+ use std::collections::HashSet; + /// Retrieves transactions for the given ids, sorted by their sequence number. fn get_txs_for_ids(&self, ids: &[TxId]) -> Vec<Tx> { + let ids_set: HashSet<&TxId> = ids.iter().collect(); // Make sure that the result are sorted by seq_no let mut result: Vec<(&TxId, &MempoolEntry<Tx>)> = self .entries_by_id .iter() - .filter(|(key, _)| ids.contains(*key)) + .filter(|(key, _)| ids_set.contains(key)) .collect(); result.sort_by_key(|(_, entry)| entry.seq_no); result .into_iter() .map(|(_, entry)| entry.tx.clone()) .collect() }
259-286:acknowledgecallskeys()twice per entry – minor inefficiency.The closure
keys(&entry.tx)is invoked once during the filter to collectseq_nos_to_remove, and again duringretain. Ifkeys()is expensive (e.g., involves parsing or hashing), this doubles the work. Not a blocker for an in-memory impl, but worth noting for future optimization.🔎 One-pass approach
fn acknowledge<TxKey: Ord, I>(&self, tx: &Tx, keys: fn(&Tx) -> I) where I: IntoIterator<Item = TxKey>, Self: Sized, { let keys_to_remove = BTreeSet::from_iter(keys(tx)); let mut inner = self.inner.write(); - // remove entries matching the keys criteria in both maps - let seq_nos_to_remove: Vec<MempoolSeqNo> = inner - .entries_by_id - .values() - .filter(|entry| { - keys(&entry.tx) - .into_iter() - .any(|k| keys_to_remove.contains(&k)) - }) - .map(|entry| entry.seq_no) - .collect(); - inner.entries_by_id.retain(|_, entry| { - !keys(&entry.tx) - .into_iter() - .any(|k| keys_to_remove.contains(&k)) - }); - for seq_no in seq_nos_to_remove { - inner.entries_by_seq.remove(&seq_no); - } + // Single pass: collect seq_nos to remove while filtering entries_by_id + let mut seq_nos_to_remove = Vec::new(); + inner.entries_by_id.retain(|_, entry| { + let dominated = keys(&entry.tx) + .into_iter() + .any(|k| keys_to_remove.contains(&k)); + if dominated { + seq_nos_to_remove.push(entry.seq_no); + } + !dominated + }); + for seq_no in seq_nos_to_remove { + inner.entries_by_seq.remove(&seq_no); + } }
📜 Review details
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (50)
Cargo.toml(3 hunks)crates/amaru-consensus/Cargo.toml(1 hunks)crates/amaru-consensus/src/consensus/effects/base_effects.rs(3 hunks)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs(10 hunks)crates/amaru-consensus/src/consensus/effects/network_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/mod.rs(0 hunks)crates/amaru-consensus/src/consensus/stages/forward_chain.rs(1 hunks)crates/amaru-kernel/src/is_header/mod.rs(1 hunks)crates/amaru-kernel/src/is_header/tip.rs(1 hunks)crates/amaru-kernel/src/lib.rs(1 hunks)crates/amaru-mempool/Cargo.toml(1 hunks)crates/amaru-mempool/src/lib.rs(1 hunks)crates/amaru-mempool/src/strategies/dummy.rs(2 hunks)crates/amaru-mempool/src/strategies/in_memory_mempool.rs(1 hunks)crates/amaru-mempool/src/strategies/mod.rs(1 hunks)crates/amaru-network/Cargo.toml(2 hunks)crates/amaru-network/src/connection.rs(1 hunks)crates/amaru-network/src/handshake/tests.rs(8 hunks)crates/amaru-network/src/keepalive/mod.rs(1 hunks)crates/amaru-network/src/keepalive/tests.rs(1 hunks)crates/amaru-network/src/lib.rs(1 hunks)crates/amaru-network/src/mempool_effects.rs(1 hunks)crates/amaru-network/src/socket.rs(1 hunks)crates/amaru-network/src/tx_submission/initiator_state.rs(1 hunks)crates/amaru-network/src/tx_submission/messages.rs(1 hunks)crates/amaru-network/src/tx_submission/mod.rs(1 hunks)crates/amaru-network/src/tx_submission/outcome.rs(1 hunks)crates/amaru-network/src/tx_submission/responder_params.rs(1 hunks)crates/amaru-network/src/tx_submission/responder_state.rs(1 hunks)crates/amaru-network/src/tx_submission/stage.rs(1 hunks)crates/amaru-network/src/tx_submission/tests/assertions.rs(1 hunks)crates/amaru-network/src/tx_submission/tests/faulty_tx_validator.rs(1 hunks)crates/amaru-network/src/tx_submission/tests/mod.rs(1 hunks)crates/amaru-network/src/tx_submission/tests/nodes.rs(1 hunks)crates/amaru-network/src/tx_submission/tests/nodes_options.rs(1 hunks)crates/amaru-network/src/tx_submission/tests/sized_mempool.rs(1 hunks)crates/amaru-network/src/tx_submission/tests/system_test.rs(1 hunks)crates/amaru-network/src/tx_submission/tests/test_cases.rs(1 hunks)crates/amaru-network/src/tx_submission/tests/test_data.rs(1 hunks)crates/amaru-ouroboros-traits/Cargo.toml(1 hunks)crates/amaru-ouroboros-traits/src/can_validate_transactions/mock.rs(1 hunks)crates/amaru-ouroboros-traits/src/can_validate_transactions/mod.rs(1 hunks)crates/amaru-ouroboros-traits/src/lib.rs(1 hunks)crates/amaru-ouroboros-traits/src/mempool.rs(1 hunks)crates/amaru/Cargo.toml(1 hunks)crates/amaru/src/stages/build_stage_graph.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(1 hunks)crates/amaru/src/stages/mod.rs(4 hunks)simulation/amaru-sim/src/simulator/run.rs(1 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru-consensus/src/consensus/mod.rs
✅ Files skipped from review due to trivial changes (1)
- crates/amaru-consensus/src/consensus/effects/network_effects.rs
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/amaru-network/src/keepalive/tests.rs
🧰 Additional context used
🧠 Learnings (26)
📚 Learning: 2025-08-23T15:44:44.318Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 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-kernel/src/lib.rscrates/amaru-kernel/src/is_header/mod.rscrates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rscrates/amaru-kernel/src/is_header/tip.rssimulation/amaru-sim/src/simulator/run.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.rscrates/amaru-consensus/src/consensus/effects/consensus_effects.rs
📚 Learning: 2025-12-16T21:32:37.668Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 584
File: crates/amaru-network/src/handshake/tests.rs:40-47
Timestamp: 2025-12-16T21:32:37.668Z
Learning: In Rust, shadowing a binding with a new let does not drop the previous binding until the end of the scope. All shadowed bindings in a scope are dropped in reverse-declaration order when the scope ends. Therefore, multiple let _guard = register_*() calls will keep all guards alive until the end of the function (or the surrounding scope). When reviewing code, be mindful that resources tied to shadowed bindings persist longer than the most recent binding; to release early, constrain the lifetime in an inner block or explicitly drop guards when appropriate.
Applied to files:
crates/amaru-kernel/src/lib.rscrates/amaru-network/src/socket.rscrates/amaru-kernel/src/is_header/mod.rscrates/amaru-ouroboros-traits/src/lib.rscrates/amaru-network/src/tx_submission/tests/system_test.rscrates/amaru-consensus/src/consensus/stages/forward_chain.rscrates/amaru-network/src/lib.rscrates/amaru-mempool/src/lib.rscrates/amaru-consensus/src/consensus/effects/base_effects.rscrates/amaru-network/src/tx_submission/responder_state.rscrates/amaru-network/src/tx_submission/tests/nodes.rscrates/amaru-network/src/tx_submission/tests/assertions.rscrates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rscrates/amaru-network/src/tx_submission/tests/test_data.rscrates/amaru-network/src/tx_submission/responder_params.rscrates/amaru-network/src/tx_submission/messages.rscrates/amaru-kernel/src/is_header/tip.rscrates/amaru-network/src/keepalive/mod.rscrates/amaru-ouroboros-traits/src/can_validate_transactions/mock.rscrates/amaru-ouroboros-traits/src/can_validate_transactions/mod.rscrates/amaru-network/src/tx_submission/initiator_state.rscrates/amaru-network/src/tx_submission/tests/faulty_tx_validator.rscrates/amaru-network/src/tx_submission/tests/test_cases.rscrates/amaru/src/stages/mod.rscrates/amaru-network/src/tx_submission/tests/mod.rscrates/amaru-network/src/tx_submission/stage.rssimulation/amaru-sim/src/simulator/run.rscrates/amaru-mempool/src/strategies/mod.rscrates/amaru-network/src/connection.rscrates/amaru/src/stages/build_stage_graph.rscrates/amaru-network/src/handshake/tests.rscrates/amaru-network/src/tx_submission/outcome.rscrates/amaru-network/src/mempool_effects.rscrates/amaru-network/src/tx_submission/tests/sized_mempool.rscrates/amaru-ouroboros-traits/src/mempool.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.rscrates/amaru-network/src/tx_submission/mod.rscrates/amaru-mempool/src/strategies/dummy.rscrates/amaru-consensus/src/consensus/effects/consensus_effects.rscrates/amaru-network/src/tx_submission/tests/nodes_options.rscrates/amaru-mempool/src/strategies/in_memory_mempool.rs
📚 Learning: 2025-05-12T14:21:27.470Z
Learnt from: stevana
Repo: pragma-org/amaru PR: 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:
crates/amaru-network/src/tx_submission/tests/system_test.rscrates/amaru-network/src/tx_submission/tests/nodes.rscrates/amaru-network/src/tx_submission/tests/assertions.rssimulation/amaru-sim/src/simulator/run.rscrates/amaru-network/src/handshake/tests.rscrates/amaru-consensus/src/consensus/effects/consensus_effects.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 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/stages/forward_chain.rscrates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 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/src/stages/consensus/forward_chain/tcp_forward_chain_server.rscrates/amaru-kernel/src/is_header/tip.rssimulation/amaru-sim/src/simulator/run.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.rs
📚 Learning: 2025-08-18T08:10:32.640Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 374
File: crates/amaru-stores/src/in_memory/mod.rs:427-433
Timestamp: 2025-08-18T08:10:32.640Z
Learning: The MemoryStore in crates/amaru-stores/src/in_memory/mod.rs is planned for a major revamp, so unimplemented methods like set_proposals_roots and set_constitution are intentionally left as placeholders until the revamp is complete.
Applied to files:
crates/amaru-mempool/src/lib.rscrates/amaru/src/stages/mod.rscrates/amaru-mempool/src/strategies/mod.rs
📚 Learning: 2025-08-18T08:10:35.849Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 374
File: crates/amaru-stores/src/in_memory/mod.rs:431-433
Timestamp: 2025-08-18T08:10:35.849Z
Learning: The MemoryStore in crates/amaru-stores/src/in_memory/mod.rs is planned for a major revamp soon, so unimplemented methods like set_constitution, set_proposals_roots are intentionally left aside until the revamp is complete.
Applied to files:
crates/amaru-mempool/src/lib.rscrates/amaru-mempool/src/strategies/mod.rs
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 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:
crates/amaru-network/Cargo.tomlcrates/amaru/Cargo.tomlcrates/amaru/src/stages/mod.rscrates/amaru-network/src/tx_submission/tests/mod.rscrates/amaru-mempool/src/strategies/mod.rsCargo.tomlcrates/amaru-consensus/Cargo.tomlcrates/amaru/src/stages/build_stage_graph.rscrates/amaru-mempool/Cargo.tomlcrates/amaru-ouroboros-traits/Cargo.tomlcrates/amaru-network/src/tx_submission/mod.rscrates/amaru-consensus/src/consensus/effects/consensus_effects.rs
📚 Learning: 2025-08-12T12:28:24.027Z
Learnt from: etorreborre
Repo: pragma-org/amaru PR: 372
File: simulation/amaru-sim/src/simulator/mod.rs:410-412
Timestamp: 2025-08-12T12:28:24.027Z
Learning: In the Amaru project, panic statements are acceptable in simulation/test code (like amaru-sim crate) as they help identify configuration issues quickly during development, rather than needing proper error handling like production code.
Applied to files:
crates/amaru-network/Cargo.tomlcrates/amaru-network/src/tx_submission/tests/mod.rsCargo.tomlcrates/amaru-ouroboros-traits/Cargo.tomlcrates/amaru-consensus/src/consensus/effects/consensus_effects.rs
📚 Learning: 2025-06-14T16:31:53.134Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 263
File: simulation/amaru-sim/src/simulator/simulate.rs:298-300
Timestamp: 2025-06-14T16:31:53.134Z
Learning: StageRef in the pure-stage crate supports serde serialization and deserialization (derives serde::Serialize and serde::Deserialize), enabling it to be used in structs that also derive these traits for TraceBuffer and replay functionality.
Applied to files:
crates/amaru-consensus/src/consensus/effects/base_effects.rscrates/amaru-network/src/tx_submission/stage.rs
📚 Learning: 2025-05-05T08:15:24.192Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 206
File: crates/pure-stage/src/simulation/state.rs:4-8
Timestamp: 2025-05-05T08:15:24.192Z
Learning: The `State` trait in the pure-stage crate already requires `Send` with its definition: `pub trait State: Any + fmt::Debug + Send + 'static`, making additional `+ Send` bounds redundant when using `Box<dyn State>`.
Applied to files:
crates/amaru-consensus/src/consensus/effects/base_effects.rs
📚 Learning: 2025-04-20T17:57:23.233Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward/test_infra.rs:272-285
Timestamp: 2025-04-20T17:57:23.233Z
Learning: In test infrastructure code, rkuhn prefers explicit panics (using .unwrap() or similar) over returning Result types, as test failures should be immediate and obvious.
Applied to files:
crates/amaru-network/src/tx_submission/tests/assertions.rscrates/amaru-network/src/tx_submission/tests/mod.rscrates/amaru-network/src/handshake/tests.rscrates/amaru-consensus/src/consensus/effects/consensus_effects.rs
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 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-network/src/tx_submission/tests/assertions.rscrates/amaru/src/stages/mod.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.rscrates/amaru-consensus/src/consensus/effects/consensus_effects.rscrates/amaru-mempool/src/strategies/in_memory_mempool.rs
📚 Learning: 2025-08-08T14:46:53.013Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/pool_params.rs:107-116
Timestamp: 2025-08-08T14:46:53.013Z
Learning: In crates/amaru-kernel/src/pool_params.rs, when serializing Relay::SingleHostAddr IPv6 to text, the project intentionally reverses each 4-byte chunk before constructing std::net::Ipv6Addr. This matches cardano-ledger’s IPv6 representation (four little-endian Word32 chunks). Do not “simplify” by passing the raw 16 bytes directly to Ipv6Addr::from; that would break ledger compatibility.
Applied to files:
crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs
📚 Learning: 2025-02-03T11:15:22.640Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 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/src/stages/consensus/forward_chain/tcp_forward_chain_server.rscrates/amaru-consensus/src/consensus/effects/consensus_effects.rs
📚 Learning: 2025-09-05T17:30:55.869Z
Learnt from: etorreborre
Repo: pragma-org/amaru PR: 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-network/src/tx_submission/responder_params.rscrates/amaru/src/stages/mod.rs
📚 Learning: 2025-08-08T14:43:20.218Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 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-kernel/src/is_header/tip.rs
📚 Learning: 2025-08-08T14:35:35.562Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/transaction_pointer.rs:36-44
Timestamp: 2025-08-08T14:35:35.562Z
Learning: In the amaru project, when decoding CBOR arrays, prefer using minicbor_extra::heterogenous_array with the expected length to validate definite-length arrays and correctly handle indefinite-length arrays. Example: crates/amaru-kernel/src/transaction_pointer.rs Decode should use heterogenous_array(d, 2, …) instead of ignoring the length from d.array().
Applied to files:
crates/amaru-kernel/src/is_header/tip.rs
📚 Learning: 2025-06-03T06:31:57.736Z
Learnt from: stevana
Repo: pragma-org/amaru PR: 236
File: simulation/amaru-sim/src/simulator/generate.rs:141-145
Timestamp: 2025-06-03T06:31:57.736Z
Learning: In the amaru project, the team prefers to use as_bytes() instead of hex::decode() for converting hash and header strings to bytes in simulation/amaru-sim/src/simulator/generate.rs, even though they appear to be hex-encoded strings.
Applied to files:
crates/amaru-kernel/src/is_header/tip.rs
📚 Learning: 2025-09-29T16:39:24.001Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/mux.rs:317-325
Timestamp: 2025-09-29T16:39:24.001Z
Learning: In crates/amaru-network/src/mux.rs, the outgoing() method intentionally uses unwrap() after get_mut(&proto_id) as a fail-fast mechanism. This panic is designed to catch programming errors where an actor tries to send on an unregistered protocol, and should not be changed to return a Result since it represents internal code bugs that should terminate the process, not external input that should be handled gracefully.
Applied to files:
crates/amaru-network/src/keepalive/mod.rscrates/amaru-network/src/tx_submission/stage.rscrates/amaru-network/src/handshake/tests.rscrates/amaru-network/src/tx_submission/outcome.rs
📚 Learning: 2025-09-29T16:44:14.807Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 471
File: crates/amaru-network/src/protocol.rs:94-106
Timestamp: 2025-09-29T16:44:14.807Z
Learning: In the amaru-network crate protocol.rs file, the correct Cardano mini-protocol ID assignments are: PROTO_N2N_KEEP_ALIVE = 8 and PROTO_N2N_PEER_SHARE = 10, as verified against the network specification by the maintainer.
Applied to files:
crates/amaru-network/src/keepalive/mod.rscrates/amaru/src/stages/mod.rscrates/amaru-network/src/handshake/tests.rs
📚 Learning: 2025-09-29T20:08:29.906Z
Learnt from: yHSJ
Repo: pragma-org/amaru PR: 453
File: crates/amaru-metrics/src/ledger.rs:60-104
Timestamp: 2025-09-29T20:08:29.906Z
Learning: In amaru-ledger/src/state.rs, the txs_processed field represents the number of transactions in the current block (block.transaction_bodies.len() as u64), not a cumulative total. Therefore, using a Counter with .add() in the metrics is correct for accumulating per-block transaction counts.
Applied to files:
crates/amaru-network/src/tx_submission/tests/faulty_tx_validator.rscrates/amaru-consensus/src/consensus/effects/consensus_effects.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 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:
simulation/amaru-sim/src/simulator/run.rs
📚 Learning: 2025-08-18T08:11:20.028Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 374
File: crates/amaru-stores/src/in_memory/mod.rs:109-118
Timestamp: 2025-08-18T08:11:20.028Z
Learning: The proposals_roots() method in the MemoryStore in crates/amaru-stores/src/in_memory/mod.rs is intentionally left returning all None values rather than reading from stored state, as it's planned for the upcoming major MemoryStore revamp.
Applied to files:
crates/amaru-mempool/src/strategies/mod.rs
📚 Learning: 2025-04-20T17:56:39.223Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 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:
crates/amaru-network/src/handshake/tests.rs
📚 Learning: 2025-08-08T14:34:06.105Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 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:
crates/amaru-mempool/Cargo.toml
🧬 Code graph analysis (21)
crates/amaru-kernel/src/is_header/mod.rs (2)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (1)
tip(99-104)crates/amaru-ledger/src/state.rs (1)
tip(242-254)
crates/amaru-ouroboros-traits/src/lib.rs (1)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (4)
mempool(33-33)mempool(58-60)mempool(84-86)mempool(149-151)
crates/amaru-network/src/tx_submission/tests/system_test.rs (1)
crates/amaru-network/src/socket.rs (1)
create_connection(171-184)
crates/amaru-network/src/lib.rs (6)
crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
point(115-120)crates/amaru-kernel/src/is_header/mod.rs (1)
point(41-43)crates/amaru-kernel/src/is_header/tip.rs (2)
point(102-107)point(170-172)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
point(86-88)crates/amaru-kernel/src/consensus_events.rs (1)
point(128-133)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (1)
tx_submission(349-358)
crates/amaru-network/src/tx_submission/tests/assertions.rs (2)
crates/amaru-network/src/tx_submission/responder_state.rs (2)
txs(130-130)tx_ids(131-135)crates/amaru-ouroboros-traits/src/mempool.rs (1)
from(186-188)
crates/amaru-network/src/tx_submission/responder_params.rs (2)
crates/amaru-network/src/tx_submission/messages.rs (3)
decode(96-124)encode(48-92)d(116-116)crates/amaru-network/src/tx_submission/tests/nodes_options.rs (1)
default(31-40)
crates/amaru-network/src/tx_submission/messages.rs (2)
crates/amaru-network/src/tx_submission/responder_params.rs (3)
decode(69-75)encode(55-65)from(79-84)crates/amaru-network/src/tx_submission/tests/test_data.rs (1)
create_transaction(58-101)
crates/amaru-kernel/src/is_header/tip.rs (1)
simulation/amaru-sim/src/sync/chain_sync_message.rs (1)
cbor(56-56)
crates/amaru-network/src/tx_submission/tests/faulty_tx_validator.rs (2)
crates/amaru-ouroboros-traits/src/can_validate_transactions/mock.rs (1)
validate_transaction(22-24)crates/amaru-ouroboros-traits/src/can_validate_transactions/mod.rs (1)
validate_transaction(22-22)
crates/amaru-network/src/tx_submission/tests/test_cases.rs (5)
crates/amaru-network/src/tx_submission/tests/test_data.rs (5)
create_transactions(39-45)reply_tx_ids(23-25)reply_txs(27-29)request_tx_ids(31-33)request_txs(35-37)crates/amaru-network/src/tx_submission/responder_state.rs (2)
request_tx_ids(156-188)txs(130-130)crates/amaru-network/src/tx_submission/initiator_state.rs (1)
request_txs(131-152)crates/amaru-network/src/tx_submission/tests/assertions.rs (1)
assert_outcomes_eq(57-73)crates/amaru-network/src/tx_submission/tests/nodes_options.rs (1)
default(31-40)
crates/amaru-network/src/tx_submission/stage.rs (4)
crates/amaru-network/src/mempool_effects.rs (1)
new(41-43)crates/amaru-network/src/tx_submission/messages.rs (2)
decode(96-124)message_type(35-44)crates/amaru-network/src/tx_submission/responder_params.rs (1)
decode(69-75)crates/amaru-network/src/protocol.rs (1)
role(167-175)
crates/amaru-mempool/src/strategies/mod.rs (1)
crates/pure-stage/src/stagegraph.rs (1)
dummy(85-93)
crates/amaru-network/src/connection.rs (5)
crates/amaru-network/src/keepalive/mod.rs (2)
register_keepalive(121-146)new(41-43)crates/amaru-network/src/tx_submission/stage.rs (3)
register_tx_submission(158-212)eff(147-147)eff(151-151)crates/amaru-network/src/handshake/mod.rs (2)
new(48-60)stage(63-114)crates/amaru-network/src/mux.rs (4)
new(144-155)new(373-375)new(532-543)stage(200-219)crates/amaru-network/src/handshake/messages.rs (1)
v11_and_above(39-85)
crates/amaru-network/src/handshake/tests.rs (6)
crates/amaru-network/src/socket.rs (1)
create_connection(171-184)crates/amaru-network/src/handshake/messages.rs (1)
v11_and_above(39-85)crates/amaru-network/src/connection.rs (1)
new(37-46)crates/amaru-network/src/keepalive/messages.rs (1)
new(27-29)crates/amaru-network/src/handshake/mod.rs (2)
new(48-60)handler_transform(191-196)crates/amaru-network/src/bytes.rs (1)
new(38-44)
crates/amaru-network/src/mempool_effects.rs (2)
crates/amaru-ouroboros-traits/src/mempool.rs (7)
insert(51-51)get_tx(69-69)tx_ids_since(77-77)wait_for_at_least(87-90)get_txs_for_ids(93-93)last_seq_no(96-96)from(186-188)crates/pure-stage/src/effect.rs (2)
wrap_sync(409-416)wrap(396-406)
crates/amaru-network/src/tx_submission/tests/sized_mempool.rs (3)
crates/amaru-mempool/src/strategies/in_memory_mempool.rs (15)
new(49-55)default(43-45)default(81-88)insert(94-126)insert(199-206)get_tx(129-131)get_tx(208-210)tx_ids_since(134-151)tx_ids_since(212-214)wait_for_at_least(216-240)get_txs_for_ids(154-166)get_txs_for_ids(242-244)last_seq_no(246-248)take(252-257)acknowledge(259-286)crates/amaru-network/src/tx_submission/tests/nodes_options.rs (1)
default(31-40)crates/amaru-ouroboros-traits/src/mempool.rs (8)
insert(51-51)get_tx(69-69)tx_ids_since(77-77)wait_for_at_least(87-90)get_txs_for_ids(93-93)last_seq_no(96-96)take(34-34)acknowledge(39-42)
crates/amaru-ouroboros-traits/src/mempool.rs (2)
crates/amaru-mempool/src/strategies/dummy.rs (5)
take(82-84)acknowledge(86-96)keys(168-170)insert(49-55)get_tx(57-59)crates/amaru-network/src/tx_submission/tests/sized_mempool.rs (4)
take(102-104)acknowledge(106-112)insert(67-69)get_tx(71-73)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
crates/pure-stage/src/serde.rs (1)
from_cbor(654-656)
crates/amaru-mempool/src/strategies/dummy.rs (3)
crates/amaru-ouroboros-traits/src/can_validate_transactions/mock.rs (1)
validate_transaction(22-24)crates/amaru-ouroboros-traits/src/can_validate_transactions/mod.rs (2)
validate_transaction(22-22)from(49-51)crates/amaru-ouroboros-traits/src/mempool.rs (5)
insert(51-51)from(186-188)get_tx(69-69)wait_for_at_least(87-90)get_txs_for_ids(93-93)
crates/amaru-network/src/tx_submission/tests/nodes_options.rs (3)
crates/amaru-mempool/src/strategies/in_memory_mempool.rs (3)
default(43-45)default(81-88)new(49-55)crates/amaru-network/src/tx_submission/responder_params.rs (1)
default(30-35)crates/amaru-network/src/tx_submission/tests/sized_mempool.rs (2)
with_tx_validator(49-57)with_capacity(45-47)
crates/amaru-mempool/src/strategies/in_memory_mempool.rs (4)
crates/amaru-kernel/src/is_header/mod.rs (2)
new(149-151)from(188-193)crates/amaru-network/src/tx_submission/tests/sized_mempool.rs (9)
validate_transaction(61-63)insert(67-69)get_tx(71-73)tx_ids_since(75-77)get_txs_for_ids(92-94)wait_for_at_least(79-90)last_seq_no(96-98)take(102-104)acknowledge(106-112)crates/amaru-ouroboros-traits/src/can_validate_transactions/mod.rs (2)
validate_transaction(22-22)from(49-51)crates/amaru-ouroboros-traits/src/mempool.rs (11)
insert(51-51)from(186-188)get_tx(69-69)tx_ids_since(77-77)get_txs_for_ids(93-93)wait_for_at_least(87-90)last_seq_no(96-96)take(34-34)acknowledge(39-42)add(63-66)add(110-112)
⏰ 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). (1)
- GitHub Check: Analyze (rust)
| use amaru_ouroboros_traits::{ | ||
| CanValidateTransactions, Mempool, MempoolSeqNo, TransactionValidationError, TxId, TxOrigin, | ||
| TxRejectReason, TxSubmissionMempool, | ||
| }; | ||
| use minicbor::Encode; | ||
| use parking_lot::RwLock; | ||
| use std::pin::Pin; | ||
| use std::{collections::BTreeSet, mem}; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's see the full file to understand its structure
wc -l crates/amaru-mempool/src/strategies/dummy.rsRepository: pragma-org/amaru
Length of output: 108
🏁 Script executed:
# Check around line 68 for Future usage
sed -n '60,75p' crates/amaru-mempool/src/strategies/dummy.rsRepository: pragma-org/amaru
Length of output: 439
🏁 Script executed:
# Check all Future imports in the file
rg -n "use.*Future|impl.*Future" crates/amaru-mempool/src/strategies/dummy.rsRepository: pragma-org/amaru
Length of output: 42
🏁 Script executed:
# Also check the imports section more broadly
head -30 crates/amaru-mempool/src/strategies/dummy.rsRepository: pragma-org/amaru
Length of output: 1071
Missing Future trait import will cause a compile error.
Line 68 uses Future<Output = bool> in the return type of wait_for_at_least, but there's no import for it. Classic oversight, happens to the best of us! Without std::future::Future, this won't compile.
use parking_lot::RwLock;
use std::pin::Pin;
+use std::future::Future;
use std::{collections::BTreeSet, mem};📝 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 amaru_ouroboros_traits::{ | |
| CanValidateTransactions, Mempool, MempoolSeqNo, TransactionValidationError, TxId, TxOrigin, | |
| TxRejectReason, TxSubmissionMempool, | |
| }; | |
| use minicbor::Encode; | |
| use parking_lot::RwLock; | |
| use std::pin::Pin; | |
| use std::{collections::BTreeSet, mem}; | |
| use amaru_ouroboros_traits::{ | |
| CanValidateTransactions, Mempool, MempoolSeqNo, TransactionValidationError, TxId, TxOrigin, | |
| TxRejectReason, TxSubmissionMempool, | |
| }; | |
| use minicbor::Encode; | |
| use parking_lot::RwLock; | |
| use std::pin::Pin; | |
| use std::future::Future; | |
| use std::{collections::BTreeSet, mem}; |
🤖 Prompt for AI Agents
In crates/amaru-mempool/src/strategies/dummy.rs around lines 15 to 22, the file
uses Future<Output = bool> in wait_for_at_least's return type but never imports
std::future::Future; add an import for std::future::Future to the top use/import
block so the Future trait is in scope and the signature compiles.
| impl<Tx: Encode<()> + Send + Sync + 'static> TxSubmissionMempool<Tx> for DummyMempool<Tx> { | ||
| fn insert(&self, tx: Tx, _tx_origin: TxOrigin) -> Result<(TxId, MempoolSeqNo), TxRejectReason> { | ||
| let tx_id = TxId::from(&tx); | ||
| let mut inner = self.inner.write(); | ||
| inner.transactions.push(tx); | ||
| let seq_no = MempoolSeqNo(inner.transactions.len() as u64 - 1); | ||
| Ok((tx_id, seq_no)) | ||
| } |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Search for InMemoryMempool and next_seq initialization
echo "=== Searching for InMemoryMempool ==="
rg -n "next_seq\|struct InMemoryMempool\|impl.*InMemoryMempool" crates/amaru-mempool/src/strategies/ -A 3 -B 1Repository: pragma-org/amaru
Length of output: 97
🏁 Script executed:
#!/bin/bash
# First, let's see what files exist in the strategies directory
echo "=== Files in strategies directory ==="
find crates/amaru-mempool/src/strategies -type f -name "*.rs" 2>/dev/null | head -20
echo ""
echo "=== Searching for seq_no references ==="
rg -n "seq_no\|MempoolSeqNo\|next_seq" crates/amaru-mempool/src/ 2>/dev/null | head -30
echo ""
echo "=== Searching for InMemory ==="
rg -n "InMemory" crates/amaru-mempool/src/ 2>/dev/null | head -20Repository: pragma-org/amaru
Length of output: 1231
🏁 Script executed:
#!/bin/bash
# Read the DummyMempool insert method (lines 48-55 as mentioned in review)
echo "=== DummyMempool insert implementation ==="
sed -n '48,60p' crates/amaru-mempool/src/strategies/dummy.rs
echo ""
echo "=== InMemoryMempool implementation (first 100 lines) ==="
head -100 crates/amaru-mempool/src/strategies/in_memory_mempool.rs
echo ""
echo "=== Search for insert method in InMemoryMempool ==="
rg -n "fn insert" crates/amaru-mempool/src/strategies/in_memory_mempool.rs -A 10Repository: pragma-org/amaru
Length of output: 5315
🏁 Script executed:
#!/bin/bash
# Read more of the insert method implementation
echo "=== InMemoryMempool insert method (lines 94-140) ==="
sed -n '94,140p' crates/amaru-mempool/src/strategies/in_memory_mempool.rs
echo ""
echo "=== Look for MempoolSeqNo assignment ==="
rg -n "MempoolSeqNo\|next_seq" crates/amaru-mempool/src/strategies/in_memory_mempool.rs -B 2 -A 2Repository: pragma-org/amaru
Length of output: 1677
Align seq_no calculation with InMemoryMempool semantics.
DummyMempool calculates seq_no = len - 1 (0-indexed), giving the first transaction seq_no 0. However, InMemoryMempool initializes next_seq: 1 and assigns it before incrementing, giving the first transaction seq_no 1. If these implementations are meant to be interchangeable, they should use consistent indexing. Either update DummyMempool to start at 1, or document why the 0-indexed approach is intentional.
🤖 Prompt for AI Agents
In crates/amaru-mempool/src/strategies/dummy.rs around lines 48 to 55,
DummyMempool currently computes seq_no as len - 1 (0-indexed) which conflicts
with InMemoryMempool that starts seq numbers at 1; change the seq_no calculation
to use the post-push length directly (e.g. MempoolSeqNo(inner.transactions.len()
as u64)) so the first inserted transaction gets seq_no 1, and update any related
tests/comments to reflect the 1-based sequencing.
| impl<Tx: Send + Sync + 'static + Encode<()> + Clone> Mempool<Tx> for InMemoryMempool<Tx> { | ||
| fn take(&self) -> Vec<Tx> { | ||
| let mut inner = self.inner.write(); | ||
| let entries = mem::take(&mut inner.entries_by_id); | ||
| let _ = mem::take(&mut inner.entries_by_seq); | ||
| entries.into_values().map(|entry| entry.tx).collect() | ||
| } |
There was a problem hiding this comment.
take() returns transactions in TxId order, not insertion order.
Since entries_by_id is keyed by TxId (a hash), into_values() iterates in hash order, not sequence order. If consumers expect transactions in insertion order (which seems likely for block building), this could cause subtle issues – like shuffling a playlist when you wanted to keep your carefully curated order!
You might want to sort by seq_no before returning, or iterate entries_by_seq instead.
🔎 Proposed fix to preserve insertion order
fn take(&self) -> Vec<Tx> {
let mut inner = self.inner.write();
- let entries = mem::take(&mut inner.entries_by_id);
- let _ = mem::take(&mut inner.entries_by_seq);
- entries.into_values().map(|entry| entry.tx).collect()
+ let entries_by_seq = mem::take(&mut inner.entries_by_seq);
+ let mut entries_by_id = mem::take(&mut inner.entries_by_id);
+ entries_by_seq
+ .into_values()
+ .filter_map(|tx_id| entries_by_id.remove(&tx_id).map(|e| e.tx))
+ .collect()
}🤖 Prompt for AI Agents
In crates/amaru-mempool/src/strategies/in_memory_mempool.rs around lines 251 to
257, take() currently drains entries_by_id and returns values which yields
hash-key order; change it to preserve insertion/sequence order by
draining/iterating entries_by_seq (or by sorting the drained entries by their
seq_no) and return txs in that sequence order; ensure you still remove both maps
from inner and hold the write lock while taking to avoid races.
| let keepalive = eff | ||
| .wire_up(keepalive, KeepAlive::new(muxer.clone(), Cookie::new())) | ||
| .await; | ||
| eff.send( | ||
| &muxer, | ||
| MuxMessage::Register { | ||
| protocol: PROTO_N2N_KEEP_ALIVE.erase(), | ||
| frame: mux::Frame::OneCborItem, | ||
| handler: keepalive.clone(), | ||
| max_buffer: 65535, | ||
| }, | ||
| ) | ||
| .await; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Examine the keepalive module and protocol definitions
echo "=== Checking keepalive/mod.rs ==="
wc -l crates/amaru-network/src/keepalive/mod.rs
echo -e "\n=== Lines 100-150 of keepalive/mod.rs ==="
sed -n '100,150p' crates/amaru-network/src/keepalive/mod.rs
echo -e "\n=== Checking protocol.rs for protocol ID definitions ==="
grep -n "PROTO_N2N_KEEP_ALIVE\|responder\|erase" crates/amaru-network/src/protocol.rs | head -30Repository: pragma-org/amaru
Length of output: 2081
🏁 Script executed:
#!/bin/bash
# Look for how erase() and responder() work with protocol IDs
echo "=== Searching for erase() and responder() method implementations ==="
rg -n "\.erase\(\)|\.responder\(\)" crates/amaru-network/src/ -A2 -B2 --type rust | head -50
echo -e "\n=== Checking protocol.rs structure ==="
head -100 crates/amaru-network/src/protocol.rsRepository: pragma-org/amaru
Length of output: 5791
🏁 Script executed:
#!/bin/bash
# Get the full protocol.rs file to see responder() and opposite() implementations
echo "=== Full protocol.rs content ==="
cat crates/amaru-network/src/protocol.rs | head -250Repository: pragma-org/amaru
Length of output: 5497
Protocol ID mismatch between registration and responder handler.
The registration at line 137 uses PROTO_N2N_KEEP_ALIVE.erase() (ID 8), but the responder handler at lines 113 and 115 uses PROTO_N2N_KEEP_ALIVE.responder().erase() (ID 32776, since the responder() method sets the 0x8000 bit via self.0 | RESPONDER). The mux's outgoing() method will panic when the responder tries to send or want-next because it's looking for a handler registered under ID 32776, which doesn't exist — only ID 8 is registered.
Per the Cardano specification, the mode bit distinguishes dual instances of a mini-protocol, with mode set to 0 for initiator segments and 1 for responder segments. This mode bit encoding should happen at the wire/multiplexer layer, not at the protocol registration layer. Both initiator and responder should register with the base protocol ID (8), and let the mux handle the mode bit when actually transmitting.
Either register the responder with PROTO_N2N_KEEP_ALIVE.responder().erase() to match its usage, or standardize on the base ID for registration and handle role differentiation in the mux layer.
🤖 Prompt for AI Agents
crates/amaru-network/src/keepalive/mod.rs around lines 131-143: the code
registers the protocol under PROTO_N2N_KEEP_ALIVE.erase() (base ID 8) but the
responder handler uses PROTO_N2N_KEEP_ALIVE.responder().erase() (responder ID
with RESPONDER bit set), causing a protocol ID mismatch and mux panics; fix by
registering the handler with the responder ID instead (use
PROTO_N2N_KEEP_ALIVE.responder().erase() in the eff.send Register call) so the
registered protocol ID matches the responder's ID, or alternatively ensure both
registration and handler use the base ID and handle the mode bit in the mux
layer consistently.
| use amaru_kernel::Tx; | ||
| use amaru_ouroboros_traits::{ | ||
| CanValidateTransactions, MempoolSeqNo, TransactionValidationError, TxId, TxOrigin, | ||
| TxRejectReason, TxSubmissionMempool, | ||
| }; | ||
| use pure_stage::{ | ||
| BoxFuture, Effects, ExternalEffect, ExternalEffectAPI, ExternalEffectSync, Resources, SendData, | ||
| }; | ||
| use serde::{Deserialize, Serialize}; | ||
| use std::fmt::Debug; | ||
| use std::pin::Pin; | ||
| use std::sync::Arc; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Check Future imports in the file
echo "=== Searching for Future imports ==="
rg -n "use.*Future" crates/amaru-network/src/mempool_effects.rs
echo -e "\n=== Checking line 87 and surrounding context ==="
sed -n '80,95p' crates/amaru-network/src/mempool_effects.rsRepository: pragma-org/amaru
Length of output: 685
Missing Future trait import—line 87 needs it.
The wait_for_at_least method uses Pin<Box<dyn Future<Output = bool> + Send + '_>> on line 87, but there's no explicit Future import. Add it straight after the other std imports to keep things tidy.
use std::fmt::Debug;
+use std::future::Future;
use std::pin::Pin;📝 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 amaru_kernel::Tx; | |
| use amaru_ouroboros_traits::{ | |
| CanValidateTransactions, MempoolSeqNo, TransactionValidationError, TxId, TxOrigin, | |
| TxRejectReason, TxSubmissionMempool, | |
| }; | |
| use pure_stage::{ | |
| BoxFuture, Effects, ExternalEffect, ExternalEffectAPI, ExternalEffectSync, Resources, SendData, | |
| }; | |
| use serde::{Deserialize, Serialize}; | |
| use std::fmt::Debug; | |
| use std::pin::Pin; | |
| use std::sync::Arc; | |
| use amaru_kernel::Tx; | |
| use amaru_ouroboros_traits::{ | |
| CanValidateTransactions, MempoolSeqNo, TransactionValidationError, TxId, TxOrigin, | |
| TxRejectReason, TxSubmissionMempool, | |
| }; | |
| use pure_stage::{ | |
| BoxFuture, Effects, ExternalEffect, ExternalEffectAPI, ExternalEffectSync, Resources, SendData, | |
| }; | |
| use serde::{Deserialize, Serialize}; | |
| use std::fmt::Debug; | |
| use std::future::Future; | |
| use std::pin::Pin; | |
| use std::sync::Arc; |
🤖 Prompt for AI Agents
In crates/amaru-network/src/mempool_effects.rs around lines 15–26 (and
referenced on line 87), the Future trait used in the Pin<Box<dyn Future<Output =
bool> + Send + '_>> type is not imported; add an import for std::future::Future
alongside the other std imports (place it with the existing std::fmt/Pin/Arc
imports) so the Future type name resolves.
| /// This stage handles the tx submission protocol for the initiator role. | ||
| pub async fn responder_stage( |
There was a problem hiding this comment.
Docstring says "initiator" but this is the responder stage.
Oi, we've got a bit of a Memento situation here – the comment's remembering the wrong function! The docstring says "initiator role" but this is clearly responder_stage.
🔎 Proposed fix
-/// This stage handles the tx submission protocol for the initiator role.
+/// This stage handles the tx submission protocol for the responder role.
pub async fn responder_stage(🤖 Prompt for AI Agents
In crates/amaru-network/src/tx_submission/stage.rs around lines 84 to 85, the
function docstring incorrectly says "initiator role" for the responder_stage
function; update the comment to accurately describe that this stage handles the
tx submission protocol for the responder role (or remove/replace the misleading
phrase), keeping wording consistent with surrounding docs and naming.
| use amaru_mempool::strategies::InMemoryMempool; | ||
| use amaru_mempool::{DefaultCanValidateTransactions, MempoolConfig}; | ||
| use amaru_ouroboros_traits::{ | ||
| CanValidateTransactions, Mempool, MempoolSeqNo, TransactionValidationError, TxId, TxOrigin, | ||
| TxRejectReason, TxSubmissionMempool, | ||
| }; | ||
| use pallas_primitives::conway::Tx; | ||
| use std::pin::Pin; | ||
| use std::sync::Arc; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Read the sized_mempool.rs file to check line 82 and all imports
cat -n crates/amaru-network/src/tx_submission/tests/sized_mempool.rs | head -100Repository: pragma-org/amaru
Length of output: 4012
🏁 Script executed:
#!/bin/bash
# First, let's find and read the file
if [ -f "crates/amaru-network/src/tx_submission/tests/sized_mempool.rs" ]; then
echo "=== File contents (first 120 lines) ==="
head -120 crates/amaru-network/src/tx_submission/tests/sized_mempool.rs | cat -n
else
echo "File not found at expected path"
# Try to find it
find . -name "sized_mempool.rs" -type f 2>/dev/null
fiRepository: pragma-org/amaru
Length of output: 4455
🏁 Script executed:
#!/bin/bash
# Check the file and search for Future usage
if [ -f "crates/amaru-network/src/tx_submission/tests/sized_mempool.rs" ]; then
head -120 crates/amaru-network/src/tx_submission/tests/sized_mempool.rs | cat -n
echo ""
echo "=== Searching for Future usage ==="
grep -n "Future" crates/amaru-network/src/tx_submission/tests/sized_mempool.rs
else
echo "File not found"
find . -name "sized_mempool.rs" 2>/dev/null | head -5
fiRepository: pragma-org/amaru
Length of output: 4513
🏁 Script executed:
#!/bin/bash
# Find the ouroboros_traits crate and check what's exported
find . -path "*/amaru-ouroboros-traits/src/*.rs" -o -path "*/ouroboros-traits/src/*.rs" | head -5
# Also check the crate name in Cargo.toml if it exists
if [ -f "Cargo.toml" ]; then
grep -A 5 "amaru.*ouroboros" Cargo.toml 2>/dev/null | head -10
fi
# Find the actual trait definition
find . -name "*.rs" -path "*ouroboros*" -type f 2>/dev/null | head -10Repository: pragma-org/amaru
Length of output: 1418
🏁 Script executed:
#!/bin/bash
# Find the TxSubmissionMempool trait definition
grep -r "trait TxSubmissionMempool" crates/amaru-ouroboros-traits/src/ --include="*.rs"
# Also check the lib.rs file to see what's exported
find crates/amaru-ouroboros-traits/src -name "lib.rs" -exec cat {} \;Repository: pragma-org/amaru
Length of output: 1259
🏁 Script executed:
#!/bin/bash
# Read the mempool.rs file to see the TxSubmissionMempool trait definition and check for Future imports/exports
cat crates/amaru-ouroboros-traits/src/mempool.rsRepository: pragma-org/amaru
Length of output: 6540
🏁 Script executed:
#!/bin/bash
# Check if mempool.rs compiles by looking at the full imports section more carefully
head -50 crates/amaru-ouroboros-traits/src/mempool.rs | grep -n "use"
# Also check if maybe Future is re-exported from somewhere or in a prelude
grep -r "pub use.*Future" crates/amaru-ouroboros-traits/src/ --include="*.rs"
# Let's also verify the exact line that uses Future in the trait
grep -n "Future" crates/amaru-ouroboros-traits/src/mempool.rsRepository: pragma-org/amaru
Length of output: 572
🏁 Script executed:
#!/bin/bash
# Check the full mempool.rs imports - maybe there's a prelude or wildcard import
head -30 crates/amaru-ouroboros-traits/src/mempool.rs
# Check if there's a prelude in the crate
grep -r "prelude" crates/amaru-ouroboros-traits/src/ --include="*.rs"
# Let's also check if the crate actually compiles by looking for any CI/build configs
find . -name "*.yml" -o -name "*.yaml" | xargs grep -l "test\|build" 2>/dev/null | head -3Repository: pragma-org/amaru
Length of output: 1465
🏁 Script executed:
#!/bin/bash
# Check if the trait definition in mempool.rs actually compiles or if it also has the same issue
# by checking if there's a use statement for Future anywhere in the file
grep -n "use.*Future\|use std::future" crates/amaru-ouroboros-traits/src/mempool.rs
# Also let's verify the sized_mempool file again and check if it's using Future from somewhere else
grep -n "Future\|use std::future" crates/amaru-network/src/tx_submission/tests/sized_mempool.rsRepository: pragma-org/amaru
Length of output: 121
Add the missing Future trait import.
Line 82 uses Future in the return type but there's no import for it. This'll cause a compile error straight up, mate.
use std::pin::Pin;
+use std::future::Future;
use std::sync::Arc;📝 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 amaru_mempool::strategies::InMemoryMempool; | |
| use amaru_mempool::{DefaultCanValidateTransactions, MempoolConfig}; | |
| use amaru_ouroboros_traits::{ | |
| CanValidateTransactions, Mempool, MempoolSeqNo, TransactionValidationError, TxId, TxOrigin, | |
| TxRejectReason, TxSubmissionMempool, | |
| }; | |
| use pallas_primitives::conway::Tx; | |
| use std::pin::Pin; | |
| use std::sync::Arc; | |
| use amaru_mempool::strategies::InMemoryMempool; | |
| use amaru_mempool::{DefaultCanValidateTransactions, MempoolConfig}; | |
| use amaru_ouroboros_traits::{ | |
| CanValidateTransactions, Mempool, MempoolSeqNo, TransactionValidationError, TxId, TxOrigin, | |
| TxRejectReason, TxSubmissionMempool, | |
| }; | |
| use pallas_primitives::conway::Tx; | |
| use std::pin::Pin; | |
| use std::future::Future; | |
| use std::sync::Arc; |
🤖 Prompt for AI Agents
In crates/amaru-network/src/tx_submission/tests/sized_mempool.rs around lines 15
to 23, the Future trait is used later (line ~82) but not imported; add an import
for std::future::Future at the top of the file with the other use statements so
the return type compiles properly.
| use crate::CanValidateTransactions; | ||
| use amaru_kernel::cbor::{Decode, Decoder, Encode, Encoder, decode, encode}; | ||
| use amaru_kernel::{Hash, Hasher}; | ||
| use serde::{Deserialize, Serialize}; | ||
| use std::fmt; | ||
| use std::fmt::{Display, Formatter}; | ||
| use std::pin::Pin; |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, let's look at the full imports and the area around line 90
head -n 100 crates/amaru-ouroboros-traits/src/mempool.rs | cat -nRepository: pragma-org/amaru
Length of output: 5122
🏁 Script executed:
# Also check if Future appears in the file
rg -n "Future" crates/amaru-ouroboros-traits/src/mempool.rsRepository: pragma-org/amaru
Length of output: 120
🏁 Script executed:
# Run cargo check to see if there's actually a compilation error
cargo check -p amaru-ouroboros-traits 2>&1 | grep -A 5 "error\|Future"Repository: pragma-org/amaru
Length of output: 169
🏁 Script executed:
# Get lines 1-30 to see all imports at top of file
sed -n '1,30p' crates/amaru-ouroboros-traits/src/mempool.rs | cat -nRepository: pragma-org/amaru
Length of output: 1559
🏁 Script executed:
# Check if there's a prelude import or any wildcard imports
rg -n "use.*\*|prelude" crates/amaru-ouroboros-traits/src/mempool.rsRepository: pragma-org/amaru
Length of output: 42
🏁 Script executed:
# Check if Future might be imported via a parent module's prelude
cat crates/amaru-ouroboros-traits/src/lib.rs | grep -n "prelude\|pub use"Repository: pragma-org/amaru
Length of output: 415
Add missing Future import on line 90.
Like a director realizing halfway through production that the lead actor never showed up—line 90 uses Future in the return type without importing it. Pin and Box are there, but Future got left on the cutting room floor.
use std::fmt;
use std::fmt::{Display, Formatter};
+use std::future::Future;
use std::pin::Pin;📝 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::CanValidateTransactions; | |
| use amaru_kernel::cbor::{Decode, Decoder, Encode, Encoder, decode, encode}; | |
| use amaru_kernel::{Hash, Hasher}; | |
| use serde::{Deserialize, Serialize}; | |
| use std::fmt; | |
| use std::fmt::{Display, Formatter}; | |
| use std::pin::Pin; | |
| use crate::CanValidateTransactions; | |
| use amaru_kernel::cbor::{Decode, Decoder, Encode, Encoder, decode, encode}; | |
| use amaru_kernel::{Hash, Hasher}; | |
| use serde::{Deserialize, Serialize}; | |
| use std::fmt; | |
| use std::fmt::{Display, Formatter}; | |
| use std::future::Future; | |
| use std::pin::Pin; |
🤖 Prompt for AI Agents
In crates/amaru-ouroboros-traits/src/mempool.rs around line 90, the return type
uses Future but the type isn't imported; add the missing import by adding use
std::future::Future; near the other std imports (e.g., with Pin and Box) so the
Future symbol is in scope and the function signature compiles.
| /// Origin of a transaction being inserted into the mempool: | ||
| /// - Local: created locally | ||
| /// - Remote(Peer): received from a remote peer | ||
| #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] | ||
| pub enum TxOrigin { | ||
| Local, | ||
| Remote, | ||
| } |
There was a problem hiding this comment.
TxOrigin::Remote docs mention peer info but variant doesn't have it.
The doc comment says "TxOrigin::Remote(Peer): received from a remote peer" but the actual enum is just Remote without the peer parameter. It's like the game manual describing a feature that got cut!
Either update the comment or add the peer field if needed:
🔎 Proposed doc fix
/// Origin of a transaction being inserted into the mempool:
-/// - Local: created locally
-/// - Remote(Peer): received from a remote peer
+/// - Local: created locally on this node
+/// - Remote: received from a remote peer
#[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)]📝 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.
| /// Origin of a transaction being inserted into the mempool: | |
| /// - Local: created locally | |
| /// - Remote(Peer): received from a remote peer | |
| #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] | |
| pub enum TxOrigin { | |
| Local, | |
| Remote, | |
| } | |
| /// Origin of a transaction being inserted into the mempool: | |
| /// - Local: created locally on this node | |
| /// - Remote: received from a remote peer | |
| #[derive(Debug, Clone, PartialEq, Eq, PartialOrd, Ord, Serialize, Deserialize)] | |
| pub enum TxOrigin { | |
| Local, | |
| Remote, | |
| } |
🤖 Prompt for AI Agents
In crates/amaru-ouroboros-traits/src/mempool.rs around lines 137 to 144, the doc
comment for TxOrigin refers to "Remote(Peer)" but the enum defines Remote with
no peer payload; update the docs to match the current enum by removing "(Peer)"
and describing Remote as simply "received from a remote peer", or if you intend
to store the peer, change the Remote variant to carry the peer identifier (e.g.
Remote(PeerId)) and update derives/serde usages accordingly and import the
PeerId type.
Summary by CodeRabbit
New Features
Testing
Chores
✏️ Tip: You can customize this high-level summary in your review settings.