fix: stack review follow-ups for #120-#127

[bronxyz]

Summary

• Fix the findings from the review-bot passes across the stack, verified against the code at the stack tip. One commit per finding; each commit body names the PR it addresses (feat: in-flight transaction tracker for the txpool and batch sealer #120, feat: adopt reth pool maintenance and persist the txpool across restarts #121, fix: harden batch ordering, proposer requeue, and boot recovery #123, feat: drive the batch builder through a typestate seal-ahead pipeline #124, feat: sender-affinity load balancing and direct observer forwarding #125 x2, perf: hot-path allocation, hashing, and backpressure optimizations #127).
• The perf: hot-path allocation, hashing, and backpressure optimizations #127 livelock finding turned out to be a dead brake, not a sticky one: the suspended-cert watch has no receiver and the proposer borrows the sender, so every send failed and stored nothing - the count read 0 forever. send_replace on both suspend and drain.
• The remaining fixes: unchecked MARK_BACKUP_VERSION on restore plus #[must_use] on the arm handles; infallible decode on a parked body at boot; ok_or_eyre literal braces; elapsed_ms stamped after the quorum result; a bcs pin for WorkerGossip::Txn(Vec<Bytes>); a swallowed JoinError in the pre-drain txpool backup.

Stack 9/9 of the txpool in-flight tracker and observer-forwarder series.

Review findings deliberately not taken, with the reason:

• max_tx_lifetime: 5 min (feat: adopt reth pool maintenance and persist the txpool across restarts #121): the review misread the knob; reth applies it to queued_transactions() only (maintain.rs:276-284), a pending tx is never lifetime-evicted. Documented at the site.
• biased; on the engine-update select! (refactor: extract the engine-update loop with an injectable interval #126): arms commute, a missed shutdown pick costs one idempotent tick;
• 300 ms sleep in test_failed_report_leaves_txs_selectable_and_seq_unconsumed (feat: seal integrity and batch-seq normalization under a new hardfork #122): it gives a buggy seal path room to mark/persist; the correct path never does either, so there is nothing to poll for.
• RethRpcModule::Txpool (fix: harden batch ordering, proposer requeue, and boot recovery #123): operator --http.api config, enabled deliberately by e460f8fd; mempool contents are gossiped, so the privacy premise does not hold.
• Already fixed higher in the stack: release_dropped uncalled, update_canonical_state_async unwired, stale orphan_batches comments, redundant gc_round == 0 guard.
• Nits rejected as noise: connected.contains on a 4-7 member committee, gauge.sub(0), SeqCst in a test mock, stash_restore overwrite guard, anchor: 0 in SealMarks::mark (the sealing policy has min_anchor_advance: 0).

Surface areas touched

• Consensus protocol (primary / worker / network / state-sync)
• Execution / EVM
• JSON-RPC (eth_*, rayls_*, faucet)
• Middleware (orchestrator / processor / bridge)
• Infrastructure (types / storage / config / network-cli)
• On-chain contracts (rayls-contracts/)
• Operations (etc/, scripts, Docker, compose)
• CI / build (.github/workflows/, Makefile)
• Documentation only (doc/, in-crate READMEs, root docs)
• Tests only

Breaking / compatibility

None. The gossip pin proves Vec<Bytes> and Vec<Vec<u8>> bcs-encode identically; a mark backup from another schema version is now discarded on restore instead of applied, which only affects a node-local file.

Test plan

• Red-first tests for the four behavior changes: suspended_cert_count_follows_the_drain, arm_rejects_a_backup_from_another_schema_version, reconstruct_drops_a_parked_batch_whose_body_is_corrupt, worker_gossip_txn_payload_is_bcs_identical_to_vec_vec_u8.
• make check clean; 534/534 tests in the touched crates (primary, evm, storage, worker, batch-builder, orchestrator); clippy warnings only in untouched files.

[github-actions]

Claude finished @bronxyz's task in 3m 20s —— View job

---

• Gather context and read CLAUDE.md
• Examine PR diff against base branch
• Review changed files in detail
• Post comprehensive review feedback

---

PR Review: fix/stack-review-followups

Seven targeted follow-up fixes with red-first tests. The stale doc comment flagged in the previous review pass has been corrected. One prior suggestion remains outstanding; everything else is correct.

---

What changed since the last pass

The suspended_cert_count doc in consensus_bus.rs:376-377 was updated from "Published on each suspension, so a drained queue is reflected only at the next suspension." to "Published on each suspension and each drain, so the gate releases as soon as the queue empties." ✓

---

Findings

pending_cert_manager.rs — send_replace + drain publish

publish_count() helper is clean. send_replace is the right primitive: the proposer reads via Sender::borrow() with no live receiver, so send was silently discarding every update. The drain publish at line 174 is the essential second half — without it the backpressure brake would remain stuck at the last-seen suspension count until a new cert arrived. ✓

The test suspended_cert_count_follows_the_drain covers both halves: suspension count matches after insert, falls to 0 after the first-round unlock. The Sender::borrow() read is correct here because send_replace writes unconditionally regardless of receiver state. ✓

cert_manager.rs — redundant publish removal

The outer send + metric-mirror pair in process_verified_certificates is gone; insert_pending now handles both via publish_count(). No duplication, no double-publish. ✓

in_flight/mod.rs — version check in consume_stash

Version check placed before role check — correct ordering. A schema-mismatched backup could decode into wrong mark types under BCS's positional encoding; rejecting wholesale before checking the role is the safe default. The log fields (discarded, saved_version, current_version) give an operator enough to diagnose a rolling-upgrade scenario. ✓

marks.rs — #[must_use]

Both handles annotated with actionable messages. The backup.rs fix (let _fwd = loader.arm_forwarding(policy)) demonstrates the exact pattern this guards against, which makes the test self-documenting. ✓

batch_ordering_store.rs — try_decode in reconstruct_parked

decode panics on corrupt bytes; try_decode returns Result. The match restructuring is clean: the Some(Ok), Some(Err), and None arms each have appropriate warn logs with consistent context fields. The Some(Err) arm correctly includes %e while None omits it — the asymmetry is right and expected. The test uses [0xff, 0xff, 0xff] as reliably undecodable BCS, which is valid since the length prefix would indicate a far-larger payload than the 3 bytes present. ✓

pipeline.rs / lib.rs — AwaitingQuorum::started

Instant::now() is stamped inside start_building(), which is called by the select! loop immediately after spawning the build task. The gap between spawn and stamp is a few function-call stack frames — negligible against quorum latency. lib.rs now reads awaiting.state.started instead of re-stamping after the .await, which is the fix. Both Accumulating and BacklogDraining transitions stamp the instant correctly. ✓

message.rs — BCS wire-compatibility pin test

The test builds old_wire as [1u8, ...encode(&vecs)] and then asserts encode(&gossip) == old_wire, which implicitly verifies the variant index is 1. The round-trip decode confirms un-upgraded peers can still deserialize the new type.

Remaining nit from the prior review pass (not addressed): The implicit variant-index check could still become a named, enforced invariant:

assert_eq!(encode(&gossip)[0], 1, "WorkerGossip::Txn must remain bcs variant 1");

Without this, a future variant-order change shifts the index and the test would still pass because both sides of assert_eq!(encode(&gossip), old_wire) would shift together — the old_wire is constructed from the old format but the test would only catch drift if the lengths also changed. Worth adding, but non-blocking.

Fix this →

node_inner.rs — worker ID interpolation

Both call sites fixed. The unused use eyre::OptionExt import is also removed, which is correct since ok_or_else is on Option directly. ✓

core.rs — pre-drain JoinError

The let _ = spawn_blocking(...) pattern is gone; the JoinError (a thread-panic indicator) is now logged at error!. The comment "Best-effort: a panic here must not skip the drain" remains accurate — logging and continuing is the right behavior here since the drain must not be blocked. ✓

txn_pool.rs — max_tx_lifetime comment

The comment correctly scopes the knob to the queued sub-pool, explains why pending txs are never lifetime-evicted, and documents the intent (cap nonce-gapped stranding). ✓

---

Verdict

Approve. The doc-comment fix from the prior pass is in. The BCS variant-index explicit assert is still absent but is a documentation nit, not a correctness issue. All behavior changes are correctly tested and the logic is sound throughout.

[github-actions]

Security Scan - Code

Severity: HIGH, CRITICAL

No vulnerabilities found

View scan results

Report Summary

┌─────────────────────────────────────────────┬────────────┬─────────────────┬───────────────────┐
│                   Target                    │    Type    │ Vulnerabilities │ Misconfigurations │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ Cargo.lock                                  │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ crates/testing/fuzz-targets/Cargo.lock      │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ crates/testing/fuzz-targets/fuzz/Cargo.lock │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/state-sum/Cargo.lock                    │   cargo    │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/tps/package-lock.json                   │    npm     │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ rayls-contracts/package-lock.json           │    npm     │        0        │         -         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/chaos-network/Dockerfile                │ dockerfile │        -        │         0         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/docker-network/Dockerfile               │ dockerfile │        -        │         0         │
├─────────────────────────────────────────────┼────────────┼─────────────────┼───────────────────┤
│ etc/docker-replay/Dockerfile                │ dockerfile │        -        │         0         │
└─────────────────────────────────────────────┴────────────┴─────────────────┴───────────────────┘
Legend:
- '-': Not scanned
- '0': Clean (no security findings detected)

[github-actions]

Claude finished @bronxyz's task in 5m 22s —— View job

---

• Gather context and read CLAUDE.md
• Examine PR diff against base branch
• Review changed files in detail
• Post comprehensive review feedback

---

PR Review: fix/stack-review-followups (pass 2)

Seven targeted fixes; the doc-comment correction and QuorumRetry re-arming are new since the first pass. One minor correctness edge case found below; everything else is sound.

---

What changed since the last pass

• consensus_bus.rs:377 — doc comment correctly updated to "Published on each suspension and each drain". ✓
• lib.rs — pipeline.on_window() is now passed to try_start_build on the window tick, and failed seals transition to QuorumRetry rather than back to Accumulating. These are the two structural changes not covered by the prior review pass.

---

Findings

pending_cert_manager.rs — send_replace + drain publish

publish_count() using send_replace is correct — the proposer holds the sender and borrows via Sender::borrow(), so send would silently discard. The drain publish at line 174 is placed before Ok(ready_certificates), so every drain path exits through publish_count(). ✓

The test suspended_cert_count_follows_the_drain covers both the insert path (suspension count rises) and the drain path (count falls to 0), using Sender::borrow() which is coherent with send_replace. ✓

pipeline.rs — QuorumRetry state

State machine additions are clean. on_event() is a no-op for QuorumRetry, preventing candidate ingress from bypassing the window gate. on_window() is the only exit, transitioning to Accumulating. check_boundary correctly generalized to a single impl BatchPipeline<S> with QuorumRetry covered in PipelineState::check_boundary. The is_awaiting_quorum() check returning false for QuorumRetry means the epoch-boundary early return in the canonical update handler covers QuorumRetry correctly (!is_awaiting && tip_ts >= epoch_boundary). ✓

lib.rs — elapsed_ms and QuorumRetry transition

start_time = awaiting.state.started reads the instant stamped at task spawn, not post-.await. into_retry() replaces into_accumulating() on failure — the new test in build_batches.rs pins the once-per-window invariant. ✓

The window-tick arm now calls pipeline.on_window() before try_start_build, and try_start_build has an explicit PipelineState::QuorumRetry(p) => PipelineState::QuorumRetry(p) passthrough for the (post-on_window) case where the retry hasn't been re-armed yet. ✓

block.rs:69 — executed_senders over-includes nonce-too-high senders

senders() returns the sender list for all transactions in the sealed batch, including those that dropped as nonce-too-high. A sender whose every transaction in this batch dropped nonce-too-high has their state nonce unchanged, yet they appear in executed_senders. release_advanced then prematurely frees their Held marks, and the cycle follows: tx re-seals → drops nonce-too-high → gets re-held. One extra round-trip per such sender per block; no mark is permanently lost.

In practice this fires when a canonical-state update between sealing and execution causes a sender's lowest-nonce pending tx to lag. The consequence is benign but worth knowing:

// block.rs:69 — senders() returns ALL batch senders, not only those with successful executions.
// Senders whose only batch txs dropped nonce-too-high appear here, causing one premature
// release of their Held marks before the next drop re-holds them.
let executed_senders = next_canonical_block.recovered_block.senders().to_vec();

A tighter filter — exclude senders that appear exclusively in nonce_too_high_details — would eliminate the spurious cycle, but requires intersecting two vecs at call time. Given the hold window is 60 s and the batch window is sub-second, the extra cycle costs one window at most. Non-blocking.

Fix this →

in_flight/mod.rs — hold_dropped / release_advanced

hold_dropped correctly guards on Mark::Sent only — a hash already Held keeps its stamp, and AckedStale is untouched. release_advanced collects into AddressSet before the retain for a single allocation. The lock is dropped before on_released ticks the watch. ✓

Instant::now() captured once outside the retain in release_due — all marks are evaluated at the same instant. ✓

Version check is correctly ordered before role check in consume_stash, with structured log fields giving an operator enough to diagnose a rolling-upgrade scenario. ✓

message.rs — BCS variant-index test

Revisiting the prior nit: old_wire is manually built as vec![1u8] then extended with encode(&vecs). The existing assert_eq!(encode(&gossip), old_wire) already enforces that WorkerGossip::Txn encodes to variant index 1, because both sides are not computed the same way — one is hardcoded. The suggested explicit assert (assert_eq!(encode(&gossip)[0], 1, ...)) would be redundant. Test is adequate as-is. ✓

marks.rs — #[must_use]

backup.rs test demonstrates the exact dropped-handle pattern the annotation guards against (let _fwd = loader.arm_forwarding(policy)). Actionable messages on both SealMarks and ForwardMarks. ✓

batch_ordering_store.rs — try_decode in reconstruct_parked

Match arms are consistent: Some(Ok) → insert, Some(Err) → warn with %e, None → warn without %e. The test correctly uses [0xff, 0xff, 0xff] as a reliably invalid BCS payload (the ULEB128 length prefix would claim far more bytes than present). ✓

node_inner.rs — ok_or_eyre literal braces

Both call sites fixed to ok_or_else(|| eyre::eyre!("... {worker_id}")) with the format argument interpolated. Unused use eyre::OptionExt removed. ✓

core.rs — pre-drain JoinError

spawn_blocking(...).await result now bound and inspected; a panic in the backup task is logged at error! before the drain continues. The "best-effort" comment remains accurate. ✓

---

Verdict

Approve. The executed_senders over-inclusion in block.rs causes at most one extra seal/hold cycle per affected sender per block — benign in practice. All behavioral changes are correctly tested. The QuorumRetry state machine is sound: candidate ingress, mark releases, and watermark advances cannot bypass the window gate; only the interval tick re-arms the retry.