Conversation
WalkthroughRefactors consensus into a trait-based ops surface (ConsensusOps) with sub-ops (Base/Network/Ledger/Store/Metrics); replaces many ExternalEffect usages with trait delegations and external_sync; introduces HeadersTreeState; extends pure-stage Effects with Resources and external_sync; updates stages, stores, tests, and Cargo dependency features. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Stage as Stage
participant CE as ConsensusEffects (impl ConsensusOps)
participant Net as NetworkOps
participant Led as LedgerOps
participant Sto as Store (ChainStore)
participant Base as BaseOps
participant Met as MetricsOps
participant Res as Resources
rect rgba(230,240,255,0.6)
note over Stage,CE: Trait-based dispatch replaces direct ExternalEffect calls
Stage->>CE: eff.network()/eff.ledger()/eff.store()/eff.base()/eff.metrics()
CE->>Res: access resources (era_history, stores, clients)
end
Stage->>Net: fetch_block(peer, point)
Net->>Res: use BlockFetcher / external resource
Net-->>Stage: Vec<u8> | ConsensusError
alt validate
Stage->>Led: validate(peer, point, RawBlock)
Led->>Res: roll_forward_block(...)
Led-->>Stage: LedgerMetrics | BlockValidationError
Stage->>Met: record(event)
else rollback
Stage->>Led: rollback(peer, header)
Led->>Res: rollback_block(...)
Led-->>Stage: Result
end
Stage->>Sto: store_header / store_block (via external_sync)
Sto->>Res: ResourceHeaderStore
Sto-->>Stage: StoreResponse
Stage->>Base: send(downstream/errors, msg)
Base-->>Stage: ack
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests
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 |
etorreborre
force-pushed
the
etorreborre/feat/more-encapsulated-effects
branch
10 times, most recently
from
89f8b1e to
4efba8c
Compare
Signed-off-by: etorreborre <etorreborre@yahoo.com> | Conflicts: | crates/amaru-consensus/src/consensus/effects/mod.rs | crates/amaru-consensus/src/consensus/effects/network_effects.rs | crates/amaru-consensus/src/consensus/stages/fetch_block.rs | crates/amaru-consensus/src/consensus/stages/validate_block.rs | crates/amaru/src/stages/build_stage_graph.rs | crates/amaru/src/stages/mod.rs | simulation/amaru-sim/src/simulator/run.rs | Conflicts: | crates/amaru-consensus/src/consensus/effects/mod.rs | crates/amaru-consensus/src/consensus/stages/select_chain.rs | crates/amaru-consensus/src/consensus/stages/validate_block.rs | crates/amaru/src/stages/mod.rs
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com>
Signed-off-by: etorreborre <etorreborre@yahoo.com> | Conflicts: | crates/pure-stage/src/simulation.rs
Signed-off-by: etorreborre <etorreborre@yahoo.com>
etorreborre
force-pushed
the
etorreborre/feat/more-encapsulated-effects
branch
from
4efba8c to
81346ec
Compare
|
@coderabbitai review |
✅ Actions performedReview triggered.
|
etorreborre
force-pushed
the
etorreborre/feat/more-encapsulated-effects
branch
2 times, most recently
from
7d647bb to
2b924c8
Compare
| use std::sync::Arc; | ||
| use tracing::instrument; | ||
|
|
||
| /// Type alias for a header hash to improve readability |
There was a problem hiding this comment.
This has been moved to the store_effects module in order to be reused in other stages.
| #[derive(Debug, Error)] | ||
| pub struct ProcessingFailed { | ||
| pub peer: Peer, | ||
| pub peer: Option<Peer>, |
There was a problem hiding this comment.
The peer might not always be available when there is a processing error.
| use std::collections::BTreeMap; | ||
| use std::fmt::{Debug, Display, Formatter}; | ||
|
|
||
| /// Type alias for a Header hash |
There was a problem hiding this comment.
This was moved in order to be shared with other files.
| span, | ||
| } => { | ||
| let header = match receive_header(&point, raw_header.as_slice()) { | ||
| let header = match decode_header(&point, raw_header.as_slice()) { |
There was a problem hiding this comment.
I renamed this function for clarity
|
|
||
| impl SelectChain { | ||
| pub fn new(chain_selector: HeadersTree<Header>, peers: &[Peer]) -> Self { | ||
| pub fn new(tree_state: HeadersTreeState, peers: &[Peer]) -> Self { |
There was a problem hiding this comment.
We just need to pass the transient headers tree state to create the select chain transient state.
| /// | ||
| /// The main implementation is in terms of [`Effects`](pure_stage::Effects), but other implementations | ||
| /// are used e.g. for testing. | ||
| pub trait StoreOps { |
There was a problem hiding this comment.
This "store" effect is now implemented by returning a full ChainStore trait implementation since we need many more methods.
![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
♻️ Duplicate comments (2)
crates/amaru-consensus/src/consensus/stages/select_chain.rs (2)
150-156: Samemem::takepattern—same concern.This method uses the same
mem::takepattern asselect_chain, so theDefaulttrait requirement applies here too. If you address it forselect_chain, you're golden here as well—no need to mention it twice, like a broken record on a jukebox!
413-448: Same brittle assertion issue here.Same deal as the previous test—using
format!("{:?}", ...)for assertions. If you address it in the first test by updating the mock to store actual typed events, you can apply the same fix here. No need to repeat myself like a scratched DVD!
🧹 Nitpick comments (3)
crates/amaru-kernel/src/string_utils.rs (1)
79-91: Implementation is solid as, but could use a quick unit test, yeah?The impl follows the same playbook as
ListsToString—delegates tolist_debugfor inner lists andlist_to_stringfor the outer Vec. Logic's spot-on and the HRTB bounds match the pattern perfectly. G'day to that!One thing though: there's no test coverage for
lists_debugin the test module (lines 93-114), and given you're adding this for the consensus headers tree debugging (as per the PR context), it'd be ace to have a quick test to lock in the behavior—especially for nestedDebugtypes that might have quirky formatting.Reckon you could add a test like this to match the vibe of
make_a_string_from_a_list_of_lists?+ #[test] + fn make_a_debug_string_from_a_list_of_lists() { + let actual = vec![vec![Some(1), Some(2)], vec![Some(3)]].lists_debug(", ", " | "); + assert_eq!(actual, "[Some(1), Some(2)] | [Some(3)]"); + }crates/amaru-consensus/src/consensus/stages/select_chain.rs (1)
384-411: Brittle test assertions usingDebugformat strings.Oi, the test here is comparing received messages using
format!("{:?}", output), which is a bit dodgy becauseDebugoutput can change if you tweak the struct or add fields. This makes the test fragile—like a sandcastle at high tide!Better to compare the actual values instead of string representations. For instance, if
MockBaseOpscan return actual typed messages (not strings), you could assert on those directly.If
MockBaseOps::received()returnsVec<ValidateHeaderEvent>instead ofVec<String>, you could do:- assert_eq!( - consensus_ops.mock_base.received(), - BTreeMap::from_iter(vec![( - "downstream".to_string(), - vec![format!("{:?}", output)] - )]) - ); + assert_eq!( + consensus_ops.mock_base.received(), + BTreeMap::from_iter(vec![( + "downstream".to_string(), + vec![output] + )]) + );(Assuming the mock can be updated to store actual events instead of their debug strings.)
crates/amaru-stores/Cargo.toml (1)
27-27: Optional refactor: Explicit empty features list is redundant.The explicit
features = []on line 27 is functionally equivalent toworkspace = true(it selects the default features). This is harmless but adds visual noise. You can simplify toamaru-ouroboros-traits.workspace = trueif preferred.Apply this diff to simplify:
-amaru-ouroboros-traits = { workspace = true, features = [] } +amaru-ouroboros-traits.workspace = true
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (38)
crates/amaru-consensus/Cargo.toml(1 hunks)crates/amaru-consensus/src/consensus/effects/base_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/block_effects.rs(0 hunks)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/ledger_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/metrics_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/network_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/store_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/errors.rs(3 hunks)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs(28 hunks)crates/amaru-consensus/src/consensus/headers_tree/tree.rs(1 hunks)crates/amaru-consensus/src/consensus/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/fetch_block.rs(4 hunks)crates/amaru-consensus/src/consensus/stages/forward_chain.rs(4 hunks)crates/amaru-consensus/src/consensus/stages/receive_header.rs(3 hunks)crates/amaru-consensus/src/consensus/stages/select_chain.rs(7 hunks)crates/amaru-consensus/src/consensus/stages/store_block.rs(3 hunks)crates/amaru-consensus/src/consensus/stages/store_header.rs(2 hunks)crates/amaru-consensus/src/consensus/stages/validate_block.rs(4 hunks)crates/amaru-consensus/src/consensus/stages/validate_header.rs(10 hunks)crates/amaru-consensus/src/consensus/store.rs(1 hunks)crates/amaru-consensus/src/consensus/tests.rs(1 hunks)crates/amaru-kernel/src/string_utils.rs(1 hunks)crates/amaru-ouroboros-traits/src/is_header/fake.rs(2 hunks)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs(3 hunks)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs(2 hunks)crates/amaru-stores/Cargo.toml(2 hunks)crates/amaru-stores/src/rocksdb/consensus.rs(8 hunks)crates/amaru/src/stages/build_stage_graph.rs(3 hunks)crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs(1 hunks)crates/amaru/src/stages/consensus/forward_chain/test_infra.rs(1 hunks)crates/amaru/src/stages/mod.rs(3 hunks)crates/pure-stage/src/effect.rs(6 hunks)crates/pure-stage/src/simulation.rs(1 hunks)crates/pure-stage/src/stage_ref.rs(1 hunks)crates/pure-stage/src/tokio.rs(1 hunks)simulation/amaru-sim/src/simulator/run.rs(6 hunks)
💤 Files with no reviewable changes (1)
- crates/amaru-consensus/src/consensus/effects/block_effects.rs
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
PR: pragma-org/amaru#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-consensus/Cargo.tomlcrates/amaru-stores/Cargo.toml
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
PR: pragma-org/amaru#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-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rscrates/amaru-consensus/src/consensus/stages/validate_header.rscrates/amaru-stores/src/rocksdb/consensus.rs
📚 Learning: 2025-08-23T15:44:44.318Z
Learnt from: rkuhn
PR: pragma-org/amaru#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-consensus/src/consensus/stages/validate_header.rscrates/amaru-stores/src/rocksdb/consensus.rscrates/amaru-consensus/src/consensus/tests.rscrates/amaru-ouroboros-traits/src/is_header/fake.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, ValidateHeader::handle_roll_forward returns a Result<PullEvent, ConsensusError>, not ValidateHeaderEvent as might be expected from the older code structure.
Applied to files:
crates/amaru-consensus/src/consensus/stages/validate_header.rs
📚 Learning: 2025-09-05T17:30:55.869Z
Learnt from: etorreborre
PR: pragma-org/amaru#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/fetch_block.rs
📚 Learning: 2025-05-12T14:21:27.470Z
Learnt from: stevana
PR: pragma-org/amaru#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:
simulation/amaru-sim/src/simulator/run.rs
🧬 Code graph analysis (28)
crates/amaru-consensus/src/consensus/stages/receive_header.rs (6)
crates/amaru-consensus/src/consensus/span.rs (2)
adopt_current_span(32-36)adopt_current_span(97-99)crates/pure-stage/src/stage_ref.rs (2)
name(84-86)named(80-82)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (2)
mock_consensus_ops(306-314)received(232-234)crates/amaru-consensus/src/consensus/stages/fetch_block.rs (1)
stage(38-88)crates/amaru-consensus/src/consensus/stages/select_chain.rs (1)
stage(317-340)crates/amaru-consensus/src/consensus/stages/store_header.rs (1)
stage(26-46)
crates/pure-stage/src/stage_ref.rs (1)
crates/pure-stage/src/types.rs (1)
from(177-179)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (7)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (17)
store(519-519)store(545-545)store(571-571)store(597-597)new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)new(344-346)new(372-374)new(400-402)new(426-428)era_history(129-131)crates/amaru-consensus/src/consensus/effects/ledger_effects.rs (7)
new(46-48)new(87-93)new(122-127)validate(28-33)validate(52-61)rollback(35-39)rollback(63-70)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (1)
era_history(153-153)crates/amaru-consensus/src/consensus/effects/network_effects.rs (8)
fetch_block(32-36)fetch_block(61-67)point(147-152)send_forward_event(38-42)send_forward_event(69-76)send_backward_event(44-48)send_backward_event(78-87)send(137-137)crates/amaru-consensus/src/consensus/effects/base_effects.rs (8)
send(21-25)send(42-48)clock(26-26)clock(50-52)wait(27-27)wait(54-56)terminate(28-28)terminate(58-60)crates/pure-stage/src/effect.rs (4)
send(105-111)clock(114-119)wait(122-127)terminate(235-237)crates/amaru-consensus/src/consensus/effects/metrics_effects.rs (2)
record(23-23)record(37-39)
crates/pure-stage/src/simulation.rs (3)
crates/pure-stage/src/effect.rs (4)
new(71-85)new(340-342)me(90-92)self_sender(97-99)crates/pure-stage/src/tokio.rs (1)
new(62-70)crates/pure-stage/src/simulation/running.rs (1)
new(79-107)
crates/amaru-consensus/src/consensus/stages/store_header.rs (1)
crates/amaru-consensus/src/consensus/span.rs (2)
adopt_current_span(32-36)adopt_current_span(97-99)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (3)
crates/amaru-consensus/src/consensus/stages/select_chain.rs (4)
select_chain(105-142)select_chain(488-493)new(49-52)new(70-76)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
load_headers(29-29)load_headers(98-100)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
load_headers(79-89)
crates/amaru-consensus/src/consensus/stages/forward_chain.rs (5)
crates/amaru-consensus/src/consensus/stages/fetch_block.rs (2)
stage(38-88)new(125-133)crates/amaru-consensus/src/consensus/stages/validate_block.rs (1)
stage(35-115)crates/amaru-consensus/src/consensus/effects/base_effects.rs (1)
new(36-38)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (1)
new(50-55)crates/amaru-consensus/src/consensus/effects/network_effects.rs (3)
new(55-57)new(109-114)new(171-176)
crates/amaru-consensus/src/consensus/stages/validate_block.rs (2)
crates/amaru-consensus/src/consensus/span.rs (2)
adopt_current_span(32-36)adopt_current_span(97-99)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (4)
metrics(38-38)metrics(73-75)metrics(95-97)metrics(144-146)
crates/amaru-kernel/src/string_utils.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
format(140-173)
crates/amaru-consensus/src/consensus/errors.rs (2)
crates/amaru-consensus/src/consensus/headers_tree/data_generation/actions.rs (1)
peer(81-86)crates/amaru-consensus/src/consensus/events.rs (2)
peer(101-107)peer(261-267)
crates/amaru/src/stages/mod.rs (2)
crates/amaru-consensus/src/consensus/stages/select_chain.rs (2)
select_chain(105-142)select_chain(488-493)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (2)
anchor(425-427)peers(74-76)
crates/amaru-consensus/src/consensus/stages/validate_header.rs (3)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (4)
store(29-29)store(57-59)store(79-81)store(128-130)crates/amaru-consensus/src/consensus/effects/store_effects.rs (39)
store(519-519)store(545-545)store(571-571)store(597-597)new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)new(344-346)new(372-374)new(400-402)new(426-428)new(452-454)new(480-482)new(506-508)new(532-534)has_header(95-97)load_header(50-52)get_children(75-77)get_anchor_hash(79-81)get_best_chain_hash(83-85)load_block(87-89)load_headers(54-56)load_nonces(58-60)load_blocks(62-64)load_parents_children(66-73)get_nonces(91-93)set_anchor_hash(101-103)set_best_chain_hash(105-107)update_best_chain(109-111)store_header(113-115)store_block(117-119)remove_header(121-123)put_nonces(125-127)era_history(129-131)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (19)
has_header(149-152)load_header(73-76)get_children(155-162)get_anchor_hash(165-168)get_best_chain_hash(171-174)load_block(139-146)load_headers(79-89)load_nonces(92-102)load_blocks(105-115)load_parents_children(118-130)get_nonces(133-136)set_anchor_hash(233-237)set_best_chain_hash(240-244)update_best_chain(247-252)store_header(179-190)store_block(204-208)remove_header(211-230)put_nonces(193-197)era_history(199-201)
crates/amaru-stores/src/rocksdb/consensus.rs (1)
crates/amaru-ouroboros-traits/src/is_header/fake.rs (3)
any_fake_header(203-216)any_fake_headers_chain(180-182)run(220-223)
crates/amaru-consensus/src/consensus/tests.rs (1)
crates/amaru-ouroboros-traits/src/is_header/fake.rs (1)
any_header_hash(198-200)
crates/amaru-consensus/src/consensus/stages/fetch_block.rs (4)
crates/amaru-consensus/src/consensus/span.rs (2)
adopt_current_span(32-36)adopt_current_span(97-99)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (3)
new(50-55)mock_consensus_ops(306-314)received(232-234)crates/amaru-consensus/src/consensus/tests.rs (1)
any_header(42-68)crates/pure-stage/src/stage_ref.rs (1)
named(80-82)
crates/amaru-consensus/src/consensus/stages/select_chain.rs (4)
crates/amaru-consensus/src/consensus/span.rs (2)
adopt_current_span(32-36)adopt_current_span(97-99)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (7)
fmt(118-123)fmt(127-129)fmt(133-135)new(65-70)new(201-210)peers(74-76)create(219-227)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (5)
store(29-29)store(57-59)store(79-81)store(128-130)mock_consensus_ops(306-314)crates/amaru-consensus/src/consensus/stages/store_header.rs (1)
stage(26-46)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (3)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (5)
new(50-55)store(29-29)store(57-59)store(79-81)store(128-130)crates/pure-stage/src/effect.rs (8)
new(71-85)new(340-342)external_sync(210-217)run(251-251)run(322-324)run(361-363)wrap_sync(267-274)wrap(254-264)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (30)
era_history(153-153)load_header(28-28)load_header(94-96)load_headers(29-29)load_headers(98-100)load_nonces(30-30)load_nonces(102-104)load_blocks(31-31)load_blocks(106-108)load_parents_children(32-34)load_parents_children(110-114)get_children(35-35)get_children(116-118)get_anchor_hash(36-36)get_anchor_hash(120-122)get_best_chain_hash(37-37)get_best_chain_hash(124-126)load_block(38-38)load_block(128-130)get_nonces(39-39)get_nonces(132-134)has_header(40-40)has_header(136-138)set_anchor_hash(147-147)set_best_chain_hash(148-148)update_best_chain(149-149)store_header(146-146)store_block(151-151)remove_header(150-150)put_nonces(152-152)
crates/amaru-consensus/src/consensus/store.rs (3)
crates/amaru-ledger/src/state.rs (1)
protocol_parameters(231-233)crates/amaru-ledger/src/store.rs (1)
protocol_parameters(166-166)crates/amaru-stores/src/in_memory/mod.rs (2)
protocol_parameters(101-103)protocol_parameters(367-369)
crates/amaru-consensus/src/consensus/stages/store_block.rs (2)
crates/amaru-consensus/src/consensus/span.rs (2)
adopt_current_span(32-36)adopt_current_span(97-99)crates/amaru-consensus/src/consensus/effects/store_effects.rs (16)
new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)new(344-346)new(372-374)new(400-402)new(426-428)new(452-454)new(480-482)new(506-508)new(532-534)
crates/amaru-consensus/src/consensus/effects/metrics_effects.rs (3)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (3)
record(295-297)record(301-303)new(50-55)crates/amaru-consensus/src/consensus/effects/ledger_effects.rs (3)
new(46-48)new(87-93)new(122-127)crates/amaru-consensus/src/consensus/effects/store_effects.rs (11)
new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)new(344-346)new(372-374)new(400-402)
crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (2)
anchor(425-427)ancestors_hashes(495-497)
simulation/amaru-sim/src/simulator/run.rs (4)
crates/amaru-consensus/src/consensus/stages/select_chain.rs (4)
select_chain(105-142)select_chain(488-493)new(49-52)new(70-76)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
era_history(199-201)crates/amaru/src/stages/build_stage_graph.rs (1)
build_stage_graph(32-154)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (4)
new(65-70)new(201-210)peers(74-76)anchor(425-427)
crates/amaru-consensus/src/consensus/effects/base_effects.rs (2)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (9)
send(238-249)send(265-276)clock(251-253)clock(278-280)wait(255-257)wait(282-284)terminate(259-261)terminate(286-288)new(50-55)crates/pure-stage/src/effect.rs (4)
send(105-111)clock(114-119)wait(122-127)terminate(235-237)
crates/amaru-consensus/src/consensus/effects/network_effects.rs (3)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (4)
fetch_block(171-184)send_forward_event(186-192)send_backward_event(194-200)new(50-55)crates/amaru-consensus/src/consensus/stages/fetch_block.rs (3)
fetch_block(93-93)fetch_block(138-140)new(125-133)crates/pure-stage/src/effect.rs (1)
wrap(254-264)
crates/pure-stage/src/effect.rs (5)
crates/pure-stage/src/simulation/running.rs (5)
resources(112-114)effect(156-156)effect(160-160)effect(269-271)new(79-107)crates/pure-stage/src/stagegraph.rs (2)
resources(222-222)new(36-39)crates/amaru-consensus/src/consensus/effects/store_effects.rs (21)
resources(154-155)resources(183-184)resources(211-212)resources(239-240)resources(268-269)resources(296-297)resources(325-326)resources(353-354)resources(381-382)resources(409-410)resources(435-436)resources(461-462)external_sync(41-46)new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)crates/pure-stage/src/simulation.rs (1)
resources(334-336)crates/pure-stage/src/tokio.rs (2)
resources(214-216)new(62-70)
crates/pure-stage/src/tokio.rs (3)
crates/pure-stage/src/simulation.rs (1)
resources(334-336)crates/pure-stage/src/simulation/running.rs (5)
resources(112-114)new(79-107)effect(156-156)effect(160-160)effect(269-271)crates/pure-stage/src/effect.rs (3)
new(71-85)new(340-342)me(90-92)
crates/amaru-consensus/src/consensus/effects/ledger_effects.rs (2)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (7)
ledger(34-34)ledger(65-67)ledger(87-89)ledger(136-138)validate(207-215)rollback(217-223)new(50-55)crates/amaru-consensus/src/consensus/effects/network_effects.rs (8)
point(147-152)new(55-57)new(109-114)new(171-176)run(119-130)run(181-201)resources(124-125)resources(186-187)
crates/amaru/src/stages/build_stage_graph.rs (7)
crates/amaru-ledger/src/state.rs (3)
global_parameters(235-237)era_history(227-229)network(223-225)crates/amaru-consensus/src/consensus/effects/store_effects.rs (14)
era_history(129-131)store_header(113-115)store_block(117-119)new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)new(344-346)new(372-374)new(400-402)crates/amaru-consensus/src/consensus/stages/validate_header.rs (4)
era_history(342-344)stage(172-218)store_header(326-328)store_block(330-332)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (5)
network(31-31)network(61-63)network(83-85)network(132-134)fetch_block(171-184)crates/amaru-consensus/src/consensus/stages/store_header.rs (1)
stage(26-46)crates/pure-stage/src/simulation.rs (2)
stage(210-262)state(234-234)crates/pure-stage/src/tokio.rs (1)
stage(111-132)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (9)
- GitHub Check: Test the CLI
- GitHub Check: Build riscv32
- GitHub Check: Benches
- GitHub Check: Build aarch64/macos
- GitHub Check: Build x86_64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Build aarch64/linux
- GitHub Check: Test coverage
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
🔇 Additional comments (55)
crates/amaru-ouroboros-traits/src/is_header/fake.rs (3)
176-177: No dramas here, mate!The new imports for
ValueTreeandTestRunnerare spot-on for therunutility function you're adding below. Clean as a whistle.
180-182: Ripper of a rename!Switching from
any_headers_chaintoany_fake_headers_chainis like putting a proper label on the tin — makes it crystal clear these are for testing, no confusion. Love it when names tell the full story.
218-223: Brilliant test helper, no worries!The
runutility is a beaut — gives you a quick way to fire off a strategy and grab the value. Theunwrap()panic behavior is exactly what you want here for test code, as it'll blow up fast if something's dodgy with your test data during development. The#[expect(clippy::unwrap_used)]annotation keeps the linter happy too.Based on learnings, this panic-on-failure approach is the go for test utilities in this file.
crates/amaru-kernel/src/string_utils.rs (1)
72-77: Trait definition looks mint, mate!The
ListsDebugtrait mirrors theListsToStringpattern beautifully—same method signature vibe, clean doc example withOption<i32>(classic choice for showing offDebug). The documentation example checks out perfectly when you trace through the implementation logic. Noice work!crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (5)
21-21: Righto, imports sorted! Arc + Mutex combo is the classic shared-state recipe.The addition of
ArcandMuteximports sets up the infrastructure for thread-safe shared ownership, which aligns perfectly with the broader trait-based refactor in this PR. No dramas here, mate!
24-24: Clone derive looking schmick! Arc makes this a cheap ref-count bump, not a full deep copy.Adding
Clonehere is spot-on for the new architecture. Since the inner field is wrapped inArc, cloning is just incrementing a reference counter—fast as, and perfect for sharing the store across different consensus stages without lugging around heavy duplicates. It's like sharing a Netflix password instead of buying separate subscriptions, ya know?
26-26: Arc<Mutex<...>> pattern is bonzer for shared mutable state!Wrapping the inner store in
Arc<Mutex<...>>is the textbook Rust pattern for thread-safe shared ownership. All the methods I scoped out (lines 73-252) follow a clean "lock → work → drop" rhythm with no nested locks or long-held guards, so deadlock risk is minimal. Strewth!One thing to keep in your back pocket: if you get heaps of threads hammering this store concurrently, lock contention could become a bottleneck. For now, though, this implementation is perfectly adequate for an in-memory test store. If performance becomes a concern down the track, you could consider
RwLock(if reads dominate) or sharding, but that's a refactor for another day.
36-40: Constructor update is mint! Initializes the Arc-wrapped store correctly.The
new()method now properly wraps the inner store inArc::new(Mutex::new(...)), matching the updated field type. Simple as, no hidden surprises here. It's like upgrading from a push-bike to an e-bike—same destination, just with better infrastructure.
71-71: Trait impl signature is solid, bounds make perfect sense for this Arc-based pattern.The
ReadOnlyChainStoreimpl requiresH: IsHeader + Clone + Send + Sync + 'static, which is exactly what you need here:
Cloneis used throughout (e.g., line 75's.cloned())Send + Syncare required for theArc<Mutex<...>>pattern to be thread-safe'staticallows the store to be shared across thread boundaries without lifetime issuesReflowing to a single line is just a tidy-up—no semantic change. All good in the hood!
simulation/amaru-sim/src/simulator/run.rs (6)
23-27: LGTM! Imports align with the refactor to trait-based consensus ops.The switch from
HeadersTreetoHeadersTreeStateand the addition ofEraHistoryare bang on for the new architecture, mate. Clean as a whistle!Also applies to: 30-30, 43-43
107-108: Nice work threading network_name through the initialization flow.The updated destructuring captures
network_namefrominit_node, which is then used to deriveera_historydown at line 162. Strewth, that's a tidy chain of dependencies!
228-228: LGTM! Return statement matches the new signature.The
network_nameis properly threaded through the return tuple. Like a good team-up move in a fighting game - perfectly synced!
196-204: init_node call sites updated
Scanned everywhere—only the run.rs:108 invocation exists and it's updated for NetworkName. Mission accomplished.
162-162: No lifetime mismatch –network_name.into()yields a'staticreference
TheFrom<NetworkName>impl returns an&'static EraHistory, so passingera_historyintobuild_stage_graphis safe.
235-244: initialize_peer implementation verified Theinitialize_peermethod onHeadersTreeStatematches the call inrun.rs—no changes needed.crates/amaru-consensus/src/consensus/stages/select_chain.rs (7)
15-33: LGTM mate!Imports are spot on for the refactoring—bringing in
ConsensusOps,HeadersTreeState,Arc, andmemto support the trait-based ops and state extraction. Clean as a whistle!
37-41: Nice one!The
SelectChainstruct now holdstree_state: HeadersTreeStatewhich aligns perfectly with the PR's goal of extracting transient headers tree state. Serde derives look good for persistence and testing.
70-76: Sweet as!Constructor now takes
tree_stateas a parameter, which is bang on for the refactor. Externalising state construction = more testable code, no worries!
184-205: Ripper refactor!The method now returns a
BoxFuture, which is a classic move when you need to return a future from a non-async function (handy for trait methods or dynamic dispatch). TheBox::pin(async move { ... })pattern is textbook stuff. Logic inside the match is clean—delegates toselect_chain,select_rollback, orcaught_upas appropriate. Top notch!
317-340: Absolutely cracking!The
stagefunction now takeseff: impl ConsensusOps, which is the heart of the trait-based refactor. It callseff.store()to get the store andeff.base().send(...)to send messages—textbook usage of the ops pattern. Error handling is solid (early return with error sent toerrorsref), and tracing is in place. No drama here, mate!
450-495: Helper functions are mint!All the test helpers look solid—
make_state,make_roll_forward_message,make_rollback_message,make_validated_event, andcheck_peersare clear, concise, and do what they say on the tin. No worries here!
112-117: No changes required: HeadersTreeState implements Default
HeadersTreeStateis annotated with#[derive(Default)], somem::take(&mut self.tree_state)compiles correctly.crates/amaru-stores/src/rocksdb/consensus.rs (2)
489-489: Sweet as, mate! The new test helpers are bang on.Swapping to
run(any_fake_headers_chain(n))is a ripper move—cleaner than the old proptest scaffolding for these unit tests. The pattern's consistent across all your test cases, so no worries here. Just make sure the import path from my previous comment is sorted, and you're golden.Also applies to: 537-537, 566-566, 591-591, 612-612
95-95: Clone bound on H is spot-on—FakeHeader/TestHeader derive Clone. Verify your core Header type also implements Clone before merging.crates/pure-stage/src/stage_ref.rs (1)
80-82: Fair dinkum, this is a beaut addition!The
StageRef::namedconvenience constructor is a ripper of an ergonomic win—now you can spawn a StageRef from a string slice without manually building aNamefirst. Logic is spot-on (delegates to existingStageRef::new(Name::from(name))), and it's already getting proper usage in the test suite (e.g.,receive_header.rsline 223). No dramas here, mate.crates/pure-stage/src/tokio.rs (1)
146-146: She'll be right, this one checks out!Cloning
resourcesfromself.inner(line 146) and passing it as the 5th argument toEffects::new(lines 152-153) is the correct wiring to propagateResourcesinto the newEffectsinstance. This aligns perfectly with the PR's goal of supporting external_sync store operations via theresourcesfield. The signature ineffect.rs(lines 70-84) confirmsResourcesis now the 5th param, and both the tokio and simulation implementations are in sync. No worries here, mate.Also applies to: 152-153
crates/amaru-consensus/src/consensus/store.rs (1)
15-20: This refactor is mint, mate!Removing the public trait
StoreOpsand its methods (as noted in the AI summary) reduces the public API surface while keepingPraosChainStoreintact. The updated imports (line 15 now only brings inNonce,Point,GlobalParametersfrom amaru_kernel, droppingHeader,RawBlock,Peer) and the adjustedArcimport (line 20) align with the PR's goal of moving store operations to external effects. The remainingPraosChainStoreimpl (lines 24-118) and tests (lines 138-285) are rock solid. No syntax errors, no logic issues—just a cleaner, more encapsulated interface.crates/amaru-consensus/src/consensus/stages/receive_header.rs (3)
15-16: This is a ripper of a refactor, mate!Switching the stage to use the
ConsensusOpsabstraction (lines 15-16 imports, line 20 brings inStageRef, line 33 signature takeseff: impl ConsensusOps) is spot-on. The stage now delegates all effects viaeff.base()(lines 47-49, 53-63, 70-79, 82-84), which makes it testable in isolation without depending on a pure-stage runtime. This is exactly the vibe the PR is going for—trait-based ops for clean integration testing. No worries here!Also applies to: 20-20, 33-33
90-108: Beaut extraction, this decode_header function!Pulling
decode_headerout as a standalone instrumented function (lines 90-108) is a top-notch move. It's now a pure function that returnsResult<Header, ConsensusError>, making it easy to test and reason about. The instrumentation fields (lines 94-97) give you proper observability for the decoding step. No logic errors, no syntax dramas—just clean, testable code. Like separating the wheat from the chaff in a Coen Brothers film, mate!
110-227: These tests are absolutely mint!The new test suite (lines 110-227) is comprehensive and well-structured:
a_header_that_can_be_decoded_is_sent_downstream(lines 123-151) verifies happy patha_header_that_cannot_be_decoded_sends_an_error(lines 153-179) checks error handlingrollback_is_just_sent_downstream(lines 181-199) confirms passthroughcaughtup_is_just_sent_downstream(lines 201-218) confirms passthroughAll tests use
mock_consensus_opsand verify received messages viaconsensus_ops.mock_base.received(), which is the proper way to test stages in isolation. No missing cases, no logic errors—this is exactly the kind of test coverage you want after a big refactor. Fair dinkum, well done!crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (3)
26-39: This trait is the bee's knees!The
ConsensusOpstrait (lines 26-39) is a beauty—it defines a clean, composable surface for all consensus effects (store, network, ledger, base, metrics). Usingimpl Traitfor return types is idiomatic RPITIT (return position impl Trait in trait), which keeps the trait flexible and ergonomic. The sub-ops abstraction (store returnsArc<dyn ChainStore<Header>>, others return impl sub-traits) is exactly what you want for testable, decoupled stages. This is the heart of the refactor, and it's spot-on.
43-98: ConsensusEffects is a ripper of a wrapper!The
ConsensusEffects<T>struct (lines 43-54) wrapsEffects<T>andEraHistory, and the impl methods (lines 57-75) delegate to the corresponding sub-ops constructors (Store::new,Network::new,Ledger::new,Base::new,Metrics::new). TheConsensusOpsimpl (lines 78-98) just forwards to these methods—simple, clean delegation. No logic errors, no type issues, and it aligns perfectly with the PR's goal of trait-based ops. This is exactly how you build a composable, testable architecture. Mint work!
100-315: These test mocks are absolutely crackin'!The test module (lines 100-315) provides comprehensive mock implementations for testing:
MockConsensusOps(lines 117-147) usesInMemConsensusStoreand mock sub-opsMockNetworkOps(lines 149-201) hasreturn_blockto inject test responsesMockLedgerOps(lines 203-224) returns happy-path defaultsMockBaseOps(lines 226-289) tracks sent messages inArc<Mutex<BTreeMap>>for verificationMockMetricsOps(lines 291-304) is a no-op mockThe
mock_consensus_ops()helper (lines 306-314) wires it all up. This is exactly the kind of test infrastructure you need after a big refactor—it lets you test stages in isolation without a full runtime. No concurrency issues (Mutex usage is safe), no logic errors, and the API is ergonomic. Fair dinkum, this is top-shelf testing support!crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
44-55: This extraction is mint, mate!Pulling
HeadersTreeStateout as a separate struct (lines 57-94) is a ripper of a design move. The transient state (max_length, peers) is now cleanly separated from the persistent state (chain_store), which makes it easy to pass through stages likeselect_chain. Theinto_tree_statemethod (lines 52-54) consumes the tree and returns the state, which is the correct ownership transfer for this pattern. TheHeadersTreeState::initialize_peermethod (lines 79-93) is a sensible place to put peer initialization logic, and it still returnsResult<(), ConsensusError>for proper error handling.The serialization setup (line 58:
Serialize, Deserialize) is spot-on for persisting or passing this state around. All field accesses are consistently updated throughout the file (e.g.,self.tree_state.peers,self.tree_state.max_length), and the tests are updated to useHeadersTreeState::neworcreate(lines 706, 712-713, 717-718, 733-734). No logic errors, no type mismatches, no serialization dramas—this is exactly the kind of clean refactor you want. Fair dinkum, well done!Also applies to: 57-94
crates/amaru-consensus/src/consensus/stages/validate_header.rs (2)
15-16: This refactor is absolutely mint!The stage now uses the
ConsensusOpsabstraction (lines 15-16 imports, line 28 brings inStageRef, line 172 signature takeseff: impl ConsensusOps, line 173 obtains store viaeff.store()), which is exactly the vibe the PR is going for. Thehandle_roll_forwardsignature change (line 126 now takesstore: Arc<dyn ChainStore<Header>>param) is the correct way to pass the ChainStore dependency, and wrapping it withPraosChainStore::new(store)(line 132) is spot-on for Praos trait operations (evolve_nonce at line 132). The stage also delegates all message sending viaeff.base()(lines 185-186, 189-190, 205, 208-210). No logic errors, no type mismatches—this is clean, testable code that aligns perfectly with the trait-based ops refactor. Fair dinkum, well done!Also applies to: 26-26, 28-28, 126-126, 132-132, 172-173
220-445: These test updates are a ripper!The new test infrastructure (lines 220-445) is comprehensive and well-structured:
FailingStore(lines 241-344) is a proper test double that implements bothReadOnlyChainStoreandChainStore, delegating all operations to anInMemConsensusStoreinstance. This lets you inject failures for specific operations (e.g.,fail_on_evolve_nonceat line 254) while keeping the rest functional.create_test_datahelper (lines 347-389) now usesrun(any_header())(line 359), which is the correct way to generate test headers per the project's testing patterns.- Test cases (lines 391-416, 418-444) verify that
NoncesError::UnknownParentis raised whenevolve_noncefails, which is the expected error for missing parent nonces.The FailingStore's delegation pattern (lines 260-344) is clean and avoids duplication—each ChainStore method just forwards to
self.store.<method>. No logic errors, no test gaps, and the error verification (lines 408-415, 436-443) uses proper pattern matching. This is exactly the kind of test coverage you want after a big refactor. Mint work, mate!crates/amaru-consensus/src/consensus/stages/store_block.rs (1)
15-15: This stage conversion is spot-on, mate!The stage now uses the
ConsensusOpsabstraction (line 15 imports, line 21 brings inStageRef, line 37 signature takeseff: impl ConsensusOps), and the logic is clean:
- Line 47:
eff.store().store_block(&header.hash(), block)correctly stores the block via the ChainStore- Lines 49-54: Error handling converts
StoreErrortoanyhow::Errorviaanyhow!(e)(line 20 import), then sendsProcessingFailedto the errors stage—this is the proper way to propagate storage errors- Line 57: Rollback case just forwards the message via
eff.base().send, which is correct (no storage needed for rollback)No logic errors, no syntax dramas, and it aligns perfectly with the trait-based ops refactor. This is exactly how you wire up a stage to use external effects. Fair dinkum, well done!
Also applies to: 20-21, 37-37, 46-57
crates/amaru-consensus/Cargo.toml (1)
58-58: Sweet as, mate! Test utilities properly wired.The dev-dependency declaration for
amaru-ouroboros-traitswithtest-utilsis spot on, enabling the test scaffolding and mocks introduced across the PR. This aligns perfectly with the new trait-based testing architecture.crates/amaru/src/stages/consensus/forward_chain/tcp_forward_chain_server.rs (1)
20-20: LGTM! Module path reorganization.The import path for
ForwardEventandForwardEventListenerhas been updated to reflect the new re-export fromamaru_consensus::consensus::effects. No functional changes here, just following the module reorganization in the consensus effects layer. Dead easy!crates/amaru/src/stages/consensus/forward_chain/test_infra.rs (1)
19-19: Rippa! Import path updated.The import path for
ForwardEventandForwardEventListenerhas been updated to the new re-export location. The removal of commented-out code (not shown here) is also top-notch housekeeping.crates/amaru-consensus/src/consensus/mod.rs (1)
22-23: Beauty! Test module properly gated.The new
testsmodule is correctly gated with#[cfg(any(test, feature = "test-utils"))], making test utilities available for development while keeping them out of production builds. This aligns with the new trait-based testing architecture introduced in the PR.crates/amaru-consensus/src/consensus/tests.rs (3)
23-25: Choice test chain generator!The
any_headers_chainstrategy generatesnarbitrary headers and transforms them viamake_header()to create a valid parent-linked chain. This is a solid approach for property-based testing.
28-39: LGTM! Chain structure enforced correctly.The
make_header()helper iterates through the generated headers, setting sequentialblock_numberandslot, and linkingprev_hashto form a valid parent chain. The logic is sound.
42-68: Bonzer! Arbitrary header generator looks good.The
any_header()strategy generates aPseudoHeaderwith arbitraryblock_number,slot, and an optionalprev_hash(weighted at 1% probability). Placeholder values for keys, signatures, and certs are acceptable for property-based testing, where the structure matters more than the content. The emptyBytesand zeroHashare valid test data.Note: The 99% None weighting for
prev_hashmeans most generated headers start as orphans, butmake_header()correctly overwrites this to form a chain, so no worries there.crates/pure-stage/src/simulation.rs (1)
225-231: Good on ya! Effects wired with resources.The
Effects::newcall has been updated to includeself.resources.clone(), aligning with the broader refactor that introducesResourcesto support synchronous external effects for ChainStore operations. This matches the signature ineffect.rsand is consistent with the pattern used in other builders.crates/amaru-stores/Cargo.toml (1)
37-37: Ripper! Dev-dependency test-utils enabled.The dev-dependency declaration for
amaru-ouroboros-traitswithtest-utilsis spot on, consistent with the pattern incrates/amaru-consensus/Cargo.toml. This enables test scaffolding for development builds.crates/amaru-consensus/src/consensus/errors.rs (1)
131-196: All ProcessingFailed usages handle Option correctly
No direct struct literals or pattern matches remain; constructors and Display coverNonegracefully.crates/pure-stage/src/effect.rs (1)
209-217: Slick synchronous bridgeLine 209: love how this slots the store calls into a bullet-time lane—
external_syncgives us a tidy way to run the effect instantly without wiring up half the Matrix just for RocksDB pokes. Feels like hitting slow‑mo in Max Payne, but for consensus plumbing.crates/amaru-consensus/src/consensus/effects/ledger_effects.rs (3)
25-40: LGTM! Clean trait design for ledger ops.The
LedgerOpstrait provides a clear contract for validation and rollback, returning boxed futures for async ops. The nestedResult<Result<LedgerMetrics, BlockValidationError>, BlockValidationError>on line 33 is a bit of a head-scratcher at first glance (like that bit in Inception where we're dreaming inside a dream, mate), but it makes sense if the outerResultrepresents transport/external errors and the inner one represents ledger-specific validation failures. Just confirming: is that the intent? If so, might be worth a quick inline comment to clarify the distinction for future maintainers.Do you want me to verify whether the double-Result pattern is used consistently across the codebase for similar external effects, or would you prefer to add a brief doc comment explaining the error semantics here?
42-71: Solid delegation pattern, no dramas here.The
Ledgerwrapper cleanly delegates to external effects viaself.0.external(...). This pattern keeps the stage logic decoupled from the effects runtime, which is ace for testing. The lifetime'aties the wrapper to theEffectsreference, so it can't outlive the effects — good safety there, no worries.
96-109: ResourceBlockValidation registration verifiedResourceBlockValidation is registered in crates/amaru/src/stages/mod.rs; the
.expect()cannot panic in production.crates/amaru-consensus/src/consensus/effects/network_effects.rs (4)
29-49: NetworkOps trait looks choice — clean async contract.The
NetworkOpstrait gives you a tidy interface for fetching blocks and forwarding events (both forward and backward). The method signatures are consistent with the ledger ops pattern, returningBoxFuturefor async work. No dramas with the design here — it's as clean as a fresh-pressed latte, mate.
51-88: Network wrapper delegates like a boss.The
Networkwrapper follows the same clean delegation pattern asLedger. Each method routes to the appropriate external effect (FetchBlockEffectorForwardEventEffect), keeping the stage logic blissfully unaware of the runtime. It's like having a good mate who handles all the messy logistics while you just enjoy the ride — top stuff.
164-206: ForwardEventEffect is solid as a rock.The
ForwardEventEffectimplementation follows the same pattern as the other external effects: retrieve resource with.expect(), call the async method, wrap the result. Line 191 extracts the point for the error message, which is a nice touch — makes debugging heaps easier when things go pear-shaped. No worries here, mate.
117-131: ResourceBlockFetcher is safely registeredResourceBlockFetcher is inserted in both the main runtime (ClientsBlockFetcher) and simulation (FakeBlockFetcher), so the
.expect()won’t panic.
etorreborre
force-pushed
the
etorreborre/feat/more-encapsulated-effects
branch
from
2b924c8 to
1e27f9e
Compare
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
crates/amaru-stores/src/rocksdb/consensus.rs (1)
383-383: G'day cobber—this one's already been flagged!The import path here is still dodgy; you're pulling from
amaru_ouroboros_traits::fake::testswhen the actual module isamaru_ouroboros_traits::is_header::fake. Check the previous review comments—this'll cause a compilation error until it's sorted.For the record, the fix is:
-use amaru_ouroboros_traits::fake::tests::{any_fake_header, any_fake_headers_chain, run}; +use amaru_ouroboros_traits::is_header::fake::{any_fake_header, any_fake_headers_chain, run};crates/amaru/src/stages/build_stage_graph.rs (1)
15-27: Still missing the Future import, mate – build's gonna choke like Neo unplugging from the Matrix.The with_consensus_effects helper down at line 168 references
Futurein its generic bound, but we never invitedFutureto the party up here. The compiler's gonna bail with "cannot find typeFuturein this scope" faster than you can say "red pill".Apply this diff to fix it:
use amaru_consensus::consensus::effects::{ConsensusEffects, ResourceHeaderValidation}; use amaru_consensus::consensus::errors::{ProcessingFailed, ValidationFailed}; use amaru_consensus::consensus::events::ChainSyncEvent; use amaru_consensus::consensus::stages::select_chain::SelectChain; use amaru_consensus::consensus::stages::validate_header::ValidateHeader; use amaru_consensus::consensus::stages::{ fetch_block, forward_chain, receive_header, select_chain, store_block, store_header, validate_block, validate_header, }; use amaru_consensus::consensus::tip::HeaderTip; use amaru_kernel::protocol_parameters::GlobalParameters; use amaru_slot_arithmetic::EraHistory; use pure_stage::{Effects, SendData, StageGraph, StageRef}; +use std::future::Future;crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
232-247: Thisinitialize_peermethod looks solid, but...As I mentioned earlier, this is doing the same thing as
HeadersTreeState::initialize_peer(lines 79-93). Both methods:
- Check if the header exists in the store or is the ORIGIN_HASH.
- Collect ancestors in reverse order.
- Insert into peers map.
The main difference is this one uses
self.chain_storeandself.tree_state.peers, while the other takes the store as a parameter. It's like having two recipes for the same dish—one day you'll update one and forget the other, and then you're in for a bad time. Consider consolidating or at least adding a comment explaining why both exist.
🧹 Nitpick comments (10)
crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (1)
77-90: Clarify or remove the dodgy fallback inancestors_hashes
The fallback returns the input hash whenload_headerfails—spawning a ghost ancestor. Either document this behavior in the method’s doc comment or replace it with an empty iterator to avoid yielding non-existent hashes.crates/amaru/src/stages/build_stage_graph.rs (1)
162-173: Drop unnecessary Sync + Clone bounds on MsgConsensusEffects::new and Effects don’t impose Sync or Clone, and SendData doesn’t either—remove those traits from the Msg bound to avoid over-constraining.
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (1)
57-94: Extract shared logic from initialize_peer
The implementations inHeadersTreeState(line 79) andHeadersTree(line 232) are nearly identical—factor out the common behavior into a helper so they stay in sync.crates/amaru-consensus/src/consensus/stages/receive_header.rs (1)
166-177: Test is brittle: don’t assert the exact CBOR error stringMatching the decoder’s full reason string is a bit like speedrunning Elden Ring with a drift board—fun till one patch breaks it. Prefer asserting the variant and relevant fields, not the exact text.
Apply this refactor to assert variant presence without pinning the message:
- let error = ValidationFailed::new( - &peer, - ConsensusError::CannotDecodeHeader { - point: Point::Origin, - header: vec![1, 2, 3], - reason: "unexpected type u8 at position 0: expected array".to_string(), - }, - ); - assert_eq!( - consensus_ops.mock_base.received(), - BTreeMap::from_iter(vec![("errors".to_string(), vec![format!("{error:?}")])]) - ); + let sent = consensus_ops.mock_base.received(); + let errs = sent.get("errors").expect("errors channel"); + assert_eq!(errs.len(), 1); + // Keep it resilient to decoder changes: just check the variant shows up. + assert!( + errs[0].contains("CannotDecodeHeader") && errs[0].contains("Origin"), + "expected CannotDecodeHeader for Origin, got: {}", + errs[0] + );crates/amaru-consensus/src/consensus/stages/fetch_block.rs (1)
47-66: Add a bounded timeout (or ensure it exists in NetworkOps)A hanging fetch can stall the pipeline like waiting for Half-Life 3. Wrap fetch_block in a timeout or confirm it’s enforced in the network layer, and log a failure when timing out.
Based on learnings
crates/amaru-consensus/src/consensus/effects/metrics_effects.rs (1)
36-40: Relax impl bound; Clone on T seems unnecessaryMetrics::record only calls external(...) and doesn’t need T: Clone. Trim the bound to reduce constraints on Effects users.
-impl<T: Clone + SendData + Sync> MetricsOps for Metrics<'_, T> { +impl<T: SendData + Sync> MetricsOps for Metrics<'_, T> {crates/amaru-consensus/src/consensus/stages/validate_header.rs (3)
194-201: Duplicate span name: avoid double “consensus.roll_forward”Stage block creates a span with the same name as handle_roll_forward’s #[instrument]. Consider removing the manual span or renaming it (e.g., "stage.validate_header.roll_forward") to avoid noisy nesting.
Also applies to: 115-123
241-258: Dead flag in test helper: fail_on_evolve_nonce not usedThe FailingStore.fail_on_evolve_nonce field isn’t read anywhere. Either implement the toggle (e.g., force evolve_nonce failure) or drop the field to keep tests lean.
434-443: Fragile assertion on UnknownParent.parent vs point.hash()Comparing the UnknownParent’s parent to point.hash() (with point = Origin) likely isn’t stable across header generations. Just assert the variant occurs.
- match result.unwrap_err() { - NoncesError(consensus::store::NoncesError::UnknownParent { parent, .. }) - if parent == point.hash() => - { - // Expected error - } + match result.unwrap_err() { + NoncesError(consensus::store::NoncesError::UnknownParent { .. }) => { + // Expected error + }Can you confirm whether UnknownParent.parent is intended to be the header’s parent hash or derived from the provided point? If the former, we should compare to header.parent() instead of point.hash().
crates/amaru-consensus/src/consensus/effects/store_effects.rs (1)
150-160: Prefer wrap_sync for synchronous store callsAll these effects invoke immediate store methods. Using wrap_sync avoids an unnecessary async hop and guarantees external_sync’s now_or_never completes deterministically. It also trims a tiny bit of overhead.
Example change pattern:
- fn run(self: Box<Self>, resources: Resources) -> BoxFuture<'static, Box<dyn SendData>> { - Self::wrap(async move { - let store = resources.get::<ResourceHeaderStore>().expect("...").clone(); - store.set_anchor_hash(&self.hash) - }) - } + fn run(self: Box<Self>, resources: Resources) -> BoxFuture<'static, Box<dyn SendData>> { + let store = resources.get::<ResourceHeaderStore>().expect("...").clone(); + Self::wrap_sync(store.set_anchor_hash(&self.hash)) + }Apply similarly to: StoreBlockEffect, SetBestChainHashEffect, UpdateBestChainEffect, RemoveHeaderEffect, PutNoncesEffect, HasHeaderEffect, LoadHeaderEffect, GetChildrenEffect, GetAnchorHashEffect, GetBestChainHashEffect, LoadBlockEffect, LoadHeadersEffect, LoadNoncesEffect, LoadBlocksEffect, LoadParentsChildrenEffect, GetNoncesEffect.
Also applies to: 181-189, 209-217, 237-245, 265-274, 293-302, 321-331, 351-359, 379-387, 405-415, 433-441, 459-467, 485-495, 513-521, 539-547, 565-573, 591-599
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
crates/amaru-consensus/src/consensus/effects/base_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/ledger_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/metrics_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/network_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/store_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs(28 hunks)crates/amaru-consensus/src/consensus/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/fetch_block.rs(4 hunks)crates/amaru-consensus/src/consensus/stages/forward_chain.rs(4 hunks)crates/amaru-consensus/src/consensus/stages/receive_header.rs(3 hunks)crates/amaru-consensus/src/consensus/stages/select_chain.rs(7 hunks)crates/amaru-consensus/src/consensus/stages/store_header.rs(2 hunks)crates/amaru-consensus/src/consensus/stages/validate_header.rs(10 hunks)crates/amaru-consensus/src/consensus/store.rs(1 hunks)crates/amaru-consensus/src/consensus/tests.rs(1 hunks)crates/amaru-ouroboros-traits/src/is_header/fake.rs(2 hunks)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs(2 hunks)crates/amaru-stores/Cargo.toml(2 hunks)crates/amaru-stores/src/rocksdb/consensus.rs(8 hunks)crates/amaru/src/stages/build_stage_graph.rs(3 hunks)crates/pure-stage/src/effect.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- crates/amaru-ouroboros-traits/src/is_header/fake.rs
- crates/amaru-consensus/src/consensus/effects/base_effects.rs
- crates/amaru-consensus/src/consensus/effects/consensus_effects.rs
🧰 Additional context used
🧠 Learnings (6)
📚 Learning: 2025-09-05T17:30:55.869Z
Learnt from: etorreborre
PR: pragma-org/amaru#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/fetch_block.rs
📚 Learning: 2025-08-20T13:02:25.763Z
Learnt from: jeluard
PR: pragma-org/amaru#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/Cargo.toml
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
PR: pragma-org/amaru#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.rscrates/amaru-consensus/src/consensus/stages/validate_header.rs
📚 Learning: 2025-08-23T15:44:44.318Z
Learnt from: rkuhn
PR: pragma-org/amaru#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-stores/src/rocksdb/consensus.rscrates/amaru-consensus/src/consensus/tests.rscrates/amaru-consensus/src/consensus/stages/validate_header.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, ValidateHeader::handle_roll_forward returns a Result<PullEvent, ConsensusError>, not ValidateHeaderEvent as might be expected from the older code structure.
Applied to files:
crates/amaru-consensus/src/consensus/stages/validate_header.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
PR: pragma-org/amaru#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.rs
🧬 Code graph analysis (15)
crates/amaru-consensus/src/consensus/stages/fetch_block.rs (5)
crates/amaru-consensus/src/consensus/span.rs (2)
adopt_current_span(32-36)adopt_current_span(97-99)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (3)
new(50-55)mock_consensus_ops(306-314)received(232-234)crates/amaru-consensus/src/consensus/stages/select_chain.rs (3)
new(49-52)new(70-76)stage(317-340)crates/amaru-consensus/src/consensus/stages/validate_block.rs (1)
stage(35-115)crates/amaru-consensus/src/consensus/stages/store_block.rs (1)
stage(34-60)
crates/amaru-consensus/src/consensus/stages/receive_header.rs (7)
crates/amaru-consensus/src/consensus/span.rs (2)
adopt_current_span(32-36)adopt_current_span(97-99)crates/pure-stage/src/stage_ref.rs (2)
name(84-86)named(80-82)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (2)
mock_consensus_ops(306-314)received(232-234)crates/amaru-consensus/src/consensus/stages/fetch_block.rs (1)
stage(38-88)crates/amaru-consensus/src/consensus/stages/forward_chain.rs (1)
stage(43-113)crates/amaru-consensus/src/consensus/stages/store_header.rs (1)
stage(26-46)crates/amaru-consensus/src/consensus/stages/validate_header.rs (1)
stage(172-218)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (5)
crates/amaru-kernel/src/point.rs (1)
hash(34-40)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (5)
new(50-55)store(29-29)store(57-59)store(79-81)store(128-130)crates/pure-stage/src/effect.rs (8)
new(71-85)new(340-342)external_sync(210-217)run(251-251)run(322-324)run(361-363)wrap_sync(267-274)wrap(254-264)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (30)
era_history(153-153)load_header(28-28)load_header(94-96)load_headers(29-29)load_headers(98-100)load_nonces(30-30)load_nonces(102-104)load_blocks(31-31)load_blocks(106-108)load_parents_children(32-34)load_parents_children(110-114)get_children(35-35)get_children(116-118)get_anchor_hash(36-36)get_anchor_hash(120-122)get_best_chain_hash(37-37)get_best_chain_hash(124-126)load_block(38-38)load_block(128-130)get_nonces(39-39)get_nonces(132-134)has_header(40-40)has_header(136-138)set_anchor_hash(147-147)set_best_chain_hash(148-148)update_best_chain(149-149)store_header(146-146)store_block(151-151)remove_header(150-150)put_nonces(152-152)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (19)
era_history(199-201)load_header(73-76)load_headers(79-89)load_nonces(92-102)load_blocks(105-115)load_parents_children(118-130)get_children(155-162)get_anchor_hash(165-168)get_best_chain_hash(171-174)load_block(139-146)get_nonces(133-136)has_header(149-152)set_anchor_hash(233-237)set_best_chain_hash(240-244)update_best_chain(247-252)store_header(179-190)store_block(204-208)remove_header(211-230)put_nonces(193-197)
crates/amaru/src/stages/build_stage_graph.rs (4)
crates/amaru-ledger/src/state.rs (2)
era_history(227-229)network(223-225)crates/amaru-consensus/src/consensus/effects/store_effects.rs (12)
era_history(129-131)new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)new(344-346)new(372-374)new(400-402)crates/pure-stage/src/simulation.rs (2)
stage(210-262)state(234-234)crates/pure-stage/src/tokio.rs (1)
stage(111-132)
crates/amaru-stores/src/rocksdb/consensus.rs (1)
crates/amaru-ouroboros-traits/src/is_header/fake.rs (3)
any_fake_header(203-216)any_fake_headers_chain(180-182)run(220-223)
crates/amaru-consensus/src/consensus/stages/select_chain.rs (4)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (4)
new(65-70)new(201-210)peers(74-76)create(219-227)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (5)
store(29-29)store(57-59)store(79-81)store(128-130)mock_consensus_ops(306-314)crates/amaru-consensus/src/consensus/tests.rs (2)
any_header(42-68)any_headers_chain(23-25)crates/amaru-consensus/src/consensus/stages/store_header.rs (1)
stage(26-46)
crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (1)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (2)
anchor(425-427)ancestors_hashes(495-497)
crates/amaru-consensus/src/consensus/tests.rs (1)
crates/amaru-ouroboros-traits/src/is_header/fake.rs (1)
any_header_hash(198-200)
crates/amaru-consensus/src/consensus/stages/validate_header.rs (5)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (4)
store(29-29)store(57-59)store(79-81)store(128-130)crates/amaru-consensus/src/consensus/effects/store_effects.rs (55)
store(519-519)store(545-545)store(571-571)store(597-597)new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)new(344-346)new(372-374)new(400-402)new(426-428)new(452-454)new(480-482)new(506-508)new(532-534)run(152-160)run(181-189)run(209-217)run(237-245)run(266-274)run(294-302)run(323-331)run(351-359)run(379-387)run(407-415)run(433-441)run(459-467)run(487-495)run(513-521)run(539-547)run(565-573)has_header(95-97)load_header(50-52)get_children(75-77)get_anchor_hash(79-81)get_best_chain_hash(83-85)load_block(87-89)load_headers(54-56)load_nonces(58-60)load_blocks(62-64)load_parents_children(66-73)get_nonces(91-93)set_anchor_hash(101-103)set_best_chain_hash(105-107)update_best_chain(109-111)store_header(113-115)store_block(117-119)remove_header(121-123)put_nonces(125-127)era_history(129-131)crates/amaru-consensus/src/consensus/tests.rs (1)
any_header(42-68)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (30)
has_header(40-40)has_header(136-138)load_header(28-28)load_header(94-96)get_children(35-35)get_children(116-118)get_anchor_hash(36-36)get_anchor_hash(120-122)get_best_chain_hash(37-37)get_best_chain_hash(124-126)load_block(38-38)load_block(128-130)load_headers(29-29)load_headers(98-100)load_nonces(30-30)load_nonces(102-104)load_blocks(31-31)load_blocks(106-108)load_parents_children(32-34)load_parents_children(110-114)get_nonces(39-39)get_nonces(132-134)set_anchor_hash(147-147)set_best_chain_hash(148-148)update_best_chain(149-149)store_header(146-146)store_block(151-151)remove_header(150-150)put_nonces(152-152)era_history(153-153)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (19)
has_header(149-152)load_header(73-76)get_children(155-162)get_anchor_hash(165-168)get_best_chain_hash(171-174)load_block(139-146)load_headers(79-89)load_nonces(92-102)load_blocks(105-115)load_parents_children(118-130)get_nonces(133-136)set_anchor_hash(233-237)set_best_chain_hash(240-244)update_best_chain(247-252)store_header(179-190)store_block(204-208)remove_header(211-230)put_nonces(193-197)era_history(199-201)
crates/amaru-consensus/src/consensus/stages/store_header.rs (2)
crates/amaru-consensus/src/consensus/span.rs (2)
adopt_current_span(32-36)adopt_current_span(97-99)crates/amaru-consensus/src/consensus/events.rs (2)
peer(101-107)peer(261-267)
crates/amaru-consensus/src/consensus/effects/ledger_effects.rs (2)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (7)
ledger(34-34)ledger(65-67)ledger(87-89)ledger(136-138)validate(207-215)rollback(217-223)new(50-55)crates/amaru-consensus/src/consensus/effects/network_effects.rs (8)
point(147-152)new(55-57)new(109-114)new(171-176)run(119-130)run(181-201)resources(124-125)resources(186-187)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (4)
crates/amaru-consensus/src/consensus/stages/select_chain.rs (4)
select_chain(105-142)select_chain(488-493)new(49-52)new(70-76)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (4)
store(29-29)store(57-59)store(79-81)store(128-130)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
load_headers(29-29)load_headers(98-100)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
load_headers(79-89)
crates/amaru-consensus/src/consensus/stages/forward_chain.rs (5)
crates/amaru-consensus/src/consensus/stages/fetch_block.rs (2)
stage(38-88)new(125-133)crates/amaru-consensus/src/consensus/stages/validate_block.rs (1)
stage(35-115)crates/amaru-consensus/src/consensus/effects/base_effects.rs (1)
new(36-38)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (1)
new(50-55)crates/amaru-consensus/src/consensus/effects/network_effects.rs (3)
new(55-57)new(109-114)new(171-176)
crates/amaru-consensus/src/consensus/effects/network_effects.rs (3)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (4)
fetch_block(171-184)send_forward_event(186-192)send_backward_event(194-200)new(50-55)crates/amaru-consensus/src/consensus/stages/fetch_block.rs (3)
fetch_block(93-93)fetch_block(138-140)new(125-133)crates/pure-stage/src/effect.rs (1)
wrap(254-264)
crates/amaru-consensus/src/consensus/effects/metrics_effects.rs (2)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (3)
record(295-297)record(301-303)new(50-55)crates/amaru-consensus/src/consensus/effects/ledger_effects.rs (3)
new(46-48)new(87-93)new(122-127)
⏰ 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 aarch64/macos
- GitHub Check: Build aarch64/linux
- GitHub Check: Build x86_64/windows
- GitHub Check: Build x86_64/linux
- GitHub Check: Test the CLI
- GitHub Check: Test coverage
- GitHub Check: Benches
- GitHub Check: End-to-end snapshot tests (preprod, 1, 10.1.4)
- GitHub Check: Analyze (rust)
🔇 Additional comments (31)
crates/amaru-stores/src/rocksdb/consensus.rs (2)
95-95: Clean addition of theClonebound, legend!You've added the
Cloneconstraint to both theReadOnlyChainStoremacro and theChainStoreimpl consistently. This aligns with the PR's shift toward theConsensusOpstrait surface—no dramas here.Also applies to: 273-273
489-492: Top-notch test refactor, mate!You've switched from the old
any_headers_chain_sizedto the newany_fake_headers_chainhelper, and it's all looking solid—clean usage ofrunto generate test data. Once you sort the import path (flagged above), these tests will be golden.Also applies to: 537-543, 566-566, 591-591, 612-612
crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
21-21: Sweet as, mate! Import looks spot on.The
successorsimport is the right tool for building the ancestor traversal iterator. No dramas here.
61-75: Approveancestorsas-is. Traversal cleanly yields headers up to the anchor and includes the start; cycle detection falls under upstream data validation, so no changes needed here.crates/amaru/src/stages/build_stage_graph.rs (2)
34-34: EraHistory injection looks solid – nice and clean like a freshly-poured Guinness.The
&'staticlifetime is the right call here since you're threading this through into stage closures that need to be'static + Send. Matches the pattern from the store_effects wiring too.
40-71: Stage wiring with ConsensusEffects wrapper looks mint – consistent as a speedrunner's route.Every stage gets the era_history injection via
with_consensus_effects, and the pattern's uniform across all eight stages. The wrapper signature lines up with the simulation/tokio stage builders from the relevant snippets, so we're golden.crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (6)
50-55: Beauty! Thisinto_tree_statemethod is clean as a whistle.Standard Rust ownership pattern—consumes the tree, hands over the transient state. Perfect for the select_chain flow where you need to shuffle state between stages without cloning the whole store. It's like passing the baton in a relay race, but with less spandex.
200-210: Solid move gating this under test-utils, mate.The
newconstructor is now properly wrapped in#[cfg(any(test, feature = "test-utils"))], which makes sense since production code should use the more explicitcreatemethod. Theassert!on line 202 is a good guardrail—can't have a headers tree with a max length shorter than your average Hobbit's attention span.One minor nitpick: the
retrieve_best_chain()call on line 207 feels like it's doing some I/O under the hood. Just make sure that's kosher in a test context where you might want deterministic behaviour without side effects.
219-227: Nice one! Thecreatemethod is now the main entry point.Taking
tree_state: HeadersTreeStateas a parameter instead of deriving it internally is a cleaner API—it's like bringing your own ingredients to a potluck instead of hoping the host has what you need. Thepub(crate)visibility is spot on for internal wiring.
158-660: Thetree_stateusage throughout the file is consistent as a Witcher's contract.Every access to
peersandmax_lengthnow goes throughself.tree_state, maintaining proper encapsulation. No sneaky direct field access—it's all above board. The refactoring is thorough and doesn't leave any orphaned references to the old structure.
710-1321: Test updates are looking ace, mate!All the tests have been properly refactored to use the new
HeadersTree::createconstructor with explicitHeadersTreeState::new(10)initialization. It's like upgrading from cassette tapes to streaming—same content, better infrastructure. The test coverage seems intact, and the changes are systematic.
126-136: The'staticbounds on Debug and Display impls make sense in context.Adding
'staticto these trait impls is likely driven by the new trait-based ConsensusOps surface introduced in the PR. Since these operations need to be Send + Sync + 'static to work across async boundaries, the types they operate on need the same guarantees. It's like making sure all your party guests have valid IDs before letting them into the VIP section.crates/amaru-consensus/src/consensus/mod.rs (1)
22-23: Top shelf move exposing the tests module under test-utils!This is exactly the kind of thing you want when building integration tests—exposing the test scaffolding so other crates can use it without polluting the production binary. It's like having a secret menu at In-N-Out—only the cool kids know about it, but when you need it, it's there.
crates/amaru-consensus/src/consensus/store.rs (1)
15-20: Clean import tidy-up, mate!Removing the unused imports (Header, RawBlock, Peer) and simplifying the Arc import is good housekeeping. With the StoreOps trait gone (moved to the new ConsensusOps surface), these imports are no longer needed. It's like Marie Kondo-ing your dependencies—if it doesn't spark joy (or compile), chuck it out.
crates/amaru-stores/Cargo.toml (1)
27-37: Explicit feature declarations are a win, champion!Setting
features = []on line 27 andfeatures = ["test-utils"]on line 37 makes the dependency intent crystal clear—like labeling your Tupperware so you don't accidentally eat last week's curry. The test-utils feature in dev-dependencies aligns with the PR's broader pattern of enabling test scaffolding for integration tests.Based on learnings, amaru-ouroboros-traits is used in main code (not just tests), so keeping it as a regular dependency is correct.
crates/amaru-consensus/src/consensus/stages/store_header.rs (2)
15-29: Beautiful trait-based refactor, mate!Switching from a concrete
Effectstype toimpl ConsensusOpsis a textbook dependency injection move—like swapping out your engine for a test rig so you can run diagnostics without burning real fuel. The imports are tidy, and the signature change sets up the stage for mockable testing.
34-43: Effect routing is spot on, cobber!All the operations are now channeled through the trait-based interface—
eff.store()for storage,eff.base()for messaging and termination. It's like having a universal remote instead of juggling three separate ones. The consistency is top-notch, and this makes mocking out individual subsystems a breeze for testing.crates/amaru-consensus/src/consensus/stages/forward_chain.rs (2)
15-43: Ripper refactor on the signature, mate!The switch to
impl ConsensusOpsis consistent with the other stages in the PR—like upgrading all your kitchen appliances to the same smart home system so they actually talk to each other. The imports are clean, and the trait-based approach is going to make testing this stage much easier.
61-101: Network effect routing is clean as a whistle!All the forward and backward event dispatch now goes through
eff.network().send_forward_eventandeff.network().send_backward_event—no more directForwardEventEffectwrangling. Error handling consistently routes througheff.base().send, which keeps the error-propagation path predictable. It's like having a proper filing system instead of just chucking papers in a drawer and hoping for the best.crates/amaru-consensus/src/consensus/tests.rs (3)
22-39: Top-notch property-based test helper, champion!The
any_headers_chaingenerator combined withmake_headeris a clean way to generate valid header chains for proptest. The logic for linking headers (settingblock_number,slot, andprev_hash) is spot on—like assembling IKEA furniture but with fewer leftover screws. The parent tracking in the loop ensures each header points to the previous one correctly.
41-68: Any_header generator is solid, but watch those placeholders.The generator creates valid
Headerinstances with all the required fields, which is exactly what you want for property-based testing. Theprop::option::weighted(0.01, any_header_hash())on line 46 is a nice touch—99% of headers will have a parent, which mimics realistic chains.One thing to eyeball: the placeholder values for
issuer_vkey,vrf_vkey,vrf_result, andoperational_cert(lines 53-63) are all empty or zero-filled. If any validation logic in the consensus pipeline expects these fields to be well-formed (e.g., valid signatures, non-empty keys), tests using these headers might pass when they shouldn't. Based on learnings, FakeHeader in test code is designed to panic on malformed data to catch issues quickly—so if you're using this for integration tests, make sure the downstream validation can handle these placeholders gracefully.
15-20: Imports are tidy and purposeful!All the imports are necessary for the test generators—no cruft, no orphans. The
any_header_hashimport fromamaru_ouroboros_traits::fake::testsis exactly where you'd expect test utilities to live when thetest-utilsfeature is enabled.crates/pure-stage/src/effect.rs (3)
35-83: Resources field integration is smooth as butter!Adding the
resources: Resourcesfield toEffects<M>(line 58) and updating the constructor (lines 76-83) is a clean way to give effects access to shared services like stores, clocks, and other infrastructure. TheCloneimpl is properly updated on line 42 to include the new field, which is critical for avoiding subtle bugs when cloning effects. It's like upgrading your toolbelt—now every effect has access to the full workshop.
206-217: External_sync is a handy addition, but don't forget that TODO.This method provides a synchronous path for external effects that complete immediately—like using a vending machine instead of ordering delivery. The
now_or_never()call on line 212 is the key bit: it expects the future to resolve instantly, and panics if it doesn't. The expect messages on lines 214 and 216 are clear and helpful.However, the TODO on line 209 is a proper heads-up: "the call to this effect is not traced so we won't see it executed in the TraceBuffer." That's a significant observability gap—synchronous effects are invisible to your tracing infrastructure, which could make debugging a right pain. If you're using this in production paths, make sure you're okay with that blind spot, or add a follow-up issue to track this work.
Consider whether the lack of tracing for sync effects is acceptable in the current use cases, or if you need to prioritize that TODO.
266-274: Wrap_sync is a neat little helper, mate!This helper method standardizes how synchronous responses are wrapped into the same
BoxFuture<Box<dyn SendData>>type that async effects use. Thefuture::readycall on line 273 creates an immediately-resolved future, which is exactly what you want for sync effects. It's like having a converter plug so your sync and async effects can use the same socket—keeps the API consistent and predictable.crates/amaru-consensus/src/consensus/stages/receive_header.rs (2)
53-64: ConsensusOps routing looks spot onNice pivot to eff.base().send(...) across all branches. Clean, consistent, and keeps the stage pure. Good on ya.
Also applies to: 70-79, 82-84
90-108: decode_header: clear error mapping and useful span fieldsCBOR decode → MintedHeader → Header with a precise CannotDecodeHeader payload and span fields on slot/hash. Solid.
crates/amaru-consensus/src/consensus/stages/fetch_block.rs (2)
38-61: Stage wiring: clean and cohesiveThe hand-off to ValidateBlockEvent on success and ValidationFailed on error is tidy. LGTM.
Also applies to: 75-84
118-120: Prefer structured logging Replace positional formatting with structured fields so logs are easier to query:- error!(target: "amaru::consensus", "failed to fetch block from peer {}: {}", peer.name, e); + error!(target: "amaru::consensus", %peer, error = %e, "failed to fetch block");crates/amaru-consensus/src/consensus/stages/validate_header.rs (1)
167-218: Stage refactor to ConsensusOps: 👍Grabs store via eff.store(), routes via eff.base().send; clear separation of concerns. Nicely done.
crates/amaru-consensus/src/consensus/effects/store_effects.rs (1)
32-47: Store wrapper: clear, cohesive APINeat encapsulation over Effects with external_sync, and trait impls line up with ChainStore/ReadOnlyChainStore. Looks good.
Also applies to: 49-98, 100-132
Signed-off-by: etorreborre <etorreborre@yahoo.com>
etorreborre
force-pushed
the
etorreborre/feat/more-encapsulated-effects
branch
from
1e27f9e to
73f5c60
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (3)
crates/amaru/src/stages/build_stage_graph.rs (1)
165-173: Pull inFutureto unblock the build.We’re name-dropping
Futurein the bounds, but without the import the compiler goes full Mad Max and stares you down with “cannot find typeFuturein this scope.” Toss in the std import so the stage graph keeps rolling.+use std::future::Future;crates/amaru-stores/src/rocksdb/consensus.rs (1)
383-383: Fix the import path, cobber!As flagged in the previous review, the import path is dodgy. Should be
amaru_ouroboros_traits::is_header::fake::testsnotamaru_ouroboros_traits::fake::tests.Apply this diff to fix the import path:
-use amaru_ouroboros_traits::fake::tests::{any_fake_header, any_fake_headers_chain, run}; +use amaru_ouroboros_traits::is_header::fake::tests::{any_fake_header, any_fake_headers_chain, run};crates/amaru-consensus/src/consensus/stages/select_chain.rs (1)
112-117: Oi, we've still got the Master Sword down the well, cobber!The past review flagged that
?was yeetingself.tree_statebefore restoration, and this hasn't been fully fixed. Line 115 still doesselect_roll_forward(&peer, header)?which means we bail before line 116 can restoreself.tree_statefromheaders_tree.into_tree_state(). So ifselect_roll_forwarderrors out, you're left holding an empty default tree_state — basically dropping all your progress like a speedrunner who forgot to save.The fix is to capture the
Resultfirst (without?), restore the tree_state unconditionally, then propagate the error. Same deal forselect_rollbackbelow (lines 151-156).Apply this diff to fix the error handling in
select_chain:- let result = headers_tree.select_roll_forward(&peer, header)?; - self.tree_state = headers_tree.into_tree_state(); + let result = headers_tree.select_roll_forward(&peer, header); + self.tree_state = headers_tree.into_tree_state(); + let result = result?;And this diff for
select_rollback:- let result = headers_tree.select_rollback(&peer, &rollback_point.hash())?; - self.tree_state = headers_tree.into_tree_state(); + let result = headers_tree.select_rollback(&peer, &rollback_point.hash()); + self.tree_state = headers_tree.into_tree_state(); + let result = result?;
🧹 Nitpick comments (2)
crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (1)
88-89: Swap the fallback vec foronceto skip the detourWhen the header isn’t in the store we spin up a whole
Vecjust to yield a single hash, which is like booting a gaming rig to play Minesweeper.iter::once(*hash)nails the same behaviour without the heap churn.crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (1)
237-289: Remove redundantimpl BaseOps for &MockBaseOps
The reference impl (lines 264-289) isn’t invoked anywhere—drop it to trim dead code.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (22)
crates/amaru-consensus/src/consensus/effects/base_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/ledger_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/metrics_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/network_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/effects/store_effects.rs(1 hunks)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs(28 hunks)crates/amaru-consensus/src/consensus/mod.rs(1 hunks)crates/amaru-consensus/src/consensus/stages/fetch_block.rs(4 hunks)crates/amaru-consensus/src/consensus/stages/forward_chain.rs(4 hunks)crates/amaru-consensus/src/consensus/stages/receive_header.rs(3 hunks)crates/amaru-consensus/src/consensus/stages/select_chain.rs(7 hunks)crates/amaru-consensus/src/consensus/stages/store_header.rs(2 hunks)crates/amaru-consensus/src/consensus/stages/validate_header.rs(10 hunks)crates/amaru-consensus/src/consensus/store.rs(1 hunks)crates/amaru-consensus/src/consensus/tests.rs(1 hunks)crates/amaru-ouroboros-traits/src/is_header/fake.rs(2 hunks)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs(2 hunks)crates/amaru-stores/Cargo.toml(2 hunks)crates/amaru-stores/src/rocksdb/consensus.rs(8 hunks)crates/amaru/src/stages/build_stage_graph.rs(3 hunks)crates/pure-stage/src/effect.rs(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- crates/amaru-consensus/src/consensus/stages/receive_header.rs
- crates/amaru-consensus/src/consensus/tests.rs
🧰 Additional context used
🧠 Learnings (5)
📚 Learning: 2025-08-23T15:44:44.318Z
Learnt from: rkuhn
PR: pragma-org/amaru#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-ouroboros-traits/src/is_header/fake.rscrates/amaru-consensus/src/consensus/stages/validate_header.rscrates/amaru-stores/src/rocksdb/consensus.rs
📚 Learning: 2025-09-05T17:30:55.869Z
Learnt from: etorreborre
PR: pragma-org/amaru#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/fetch_block.rs
📚 Learning: 2025-04-22T09:18:19.893Z
Learnt from: abailly
PR: pragma-org/amaru#195
File: simulation/amaru-sim/src/simulator/mod.rs:167-182
Timestamp: 2025-04-22T09:18:19.893Z
Learning: In the Amaru consensus pipeline refactor, ValidateHeader::handle_roll_forward returns a Result<PullEvent, ConsensusError>, not ValidateHeaderEvent as might be expected from the older code structure.
Applied to files:
crates/amaru-consensus/src/consensus/stages/validate_header.rs
📚 Learning: 2025-06-24T06:36:05.931Z
Learnt from: abailly
PR: pragma-org/amaru#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.rs
📚 Learning: 2025-05-21T18:58:48.631Z
Learnt from: abailly
PR: pragma-org/amaru#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.rs
🧬 Code graph analysis (16)
crates/amaru/src/stages/build_stage_graph.rs (4)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (13)
era_history(129-131)store_header(113-115)new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)new(344-346)new(372-374)new(400-402)crates/amaru-consensus/src/consensus/stages/store_header.rs (1)
stage(26-46)crates/pure-stage/src/simulation.rs (2)
stage(210-262)state(234-234)crates/pure-stage/src/tokio.rs (1)
stage(111-132)
crates/amaru-consensus/src/consensus/stages/fetch_block.rs (2)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (3)
new(50-55)mock_consensus_ops(306-314)received(232-234)crates/amaru-consensus/src/consensus/tests.rs (1)
any_header(42-68)
crates/amaru-consensus/src/consensus/stages/store_header.rs (2)
crates/amaru-consensus/src/consensus/span.rs (2)
adopt_current_span(32-36)adopt_current_span(97-99)crates/amaru-consensus/src/consensus/events.rs (2)
peer(101-107)peer(261-267)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (7)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (17)
store(519-519)store(545-545)store(571-571)store(597-597)new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)new(344-346)new(372-374)new(400-402)new(426-428)era_history(129-131)crates/amaru-consensus/src/consensus/effects/ledger_effects.rs (7)
new(46-48)new(87-93)new(122-127)validate(28-33)validate(52-61)rollback(35-39)rollback(63-70)crates/amaru-consensus/src/consensus/effects/network_effects.rs (8)
fetch_block(32-36)fetch_block(61-67)point(147-152)send_forward_event(38-42)send_forward_event(69-76)send_backward_event(44-48)send_backward_event(78-87)send(137-137)crates/amaru-consensus/src/consensus/effects/base_effects.rs (8)
send(21-25)send(42-48)clock(26-26)clock(50-52)wait(27-27)wait(54-56)terminate(28-28)terminate(58-60)crates/pure-stage/src/effect.rs (4)
send(105-111)clock(114-119)wait(122-127)terminate(235-237)crates/pure-stage/src/time.rs (1)
at_offset(112-114)crates/amaru-consensus/src/consensus/effects/metrics_effects.rs (2)
record(23-23)record(37-39)
crates/amaru-consensus/src/consensus/stages/validate_header.rs (6)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (4)
store(29-29)store(57-59)store(79-81)store(128-130)crates/amaru-consensus/src/consensus/effects/store_effects.rs (27)
store(519-519)store(545-545)store(571-571)store(597-597)new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)new(344-346)new(372-374)new(400-402)new(426-428)new(452-454)new(480-482)new(506-508)new(532-534)load_header(50-52)load_headers(54-56)load_nonces(58-60)load_parents_children(66-73)get_nonces(91-93)store_header(113-115)era_history(129-131)crates/amaru-consensus/src/consensus/stages/select_chain.rs (1)
stage(317-340)crates/amaru-consensus/src/consensus/stages/store_header.rs (1)
stage(26-46)crates/amaru-consensus/src/consensus/tests.rs (1)
any_header(42-68)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (7)
load_header(73-76)load_headers(79-89)load_nonces(92-102)load_parents_children(118-130)get_nonces(133-136)store_header(179-190)era_history(199-201)
crates/amaru-consensus/src/consensus/stages/forward_chain.rs (6)
crates/amaru-consensus/src/consensus/stages/fetch_block.rs (2)
stage(37-87)new(124-132)crates/amaru-consensus/src/consensus/stages/receive_header.rs (1)
stage(30-88)crates/amaru-consensus/src/consensus/stages/select_chain.rs (1)
stage(317-340)crates/amaru-consensus/src/consensus/stages/store_header.rs (1)
stage(26-46)crates/amaru-consensus/src/consensus/stages/validate_block.rs (1)
stage(35-115)crates/amaru-consensus/src/consensus/effects/network_effects.rs (3)
new(55-57)new(109-114)new(171-176)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (6)
crates/amaru-consensus/src/consensus/stages/select_chain.rs (4)
select_chain(105-142)select_chain(488-493)new(49-52)new(70-76)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (4)
store(29-29)store(57-59)store(79-81)store(128-130)crates/amaru-consensus/src/consensus/tip.rs (2)
hash(96-101)hash(167-169)crates/amaru-kernel/src/point.rs (1)
hash(34-40)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
load_headers(29-29)load_headers(98-100)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (1)
load_headers(79-89)
crates/amaru-consensus/src/consensus/effects/base_effects.rs (2)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (9)
send(238-249)send(265-276)clock(251-253)clock(278-280)wait(255-257)wait(282-284)terminate(259-261)terminate(286-288)new(50-55)crates/pure-stage/src/effect.rs (4)
send(105-111)clock(114-119)wait(122-127)terminate(235-237)
crates/amaru-consensus/src/consensus/effects/metrics_effects.rs (2)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (3)
record(295-297)record(301-303)new(50-55)crates/amaru-consensus/src/consensus/effects/ledger_effects.rs (3)
new(46-48)new(87-93)new(122-127)
crates/amaru-stores/src/rocksdb/consensus.rs (1)
crates/amaru-ouroboros-traits/src/is_header/fake.rs (3)
any_fake_header(203-216)any_fake_headers_chain(180-182)run(220-223)
crates/amaru-consensus/src/consensus/effects/store_effects.rs (6)
crates/amaru-kernel/src/point.rs (1)
hash(34-40)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (5)
new(50-55)store(29-29)store(57-59)store(79-81)store(128-130)crates/amaru-stores/src/rocksdb/consensus.rs (13)
new(38-55)era_history(301-303)load_nonces(564-586)load_blocks(589-605)load_parents_children(532-561)set_anchor_hash(319-325)set_best_chain_hash(327-333)update_best_chain(334-347)update_best_chain(473-481)store_header(275-291)store_block(305-311)remove_header(313-317)put_nonces(293-299)crates/pure-stage/src/effect.rs (8)
new(71-85)new(340-342)external_sync(210-217)run(251-251)run(322-324)run(361-363)wrap_sync(267-274)wrap(254-264)crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (30)
era_history(153-153)load_header(28-28)load_header(94-96)load_headers(29-29)load_headers(98-100)load_nonces(30-30)load_nonces(102-104)load_blocks(31-31)load_blocks(106-108)load_parents_children(32-34)load_parents_children(110-114)get_children(35-35)get_children(116-118)get_anchor_hash(36-36)get_anchor_hash(120-122)get_best_chain_hash(37-37)get_best_chain_hash(124-126)load_block(38-38)load_block(128-130)get_nonces(39-39)get_nonces(132-134)has_header(40-40)has_header(136-138)set_anchor_hash(147-147)set_best_chain_hash(148-148)update_best_chain(149-149)store_header(146-146)store_block(151-151)remove_header(150-150)put_nonces(152-152)crates/amaru-ouroboros-traits/src/stores/consensus/in_memory_consensus_store.rs (19)
era_history(199-201)load_header(73-76)load_headers(79-89)load_nonces(92-102)load_blocks(105-115)load_parents_children(118-130)get_children(155-162)get_anchor_hash(165-168)get_best_chain_hash(171-174)load_block(139-146)get_nonces(133-136)has_header(149-152)set_anchor_hash(233-237)set_best_chain_hash(240-244)update_best_chain(247-252)store_header(179-190)store_block(204-208)remove_header(211-230)put_nonces(193-197)
crates/amaru-consensus/src/consensus/effects/ledger_effects.rs (3)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (7)
ledger(34-34)ledger(65-67)ledger(87-89)ledger(136-138)validate(207-215)rollback(217-223)new(50-55)crates/amaru-consensus/src/consensus/effects/network_effects.rs (8)
point(147-152)new(55-57)new(109-114)new(171-176)run(119-130)run(181-201)resources(124-125)resources(186-187)crates/amaru-consensus/src/consensus/effects/store_effects.rs (37)
new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)new(344-346)new(372-374)run(152-160)run(181-189)run(209-217)run(237-245)run(266-274)run(294-302)run(323-331)run(351-359)run(379-387)run(407-415)run(433-441)run(459-467)run(487-495)resources(154-155)resources(183-184)resources(211-212)resources(239-240)resources(268-269)resources(296-297)resources(325-326)resources(353-354)resources(381-382)resources(409-410)resources(435-436)resources(461-462)resources(489-490)resources(515-516)
crates/amaru-ouroboros-traits/src/stores/consensus/mod.rs (2)
crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (2)
anchor(425-427)ancestors_hashes(495-497)crates/amaru-kernel/src/point.rs (1)
hash(34-40)
crates/amaru-consensus/src/consensus/effects/network_effects.rs (3)
crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (3)
fetch_block(171-184)send_forward_event(186-192)new(50-55)crates/amaru-consensus/src/consensus/stages/fetch_block.rs (3)
fetch_block(92-92)fetch_block(137-139)new(124-132)crates/pure-stage/src/effect.rs (1)
wrap(254-264)
crates/amaru-consensus/src/consensus/stages/select_chain.rs (5)
crates/pure-stage/src/stage_ref.rs (2)
sync(151-151)named(80-82)crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (4)
new(65-70)new(201-210)peers(74-76)create(219-227)crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (5)
store(29-29)store(57-59)store(79-81)store(128-130)mock_consensus_ops(306-314)crates/amaru-consensus/src/consensus/tests.rs (2)
any_header(42-68)any_headers_chain(23-25)crates/amaru-consensus/src/consensus/stages/store_header.rs (1)
stage(26-46)
crates/pure-stage/src/effect.rs (3)
crates/pure-stage/src/simulation/running.rs (5)
resources(112-114)effect(156-156)effect(160-160)effect(269-271)new(79-107)crates/amaru-consensus/src/consensus/effects/store_effects.rs (21)
resources(154-155)resources(183-184)resources(211-212)resources(239-240)resources(268-269)resources(296-297)resources(325-326)resources(353-354)resources(381-382)resources(409-410)resources(435-436)resources(461-462)external_sync(41-46)new(33-38)new(145-147)new(174-176)new(202-204)new(230-232)new(259-261)new(287-289)new(316-318)crates/pure-stage/src/tokio.rs (2)
resources(214-216)new(62-70)
⏰ 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 aarch64/macos
- GitHub Check: Build x86_64/windows
- GitHub Check: Build x86_64/linux
- GitHub Check: Benches
- GitHub Check: Test the CLI
- GitHub Check: Build riscv32
- 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 (46)
crates/amaru-consensus/src/consensus/store.rs (4)
15-15: Righto, import cleanup looks spot on!The streamlined imports (keeping Nonce, Point, GlobalParameters while ditching the unused ones) and the cleaner Arc path are bang-on for the refactor. This aligns with the PR's shift to ConsensusOps-based wiring—like cleaning out the garage before the big renovation, yeah?
Also applies to: 20-20
23-118: Nonce evolution logic is solid as a rock, mate!The PraosChainStore wrapper and evolve_nonce implementation handle all the tricky bits—epoch boundaries, stability windows, parent lookups, and error cases—like a seasoned game speedrunner who's memorised every skip and edge case. The fallback to Point::Origin for genesis (line 50) and the error handling for missing parents/headers are all proper.
120-136: Error enum covers all the failure modes like a boss!The NoncesError variants map perfectly to the evolve_nonce failure paths, and the error messages include enough context (header/parent hashes) to debug issues without firing up a time machine. The serde derives are a nice touch for cross-boundary error reporting.
138-285: Test coverage is top-notch—chef's kiss!The unit tests hit the key scenarios (stability window, epoch boundary, outside window) with real preprod data, and the property-based CBOR roundtrip test ensures serialization won't ghost you with random inputs. The test helper (lines 192-216) creates isolated stores for each test, which is cleaner than a fresh pair of kicks.
Based on learnings: The proptest usage (prop_compose!, proptest!, any::()) aligns with proptest 1.7.0 patterns, and the serde derives are compatible with the latest serde 1.0.226.
crates/amaru-consensus/src/consensus/stages/store_header.rs (4)
15-18: Solid import shuffle, mate!The transition to
BaseOpsandConsensusOpsis clean as a whistle. No orphaned imports hanging about like extras in a deleted scene.
26-30: Function signature upgrade looking mint!The shift from concrete
Effectstoimpl ConsensusOpsis spot-on—pure trait abstraction, no compromises. This aligns perfectly with the new testable surface you're building across stages.
33-40: RollForward flow looking sharp, with a known gotcha.The new
eff.store().store_header(header)andeff.base().terminate()paths are spot-on. The FIXME at line 37 about what to do on store failure is pre-existing tech debt—good that it's flagged, but it's not a blocker for this refactor.
42-43: Pass-through events handled like a pro.Both
RollbackandCaughtUpcorrectly delegate toeff.base().send(...). Clean, consistent, no drama.crates/amaru-consensus/src/consensus/stages/forward_chain.rs (3)
15-16: Imports and signature looking legendary!The
NetworkOps,BaseOps,ConsensusOpstrio is imported and used correctly throughout. Function signature shift toimpl ConsensusOpsis textbook refactor.Also applies to: 24-24, 43-43
46-74: Forward event dispatch is mint, legend!The new
eff.network().send_forward_event(...)is bang-on, and error routing througheff.base().send(&processing_errors, ...)keeps the error path clean. No worries here.
76-102: Backward event dispatch is spot-on!The
eff.network().send_backward_event(...)and error handling viaeff.base().send(...)are consistent with the forward path. Top-notch symmetry.crates/amaru-ouroboros-traits/src/is_header/fake.rs (3)
176-177: New imports for runtime strategy execution—nice!
ValueTreeandTestRunnerare spot-on for the newrunhelper. Clean dependencies for property-based testing scaffolding.
180-182: Fixed-size chain generation—sweet as!The rename to
any_fake_headers_chainand the shift to deterministicn-sized generation is a ripper move for test predictability. No more dodgy bounded ranges.
218-223: Runtime strategy execution helper is mint!The
runhelper with the#[expect(clippy::unwrap_used)]annotation is spot-on for test utilities—panic behavior is the right call here, as per project conventions. This'll make deterministic test setup a breeze.Based on learnings.
crates/amaru-stores/src/rocksdb/consensus.rs (1)
489-489: Test helper usage looking legendary!All the
run(any_fake_headers_chain(n))calls are spot-on for deterministic test setup. Once the import path is sorted, these'll be golden.Also applies to: 537-537, 566-566, 591-591, 612-612
crates/amaru-consensus/src/consensus/effects/base_effects.rs (3)
18-29: BaseOps trait is tight and focused—nice work!The trait surface covers the essentials (send, clock, wait, terminate) with proper async signatures.
Clone + Sendbounds are spot-on for multi-stage usage.
31-39: Base wrapper struct is mint!The newtype pattern with a reference to
Effectsis textbook Rust. Lifetime and Clone are spot-on.
41-61: Pure delegation in BaseOps impl—clean as a whistle!All methods correctly delegate to the inner
Effectsinstance. No unnecessary logic, just a clean trait wrapper. Bounds onTare spot-on.crates/amaru-consensus/src/consensus/stages/fetch_block.rs (6)
15-15: Import and signature looking solid, mate!The
NetworkOpsimport at line 15 is actually needed for theeff.network()return type at line 46. The previous review comment flagging it as unused is off-base. Signature shift toimpl ConsensusOpsis spot-on.Also applies to: 22-22, 40-40
44-66: Fetch and forward flow is mint!The
eff.network().fetch_block(...)and downstream routing viaeff.base().send(...)is textbook. Error path is clean too.
68-85: Rollback pass-through is spot-on!Clean delegation to
eff.base().send(...)for the rollback event. No dramas here.
100-121: Lock held across await—watch out, legend!The
fetchhelper grabs a read lock at line 107 and holds it across theclients.get(...)andcloned()calls. The lock is released before theawaitat line 114, so you're safe here. But it's worth noting—learned from past that holding locks across awaits is dodgy business.Based on learnings.
89-140: BlockFetcher abstraction is clean as!The trait and
ClientsBlockFetcherimpl are solid. FIXME at line 103 is acknowledged tech debt (should fetch from any peer with the block, not just the original). Not a blocker for this refactor.
142-192: Test coverage is a ripper!The new test using
mock_consensus_ops()verifies the fetch-and-forward flow. Nice assertions on the downstream message. Solid coverage for the trait-based refactor.crates/amaru-consensus/src/consensus/stages/validate_header.rs (5)
15-16: Import shuffle is mint!The new imports (
BaseOps,ConsensusOps,PraosChainStore,ChainStore) align perfectly with the trait-based store surface. Clean as.Also applies to: 26-27, 29-29
124-151: Store abstraction in handle_roll_forward is spot-on!The shift to
Arc<dyn ChainStore<Header>>andPraosChainStore::new(store)for nonce evolution is clean. Logic preserved, trait surface aligned.
172-218: Stage function refactor is legendary!The
eff.store()retrieval andeff.base().send(...)routing are textbook. Error handling and success paths are spot-on. No issues here.
241-345: FailingStore test helper is a ripper!The
ReadOnlyChainStoreandChainStoreimpls correctly delegate to the innerInMemConsensusStore. Comprehensive trait coverage for testing.
391-444: Test coverage for error paths is mint!Both tests correctly use
Arc::new(failing_store)and verify nonce-related errors. Solid coverage for the new store abstraction.crates/amaru-consensus/src/consensus/effects/consensus_effects.rs (3)
26-39: ConsensusOps trait is a beauty!The centralized trait surface for
store,network,ledger,base, andmetricsis spot-on.Send + Sync + Clonebounds are essential for stage wiring. Impl Trait return types keep flexibility.
41-98: ConsensusEffects struct is mint!The struct holds
EffectsandEraHistory, and the accessors correctly construct trait wrappers. TheConsensusOpsimpl delegates cleanly. No issues here.
306-314: Mock factory is a ripper!The
mock_consensus_ops()factory function simplifies test setup by constructing all the mocks in one go. Clean and convenient.crates/amaru-consensus/src/consensus/stages/select_chain.rs (4)
37-41: Nice one, mate! The transient state extraction is spot on.Splitting out
HeadersTreeStateand makingSelectChaincloneable is a clean refactor. The newtree_statefield aligns with the PR's goal to pass transient state throughselect_chainwithout dragging the wholeHeadersTreearound. This makes the stage more testable and the state boundaries clearer.
184-205: Solid refactor to BoxFuture, like upgrading from a scooter to a motorbike!Changing
handle_chain_syncto returnBoxFutureand explicitly threading thestorethrough is a clean move. This aligns with the new ConsensusOps-based architecture and makes the store dependency explicit, which is mint for testing and reasoning about state flow.
317-340: Beauty! The ConsensusOps wiring is cleaner than a freshly waxed surfboard.Switching from
Effects<DecodedChainSyncEvent>toimpl ConsensusOpsand grabbing the store viaeff.store()is a ripper move. It decouples the stage from the concrete Effects type, making it way easier to unit test with mock ops. The message routing througheff.base().send()keeps everything tidy too.
384-496: G'day, these tests are choice as! Great coverage for the new tree_state flow.The new tests (
a_roll_forward_updates_the_tree_stateanda_rollback_updates_the_tree_state) verify that the tree_state is correctly updated after operations, which is exactly what you'd want when you're extracting state like this. The helper functions (e.g.,make_state,check_peers) keep things DRY and readable, and usingmock_consensus_opsmeans you're testing the stage in isolation without dragging in the full runtime. Mint work!crates/amaru-consensus/src/consensus/headers_tree/headers_tree.rs (4)
57-94: Top-notch extraction, mate! HeadersTreeState is a beaut.Pulling out the transient state into its own struct (
HeadersTreeState) is a solid refactor. Thenew()constructor,initialize_peer()method, and test-onlypeers()accessor are all well-placed. This makes the state boundaries crystal clear and lets you pass the tree_state around without lugging the wholeHeadersTree. It's like separating your backpack from your tent — way easier to manage!
50-55: Into_tree_state is the MVP here, no cap!Consuming
selfand returning thetree_stateis exactly what you need for the select_chain pattern. It lets you create a temporaryHeadersTree, run operations, and then extract the updated state to stash back inSelectChain. This is a clean API choice that keeps the state lifecycle explicit.
198-227: Constructor wiring is slick as, no worries!The
new()andcreate()constructors are well-aligned with the new state model.new()initializes thetree_statefrom the store's best chain, andcreate()takes an explicittree_statefor reconstruction. This is a clean separation of concerns and makes it dead easy to test different state scenarios.
126-662: Ripper job updating all the method accesses, mate!Every method has been consistently updated to access
self.tree_state.peersinstead of direct fields. The Debug/Display implementations, tree operations (best_length,best_chains,trim_chain, etc.), and peer management all look spot on. It's a big refactor, but it's been done cleanly without breaking the logic. Stoked!crates/amaru-consensus/src/consensus/effects/ledger_effects.rs (3)
25-40: LedgerOps trait is clean as a whistle, mate!The
LedgerOpstrait definesvalidateandrollbackoperations withBoxFuturereturns, which is consistent with the async external-effect model. The nestedResultinvalidate(for handling both validation errors and fetch errors) is a bit of a head-scratcher at first glance, but it aligns with the existing error-handling pattern, so fair dinkum!
42-71: Ledger wrapper is mint, proper delegation game!The
Ledgerstruct is a clean wrapper aroundEffectsthat delegates toValidateBlockEffectandRollbackBlockEffect. This keeps the trait implementation simple and makes it easy to mock for tests. The pattern is consistent with theNetworkwrapper innetwork_effects.rs, so you're keeping things nice and uniform across the codebase.
75-148: External effects are top-tier, and the copy-paste gremlin is squashed!The
ValidateBlockEffectandRollbackBlockEffectare well-implemented with properExternalEffectandExternalEffectAPItraits. The resource types (ResourceBlockValidation,ResourceHeaderValidation) are clearly defined, and therunmethods correctly retrieve and use the validation resources.I see the past review's copy-paste error in
RollbackBlockEffect(line 136) has been fixed — the panic message now correctly says"RollbackBlockEffect requires a ResourceBlockValidation resource". Nice work catching that one! The error mapping toProcessingFailedinrollback_block(line 140) is also spot on.crates/amaru-consensus/src/consensus/effects/network_effects.rs (3)
29-49: NetworkOps trait is solid gold, mate!The
NetworkOpstrait defines the essential network operations (fetch_block,send_forward_event,send_backward_event) withBoxFuturereturns, which is consistent with the async external-effect model. The separation of forward and backward events makes the API clear and easy to reason about. This is a ripper foundation for the network layer!
51-88: Network wrapper is mint, keeping things tidy!The
Networkstruct is a clean wrapper aroundEffectsthat delegates toFetchBlockEffectandForwardEventEffect. This keeps the trait implementation simple and mockable for tests. The pattern is consistent with theLedgerwrapper inledger_effects.rs, so you're maintaining a uniform architecture across the consensus effects. Choice as!
90-206: External effects are ace, no worries!The
FetchBlockEffectandForwardEventEffectare well-implemented with proper trait support. The resource types (ResourceBlockFetcher,ResourceForwardEventListener) are clearly defined, and therunmethods correctly retrieve and use the resources.The
ForwardEventenum (withForward/Backwardvariants) and theForwardEventListenertrait are clean abstractions for handling block propagation. The error mapping toProcessingFailed(line 193-197) is consistent with the ledger effects, and thepoint()method onForwardEvent(lines 147-152) is a handy utility. Stoked with this one!
rkuhn
left a comment
rkuhn
left a comment
There was a problem hiding this comment.
In principle this should work, but it isn’t what I had in mind when designing pure-stage. We can try it this way, many of the effects will evolve anyway.
|
|
||
| pub fn store(&self) -> Arc<dyn ChainStore<Header>> { | ||
| Arc::new(Store::new(self.effects.clone(), self.era_history.clone())) | ||
| } |
There was a problem hiding this comment.
Is there a specific reason for exposing these implementations on the struct itself instead of just putting the code into the trait impl? If not, then the typical approach would have only the latter.
There was a problem hiding this comment.
That's because I can swap out only some implementations, say NetworkOps without having to declare the whole trait implementation. See MockConsensusOps for example.
There was a problem hiding this comment.
That doesn’t seem to be the case, though: the trait methods always need to be implemented since there is no reasonable default implementation we could offer. Maybe we’re talking past each other: what I’m proposing is that ConsensusEffects should be implemented using the same syntactic structure as MockConsensusOps.
| peer: &Peer, | ||
| point: &Point, | ||
| block: RawBlock, | ||
| ) -> BoxFuture<'_, Result<Result<LedgerMetrics, BlockValidationError>, BlockValidationError>> |
There was a problem hiding this comment.
Is this shortened lifetime deliberate? It might be a neat trick to make it impossible to tokio::spawn this Future, but I haven’t tried it.
There was a problem hiding this comment.
No this wasn't intentional, I can put it back to 'static, in this PR or the next one.
| let consensus_ops = mock_consensus_ops(); | ||
| consensus_ops.mock_network.return_block(Ok(block.clone())); | ||
|
|
||
| stage(make_state(), message, consensus_ops.clone()).await; |
There was a problem hiding this comment.
Hmm, I see what you did here and it was what you wanted, but I still have a bad feeling about this approach of being able to run pure-stage code without purity at all. Taking your work at face value, this rejects my proposal of tracking effects and asserting them in tests. In other words, you’ve built an abstraction layer that removes the determinism and makes it optional. Hmm.
There was a problem hiding this comment.
Yes, I did this because it looks like a convenient way to do unit testing without involving the runtime. I think that this is also what @abailly had in mind.
rkuhn
left a comment
rkuhn
left a comment
There was a problem hiding this comment.
I thought some more about this and we can — should we want to — still ensure that the effects that are actually used are nicely visible on the level of the stage (basically by adding phantom types), so let’s go with this.
|
|
||
| pub fn store(&self) -> Arc<dyn ChainStore<Header>> { | ||
| Arc::new(Store::new(self.effects.clone(), self.era_history.clone())) | ||
| } |
There was a problem hiding this comment.
That doesn’t seem to be the case, though: the trait methods always need to be implemented since there is no reasonable default implementation we could offer. Maybe we’re talking past each other: what I’m proposing is that ConsensusEffects should be implemented using the same syntactic structure as MockConsensusOps.
2 participants
Presentation
This PR:
ChainStoreto external store effects.receive_header,select_chainandvalidate_headerstages) but more should be added in subsequent PRSvalidate_headerstage. This will be done in a follow-up PR because this one is large enough.Main modifications
pure-stagestage now uses animpl ConsensusOpstrait in order to be testable in isolatio.ChainStoreoperations:resources: Resourcesfield has been added toEffectsin order to runChainStoreoperations as external effects with theirrunmethodHeaderTreeStateencapsulating the transient state of the headers tree has been extracted in order to be passed around in theselect_chainstate.ListsDebugimplementation has been added to display lists of lists which can be useful during debugging.Summary by CodeRabbit
New Features
Refactor
Tests / Chores