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)