feat: serve chain from store to followers#537
Conversation
WalkthroughThis pull request introduces a new Changes
Sequence Diagram(s)sequenceDiagram
participant Client as Downstream Client
participant Proto as client_protocol
participant Follower as ChainFollower
participant Store as ReadOnlyChainStore
Client->>Proto: Send intersection points
activate Proto
rect rgb(220, 250, 220)
Note over Follower,Store: Initialization Phase
Proto->>Follower: new(store, current_tip, points)
activate Follower
Follower->>Store: Load headers from current_tip backward
Follower-->>Proto: ChainFollower ready
deactivate Follower
end
rect rgb(220, 240, 255)
Note over Follower,Store: Forward Chain Traversal
loop Until intersection found or chain exhausted
Proto->>Follower: next_op(store)
activate Follower
alt Behind anchor
Follower->>Store: next_best_chain(current_point)
Store-->>Follower: Option<Point>
Follower->>Store: Load child header at Point
Follower-->>Proto: ClientOp::Forward(header)
else Ahead or at anchor
Follower-->>Proto: ClientOp from ops queue
end
deactivate Follower
Proto->>Client: Send header or intersection
end
end
Client->>Proto: Acknowledge / Request next
Proto-->>Client: Continue or sync complete
deactivate Proto
Estimated code review effort🎯 4 (Complex) | ⏱️ ~65 minutes Areas requiring extra attention:
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
📜 Recent review detailsConfiguration used: CodeRabbit UI Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🧰 Additional context used🧠 Learnings (15)📓 Common learnings📚 Learning: 2025-02-03T11:15:22.640ZApplied to files:
📚 Learning: 2025-05-21T18:58:48.631ZApplied to files:
📚 Learning: 2025-08-20T13:02:25.763ZApplied to files:
📚 Learning: 2025-06-24T06:36:05.931ZApplied to files:
📚 Learning: 2025-04-20T17:56:48.565ZApplied to files:
📚 Learning: 2025-04-22T09:18:19.893ZApplied to files:
📚 Learning: 2025-08-23T15:44:44.318ZApplied to files:
📚 Learning: 2025-08-08T14:39:50.527ZApplied to files:
📚 Learning: 2025-04-20T17:57:23.233ZApplied to files:
📚 Learning: 2025-08-08T14:43:20.218ZApplied to files:
📚 Learning: 2025-09-06T09:16:25.025ZApplied to files:
📚 Learning: 2025-09-29T16:39:24.001ZApplied to files:
📚 Learning: 2025-04-20T18:02:25.073ZApplied to files:
📚 Learning: 2025-08-12T12:28:24.027ZApplied to files:
🧬 Code graph analysis (1)crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (5)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/amaru/Cargo.toml (1)
75-79: Remove the unnecessary build-dependency on amaru-kernel.Good news and bad news, mate. The dev-dependency with
test-utils? That's totally legit – keep it.But here's the thing: I've had a proper squiz at your
build.rs, and it only uses thebuiltcrate to write build-time info and the standard lib to set up theAMARU_NETWORKenv var. There's zero usage ofamaru-kernelanywhere in that build script – not even a sneaky import hiding in the background like a bonus scene in an Avengers flick.That build-dependency entry on line 79 is pure cruft and should be binned. It's like carrying a toolbox when you only needed a hammer.
amaru-kernel = { workspace = true, features = ["test-utils"] } [build-dependencies] built = { workspace = true, features = ["git2"] }Just drop the
amaru-kernel.workspace = trueline from build-dependencies and you're golden.
🧹 Nitpick comments (1)
crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
129-146: Logic's spot on, but there's a cheeky optimization opportunity here, mate!The core logic is absolutely sound – you're finding the next point on the best chain with a slot greater than the input point. That'll work a treat for the downstream sync use case.
That said, the implementation's doing a bit of unnecessary gymnastics with that intermediate
Veccollection. You're collecting into a Vec just to check if it's empty, when the iterator could give you the answer directly. It's like making a sandwich, putting it in a box, then immediately taking it out to eat it – the box wasn't really needed, yeah?Here's a cleaner approach that skips the intermediate collection:
fn next_best_chain(&self, point: &Point) -> Option<Point> { let inner = self.inner.lock().unwrap(); let min_slot = point.slot_or_default(); - - let next: Vec<&Point> = inner - .chain - .iter() - .filter(move |p| p.slot_or_default() > min_slot) - .take(1) - .collect(); - - if next.is_empty() { - None - } else { - Some(next[0].clone()) - } + inner + .chain + .iter() + .find(|p| p.slot_or_default() > min_slot) + .cloned() }Using
.find()is more idiomatic and avoids the allocation – it returnsOption<&Point>directly, then.cloned()gives you the ownedPoint. Reads cleaner too!
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
crates/amaru-consensus/src/consensus/effects/store_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/validate_block.rs(3 hunks)crates/amaru-consensus/src/consensus/stages/validate_header.rs(1 hunks)crates/amaru-kernel/src/is_header/tests.rs(1 hunks)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs(1 hunks)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs(2 hunks)crates/amaru-stores/src/rocksdb/consensus/mod.rs(2 hunks)crates/amaru/Cargo.toml(1 hunks)crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs(3 hunks)crates/amaru/src/stages/consensus/forward_chain/client_state.rs(0 hunks)crates/amaru/src/stages/consensus/forward_chain/mod.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain/tests.rs(1 hunks)crates/amaru/src/stages/mod.rs(3 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru/src/stages/consensus/forward_chain/client_state.rs
🧰 Additional context used
🧠 Learnings (16)
📓 Common learnings
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.
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.
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.
📚 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/validate_header.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.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-consensus/src/consensus/stages/validate_header.rscrates/amaru-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.rscrates/amaru-ouroboros-traits/src/stores/consensus/mod.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/validate_header.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.rscrates/amaru-consensus/src/consensus/stages/validate_block.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-consensus/src/consensus/stages/validate_header.rscrates/amaru-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.rscrates/amaru/src/stages/consensus/forward_chain/tests.rscrates/amaru-consensus/src/consensus/effects/store_effects.rscrates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rscrates/amaru/src/stages/consensus/forward_chain/client_protocol.rscrates/amaru/src/stages/mod.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.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-consensus/src/consensus/stages/validate_header.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 the amaru consensus pipeline, headers are now stored before validation (store_header stage runs before validate_header stage). The validity state of headers should be tracked in the database to avoid revalidating already-valid headers and to prevent flood attacks with duplicate invalid headers.
Applied to files:
crates/amaru-consensus/src/consensus/stages/validate_header.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.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-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/consensus/forward_chain/mod.rscrates/amaru/src/stages/consensus/forward_chain/client_protocol.rscrates/amaru/src/stages/mod.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.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-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/consensus/forward_chain/tests.rscrates/amaru/src/stages/mod.rscrates/amaru-kernel/src/is_header/tests.rscrates/amaru/Cargo.toml
📚 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/src/stages/consensus/forward_chain/test_infra.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.rscrates/amaru-kernel/src/is_header/tests.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/src/stages/consensus/forward_chain/tests.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-consensus/src/consensus/effects/store_effects.rscrates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.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-consensus/src/consensus/effects/store_effects.rscrates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rscrates/amaru/src/stages/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-consensus/src/consensus/effects/store_effects.rscrates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs
📚 Learning: 2025-04-20T17:56:48.565Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/mod.rs:0-0
Timestamp: 2025-04-20T17:56:48.565Z
Learning: When bootstrapping a node in Amaru, it's important to handle the case where the tip is Origin (for a fresh node). Instead of unconditionally trying to load a header from the chain store, check if the tip is Origin or Specific first, and handle each case appropriately.
Applied to files:
crates/amaru/src/stages/mod.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.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-kernel/src/is_header/tests.rs
🧬 Code graph analysis (10)
crates/amaru-consensus/src/consensus/stages/validate_header.rs (3)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (1)
next_best_chain(75-77)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
next_best_chain(130-146)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
next_best_chain(40-40)next_best_chain(145-147)
crates/amaru-stores/src/rocksdb/consensus/mod.rs (6)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (1)
next_best_chain(75-77)crates/amaru-consensus/src/consensus/stages/validate_header.rs (1)
next_best_chain(287-289)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
next_best_chain(130-146)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
next_best_chain(40-40)next_best_chain(145-147)crates/amaru-kernel/src/point.rs (3)
from(65-67)slot_or_default(33-38)hash(40-46)crates/amaru-stores/src/rocksdb/mod.rs (3)
from(157-159)from(163-169)iter(986-1023)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (4)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (4)
store(27-27)store(50-52)store(72-74)store(124-126)crates/amaru-kernel/src/point.rs (1)
hash(40-46)crates/amaru-consensus/src/consensus/tip.rs (2)
hash(94-99)hash(165-167)crates/amaru-kernel/src/is_header/mod.rs (5)
hash(35-35)hash(123-125)hash(185-187)hash(207-209)header(136-138)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (3)
crates/amaru-consensus/src/consensus/stages/validate_header.rs (1)
next_best_chain(287-289)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
next_best_chain(130-146)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
next_best_chain(40-40)next_best_chain(145-147)
crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (3)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (1)
next_best_chain(75-77)crates/amaru-consensus/src/consensus/stages/validate_header.rs (1)
next_best_chain(287-289)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
next_best_chain(40-40)next_best_chain(145-147)
crates/amaru-consensus/src/consensus/stages/validate_block.rs (5)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
point(87-89)crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
point(118-123)crates/amaru-kernel/src/consensus_events.rs (1)
point(115-120)crates/amaru-consensus/src/consensus/tip.rs (2)
point(101-106)point(169-171)crates/amaru-kernel/src/is_header/mod.rs (1)
point(38-40)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (1)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (1)
new(66-146)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (5)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (2)
hash_point(124-129)tip(99-104)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (5)
point(87-89)new(102-127)hash(79-81)mk_in_memory_store(43-77)hex(83-85)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
load_from_best_chain(36-36)load_from_best_chain(141-143)crates/amaru/src/stages/mod.rs (3)
from(154-159)as_tip(382-382)as_tip(386-388)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
load_from_best_chain(124-127)
crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (4)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (1)
next_best_chain(75-77)crates/amaru-consensus/src/consensus/stages/validate_header.rs (1)
next_best_chain(287-289)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
next_best_chain(130-146)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
point(87-89)
crates/amaru-kernel/src/is_header/tests.rs (1)
crates/amaru-kernel/src/is_header/mod.rs (1)
header(136-138)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Build x86_64/windows
- GitHub Check: Test the CLI
- GitHub Check: Benches
- GitHub Check: Build aarch64/macos
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Build wasm32
- GitHub Check: Build riscv32
- GitHub Check: Build x86_64/linux
- GitHub Check: Build aarch64/linux
- GitHub Check: Test coverage
- GitHub Check: clippy
- GitHub Check: Analyze (rust)
🔇 Additional comments (17)
crates/amaru-kernel/src/is_header/tests.rs (1)
64-67: Fair dinkum, this change makes total sense!Starting the chain numbering at 1 rather than 0 aligns with blockchain conventions where there's no block 0. The explanatory comments are ripper – they make the intent crystal clear for anyone reading this test utility code later. It's like making sure everyone knows the rules before the game starts, ya know?
crates/amaru/src/stages/consensus/forward_chain/tests.rs (1)
15-15: Clean as a whistle, mate!The import path update reflects the architectural refactor from
client_statetochain_follower. No worries here – just keeping the imports aligned with the new module structure. Easy as!crates/amaru-consensus/src/consensus/stages/validate_block.rs (1)
63-95: Bloody brilliant observability improvement!Adding the point context to these error logs is like giving future you (or whoever's on-call) a proper map instead of just saying "something went wrong somewhere." When you're troubleshooting memory spikes or sync stalls with downstream clients (which is what this PR tackles), having that point information in the logs is gonna be absolutely clutch. It's the difference between "we failed somewhere" and "we failed at this exact point" – proper detective work, that.
crates/amaru/src/stages/consensus/forward_chain/mod.rs (1)
15-15: Module refactor looking proper!Swapping out
client_stateforchain_followermakes sense given the PR's introducing a dedicatedChainFollowerstructure to manage downstream peer sync state. It's like reorganizing your garage so the tools are actually where you'd expect them, yeah? Keeps the architecture clean and intentional.crates/amaru-consensus/src/consensus/stages/validate_header.rs (1)
287-289: Delegation pattern executed perfectly, champion!The
next_best_chainimplementation forFailingStorefollows the same delegation pattern as the other methods in this test helper – just forwards the call to the inner store. Clean as, no worries at all. It's like theFailingStoreis saying "I'm not here to muck about with next_best_chain logic, I'll let the real store handle that."crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (2)
41-41: FIRST_HEADER constant looking lonely here, mate!You've added this
FIRST_HEADERconstant but it's not actually used anywhere in this file. That's totally fine though – I'm guessing it's being used in other test files that import from this test_infra module. Just wanted to give it a shout-out since constants that aren't used locally sometimes make people go "oi, what's this doing here?"
67-75: The review comment is based on a misunderstanding of how the code actually works—it's not incorrect, mate.Here's the rub:
retrieve_best_chain()doesn't depend onroll_forward_chainbeing called first. They're operating on two completely different levels:
retrieve_best_chain()walks the stored persistent header data using the anchor and best_chain_hash pointers to navigate parent-child relationshipsroll_forward_chain()rebuilds the in-memory consensus state from those stored headersIt's like the difference between reading from a database and populating your application cache—no chicken-and-egg situation, just a sensible two-step bootstrap:
- Store headers + set anchors/pointers in persistent storage (lines 54-59)
- Walk the stored headers and replay them to rebuild in-memory state (lines 62-66)
The anchor is already set to the first header at line 54-55 before
retrieve_best_chain()is called, so the function has everything it needs from day one.Likely an incorrect or invalid review comment.
crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
38-40: Clean trait extension, mate!The new
next_best_chainmethod is well-documented and the signature makes sense. Returns the successor point on the best chain, or None if we've hit the tip or the point doesn't exist. Simple as.
145-147: Standard forwarding pattern, no dramas.The Box<dyn ChainStore> delegation is exactly what you'd expect - forwards straight through to the underlying implementation. All good here.
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (4)
17-17: Module import updated to the new chain_follower structure.Straightforward refactor moving ChainFollower to its own dedicated module. Clean separation of concerns - like moving from a sharehouser situation to your own flat, yeah?
190-190: Debug logging adjustment looks spot on.Changed from
our_tip.0to justour_tipwhich suggests the structure changed. Debug formatting will now show the whole Tip rather than just the Point. More context in the logs is always a win!
194-207: ChainFollower initialization with store - design tradeoff acknowledged.The ChainFollower::new now takes
store.clone()and later calls tonext_opalso pass the store. I see from your PR description you're aware of the tradeoff here - ideally you'd keep an iterator in ChainFollower, butArc<T>not being Send is blocking that approach.Just for clarity: Arc::clone is cheap (just bumps a reference count), so the performance hit should be minimal. The real question is whether passing the store on every
next_opcall (lines 223, 231) accumulates enough overhead when serving many downstream peers.Worth measuring if you can - spin up a downstream client that's way behind and watch the memory/CPU patterns when it catches up. If you're seeing issues with the Arc cloning frequency, might be worth exploring workarounds like:
- Store an opaque iterator handle (if you can work around Send)
- Cache recent chain points in ChainFollower to reduce store hits
But honestly, this looks like a solid pragmatic solution for now!
201-206: Explicit intersection handling - nice and clear!Rather than inlining the intersection logic, you've extracted it into a variable and logged it separately. Makes the flow much easier to follow when debugging chain sync issues. Good stuff!
crates/amaru-consensus/src/consensus/effects/store_effects.rs (1)
75-77: Placeholder implementation consistent with the pattern here.Returns None, same as
load_from_best_chainabove it (line 71-73). Since Store wraps the effects system and there's no ExternalEffect defined for next_best_chain, this is just a no-op stub. The real implementations live in RocksDBStore and InMemConsensusStore. All good, mate!crates/amaru-stores/src/rocksdb/consensus/mod.rs (1)
751-796: Test coverage is absolutely mint!You've covered all the key scenarios:
- Finding a valid successor (752-762)
- Handling Origin point (764-775)
- Gracefully returning None for invalid points (777-785)
- Returning None when already at tip (787-796)
This gives me confidence the implementation handles edge cases properly. Nice one!
crates/amaru/src/stages/mod.rs (2)
188-192: Bootstrap flow refactor - clean separation of concerns!The new flow is much clearer:
make_chain_storereturns the store directly instead of mutating a passed-in parameter. Then you load the header and build the tip. The fallback toHeaderTip::new(Point::Origin, 0)handles fresh nodes nicely. Good stuff!
272-296: Store creation and validation - thorough and clear.The refactored
make_chain_storeis solid:
- Creates the appropriate store (in-memory or RocksDB) based on config
- Validates the tip exists in the store (unless it's ORIGIN - nice handling of fresh nodes there, matches the learning about Origin handling during bootstrap)
- Sets anchor and best chain hash to the tip
- Returns the store
The panic message at lines 287-290 is actually helpful - tells the user exactly what went wrong and which database path is involved. Much better than a cryptic "unwrap on None" panic.
One tiny thing to consider: the panic could theoretically be avoided by returning a Result and letting the caller decide how to handle the missing tip. But given this is bootstrap code and a missing tip in an existing database is a critical misconfiguration that should stop the node, the panic is reasonable here. Your call whether you want to make it a proper error type instead.
| fn next_best_chain(&self, point: &Point) -> Option<Point> { | ||
| let readopts = ReadOptions::default(); | ||
| let prefix = [&CHAIN_PREFIX[..], &(u64::from(point.slot_or_default()) + 1).to_be_bytes()].concat(); | ||
| let mut iter = self.db.iterator_opt(IteratorMode::From(&prefix, rocksdb::Direction::Forward), readopts); | ||
|
|
||
| if let Some(Ok((k, v))) = iter.next() { | ||
| let slot_bytes = &k[CHAIN_PREFIX.len()..CHAIN_PREFIX.len() + 8]; | ||
| let slot = u64::from_be_bytes(slot_bytes.try_into().unwrap()); | ||
| if v.len() == HEADER_HASH_SIZE { | ||
| let hash = HeaderHash::from(v.as_ref()); | ||
| Some(Point::Specific(slot, hash.to_vec())) | ||
| } else { | ||
| None | ||
| } | ||
| } else { | ||
| None | ||
| } | ||
| } |
There was a problem hiding this comment.
Implementation looks solid, but there's a potential panic lurking.
The logic is straightforward - start iterating from slot+1, grab the first result, parse it, done. However, line 260 has an .unwrap() that could panic if the key slice isn't exactly 8 bytes:
let slot = u64::from_be_bytes(slot_bytes.try_into().unwrap());If the database is corrupted or the key format somehow changes, this'll bring the whole node down faster than you can say "crikey".
Consider replacing with:
- let slot = u64::from_be_bytes(slot_bytes.try_into().unwrap());
+ let slot_array: [u8; 8] = match slot_bytes.try_into() {
+ Ok(arr) => arr,
+ Err(_) => {
+ tracing::error!("Invalid slot bytes length in chain store");
+ return None;
+ }
+ };
+ let slot = u64::from_be_bytes(slot_array);Or at minimum use .expect() with a descriptive message so you know what went wrong when investigating the crash.
Also, minor observation: the implementation relies on the iterator's prefix filtering to ensure keys start with CHAIN_PREFIX. This is fine, but if you're being defensive you could add an explicit check - though the iterator prefix mode should handle this already.
etorreborre
left a comment
etorreborre
left a comment
There was a problem hiding this comment.
⛓️ I would be good to be able to test that logic in the simulation and see, for example, that the nodes are converging on the same chain, even they start with different states.
| .take(1) | ||
| .collect(); | ||
|
|
||
| if next.is_empty() { |
| }) | ||
| } | ||
|
|
||
| pub fn next_op(&mut self, store: Arc<dyn ReadOnlyChainStore<H>>) -> Option<ClientOp<H>> { |
There was a problem hiding this comment.
Why pass a store reference again here since we already have self.store?
There was a problem hiding this comment.
I don't think we have it in self, or at least I removed it! I need to pass it because Arc<dyn ChainStore> is not Send and therefore I cannot store it in self, that's the solution I found: pass it to next_op
There was a problem hiding this comment.
I tried it and that works fine. See my commit in abailly/follow-chain-from-store-eric.
abailly
force-pushed
the
abailly/follow-chain-from-store
branch
from
92f92f4 to
ea28195
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
crates/amaru-stores/src/rocksdb/consensus/mod.rs (1)
253-270: Handle slot overflow and dodgy key bytes safelyLine 255 is doing a raw
+ 1on the slot; hitu64::MAXand we wrap (release) or panic (debug), which is the opposite of “resilient followers”. Then Line 260 slices andunwrap()s without checking the key prefix/length, so a single bit-flip in RocksDB can drop the whole node faster than Ganondorf grabbing the Master Sword. Let’s fail soft instead: guard the increment withchecked_add, make sure the key really starts withCHAIN_PREFIX, and only turn the bytes into a slot when the slice converts cleanly (ditch theunwrap).- let prefix = [&CHAIN_PREFIX[..], &(u64::from(point.slot_or_default()) + 1).to_be_bytes()].concat(); + let Some(start_slot) = u64::from(point.slot_or_default()).checked_add(1) else { + return None; + }; + let prefix = [&CHAIN_PREFIX[..], &start_slot.to_be_bytes()].concat(); let mut iter = self.db.iterator_opt(IteratorMode::From(&prefix, rocksdb::Direction::Forward), readopts); if let Some(Ok((k, v))) = iter.next() { - let slot_bytes = &k[CHAIN_PREFIX.len()..CHAIN_PREFIX.len() + 8]; - let slot = u64::from_be_bytes(slot_bytes.try_into().unwrap()); + if !k.starts_with(&CHAIN_PREFIX[..]) { + return None; + } + let slot_bytes: [u8; 8] = match k[CHAIN_PREFIX.len()..].get(..8).and_then(|slice| slice.try_into().ok()) { + Some(bytes) => bytes, + None => { + tracing::warn!("invalid chain slot key length in RocksDB"); + return None; + } + }; + let slot = u64::from_be_bytes(slot_bytes);crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (2)
185-186: Mate, you're ghosting the intersection update when draining the ops queue!So here's the deal: when you
pop_front()an op at line 185, you're sending it downstream but leavingself.intersectionfrozen in time—it's still pointing at the old value (usually the anchor or initial intersection). Next timenext_opgets called, it's making decisions with yesterday's news, and anyone callingintersection_found()is gonna see a cursor that's stuck like a scratched DVD.Think of it like this: you're sending headers downstream but not updating your own map. It's like playing Civilization and forgetting to update the fog of war—total chaos!
Before you return the op, you need to update
self.intersectionto match what you're sending. Grab the tip from the op and advance the cursor:- self.ops.pop_front() + if let Some(op) = self.ops.pop_front() { + self.intersection = op.tip(); + Some(op) + } else { + None + }The
ClientOpalready has a.tip()method (see client_protocol.rs lines 98-103 in the snippets), so just use that to keep your state consistent with what you've sent, yeah?
108-134: Crikey! The tip header's gone walkabout – it never makes it into the ops queue.Right, so here's the craic: your backward walk only collects parent headers (line 126), but the
start_headeritself—the actual tip—never gets queued. When the intersection sits at or near the anchor, you'll break early with an empty or partialheadersvec, and the client ends up like Marty McFly stuck in 1955—never quite catching up to the present.Picture this: intersection == anchor. The while loop bails immediately,
headersis empty, client gets a rollback to anchor, maybe one forward foranchor+1, but the current tip? Ghost. Vanished. Gone like Keyser Söze.To fix it, you need to enqueue the
start_headerbefore you start the walk, or append it after reversing. Something like:let mut current_header = start_header; let mut headers = vec![]; + +// Include the starting header (the tip) in the ops +headers.push(ClientOp::Forward(current_header.clone())); // walk backwards until either: // 1. we find our intersection // 2. or we find the anchor while let Some(parent_hash) = current_header.parent() {or after the reverse:
// headers contains a list of Fwd operations in reverse order headers.reverse(); +// Ensure the tip (start_header) is included +headers.push(ClientOp::Forward(start_header));Either way, make sure every block above the intersection gets sent downstream, mate. No one likes an incomplete sync!
🧹 Nitpick comments (1)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (1)
253-253: Fair dinkum, you could tidy up these test assertions with.as_tip()for consistency.In your test assertions at lines 253, 297, and 315, you're manually constructing
Tipvalues with hardcoded heights or points. While this works fine, it's a bit like hand-rolling your own crypto—you can do it, but why not use the library?Consider loading the header and calling
.as_tip()instead. It's more consistent with the codebase (see relevant snippets from client_protocol.rs), and you won't risk typos in the height values. Something like:// Line 253 let start_header = store.load_header(&hash_point(&tip)).unwrap(); assert_eq!( chain_follower.next_op(store.clone()), Some(ClientOp::Backward(start_header.as_tip())) );// Line 297 let expected_header = store.load_header(&Hash::from(hex::decode(WINNER_47).unwrap().as_slice())).unwrap(); assert_eq!( chain_follower.next_op(store.clone()), Some(ClientOp::Backward(expected_header.as_tip())) );For line 315,
Tip(Point::Origin, 0)is already pretty clear, so maybe leave that one as-is unless you've got a canonical Origin tip somewhere.Not a blocker at all, just a bit of polish to keep things idiomatic. Based on past review comments suggesting
.as_tip()in similar spots.Also applies to: 297-297, 315-315
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
crates/amaru-consensus/src/consensus/effects/store_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/validate_block.rs(3 hunks)crates/amaru-consensus/src/consensus/stages/validate_header.rs(1 hunks)crates/amaru-kernel/src/is_header/tests.rs(1 hunks)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs(1 hunks)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs(2 hunks)crates/amaru-stores/src/rocksdb/consensus/mod.rs(2 hunks)crates/amaru/Cargo.toml(1 hunks)crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs(3 hunks)crates/amaru/src/stages/consensus/forward_chain/client_state.rs(0 hunks)crates/amaru/src/stages/consensus/forward_chain/mod.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain/tests.rs(1 hunks)crates/amaru/src/stages/mod.rs(3 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru/src/stages/consensus/forward_chain/client_state.rs
🚧 Files skipped from review as they are similar to previous changes (5)
- crates/amaru-consensus/src/consensus/stages/validate_block.rs
- crates/amaru-consensus/src/consensus/effects/store_effects.rs
- crates/amaru-consensus/src/consensus/stages/validate_header.rs
- crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs
- crates/amaru/src/stages/consensus/forward_chain/tests.rs
🧰 Additional context used
🧠 Learnings (19)
📓 Common learnings
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.
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.
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.
📚 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/mod.rscrates/amaru/src/stages/consensus/forward_chain/client_protocol.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.rscrates/amaru-stores/src/rocksdb/consensus/mod.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/src/stages/consensus/forward_chain/test_infra.rscrates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rscrates/amaru/src/stages/mod.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.rscrates/amaru-stores/src/rocksdb/consensus/mod.rs
📚 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/src/stages/consensus/forward_chain/test_infra.rscrates/amaru-kernel/src/is_header/tests.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.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/src/stages/consensus/forward_chain/test_infra.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.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/src/stages/consensus/forward_chain/test_infra.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.rscrates/amaru-stores/src/rocksdb/consensus/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/src/stages/consensus/forward_chain/test_infra.rscrates/amaru-stores/src/rocksdb/consensus/mod.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/src/stages/consensus/forward_chain/test_infra.rscrates/amaru-stores/src/rocksdb/consensus/mod.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:
crates/amaru/src/stages/consensus/forward_chain/test_infra.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-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rscrates/amaru-stores/src/rocksdb/consensus/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-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rscrates/amaru-stores/src/rocksdb/consensus/mod.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-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rscrates/amaru/src/stages/mod.rscrates/amaru-stores/src/rocksdb/consensus/mod.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-kernel/src/is_header/tests.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-kernel/src/is_header/tests.rscrates/amaru/Cargo.tomlcrates/amaru-stores/src/rocksdb/consensus/mod.rs
📚 Learning: 2025-04-20T17:56:48.565Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/mod.rs:0-0
Timestamp: 2025-04-20T17:56:48.565Z
Learning: When bootstrapping a node in Amaru, it's important to handle the case where the tip is Origin (for a fresh node). Instead of unconditionally trying to load a header from the chain store, check if the tip is Origin or Specific first, and handle each case appropriately.
Applied to files:
crates/amaru/src/stages/mod.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.rs
📚 Learning: 2025-09-06T09:16:25.025Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 435
File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116
Timestamp: 2025-09-06T09:16:25.025Z
Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.
Applied to files:
crates/amaru-stores/src/rocksdb/consensus/mod.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-stores/src/rocksdb/consensus/mod.rs
📚 Learning: 2025-01-21T15:32:17.911Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.
Applied to files:
crates/amaru-stores/src/rocksdb/consensus/mod.rs
📚 Learning: 2025-08-08T14:39:50.527Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/borrowed_datum.rs:32-39
Timestamp: 2025-08-08T14:39:50.527Z
Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.
Applied to files:
crates/amaru-stores/src/rocksdb/consensus/mod.rs
🧬 Code graph analysis (6)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (5)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (4)
store(27-27)store(50-52)store(72-74)store(124-126)crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
hash(70-75)crates/amaru-kernel/src/point.rs (1)
hash(40-46)crates/amaru-consensus/src/consensus/tip.rs (2)
hash(94-99)hash(165-167)crates/amaru-kernel/src/is_header/mod.rs (5)
hash(35-35)hash(123-125)hash(185-187)hash(207-209)header(136-138)
crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (3)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (1)
next_best_chain(75-77)crates/amaru-consensus/src/consensus/stages/validate_header.rs (1)
next_best_chain(287-289)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
next_best_chain(40-40)next_best_chain(145-147)
crates/amaru-kernel/src/is_header/tests.rs (1)
crates/amaru-kernel/src/is_header/mod.rs (1)
header(136-138)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (1)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (1)
new(66-146)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (5)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (4)
hash_point(124-129)fmt(71-95)fmt(114-121)tip(99-104)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (6)
point(87-89)fmt(226-245)new(102-127)hash(79-81)mk_in_memory_store(43-77)hex(83-85)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (3)
fmt(181-193)load_from_best_chain(36-36)load_from_best_chain(141-143)crates/amaru/src/stages/mod.rs (5)
fmt(80-85)fmt(129-134)from(154-159)as_tip(382-382)as_tip(386-388)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
load_from_best_chain(124-127)
crates/amaru-stores/src/rocksdb/consensus/mod.rs (5)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (1)
next_best_chain(75-77)crates/amaru-consensus/src/consensus/stages/validate_header.rs (1)
next_best_chain(287-289)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
next_best_chain(130-146)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
next_best_chain(40-40)next_best_chain(145-147)crates/amaru-kernel/src/point.rs (3)
from(65-67)slot_or_default(33-38)hash(40-46)
⏰ 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). (10)
- GitHub Check: Test the CLI
- GitHub Check: Benches
- GitHub Check: Build x86_64/windows
- GitHub Check: Build riscv32
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/linux
- GitHub Check: Build aarch64/linux
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
crates/amaru/Cargo.toml (2)
74-74: Confirm the purpose of addingtest-utilsfeature for dev-dependency.The addition makes sense for the test scaffolding mentioned in the PR context—your new ChainFollower and next_best_chain functionality likely need test utilities. However, check that the
test-utilsfeature is indeed what you need (rather than something else from amaru-kernel).
78-78: Clarify why the build script needsamaru-kernel.You're adding amaru-kernel to
[build-dependencies]with no feature specification. This is a bit like bringing a full toolkit to the construction site when you've already got your hammer—what's the build.rs actually using from amaru-kernel? Is this for generating code, or is there something else going on?crates/amaru-kernel/src/is_header/tests.rs (1)
64-67: Solid as a rock, mate – this one's good to go.Dead right on the numbering convention. I've dug through the actual test code where
any_headers_chaingets used – across the rocksdb consensus tests and the select_chain stages – and there's bugger all hardcoded assertions checking for block 0 or slot 0. The tests are all structural checks: they're validating parent-child hash linkages, storing/retrieving headers, and checking the chain relationships. They're not relying on specific numeric values.The beauty here is you've kept the parent hash logic ('prev_hash') untouched, so all that chain-linking mojo still works exactly the same. It's like swapping the house numbers on a street – the roads and connections stay put, just the addresses change. And since those addresses now match blockchain semantics (no genesis block 0 weirdness), everyone's happy.
The +1 offset is consistent across both
block_numberandslot, which is clean. Comments are crystal clear too – future devs won't scratch their heads wondering why you did the +1 dance.crates/amaru/src/stages/mod.rs (1)
286-295: Love the early sanity checkDigging this guard—bailing out if the ledger tip isn’t actually in the store is like checking the Mako’s fuel before cruising across Illium. Saves us from some gnarly half-synced states later on.
There was a problem hiding this comment.
Actionable comments posted: 3
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/multi-client.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
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.
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.
🪛 Shellcheck (0.11.0)
scripts/multi-client.sh
[warning] 14-14: Quote this to prevent word splitting.
(SC2046)
⏰ 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). (10)
- GitHub Check: Build riscv32
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/linux
- GitHub Check: Benches
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Test the CLI
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/linux
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
scripts/multi-client.sh (1)
15-15: Clarify the intent of the 60-second sleep—it's a long pause between launches.Is this to give each node time to bootstrap and stabilize before the next one starts? If so, that'll add significant latency for testing (10 minutes for 10 nodes, innit?). If it's intentional, a comment explaining why would help future maintainers. If not, consider making it configurable or reducing it based on empirical testing.
| NUM_NODES=$1 | ||
|
|
There was a problem hiding this comment.
Add argument validation—the script will fail cryptically if NUM_NODES is missing or bogus.
Right now, if someone forgets to pass NUM_NODES or passes something that's not a number, the script either runs forever or exits with an unhelpful error from the seq command. Like trying to find an NPC in an open-world game without a quest marker, y'know? Add a quick validation check at the top.
NUM_NODES=$1
+
+if [[ -z "$NUM_NODES" ]] || ! [[ "$NUM_NODES" =~ ^[0-9]+$ ]]; then
+ echo "Error: NUM_NODES must be a positive integer" >&2
+ exit 1
+fi📝 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.
| NUM_NODES=$1 | |
| NUM_NODES=$1 | |
| if [[ -z "$NUM_NODES" ]] || ! [[ "$NUM_NODES" =~ ^[0-9]+$ ]]; then | |
| echo "Error: NUM_NODES must be a positive integer" >&2 | |
| exit 1 | |
| fi |
🤖 Prompt for AI Agents
In scripts/multi-client.sh around lines 7 to 8, NUM_NODES is taken directly from
$1 with no validation causing cryptic failures; add a short validation block
that (1) checks that $1 is present, (2) ensures it is a positive integer (e.g.
regex or POSIX test), and (3) prints a clear usage message like "Usage:
multi-client.sh NUM_NODES (positive integer)" and exits with a non-zero status
if the check fails; keep the check near the top before any use of NUM_NODES so
subsequent seq/loops only run with a valid value.
| amaru bootstrap --ledger-dir "${NODE_ID}/ledger.db" --chain-dir "${NODE_ID}/chain.db" --network preview | ||
| amaru --service-name $NODE_ID --with-open-telemetry daemon --peer-address localhost:3000 --listen-address localhost:$(( 4000 + $i)) --ledger-dir "${NODE_ID}/ledger.db" --chain-dir "${NODE_ID}/chain.db" --network preview > $NODE_ID.log 2>&1 & |
There was a problem hiding this comment.
No error handling if bootstrap fails—the entire script exits with set -e.
If bootstrap fails on node 2 of 5, the whole orchestration stops, leaving nodes 1 running orphaned. That's a rough state to debug. Consider wrapping the bootstrap in a conditional or at minimum logging the failure and continuing (depending on your UX goals).
Also, the daemon launch command on line 12 is a bit of a wall-of-text. Consider breaking it into variables for readability:
NODE_ID=amaru-$i
+ NODE_LOG=$NODE_ID.log
+ NODE_PORT=$((4000 + i))
+
amaru bootstrap --ledger-dir "${NODE_ID}/ledger.db" --chain-dir "${NODE_ID}/chain.db" --network preview
- amaru --service-name $NODE_ID --with-open-telemetry daemon --peer-address localhost:3000 --listen-address localhost:$(( 4000 + $i)) --ledger-dir "${NODE_ID}/ledger.db" --chain-dir "${NODE_ID}/chain.db" --network preview > $NODE_ID.log 2>&1 &
+ amaru --service-name "$NODE_ID" \
+ --with-open-telemetry daemon \
+ --peer-address localhost:3000 \
+ --listen-address "localhost:${NODE_PORT}" \
+ --ledger-dir "${NODE_ID}/ledger.db" \
+ --chain-dir "${NODE_ID}/chain.db" \
+ --network preview \
+ > "$NODE_LOG" 2>&1 &🤖 Prompt for AI Agents
In scripts/multi-client.sh around lines 11 to 12, the bootstrap step is
currently run under set -e so a single bootstrap failure will terminate the
entire script and leave already-started nodes orphaned; modify the logic to run
the bootstrap command in a conditional (capture its exit code), log a clear
error message (with node id and exit code) if it fails, and decide whether to
continue or retry based on that code (e.g., continue to next node after
logging). Also simplify the long daemon invocation by extracting its options
into descriptive variables (e.g., PEER_ADDR, LISTEN_ADDR, LEDGER_DIR, CHAIN_DIR,
LOG_FILE) and then using those variables in the amaru daemon command and
redirect, improving readability and making future error handling or retries
easier.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (2)
scripts/multi-client.sh (2)
16-17: Missing argument validation—script will crater if NUM_NODES or PEER_ADDRESS are missing or dodgy.Right now this reads like jumping into a boss fight without checking your weapon—you'll get rekt by cryptic errors from
seqor the daemon if someone forgets an argument or passes garbage. NUM_NODES should be validated as a positive integer, and PEER_ADDRESS should at least be checked for non-empty.Apply this validation block right after line 17:
NUM_NODES=$1 PEER_ADDRESS=$2 + +if [[ -z "$NUM_NODES" ]] || ! [[ "$NUM_NODES" =~ ^[0-9]+$ ]]; then + echo "Error: NUM_NODES must be a positive integer" >&2 + exit 1 +fi + +if [[ -z "$PEER_ADDRESS" ]]; then + echo "Error: PEER_ADDRESS cannot be empty" >&2 + exit 1 +fi
22-22: No error handling if bootstrap fails—entire script exits leaving nodes orphaned. Plus, daemon command is a wall-of-text.If bootstrap fails on node 2 of 5, the whole thing bails thanks to
set -e, leaving node 1 just... sitting there. That's a debugging nightmare. Also, the daemon invocation across lines 23–27 is hard to parse.Refactor with error handling and clearer variable breakdown:
NODE_ID=amaru-$i + NODE_LOG="${NODE_ID}.log" + NODE_PORT=$((4000 + i)) + echo "$(date -Iseconds) : bootstrapping $NODE_ID" - amaru bootstrap --ledger-dir "${NODE_ID}/ledger.db" --chain-dir "${NODE_ID}/chain.db" --network preview > "$NODE_ID.log" 2>&1 + if ! amaru bootstrap --ledger-dir "${NODE_ID}/ledger.db" --chain-dir "${NODE_ID}/chain.db" --network preview >> "$NODE_LOG" 2>&1; then + echo "$(date -Iseconds) : bootstrap failed for $NODE_ID, skipping" >&2 + continue + fi + - amaru daemon --peer-address "${PEER_ADDRESS}" \ - --listen-address localhost:$(( 4000 + i)) \ - --ledger-dir "${NODE_ID}/ledger.db" \ - --chain-dir "${NODE_ID}/chain.db" \ - --network preview >> "$NODE_ID.log" 2>&1 & + amaru daemon \ + --peer-address "${PEER_ADDRESS}" \ + --listen-address "localhost:${NODE_PORT}" \ + --ledger-dir "${NODE_ID}/ledger.db" \ + --chain-dir "${NODE_ID}/chain.db" \ + --network preview \ + >> "$NODE_LOG" 2>&1 &Also applies to: 23-27
🧹 Nitpick comments (2)
scripts/multi-client.sh (2)
20-20: Unquoted variable expansion for consistency.While it works here,
NODE_ID=amaru-$iis a bit inconsistent with the quoted variable usage elsewhere. Minor nit, but best practice isamaru-${i}to make it clear where the variable boundary is.- NODE_ID=amaru-$i + NODE_ID="amaru-${i}"
30-30: 60-second delay per node might be overkill—consider making it configurable.Starting 10 nodes would take 10 minutes just for staggered bootup. If that's intentional for load dampening, fair dinkum, but it might be worth either making it a parameter or reducing it. Quick question: is this delay meant to avoid thundering-herd bootstrap issues, or just testing throughput?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
scripts/multi-client.sh(1 hunks)
🧰 Additional context used
🧠 Learnings (1)
📓 Common learnings
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.
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.
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.
⏰ 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). (10)
- GitHub Check: Test the CLI
- GitHub Check: Benches
- GitHub Check: Build x86_64/windows
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Build aarch64/linux
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/linux
- GitHub Check: Build riscv32
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
scripts/multi-client.sh (1)
21-21: Good on ya—date variables are properly quoted now!The $(date -Iseconds) calls on lines 21 and 29 are correctly quoted, which prevents word splitting. Past review flagged this; looks like it's sorted. 👌
Also applies to: 29-29
|
I ran a small experiment with a remote node running 
|
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
abailly
force-pushed
the
abailly/follow-chain-from-store
branch
from
b3648e9 to
b102f4c
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
♻️ Duplicate comments (4)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (3)
114-131: Crikey, the tip's gone walkabout!Right, so this backward jaunt collects all the ancestors but never actually adds the
start_header(your tip block at the beginning of the walk) into theheadersvector. When a punter connects with an intersection equal to or close to the anchor, they'll get everything except the current tip—bit like ordering a burger and getting everything but the patty, yeah?The past review nailed it: if intersection == anchor, the client gets a rollback to anchor and maybe one forward for anchor+1, but never the actual tip at
current_tip.Toss the
start_headerinto the queue before or after the reverse—something like this should sort it:headers.push(ClientOp::Forward(header.clone())); current_header = header; } None => return None, } } +// Include the tip itself +headers.push(ClientOp::Forward(start_header.clone())); + // headers contains a list of Fwd operations in reverse order headers.reverse();Or if you reverse first, prepend it after:
// headers contains a list of Fwd operations in reverse order headers.reverse(); +headers.push(ClientOp::Forward(start_header));
129-129: This broken chain deserves a proper rage-quit, not a quiet exit.Returning
Nonehere masks a serious store invariant violation—if a header exists but its parent doesn't, that's like finding a sequel with no original film. The codebase already panics for similar "this should never happen" scenarios (seeHeadersTree'sunsafe_get_header), and the learnings confirm this pattern.Based on learnings.
Apply this diff to fail fast with context:
- None => return None, // FIXME: Broken chain, shouldn't we panic? + None => panic!( + "Store invariant violated: header {} exists but parent {} not found", + current_header.hash(), parent_hash + ),
185-186: Mate, you're ghosting the intersection update!When you pop from
self.opshere, you're handing back a Forward or Backward op but leavingself.intersectionstuck in the past—typically frozen at the anchor value. After the first queued op goes out, any subsequentnext_opcalls are making decisions with yesterday's news, and anyone checkingintersection_found()never sees the headers you just served.The past review spotted this straight away: you need to update
self.intersectionto match the op you're returning, whether it's Forward or Backward.Fix it by syncing the cursor before you return:
- self.ops.pop_front() + if let Some(op) = self.ops.pop_front() { + self.intersection = op.tip(); + Some(op) + } else { + None + }That
op.tip()method exists in client_protocol.rs (lines 98-103 in the relevant snippets) and handles both Forward and Backward variants, so you're sorted.crates/amaru-stores/src/rocksdb/consensus/mod.rs (1)
253-270: Guard against malformed chain keys before decoding the slotWe’re slicing the RocksDB key and calling
try_into().unwrap()under the assumption that the key is perfect. If the store ever contains a truncated or unexpected key (bit-rot, manual fiddling, whatever), we’ll panic here and take the node down—been there, feels like watching Blade Runner’s spinner drop out of the sky mid-chase. Let’s fail gracefully by checking the slice length and handling the conversion error explicitly.- if let Some(Ok((k, v))) = iter.next() { - let slot_bytes = &k[CHAIN_PREFIX.len()..CHAIN_PREFIX.len() + 8]; - let slot = u64::from_be_bytes(slot_bytes.try_into().unwrap()); + if let Some(Ok((k, v))) = iter.next() { + let slot_start = CHAIN_PREFIX.len(); + let slot_end = slot_start + std::mem::size_of::<u64>(); + if k.len() < slot_end { + tracing::error!( + "invalid chain key length ({} bytes), expected at least {}", + k.len(), + slot_end + ); + return None; + } + let slot_bytes: [u8; std::mem::size_of::<u64>()] = + match k[slot_start..slot_end].try_into() { + Ok(bytes) => bytes, + Err(_) => { + tracing::error!("failed to decode slot bytes for chain key"); + return None; + } + }; + let slot = u64::from_be_bytes(slot_bytes);
🧹 Nitpick comments (3)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (3)
247-261: Sweet test coverage, though a tiny style nit if you're keen.This test correctly verifies the behavior when the client's already at the tip—backwards rollback to the start position, all good. The logic is sound.
etorreborre noted in a past comment that you could use
.as_tip()on line 253 instead of manually constructing theTip. The current code works fine, but the helper method would be a touch cleaner:- let start = Tip(tip.clone(), store.get_height(TIP_47)); + let start = store.load_header(&hash(TIP_47)).unwrap().as_tip();But honestly, this is just tidying up—your current approach is perfectly clear.
281-299: Another spot where.as_tip()could save you a line.Similar vibe to the previous test—everything works, but you're manually building a
Tipwhen the helper exists. etorreborre flagged this one too in past reviews.let expected = store.get_point(WINNER_47); + let expected_header = store.load_header(&hash(WINNER_47)).unwrap(); let mut chain_follower = ChainFollower::new(store.clone(), &tip, &points).unwrap(); assert_eq!( chain_follower.next_op(store.clone()), - Some(ClientOp::Backward(Tip(expected, 8))) + Some(ClientOp::Backward(expected_header.as_tip())) );Again, totally optional—just makes the intent a bit more obvious.
301-321: One more.as_tip()opportunity in this test.This test verifies the "start from Origin" scenario when the requested intersection isn't on the best chain—solid coverage. The manual
Tip(Point::Origin, 0)construction works, but for consistency you might consider using the pattern from other tests.That said, for
Point::Originspecifically, manually constructingTip(Point::Origin, 0)is actually pretty clear and explicit, so this is really just for consistency with the other suggestions. Up to you whether it's worth the change.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (15)
crates/amaru-consensus/src/consensus/effects/store_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/validate_block.rs(3 hunks)crates/amaru-consensus/src/consensus/stages/validate_header.rs(1 hunks)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs(1 hunks)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs(2 hunks)crates/amaru-stores/src/rocksdb/consensus/mod.rs(2 hunks)crates/amaru/Cargo.toml(1 hunks)crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs(3 hunks)crates/amaru/src/stages/consensus/forward_chain/client_state.rs(0 hunks)crates/amaru/src/stages/consensus/forward_chain/mod.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs(4 hunks)crates/amaru/src/stages/consensus/forward_chain/tests.rs(1 hunks)crates/amaru/src/stages/mod.rs(3 hunks)scripts/multi-client.sh(1 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru/src/stages/consensus/forward_chain/client_state.rs
🚧 Files skipped from review as they are similar to previous changes (6)
- crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs
- crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs
- crates/amaru-consensus/src/consensus/stages/validate_header.rs
- crates/amaru-consensus/src/consensus/effects/store_effects.rs
- crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs
- scripts/multi-client.sh
🧰 Additional context used
🧠 Learnings (20)
📓 Common learnings
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.
📚 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-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.rscrates/amaru/src/stages/mod.rscrates/amaru/src/stages/consensus/forward_chain/tests.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-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.rscrates/amaru/src/stages/consensus/forward_chain/tests.rscrates/amaru/Cargo.toml
📚 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-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/consensus/forward_chain/mod.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.rscrates/amaru/src/stages/consensus/forward_chain/tests.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-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/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-stores/src/rocksdb/consensus/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-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.rs
📚 Learning: 2025-09-06T09:16:25.025Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 435
File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116
Timestamp: 2025-09-06T09:16:25.025Z
Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.
Applied to files:
crates/amaru-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.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-stores/src/rocksdb/consensus/mod.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-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.rs
📚 Learning: 2025-01-21T15:32:17.911Z
Learnt from: jeluard
Repo: pragma-org/amaru PR: 69
File: crates/amaru/src/ledger/state/diff_epoch_reg.rs:112-117
Timestamp: 2025-01-21T15:32:17.911Z
Learning: When suggesting code changes in Rust, always verify that the types align correctly, especially when dealing with references and Options. The `Fold::Registered` variant in `diff_epoch_reg.rs` expects a reference `&'a V`, so unwrapping an `Option<&V>` requires only a single `.expect()`.
Applied to files:
crates/amaru-stores/src/rocksdb/consensus/mod.rs
📚 Learning: 2025-08-08T14:39:50.527Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/borrowed_datum.rs:32-39
Timestamp: 2025-08-08T14:39:50.527Z
Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.
Applied to files:
crates/amaru-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.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-stores/src/rocksdb/consensus/mod.rscrates/amaru/src/stages/consensus/forward_chain/test_infra.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.rs
📚 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/src/stages/consensus/forward_chain/test_infra.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.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/src/stages/consensus/forward_chain/test_infra.rscrates/amaru/src/stages/consensus/forward_chain/chain_follower.rscrates/amaru-consensus/src/consensus/stages/validate_block.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:
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs
📚 Learning: 2025-04-20T17:56:48.565Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/mod.rs:0-0
Timestamp: 2025-04-20T17:56:48.565Z
Learning: When bootstrapping a node in Amaru, it's important to handle the case where the tip is Origin (for a fresh node). Instead of unconditionally trying to load a header from the chain store, check if the tip is Origin or Specific first, and handle each case appropriately.
Applied to files:
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rscrates/amaru/src/stages/mod.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/src/stages/consensus/forward_chain/chain_follower.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/src/stages/consensus/forward_chain/chain_follower.rs
📚 Learning: 2025-04-20T18:02:25.073Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward.rs:73-75
Timestamp: 2025-04-20T18:02:25.073Z
Learning: In the current development stage, rkuhn prefers using explicit panics (via `.expect()` or `.unwrap()`) for fatal errors in the application code that would tear down the node, rather than propagating errors with `Result`. The intention is to eventually transition to proper error handling with `Result` as the codebase matures.
Applied to files:
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs
🧬 Code graph analysis (4)
crates/amaru-stores/src/rocksdb/consensus/mod.rs (4)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (1)
next_best_chain(75-77)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
next_best_chain(130-146)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
next_best_chain(40-40)next_best_chain(145-147)crates/amaru-kernel/src/point.rs (4)
from(65-67)slot_or_default(33-38)hash(40-46)random_hash(146-148)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (4)
store(27-27)store(50-52)store(72-74)store(124-126)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (5)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (4)
hash_point(124-129)fmt(71-95)fmt(114-121)tip(99-104)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (6)
point(87-89)fmt(226-245)new(102-127)hash(79-81)mk_in_memory_store(43-77)hex(83-85)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (3)
fmt(181-193)load_from_best_chain(36-36)load_from_best_chain(141-143)crates/amaru/src/stages/mod.rs (5)
fmt(80-85)fmt(129-134)from(154-159)as_tip(382-382)as_tip(386-388)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
load_from_best_chain(124-127)
crates/amaru-consensus/src/consensus/stages/validate_block.rs (5)
crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
point(87-89)crates/amaru-kernel/src/consensus_events.rs (1)
point(128-133)crates/amaru-kernel/src/is_header/mod.rs (1)
point(38-40)crates/amaru-consensus/src/consensus/effects/network_effects.rs (1)
point(118-123)crates/amaru-consensus/src/consensus/tip.rs (2)
point(101-106)point(169-171)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: clippy
- GitHub Check: Build wasm32
- GitHub Check: Build riscv32
- GitHub Check: Test the CLI
- GitHub Check: Benches
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Build aarch64/linux
- GitHub Check: Build x86_64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/macos
- GitHub Check: Test coverage
- GitHub Check: Analyze (rust)
🔇 Additional comments (1)
crates/amaru/Cargo.toml (1)
50-50: This review comment is based on a misunderstanding of Cargo's design.Here's the thing, mate — having
amaru-kernelin both [dependencies] and [dev-dependencies] with different features is actually spot-on practice, not a duplication issue. It's like how you'd pack different gear for a road trip versus a weekend camping trip with your mates.The evidence shows
amaru-kernelis extensively used throughout the main crate — stages, CLI commands, network logic, the lot. The "tracing" feature in [dependencies] is essential for production logging. The "test-utils" feature in [dev-dependencies] is supplemental testing infrastructure that doesn't need bloating the shipped binary.This is precisely what Cargo was designed for: specifying different feature sets per dependency scope. Combining them into one would be the actual antipattern here — you'd be shipping unnecessary test utilities into production, which is a no-go for optimization.
The three entries (main deps, dev-deps, build-deps) are all legit and serve their purpose. No changes needed.
Likely an incorrect or invalid review comment.
| chain_store.set_anchor_hash(tip)?; | ||
| chain_store.set_best_chain_hash(tip)?; | ||
| Ok(chain_store) |
There was a problem hiding this comment.
Handle Origin tip before setting anchor
If the ledger tip is still Origin when we boot, this code calls set_anchor_hash(ORIGIN_HASH) and set_best_chain_hash(ORIGIN_HASH). The store doesn’t have a header for that hash, so it errors out and the node bails during bootstrap—kind of like getting wiped in the tutorial mission before the real game even starts. Guard the Origin case before touching the store so fresh nodes can actually come online.
Apply this diff to prevent the blow-up:
- chain_store.set_anchor_hash(tip)?;
- chain_store.set_best_chain_hash(tip)?;
+ if *tip != ORIGIN_HASH {
+ chain_store.set_anchor_hash(tip)?;
+ chain_store.set_best_chain_hash(tip)?;
+ }Based on learnings
🤖 Prompt for AI Agents
In crates/amaru/src/stages/mod.rs around lines 293–295, the code unconditionally
calls chain_store.set_anchor_hash(tip) and set_best_chain_hash(tip) which fails
when the ledger tip is still Origin; check the tip first and skip setting
anchor/best when tip is Origin (e.g., if tip.is_origin() or matches the Origin
variant, return Ok(chain_store) or proceed without calling the setters),
otherwise call the two set_* methods as before and propagate errors.
There was a problem hiding this comment.
This is not possible as Amaru explicit does not support starting from ORIGIN
There was a problem hiding this comment.
@abailly, fair play! If Amaru requires folks to bootstrap their ledger first—kind of like needing to complete the tutorial before you can actually play the game—then yeah, the ORIGIN case is off the table by design. The panic message "Have you bootstrapped your node?" already tells the story: no sneaking past the bouncer without proper ID.
Cheers for clarifying that constraint, mate. I'll leave the code as-is since the architectural decision handles it upstream. 🍻
🧠 Learnings used
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/mod.rs:0-0
Timestamp: 2025-04-20T17:56:48.565Z
Learning: When bootstrapping a node in Amaru, it's important to handle the case where the tip is Origin (for a fresh node). Instead of unconditionally trying to load a header from the chain store, check if the tip is Origin or Specific first, and handle each case appropriately.
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.
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.
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.
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.
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.
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.
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.
There was a problem hiding this comment.
Actionable comments posted: 0
♻️ Duplicate comments (1)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (1)
129-129: Mate, this broken chain situation needs a proper freakout, not a quiet exit.Right now if a parent header's gone walkabout from the store, we just return
Noneand pretend everything's grand—but that's a full-blown store invariant violation (parent exists in theory, vanishes in practice). The project's vibe (seeHeadersTree'sunsafe_get_headerpattern) is to fail fast with a panic when the database gets dodgy, not sweep it under the rug.Apply this diff to make it panic with diagnostic context:
- None => return None, // FIXME: Broken chain, shouldn't we panic? + None => panic!( + "Store invariant violated: header {} exists but parent {} not found", + current_header.hash(), parent_hash + ),Based on learnings.
🧹 Nitpick comments (1)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (1)
192-210: This rollback-handling logic's pretty clever, but it's doing some serious acrobatics.The
add_opmethod's got two modes when aBackwardrolls in:
- If the rollback target's already queued (line 197:
rfinda matching Forward), truncate ops to that point—basically "don't send anything past here."- Otherwise, nuke the queue and push an explicit
Backwardop.The logic looks sound (truncate to
index + 1keeps everything up to and including the target), but this kind of coalescing can be gnarly to debug when things go sideways. Since you're already knee-deep in edge cases (per your PR notes), might be worth a quick verification that the truncation index and queue state stay consistent across various rollback scenarios (e.g., rollback to queued point, rollback past queue, rollback to already-sent point).Consider adding a unit test that exercises:
add_op(Backward(tip))whenopscontainsForward(tip)(should truncate)add_op(Backward(old_point))whenopsis non-empty but doesn't containold_point(should clear and push Backward)- Sequence: add several Forwards, then Backward, then more Forwards—verify queue state at each step
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs(1 hunks)
🧰 Additional context used
🧠 Learnings (14)
📚 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/chain_follower.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/src/stages/consensus/forward_chain/chain_follower.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/src/stages/consensus/forward_chain/chain_follower.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/src/stages/consensus/forward_chain/chain_follower.rs
📚 Learning: 2025-04-20T17:56:48.565Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/mod.rs:0-0
Timestamp: 2025-04-20T17:56:48.565Z
Learning: When bootstrapping a node in Amaru, it's important to handle the case where the tip is Origin (for a fresh node). Instead of unconditionally trying to load a header from the chain store, check if the tip is Origin or Specific first, and handle each case appropriately.
Applied to files:
crates/amaru/src/stages/consensus/forward_chain/chain_follower.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/src/stages/consensus/forward_chain/chain_follower.rs
📚 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/src/stages/consensus/forward_chain/chain_follower.rs
📚 Learning: 2025-08-08T14:39:50.527Z
Learnt from: KtorZ
Repo: pragma-org/amaru PR: 370
File: crates/amaru-kernel/src/borrowed_datum.rs:32-39
Timestamp: 2025-08-08T14:39:50.527Z
Learning: In the amaru project, when converting BorrowedDatumOption::Data to an owned DatumOption in crates/amaru-kernel/src/borrowed_datum.rs, the call `.unwrap()` refers to pallas’s KeepRaw::unwrap, which is infallible (always returns the inner value) and is not a panic risk. Future reviews should not flag this unwrap as dangerous.
Applied to files:
crates/amaru/src/stages/consensus/forward_chain/chain_follower.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/src/stages/consensus/forward_chain/chain_follower.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/src/stages/consensus/forward_chain/chain_follower.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/src/stages/consensus/forward_chain/chain_follower.rs
📚 Learning: 2025-04-20T18:02:25.073Z
Learnt from: rkuhn
Repo: pragma-org/amaru PR: 149
File: crates/amaru/src/stages/consensus/chain_forward.rs:73-75
Timestamp: 2025-04-20T18:02:25.073Z
Learning: In the current development stage, rkuhn prefers using explicit panics (via `.expect()` or `.unwrap()`) for fatal errors in the application code that would tear down the node, rather than propagating errors with `Result`. The intention is to eventually transition to proper error handling with `Result` as the codebase matures.
Applied to files:
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs
📚 Learning: 2025-09-06T09:16:25.025Z
Learnt from: abailly
Repo: pragma-org/amaru PR: 435
File: crates/amaru/src/bin/amaru/cmd/convert_ledger_state.rs:113-116
Timestamp: 2025-09-06T09:16:25.025Z
Learning: In cardano-node serialized ledger state CBOR encoding, indefinite-length structures may be terminated with 0xFF "break" markers. The current code in convert_ledger_state.rs unconditionally strips the last byte (bytes[p..bytes.len() - 1]), which could corrupt data if the trailing byte is not actually a CBOR break marker.
Applied to files:
crates/amaru/src/stages/consensus/forward_chain/chain_follower.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/src/stages/consensus/forward_chain/chain_follower.rs
🧬 Code graph analysis (1)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (6)
crates/amaru/src/stages/consensus/forward_chain/client_protocol.rs (4)
hash_point(124-129)fmt(71-95)fmt(114-121)tip(99-104)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (6)
point(87-89)fmt(226-245)new(102-127)hash(79-81)mk_in_memory_store(43-77)hex(83-85)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (3)
fmt(181-193)load_from_best_chain(36-36)load_from_best_chain(141-143)crates/amaru/src/stages/mod.rs (5)
fmt(80-85)fmt(129-134)from(154-159)as_tip(382-382)as_tip(386-388)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (3)
new(35-39)new(59-69)load_from_best_chain(124-127)crates/amaru-consensus/src/consensus/effects/store_effects.rs (26)
new(29-31)new(121-123)new(150-152)new(178-180)new(206-208)new(235-237)new(263-265)new(291-293)new(319-321)new(345-347)new(371-373)run(128-136)run(157-165)run(185-193)run(213-221)run(242-250)run(270-278)run(298-306)run(326-334)run(352-360)run(378-386)run(406-414)run(434-442)run(462-470)run(490-498)load_from_best_chain(71-73)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (12)
- GitHub Check: Test the CLI
- GitHub Check: Benches
- GitHub Check: Build x86_64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/macos
- GitHub Check: Build wasm32
- GitHub Check: Build riscv32
- GitHub Check: Build aarch64/linux
- GitHub Check: Test coverage
- GitHub Check: clippy
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Analyze (rust)
🔇 Additional comments (4)
crates/amaru/src/stages/consensus/forward_chain/chain_follower.rs (4)
158-182: Solid handling of the behind-anchor case, panic and all!The logic here's bang-on: when the follower's trailing the anchor, you query the store for the next best chain point, load the header, update the intersection, and forward it. The panic at lines 171-174 (when
next_best_chainreturns a point but the header's missing) is exactly the right move for catching invariant violations. Well played!
184-189: Nice one—intersection cursor stays in sync when draining ops!This bit (line 185:
self.intersection = op.tip()) ensures the intersection advances as we emit queued operations, keeping the follower's state consistent with what's been sent downstream. Past review flagged this as a concern, and you've nailed it.
286-303: Heads up: test comment reckons the intersection logic doesn't match the documented spec.Line 290-291 says "Note that the below scheme does not match the documented behaviour, which shall pick the first from the list that is on the same chain. But that doesn't make sense to me at all."
The implementation (line 99 in
new()) usesmax_by_keyon slot, meaning it picks the highest slot from the requested points. If the spec actually says "pick the first point from the list that's on the best chain," that's a different algorithm (order-dependent vs. slot-based). Might want to double-check which behavior is intended, or update the comment if the max-slot approach is now canonical.Could you clarify whether:
- The max-slot approach is the correct implementation (and the comment's just outdated)
- Or there's a genuine spec deviation here that needs addressing?
134-145: The concern is valid—start_header never makes it into ops.The evidence is pretty clear:
start_header = store.load_header(&hash_point(current_tip))at line 72 loads the actual current tip. Then the while loop (lines 115–132) walks backwards from it, but the loop breaks without ever pushingstart_headeror even the intersection point itself. After reversing, ops contains only the headers between the intersection and the tip—exclusive on both ends.Here's the flow:
- Early exit (lines 82–88): If client already has
current_tipin their points, it setsinitial = start_headerandops = []. This case works fine.- Normal path (lines 137–146): Sets
initial = best_intersection(NOTstart_header) andops = walked ancestors (without start_header).When
next_op()runs (lines 151–169), the client gets aBackwardtobest_intersection, then consumes the ops, but never receivesstart_headeras aForward. The comment at lines 47–50 even hints at the gap: "intersection represents the parent of the next header to forward to client," suggesting something still needs forwarding.Whether this is intentional design (with a follow-up sync handling the gap) or an actual bug that needs fixing would require checking how this integrates with the client sync protocol and whether there's a continuation mechanism downstream.
Signed-off-by: Arnaud Bailly <arnaud@pankzsoft.com>
abailly
force-pushed
the
abailly/follow-chain-from-store
branch
from
aa909f4 to
9a770b6
Compare
2 participants
This PR introduces a
ChainFollowerstructure that keeps track of a single downstream peer's synchronization state with two modalities:intersectionand then evolving) is behind theanchorof the chain, the forwarded headers are loaded from the store,ClientOpmessages accumulated in a queue while it was synchronising. This queue is initially filled with the "volatile" part of the chain, e.g the part between theanchorand thetip.The code is surprisingly convoluted and there are a few edge cases one needs to deal with that should be covered by the store's invariant, e.g the store should guarantee that if there's a child relationship between 2 headers, then both of them are in the store. However it's really not obvious to me how to enforce that and expose it in the API.
Note that ideally I would have each
ChainFollowerkeep anIteratorover the underlyingChainStorebut asArc<T>is notSendit's seemingly not possible. I would be interested in knowing from Rust experts whether or not this is feasible as I think it would somewhat simplify the design and possibly improve the performances.Fix #426
Summary by CodeRabbit
New Features
Improvements
Infrastructure