Restructure consensus simulator#250
Conversation
WalkthroughThis update refactors the consensus and simulation crates to introduce asynchronous, effect-based nonce evolution in header validation using the Changes
Sequence Diagram(s)sequenceDiagram
participant Peer
participant Effects
participant ValidateHeader
participant EvolveNonceEffect
participant ChainStore
Peer->>ValidateHeader: Submit RollForward event (header, point, etc.)
ValidateHeader->>Effects: Request EvolveNonceEffect (with header, store, params)
Effects->>EvolveNonceEffect: Invoke execute()
EvolveNonceEffect->>ChainStore: evolve_nonce(header, params)
ChainStore-->>EvolveNonceEffect: Return epoch nonce
EvolveNonceEffect-->>Effects: Return epoch nonce
Effects-->>ValidateHeader: Provide epoch nonce
ValidateHeader->>ValidateHeader: Validate header using nonce
ValidateHeader-->>Peer: Return validated RollForward event
Possibly related PRs
Suggested reviewers
Poem
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
|
The wasm target build fails with: I suppose this is because of pure-stage? |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 4
🔭 Outside diff range comments (3)
simulation/amaru-sim/src/simulator/ledger.rs (1)
227-233: 🛠️ Refactor suggestionHard-coding the stake makes the test brittle, mate
If the JSON fixture shifts again, this magic number will bite us harder than a Dark Souls boss fight. Consider loading
stake_distribution.total_active_stake(or at least parsing the value for the specific pool) instead of pinning the literal.crates/amaru-consensus/src/consensus/mod.rs (1)
45-59:⚠️ Potential issue
PartialEqderive will not compile –tracing::Spanisn’t comparable
tracing::Spandoesn’t implementPartialEq, so the blanket derive will give you a compiler kaboom. You’ll need a manual impl that ignores theSpan, or wrapSpanin a newtype with a dummy compare.-#[derive(Clone, Debug, PartialEq)] +#[derive(Clone, Debug)]—or—
impl PartialEq for DecodedChainSyncEvent { fn eq(&self, other: &Self) -> bool { use DecodedChainSyncEvent::*; match (self, other) { (Rollback { peer: p1, rollback_point: r1, .. }, Rollback { peer: p2, rollback_point: r2, .. }) => p1 == p2 && r1 == r2, (RollForward { peer: p1, point: pt1, header: h1, .. }, RollForward { peer: p2, point: pt2, header: h2, .. }) => p1 == p2 && pt1 == pt2 && h1 == h2, _ => false, } } }crates/amaru-consensus/src/consensus/validate_header.rs (1)
143-263: 🛠️ Refactor suggestionCrikey, we've got ourselves a case of the clone wars here!
You've got
handle_roll_forwardandhandle_roll_forward_newdoing basically the same thing - like having two controllers when you're playing split-screen Mario Kart. The new one uses the fancy Effects system while the old one's still hanging around.If you're migrating to the new approach, consider deprecating the old method or at least add a comment explaining why both exist, yeah?
Consider either:
- Marking
handle_roll_forwardas deprecated and documenting the migration path- Having
handle_roll_forwarddelegate tohandle_roll_forward_newinternally- Removing the old method if it's no longer needed
🧹 Nitpick comments (6)
crates/pure-stage/src/lib.rs (1)
6-6: Public exposure needs docs & stability promiseMaking
stage_refpublic opens the floodgates for downstream crates. Sweet as, but maybe jot a short module-level doc (even a 🍻 emoji will do) and tag it#[doc(hidden)]if it’s still experimental; otherwise breaking changes later will feel like a Dark Souls boss fight for library users.crates/amaru-consensus/src/lib.rs (1)
42-43: Minor perf nit – avoid extra allocation in error path
hex::encode(header)clones the vector into a freshString. Since this only fires on decode failure, it’s not end-of-the-world, but you can save a copy withhex::encode(&header)(slice) or log viahex::encode_upperif you fancy consistency with CLI tools.crates/amaru-consensus/src/consensus/receive_header.rs (1)
32-36: Clone-heavy error construction
raw_header.into()turns the slice into aVec, doubling memory on the unhappy path. Tiny, yeah, but you can reuse the already-ownedraw_headerfrom caller by changing the API to acceptVec<u8>or by slicing inside the error:- header: raw_header.into(), + header: raw_header.to_vec(),No drama if you leave it, just a friendly heads-up before the profiler starts screaming louder than a Xenomorph.
simulation/amaru-sim/src/bin/amaru-sim/main.rs (1)
18-28: Fair dinkum, explicit runtime management - love to see it!Making the runtime creation explicit gives you more control than a Dark Souls boss fight. Though I reckon that
unwrap()on line 26 could use some proper error handling - maybe return aResultfrom main or useexpect()with a meaningful message?- let rt = tokio::runtime::Runtime::new().unwrap(); + let rt = tokio::runtime::Runtime::new() + .expect("Failed to create Tokio runtime - this is game over, mate");simulation/amaru-sim/src/simulator/mod.rs (1)
283-289: Fair dinkum, we've got some unfinished business here!You've got a FIXME about node name storage and an XXX about broadcasting. The hardcoded "n1" is like playing Zelda with Link renamed to "TEST_HERO" - functional but not quite right, innit?
Would you like me to help implement proper node name management and broadcast functionality? Could spin up a proper solution that tracks node identities and handles multi-follower broadcasts.
simulation/amaru-sim/src/simulator/simulate.rs (1)
228-237: Consider documenting those trait bounds, legend!While the trait bounds make sense, future devs might appreciate a quick comment explaining why each one's needed - like leaving notes in Dark Souls for other players.
+/// Simulates a distributed system with generic message types. +/// +/// Type bounds: +/// - Debug: for error reporting and trace output +/// - PartialEq: for message comparison in tests +/// - Clone: for message distribution to multiple nodes pub fn simulate<Msg, F>(
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (18)
crates/amaru-consensus/Cargo.toml(2 hunks)crates/amaru-consensus/src/consensus/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/receive_header.rs(1 hunks)crates/amaru-consensus/src/consensus/store.rs(1 hunks)crates/amaru-consensus/src/consensus/store_header.rs(1 hunks)crates/amaru-consensus/src/consensus/validate_header.rs(4 hunks)crates/amaru-consensus/src/lib.rs(1 hunks)crates/amaru-kernel/src/protocol_parameters.rs(1 hunks)crates/amaru/src/stages/mod.rs(1 hunks)crates/pure-stage/src/lib.rs(1 hunks)simulation/amaru-sim/src/bin/amaru-sim/main.rs(1 hunks)simulation/amaru-sim/src/simulator/bytes.rs(2 hunks)simulation/amaru-sim/src/simulator/generate.rs(15 hunks)simulation/amaru-sim/src/simulator/ledger.rs(1 hunks)simulation/amaru-sim/src/simulator/mod.rs(4 hunks)simulation/amaru-sim/src/simulator/simulate.rs(10 hunks)simulation/amaru-sim/src/simulator/sync.rs(4 hunks)simulation/amaru-sim/tests/data/stake-distribution.json(2 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (3)
simulation/amaru-sim/src/bin/amaru-sim/main.rs (1)
simulation/amaru-sim/src/simulator/mod.rs (1)
run(87-89)
crates/pure-stage/src/lib.rs (1)
crates/pure-stage/src/stage_ref.rs (1)
stage_ref(58-69)
simulation/amaru-sim/src/simulator/bytes.rs (1)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
hex(140-142)
🪛 Gitleaks (8.26.0)
simulation/amaru-sim/tests/data/stake-distribution.json
4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
30-30: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
38-38: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
55-55: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
64-64: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
68-68: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
89-89: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
98-98: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
102-102: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
106-106: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
115-115: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
119-119: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
123-123: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
132-132: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
136-136: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
140-140: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
149-149: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
153-153: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
157-157: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
166-166: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
170-170: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
174-174: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
183-183: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
187-187: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
191-191: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
200-200: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
204-204: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
208-208: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
217-217: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
221-221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
225-225: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
234-234: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
238-238: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
242-242: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
251-251: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
255-255: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Simulation
- GitHub Check: Coverage
- GitHub Check: Snapshots (preprod, 10.1.4)
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Build on ubuntu-latest with target aarch64-unknown-linux-musl
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Sanity
🔇 Additional comments (18)
crates/amaru-consensus/Cargo.toml (1)
16-17: Double-check new deps for the wasm build issueAdding
hexis grand, butpure-stagedrags in full-fat Tokio by default. That’s the same surfboard that wiped out the wasm build earlier (onlyrt,time,macros,io-util,syncare kosher on wasm). Make surepure-stageis pulled with a slim feature set or add adefault-features = falsestanza.Also applies to: 28-28
crates/amaru-kernel/src/protocol_parameters.rs (1)
312-314: Nice one –Debug+PartialEqonGlobalParametersThis’ll make test asserts and logging a breeze. No worries here.
crates/amaru-consensus/src/consensus/store.rs (1)
89-105:PartialEqonNoncesErrormay still fail
TimeHorizonError(fromslot_arithmetic) doesn’t exposePartialEqin current releases, so the derive could choke at compile time just like the Span case above. Either:
- Add
PartialEqupstream toTimeHorizonError, or- Drop the derive here and hand-roll equality (or live without it).
crates/amaru/src/stages/mod.rs (1)
134-142: Mind the thread-safety contract when swappingBoxforArcGrand move converting the stake-distribution view to an
Arc, mate, but remember:Arc<T>doesn’t automatically makeTSend/Sync.ValidateHeaderwill likely hop across tasks; ifview_stake_distribution()hands back a plainRc-backed value or keeps interior mutability without locking, we’re courting a sneaky data race faster than Mad Max in the desert. Double-check that the concrete type behinddyn HasStakeDistributionis thread-safe (or wrap it in aMutex/RwLock) and thatValidateHeader’s bound is indeeddyn HasStakeDistribution + Send + Sync.simulation/amaru-sim/src/simulator/bytes.rs (2)
17-17: Good on ya for adding those comparison traits, mate!The addition of
Eq,PartialOrd, andOrdis spot on - now yourBytescan be sorted and compared properly, just like how we'd rank the best coffee shops in Melbourne.
46-54: Sweet as! The hex conversion is a ripper addition.This
TryFrom<&str>implementation is cleaner than a fresh pair of Vans. The error handling withhex::FromHexErroris proper too - no unwrapping like a Christmas pressie here.simulation/amaru-sim/tests/data/stake-distribution.json (1)
2-275: No worries here, just test data doing its thing!These stake distribution entries are as legitimate as a Tim Tam slam. The static analysis tool's having a laugh thinking these hex strings are API keys - they're just test hashes, nothing suss.
🧰 Tools
🪛 Gitleaks (8.26.0)
4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
30-30: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
38-38: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
55-55: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
64-64: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
68-68: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
72-72: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
81-81: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
85-85: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
89-89: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
98-98: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
102-102: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
106-106: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
115-115: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
119-119: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
123-123: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
132-132: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
136-136: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
140-140: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
149-149: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
153-153: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
157-157: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
166-166: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
170-170: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
174-174: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
183-183: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
187-187: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
191-191: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
200-200: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
204-204: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
208-208: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
217-217: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
221-221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
225-225: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
234-234: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
238-238: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
242-242: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
251-251: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
255-255: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
259-259: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
268-268: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
272-272: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
simulation/amaru-sim/src/simulator/sync.rs (3)
83-87: Bonzer Default implementations, cobber!These Default traits are as standard as ordering a flat white - just delegating to
new(). Clean and simple, like a minimalist cafe in Fitzroy.Also applies to: 130-134
127-127: Making things public like an open mic night - I dig it!Opening up the visibility on these fields and methods gives more flexibility for external usage. Just remember - with great power comes great responsibility (cheers, Uncle Ben).
Also applies to: 137-137, 142-142
200-200: Hex encoding for Debug - now we're talking!Switching from UTF-8 lossy to hex encoding is like upgrading from VHS to Blu-ray. Binary data deserves proper representation, not some garbled text nonsense.
Also applies to: 203-203, 212-212
simulation/amaru-sim/src/simulator/generate.rs (5)
15-19: Crikey, property-based testing! That's ace!The new
generate_inputs_strategyfunction is a beaut addition for proptest. Seeding the RNG withu64gives you deterministic randomness - it's like having infinite test cases without the infinite runtime.Also applies to: 298-305
42-48: Public fields on the Block struct - living dangerously, eh?Making these fields public is like leaving your front door unlocked in a friendly neighborhood. It's probably fine, but consider if you need any invariants maintained. If not, carry on!
70-72: Better error handling than a try-catch in The Matrix!That error message with
file_path.display()is chef's kiss - users will know exactly what went wrong instead of getting a cryptic panic.
50-60: Display implementation showing hex previews - noice!Showing the first 6 bytes in hex is like a movie trailer - gives you just enough to know what you're dealing with without the full spoiler.
93-93: Bytes references everywhere - consistency is key!The wholesale replacement of
Stringrefs withBytesrefs throughout these functions is thorough as. It's like when they remastered all the Star Wars films - everything's consistent now.Also applies to: 120-120, 260-260, 287-287
simulation/amaru-sim/src/simulator/mod.rs (2)
87-91: Nice refactor to the staged pipeline approach!Moving from the async message loop to this declarative staged pipeline is cleaner than a fresh install of Arch Linux. The explicit runtime passing gives you more control over the execution environment too.
Just keep in mind this still won't solve the wasm build issue since you're still using tokio under the hood.
130-150: Got some unused bits lying around here!You're passing
_store_headerand_select_chaintorun_simulatorbut they're gathering dust like old Nintendo cartridges. Plus,number_of_nodesis hardcoded to 1.Are these parameters intended for future use, or leftovers from refactoring? If they're for future features, consider adding a TODO comment. If not, maybe remove them to keep things tidy.
simulation/amaru-sim/src/simulator/simulate.rs (1)
68-91: Top-notch generalization work here!The way you've abstracted from
EchoMessageto genericMsgis smoother than a speedrun through Portal 2. The trait boundsPartialEq + Clone + Debug + Send + 'staticcover all the bases nicely.
| pub store: Arc<Mutex<dyn ChainStore<Header>>>, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
store field went public—watch those invariants
Exposing the store allows any caller to poke inside and bypass validation rules. Unless this struct is purely POD, consider accessor methods or pub(crate) visibility to keep invariants tighter than a Hobbit’s waistcoat.
🤖 Prompt for AI Agents
In crates/amaru-consensus/src/consensus/store_header.rs at lines 24-25, the
`store` field is currently public, which risks breaking invariants by allowing
external code to access it directly. Change the visibility of the `store` field
from `pub` to either `pub(crate)` or private, and provide controlled accessor
methods if external access is needed, to ensure validation rules and invariants
are preserved.
| use pure_stage::{Effects, ExternalEffect, ExternalEffectAPI, StageRef, Void}; | ||
| use std::{fmt, sync::Arc}; | ||
| use tokio::sync::Mutex; |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Heads up about that wasm build drama!
According to the PR comments, this is causing grief when building for wasm - tokio's throwing a wobbly about unsupported features. The pure_stage framework might be pulling in tokio features that don't play nice with wasm (like the full runtime).
Let's check what tokio features are being pulled in:
Consider using conditional compilation or feature flags to handle wasm builds differently, like using wasm-bindgen-futures instead of tokio for wasm targets.
🏁 Script executed:
#!/bin/bash
# Description: Check tokio dependencies and features that might cause wasm build issues
# Find all Cargo.toml files that might specify tokio features
fd Cargo.toml | xargs rg -A 5 'tokio.*features'
# Check pure-stage's dependencies
fd Cargo.toml -g '*pure-stage*' | xargs catLength of output: 1872
Gate Tokio features for wasm32 targets
I checked the workspace Cargo.toml entries and found that both pure-stage and our amaru-consensus crate pull in runtime features that won’t fly on wasm32 (no threads, no blocking). We’re currently importing:
- crates/pure-stage/Cargo.toml →
tokio = { workspace = true, features = ["rt", "time", "rt-multi-thread"] } - crates/amaru-consensus/Cargo.toml →
tokio = { workspace = true, features = ["rt", "macros"] } - In
src/consensus/validate_header.rs:use tokio::sync::Mutex;
Wasm builds choke on Tokio’s threaded runtime and sync primitives—like trying to run Halo Infinite on a ZX Spectrum. Let’s conditionally swap in a single-threaded mutex (e.g. from async_lock or futures) or gate the import behind a wasm feature:
- use tokio::sync::Mutex;
+ #[cfg(not(target_arch = "wasm32"))]
+ use tokio::sync::Mutex;
+ #[cfg(target_arch = "wasm32")]
+ use async_lock::Mutex; // or futures::lock::MutexAnd in your Cargo.toml for amaru-consensus:
[dependencies]
-tokio = { workspace = true, features = ["rt", "macros"] }
+tokio = { workspace = true, default-features = false, features = ["macros"], optional = true }
+async-lock = { version = "2", optional = true }
+
+[features]
+default = ["native"]
+native = ["tokio"]
+wasm = ["async-lock"]This way, we avoid dragging in Tokio’s multi-threaded runtime on wasm32. –Cheers!
🤖 Prompt for AI Agents
In crates/amaru-consensus/src/consensus/validate_header.rs around lines 20 to
22, the import of tokio::sync::Mutex causes wasm build failures due to
unsupported Tokio runtime features. To fix this, gate the import of
tokio::sync::Mutex behind a conditional compilation attribute that excludes
wasm32 targets, and instead import a single-threaded async mutex (e.g., from
async_lock or futures) when compiling for wasm32. Additionally, update the
Cargo.toml to conditionally include Tokio features only for non-wasm targets to
prevent pulling in incompatible runtime components.
There was a problem hiding this comment.
Just remove the rt-multi-thread dependency from pure-stage as I am unsure whether or not it's essential right now
There was a problem hiding this comment.
@abailly No worries, mate — rt-multi-thread has already been yanked from crates/pure-stage/Cargo.toml, so we’re now only pulling in Tokio with rt + time, which keeps the wasm build on the straight and narrow.
Give the WASM build another spin; if it still spits the dummy, we can look at further slimming the Tokio feature set or swapping in a lightweight mutex down the track. But for now, removing rt-multi-thread should do the trick. 🍻
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/amaru-consensus/src/consensus/validate_header.rs (1)
20-22: 🛠️ Refactor suggestion
⚠️ Potential issueGate the Tokio mutex for
wasm32targetsThe unconditional
tokio::sync::Muteximport is still the culprit behind the WASM build face-plant that Stevana called out. Tokio’s sync primitives drag in thread-heavy features that aren’t available in the browser runtime ‑ cue compile-time panic.- use tokio::sync::Mutex; +#[cfg(not(target_arch = "wasm32"))] +use tokio::sync::Mutex; +#[cfg(target_arch = "wasm32")] +use async_lock::Mutex; // single-threaded, wasm-friendly mutexDon’t forget to update
Cargo.tomlwith awasmfeature that swaps Tokio out forasync-lock, so nightly builds don’t go all Cyberpunk 2077 at launch on us.
🧹 Nitpick comments (2)
crates/amaru-consensus/src/consensus/validate_header.rs (2)
137-177: Duplicate implementation ‑ risk of code drift
handle_roll_forward(the “old” path) andhandle_roll_forward_new(thepure_stagepath) are virtually identical. Two codepaths means double maintenance and eventual divergence (the Dark Souls of tech debt).Recommend refactoring the shared logic into a private helper that both call, or delete the legacy method once the staged pipeline is the single source of truth.
241-266:&mut selfnot required – loosen the borrowNeither
validate_headernorhandle_roll_forward_newmutatesself; the only state touched is behindArc<Mutex<…>>. Switching these receivers to&selfwill make the API friendlier and avoid needless exclusive borrows when composing stages.- pub async fn validate_header(&mut self, + pub async fn validate_header(&self,Same goes for
handle_roll_forward_new. Keeps callers from juggling&mutreferences like a game of Mario Kart banana peels.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
crates/amaru-consensus/src/consensus/store_header.rs(1 hunks)crates/amaru-consensus/src/consensus/validate_header.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- crates/amaru-consensus/src/consensus/store_header.rs
⏰ Context from checks skipped due to timeout of 90000ms (9)
- GitHub Check: Simulation
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Build on ubuntu-latest with target aarch64-unknown-linux-musl
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Coverage
- GitHub Check: Snapshots (preprod, 10.1.4)
- GitHub Check: Sanity
🔇 Additional comments (2)
crates/amaru-consensus/src/consensus/validate_header.rs (2)
65-72: 👍 Debug impl sortedNice one replacing the old
unimplemented!()landmine with a safe, placeholder Debug print. No more surprise panics when someone cracks open{:?}in a log.
103-114: Future dropped on the floor? double-checkMessagetrait usage
ExternalEffect::runboxes theResult<Nonces, NoncesError>intoBox<dyn pure_stage::Message>. That’s fine as long as both sides of the channel agree thatResult<…>implementsMessage; otherwise you’ll get a down-cast failure at runtime that’s harder to trace than the Metal Gear Solid plot.If
Messageisn’t blanket-implemented for all types, consider wrapping the result in a dedicated enum tagged for the stage.
| #[derive(Clone)] | ||
| pub struct ValidateHeader { | ||
| ledger: Box<dyn HasStakeDistribution>, | ||
| pub ledger: Arc<dyn HasStakeDistribution>, | ||
| pub store: Arc<Mutex<dyn ChainStore<Header>>>, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Tighten the trait object bounds for thread safety
Mutex<dyn ChainStore<Header>> assumes ChainStore is Send + Sync, otherwise the compiler will complain the moment we try to shuffle this across an async boundary. Be explicit and future-proof:
- pub store: Arc<Mutex<dyn ChainStore<Header>>>,
+ pub store: Arc<Mutex<dyn ChainStore<Header> + Send + Sync>>,Keeps the type self-documenting and avoids “not Send” errors popping up later like a jump scare in Resident Evil.
📝 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.
| #[derive(Clone)] | |
| pub struct ValidateHeader { | |
| ledger: Box<dyn HasStakeDistribution>, | |
| pub ledger: Arc<dyn HasStakeDistribution>, | |
| pub store: Arc<Mutex<dyn ChainStore<Header>>>, | |
| } | |
| #[derive(Clone)] | |
| pub struct ValidateHeader { | |
| pub ledger: Arc<dyn HasStakeDistribution>, | |
| pub store: Arc<Mutex<dyn ChainStore<Header> + Send + Sync>>, | |
| } |
🤖 Prompt for AI Agents
In crates/amaru-consensus/src/consensus/validate_header.rs around lines 59 to
63, the trait object for ChainStore used inside Mutex lacks explicit Send and
Sync bounds, which can cause compiler errors when used across async boundaries.
Update the trait object to include + Send + Sync bounds explicitly, making the
type thread-safe and preventing future "not Send" errors. This involves changing
the type from Mutex<dyn ChainStore<Header>> to Mutex<dyn ChainStore<Header> +
Send + Sync>.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
crates/amaru-consensus/src/consensus/validate_header.rs (1)
61-63:⚠️ Potential issueTrait objects still not
Send + Sync– compile gremlins aheadDéjà vu, mate – the store (& now the ledger) sit inside an async context that can hop across threads, but the trait objects aren’t flagged as
Send + Sync. Rust’ll spit the dummy the minute this future crosses a thread boundary.-pub ledger: Arc<dyn HasStakeDistribution>, -pub store: Arc<Mutex<dyn ChainStore<Header>>>, +pub ledger: Arc<dyn HasStakeDistribution + Send + Sync>, +pub store: Arc<Mutex<dyn ChainStore<Header> + Send + Sync>>,Same fix the bot suggested last round; worth landing before the build server goes all “Not Send” on us.
🧹 Nitpick comments (2)
crates/amaru-consensus/src/consensus/mod.rs (1)
48-68: Beauty of aDebug, but a wee micro-optimisation is handyLovin’ the custom formatting – reads cleaner than a MCU end-credits scene.
Tiny nit:point.to_string()&header.hash().to_string()allocate a newStringevery timeDebuggets called. Swapping to&point/&header.hash()lets the formatter handle it straight off the bat and spares an alloc or two.- .field("point", &point.to_string()) + .field("point", point)(Same idea everywhere you call
to_string()in this block.)Not life-or-death, but every dropped alloc is one fewer gremlin in the heap.
crates/amaru-consensus/src/consensus/validate_header.rs (1)
179-220: Two roll-forward paths = double maintenance
handle_roll_forwardandhandle_roll_forward_newdo near-identical work; the only delta is “external vs direct” nonce evolution. Long-term, this’ll drift faster than the Fast & Furious timeline.Maybe collapse the common bits into a helper or retire the old path once the effect system beds in.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
crates/amaru-consensus/src/consensus/mod.rs(4 hunks)crates/amaru-consensus/src/consensus/validate_header.rs(4 hunks)crates/pure-stage/src/simulation/running.rs(3 hunks)
✅ Files skipped from review due to trivial changes (1)
- crates/pure-stage/src/simulation/running.rs
🧰 Additional context used
🧬 Code Graph Analysis (1)
crates/amaru-consensus/src/consensus/mod.rs (5)
crates/amaru-consensus/src/consensus/validate_header.rs (2)
fmt(66-71)fmt(95-100)crates/amaru-consensus/src/consensus/store.rs (1)
fmt(32-39)crates/amaru-kernel/src/lib.rs (7)
fmt(161-169)fmt(769-774)encode(183-194)encode(374-390)encode(509-518)encode(544-553)encode(579-588)crates/amaru-consensus/src/consensus/chain_selection.rs (2)
point(121-126)encode(92-101)crates/ouroboros-traits/src/is_header/mod.rs (1)
point(33-35)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Coverage
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Build on ubuntu-latest with target aarch64-unknown-linux-musl
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Simulation
- GitHub Check: Snapshots (preprod, 10.1.4)
🔇 Additional comments (1)
crates/amaru-consensus/src/consensus/validate_header.rs (1)
103-113: Return type ofrun()must implementMessage– verify blanket impl
run()boxes aResult<Nonces, NoncesError>intoBox<dyn Message>. That relies on a blanketimpl<T: Any + Send> Message for T. If thepure-stagecrate ever tightens that bound, we’ll be left like Mario without mushrooms.Can you confirm
Result<…>is indeedAny + Sendand the trait impl is present? Otherwise we’ll need a small wrapper enum implementingMessage.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Nitpick comments (1)
simulation/amaru-sim/tests/simulation.rs (1)
14-17:init()will panic if another test beat it to the mic.
tracing_subscriber::fmt().init()may panic with “set_logger called twice” when multiple tests run in the same process (cargo test’s default).
Swap totry_init()and silently ignore the already-initialised error to keep the test suite zen-like.- .init(); + .try_init().ok();
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (3)
.github/workflows/CI.yml(0 hunks)simulation/amaru-sim/src/simulator/mod.rs(4 hunks)simulation/amaru-sim/tests/simulation.rs(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/CI.yml
🔇 Additional comments (1)
simulation/amaru-sim/tests/simulation.rs (1)
19-20: Pass a &Runtime, don’t sacrifice it to the simulator.You move the
Runtimeintosimulator::run, so it’s dropped as soon asrunreturns.
If background tasks outliverun, they’ll panic when the runtime evaporates. Consider passing&tokio::runtime::Runtime(or just its handle) instead.
| #[test] | ||
| fn run_simulator() { | ||
| let args = Args { | ||
| stake_distribution_file: "tests/data/stake-distribution.json".into(), | ||
| consensus_context_file: "tests/data/consensus-context.json".into(), | ||
| chain_dir: ".".into(), | ||
| block_tree_file: "tests/data/chain.json".into(), | ||
| start_header: Hash::from([0; 32]), | ||
| }; | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
No checks? The test is all crackle and no pop.
The test spins up the simulator but never asserts anything.
Right now it’ll pass even if the simulator goes full Skynet and deletes the chain store. Add at least one assertion on the trace or the process exit code so the test has teeth.
🤖 Prompt for AI Agents
In simulation/amaru-sim/tests/simulation.rs around lines 4 to 13, the test
run_simulator does not include any assertions to verify correct behavior. Add at
least one assertion to check the simulator's output, such as validating the
trace results or confirming the process exit code is successful, to ensure the
test fails if the simulator malfunctions.
There was a problem hiding this comment.
The assertion is done in the simulate::simulate function further down the call stack
There was a problem hiding this comment.
@abailly Ah, gotcha — the teeth are hiding a couple of frames down!
simulator::run eventually dives into simulate::simulate, where the provided property closure panics if the last trace element isn’t the expected forward of the last block. That panic bubbles back up to the test, so any divergence will indeed make the test fail.
I’ll stand down on the “missing assertion” remark. Maybe a tiny comment in the test to flag that the heavy-lifting assertions live inside simulate could fend off future reviewers (or future me) from raising the same false alarm, but functionally you’re already covered.
Cheers for the clarification!
| fn run_simulator( | ||
| rt: tokio::runtime::Runtime, | ||
| global: GlobalParameters, | ||
| validate_header: &mut ValidateHeader, | ||
| store_header: &mut StoreHeader, | ||
| select_chain: &mut SelectChain, | ||
| chain_data_path: &PathBuf, | ||
| ) { | ||
| loop { | ||
| let span = tracing::info_span!("simulator"); | ||
| match input_reader.read().await { | ||
| Err(err) => { | ||
| tracing::error!("Error reading message: {:?}", err); | ||
| break; | ||
| } | ||
| Ok(msg) => { | ||
| // receive stage | ||
| let chain_sync_event = | ||
| mk_message(msg, span).and_then(|chain_sync: ChainSyncEvent| { | ||
| handle_chain_sync(chain_sync).map_err(|_| WorkerError::Recv) | ||
| }); | ||
|
|
||
| // validate stage | ||
| let validation_event = match chain_sync_event { | ||
| Ok(event) => match event { | ||
| DecodedChainSyncEvent::RollForward { | ||
| peer, | ||
| point, | ||
| header, | ||
| .. | ||
| } => validate_header | ||
| .handle_roll_forward(peer, point, header, global_parameters) | ||
| .await | ||
| .expect("unexpected error on roll forward"), | ||
| DecodedChainSyncEvent::Rollback { .. } => event, | ||
| }, | ||
| Err(_) => panic!("got error validating chain sync"), | ||
| }; | ||
| let config = Config::default(); |
There was a problem hiding this comment.
Signature mismatch: expects owned GlobalParameters, gets a reference.
See call-site above. Switch the parameter to a reference to keep ownership simple:
-fn run_simulator(
- rt: tokio::runtime::Runtime,
- global: GlobalParameters,
+fn run_simulator(
+ rt: tokio::runtime::Runtime,
+ global: &GlobalParameters,(and adjust uses inside the function). Without this, the module won’t compile.
📝 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.
| fn run_simulator( | |
| rt: tokio::runtime::Runtime, | |
| global: GlobalParameters, | |
| validate_header: &mut ValidateHeader, | |
| store_header: &mut StoreHeader, | |
| select_chain: &mut SelectChain, | |
| chain_data_path: &PathBuf, | |
| ) { | |
| loop { | |
| let span = tracing::info_span!("simulator"); | |
| match input_reader.read().await { | |
| Err(err) => { | |
| tracing::error!("Error reading message: {:?}", err); | |
| break; | |
| } | |
| Ok(msg) => { | |
| // receive stage | |
| let chain_sync_event = | |
| mk_message(msg, span).and_then(|chain_sync: ChainSyncEvent| { | |
| handle_chain_sync(chain_sync).map_err(|_| WorkerError::Recv) | |
| }); | |
| // validate stage | |
| let validation_event = match chain_sync_event { | |
| Ok(event) => match event { | |
| DecodedChainSyncEvent::RollForward { | |
| peer, | |
| point, | |
| header, | |
| .. | |
| } => validate_header | |
| .handle_roll_forward(peer, point, header, global_parameters) | |
| .await | |
| .expect("unexpected error on roll forward"), | |
| DecodedChainSyncEvent::Rollback { .. } => event, | |
| }, | |
| Err(_) => panic!("got error validating chain sync"), | |
| }; | |
| let config = Config::default(); | |
| fn run_simulator( | |
| rt: tokio::runtime::Runtime, | |
| global: &GlobalParameters, | |
| validate_header: &mut ValidateHeader, | |
| chain_data_path: &PathBuf, | |
| ) { | |
| let config = Config::default(); | |
| // … | |
| } |
🤖 Prompt for AI Agents
In simulation/amaru-sim/src/simulator/mod.rs around lines 123 to 129, the
function run_simulator currently takes GlobalParameters by value but is called
with a reference. Change the parameter type from owned GlobalParameters to a
reference (&GlobalParameters) and update all uses inside the function
accordingly to fix the signature mismatch and ensure the module compiles.
| let init_store = StoreHeader { | ||
| store: validate_header.store.clone(), | ||
| }; | ||
|
|
||
| // chain selection stage | ||
| match select_chain.handle_chain_sync(store_event).await { | ||
| Ok(events) => { | ||
| let mut w = output_writer.lock().await; | ||
| write_events(&mut w, &store, &events).await; | ||
| let receive_stage = network.stage( | ||
| "receive_header", | ||
| async |(_state, downstream, out), | ||
| msg: Envelope<ChainSyncMessage>, | ||
| eff| | ||
| -> Result< | ||
| ( |
There was a problem hiding this comment.
ChainSyncMessage is used but never imported – the compiler’s gonna spit the dummy.
Every stage refers to ChainSyncMessage, yet there’s no use bringing it into scope. Add the correct path, e.g.:
use amaru_consensus::{
consensus::{
chain_selection::{ChainSelector, ChainSelectorBuilder},
@@
- ChainSyncEvent, DecodedChainSyncEvent,
+ ChainSyncEvent, DecodedChainSyncEvent, chain_sync::ChainSyncMessage,
},Double-check the exact module path, but you get the gist.
📝 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.
| let init_store = StoreHeader { | |
| store: validate_header.store.clone(), | |
| }; | |
| // chain selection stage | |
| match select_chain.handle_chain_sync(store_event).await { | |
| Ok(events) => { | |
| let mut w = output_writer.lock().await; | |
| write_events(&mut w, &store, &events).await; | |
| let receive_stage = network.stage( | |
| "receive_header", | |
| async |(_state, downstream, out), | |
| msg: Envelope<ChainSyncMessage>, | |
| eff| | |
| -> Result< | |
| ( | |
| // at the top of simulation/amaru-sim/src/simulator/mod.rs | |
| use amaru_consensus::{ | |
| consensus::{ | |
| chain_selection::{ChainSelector, ChainSelectorBuilder}, | |
| ChainSyncEvent, DecodedChainSyncEvent, chain_sync::ChainSyncMessage, | |
| }, | |
| // … other imports … | |
| }; |
🤖 Prompt for AI Agents
In simulation/amaru-sim/src/simulator/mod.rs around lines 139 to 149, the type
ChainSyncMessage is used but not imported, causing a compiler error. Identify
the correct module path for ChainSyncMessage in the project or dependencies and
add a corresponding use statement at the top of the file to bring
ChainSyncMessage into scope.
| rt, | ||
| global_parameters.clone(), | ||
| &mut consensus, | ||
| &mut store_header, | ||
| &mut select_chain, | ||
| ) | ||
| .await; | ||
| &chain_data_path, | ||
| ); |
There was a problem hiding this comment.
global_parameters.clone() won’t compile – it’s a &'static GlobalParameters.
NetworkName::Testnet(42).into() yields a &'static GlobalParameters, which is neither Clone nor owned.
Either change run_simulator to take &GlobalParameters or dereference the constant ((*global_parameters).clone()) after implementing Clone for the concrete type.
Suggested fix (simplest):
- global_parameters.clone(),
+ global_parameters,…paired with the signature tweak below.
📝 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.
| rt, | |
| global_parameters.clone(), | |
| &mut consensus, | |
| &mut store_header, | |
| &mut select_chain, | |
| ) | |
| .await; | |
| &chain_data_path, | |
| ); | |
| rt, | |
| global_parameters, | |
| &mut consensus, | |
| &chain_data_path, | |
| ); |
🤖 Prompt for AI Agents
In simulation/amaru-sim/src/simulator/mod.rs around lines 116 to 120, the call
to run_simulator passes global_parameters.clone(), but global_parameters is a
&'static GlobalParameters reference which does not implement Clone and cannot be
cloned. To fix this, change the run_simulator function signature to accept a
reference to GlobalParameters instead of an owned value, and pass
global_parameters directly without cloning. This avoids the need to clone the
static reference and resolves the compilation error.
| use anyhow::Error; | ||
| use bytes::Bytes; | ||
| use clap::Parser; |
There was a problem hiding this comment.
Wrong Bytes, mate – you’re importing the crate, but constructing your own struct.
use bytes::Bytes; pulls in the bytes crate type, yet later you build Bytes { bytes: … }, which is a custom struct from mod bytes;.
Compilation will face-plant.
-use bytes::Bytes;
+use self::bytes::Bytes;Alternatively rename your local struct to avoid the namespace clash – whatever tickles your fancy.
📝 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 anyhow::Error; | |
| use bytes::Bytes; | |
| use clap::Parser; | |
| use anyhow::Error; | |
| use self::bytes::Bytes; | |
| use clap::Parser; |
🤖 Prompt for AI Agents
In simulation/amaru-sim/src/simulator/mod.rs around lines 35 to 37, you are
importing Bytes from the external bytes crate but also defining your own Bytes
struct locally, causing a naming conflict. To fix this, either rename your local
Bytes struct to a different name to avoid clashing with the external crate or
remove the use bytes::Bytes import if you only intend to use your local struct.
Ensure that all references to Bytes in the code match the correct struct after
renaming or import adjustment.
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
This is needed otherwise the underlyign machinery will choke using a runtime that's also dropped when simulate function returns. Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
not really tested as the receive stage fails to decode random headers, so we need to wire the "generator" from a file Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Stevan A <stevana@users.noreply.github.com> Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Stevan A <stevana@users.noreply.github.com>
Signed-off-by: Stevan A <stevana@users.noreply.github.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Stevan A <stevana@users.noreply.github.com>
Signed-off-by: Stevan A <stevana@users.noreply.github.com> Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
abailly
force-pushed
the
restructure-consensus-simulator
branch
from
c73c12a to
f9eb5f7
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (2)
simulation/amaru-sim/src/simulator/mod.rs (2)
124-129:⚠️ Potential issueUpdate
run_simulatorsignature to accept a reference-fn run_simulator( - rt: tokio::runtime::Runtime, - global: GlobalParameters, +fn run_simulator( + rt: tokio::runtime::Runtime, + global: &GlobalParameters,Adjust downstream code to use
*globalorglobal.clone()(after confirmingGlobalParameters: Clone) where an owned copy is really needed.
115-120: 🛠️ Refactor suggestionType mismatch: passing
&GlobalParametersto a function that wants an owned value
global_parameters.clone()is only cloning the reference (&GlobalParameters), butrun_simulatorexpects an ownedGlobalParameters. Compiler will throw a tanty.Quickest fix: make
run_simulatortake a reference instead.- rt, - global_parameters.clone(), + rt, + global_parameters,
🧹 Nitpick comments (1)
crates/amaru-consensus/src/consensus/validate_header.rs (1)
146-178: Two competing roll-forward paths – pick one & bin the otherWe now have
handle_roll_forward(old, direct call) andhandle_roll_forward_new(effect-based). Only the second is used by the staged pipeline, yethandle_chain_syncstill calls the old one. That’s confusion territory – like keeping both VHS and Blu-ray in the same box set.Either:
- Re-wire
handle_chain_syncto the new method, or- Remove the legacy method entirely.
Keeps the codebase slim and future maintainers happy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (2)
Cargo.lockis excluded by!**/*.lockexamples/Cargo.lockis excluded by!**/*.lock
📒 Files selected for processing (22)
.github/workflows/CI.yml(0 hunks)crates/amaru-consensus/Cargo.toml(2 hunks)crates/amaru-consensus/src/consensus/mod.rs(4 hunks)crates/amaru-consensus/src/consensus/receive_header.rs(1 hunks)crates/amaru-consensus/src/consensus/store.rs(1 hunks)crates/amaru-consensus/src/consensus/store_header.rs(1 hunks)crates/amaru-consensus/src/consensus/validate_header.rs(4 hunks)crates/amaru-consensus/src/lib.rs(1 hunks)crates/amaru-kernel/src/protocol_parameters.rs(1 hunks)crates/amaru/src/stages/mod.rs(1 hunks)crates/pure-stage/Cargo.toml(1 hunks)crates/pure-stage/src/lib.rs(1 hunks)crates/pure-stage/src/simulation/running.rs(3 hunks)simulation/amaru-sim/src/bin/amaru-sim/main.rs(1 hunks)simulation/amaru-sim/src/simulator/bytes.rs(2 hunks)simulation/amaru-sim/src/simulator/generate.rs(15 hunks)simulation/amaru-sim/src/simulator/ledger.rs(1 hunks)simulation/amaru-sim/src/simulator/mod.rs(4 hunks)simulation/amaru-sim/src/simulator/simulate.rs(10 hunks)simulation/amaru-sim/src/simulator/sync.rs(4 hunks)simulation/amaru-sim/tests/data/stake-distribution.json(2 hunks)simulation/amaru-sim/tests/simulation.rs(1 hunks)
💤 Files with no reviewable changes (1)
- .github/workflows/CI.yml
🚧 Files skipped from review as they are similar to previous changes (18)
- crates/amaru/src/stages/mod.rs
- crates/amaru-consensus/Cargo.toml
- crates/amaru-kernel/src/protocol_parameters.rs
- crates/pure-stage/src/lib.rs
- crates/amaru-consensus/src/consensus/receive_header.rs
- simulation/amaru-sim/src/simulator/ledger.rs
- simulation/amaru-sim/tests/simulation.rs
- simulation/amaru-sim/src/bin/amaru-sim/main.rs
- crates/pure-stage/Cargo.toml
- crates/amaru-consensus/src/lib.rs
- crates/amaru-consensus/src/consensus/store.rs
- crates/amaru-consensus/src/consensus/mod.rs
- simulation/amaru-sim/src/simulator/sync.rs
- simulation/amaru-sim/src/simulator/bytes.rs
- crates/pure-stage/src/simulation/running.rs
- crates/amaru-consensus/src/consensus/store_header.rs
- simulation/amaru-sim/src/simulator/generate.rs
- simulation/amaru-sim/src/simulator/simulate.rs
🧰 Additional context used
🪛 Gitleaks (8.26.0)
simulation/amaru-sim/tests/data/stake-distribution.json
4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
21-21: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
30-30: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
34-34: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
38-38: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
47-47: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
51-51: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
55-55: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
64-64: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
68-68: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
89-89: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
98-98: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
102-102: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
106-106: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
115-115: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
119-119: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
123-123: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
132-132: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
136-136: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
140-140: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
149-149: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
153-153: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
157-157: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
166-166: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
170-170: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
174-174: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
183-183: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
187-187: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
191-191: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
200-200: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
204-204: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
208-208: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
217-217: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
221-221: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
225-225: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
234-234: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
238-238: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
242-242: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
251-251: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
255-255: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Build on ubuntu-latest with target aarch64-unknown-linux-musl
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Snapshots (preprod, 10.1.4)
- GitHub Check: Coverage
🔇 Additional comments (2)
crates/amaru-consensus/src/consensus/validate_header.rs (1)
21-23: Tokio mutex on WASM – double-check feature gate
tokio::sync::Mutexis fine on native, but onwasm32it only works if thesyncfeature is enabled. Confirm your workspace’stokioentry includesfeatures = ["rt","time","sync"], otherwise the build will spit the dummy again.simulation/amaru-sim/src/simulator/mod.rs (1)
293-294:global.clone()inside spawn may still fail after the changeIf
globalis now a&GlobalParameters, callingclone()just gives another reference. When wiring the stage you probably want a real copy:let init_global = (*global).clone(); // requires Clone on GlobalParametersor pass the reference through the stage tuple if cloning isn’t necessary.
| { | ||
| "chain": "fcb4a51804f14f3f5b5ad841199b557aed0187280f7855736bdb153b0d202bb6", | ||
| "coldSignKey": "7d69f26449fea78dd035fd88a360205ee15fe03d4e3f019814fc12cce0fac334", | ||
| "individualStake": { | ||
| "individualPoolStake": { | ||
| "denominator": 16, | ||
| "numerator": 1 | ||
| }, | ||
| "individualPoolStakeVrf": "5e32e78311c07da914ac925dbfc4aecbb874245a72f94c2253dd64e39340b085", | ||
| "individualTotalPoolStake": 625000000000000 | ||
| }, | ||
| "kesSignKey": "cba58fe1e36fbe608d047ba4f1774efebad8fbac39b53d9b73bc80a8f8dd4c44e8eb434c62d143454d85f5ece2a675cade637e20fe4cff3f11556ecde7ad2ff0ac24643578b384023b70d7a93e3f2435e4f9e38dc65c40df258d7379228b6ea42ea108ce0f27da1e113e3017311451c8972eaf8127f19b900fab6c241f06dcca4d1f153541b7cd87d47144f44c9bf945501f3ccb9de9e7664b473eedb7710a3af57cced81eb964eff669ef38c7e5036eb8d4f8d664bd7b5a50f03dd56e0435ec017e92b07be6ec30c20724593c79648978fbe94501ef85f261eb38209f8a2f8260b794ec16692996a80f7620b0e256c0bb45da1e701ce21bfd3edfaf908f1f6d9352981606a4c6d22d3f14a0b4f2f37c4b8085974aed296379d80e66f83506b7513a9e3c8b9ad6268994e42fa50be81faae29e8cf62bd7bd75b09a06652f9e0e5b0e52068251c13bdb85261d4aa9c22f18235f019c4e2cf4e94cb49c38f2eea330b0f898800be71e896a06a157e8ad8801887a2777e10d1d3eb6065b2712946f51b89bd9d239ed01417eef7f09a94eb286a1a4c08e6ad99a767346ec031c3797d9a18aef202ac7782b91a08fe3075afb79b4880613aa78210de4c5ba7a20c699b4c48e149cb0b5dea39cdf1ba4977433afea1261a9992f207e0b8589e048e85abc751c50b717dc2f3ddc099109d6637cc881d59fd58c865305bae525fc84de10c9aefa0f0b8a83da104138bd998daf300dad244a8ad10a42c2a61b372c34cb1ff5728e848bfd0722355748f9dfe3765aad64cccf0c030442258fd81a6dba936803bedc6096ff1d90072ef1a24e231f60c43c3d4b5f31776fe56287ecf228013c", | ||
| "ocertCounter": 49, | ||
| "poolId": "a0664cae7b12c6c64f08c08e9affef46b8d096c18bc7c3adba382f0f", | ||
| "poolIdx": 16, | ||
| "vrfSignKey": "82c7e81244a3bce96904feaeabaffc4270245f77b0d26e89ed0ce42a2a89b2af6ee3a36d9b941fcbb53e647468d10b0ae958d8a77b9f60d2ebd247a05a654c6d" | ||
| }, |
There was a problem hiding this comment.
Whoa, full private keys committed – even for test data that’s risky
The JSON contains un-truncated KES / VRF / cold signing keys that look like production-sized secrets. Gitleaks is screaming, and rightly so: anyone cloning the repo now owns those keys.
If these are only throw-away fixtures, trim them (e.g. keep first/last 8 bytes) or load them from an encrypted fixture at test-time. Otherwise you’ve just parked a ute full of private keys in the public pub car park.
🧰 Tools
🪛 Gitleaks (8.26.0)
4-4: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
13-13: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
17-17: Detected a Generic API Key, potentially exposing access to various services and sensitive operations.
(generic-api-key)
🤖 Prompt for AI Agents
In simulation/amaru-sim/tests/data/stake-distribution.json lines 2 to 18, the
file contains full private keys for KES, VRF, and cold signing keys, which is a
security risk. To fix this, replace the full private keys with truncated
versions showing only the first and last 8 bytes or alternatively load these
keys from an encrypted fixture during test execution to avoid exposing sensitive
data in the repository.
There was a problem hiding this comment.
This data is completely generated for test purposes so it does not matter who owns those keys
There was a problem hiding this comment.
@abailly Ah, sweet as then! If those keys are just synthetic test fodder, there’s no real danger of your mainnet wallet getting nicked. 🚐🔑
Two tiny housekeeping ideas you might still consider (purely to keep the CI scanners from crying wolf and to trim repo girth):
- Drop a short comment at the top of the JSON (
// generated test keys – non-secret) so future reviewers (and tools like Gitleaks) know they’re safe. - Or add an entry to your
.gitleaks.toml/.gitignoreto silence that specific path, so the pipeline stays green.
Totally optional—your call. Cheers for clarifying, mate! 🍻🎮
✏️ Learnings added
Learnt from: abailly
PR: pragma-org/amaru#250
File: simulation/amaru-sim/tests/data/stake-distribution.json:2-18
Timestamp: 2025-06-13T09:06:16.000Z
Learning: The file `simulation/amaru-sim/tests/data/stake-distribution.json` contains fully generated, non-sensitive private key values used solely for testing; they are considered safe to keep in the repository despite resembling real secrets.
Note: Learnings are effective only in the context of similar code segments. To apply general coding guidelines, please configure review instructions. You can manage existing learnings in the app.
| pub ledger: Arc<dyn HasStakeDistribution>, | ||
| pub store: Arc<Mutex<dyn ChainStore<Header>>>, | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add Send + Sync bounds to the ChainStore trait object
Arc<Mutex<dyn ChainStore<Header>>> crosses async await points; without + Send + Sync you’ll hit a “Future is not Send” wall as soon as another crate awaits on it.
- pub store: Arc<Mutex<dyn ChainStore<Header>>>,
+ pub store: Arc<Mutex<dyn ChainStore<Header> + Send + Sync>>,📝 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.
| pub ledger: Arc<dyn HasStakeDistribution>, | |
| pub store: Arc<Mutex<dyn ChainStore<Header>>>, | |
| } | |
| pub ledger: Arc<dyn HasStakeDistribution>, | |
| pub store: Arc<Mutex<dyn ChainStore<Header> + Send + Sync>>, | |
| } |
🤖 Prompt for AI Agents
In crates/amaru-consensus/src/consensus/validate_header.rs around lines 61 to
63, the ChainStore trait object inside the Arc<Mutex<dyn ChainStore<Header>>>
lacks Send + Sync bounds, which causes issues when crossing async await points.
To fix this, add + Send + Sync to the ChainStore trait object, changing it to
Arc<Mutex<dyn ChainStore<Header> + Send + Sync>> to ensure it is safe to send
and share across threads in async contexts.
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
abailly
force-pushed
the
restructure-consensus-simulator
branch
from
8195d20 to
f00ff7c
Compare
also fix some formatting issues... :sigh: Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🔭 Outside diff range comments (1)
simulation/amaru-sim/src/simulator/generate.rs (1)
181-183:⚠️ Potential issue
random_rangewill not compile – switch togen_range
rand::Rngdoesn’t expose arandom_rangemethod, so the compiler is going to go full “Fatal Error” on ya.
Usegen_rangeinstead (or implement your own extension trait if you fancy).- let random_not_visited_index = rng.random_range(0..not_visited.len()); + let random_not_visited_index = rng.gen_range(0..not_visited.len());
♻️ Duplicate comments (4)
simulation/amaru-sim/src/simulator/mod.rs (4)
35-37: Still importing the wrongBytesYou’re grabbing
bytes::Bytesfrom the external crate while later constructing your ownBytes { bytes: … }. That clash will keep the compiler redder than Darth Maul.-use bytes::Bytes; +use self::bytes::Bytes;
116-120:global_parameters.clone()won’t fly
global_parametersis a&GlobalParameters; references aren’tClone.
Either pass the reference straight through or makeGlobalParametersclonable.- global_parameters.clone(), + global_parameters,
295-297: Further.clone()onglobal– will break once the above fix landsAfter switching to a borrowed
&GlobalParameters, this clone has to disappear too:- (init_st, global.clone(), store_header_stage.sender()), + (init_st, global, store_header_stage.sender()),
123-128:⚠️ Potential issueSignature mismatch for
run_simulatorcauses type chaosBecause you’re now passing a
&GlobalParameters, the function should accept just that. Also drop the internal.clone()uses or deriveCloneon the struct.-fn run_simulator( - rt: tokio::runtime::Runtime, - global: GlobalParameters, +fn run_simulator( + rt: tokio::runtime::Runtime, + global: &GlobalParameters,(remember to tweak every
global.clone()inside).
🧹 Nitpick comments (1)
simulation/amaru-sim/src/simulator/generate.rs (1)
70-72: Panic-on-error file reading – consider surfacing an error instead
unwrap_or_else(panic!(…))will crash the sim if the JSON file is missing.
Returning anio::Result<String>(or bubbling the error up) gives callers a chance to recover gracefully – a bit friendlier than a sudden “ya dead” screen.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
crates/amaru-consensus/src/consensus/mod.rs(4 hunks)crates/pure-stage/src/simulation/running.rs(3 hunks)simulation/amaru-sim/src/simulator/bytes.rs(2 hunks)simulation/amaru-sim/src/simulator/generate.rs(15 hunks)simulation/amaru-sim/src/simulator/mod.rs(4 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- simulation/amaru-sim/src/simulator/bytes.rs
- crates/pure-stage/src/simulation/running.rs
- crates/amaru-consensus/src/consensus/mod.rs
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Snapshots (preprod, 10.1.4)
- GitHub Check: Coverage
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-latest with target aarch64-unknown-linux-musl
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
| pub fn generate_inputs_strategy( | ||
| file_path: &PathBuf, | ||
| ) -> impl Strategy<Value = Vec<ChainSyncMessage>> + use<'_> { | ||
| any::<u64>().prop_map(|seed| { | ||
| let mut rng = StdRng::seed_from_u64(seed); | ||
| generate_inputs(&mut rng, file_path).unwrap() | ||
| }) | ||
| } |
There was a problem hiding this comment.
Garbage lifetime bound + use<'_> nukes the return type
use<'_> isn’t valid Rust syntax – the compiler will trip harder than Mario on a banana peel.
Just drop the bogus bound (or add a real lifetime / Clone / 'static as needed).
-) -> impl Strategy<Value = Vec<ChainSyncMessage>> + use<'_> {
+) -> impl Strategy<Value = Vec<ChainSyncMessage>> {📝 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.
| pub fn generate_inputs_strategy( | |
| file_path: &PathBuf, | |
| ) -> impl Strategy<Value = Vec<ChainSyncMessage>> + use<'_> { | |
| any::<u64>().prop_map(|seed| { | |
| let mut rng = StdRng::seed_from_u64(seed); | |
| generate_inputs(&mut rng, file_path).unwrap() | |
| }) | |
| } | |
| pub fn generate_inputs_strategy( | |
| file_path: &PathBuf, | |
| ) -> impl Strategy<Value = Vec<ChainSyncMessage>> { | |
| any::<u64>().prop_map(|seed| { | |
| let mut rng = StdRng::seed_from_u64(seed); | |
| generate_inputs(&mut rng, file_path).unwrap() | |
| }) | |
| } |
🤖 Prompt for AI Agents
In simulation/amaru-sim/src/simulator/generate.rs around lines 298 to 305, the
return type of the function includes an invalid lifetime bound `+ use<'_>`,
which is not valid Rust syntax and causes compilation errors. Remove the `+
use<'_>` bound entirely from the return type to fix the issue, ensuring the
function signature only specifies the correct Strategy return type without the
bogus lifetime bound.
There was a problem hiding this comment.
The code actually compiles so this comment is weird
There was a problem hiding this comment.
@abailly Ah, my bad—looks like I was swinging at thin air there. 🤦♂️
+ use<'_> isn’t some rogue lifetime; it’s the proptest marker trait proptest::strategy::Strategy::Value = ... + Use<'a> (spelled use in the auto-expanded code). Because the macro re-exports it lowercase, it’s perfectly valid and compiles fine on nightly/stable alike.
Thanks for the heads-up; I’ll holster that banana-peel warning. 🍌✨
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (3)
simulation/amaru-sim/src/simulator/mod.rs (3)
15-24:⚠️ Potential issueMissing
ChainSyncMessageimport – compiler will chuck a wobbly, mate
ChainSyncMessageis used from line 150 onwards but never brought into scope.
Add the properuseso the borrow-checker doesn’t go walkabout:use amaru_consensus::{ consensus::{ chain_selection::{ChainSelector, ChainSelectorBuilder}, receive_header::handle_chain_sync, store::ChainStore, store_header::StoreHeader, validate_header::ValidateHeader, - ChainSyncEvent, DecodedChainSyncEvent, + ChainSyncEvent, DecodedChainSyncEvent, chain_sync::ChainSyncMessage, },
35-38:⚠️ Potential issueName clash with external
bytes::Bytes– wrong struct, wrong pub crawlYou import
bytes::Bytes, then later constructBytes { bytes: … }– that field doesn’t exist on the crate type.
Either drop the external import or qualify the local one (self::bytes::Bytes) to avoid the identity crisis.-use bytes::Bytes; +use self::bytes::Bytes;
115-126:⚠️ Potential issueSignature mismatch – passing
&GlobalParameterswhere an owned value is expected
bootstrapcalls:run_simulator(rt, global_parameters.clone(), …)
global_parametersis a&GlobalParameters; cloning keeps it a reference, but
run_simulatorwants an ownedGlobalParameters. The compiler’s gonna rage-quit.Quickest fix: take a reference instead and drop the clone:
-fn run_simulator( - rt: tokio::runtime::Runtime, - global: GlobalParameters, +fn run_simulator( + rt: tokio::runtime::Runtime, + global: &GlobalParameters, … - global_parameters.clone(), + global_parameters,
🧹 Nitpick comments (1)
simulation/amaru-sim/src/simulator/mod.rs (1)
276-284: Hard-coded"n1"/ single-dest broadcast – future pain inboundYou’ve stubbed the source as
"n1"and the dest as the single peer.
Once you spin up more than one node, this will behave like a boom-gate stuck on red.Consider deriving the local node id at startup and fanning the message to all followers here.
Just a tiny tweak now will save a Mad-Max-style refactor later.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
simulation/amaru-sim/src/simulator/mod.rs(4 hunks)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Coverage
- GitHub Check: Snapshots (preprod, 10.1.4)
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
- GitHub Check: Build on ubuntu-latest with target aarch64-unknown-linux-musl
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (2)
simulation/amaru-sim/src/simulator/mod.rs (2)
35-37:⚠️ Potential issueStill importing the wrong
Bytes– naming clash will KO the compilerWe’re again pulling in
bytes::Bytesfrom the external crate while you’ve got your own hipster‐craftedbytesmodule just below. The compiler will get as confused as Marty McFly meeting his future self.-use bytes::Bytes; +use self::bytes::Bytes;Same fix as previously suggested – either qualify the local struct or rename it, but right now the code won’t build.
115-127:⚠️ Potential issueSignature mismatch between
bootstrap→run_simulatoris still hanging about
bootstraphands over a&GlobalParameters(global_parameters.clone()is still a&GlobalParameters, because references areCopy + Clone), butrun_simulatorexpects an ownedGlobalParameters. Result: type error, compilation face-plants.Patch it by passing a reference throughout:
-fn run_simulator( - rt: tokio::runtime::Runtime, - global: GlobalParameters, +fn run_simulator( + rt: tokio::runtime::Runtime, + global: &GlobalParameters,and in the call-site:
- run_simulator( - rt, - global_parameters.clone(), + run_simulator( + rt, + global_parameters,Inside
run_simulatoryou can keep usingglobal.clone()because a reference isCopy.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
simulation/amaru-sim/src/simulator/mod.rs(4 hunks)
🧰 Additional context used
🧬 Code Graph Analysis (1)
simulation/amaru-sim/src/simulator/mod.rs (9)
simulation/amaru-sim/src/simulator/generate.rs (3)
generate_inputs_strategy(298-305)parse_json(74-77)read_chain_json(70-72)simulation/amaru-sim/src/simulator/ledger.rs (3)
populate_chain_store(128-151)from_file(58-68)new(70-85)simulation/amaru-sim/src/simulator/simulate.rs (4)
simulate(228-281)pure_stage_node_handle(73-91)new(157-166)rx(85-85)simulation/amaru-sim/src/simulator/sync.rs (5)
from(220-237)new(90-93)new(137-140)default(84-86)default(131-133)crates/amaru/src/stages/consensus/validate_header.rs (1)
new(36-43)crates/amaru/src/stages/consensus/select_chain.rs (1)
new(36-42)crates/amaru/src/stages/consensus/store_header.rs (1)
new(34-40)simulation/amaru-sim/tests/simulation.rs (1)
run_simulator(5-21)crates/amaru-consensus/src/consensus/receive_header.rs (1)
handle_chain_sync(41-69)
⏰ Context from checks skipped due to timeout of 90000ms (7)
- GitHub Check: Build on ubuntu-latest with target riscv32im-risc0-zkvm-elf
- GitHub Check: Build on ubuntu-latest with target x86_64-unknown-linux-gnu
- GitHub Check: Build on windows-latest with target x86_64-pc-windows-msvc
- GitHub Check: Build on ubuntu-latest with target aarch64-unknown-linux-musl
- GitHub Check: Build on macos-latest with target aarch64-apple-darwin
- GitHub Check: Snapshots (preprod, 10.1.4)
- GitHub Check: Coverage
| hash: match point { | ||
| Origin => Bytes { bytes: vec![0; 32] }, | ||
| Specific(_slot, hash) => Bytes { bytes: hash }, | ||
| }, | ||
| header: Bytes { | ||
| bytes: to_cbor(&header), | ||
| }, | ||
| }, | ||
| ), | ||
| DecodedChainSyncEvent::Rollback { | ||
| peer, | ||
| rollback_point, | ||
| .. | ||
| } => ( | ||
| peer, | ||
| ChainSyncMessage::Bck { | ||
| msg_id, | ||
| slot: rollback_point.slot_or_default(), | ||
| hash: match rollback_point { | ||
| Origin => Bytes { bytes: vec![0; 32] }, | ||
| Specific(_slot, hash) => Bytes { bytes: hash }, | ||
| }, |
There was a problem hiding this comment.
Wrong type fed into Bytes { bytes: … } – Hash<32> ≠ Vec<u8>
When forging the Fwd / Bck messages we shove a Hash<32> straight into the bytes field that expects a Vec<u8>. The compiler will throw a tantrum harder than Kratos in traffic.
- Specific(_slot, hash) => Bytes { bytes: hash },
+ Specific(_slot, hash) => Bytes { bytes: hash.to_vec() },…and the same fix down in the rollback arm:
- Specific(_slot, hash) => Bytes { bytes: hash },
+ Specific(_slot, hash) => Bytes { bytes: hash.to_vec() },That’ll keep the type checker happy.
📝 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.
| hash: match point { | |
| Origin => Bytes { bytes: vec![0; 32] }, | |
| Specific(_slot, hash) => Bytes { bytes: hash }, | |
| }, | |
| header: Bytes { | |
| bytes: to_cbor(&header), | |
| }, | |
| }, | |
| ), | |
| DecodedChainSyncEvent::Rollback { | |
| peer, | |
| rollback_point, | |
| .. | |
| } => ( | |
| peer, | |
| ChainSyncMessage::Bck { | |
| msg_id, | |
| slot: rollback_point.slot_or_default(), | |
| hash: match rollback_point { | |
| Origin => Bytes { bytes: vec![0; 32] }, | |
| Specific(_slot, hash) => Bytes { bytes: hash }, | |
| }, | |
| hash: match point { | |
| Origin => Bytes { bytes: vec![0; 32] }, | |
| Specific(_slot, hash) => Bytes { bytes: hash.to_vec() }, | |
| }, | |
| header: Bytes { | |
| bytes: to_cbor(&header), | |
| }, | |
| }, | |
| ), | |
| DecodedChainSyncEvent::Rollback { | |
| peer, | |
| rollback_point, | |
| .. | |
| } => ( | |
| peer, | |
| ChainSyncMessage::Bck { | |
| msg_id, | |
| slot: rollback_point.slot_or_default(), | |
| hash: match rollback_point { | |
| Origin => Bytes { bytes: vec![0; 32] }, | |
| Specific(_slot, hash) => Bytes { bytes: hash.to_vec() }, | |
| }, |
🤖 Prompt for AI Agents
In simulation/amaru-sim/src/simulator/mod.rs around lines 250 to 271, the code
incorrectly passes a Hash<32> type directly into the bytes field of Bytes, which
expects a Vec<u8>. To fix this, convert the Hash<32> value to a Vec<u8> before
assigning it to the bytes field in both the Fwd and Bck message constructions,
ensuring the type matches and the compiler error is resolved.
2 participants
This PR adds a first version of the simulator actually testing (parts of) the consensus pipeline. The property that's checked is that the last message sent from the SUT is the forward to the last block of the pre-generated block tree.
Summary by CodeRabbit
New Features
Improvements
Bug Fixes
Documentation
Style
Chores