Repository navigation
Conversation
![github-actions[bot]](https://avatars.githubusercontent.com/in/15368?s=60&v=4){kind=small}
| from: from.clone(), | ||
| to: to.clone(), | ||
| }); | ||
| self.remove_peer(peer_id, format!("Block fetch {from}..{to} failed")).await; |
There was a problem hiding this comment.
🔴 Important: this disconnects a peer for a legitimate protocol response.
PeerEvent::BlockFetchFailed is emitted from exactly one place — peer_task.rs:437-441, the Ok(false) / NoBlocks arm. The connection is healthy; the peer simply doesn't have that range yet (it rolled back, or we asked ahead of its tip). The comment directly above (lines 914-921, unchanged by this PR) still says the opposite: "Don't remove from fragments — the peer may still have the blocks (transient failure)", and NetworkEvent::BlockFetchFailed's own doc (multi_peer/types.rs:64-69) says the app should "put it on cooldown and re-route", not that the peer is gone.
Two concrete consequences:
- Every
NoBlocksnow aborts the peer task and schedules a reconnect (remove_peer→task_handle.abort()+ backoff), so a node that asks for a block slightly early churns its whole upstream set. remove_peertreats any reason not in its locally-initiated allow-list ("disconnect requested","peer command channel full","peer command channel closed", coordinator.rs:1326-1331) as "the peer reset us", so a peer with a PeerSharing request in flight gets its address added topeershare_hostilepermanently on a singleNoBlocks.
If the intent is to stop re-routing to this peer, use a cooldown on the app side (which the event already supports) rather than tearing the connection down. The same concern applies to the new LeiosBlockFetchFailed arm at line 872, though that one at least follows a real protocol error.
| self.remove_peer(peer_id, format!("Block fetch {from}..{to} failed")).await; | |
| } |
| pub fn start_eb_fetch(&mut self, point: Point, peers: &[PeerId]) -> bool { | ||
| if self.eb_attempts.get(&point).is_some_and(|x| peers.iter().any(|p| x.contains(p))) { | ||
| // already started | ||
| return false; | ||
| } | ||
|
|
||
| let attempts = self.eb_attempts.entry(point).or_insert(BTreeSet::new()); | ||
| attempts.append(&mut BTreeSet::from_iter(peers.iter().copied())); | ||
|
|
||
| true | ||
| } | ||
| pub fn available_eb_candidates(&self, point: &Point) -> Vec<PeerId> { | ||
| let eb_offers = self.eb_candidates(point); | ||
| let Some(peers) = self.eb_attempts.get(point) else { | ||
| tracing::info!("EB fetch: {point}; all available: {:?}", eb_offers); | ||
| return eb_offers; | ||
| }; | ||
| let available = eb_offers.iter().filter(|x| !peers.contains(x)).copied().collect(); | ||
| tracing::info!("EB fetch: {point}; peers attempted: {peers:?}, offers: {eb_offers:?}, available: {:?}", available); | ||
| available | ||
| } |
There was a problem hiding this comment.
🔴 Important: the new attempt bookkeeping both (a) silently drops the EB-fetch dedup and (b) makes the retry path added in this PR a no-op in the common case.
Dedup is gone. start_eb_fetch no longer inserts into pending_eb_fetches, and nothing else ever does (pending_eb_fetches is now written only by finish_eb_fetch/prune_below_slot, both removals). So finish_eb_fetch is dead, state_sizes()'s pending-EB slot is permanently 0, and start_eb_fetch effectively always returns true — the guard on line 386 can only fire if eb_policy.pick returns a peer that wasn't in the candidate list, but do_eb_fetch already filtered the list through available_eb_candidates, so pick only ever returns un-attempted peers. The only remaining dedup is LeiosState::in_flight, whose TTL this PR also cut from 15 s to 5 s (leios.rs:43) — so an offer from a second peer 5 s after the first now fires a duplicate concurrent fetch of the same EB, where before the pending set blocked it.
Retry can't retry. eb_attempts is only ever cleared by forget_peer (disconnect) or prune_below_slot. Neither on_eb_fetch_failed nor on_eb_fetched clears it, and evict_stale_in_flight doesn't either. So for an EB that only one peer offered — or once every offerer has been tried once — available_eb_candidates returns empty forever and the new LeiosState::retry_eb_fetch returns Vec::new() every time. That's precisely the "block producing chains with missing EBs" case this PR is meant to fix.
Suggest keeping the pending-set insert for dedup, and clearing (or at least ageing out) the attempt entry for the peer whose fetch timed out via evict_stale_in_flight so a retry has somewhere to go. There's also no unit test for start_eb_fetch/available_eb_candidates/retry_eb_fetch, so neither behaviour is pinned.
| pub fn next_block_number(&self) -> u64 { | ||
| self.adopted_tip_hash | ||
| .and_then(|h| self.chain_tree.block_number(&h)) | ||
| pub fn next_block_number_after(&self, tip: &Option<[u8; 32]>) -> u64 { |
There was a problem hiding this comment.
🔴 Important: breaking public-API changes in shared-consensus with nothing in the PR description.
Per the root CLAUDE.md, net-node (Piranha) consumes this crate through a path dep, so these are source-breaking for that build:
PraosState::next_block_number()→next_block_number_after(&Option<[u8; 32]>)(this line). Renamed and re-parameterised; every downstream call site breaks, and the new one silently accepts a tip that isn't the adopted tip.TickCtx::newgained a third positional parameter (env.rs:167) — an existing 3-arg call still type-checks shape-wise at the call site only if the caller happens to pass a&DynamicEnvin slot 2, so this can mis-bind rather than fail loudly.CandidateTracker::start_eb_fetchgained apeers: &[PeerId]parameter and changed meaning (see the separate comment onfetch.rs).
The PR body and the single commit message are empty of any migration note. sim-rs was updated in-tree (sim/shared_consensus.rs), but there's no statement that net-node was built against this. Please either keep a deprecated shim for next_block_number() or spell the downstream migration out in the PR description.
There was a problem hiding this comment.
🟡 Changes recommended
A missing test-context field, exhausted EB retries, and disconnects on valid NoBlocks responses remain unresolved.
3 open issues
What changed in this PR
Adds shared-consensus helpers for handling missing endorser blocks (EBs), expands fetch recovery, and introduces behavior-tree sensor inputs.
Changes:
- Adds certified-EB availability checks and explicit-parent block numbering.
- Tracks EB fetch attempts and propagates fetch failures through networking.
- Adds sensor resolution and simplifies test context initialization.
Tests and builds were not run.
| File | Description |
|---|---|
| sim-rs/sim-core/src/sim/shared_consensus.rs | Supplies an empty sensor environment. |
| shared-rs/consensus/src/praos.rs | Adds explicit-tip numbering and exposes EB fetching. |
| shared-rs/consensus/src/pipeline.rs | Enables election debug formatting. |
| shared-rs/consensus/src/leios.rs | Adds retry helpers and extends candidate retention. |
| shared-rs/consensus/src/fetch.rs | Tracks attempted EB peers. |
| shared-rs/consensus/src/committee.rs | Adds a simulated-signature helper. |
| shared-rs/consensus/src/chain_tree.rs | Queries missing certified EBs. |
| shared-rs/consensus/src/behaviour/tree/env.rs | Adds sensors and default tick contexts. |
| shared-rs/consensus/src/behaviour/tree/control.rs | Adds a production-control query. |
| shared-rs/consensus/src/behaviour/tree/config.rs | Updates test contexts. |
| shared-rs/consensus/src/behaviour/tree/condition.rs | Supports numeric sensor references. |
| shared-rs/consensus/src/behaviour/tree/behaviour.rs | Updates test contexts and constructor calls. |
| shared-rs/consensus/src/behaviour/tree/actions.rs | Updates the action test context. |
| shared-rs/consensus/src/behaviour/actions/wrong_len_cert.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/wrong_context_cert.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/withhold_txs.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/withhold_eb_announce.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/vote_flood.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/tx_flood.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/t22.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/single_bls_cert.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/rb_equivocator.rs | Uses context defaults with slot overrides. |
| shared-rs/consensus/src/behaviour/actions/non_member_cert.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/lie_about_eb_size.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/lazy_voter.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/fetch_flood.rs | Uses default context fields. |
| shared-rs/consensus/src/behaviour/actions/fake_eb.rs | Uses default context fields. |
| shared-rs/consensus/src/behaviour/actions/fake_announce.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/echo_to_source.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/eb_burst.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/drop_inbound.rs | Uses context defaults with slot overrides. |
| shared-rs/consensus/src/behaviour/actions/deep_reorg.rs | Uses context defaults with slot overrides. |
| shared-rs/consensus/src/behaviour/actions/cert_suppressor.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/announce_slot_skew.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/announce_size_lie.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/announce_flood.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/announce_equivocate.rs | Simplifies test context setup. |
| shared-rs/consensus/src/behaviour/actions/announce_dangling.rs | Simplifies test context setup. |
| net-rs/net-core/src/protocols/leios_fetch/mod.rs | Clones the request point before sending. |
| net-rs/net-core/src/peer/types.rs | Adds an EB fetch-failure event. |
| net-rs/net-core/src/peer/peer_task.rs | Emits point-specific EB fetch failures. |
| net-rs/net-core/src/multi_peer/types.rs | Exposes EB fetch failures to applications. |
| net-rs/net-core/src/multi_peer/coordinator.rs | Forwards failures and changes peer teardown. |
| net-rs/net-codec/src/block.rs | Allows an unused counting helper. |
| net-rs/net-cli/src/multi_follow.rs | Displays EB fetch failures. |
Review effort: Balanced
| /// Separate namespace for sensor parameters | ||
| pub sensors: &'a DynamicEnv, |
| tracing::info!("EB fetch: {point}; all available: {:?}", eb_offers); | ||
| return eb_offers; | ||
| }; | ||
| let available = eb_offers.iter().filter(|x| !peers.contains(x)).copied().collect(); |
| ValueRef::Sensor(name) => Err(format!( | ||
| "sensor reference sensor.{name} is not valid in this condition context" | ||
| )), | ||
| ValueRef::Sensor(_name) => Ok(TypeClass::Numeric), |
There was a problem hiding this comment.
🟡 Nit: accepting every sensor.* name as Numeric turns a load-time error into a silent always-false condition, and the resolver's doc above is now stale.
type_class is the only reference check consensus-mode conditions get. With it returning Ok(Numeric) unconditionally, a typo'd or unknown sensor name no longer fails validate(); at tick time resolve_number returns None, eval_with falls through to false (lines 237-243), and the branch never fires — exactly the "fallback that turns an error into a plausible value" shape. Notably the only in-tree caller, sim-rs/sim-core/src/sim/shared_consensus.rs:612, builds let sensors = DynamicEnv::new() fresh every tick, so every sensor.* condition in a sim config evaluates false forever with no diagnostic.
Also, the doc comment on this resolver impl (lines 123-125) still reads "every other reference is None (the consensus context has no sensors or bare progress idents)", which this change contradicts — worth updating along with the ValueRef::Sensor doc.
Consider validating against the set of sensor names actually declared for the run (the way env.* is checked against env), or at minimum tracing::warn! once on an unresolved sensor reference.
| impl ControlSignal { | ||
| pub fn requires_tx_production(&self) -> bool { | ||
| self != &Self::default() | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Nit: the name promises far more than the body delivers, and there's no caller or test.
self != &Self::default() is "any field of any of the three sub-signals is non-default". It returns true for a cert-suppressor, a lazy-voter, a drop-inbound — none of which have anything to do with tx production. A downstream caller reading the name will gate tx production on "is any behaviour active at all". If the intent is "the mempool control asks us to produce txs", scope it to the field that actually says so (e.g. self.mempool != MempoolControl::default(), or the specific flood/rate field); otherwise name it is_active(). Either way it needs a doc comment and a unit test — nothing in this repo calls it.
| pub fn imitate_bls_signature(voter_id: &[u8], eb_hash: &[u8; 32], eb_slot: u64) -> Vec<u8> { | ||
| npv_eligibility_signature(voter_id, eb_hash, eb_slot) | ||
| } |
There was a problem hiding this comment.
🟡 Nit: this is a pure alias for npv_eligibility_signature (identical signature, body is a straight delegation) with no caller anywhere in the repo and no doc comment.
Two concerns: it duplicates an existing helper rather than reusing it, and the name invites misuse — npv_eligibility_signature is explicitly documented as not BLS ("in real Cardano this would be a BLS signature requiring the pool's secret key; here it's reproducible by anyone given the same inputs — we don't model BLS", lines 153-157). A function called imitate_bls_signature with no doc of its own reads like it produces something BLS-shaped and verifiable. If a downstream caller needs this spelling, please add a doc comment carrying that caveat forward; otherwise drop it and call npv_eligibility_signature directly.
|
3 important, 1 nit — plus 2 more nits posted inline (6 inline comments total: 3 important, 3 nits).
Not posted as findings: the PR description is empty, so the only claim is the commit subject; the "missing EBs" half is implemented but undermined by the Claude review of the whole PR at 062da94 |
|
No issues found in the new commits. Delta reviewed: Still open from earlier reviews (unchanged by this commit):
|
|
0 important, 3 nits (delta review of
Resolved by this commit: the Still open from earlier reviews: Caveat: I can't run |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
No issues found in the new commits (delta review of
Resolved since the last review (no longer open): the Still open from earlier reviews (not re-posted):
I can't run the test suite, so claims resting on test results are unverified. |
|
No issues found in the two new commits ( Both are doc-comment fixes that correctly apply suggestions from the previous review, verified against the code:
Still open from earlier reviews (untouched by these commits):
I can't run the test suite, so claims resting on test results are unverified here. |
| NetworkEvent::LeiosBlockFetchFailed { point } => { | ||
| println!(" leios block fetch failed: {point}"); | ||
| } |
There was a problem hiding this comment.
🔴 Important: this breaks the net-rs build — the pattern no longer matches the variant.
This commit added source: PeerId to NetworkEvent::LeiosBlockFetchFailed (net-rs/net-core/src/multi_peer/types.rs:115-118), but this arm, added earlier in the same PR, still destructures { point } only. A struct-variant pattern must mention every field (or use ..), so cargo build/cargo test in net-rs/ fails here with E0027 "pattern does not mention field source". net-cli is a workspace member, so the whole workspace build fails, not just this binary.
Surfacing the peer is also the useful thing to print, since the coordinator disconnects that peer right before emitting the event (coordinator.rs:874-875):
| NetworkEvent::LeiosBlockFetchFailed { point } => { | |
| println!(" leios block fetch failed: {point}"); | |
| } | |
| NetworkEvent::LeiosBlockFetchFailed { source, point } => { | |
| println!(" leios block fetch failed: {point} (from {source})"); | |
| } |
| pub fn finish_eb_fetch(&mut self, source: PeerId, point: &Point) -> bool { | ||
| if let Some(peers) = self.pending_eb_fetches.get_mut(point) { | ||
| peers.remove(&source); | ||
| return peers.is_empty(); | ||
| } | ||
| true | ||
| } |
There was a problem hiding this comment.
🟡 Nit: the per-point entry is never removed, so the pending_eb diagnostic no longer means "EB fetches in flight".
finish_eb_fetch now only removes source from the set and leaves an empty BTreeSet behind, so pending_eb_fetches.len() — slot 4 of state_sizes() (line 332), which feeds LeiosState::log_state_sizes / the per-slot telemetry adapter — counts every point ever fetched rather than the points with an outstanding request. The new tests pin that drift rather than flagging it: fetch.rs:695 asserts pending_eb_fetches[&point].is_empty() and leios.rs:2116-2121 asserts state_sizes().4 == 2 after all fetches have finished. Entries are only reclaimed by prune_below_slot, so between prunes the gauge is monotonically rising.
This also contradicts point 4 of the new doc on the field (lines 288-289): "When peer action is resolved (either EB received or EB timed out), the record is removed from pending_eb_fetches" — the record is not removed, only the peer.
Dropping the point when its set becomes empty (self.pending_eb_fetches.remove(point) before returning true) restores the gauge and doesn't change available_eb_candidates, which treats a missing entry and an empty set identically (lines 410-419); the two assertions above would need updating.
|
1 important, 1 nit (delta review of
Still open from earlier reviews, and extended by this commit:
Behaviour I checked and did not flag: |
|
2 important, 1 nit (delta review of The commit does what it says —
Verified fine: I can't run tests here, so the two failures above are read from the code; both are deterministic, no-input tests. Earlier review findings outside these commits were not re-checked in delta mode. |
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
|
No new issues found in the delta ( The changed assertion is correct: Still open from previous reviews (not re-posted):
I can't run tests, so the |
| peer_id, | ||
| } => { | ||
| assert_eq!(peer_id, PeerId(7)); | ||
| } => { |
There was a problem hiding this comment.
🔴 Important: this match arm has two => { bodies for one pattern — net-rs no longer compiles.
The earlier suggestion was pasted in without removing the } => { that already followed it, so the arm now reads:
NetworkEvent::LeiosBlockTxsReceived { point: p, transactions, peer_id } => {
assert_eq!(peer_id, PeerId(7));
} => { // <- second body, syntax error
assert_eq!(p, point);
assert_eq!(transactions, vec![body2, body0]);
}rustc rejects this at parse time (expected one of ,, }, or an expression, found =>), so cargo test in net-rs/ fails for the whole workspace, not just this test. net-cli and net-codec build too, so nothing in net-rs/ can be tested until this is fixed. The two assertions that were there before (p == point, transactions == vec![body2, body0]) are also orphaned into the dead second body and would stop running.
Merge the two bodies into one arm:
| peer_id, | |
| } => { | |
| assert_eq!(peer_id, PeerId(7)); | |
| } => { | |
| peer_id, | |
| } => { | |
| assert_eq!(peer_id, PeerId(7)); |
|
1 important, 0 nits (delta review of
The other commit in this range ( Resolved since the last review: the Still open from earlier reviews: the breaking I can't run the test suite, so the compile failure above is read off the source; everything else about test results in this PR is unverified on my side. |
| /// fires identically). | ||
| LeiosBlockReceived { | ||
| source: Option<PeerId>, | ||
| source: PeerId, |
There was a problem hiding this comment.
🟡 Nit: the doc three lines above now contradicts the field, and the self-produced case it describes can no longer be expressed.
Lines 105-108 still read "source is the peer it was fetched from (None for self-produced EBs, which the node feeds through the same receive path so manifest-recording fires identically)". With source: PeerId there is no None, so that sentence documents a state the type forbids. The same stale claim sits in coordinator.rs:830-832 ("Self-produced EBs never take this path (they enter via InjectLeiosBlock, source=None)").
This is more than wording. In-tree the only emitter is coordinator.rs:863, which always had a real peer_id, so nothing here breaks — but the doc is the only record of the self-produced path, and that path lives in net-node (path dep per the root CLAUDE.md). The sibling variant that consumes this tagging, NetworkCommand::RecordEbManifest, still carries source: Option<PeerId> with the same contract (types.rs:240-243: "the peer that supplied the EB body (None if self-produced); the resulting BlockTxsOffer is tagged with it so LeiosNotify ..."). If net-node's self-produced EBs were flowing through LeiosBlockReceived { source: None }, it now has to invent a PeerId for them, and feeding that invented id on to RecordEbManifest would tag a self-produced EB as supplied by a real peer — suppressing an offer that should go out. Worth confirming against the net-node build, and either way the two comments should be rewritten to match the non-optional field.
|
1 nit (delta review of
The commit's other hunk fixes the duplicated Previously reported, still open:
I can't run tests in CI, so compilation and the |
|
No issues found in the new commit.
Still open from earlier reviews:
I can't run the build or tests, so the |
|
Part of PR109 |
…sors added