Skip to content

fix: harden batch ordering, proposer requeue, and boot recovery - #123

Draft
bronxyz wants to merge 5 commits into
feature/txpool-seal-integrity-and-seq-hardforkfrom
feature/txpool-ordering-and-proposer-hardening
Draft

fix: harden batch ordering, proposer requeue, and boot recovery#123
bronxyz wants to merge 5 commits into
feature/txpool-seal-integrity-and-seq-hardforkfrom
feature/txpool-ordering-and-proposer-hardening

Conversation

@bronxyz

@bronxyz bronxyz commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Harden batch-ordering state: drain_epoch can no longer rewind the epoch on a crash-replay of an already-passed output; the boot history walk opts out of the long-read cap and fails loud instead of silently booting with empty watermarks; parked batches persist by digest (ParkedRef) with the snapshot taken under the lock and written outside it, and the authority map becomes a BTreeMap for deterministic persisted bytes. Legacy by-value blobs still decode via a fallback.
  • Requeue evicted and lag-stranded proposer digests: GC keyed its horizon on self.round and dropped the evicted headers' digests, permanently gapping the per-authority seq stream on peers; the horizon now keys on the monotonic committed round and eviction requeues FIFO-ahead. The foreign-commit fallback retransmitted every proposed header; it now splits at a grace horizon below the commit round so ordinary lag never triggers it.
  • Drop the dead NodeBatchesCache writes (the orphan rescue was their only reader) and the boundary flag that justified them; close three robustness gaps (a clean external shutdown misclassified as a crash inside run_epoch, a multi-worker in-flight wipe fenced by ensure!, a debug_assert promoted to a release assert); restore the observability trio (ForceDrained tracker stage, AlreadyImported demoted to debug, txpool RPC module enabled).

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

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 on the wire. The persisted batch-ordering blob changes shape; a legacy by-value blob is still decoded (pinned by stored_and_legacy_ordering_blobs_are_distinguishable_by_decode). NodeBatchesCache stays declared (stable column-family set) and is still cleared on foreign-DB sanitization. The txpool_* RPC module becomes available.

Test plan

  • drain_epoch_never_rewinds_to_an_already_passed_epoch, stored_and_legacy_ordering_blobs_are_distinguishable_by_decode.
  • Red-proven proposer tests: fallback_requeue_forgives_ordinary_commit_lag fails at grace 0, digests_survive_gc_advance_and_later_round_commit fails when the requeue drops; push_digest_never_drops and fallback_requeue_stops_at_the_commit_round pin the contracts.
  • The el-to-cl seal test asserts no NodeBatchesCache row is written.
  • The run_epoch shutdown race and the multi-worker wipe need multi-task fault injection to demonstrate red and land as hardening without failing-first tests.
  • make check on the stack tip; CI on this branch.

@bronxyz bronxyz changed the title feature/txpool ordering and proposer hardening fix: harden batch ordering, proposer requeue, and boot recovery Aug 20, 2026
@bronxyz
bronxyz force-pushed the feature/txpool-ordering-and-proposer-hardening branch from e2a2741 to c373933 Compare August 24, 2026 11:30
@raylsnetwork raylsnetwork deleted a comment from github-actions Bot Aug 24, 2026
- never rewind the epoch: drain_epoch treated any change as a forward crossing, so a
  crash-replay of an already-passed epoch's output drained the live parked set and moved
  the epoch backward (a watermark with a backward path); a stale-epoch drain is now a
  warned no-op, pinned by drain_epoch_never_rewinds_to_an_already_passed_epoch
- fail closed and uncap the boot history walk: under the default long-read cap a slow
  cold boot aborted the txn mid-walk and unwrap_or_default booted with EMPTY watermarks,
  silently disabling gap detection (the after_round starvation class); the walk opts out
  of the cap like the catch-up accumulator, fails loud, and decodes only the leader epoch
  and payload digests via the zero-copy bcs projection
- persist parked batches by digest (ParkedRef), not by value: a committed batch's Batches
  row outlives the reboot, so bodies only duplicated bytes in the blob; the restart read
  reloads them in short raw-read txns; snapshot under the lock, write OUTSIDE it (the
  persist-starvation class); the authority map becomes BTreeMap for deterministic
  persisted bytes and recovery iteration
- backwards compatible: a legacy by-value blob fails the compact decode and decodes via
  the fallback, pinned by stored_and_legacy_ordering_blobs_are_distinguishable_by_decode
… they justified

- the orphan-batch rescue was NodeBatchesCache's only reader; since its deletion every
  seal still paid a write, the proposer a per-commit cleanup txn, and the boundary a
  table clear, all for rows nothing reads
- the epoch-boundary-drop bool existed solely to skip that cleanup for boundary headers,
  so committed_certificates and committed_own_headers now carry certificates and rounds
  plain; the proposer filter-then-maps its own committed rounds
- the table stays declared (stable column-family set) and is still cleared on foreign-DB
  sanitization to purge rows written by older binaries
- pinned: the el-to-cl seal test asserts no NodeBatchesCache row is written
…ted-round horizon

- GC keyed the eviction horizon on self.round and silently dropped the evicted headers'
  digests: those digests are quorum'd and seq-consumed, so a drop gaps the per-authority
  seq stream permanently on peers (the in-flight TTL only re-seals the txs under a NEW
  seq); the horizon now keys on the monotonic committed round - commits trail the
  proposal frontier, so a header in the lag window is still committable - and eviction
  requeues the digests FIFO-ahead of the queue (RequeueReason::GcEvict)
- the foreign-commit fallback retransmitted EVERY proposed header, fresh ones included,
  manufacturing duplicate commits for headers whose certificates were still collecting
  votes; it now splits at a horizon FALLBACK_REQUEUE_GRACE_ROUNDS below the commit round,
  so ordinary commit lag never triggers it and fresh headers stay proposed
  (RequeueReason::CommitLag)
- BatchStage::RequeuedInProposer joins the tracker lifecycle
- red-proven: fallback_requeue_forgives_ordinary_commit_lag fails at grace 0,
  digests_survive_gc_advance_and_later_round_commit fails when the requeue drops;
  push_digest_never_drops and fallback_requeue_stops_at_the_commit_round pin the
  never-drop and fresh-header contracts
- run_epoch: an external consensus shutdown arriving DURING the running select resolved
  through the task-manager join arm, where a critical task exiting Ok in response was
  misclassified as a crash and killed the node - was_externally_shutdown only samples the
  state before the select; a biased epoch-shutdown arm above the boundary arm now
  resolves it as a clean NodeShutdown
- fence the batch builder on a single worker: the node-scoped in-flight tracker is shared
  by every worker pool, so a second worker's boundary clear or reconcile would wipe the
  sibling's marks; ensure! names the assumption instead of corrupting silently
- promote the batches/digests parity check to a release assert: debug_assert_eq compiled
  out and the raw digest indexing then aborted messageless on a mismatch

Hardening: the shutdown race and the multi-worker wipe need multi-task fault injection to
demonstrate red, so these land without failing-first tests.
- mark force-drained parked batches in the tracker (ForceDrained; pre-fork boundary path
  only, the fork discards instead) so an out-of-order boundary execution is attributable
- demote AlreadyImported gossip-admission errors to debug: overlapping observer forwards
  make it the common outcome under load, and warn buried the submissions that did fail
- enable the txpool RPC module (txpool_content/txpool_status) for pool inspection
@bronxyz
bronxyz force-pushed the feature/txpool-ordering-and-proposer-hardening branch from c373933 to e460f8f Compare August 26, 2026 11:43
@github-actions

github-actions Bot commented Aug 26, 2026

Copy link
Copy Markdown
Contributor

Claude finished @bronxyz's task in 2m 6s —— View job


PR Review: feature/txpool-ordering-and-proposer-hardening

  • Read CLAUDE.md for repo guidelines
  • Check whether prior critical/medium findings were addressed
  • Analyze new commits in the diff
  • Review critical files
  • Post comprehensive review

This is a re-review on the latest push (5 commits). The structural work is sound and the addressed items from the last cycle remain clean. One critical finding from the previous two cycles is still unresolved and must be fixed before merge.


Critical (Third Cycle — Unresolved)

decode(bytes) still uses the infallible variant — batch_ordering_store.rs:94

// batch_ordering_store.rs line 94
let batch: Batch = decode(bytes);

decode panics on corrupt or truncated bytes. try_decode is already imported on line 7. This path is the boot-time reconstruction of parked batches from MDBX — exactly the path that defends against bit-rot and partial writes. A panic here defeats the purpose of the defensive drop pattern used for the missing-body case directly above it (lines 97–104), and causes a hard node crash rather than a graceful skip on a corrupt Batches row.

The fix is one-for-one with the existing None arm:

Some(bytes) => match try_decode::<Batch>(bytes) {
    Ok(batch) => {
        parked.insert(seq, reference.into_prepared(Arc::new(batch)));
    }
    Err(e) => warn!(
        target: "engine",
        ?addr,
        seq,
        batch_digest = ?reference.batch_digest,
        err = %e,
        "dropping parked batch on restart: corrupt body bytes"
    ),
},

This has been flagged in the previous two review cycles with no resolution. It should block merge.

Fix this →


Medium (Still Unconfirmed)

txpool RPC module now exposed — config.rs:102

RethRpcModule::Txpool is now included in ALL_MODULES. The validate_rpc_modules gating is structurally correct (it intersects any caller-supplied set against ALL_MODULES), so the module is now reachable via eth_*-style endpoints. txpool_content and txpool_inspect return pending transaction sender addresses, nonces, gas prices, and calldata ahead of execution.

On a privacy-preserving chain this leaks transaction metadata before finality. This was raised in the previous cycle with no visible resolution. Before merging to a release branch, please confirm either: (a) the txpool_* endpoints are firewalled / allowlisted at the infra layer so they are not externally reachable, or (b) the data exposure is intentional and accepted for this chain configuration.


Addressed Since Last Cycle (Confirmed Clean)

disable_long_read_safety() comment — the inline comment now correctly explains boot-time use before any writer contention. Satisfied.

committed_certificates / committed_own_headers type simplificationVec<(Round, bool)>Vec<Round> throughout consensus_bus.rs, state_handler.rs, and the proposer is clean. The removed bool boundary-drop flag is consistently gone.

epoch_shutdown_rx arm in the biased selectcore.rs:680 correctly places the external-shutdown arm ahead of the task-join arm, closing the TOCTOU window. The was_externally_shutdown pre-select snapshot still handles the earlier case at line 678. Clean.

drain_epoch epoch-rewind guardOrdering::Less arm at ordering.rs:219 preserves monotonicity. Correct.

BTreeMap for authorities — deterministic iteration is used consistently through types, store, and ordering layers. Clean.

ensure! guard for single-worker constraintnode_inner.rs:153 surfaces the architectural constraint explicitly. Clean.


Low / Nit (Still Present)

Redundant gc_round == 0 guard — recovery.rs:66-68

let Some(gc_round) = committed.checked_sub(self.gc_depth) else { return; };
if gc_round == 0 { return; }

checked_sub already returns None when committed < gc_depth. If committed == gc_depth, checked_sub returns Some(0), and split_off(&1) with gc_round == 0 retains everything ≥ 1 — round-0 headers are never created, so this is a no-op. Not harmful, but the guard adds noise without providing protection.

FALLBACK_REQUEUE_GRACE_ROUNDS = 4 derivation — recovery.rs:10

The doc comment explains the concept well but not why 4 specifically. A one-liner like "must exceed typical commit latency in a 4-node BFT committee (≤ 3 rounds); pinned by fallback_requeue_forgives_ordinary_commit_lag" would protect against arbitrary future re-tuning.


Positive Observations (Carried Forward)

  • Lock-outside-DB-write in persist() — snapshot-before-write avoids persist-starvation. Correct pattern.
  • Backward compatibility decode path — format-distinguishability test gives a clear contract; two-pass read (raw reads only, decode after txn closes) avoids MDBX read-txn timeout under a parking storm.
  • requeue_front helper — clean extraction; the FIFO prepend is correct and the RequeueReason tagging gives the batch tracker full visibility into GC-evict vs commit-lag paths.
  • process_committed_headers split-at-horizon — the grace-window split cleanly separates ordinary commit lag from clear-miss retransmit, with the dedup registry as the stated load-bearing guard against double-commit.
  • Test suite — the four targeted tests (drain_epoch_never_rewinds, stored_and_legacy_ordering_blobs_are_distinguishable_by_decode, digests_survive_gc_advance_and_later_round_commit, and the fallback_requeue_* pair) all directly exercise the invariants they claim to protect.

Summary: The single unresolved critical (decodetry_decode in reconstruct_parked) must be fixed before merge — this is its third review cycle. The txpool exposure should be explicitly confirmed or firewalled. Everything else is clean.

@raylsnetwork raylsnetwork deleted a comment from github-actions Bot Aug 26, 2026
@raylsnetwork raylsnetwork deleted a comment from github-actions Bot Aug 26, 2026
@raylsnetwork raylsnetwork deleted a comment from github-actions Bot Aug 26, 2026
bronxyz added a commit that referenced this pull request Aug 26, 2026
…ting

- boot recovery already drops a parked ref with no `Batches` row; a corrupt row used the
  infallible decode and aborted the node on the same defensive path

- addresses the PR #123 (feature/txpool-ordering-and-proposer-hardening) review: infallible
  `decode` in `reconstruct_parked`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant