Skip to content

feat: in-flight transaction tracker for the txpool and batch sealer - #120

Draft
bronxyz wants to merge 3 commits into
mainfrom
feature/txpool-in-flight-tracker-core
Draft

feat: in-flight transaction tracker for the txpool and batch sealer#120
bronxyz wants to merge 3 commits into
mainfrom
feature/txpool-in-flight-tracker-core

Conversation

@bronxyz

@bronxyz bronxyz commented Aug 20, 2026

Copy link
Copy Markdown
Collaborator

Summary

  • Add InFlightTracker: the txpool records hashes sealed into a batch but not yet observed mined, so the next sealing round skips them instead of re-sealing a stuck backlog batch after batch. Marks are armed per role behind a capability handle (SealMarks / ForwardMarks), released by a reconcile against the pending sub-pool plus a backoff-gated sweep, and snapshot/restored across restarts through a versioned MarkBackup.
  • On quorum the sealer now marks the batch in flight instead of evicting it from the pool, so sealed transactions stay pending and RPC-visible until execution drains them.
  • Defuse two latent traps ahead of the rework: the diagnostic nonce span overflowed at u64::MAX (overflow-checks + panic=abort turned one crafted batch into a whole-network halt), and the workspace metrics facade is pinned to 0.24 so a crate adopting 0.23 cannot bind a second global whose macros silently no-op.

Stack 1/9 of the txpool in-flight tracker and observer-forwarder series. The builder pipeline and the forwarder that also consume the tracker land in later PRs; this one scopes to the pool and sealer.

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. No wire or storage format changes; the mark backup file is new and only read when present.

Test plan

  • nonce_span_saturates_at_the_max_nonce fails on the pre-fix span arithmetic.
  • Failing-first regression that a sealed batch is not re-proposed while in flight; four builder tests retargeted from drain-on-quorum to stay-pending-and-marked.
  • Proptest drives concurrent tracker writers and checks marked - reconcile - ttl - clear equals the live set and the gauge.
  • make check on the stack tip; CI (fmt, clippy, workspace tests) on this branch.

@github-actions

Copy link
Copy Markdown
Contributor

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)

@bronxyz bronxyz changed the title feature/txpool in flight tracker core feat: in-flight transaction tracker for the txpool and batch sealer Aug 20, 2026
@bronxyz
bronxyz force-pushed the feature/txpool-in-flight-tracker-core branch from b01daa0 to 8b580d3 Compare August 24, 2026 11:30
@raylsnetwork raylsnetwork deleted a comment from github-actions Bot Aug 24, 2026
- saturate the diagnostic nonce span: range.max - range.min + 1 overflowed at nonce
  u64::MAX, and under overflow-checks + panic=abort one crafted batch pairing nonce 0
  with u64::MAX from a single sender aborted every node that logs the range - a
  whole-network halt; regression nonce_span_saturates_at_the_max_nonce
- pin the workspace metrics facade to 0.24, the version reth's prometheus recorder
  registers against: the entry is uninherited today, but the first crate to adopt it at
  0.23 would bind a second global and its gauge!/counter! would silently no-op (the
  facade-version trap this repo has hit before)
- add InFlightTracker: records hashes sent to the mempool but not yet observed mined, so a sealing or forwarding round skips the hashes it already has outstanding
- arm per role behind a capability handle (SealMarks/ForwardMarks); sealing wipes its set each round, forwarding keeps its marks across the epoch boundary so the resend backoff survives
- gate release on a base wait plus capped exponential backoff plus a minimum execution-anchor advance; AckedStale marks are terminal and never released by the TTL sweep
- compute each metric delta under the same write lock that applies it, so marked - reconcile - ttl - clear equals the live set and the gauge under any interleaving (proptest drives concurrent writers)
- snapshot and restore marks across a restart through a versioned MarkBackup, applied lazily at the next matching-role arm with rebased clocks

- no production caller yet: this lands the tracker ahead of the builder and forwarder that will consume it
- WorkerTxPool owns the InFlightTracker, exposes is_in_flight, and reconciles marks against the pending sub-pool on each canonical update
- build_batch skips a hash still in flight, so a stuck inclusion backlog is not re-sealed batch after batch
- on quorum the sealer marks the batch in flight instead of evicting it, so the txs stay pending and RPC-visible until execution drains them
- the "anything to seal?" guard scans for a non-in-flight tx, so retained marks do not spin the builder on empty batches
- mark through the armed SealMarks capability, since the direct mark_in_flight helper is test-only

- add a failing-first regression that a sealed batch is not re-proposed while it is in flight
- retarget four builder tests that asserted drain-on-quorum to the new contract: sealed txs stay pending and marked in flight until execution

- scope is the pool and sealer only; the builder pipeline refactor and the observer forwarder that will also consume the tracker are deferred
@bronxyz
bronxyz force-pushed the feature/txpool-in-flight-tracker-core branch from 8b580d3 to 689f3fc 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 3m 48s —— View job


PR Review: feature/txpool in-flight tracker core

Tasks:

  • Gather context and identify trigger
  • Read changed files (in_flight/mod.rs, marks.rs, txn_pool.rs, batch-builder/lib.rs)
  • Check status of all prior review findings
  • Identify any new issues in latest commit
  • Post final review

Overview

The latest commit (689f3fc) correctly wires the tracker into the txpool and batch sealer: build_batch skips in-flight hashes, the sealer marks on quorum via the armed SealMarks capability, and reconcile_in_flight releases marks when the pending sub-pool drains them. The in-flight skip logic in batch.rs:89-91 is sound — the continue (rather than mark_invalid) preserves descendants for sealing, and the no_updates() snapshot prevents nonce gaps from new arrivals mid-iteration.

None of the previously flagged issues have been resolved. The same two merge-blockers remain, plus a new severity upgrade on the stale-comment finding.


Bug: MarkBackup::version is never validated on restore (merge-blocker — still present)

consume_stash checks the role but ignores the version field entirely (mod.rs:331-356):

fn consume_stash(guard: &mut Inner, role: MarkRole) -> usize {
    let Some(backup) = guard.pending_restore.take() else { return 0 };
    if backup.role != role { ... return 0; }
    // ← version is never read; any backup restores regardless of MARK_BACKUP_VERSION
    let now = Instant::now();
    for mark in backup.marks { ... }
}

MARK_BACKUP_VERSION is defined in marks.rs:174 so "a backup written by a prior version is rejected instead of misread," but the code never consults it. A node upgrading from an older build silently applies stale backup semantics.

Fix: after the role check, add if backup.version != MARK_BACKUP_VERSION { warn!(…); return 0; }.

Fix this →


Bug: release_dropped is defined but never called (merge-blocker — still present)

InFlightTracker::release_dropped (mod.rs:214) exists only in tests. Nothing in the EVM execution path calls it when a nonce_too_high drop occurs, even though batch_tracker.rs already counts total_nonce_too_high. The consequence is a 60-second TTL lag before such marks release and the transaction becomes re-sealable.

If this scenario is live (it is — batch_tracker.rs:351 increments total_nonce_too_high), the method needs a caller in the EVM execution path. If it is not yet reachable in this stack slice, the method and its counter should be removed and re-added in the PR that wires the EVM execution path, to avoid a misleading public API with no callers.


Bug: update_canonical_state_async added but never wired (severity upgrade — previously "stale comment")

This commit added update_canonical_state_async (txn_pool.rs:241-289) to spawn a blocking task and avoid runtime stalls under load. The comment at txn_pool.rs:328-329 describes exactly that intent, but process_canon_state_update still calls the synchronous update_canonical_state immediately after:

// async pool update spawned as blocking task to prevent RPC degradation     ← describes async
// this is safe because canonical stream updates don't require immediate deduplication
self.update_canonical_state(         ← synchronous; holds the pool write-lock inline
    tip.sealed_block(), ...
);
self.reconcile_in_flight();          ← also synchronous

process_canon_state_update runs inside spawn_critical_task("canonical txn pool", async move { … }), an async context. Calling the synchronous pool update (which acquires the pool write lock) from inside an async task blocks the runtime thread for the duration of the lock. The async version exists specifically to fix this — it was never connected.

Fix: replace self.update_canonical_state(…) with self.update_canonical_state_async(…) at txn_pool.rs:330 and verify reconcile_in_flight is still safe to call from the async context (it checks is_empty() first and calls pending_transactions() under the pool read lock; spawning it as a separate blocking task or after the async update settles may be needed depending on ordering guarantees).

Fix this →


Design: SealMarks::mark hardcodes anchor: 0 (prior review — still present)

marks.rs:87:

pub fn mark(&self, hashes: impl IntoIterator<Item = TxHash>) {
    self.tracker.track_all(hashes, Mark::Sent { at: Instant::now(), anchor: 0, attempts: 0 });
}

Today this is harmless because DuePolicy::ttl sets min_anchor_advance: 0. If a future policy change requires a block advance before TTL release, the hardcoded anchor=0 makes every mark immediately eligible regardless of how many blocks have passed.

Fix: add an anchor: u64 parameter to SealMarks::mark and update the call site in lib.rs:335 to pass this.last_canonical_update.number.

Fix this →


Design: stash_restore silently drops a prior stash (prior review — still present)

mod.rs:327-329:

pub fn stash_restore(&self, backup: MarkBackup) {
    self.inner.write().pending_restore = Some(backup);
}

A second call before the next arm silently discards the first. A warn! on overwrite, or debug_assert!(self.inner.read().pending_restore.is_none()), would surface the misuse at development time.


Perf: reconcile_in_flight is O(all pending txs) (prior review — still present, not blocking)

txn_pool.rs:359-361:

let pending: B256Set =
    self.pool.pending_transactions().iter().map(|tx| *tx.hash()).collect();

The early return on empty in-flight is a welcome guard, but the non-empty path still clones all Arc<PoolTxn> pointers and acquires the pending-pool read lock on every canonical update. A pool helper that copies only hashes would reduce per-block overhead. Not blocking, but worth tracking.


Nit: Missing #[must_use] on arm_sealing / arm_forwarding (prior review — still present)

Dropping the return value of arm_sealing wipes the prior round's marks and discards the write capability silently. #[must_use] on both methods (mod.rs:124, mod.rs:140) catches this at compile time.


Nit: on_released calls gauge.sub(0) in no-op paths (prior review — still present)

mod.rs:296: self.metrics.gauge.sub(delta as i64) is unconditional; when delta == 0 it's a no-op metric call. The existing if delta > 0 guard could equally cover the gauge call.


Summary

Severity Finding Status
Bug MarkBackup::version never checked — stale backups restored silently Still present
Bug release_dropped never called — nonce-drop marks only clear via 60s TTL Still present
Bug update_canonical_state_async added but process_canon_state_update still calls sync version — blocks async runtime Severity upgrade
Design SealMarks::mark hardcodes anchor: 0 — implicit dep on min_anchor_advance: 0 Still present
Design stash_restore silently drops a prior stash Still present
Perf reconcile_in_flight is O(all pending txs) on every canonical update Still present
Nit arm_sealing/arm_forwarding missing #[must_use] Still present
Nit on_released calls gauge.sub(0) in no-op paths Still present

The wiring in this commit is correct. I'd want the version-validation bug, the release_dropped gap, and the sync-vs-async call site resolved before merging. The anchor=0 issue is lower priority but worth fixing while the sealing-mark code is being touched.

@raylsnetwork raylsnetwork deleted a comment from github-actions Bot Aug 26, 2026
bronxyz added a commit that referenced this pull request Aug 26, 2026
- `MARK_BACKUP_VERSION` was written but never read on restore; bcs is positional, so a
  reshaped backup could decode into wrong marks instead of being refused
- `#[must_use]` on `SealMarks`/`ForwardMarks`: dropping the handle silently discards the
  capability the arm minted; one test was doing exactly that

- addresses the PR #120 (feature/txpool-in-flight-tracker-core) review: unchecked
  `MarkBackup::version` and the missing `#[must_use]` on the arm handles
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