diff --git a/AGENTS.md b/AGENTS.md index 7c628f5..b9b0f7f 100644 --- a/AGENTS.md +++ b/AGENTS.md @@ -389,6 +389,7 @@ Before finishing a change, ensure: - [`docs/protocol/`](docs/protocol/) — the authoritative protocol contracts: [`scheduler-semantics.md`](docs/protocol/scheduler-semantics.md) (the canonical acceptance algorithm, I1) and [`application-contract.md`](docs/protocol/application-contract.md) (the `Application` FFI trait contract). - [`docs/invariants.md`](docs/invariants.md) — register of cross-module invariants (what's load-bearing across files) + the fail-loud check policy. - [`docs/review/`](docs/review/) — dated correctness-review ledgers; open findings, settled designs, work packages. +- [`docs/plans/`](docs/plans/) — active coordination tracks; agreed work split, design directions, open decisions. - [`docs/threat-model/README.md`](docs/threat-model/README.md) — trust boundaries, in-scope and out-of-scope threats. - [`docs/recovery/README.md`](docs/recovery/README.md) — recovery design, TLA+ formal verification, design history. - [`docs/snapshots/`](docs/snapshots/) — app snapshots: [`format.md`](docs/snapshots/format.md) (dump trait + wire format) and [`lifecycle.md`](docs/snapshots/lifecycle.md) (take/promote/GC/lease design + crash-safety). diff --git a/CLAUDE.md b/CLAUDE.md index b4ae18c..ae9fe12 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -57,6 +57,7 @@ Rust edition 2024 / Axum API / SQLite (rusqlite, WAL) / EIP-712 signing / SSZ en - **[`docs/protocol/`](docs/protocol/)** — the authoritative protocol contracts: [`scheduler-semantics.md`](docs/protocol/scheduler-semantics.md) (canonical acceptance algorithm) and [`application-contract.md`](docs/protocol/application-contract.md) (the `Application` FFI trait). Read before touching the scheduler, the gold frontier, the fold, or an `Application` impl. - **[`docs/invariants.md`](docs/invariants.md)** — cross-module invariants register + the fail-loud check policy. Check it before changing anything it lists as load-bearing. - **[`docs/review/`](docs/review/)** — dated correctness-review ledgers: known-open findings, settled designs, work packages. Check for open findings in code you're about to touch. +- **[`docs/plans/`](docs/plans/)** — active coordination tracks: agreed work split, design directions, and open decisions across in-flight efforts. Check before starting work that might belong to a track. - **[`docs/threat-model/README.md`](docs/threat-model/README.md)** — trust boundaries and in-scope threats. - **[`docs/recovery/README.md`](docs/recovery/README.md)** — preemptive recovery design + TLA+ proofs. - **[`docs/snapshots/lifecycle.md`](docs/snapshots/lifecycle.md)** — snapshot lifecycle design + invariants (take/promote/GC, crash-safety). Read before touching the inclusion lane's safe-frontier/snapshot path. diff --git a/docs/plans/2026-07-coordination-tracks.md b/docs/plans/2026-07-coordination-tracks.md new file mode 100644 index 0000000..12db1a9 --- /dev/null +++ b/docs/plans/2026-07-coordination-tracks.md @@ -0,0 +1,296 @@ +# Coordination Tracks — 2026-07 + +**Status:** active plan of record. Six tracks, agreed 2026-07-23. Tick / annotate +as work lands; when a track completes, move its durable outcomes into the +normative docs (`docs/protocol/`, `docs/invariants.md`, `docs/snapshots/`) and +mark it done here. Multiple tracks may fold into one PR when practical. + +Context: Bart is building **libdex**, a native (non-CM) app whose backing +storage is an mmap'd flat buffer, and will reimplement the scheduler in C++. +Two of his needs shaped this plan: bit-exact dapp-state mirroring from the WS +feed, and an ergonomic/efficient `Application` dump story. We break APIs freely +at this stage — no backward-compatibility constraints. + +| # | Track | Owner | Status | +|---|-------|-------|--------| +| 1 | PR #26 WS context fields + L1 provenance | Stephen | **done — merged to main** | +| 2 | Restore `docs/review/` ledger + this plan | us | this PR | +| 3 | Feed & replay protocol redesign (type-first; generation id; historical replay) | us (design) → us/Stephen (impl) | **design drafted** — [`2026-07-track3-feed-replay-design.md`](2026-07-track3-feed-replay-design.md), under review | +| 4 | Storage decode policy (behavior + docs) | us | landed with this PR | +| 5 | Fee exponentiation LUT | us | **deferred** — decided exact-floor if built; log-space fees may become defunct (pending a separate design decision, syncing with Bart) | +| 6 | Dump / `Application` API redesign | us + Bart | **design drafted** — [`2026-07-track6-dump-api-design.md`](2026-07-track6-dump-api-design.md), under review | +| 7 | LLM context-engineering review (CLAUDE/AGENTS/docs/READMEs) | us | queued; run in a fresh session | + +**Campaign schedule (2026-07-28).** One session per bullet, roughly in order; +design docs reviewed by the team + Bart between sessions: + +1. *(now)* Land this PR (ledger + plan + decode policy, rebased over #26). + Draft the Track 3 + Track 6 design docs — these lean on investigation + context already loaded, so drafting early beats re-deriving it later. +2. Track 3 implementation after design review — likely staged PRs: replay + endpoints first (new, additive), then the WS protocol rework. +3. Track 6 implementation after the design settles with Bart. +4. Track 7 — fresh session, docs-only campaign (see its section). +5. Track 5 (fee LUT) — **deferred**: a separate pending design decision may + make log-space fees defunct entirely; revisit after syncing with Bart. + +Deferred (revisit with libdex rollout): multi-file/tar snapshot serving +(`docs/snapshots/lifecycle.md` known limitation), pending-snapshot-pool cap. + +--- + +## Track 1 — PR #26 (done) + +What landed: `OrderedL2TxRow` enum on the egress read; user-op +`nonce`/`safe_block`/`batch_nonce` and direct-input +`input_index`/`batch_nonce`/`block_timestamp`/`transaction_hash` on the WS +feed; `live_start_offset` appended to the catch-up-exceeded close reason +(stable-prefix contract on `WS_CATCHUP_WINDOW_EXCEEDED_REASON`); direct-input +L1 provenance persisted on `safe_inputs` (timestamp sourced from the +`EvmAdvance` payload — what the CM guest sees); mirror-clock and nonce +cross-check assertions in the replay harness. + +Explicitly **not** addressed there (moved to Track 3): the recovery +discontinuity contract — see F7 in +[`../review/2026-06-10-correctness-review.md`](../review/2026-06-10-correctness-review.md). + +## Track 2 — Ledger restore (this PR) + +`docs/review/` was dropped by the `8ce41cf` squash; restored from `0561d53` +with F7/WP5 statuses updated. The ledger is again the place PR reviews cite. + +## Track 3 — Feed & replay protocol redesign + +The current protocol grew ad hoc and is due a type-first redesign rather than +further field patches. Scope grew 2026-07-28: Bart needs **historical +replay** — the subscription's catch-up depth is shallow, so a consumer today +cannot bootstrap from genesis. The redesign covers the whole consumer +data-access story: paginated finalized-history endpoints + the live +subscription, composable without races. + +**Historical replay requirements (from Bart, 2026-07-28):** + +- Two paginated, finalized-only replay endpoints: + 1. **Input-box order** — the raw L1 sequence from `safe_inputs`: direct + inputs and our own batches, original order, with the PR #26 provenance + (block number/timestamp, tx hash) available per row. + 2. **Ordered L2-tx feed order** — the same coordinate space as the WS feed, + capped at the recovery-stable boundary: rows covered by + scheduler-accepted (gold) batches cannot be invalidated by recovery, so + pages below that boundary are immutable and **generation-free** (no + generation token needed; infinitely cacheable). +- **Race-free handoff:** client replays history to boundary G, then + `subscribe(generation, from_offset = G)`. The subscription window must + always cover `(G, live head]` — i.e. the catch-up depth must exceed the + finalized threshold's lag behind the live head at all times. Design + question to settle: a static cap cannot guarantee this (the soft suffix + grows unboundedly under an L1 outage), so either the cap becomes dynamic + ("everything above the gold boundary is always serveable over WS") or the + handoff contract defines a retry loop (replay again — the boundary + advanced — then subscribe). Prefer the dynamic guarantee; it makes the + pair a closed system. +- A recovery between replay and subscribe is already handled by the + generation contract: finalized pages stay valid, the subscribe is rejected + as stale, the client re-fetches only the suffix. Current shape: `GET /subscribe?from_offset=N` → +upgrade → unframed infinite stream of a two-variant enum +(`BroadcastTxMessage`), with all session-level signaling smuggled into +transport close frames. No handshake, no control plane, no +catch-up/live marker, no recovery signal. + +Design requirements: + +- **Generation id ("reality version")**, incremented per recovery cascade. + Crucial constraint: recovery follows a crash or a danger-detector process + exit, so a farewell "bad things happened" frame **cannot be load-bearing** — + the dying process isn't there to send it. Discontinuity detection must be + pull-based: a handshake/hello message carrying `{generation, live head, + window caps}`, plus a dedicated endpoint returning the current generation. + An in-band discontinuity event is welcome as best-effort for graceful paths. +- Reconnect becomes `subscribe(generation, offset)` with a typed rejection + when the generation is stale (replaces the interim "rebuild on any drop" + consumer rule recorded under F7). +- **Contract confirmed with Bart (2026-07-28).** His paraphrase, from the + one-sentence pitch: subscribe passes the generation to resume from and the + server replies with its current generation; a stale generation is rejected + with an error; if the stream state becomes invalid the server disconnects + with an error saying "roll back and recover" and bumps its generation. + That is the intended contract, with one caveat to keep explicit in the + design doc: the disconnect-with-error is **best-effort only** (a crash or + danger-detector exit cannot send a farewell frame), so the load-bearing + detection remains the stale-generation rejection at resubscribe plus the + generation endpoint. +- Structured session errors (e.g. catch-up-window-exceeded as a typed message + carrying `live_start_offset` and the snapshot-resync recipe) instead of + close-reason string smuggling. +- Candidate framing to evaluate: the feed as the sequencer's **reified + journal** — the same event vocabulary the fold/catch-up replay consumes, so + a mirror is literally running the fold. Decide: explicit + `FrameSealed`/`BatchSealed` boundary events (normalized, matches the fold) + vs. per-row denormalized context (what PR #26 ships; friendlier to stateless + consumers). Settle in the design doc with Bart. +- Agenda item shared with Track 6 (duality): the WS now carries direct-input + `block_timestamp`, but `Application::execute_direct_input` receives + `DirectInput{sender, block_number, payload}` — no timestamp — and the native + fold's input likewise. If a canonical (guest) execution reads + `EvmAdvance.blockTimestamp` into state, native soft execution cannot be + bit-exact. Either `DirectInput` grows `block_timestamp` or the application + contract explicitly forbids timestamp-dependent state transitions. Must be + settled before libdex's state layout freezes. + +Closes F7/WP5 when done. + +## Track 4 — Storage decode policy + +`sequencer/src/storage/convert.rs` module docs promise saturating decodes so +corrupted/malicious DB rows can't crash the process; `docs/invariants.md`'s +fail-loud check policy mandates the opposite for contract-impossible values; +actual call sites mix both (e.g. `decode_l2_tx_row` expects/asserts, adjacent +width conversions saturate). + +Agreed direction: **fail-loud wins** for contract-impossible values — a +saturated fabricated value flowing to a bit-exact mirror is precisely the harm +the policy exists to prevent. Saturating helpers remain only where the full +numeric range is legal (pure width conversions). + +**Resolution (landed with this PR, after a 116-call-site audit):** the split +is by conversion family, not per site. Width converters +(`i64_to_u64/u16/u32`, `u64_to_i64`, `usize_to_i64`) now fail loud — every +non-test call site was contract-bound. Three principled exceptions: clock +conversions stay saturating (wall-clock is environmental, review F8) though +`from_unix_ms` fail-louds on sign; untrusted/config-sourced SQL bounds go +through a new explicitly-named `saturating_query_bound` (WS cursors, page +limits — clamping preserves comparison semantics); and the two fee-policy +reads in `query_batch_policy` keep a documented local clamp (legal admin +ranges, tested in admin.rs). Also fixed in passing: two adjacent +`saturating_add`s on contract-bound counters became `checked_add`, and +`safe_accepted_batches.nonce` gained its missing `CHECK (nonce >= 0)`. + +## Track 5 — Fee exponentiation LUT + +Problem: `fee_to_linear` is consensus-critical (scheduler fold, guest T3 +agreement, app charging) and must be bit-identical across the Rust sequencer, +the guest, and Bart's C++ scheduler. Today's implementation +(`sequencer-core/src/fee.rs` + `build.rs`) shares a 15-entry table of +`(129/128)^(2^i)` squares, but a reimplementation must also reproduce +`fixed_mul` exactly: 256×256→512 widening multiply, `>> 64`, truncate, in the +same accumulation order. That is unreasonable to demand of a C++ port. + +Plan: replace the arithmetic with a full lookup table — one `U256` per +exponent, `0..=MAX_EXPONENT` (~17k entries ≈ 550 KB; not 64Ki — exponents +above `MAX_EXPONENT` panic today and continue to). The table becomes the +cross-implementation **spec artifact**: checked-in binary/hex file + golden +hash test; `build.rs` verifies rather than generates; C++ consumes the same +file byte-identically. `fee_to_linear` = `TABLE[n]`. `fee_from_linear` / +`log_fee_ratio` are admin-side only (`set_alpha`) — derived binary searches +over the table with a documented tie rule; they need no cross-implementation +guarantee. + +Decisions (2026-07-29): **exact-floor** — if built, the table is regenerated +as exact `floor((129/128)^n)` bignum values (the table *is* the spec, +algorithm-free); today's iterated-squaring outputs are not frozen, so replay +continuity across the upgrade is explicitly not preserved. **Deferred** — a +separate pending design decision may make log-space fees defunct entirely; +do not implement until that lands (syncing with Bart). + +## Track 6 — Dump / `Application` API redesign + +`create_dump` / `from_dump` / `delete_dump` / `state_file_in_dump` is a leaky +projection of what the inclusion lane actually needs. Requirements inventory +(from the 2026-07-22 trace of every call site): + +- **R1** checkpoint current state at batch close — atomic, crash-durable + (fsync-before-DB-row, invariant I13), exactly once per batch close. +- **R2** reconstruct at startup: latest checkpoint + replay. +- **R3** dispose checkpoints (GC + orphan sweep). +- **R4** serve canonical bytes over HTTP **without instantiating the app** + (all `state_file_in_dump` exists for — it conflates "locate the artifact" + with "the artifact is one literal file whose bytes are canonical"). +- **R5** genesis (already off-trait, stays there). + +Wished-for: cheap checkpoints (CoW), checkpointing off the lane thread (today +a full-state serialize stalls soft-confirmation acks at every batch close), +and the load-bearing fork — the app running against an mmap'd working image +the lane can flush-then-clone (the Dave sling model; libdex's natural shape). + +Findings from the CM/Dave comparison that constrain the design: + +- The CM emulator has **no commit/revert** — its API is + `load / store / clone_stored / remove_stored`; Dave's commit/revert are + node-level orchestration on top of `clone_stored`. The sequencer already + has both concepts sequencer-side (DB row as commit point; older-dump+replay + as revert) and they should stay off the trait. +- The genuinely missing primitive is **cheap clone** + (reflink/`FICLONE`/`clonefile`, hardlink read-only, graceful full-copy + fallback). But a bare `clone_dump` patch only pays off after the + working-image fork above is decided — so evaluate the fuller redesign + first: e.g. a session-shaped API (app handle opened *on* a working image, + `checkpoint() → SealedCheckpoint`, `canonical_bytes(checkpoint) → impl + Read` replacing `state_file_in_dump`). +- **Durability postures are opposite** and must not be silently inherited: + the sequencer mandates fsync inside the checkpoint (I13); CM/Dave fsync + nothing and compensate with hash-on-load + older-boundary fallback. Keep + the app-fsync posture; a CoW implementation must fsync what reflink leaves + unsynced. Tell Bart explicitly — he wrote the CM side. +- **libdex layout constraint to communicate early:** the served canonical + file must byte-match the canonical machine's `inspect_state` output (the + watchdog byte-compares). A raw mmap buffer with allocator padding, + free-lists, or pointer-valued fields breaks that: either the buffer layout + is itself canonical, or libdex needs a separate canonical projection + (which reintroduces serialize cost). +- Who implements what today: all four dump verbs are required app-author + implementations (no defaults); there is no `restore_dump` — the restore + half is `from_dump`. The sequencer owns `info.toml`, the + `dumps//{state,info.toml}` wrapper, DB rows, promotion, GC, leases, + orphan sweep. +- Fold-in from PR #26: the `SafeInputRecord` shim in + `sequencer/src/storage/l1_inputs.rs` (zero-provenance `StoredSafeInput` + vs. `IngestedSafeInput`) is churn-avoidance, not design — collapse to one + honest row model when the `DirectInput`/`block_timestamp` duality question + (Track 3 agenda) is settled. + +**New CM development to draw on (2026-07-28, Diego, +machine-emulator PR #398):** running an epoch directly from storage exposed an +API inconvenience, fixed by adding `machine:rename_stored` and making both it +and the existing `remove_stored` **durable (automatically synced)**. Two +implications for us: (a) the durable-rename primitive is exactly the +commit-point idiom the sequencer's dump lifecycle hand-rolls (temp + rename + +fsync ladder) — a redesigned trait could make sealing a sequencer-side +`rename`-based operation the app never implements; (b) the CM's no-fsync +posture we flagged as a divergence is narrowing on the CM side — re-check the +posture comparison against #398 before finalizing the design's crash-safety +section. + +Deliverable: design note in `docs/`, written jointly with the Track 3 doc and +put in front of Bart together — his on-disk layout decision depends on both. + +## Track 7 — LLM context-engineering review + +Review the repo's whole LLM-facing setup — and the human docs, which double +as agent context — against Anthropic's Claude-5-generation context +engineering guidance +(). +Distilled rules (kept in-tree so the campaign doesn't depend on the URL): + +1. **Unhobble** — remove overconstraining/conflicting instructions; let the + model use judgment from surrounding context. +2. **Rules → principles** — replace explicit constraints with principles + ("match surrounding style", not "never do X"). +3. **Examples → interfaces** — design expressive file structures and tool + parameters instead of constraining by example. +4. **Progressive disclosure** — don't front-load; move detailed guidance into + skills/docs loaded on demand. +5. **No repetition** — eliminate guidance duplicated across files; one home + per instruction. +6. **Auto-memory over manual memory notes** in CLAUDE.md. +7. **Rich references over simple specs** — point at code, tests, rubrics + rather than restating them in prose. +8. **CLAUDE.md stays lightweight** — repo purpose + repo-specific gotchas + only; drop anything the model can discover itself. +9. **Skills as lightweight discovery guides** — split long ones, encode + team-specific opinion, avoid over-constraint outside critical areas. + +Scope: `CLAUDE.md`, `AGENTS.md`, everything under `docs/`, the human +`README.md`s, and any `.claude/` assets. Watch for: duplication between +CLAUDE.md and AGENTS.md, front-loaded detail that belongs in the normative +docs, and stale pointers (the `docs/review/` dangle was one). Run as its own +fresh session (docs-only; benefits from clean context and a fan-out audit). diff --git a/docs/plans/2026-07-track3-feed-replay-design.md b/docs/plans/2026-07-track3-feed-replay-design.md new file mode 100644 index 0000000..e0c64e2 --- /dev/null +++ b/docs/plans/2026-07-track3-feed-replay-design.md @@ -0,0 +1,206 @@ +# Feed & Replay Protocol — Design Draft (Track 3) + +**Status: draft for review** (us, Stephen, Bart). Supersedes the ad-hoc WS +protocol; closes review F7/WP5. On acceptance, the normative parts graduate +into `docs/protocol/` and the README's WS section is rewritten against them. + +## 1. Motivation + +The current protocol is an unframed infinite stream of a two-variant enum +over a WS socket, with every session-level signal smuggled into transport +close frames. Three defects drive the redesign: + +- **F7 (high, open):** no invalidation/rollback signal. Recovery cascades + invalidate already-streamed rows and re-sequence replacements at higher + offsets under a *reused* batch nonce, so a cursor-resumed mirror silently + diverges. The interim rule ("treat any socket drop as a discontinuity and + rebuild the soft suffix") works but is undocumented, wasteful, and easy to + get wrong. +- **No historical bootstrap:** the catch-up window is shallow (static cap); + a consumer cannot build state from genesis over the feed at all. +- **No control plane:** policy violations are prose in close reasons; + the catch-up→live transition is unmarked; there is no handshake. + +Consumer model (Bart's libdex is the concrete instance): replay finalized +history via HTTP, then subscribe for the soft tip, maintaining a bit-exact +mirror of the dapp state — with recovery detectable and recoverable. + +## 2. Concepts and coordinates + +- **Input-box coordinate** `input_index: u64` — position in `safe_inputs` + (per-application InputBox order). Append-only, sourced from L1 safe + blocks: immutable once present. +- **Feed coordinate** `offset: u64` — position in the ordered L2-tx stream + (`valid_sequenced_l2_txs` rowid). Append-only, **but** rows above the gold + boundary can be invalidated (hidden, then re-sequenced at new offsets) by + recovery. +- **Gold boundary `G`** — the greatest offset belonging to a batch that is + scheduler-accepted on L1 (`safe_accepted_batches` frontier). Rows with + `offset <= G` are recovery-stable: they can never be invalidated. `G` only + advances. +- **Generation `g: u64`** — the stream's "reality version". Bumped by every + recovery cascade. Two events with the same offset but different + generations may differ; everything at or below `G` is generation-free. +- **Live head `H`** — the greatest currently-valid offset. + +## 3. Generation semantics + +- Persisted in a new singleton (`feed_generation`), read at subscribe time + and bumped **in the same transaction** as `cascade_invalidate_from` (both + the startup-recovery path and `setup --recovery`'s rebuild path). +- Monotonic across DB rebuilds: `setup --recovery` reconstructs the DB from + a dump + L1, so the generation must be carried in the dump's sequencer + sidecar (`DumpInfo` gains a `feed_generation` field) and bumped once on + every rebuild. Without this, a rebuilt deployment would reset to 0 and a + mirror would mistake it for a fresh reality. (Open question 3 if we would + rather derive it from something already durable.) +- Exposed three ways: `GET /generation` (trivial JSON, cheap to poll), the + subscribe handshake (§5), and best-effort in-band `Discontinuity` events. + +## 4. Historical replay endpoints (HTTP, paginated, finalized-only) + +Both endpoints serve **immutable data only** and therefore carry no +generation. Pages are stable forever: aggressively cacheable, safe to +mirror, resumable at any time. + +### 4.1 `GET /inputs?from_index=N&limit=K` + +Raw input-box order over `safe_inputs`: direct inputs **and** our own +batches, exactly as L1 ordered them, each row with the PR #26 provenance: + +```json +{ "items": [ { "input_index": 7, "sender": "0x…", "payload": "0x…", + "block_number": 123, "block_timestamp": 1700000000, + "transaction_hash": "0x…" } ], + "next_index": 8, "end_index": 41 } +``` + +`end_index` is the current exclusive upper bound of the table. Rows appear +here once the L1 safe head passes them — this endpoint's data is "safe" in +the L1 sense by construction. + +### 4.2 `GET /l2-txs?from_offset=N&limit=K` + +Feed order, **capped at the gold boundary `G`**. Items use the same shapes +as the WS data events (§5.3) so a replay page and a live event are +interchangeable inputs to a mirror: + +```json +{ "items": [ /* same JSON shapes as WS user_op / direct_input events */ ], + "next_offset": 101, "gold_boundary": 250 } +``` + +The server never serves `offset > G` here. A client sees `next_offset > +gold_boundary` as "history exhausted — switch to the subscription". + +Storage note: `G` is computable as the max offset of rows whose batch nonce +is ≤ the `safe_accepted_batches` frontier nonce; add a dedicated indexed +query, computed per page request (it only advances, so a stale read is +merely conservative). + +## 5. WS subscription v2 + +### 5.1 Handshake + +`GET /subscribe?from_offset=N[&generation=g]` upgrades, then the server's +**first frame is always** `hello`: + +```json +{ "kind": "hello", "generation": 4, "live_head": 312, "gold_boundary": 250, + "max_subscribers": 64 } +``` + +- If the client supplied `generation` and it is stale, the server sends + `{ "kind": "error", "error": "stale_generation", "current_generation": 4 }` + after `hello`, then closes 1008. The client rebuilds its soft suffix + (everything above its last known `G`) and resubscribes. +- Omitting `generation` means "no continuity claim" — accepted, and the + client must treat its own soft suffix as void (fresh-mirror semantics). + +### 5.2 Depth guarantee (replaces the static catch-up cap) + +**Invariant: any `from_offset >= G` is always serveable.** The static +50k-event cap is replaced by the rule "the WS covers everything above the +gold boundary; HTTP replay covers everything at or below it." The soft +suffix is exactly the region a mirror cannot get elsewhere, so the server +must never refuse it. A `from_offset < G` request gets: + +```json +{ "kind": "error", "error": "below_gold_boundary", "gold_boundary": 250 } +``` + +then a 1008 close — the client pages `/l2-txs` up to `G` first. This makes +the replay+subscribe pair a closed system with a two-step client loop: + +1. Page `/l2-txs` until `next_offset > gold_boundary` (call it `G0`). +2. Subscribe `from_offset = G0, generation = g` (from `/generation` or the + last `hello`). On `stale_generation`: drop the soft suffix, goto 1 (only + the suffix above the *new* `G` needs re-pulling — `G` advanced past + everything that settled meanwhile). On `below_gold_boundary`: goto 1. + +No race window: between steps 1 and 2, `G` can only advance, and rows in +`(G0, G]` are still serveable over WS by the invariant (they were above the +boundary when sequenced and remain valid). + +### 5.3 Events + +Data events keep PR #26's **denormalized per-row context** (`user_op` / +`direct_input` with nonce, safe_block, batch_nonce, input_index, +block_timestamp, transaction_hash). Rationale: Bart has confirmed this field +set suffices for a bit-exact mirror; stateless consumers need no +boundary-tracking; and the replay endpoint reuses the shapes verbatim. +Normalized `frame_sealed`/`batch_sealed` boundary events are **rejected for +now** (open question 1 records the revisit trigger). + +Control events (new, same tagged enum): + +- `hello` — §5.1. +- `live` — sent once when catch-up reaches the live head; everything after + is a soft confirmation in real time. (Consumers that only care about + settled state can hold their render until this.) +- `discontinuity { new_generation }` — **best-effort only**: sent on + graceful invalidations. A crash or danger-detector exit sends nothing — + the load-bearing detection is `stale_generation` at resubscribe. Mirrors + MUST NOT rely on receiving this event. +- `error { error, … }` — typed, replaces close-reason prose. The socket + still closes 1008 after; the close reason becomes advisory. + +### 5.4 Wire format + +JSON text frames, serde-tagged (`"kind"`), new fields mandatory (we break +old clients freely). `WsTxMessage = BroadcastTxMessage` stays the single +shared type; control events join the same enum so the SDK matches +exhaustively. + +## 6. What this replaces + +- `WS_CATCHUP_WINDOW_EXCEEDED_REASON` and the `live_start_offset` close + reason — superseded by `below_gold_boundary` + `/l2-txs`. +- The interim "rebuild on any socket drop" rule (F7) — superseded by the + generation contract; the rule survives only as the omitted-generation + fallback (§5.1). +- README's WS section — rewritten from this doc. + +## 7. Implementation stages (separate PRs) + +1. **Replay endpoints** — additive: the two HTTP routes + gold-boundary + query + tests (pagination, immutability across a forced cascade, + provenance fields). No WS changes; ships value to Bart immediately. +2. **Generation plumbing** — `feed_generation` singleton + bump-in-cascade + + `DumpInfo` carry + `GET /generation`. Still no WS change. +3. **WS v2** — handshake, control events, depth-guarantee rework, SDK + + harness update, README rewrite, F7 ledger tick. + +## 8. Open questions + +1. **Boundary events**: revisit if a consumer ever needs frame/batch + boundaries the denormalized rows can't express (e.g. exact frame fee + schedule per epoch). Trigger: a second consumer with fold-shaped needs. +2. **`/inputs` shape**: confirm with Bart how his consumer reconciles + against raw L1 order (vs. only feed order + L1 settlement via + batch_nonce) so 4.1 serves exactly that need. +3. **Generation continuity across rebuilds**: `DumpInfo` carry (proposed) vs + deriving from an existing durable counter. DumpInfo carry is simple but + means generation is only as durable as the promoted dump. +4. **Auth/limits**: replay endpoints are unauthenticated reads like + `/latest_snapshot`; decide rate limits before public exposure. diff --git a/docs/plans/2026-07-track6-dump-api-design.md b/docs/plans/2026-07-track6-dump-api-design.md new file mode 100644 index 0000000..ccbc1df --- /dev/null +++ b/docs/plans/2026-07-track6-dump-api-design.md @@ -0,0 +1,181 @@ +# Dump / `Application` API — Design Draft (Track 6) + +**Status: draft for review** (us, Bart). Companion to the Track 3 feed +design — review together; libdex's on-disk layout depends on both. On +acceptance, the trait contract graduates into +`docs/protocol/application-contract.md` and the lifecycle changes into +`docs/snapshots/lifecycle.md`. + +## 1. Motivation + +Three forces, one API: + +- **Cost.** `create_dump(&self)` is a full O(state) serialize + 3-fsync + ladder, run synchronously on the single lane thread at every batch close — + soft-confirmation acks stall for the duration. Tolerable for the toy + wallet; not for a multi-GB flat buffer. +- **Bart's app shape.** libdex runs natively against an mmap'd flat state + buffer — the Cartesi Machine's own storage approach (Bart implemented that + feature), whose ecosystem exploits CoW (`clone_stored`: reflink/hardlink + with plain-copy fallback; ~2.6 ms vs ~350 ms full store at 533 MB in + Dave's measurements). +- **Verb review.** `create_dump / from_dump / delete_dump / + state_file_in_dump` is a leaky projection of what the lane needs; the + 2026-07 investigation mapped it against the CM/Dave verb set. + +Key investigation results this design builds on: the CM emulator has **no +commit/revert** — those are node-level orchestration over `clone_stored`, +and the sequencer already has both (DB row as commit point; older-dump + +replay as revert) correctly *off* the trait. The one missing primitive is +**cheap clone**. And the CM has since moved our way on durability: +machine-emulator PR #398 adds `rename_stored` and makes it and +`remove_stored` **durable (auto-synced)** — the commit-point idiom our dump +lifecycle hand-rolls. + +## 2. Requirements + +From the lane trace (R) and the wishlist (W): + +- **R1** Checkpoint the current state at batch close — atomic, + crash-durable *before* the DB row lands (invariant I13). +- **R2** Reconstruct at startup: latest checkpoint + replay. +- **R3** Dispose checkpoints (GC + orphan sweep). +- **R4** Serve canonical bytes over HTTP without instantiating the app; + bytes must equal the canonical machine's `inspect_state` output (the + watchdog byte-compares). +- **R5** Genesis construction (off-trait today, stays off-trait). +- **W1** Checkpoints cheap enough to not shape batch policy (CoW). +- **W2** Checkpointing off the ack path. +- **W3** Natural fit for an mmap'd-working-image app without penalizing + pure-RAM apps (WalletApp). + +## 3. The fork, decided: working-image model + +The investigation flagged one load-bearing fork: cheap clones require the +app to run against an on-disk image the lane can flush-then-clone (Dave's +`SHARING_ALL` model); `create_dump(&self)` serializing live RAM can never be +cheap. **This design takes the working-image model** — it is libdex's +natural shape, it is what the CM ecosystem optimizes, and WalletApp adapts +trivially (its "working image" is a file it rewrites on flush; cost +unchanged from today's serialize). + +## 4. Proposed trait + +```rust +pub trait Application: Send + Sized { + // --- execution surface unchanged --- + + /// Open the app on a working image directory. The sequencer owns the + /// directory's lifecycle; the app owns its contents. Called at startup + /// (from a cloned checkpoint) and after genesis materialization. + fn open(working: &Path) -> Result; + + /// Make the working image consistent and durable on disk: flush + /// app-level caches, msync mapped pages, fsync files. After `Ok`, the + /// on-disk image alone reconstructs this exact logical state via + /// `open`. Called by the lane at batch close, before cloning. + fn flush(&mut self) -> Result<(), AppError>; + + /// Clone the (flushed, not currently open) image at `from` into `to` + /// (must not exist). Default: recursive plain copy + fsync ladder. + /// CoW apps override with reflink/hardlink (FICLONE / clonefile), + /// keeping the same durability contract: on `Ok`, `to` survives an + /// immediate kernel crash. + fn clone_image(from: &Path, to: &Path) -> Result<(), AppError> { … } + + /// Delete an image directory the sequencer no longer references. + /// Default: remove_dir_all. + fn delete_image(prefix: &Path) -> Result<(), AppError> { … } + + /// Locate the single canonical state file inside an image without + /// opening the app. Contract unchanged from state_file_in_dump: + /// the file's bytes equal canonical `inspect_state` output (R4). + fn canonical_file_in_image(prefix: &Path) -> PathBuf; +} +``` + +Verb mapping: `open` ≈ CM `load(SHARING_ALL)`; `flush` ≈ the msync the CM's +dirty-page sidecars make optional; `clone_image` ≈ `cm_clone_stored`; +`delete_image` ≈ `remove_stored`. Commit stays sequencer-side (DB row; +sealing by durable rename per PR #398's precedent). Revert stays +sequencer-side (older checkpoint + replay). `from_dump`/`create_dump` +disappear: restore is `clone_image(checkpoint, working)` + `open(working)`; +checkpoint is `flush()` + `clone_image(working, checkpoint)`. + +## 5. Lane lifecycle changes + +Batch close becomes: `flush()` → `clone_image(working, staging)` → +sequencer writes `info.toml` + durable-renames staging into the dumps dir → +DB row in one tx (unchanged commit point). Filesystem-first ordering, the +"orphan dir possible, dangling row never" invariant, promotion, GC, leases, +and the whole `snapshot_dumps.rs` layer are **unchanged** — the storage half +is already representation-agnostic (opaque prefix keys). + +W2 (off-ack-path) falls out for CoW apps: `flush` + reflink is +milliseconds, and the expensive part (page write-back) is the kernel's +business afterward. A dedicated async stage is *not* designed in; if a +non-CoW app's flush is slow, that is the app's cost to fix by adopting CoW. + +Startup (R2): clone the promoted/pending checkpoint into a fresh working +dir, `open`, replay. The working dir is disposable state — never promoted, +never served, deleted on clean start. + +## 6. Crash-safety posture (unchanged, now sharper) + +I13 stays: nothing may reach the DB before the corresponding image is +durable. The split is now explicit: the **app** guarantees durability of +image *contents* (`flush`, `clone_image`); the **sequencer** guarantees +durability of *directory structure* (rename + dir-fsync — exactly what CM +PR #398 now bakes into `rename_stored`/`remove_stored`, validating the +posture). Tell Bart directly: the CM's historical no-fsync stance does not +apply here — a CoW `clone_image` override must fsync what reflink leaves +unsynced, and #398 shows the CM itself now agrees for the rename/remove +verbs. + +## 7. Serving (R4) and the libdex layout constraint + +`canonical_file_in_image` keeps the single-canonical-file contract — the +HTTP snapshot routes, lease protocol, and watchdog byte-compare all survive +untouched. The constraint to put in front of Bart *before* libdex's layout +freezes: the served file must byte-match canonical `inspect_state` output, +so either (a) the flat buffer's layout is itself canonical — fully +normalized, no allocator padding, no free-lists, no pointer-valued fields, +no uninitialized gaps — and the buffer file doubles as the canonical file; +or (b) libdex writes a separate canonical projection during `flush`, which +reintroduces O(state) serialize cost and partly defeats CoW. (a) is the +performant answer and a real design constraint on his buffer format. + +## 8. Migration & cleanups folded in + +- **WalletApp:** `open` mmap-or-reads its file; `flush` = today's + serialize+fsync ladder; defaults cover the rest. No capability lost. +- **Genesis:** concrete-type constructor materializes the initial working + image, then the normal flush/clone path checkpoints it (R5 unchanged). +- **`SafeInputRecord` shim** (`storage/l1_inputs.rs`): collapse + `StoredSafeInput`/`IngestedSafeInput` into one honest row model when the + `DirectInput` provenance decision below lands. +- **`DirectInput` timestamp duality** (shared agenda with Track 3): the WS + now carries `block_timestamp`, but `execute_direct_input` and the native + fold never see it. If canonical (guest) execution reads + `EvmAdvance.blockTimestamp` into state, native soft execution cannot be + bit-exact. Decide with Bart: either `DirectInput` grows `block_timestamp` + (fold + lane + trait signature change), or the application contract + explicitly forbids timestamp-dependent state transitions. Blocks libdex's + state design either way — settle it in the same review. + +## 9. Open questions + +1. **Working-image locking:** the CM uses flock to make `SHARING_ALL` + exclusive. Do we require the app to hold an equivalent lock, or does the + sequencer's single-lane discipline suffice? (Lean: sequencer discipline + suffices; a lock is cheap defense — app's choice.) +2. **`flush` durability scope:** must `flush` fsync, or is fsync deferred to + `clone_image`? (Lean: `clone_image` owns durability of the *clone*; + `flush` owns consistency of the *source* — msync yes, fsync optional.) +3. **Non-reflink filesystems:** plain-copy fallback makes checkpoint cost + O(state) again silently. Log loudly at startup when the dumps dir does + not support reflink? (Lean: yes — one probe at bootstrap.) +4. **Dirty-page tracking for hashing/inspect:** the CM's `.dpt` sidecars + make post-clone hashing touch only dirty pages. libdex would need its + own write-barrier to replicate; out of scope for the sequencer API but + worth flagging to Bart as a cost driver for (a) in §7. diff --git a/docs/review/2026-06-10-correctness-review.md b/docs/review/2026-06-10-correctness-review.md new file mode 100644 index 0000000..8127f75 --- /dev/null +++ b/docs/review/2026-06-10-correctness-review.md @@ -0,0 +1,886 @@ +# Whole-Project Correctness Review — 2026-06-10 + +**Status:** findings ledger, fixes pending. Each finding carries a checkbox; tick it +(and link the commit/PR) when resolved. Design resolutions in §2 were settled in the +review discussion and are the agreed direction; everything else is open. + +**Provenance note (2026-07-23):** `docs/review/` was accidentally dropped from the +squash that merged the cockroach branch to main (`8ce41cf`) — AGENTS.md/CLAUDE.md +kept pointing at it while it was absent. Restored from `0561d53`, with statuses +updated to reflect what landed since (see F7/WP5). + +**Method.** Twelve parallel module reviews (one per subsystem, each reading code + +normative docs), every medium/high concern independently adversarially verified +against the tree (verifiers instructed to refute), plus a line-by-line pass over the +core files (scheduler fold, `scheduler_accepts`, inclusion lane, ingress/recovery +storage, schema, catch-up, snapshot lane, flusher). Refuted concerns are kept in §6 +so future reviews don't re-litigate them. + +**Headline.** The sequencer/scheduler duality itself is in good shape — the three +implementations of "what does the scheduler accept" (canonical fold, +`ProtocolTiming::scheduler_accepts`, the lane's live prediction) were checked +against each other from multiple angles and are consistent, including drain +attribution, staleness boundaries, flattened replay order, and empty-batch +semantics. The confirmed problems cluster at the boundary between the sequencer +and the infrastructure it stands on: fsync semantics, the local node's mempool +memory, RPC fleet coherence, and the subscriber protocol. + +Severity: **high** = potential scheduler/sequencer state divergence, fund loss, +crash-loop, or data corruption; **medium** = real correctness issue with bounded +blast radius; entries in §4/§5 are robustness/hygiene. + +--- + +## 1. Confirmed findings + +### F1 (high) — Flush slot coverage has no durable anchor; zombie batches re-enable the counterexample TLA+ excludes + +- [x] Fixed in: WP2, 2026-06-11 — R1a watermark landed: `wallet_nonce_watermark` singleton, write-before-broadcast in the poster + flusher (`WalletNonceWatermarkSink`), flush completion anchored on `safe >= watermark + 1` (Anvil test `flush_covers_watermark_slots_the_pool_forgot`). The R2 content-identity backstop landed in WP3 (2026-06-12), completing the loop. + +`docs/recovery/README.md` Implementation Constraint 1 requires `walletNonce` — the +highest wallet-nonce slot ever assigned — to be durable and never reset, so the +flush consumes every dead-batch slot. The implementation re-derives it from the +local node's volatile view at every use: + +- `MempoolFlusher::flush_and_wait` returns immediately when + `get_transaction_count(Pending) <= get_transaction_count(Safe)` + ([flusher.rs:122-131](../../sequencer/src/recovery/flusher.rs)) and submits + no-ops only for `[Latest, Pending)` per the local node. +- The poster assigns new slots from `get_transaction_count(Latest)` + ([poster.rs](../../sequencer/src/l1/submitter/poster.rs)). +- The schema persists no wallet-nonce bookkeeping at all. + +If a broadcast batch tx at slot *k* is dropped from the local node's pool (node +restart, pool eviction — fail-stop behavior the threat model tolerates) while +surviving in the wider network (the mempool is fully adversarial; zombies are a +named first-class threat), then at recovery: the flush sees `pending == safe`, +submits **zero** no-ops, and returns; `recover_post_flush` cascades the batch as +"Pending no-op'd"; the resumed submitter assigns the recovery batch (same +scheduler nonce N, different content) to the **same slot k**. Zombie and recovery +batch now compete for one slot — exactly the wallet-nonce-mutual-exclusion +counterexample in `docs/recovery/history/`. If the zombie lands fresh by +inclusion (the preemptive margin guarantees a window where this is possible), the +scheduler executes the *invalidated* content, `populate_safe_accepted_batches` +accepts it (nonce matches; content is never compared), and the system diverges +silently and permanently. + +**Fix (settled):** persisted wallet-nonce watermark — §2 R1. Detection backstop: +content-identity check — §2 R2. + +### F2 (medium) — Post-flush re-sync has no coherence check against the flusher's observation + +- [x] Fixed in: WP2, 2026-06-11 — `flush_and_wait` returns the safe block it observed resolution at; `run_flush_and_cascade` refuses to cascade (`RecoveryError::ResyncBehindFlushView`, orchestrator respawn retries) when the re-synced `current_safe_block` lags it + +`run_flush_and_cascade` ([recovery/mod.rs:329-372](../../sequencer/src/recovery/mod.rs)) +waits for `Pending <= Safe` on the flusher's provider connection, then re-syncs the +safe head through the input reader on a separate connection, then cascades +unconditionally. The documented precondition on +[`Storage::recover_post_flush`](../../sequencer/src/storage/recovery.rs) — the gold +frontier "must reflect the latest safe head" — is never verified. Behind a +load-balanced RPC endpoint (which `docs/recovery/README.md` itself names as the +most common real-world degradation), the reader can be served by a replica lagging +the flusher's view by minutes, miss batches that landed-and-finalized during the +flush window, under-extend the frontier, and cascade a batch the scheduler +actually accepted — the recovery batch then reuses its nonce: same divergence +shape as F1. + +**Fix:** `flush_and_wait` returns the safe block number at which it observed +resolution; `run_flush_and_cascade` asserts the re-synced `current_safe_block` +is `>=` that value before cascading (retry/respawn otherwise). + +### F3 (high) — External effects are emitted on non-durable WAL commits (`synchronous=NORMAL`) + +- [x] Fixed in: WP1 durability flip, 2026-06-11 (single-PR branch) — `synchronous=FULL` via the single `SYNCHRONOUS_PRAGMA` constant; round-trip benchmark deltas were noise-level on NVMe (p50 24.1 ms vs 23.8 ms baseline, p95 35.6 vs 35.8, concurrency 8) + +[`open.rs`](../../sequencer/src/storage/open.rs) sets WAL + `synchronous=NORMAL` on +all writer connections: commits survive process crash but **not** power loss / OS +crash before the next checkpoint. Two externalization points outrun durability: + +- The lane acks `POST /tx` immediately after the (non-fsynced) chunk commit + ([inclusion_lane/mod.rs:222-226](../../sequencer/src/ingress/inclusion_lane/mod.rs)). +- The submitter reads sealed batches and broadcasts them to L1. + +Worst case: batch B (scheduler nonce N) is sealed and its L1 tx mined; power loss +rewinds the DB so B is the open Tip again; the lane re-seals nonce N with +different content (B-v2); the scheduler executed B-v1 while local replay, +snapshots, and successor batches are built on B-v2 — silent divergence that no +danger arm detects (acceptance and promotion are keyed by nonce only). Lesser +case: acked user ops vanish outside the documented invalidation path. + +This is in-model, not theoretical: the repo's own crash bar includes kernel +crashes — `Application::create_dump` explicitly fsyncs against WAL/file +reordering. The DB is the unguarded half of an invariant the dump side already +pays for. + +Note: the re-seal divergence case is *detected* (not prevented) by §2 R2. + +### F4 (medium) — GC unlinks dump directories after a non-durable row delete + +- [x] Fixed in: WP1 durability flip, 2026-06-11 — under `synchronous=FULL` the GC's row-delete commit fsyncs before `delete_dump_dir` unlinks, so a power loss can no longer resurrect a row pointing at a missing directory + +`run_gc` ([snapshot.rs:61-73](../../sequencer/src/ingress/inclusion_lane/snapshot.rs)) +deletes `dumps` rows in one (non-fsynced) tx, then removes directories on disk. +Power loss before the next checkpoint can rewind the row delete while the unlink +persists: a resurrected `finalized_snapshot`/pending row points at a missing +directory. The startup sweep only handles the inverse (orphan dirs); startup GC +won't remove a still-referenced row. If the resurrected row is the resume +checkpoint, `A::from_dump` fails → lane exits → restart hits the same row → +**crash-loop with no automatic escape**, violating the documented "no dangling +row" invariant under the same kernel-crash model `lifecycle.md` §7 claims to +defend against. + +**Fix:** fsync/checkpoint the WAL after the GC commit and before `delete_dump` +(automatic under `synchronous=FULL`), or defer file deletion to the next startup. + +### F5 (high) — Safe-range scan trusts `get_logs` completeness against a separately-fetched safe head + +- [x] Fixed in: reader InputBox-index contiguity check (structural layer 2), + external review 2026-06. + +`advance_once` ([reader.rs:185-250](../../sequencer/src/l1/reader.rs)) fetches the +safe head (RPC call 1), issues `eth_getLogs` over `[floor+1, safe]` (call 2, +possibly served by a different backend), and persists the call-1 head on any `Ok`. +Geth-lineage nodes silently clamp `toBlock` to their local head and return partial +logs without error. Behind a load-balanced fleet, deposits in the tail of the +range are permanently skipped — the floor never re-scans below the persisted +head. The sequencer then advances frames' `safe_block` past a deposit it never +executed while the scheduler force-drains it: canonical state divergence. + +The threat model scopes this away ("Our own node; honest by assumption", +`docs/threat-model/README.md:51`), but nothing in code or configuration enforces +or documents single-consistent-node-only, and +[provider.rs](../../sequencer/src/l1/provider.rs) / +[partition.rs](../../sequencer/src/l1/partition.rs) explicitly anticipate +Infura/Alchemy/QuickNode. + +**Fix (two layers):** +1. Operational: after the logs query, confirm the serving node's view covers the + scanned end block (re-fetch head/safe and require `>= range end`), or pin the + scan end to the same response's view. +2. Structural (stronger): the reader currently **drops the on-chain input index** + carried by `EvmAdvanceCall`; persist it and reconcile against the local + `safe_input_index` assignment (`MAX+1`). Any gap → fail loud. This converts + silent input loss from any cause into a detected refusal, and is cheap. + +**Resolution (2026-06):** both layers landed in `advance_once`, run before the +safe head is persisted: +1. **Right prefix (contiguity).** It reads each `InputAdded.index` (per-app, + gap-free on-chain counter) and `check_input_index_contiguity` asserts the + ingested run continues densely from the stored count — equal to the assigned + local `safe_input_index`, since we ingest every event for this app from + genesis. Catches a dropped *middle* input. +2. **Complete prefix (count witness).** `check_input_count_complete` queries + `InputBox::getNumberOfInputs(app)` *pinned at the scanned safe block* and + requires it equals what we now hold (stored + received). Catches a truncated + *tail* — which is still a contiguous prefix, so step 1 alone would miss it — + on the same tick, before persisting. Pinning to the safe block also forces + the serving node to have that block's state, so it largely subsumes the + originally-proposed layer-1 same-view bound (a lagging replica errors or + returns the true count). + +Either failure returns a retryable `InputReaderError::Provider`: the partial set +is never persisted and a consistent provider self-heals on the next tick. +**Residual:** only a *byzantine* node that lies about both the logs and the count +consistently could slip through — outside the fail-stop threat model. (`F2`, the +flush/re-sync view desync, is separately **fixed** — WP2's `ResyncBehindFlushView` +resync-lag check — and unaffected by this change.) + +### F6 (medium) — Chain-id is never validated on the cached-identity fallback boot path + +- [ ] Fixed in: _pending_ + +`validate_rpc_chain_id` runs only in the `InputReader::new`-success branch +([runtime/mod.rs:141-176](../../sequencer/src/runtime/mod.rs)). On the +L1-unreachable bootstrap path the process starts from cached deployment identity +and never re-checks the RPC's chain id; the reader filters only by InputBox +address and app-contract topic ([partition.rs:33-37](../../sequencer/src/l1/partition.rs)) +— both address-based, and Cartesi contracts are commonly CREATE2-deployed at +identical addresses across chains, so wrong-chain events still pass. A wrong-chain RPC (typo, LB flip) after a fallback boot +feeds wrong-chain `InputAdded` events into `safe_inputs`. + +**Fix:** verify `eth_chainId` on the first successful provider interaction +whenever the bootstrap check was skipped (reader-side, once). + +### F7 (high) — WS feed has no invalidation/rollback signal; cursor-resumed subscribers silently diverge across recovery + +- [ ] Fixed in: _pending_ — partial progress 2026-07-23: PR #26 landed WP5's + field-extension half (user-op `nonce`/`safe_block`/`batch_nonce`; direct-input + `input_index`/`batch_nonce`/`block_timestamp`/`transaction_hash`; a + `live_start_offset` hint in the catch-up-exceeded close reason). The + invalidation contract itself is still missing — and `batch_nonce` cannot + substitute for it: recovery reopens the Tip at the **same** batch nonce, so a + cursor-resumed stream stays offset- and nonce-monotone while silently + containing dead rows. Interim consumer rule (from the `feature/ws-fields` + hint branch; must survive into the redesign): mirrors treat **any** socket + drop as a potential discontinuity and rebuild their soft suffix — safe + because every cascade coincides with a process restart. The proper fix + (generation id, pull-based) is owned by Track 3 of + [`../plans/2026-07-coordination-tracks.md`](../plans/2026-07-coordination-tracks.md). + +The feed streams Tip rows (soft confirmations) and documents cursor-based +reconnect (`from_offset`). Recovery invalidates already-streamed rows and +re-drains direct inputs at **new** offsets. A subscriber reconnecting with its +pre-recovery cursor keeps every invalidated tx as confirmed, receives re-drained +directs a second time, and cannot detect either: offset gaps are normal (batch- +submitter rows are filtered), and the protocol carries no epoch/generation +marker ([subscribe.rs](../../sequencer/src/egress/api/subscribe.rs), +[broadcast.rs](../../sequencer-core/src/broadcast.rs)). README's claim that the +feed "match[es] the on-chain execution order" holds only for fresh +subscriptions. + +**Fix shapes** (decide at implementation): a stream-generation id in the WS +handshake that changes on any cascade; an explicit invalidation event; or at +minimum a documented requirement that reconnect goes through `/latest_snapshot`. + +### F8 (medium) — Wall-clock regression wedges batch close and the recovery cascade + +- [x] Fixed in: WP6, 2026-06-11 — the two cross-column CHECKs dropped (timestamps keep NOT-NULL/`>= 0` and the write-once triggers); `should_close_batch_by_time`'s silent stall got the explanatory comment + +`sealed_at_ms >= created_at_ms` and `invalidated_at_ms >= created_at_ms` are +schema CHECKs ([0001_schema.sql:27-30](../../sequencer/src/storage/migrations/0001_schema.sql)) +stamped from the raw wall clock. A backwards clock step (NTP correction, VM +resume) larger than the open Tip's age makes every `seal_batch` fail the CHECK +(lane exits; under load this is a crash-loop until the clock catches up), and — +worse — `cascade_invalidate_from` stamps `invalidated_at_ms = now` on rows +seconds old, so **startup recovery itself** can fail → respawn → same failure. +The safety-net path's liveness is coupled to clock monotonicity. + +Note the irony: these cross-column CHECKs are defense-in-depth against self-bugs +(which repo policy explicitly avoids) and they bought a liveness wedge. + +**Deep-dive verification (2026-06-11):** no production code reads +`sealed_at_ms` or `invalidated_at_ms` as *values* — every reader is an +`IS NULL`/`IS NOT NULL` predicate (the `valid_*` views and the write-once +triggers). The CHECKs guard an ordering nobody consumes. + +**Fix (recommended):** drop the two cross-column CHECKs (keep NOT-NULL/≥0); +the timestamps are observability stamps, the write-once triggers stay. The +clamp alternative (`MAX(now, created_at_ms)`) is acceptable but keeps a +constraint that buys nothing. Note `should_close_batch_by_time`'s +`duration_since().unwrap_or_default()` independently makes the *time-based* +close trigger stall silently during regression (size trigger unaffected) — +fine to leave, worth a comment. + +### F9 (medium) — Blanket pending-snapshot clear deletes still-valid gold pendings; safety rests on an undocumented three-way coupling + +- [x] Fixed in: S3+WP6, 2026-06-11 — both recovery paths share `cascade_and_reopen`, and the clear is scoped to `nonce >= pivot.nonce` (`clear_pending_dumps_from_nonce_in`); regression test `cascade_preserves_gold_unpromoted_pending_below_pivot`; lifecycle.md §8 + invariants I4/I5 rewritten to match + +Both recovery paths run `clear_pending_dumps_in` (unfiltered `DELETE FROM +pending_snapshots`) whenever the cascade invalidated anything +([recovery.rs:298-313](../../sequencer/src/storage/recovery.rs)). The justifying +comments ("states the canonical replay will never reach" / "batches that haven't +been observed landing on L1 yet") are **false** for gold batches accepted during +the startup sync but never lane-promoted — routinely reachable after extended +downtime. The reason this doesn't recreate the §6 promote-wedge crash-loop from +`lifecycle.md` is an unstated coupling: the clear runs only when the cascade ran +⇒ `open_fresh_tip_in_tx` runs **in the same tx** ⇒ it sequences the *entire* +undrained safe-input backlog ⇒ the lane never re-observes those landings, so +`promote_finalized_in` never fires on a cleared nonce. Any future change that +scopes the reopen drain, makes the clear unconditional, or re-walks sequenced +inputs silently re-arms the wedge (gold batches never age into a cascade, so +there is no automatic escape). + +Side effect today: the skipped promotion leaves `finalized` lagging until the +next post-recovery batch lands; restarts catch up from an older checkpoint. + +**Deep-dive addition (2026-06-11): there is a *fourth* coupling.** The blanket +clear in the `RecoverTip` path would be a genuine crash-loop wedge if any +*valid in-flight* closed batch existed at clear time: its pending row is +deleted while the batch stays valid; when its tx later lands accepted, the +lane's promotion hits the deleted row (`QueryReturnedNoRows`) and — because +everything is young and healthy — **no danger arm ever fires to heal it**. +This is unreachable today only because of `check_danger`'s arm ordering: +`ClosedBatchInDanger` is checked before `TipInDanger`, and frame-safe-block +monotonicity makes the closed frontier always at-least-as-old as the Tip, so +a Tip-only cascade implies all closed batches are gold. The clear's safety +thus rests on the ordering of two `if` statements in `check_danger` plus a +monotonicity argument, documented nowhere near the clear. (Pre-watermark, the +F1 zombie also manifests through this same promote-wedge: a zombie accepted at +the recovery nonce before the recovery Tip closes has no pending row at all.) + +**Fix (recommended):** scope the delete to nonces `>=` the cascade pivot's +nonce. Verified consequences: doomed pendings deleted exactly; gold-unpromoted +pendings preserved (catch-up resumes from a *fresher* checkpoint; rows are +cleaned by the next promotion's `DELETE <= max_nonce`); `RecoverTip` deletes +nothing (the Tip never has a pending); the torn-cascade case degenerates to +the blanket clear (pivot nonce 0). Scoping removes the wedge-arming +structurally — any nonce the lane could later observe as accepted either has +its pending row intact or belongs to a post-recovery batch with a fresh row — +instead of leaning on three-to-four cross-file couplings. Fix the two comments +and `lifecycle.md` §8 to match. + +### F10 (medium) — `Application::execute_direct_input` defaults to a silent no-op + +- [x] Fixed in: trait-pruning wave, 2026-06-11 (single-PR branch) — both methods required; the same wave also deleted the dead `current_user_nonce`/`current_user_balance` and hoisted `validate_and_execute_user_op` to a non-bypassable free function (PLAN.md §7) + +Deposits are direct-input-only, making `execute_direct_input` the sole +deposit-crediting path; the trait defaults it to `Ok(Vec::new())` +([application/mod.rs:131-133](../../sequencer-core/src/application/mod.rs)). An +`Application` that forgets to override compiles and runs cleanly while every +deposit is escrowed on L1 with no L2 credit (symmetrically on both sides, so no +divergence — just silent fund inaccessibility). `executed_input_count` has the +same hazard. Make both required; test stubs can implement them explicitly. + +--- + +## 2. Design resolutions (settled in review discussion) + +### R1 — Two-path flush: durable watermark for standard recovery; best-effort for cockroach recovery + +**R1a — Standard recovery (X): wallet-nonce watermark.** + +New persisted singleton: `wallet_nonce_watermark` = the highest wallet nonce ever +*broadcast* by this deployment's submitter key. Rules: + +1. **Write-before-broadcast.** Any component about to broadcast a tx at wallet + nonce `n` first commits `watermark = max(watermark, n)` durably, then sends. + Uniform rule for *every* broadcast from our key — batch txs and flush no-ops + alike — so the invariant is simply "watermark ≥ nonce of everything we ever + sent" with no case analysis. A crash between commit and send only over-covers + (the flush later no-ops a never-used slot: one wasted no-op, harmless). +2. **Flush condition.** `flush_and_wait` is done iff + `pending <= safe && safe >= watermark + 1`. The current early-return + (`pending <= safe` alone, [flusher.rs:125](../../sequencer/src/recovery/flusher.rs)) + is exactly where the F1 hole lives: it consults only the local node's memory. +3. **No-op range.** `[latest, max(pending, watermark + 1))` instead of + `[latest, pending)`. +4. **Post-flush assert.** `get_transaction_count(Safe) >= watermark + 1` — + fail-loud confirmation that every slot we ever used is resolved at safe depth. +5. **Submitter unchanged.** `decide_submit_start` and the re-broadcast-at-same- + slot liveness behavior stay as they are; the watermark is the *flush's* floor, + not the poster's. + +Why this is sound: nonces are sequential, so any zombie of ours sits at a slot +`k ≤ watermark` with `k ≥ latest_count`; the widened no-op range covers it, the +mutual-exclusion race resolves it (either outcome is fine once a competitor +exists), and the wait condition refuses to declare victory until slot +`watermark` is consumed at safe depth. This restores TLA+ Implementation +Constraint 1 with a durable realization of `walletNonce`. Genesis: watermark +absent ⇒ nothing ever broadcast ⇒ flush trivially complete. + +**Durability dependency:** the watermark commit must actually be on disk before +the broadcast — under `synchronous=NORMAL` it isn't guaranteed. R1a therefore +depends on R3 (either `synchronous=FULL` or an explicit checkpoint on this +commit). Without it the anchor has a power-loss hole of its own. + +**Doc updates owed:** `docs/recovery/README.md` Implementation Constraints +(mechanism for Constraint 1), Step 3 description, and the TLA+ correspondence +notes. + +**R1b — Cockroach recovery (Y): best-effort flush, explicitly.** + +Y starts from a wiped DB: no watermark survives. Y's flush is therefore +*best-effort by construction* — it resolves every slot the provider remembers, +and cannot resolve a zombie the provider has forgotten. Consequences to state +plainly in PLAN.md: + +- Y's **detection** step (plain `setup` refusing when `pending != safe`) has the + same false-negative: a forgotten zombie keeps `pending == safe` locally. +- Post-Y, a zombie batch carrying exactly the resume nonce `N` can land fresh + and diverge. The wallet-slot competition with the new instance's first batch + is a coin flip, not a defense. +- Optional mitigation: `setup --recovery --flush-through-nonce ` — an + operator-supplied watermark. Its only real source is the **previous + instance's DB** (the persisted watermark singleton): explorer history shows + the ledger — landed txs — which is exactly the set the best-effort flush + already resolves via on-chain nonce counts; the gap is txs that neither + landed nor survive in the provider's pool, visible nowhere public. Key + property: the watermark is **fail-safe under corruption** — read from an + untrusted, half-destroyed old DB, too high costs a few wasted no-ops, too + low degrades to exactly best-effort (backed by R2). So Y may spelunk the old + DB for this one value without violating its don't-trust-local-state premise. +- The actual backstop that makes best-effort *acceptable* is R2: the residual + failure becomes detected-and-recoverable instead of silent. The repair loop is + Y itself: on a detected divergence, wipe and re-run `setup --recovery`; the + fold replays the zombie's content as canonical and the rebuilt state is + correct. (Cockroach recovers from the failure of its own flush.) + +### R2 — Batch content-identity check (the divergence backstop) + +> **Landed in WP3, 2026-06-12** (single-PR branch): hash-at-seal on the +> `batches` row (keccak256 of the submitter's encode path, stamped in the +> seal UPDATE, write-once trigger); acceptance-gated check in +> `populate_safe_accepted_batches`; `canonical_divergence` poison marker +> written **atomically with the detecting sync** (the frontier freezes +> instead of the sync aborting — strictly more atomic than the +> abort-then-mark sketch below, same effect); `DangerStatus:: +> CanonicalDivergence` ranked ahead of every arm; terminal +> `Refuse(CanonicalDivergence)` dispatch. Tests: hash-at-seal identity, +> foreign-landing freeze, mismatch freeze + frozen-forever, dispatch +> mapping. Test fixtures now seed accepted landings with the local batch's +> real wire bytes (`local_batch_payload`). + +The gold frontier currently equates "scheduler accepted nonce N" with "our valid +batch carrying nonce N" — identity by nonce only, content never compared. Every +silent-divergence path in this review (F1 zombie-wins, F3 power-loss re-seal, +Y's residual) flows through that equation breaking. + +**Check:** in `populate_safe_accepted_batches`, gated on **full acceptance** — +exactly where `scheduler_accepts` returns `Some` (sender + decode + fresh-by- +inclusion + expected nonce): + +1. Look up our **valid closed** batch with nonce N. If none exists, the landed + batch is foreign (we never submitted a closed batch at N) → divergence. +2. If it exists, compare the on-chain payload bytes against ours. **Preferred + variant: hash-at-seal** — persist the payload hash on the `batches` row at + seal time, computed by the same encode path the submitter uses + ([l1_submission.rs](../../sequencer/src/storage/l1_submission.rs)); the + check is then a hash lookup, and it survives wire-format upgrades (an old + batch compares against bytes produced by the code that sealed it). + Re-encode-at-check is an acceptable v1. + +**The gate matters.** "Expected nonce" alone is not sufficient: a +nonce-matching but *stale* zombie is skipped by the scheduler — a true no-op, +no divergence — and late-landing dead resubmissions are routine, so comparing +their content would false-positive. Rejected/stale/undecodable copies are +scheduler no-ops; their content is irrelevant by construction. + +**Why content (not identity) suffices.** Batches were deliberately designed +without an identifier. Content-equal copies are *effect-equal*: an accepted +batch's app effects depend on its inclusion block only through +`force_execute_overdue`, and for any fresh copy the force-executed prefix is a +subset of the first frame's drain (fresh ⇒ `inclusion − MAX_WAIT < +first_frame.safe_block`), in the same queue order. So which physical tx landed +carries no semantic weight — indistinguishability cuts both ways. + +**On detection:** persist a poison marker (committed even though the sync +aborts), surface as a new danger arm — `DangerStatus::CanonicalDivergence` → +`Refuse` at startup, detector-exit at runtime. The marker must rank **ahead of +every other danger arm** so the orchestrator's respawn loop cannot route a +diverged node into `Proceed`/`FlushAndCascade` (e.g. running a flush+cascade on +top of a diverged frontier). The remedy is **cockroach recovery, never standard +recovery**: X reconciles the *shape* of the batch tree under the assumption +that accepted nonce N is our batch N; a content mismatch means canonical state +contains executed effects with no reliable local source (Y-residual: not in the +DB at all; F1: in invalidated rows, but un-invalidating would have to unwind +soft-confirmed successors built on the other fork). Rebuild-from-L1 is the only +honest repair, and the Y fold — which executes actual on-chain payloads — +converges by construction. + +**Detection latency (contract language):** the check fires when the divergent +landing reaches *safe* and the reader syncs it, so soft confirmations issued in +that ~2-epoch window are built on already-diverged state. Inherent to the +optimistic model; bounded; should be stated where soft-confirmation honesty is +documented. + +**Bonus coverage:** the check also neutralizes the worst case of the +structural-reject simulation gap (§6 first entry): a foreign batch at the +expected nonce that the real scheduler structurally rejects would today be +sim-accepted and silently desync the frontier forever; with the check it fails +content comparison and flags. (Under self-trust, a foreign batch from our key +means key compromise — refuse-into-Y is the right severity for that too.) + +**Why this is not "defense-in-depth against self-bugs":** the adversary is the +L1 mempool (fully adversarial per the threat model) replaying *our own stale +transactions* at times we don't control. This is trust-boundary validation of +external input, exactly what the threat model prescribes. It also converts +F3's worst case and any future flush-coverage gap from silent divergence into a +detected refusal. + +**Cost:** one SSZ encode + compare per accepted batch (rare, off hot path). +**Edge:** a wire-format change between versions could false-positive across an +upgrade — acceptable; it surfaces as a refusal the operator resolves with Y. + +### R3 — Durability decision (settled in storage deep-dive: `synchronous=FULL`) + +Set `synchronous=FULL`. Evidence from the deep dive: + +- **One-line change with total coverage.** Every production writer funnels + through `open_writer_connection` and the single `SYNCHRONOUS_PRAGMA` constant + ([open.rs:20](../../sequencer/src/storage/open.rs)) — including the + `LeaseGuard`'s drop-time reconnect (`Storage::open_writer`). No second + pragma site to forget. +- **Write-frequency analysis.** The only hot-path commits are the lane's chunk + commits (single-threaded, so the fsync serializes into the existing ack + path); everything else is low-frequency — frame rotation per frontier + advance (~12s), batch close, reader append per safe-head advance, lease + acquire/release per snapshot HTTP request, GC after promotion. One fsync per + chunk commit (≈0.05–1 ms NVMe, ≈1–10 ms cloud block storage) against the + 500 ms ack budget is noise. +- **Verification before merge:** run `tests/benchmarks` ack/round-trip suites + before and after the flip; only if they regress materially, fall back to + targeted barriers at the four externalization sites (ack, submitter pickup, + GC unlink, watermark broadcast). One pragma beats four hand-maintained + barrier sites unless the numbers force otherwise. + +This resolves F3 and F4 wholesale and is a precondition for R1a's +write-before-broadcast guarantee. Update AGENTS.md's "ack tied to chunk +durability" to state the (now true) power-loss-durable meaning. + +### R4 — Process exit-code contract (signed off 2026-06-11; implementation = WP10) + +Today `main() -> Result<(), RunError>` exits 1 on every failure (101 on panic): +the orchestrator cannot distinguish "restart me" from "restarting is futile" +without parsing logs — exactly what the recovery README says it shouldn't have +to do. The `RunError` taxonomy ([runtime/error.rs](../../sequencer/src/runtime/error.rs)) +already carries every needed distinction; the contract is a projection of it, +implemented as one `exit_code()` match in the **binary** (the `run()` library +API is unchanged). Classes, by *restart productivity*: + +| Code | Class | Maps from | Orchestrator policy | +|------|-------|-----------|---------------------| +| 0 | clean shutdown | graceful SIGINT (SIGTERM once handled) | — | +| 1 / 101 | unclassified failure | worker crashes, provider errors, panics | restart with backoff | +| 10 | restart; **expect a recovery boot** | `DangerDetected{ClosedBatchInDanger\|TipInDanger}` | restart; next boot may legitimately take 15+ min (flush + safe-finality wait) — startup probes/deadlines must accommodate it | +| 20 | restart; transient refusal | `DangerDetected{L1ViewStale\|EstimatedBatchInDanger}`, `Refuse(L1ViewStale\|EstimatedBatchInDanger)`, `FirstBootRequiresL1`, `ChainIdRpc` | restart with backoff; these self-heal when the L1 view freshens (each boot re-syncs before re-checking); alert only if persisting | +| 30 | terminal; operator required | `CanonicalDivergence` (R2 marker), `IdentityError::Mismatch`/`OrphanedState`, `ChainIdMismatch`, `InvalidProtocolTiming` | do not restart (systemd `RestartPreventExitStatus=30`); page | + +Notes pinned by the design: + +- **The exit code is an ops hint, never protocol.** Authority over what the + next boot does stays with startup's own `check_danger` re-check; the code is + a prediction for restart/alert/deadline policy only. +- **Class 10's concrete payoff** is the startup-deadline problem: a recovery + boot runs the flush (~13–15 min mainnet) *inside* the new process; an + orchestrator that kills slow boots would interrupt recovery. The code makes + "this restart will be slow, that's expected" machine-readable. +- **Class 20 vs 30 is the load-bearing split**: all *existing* Refuse arms are + transient (they self-heal when the provider recovers) — conflating them with + terminal would page operators for conditions that resolve themselves. + `CanonicalDivergence` (R2) is the first genuinely terminal state, and is what + makes this contract worth having; "further boots fail" is enforced by its + persisted marker (and naturally true for identity/config mismatches), not by + the exit code itself. +- **k8s nuance**: Deployments restart regardless of exit code; there the + terminal class manifests as fast-refusing boots in CrashLoopBackOff plus + alert rules on `lastState.terminated.exitCode`. systemd can act on the code + directly. Document both in the ops guide. +- Reserve plain `1`, `2` (clap usage errors), and `101` (panic); deliberate + codes start at 10. + +### R5 — Replace the "no defense-in-depth" policy with a fail-loud / fail-silent rule (signed off 2026-06-11; landed in AGENTS.md, threat model, and docs/invariants.md) + +The current policy (AGENTS.md:261 "Don't layer defense-in-depth checks against +sequencer self-bugs"; threat-model "Self-trust") draws the line on the wrong +axis. Three pieces of evidence from this review: + +1. **The codebase's best features already violate it.** The write-once + triggers, tip-targeting triggers, nonce-contiguity trigger, + `ux_single_valid_tip`, the lane's safe-head-regression and + contiguity asserts, `seal_batch`'s row-count check — all are runtime + checks against our own bugs, and the schema's own comment says so + ("ensure the DB never reaches an inconsistent state if the writer + misbehaves"). The threat-model text is even internally inconsistent: + "internal invariants are enforced by type system, SQL constraints, and + tests — not by defensive runtime checks" — SQL constraints *are* runtime + checks. +2. **The hazards the review actually found are *silent absorbers* the policy + doesn't address**: `INSERT OR IGNORE` advancing `expected` on a no-op + insert, saturating conversions laundering corrupt rows, + `unwrap_or_default` masking clock regression. These convert bugs into + divergence — the worst outcome this system has. +3. **The one harmful "defense" (F8) is harmful for a different reason**: it + checks a non-invariant (wall-clock monotonicity) and its failure mode + wedges the safety path. The problem is *what* it asserts and *how* it + fails, not that it doubts our code. + +Proposed drop-in replacement (AGENTS.md + threat-model "Self-trust"): + +> **Impossible states fail loud; they are never handled.** +> +> - An invariant violation gets exactly one response: abort the operation +> loudly (assert, trigger `RAISE`, typed error). Cheap cross-module +> assertions of contracts at boundaries are *encouraged*: in this system a +> loud crash is recoverable by design (orchestrator respawn + startup +> recovery), while a silently-tolerated bug that externalizes (signed +> batch, ack, feed event) is divergence — theft-equivalent and +> unrecoverable at runtime. +> - **Never handle gracefully what cannot happen.** No fallback branches, no +> re-deriving a neighbor's answer to double-check it, no `Option`-handling +> for can't-be-`None`. One contract, one source of truth, no second code +> path. (This is the bloat the old policy rightly targeted — keep that.) +> - **Never absorb silently.** No `INSERT OR IGNORE`, saturating decode, or +> `unwrap_or_default` on data that is impossible under the contract; +> use the loud variant of the same operation. +> - An assertion must check a **real invariant** — true in every legitimate +> execution, including crash-recovery, replays, and clock steps — never an +> environmental assumption. (Cautionary tale: `sealed_at_ms >= +> created_at_ms` is not an invariant under NTP corrections, and its CHECK +> wedged recovery — F8.) + +Decision test for any proposed check: (a) does it assert a real invariant, +(b) is it near-zero cost, (c) does it fail loud with no alternative code +path? All three yes → write it. Any no → don't. + +The rule cleanly decides every case this review left ambiguous: F8's CHECKs +(drop — non-invariant), the contiguity trigger's NULL hole (fix — loud +variant of an existing check), `INSERT OR IGNORE` (replace with plain +`INSERT`), the direct-input double-sequencing trigger from §5 (now clearly +*allowed*: real invariant over valid rows, near-zero cost, loud), and R2's +content check (allowed without needing the "it's a trust boundary" +argument, though that argument remains true). + + + +--- + +## 3. Doc-drift ledger + +**Landed 2026-06-11** in the doc-drift + restructure pass. Two partial notes: +the flusher item's *error-log behavior* itself remains §4/WP9 (only the README claim is fixed here); +the TLA item is resolved by scoping the claim in the README — widening the spec guards stays an optional follow-up. + +All confirmed; each is a doc fix unless noted. The repo treats these documents +as normative, so drift here is load-bearing. + +- [x] **AGENTS.md:85 vs code + recovery README** — claims `Proceed` calls + `recover_aging_tip` "defensively"; the `Proceed` arm performs zero DB writes + ([recovery/mod.rs:243-256](../../sequencer/src/recovery/mod.rs)), and + `docs/recovery/README.md` correctly says the opposite. The + `recover_aging_tip` doc comment ("and defensively from Proceed", + [storage/recovery.rs](../../sequencer/src/storage/recovery.rs)) has the same + stale claim. Two normative docs currently contradict each other. +- [x] **`docs/recovery/README.md` "everything past gold is doomed" taxonomy** — + the three-row table (Silver-stale / Silver-poisoned / Pending no-op'd) assumes + every non-gold closed batch was submitted. A batch closed after the + submitter's last tick was never broadcast, is not "doomed", and is cascaded + anyway (defensible convergence policy; soft confirmations are really lost). + Document the fourth case and the policy choice. +- [x] **TLA+ over-approximation claim** (`docs/recovery/README.md` Formal + Verification + `preemptive.tla` header) — the spec discards an aging Tip at + `MAX_WAIT_BLOCKS` while the implementation acts at `danger_threshold`, and + `Resolve` has no killed-Pending-frontier case; the implementation's enabled + transitions are a *superset* of the model's for these actions, so "safety + over-approximation" does not hold action-for-action. Either widen the spec + guards to danger-threshold semantics or scope the claim and state the + external arguments explicitly. +- [x] **`lifecycle.md` §8 + recovery.rs comments on the pending clear** — see F9. +- [x] **`flusher.rs` retry log + recovery README flush note** — the `attempt` + counter increments on every healthy finality-wait pass, logging + `error!("flush retry: previous attempt timed out")` ~60 times per healthy + recovery; and README's "the outer flush_and_wait loop is unbounded" is wrong + for send failures, which return a hard `FlushError` (the unbounded retry is + the orchestrator respawn). +- [x] **AGENTS.md:251** — `SEQ_L1_READ_STALE_AFTER_BLOCKS` documented as + "derived"; code uses an independent fixed default of 600 + ([config.rs:113-120](../../sequencer/src/runtime/config.rs)) and validates + `stale < danger_threshold` (an operator raising the margin past the + equivalent point gets a startup refusal the doc doesn't predict). +- [x] **AGENTS.md:143** — names a nonexistent `storage/internals` module. +- [x] **[`submitter/mod.rs:6-9`](../../sequencer/src/l1/submitter/mod.rs)** — + states the scheduler checks "nonces are strictly increasing"; the scheduler + requires exact equality and rejects without consuming. The module's central + at-least-once safety argument rests on the correct rule (which the code + implements); only the doc is wrong. +- [x] **README.md WS shape** — `direct_input` example omits `sender` and + `block_number`, both serialized ([broadcast.rs](../../sequencer-core/src/broadcast.rs)) + and needed for deposit attribution. +- [x] **Threat model fallback-RPC tier** — describes a primary own-node plus + semi-trusted fallback (Infura/Alchemy); the code has exactly one + `SEQ_ETH_RPC_URL` used by all components with no failover. Either build the + tier or fix the table — and either way document the single-consistent-node + requirement from F5. +- [x] **[`batch.rs:36-37`](../../sequencer-core/src/batch.rs)** — claims L1 + classification is "by attempting SSZ decode"; classification is by sender + everywhere (AGENTS.md, reader). Wire-format reference file, worth fixing. +- [x] **[`snapshot.rs:84-86`](../../sequencer/src/ingress/inclusion_lane/snapshot.rs) + + `lifecycle.md` §3** — "the lane retries on its next pass" after a + `create_dump` failure; the lane actually exits the process (fail-loud), and + retry happens via orchestrator restart + catch-up. +- [x] **[`0001_schema.sql`](../../sequencer/src/storage/migrations/0001_schema.sql)** + — comment references `populate_safe_accepted_batches_inner`, which no longer + exists. +- [x] **Empty-batch consensus semantics undocumented** — empty batches are never + stale and consume the nonce (consistent in fold + simulation, test-pinned); + AGENTS.md defines staleness via `first_frame.safe_block`, undefined for + zero-frame batches. Also "stale skip = no state change" is imprecise (the + arrival still force-drains overdue directs). One paragraph in AGENTS.md. +- [x] **`find_first_batch_in_danger` comment is direction-inverted** + ([storage/recovery.rs:369-370](../../sequencer/src/storage/recovery.rs)): + "if a closed batch is in danger, the Tip is older still" — backwards; the + closed frontier has the smaller `safe_block` and is the older one + (non-decreasing frame safe_blocks). The conclusion (closed wins, cascade + covers the Tip) is correct; the stated reason is inverted — and this is the + monotonicity argument F9's fourth coupling rests on, so it should be stated + correctly. +- [x] **`run_flush_and_cascade` crash-window comment overclaims stability** + ([recovery/mod.rs:352-357](../../sequencer/src/recovery/mod.rs)): "the danger + condition that fired before still fires after the restart" is false when the + frontier batch landed gold during the flush — the restart can resolve to + `Proceed` with a no-op'd Pending batch left valid. That outcome is safe + (clean resubmission at a fresh slot), but the invariant as stated is wrong; + rewrite as a case analysis. +- [x] **Recovery README genesis-sentinel paragraph is stale** + (`docs/recovery/README.md:46-48`): says the implementation "can handle the + nonce-0 case either by submitting a sentinel batch... or by special-casing" — + it already handles it structurally (`open_fresh_tip_in_tx` roots a nonce-0 + batch when the valid path is empty). Update; also relevant to PLAN.md's + explicit-nonce-sentinel wrinkle (§ "Reconstructing the resume nonce"). + +## 4. Robustness / operational ledger + +- [ ] **Submitter confirmation-timeout path defeats pacing** — watch timeout maps + to `Ok` → `Submitted(n)` → immediate re-tick re-sends the same payloads at the + same nonces (usually "replacement underpriced", logged at error) before + sleeping ([poster.rs:129-139](../../sequencer/src/l1/submitter/poster.rs), + [worker.rs:146-151](../../sequencer/src/l1/submitter/worker.rs)). Return a + distinct outcome that sleeps. +- [ ] **SQLITE_BUSY treated as fatal in the submitter** — 50 ms read + `busy_timeout` + every non-poster error fatal = a transient BUSY burns a full + process respawn + recovery pass ([worker.rs:140-144](../../sequencer/src/l1/submitter/worker.rs), + [open.rs](../../sequencer/src/storage/open.rs)). Retry BUSY or use the + writer-grade timeout for these reads. +- [ ] **Own-sender SSZ decode failure wedges the submitter tick** — the poster + hard-fails where the scheduler and the simulation skip-and-continue + ([poster.rs:230-231](../../sequencer/src/l1/submitter/poster.rs)); an operator + manual tx from the submitter EOA stalls submission for the safe-lag duration. + Skip undecodable own-sender payloads. +- [ ] **SIGTERM not handled** — only `ctrl_c` is raced + ([workers.rs:211](../../sequencer/src/runtime/workers.rs)); under the + documented systemd/k8s deployment the graceful-drain path is dead code. +- [ ] **`Workers::spawn` error path leaks running workers** — a late spawn + failure (HTTP bind) abandons lane/reader/submitter/detector without + `request_shutdown`; masked in the binaries, real for the library API + ([workers.rs:141-196](../../sequencer/src/runtime/workers.rs)). +- [ ] **`Debug` derived on key-bearing config structs** — `L1Config` / + `RunConfig` carry the submitter private key; one future `?config` log leaks + it. Redacting newtype ([config.rs:18-25](../../sequencer/src/runtime/config.rs)). +- [ ] **Flusher spurious error logs** — see §3 (same fix lands together). +- [ ] **POST /tx 500 bodies echo raw internal error strings** + ([inclusion_lane/mod.rs:435-441](../../sequencer/src/ingress/inclusion_lane/mod.rs)); + the storage-error path already uses a generic message. Align. +- [ ] **Unbounded ack wait during large safe-input backlog drains** — the + frontier advance executes the whole newly-safe range before returning to + user-op processing; queued `POST /tx` callers wait unboundedly (latency, not + correctness). Consider bounded sub-range commits or an ack deadline → 503. +- [ ] **WS session teardown on transient read errors / silent idle on + beyond-head cursors** — no close frame on BUSY; `from_offset` past head idles + forever ([subscribe.rs](../../sequencer/src/egress/api/subscribe.rs)). + +## 5. Hardening & hygiene backlog (vetted, lower priority) + +- **Fee determinism contract under-specified** — `fixed_mul`'s comment claims a + debug_assert that doesn't exist (silent truncation; currently unreachable — + verified exhaustively up to `MAX_EXPONENT`), and bit-identical fees require + pinning the *multiplication order* (LSB-first with floor-after-each-multiply), + not just the table values ([fee.rs](../../sequencer-core/src/fee.rs)). + **Directly load-bearing for PLAN.md §2** (native == RISC-V determinism, and + any future C++ app implementing the same semantics). +- **`trg_enforce_nonce_contiguity` NULL hole** — a dangling + `parent_batch_index` makes the subquery NULL and the comparison no-op; only + the FK pragma (per-connection, default OFF) catches it. Make the trigger + NULL-safe. +- **Direct-input double-sequencing structurally unguarded** — no uniqueness on + `safe_input_index` among *valid* rows (re-drain support); the one storage bug + class that double-executes a deposit rests on lane discipline alone, while + AGENTS.md:212 claims "FK + PK constraints catch the dangerous failure modes". + Tighten the doc at minimum. +- **`seal_and_open_next_batch` accepts an unchecked `next_safe_block`** — safe + only because the lane passes `head.safe_block`; assert equality or drop the + parameter ([ingress.rs:411-431](../../sequencer/src/storage/ingress.rs)). The + bare `close_frame_and_batch` has no production callers — gate to `cfg(test)`. +- **`INSERT OR IGNORE` in `populate_safe_accepted_batches`** advances `expected` + even when the insert no-ops — dead defense that would desync silently instead + of failing loud; use plain `INSERT`. +- **`safe_accepted_batches.first_frame_safe_block` / `.inclusion_block` are + write-only columns** — no production reader; drop or mark audit-only. (R2's + hash-at-seal lives on the `batches` row, not here — this table stays a pure + acceptance log either way.) +- **Scheduler-side `AppError` handling diverges from the lane** — canonical + logs-and-continues (eprintln), sequencer fail-louds; a contract violation + produces divergence rather than detection. Decide one behavior; on the + canonical side an explicit report/halt beats a guest-console line. +- **`direct_q` unbounded in the canonical scheduler** — adversarial deposit + flood bounded in time (force-drain) but not bytes; a per-input cap or byte + budget closes a (very expensive) guest-OOM vector. +- **`MAX_BATCH_METADATA_BYTES` (71) understates real SSZ per-op overhead** + (~83+ with offsets); byte budgeting undercounts ~15% for max-payload ops. +- **Wallet nonce `wrapping_add`** — signature replay reopens after 2^32 included + ops per sender; economically implausible, but wrap-vs-saturate deserves a + justifying comment. +- **`from_snapshot_bytes` rejects duplicates but accepts unsorted entries** — + enforce strictly-ascending addresses (subsumes the duplicate check) or drop + the pretense of canonical-decode enforcement. + +**Storage deep-dive batch (2026-06-11, verified against the tree):** + +- **Gate the test-only public surface** — all callers are test-side, but + **correction (2026-06-11):** four candidates are used by integration tests + (`sequencer/tests/`), which cannot see `#[cfg(test)]` items — use a + `test-support` feature for `promote_finalized`, `initialize_open_state`, + bare `close_frame_and_batch`, `safe_input_end_exclusive`; **delete** + `ordered_l2_txs_for_batch` and `latest_batch_index` outright (only callers + are their own unit tests). Deleting those two makes `l1_submission.rs` + collapse to the submitter's two real reads — at which point rename it; its + own header already admits "despite the historical name, nothing in this + file does writes". Details: S1 in the simplification doc + the + test-coverage map's churn section. +- **`frames` lacks the immutability triggers `batches` got** — `fee` and + `safe_block` are documented immutable but only convention-protected; + asymmetric with the schema's own trigger philosophy. Add write-once triggers + or document the asymmetry. +- **Two corruption philosophies in one module** — `decode_l2_tx_row` panics on + malformed rows while `convert.rs` deliberately saturates "so corrupted rows + don't crash the process". Pick one (fail-loud is the repo's stated + preference; the saturating converters silently launder corruption). +- **Duplicated 25-line CASE/LEFT-JOIN row shape** between + `ordered_l2_txs_page_from` (egress) and `ordered_l2_txs_for_batch` + (l1_submission) — deleting the unused latter removes the duplication for + free. +- **`recover_post_flush_inner` / `recover_aging_tip_inner` share an identical + clear+reopen tail** — one helper taking a pivot-selection closure keeps the + clear/reopen conditions (F9's couplings) in exactly one place. +- **The scheduler-nonce fold exists twice** — `populate_safe_accepted_batches`' + expected-nonce threading and the submitter's `decide_submit_start` fold are + the same exact-match simulation; co-locating both next to + `scheduler_accepts` in `sequencer-core/protocol.rs` would put all + scheduler-mirroring logic in one audited place. (Also relevant to PLAN.md + §2's scheduler-library extraction — this is the natural seed of it.) +- **Workers re-open `Storage` per tick** (reader, submitter inside + `spawn_blocking`) — a held connection per worker drops per-tick open + overhead and narrows error surfaces. Low priority. +- **`partition.rs` returns `Vec` both callers collapse to the + first error**, and `should_retry_with_partition` substring-matches error + codes against `format!("{err:?}")` instead of alloy's structured JSON-RPC + code — fragile (false positives only cause harmless bisection, but still). + +## 6. Verified-clean ledger (refuted concerns — do not re-litigate without new evidence) + +- **`scheduler_accepts` omits the two structural rejections** + (`SafeBlockAboveInclusionBlock`, `NonMonotonicSafeBlocks`) — deliberate, + documented self-trust; unreachable from honest paths (monotone frame + safe_blocks; inclusion ≥ observed safe head); test-pinned by + `frontier_accepts_future_safe_block_batch_by_design`. Note: the failure shape + on violation is silent permanent divergence, so if the trust boundary ever + moves (Byzantine RPC in scope), revisit — the checks are pure and cheap to + simulate. **Update (WP3, 2026-06-12):** the worst case is now covered — a + sim-accepted batch that fails the content-identity check (foreign at the + expected nonce) freezes the frontier with a `CanonicalDivergence` marker + instead of silently desyncing (R2's bonus coverage; test + `sim_accepted_foreign_batch_freezes_frontier_with_divergence_marker`). +- **Wire fee exponents above `MAX_EXPONENT` panic `fee_to_linear`** — honest + sequencer clamps at persistence (test-pinned); only the trusted submitter key + can deliver batches. Out of scope per self-trust. +- **Stalled WS consumer pins permit/thread/reader** — real mechanics, but DoS is + explicitly infrastructure-scoped in the threat model. +- **`validate_user_op` panic via attacker-controlled frame fee** — same + clamp + self-trust scoping as above. +- **Operator `WalletConfig` silently dead on warm start** — deliberate, + documented dump-wins semantics; required for determinism. +- **Snapshot bytes history-dependent (explicit zero balances)** — deterministic + product of the transition function; format guarantees map-content + determinism, not logical-state canonicalization. + +## 7. Suggested work packages (PR-shaped) + +| # | Package | Contents | Depends on | +|---|---------|----------|-----------| +| WP1 | Durability | R3 decision (`synchronous=FULL` + measurement), F4 ordering note | — | +| WP2 | Flush anchor | R1a watermark (schema + poster write + flusher condition + assert), F2 coherence assert, TLA/README updates | WP1 | +| WP3 | Divergence detection | R2 content-identity check + `CanonicalDivergence` refuse arm + tests; threat-model + recovery doc updates | — | +| WP4 | Reader hardening | F5 (same-view bound + on-chain input-index reconciliation), F6 chain-id on fallback | — | +| WP5 | Feed protocol | F7 generation id / invalidation contract + README — **partial 2026-07-23** (PR #26: context fields + L1 provenance + close-reason offset hint; the contract half moved to Track 3 of [`../plans/2026-07-coordination-tracks.md`](../plans/2026-07-coordination-tracks.md)) | — | +| WP6 | Recovery hygiene | F8 timestamp clamp, F9 scoped clear or pinned coupling + regression test | — | +| WP7 | Doc-drift pass | §3 ledger in one commit (no behavior changes) — **done 2026-06-11**, incl. AGENTS.md/CLAUDE.md restructure + `docs/invariants.md` | — | +| WP8 | Trait contract | F10 required methods (PLAN.md §7 pulled forward) | — | +| WP9 | Ops pile | §4 items, individually trivial | — | +| WP10 | Exit-code contract | R4 projection in the binaries + per-variant class test + ops doc (incl. SIGTERM→0 from §4) | WP3 for the `CanonicalDivergence` arm; rest independent | +| WP11 | Storage hygiene | §5 storage batch: cfg(test) gating + dead-read deletion, frames triggers, NULL-safe contiguity trigger, plain INSERT, scoped pending clear (F9), CHECK drop (F8) | — | + +PLAN.md updates (Y detection caveat, best-effort flush framing, R2 as Y's +backstop, `--flush-through-nonce` option, fee-determinism note for §2) landed +with the PLAN.md review pass (commit `ea74449`). + +The simplification/refactoring queue (S1–S7, the do-not-simplify list, and the +combined S∪WP ordering) lives in +[`2026-06-10-simplification.md`](2026-06-10-simplification.md). diff --git a/docs/review/2026-06-10-simplification.md b/docs/review/2026-06-10-simplification.md new file mode 100644 index 0000000..9a97dcf --- /dev/null +++ b/docs/review/2026-06-10-simplification.md @@ -0,0 +1,203 @@ +# Simplification & Refactoring Queue — 2026-06 review + +Companion to [`2026-06-10-correctness-review.md`](2026-06-10-correctness-review.md) +(the findings ledger). That document says what is *wrong*; this one says what is +*heavier than it needs to be*, ranked and sequenced. Sources: twelve module +reviews' refactor notes, the storage deep-dive, and the line-by-line passes. + +## Verdict first + +**No architectural restructure is needed.** The module layout (one file per +writer role, `*_in(tx)` free functions composing into larger transactions, +storage-owns-SQLite / lane-owns-filesystem) is sound and should be defended, +not redesigned. The "buckling under its own weight" feeling traces to three +specific, fixable weights: + +1. **Invariants living between files with nothing pinning them** — addressed + this review by [`docs/invariants.md`](../invariants.md) and the fail-loud + policy; structurally dissolved over time as WP6/WP2/WP3 land. +2. **Test-only and dead surface presenting as production API** — a `Storage` + facade ~30% wider than what production calls, with module docs describing + consumers that don't exist. Pure deletion fixes this (S1). +3. **The same semantics written more than once** — the scheduler-acceptance + fold in two places, a 25-line SQL row-shape in two places, the clear+reopen + recovery tail in two places, the max-fee guard in two places. Each is a + drift seed; each has a single-home fix (S2, S3, S6). + +The one genuinely *new* abstraction worth building is the scheduler library +(PLAN.md §2) — everything below is pruning and consolidation that makes that +extraction smaller and safer. + +## The queue (S-items, PR-shaped) + +### S1 — Shrink the Storage surface to what production uses + +All callers are test-side; zero production behavior change. **Correction +(2026-06-11, test-coverage map):** four of the candidates are used by +*integration tests* (`sequencer/tests/`), which compile as separate crates and +cannot see `#[cfg(test)]` items — plain gating would compile-break them. + +- **Feature-gate** (e.g. a `test-support` cargo feature, or public seed + helpers): `promote_finalized` (snapshot_endpoints.rs), `initialize_open_state` + (3 integration files), bare `close_frame_and_batch` + (batch_submitter_integration.rs), `safe_input_end_exclusive` + (ws_broadcaster.rs). The lifecycle doc's "production must not call + `promote_finalized`" landmine still becomes mechanically enforced — just by + feature, not `cfg(test)`. +- **Delete** (verified: only callers are their own unit tests): + `ordered_l2_txs_for_batch`, `latest_batch_index` — deleting the former also + removes the duplicated 25-line CASE/LEFT-JOIN row shape (its twin in + `egress.rs` becomes the only copy). +- Rename `l1_submission.rs` (whose own header admits "nothing in this file + does writes") to match its real content — the submitter's two reads — and + fix `storage/mod.rs`'s description of it. +- `seal_and_open_next_batch`: drop the `next_safe_block` parameter (callers + always pass `head.safe_block`) or assert equality. +- `populate_safe_accepted_batches`: call `frontier_nonce` instead of + re-deriving `latest.nonce + 1` inline. + +### S2 — Collect the scheduler-mirroring logic in one place + +The strategic item: it is PLAN.md §2's extraction, started small. + +- Move the expected-nonce fold to `sequencer-core/protocol.rs` next to + `scheduler_accepts`; make `populate_safe_accepted_batches` and + `decide_submit_start` both consume it. +- Add the cross-referencing one-liners on both sides of the structural-reject + omission (`scheduler_accepts` ↔ `batch_reject_reason_for_block`) so the + asymmetry reads as intentional. +- Pin the fee-determinism contract in `fee.rs`: LSB-first binary + exponentiation with floor-after-each-multiply (the two orders provably + differ), and fix the `fixed_mul` comment that claims a nonexistent + debug_assert. +- Later, with R2/WP3 landed: consider extracting a shared + `batch_acceptance(batch, inclusion_block, expected) → Accept|Reject|Stale` + consumed by *both* the canonical fold and the simulator, making structural + drift impossible by construction. That is effectively PLAN PR2; don't do it + piecemeal before then. + +### S3 — One home for the recovery tail (land *before* WP6) + +~~Extract `cascade_and_reopen(tx, pivot)`~~ **done (2026-06-11, with WP6 in +the same commit — the refactor existed to make the scoped clear a one-place +edit, and they landed together on the single-PR branch).** Both recovery +paths now share `cascade_and_reopen`; the pivot selection is the only +variation. + +### S4 — Fail-loud batch (WP11 remainder; mandated by R5) + +- `INSERT OR IGNORE` → plain `INSERT` in `populate_safe_accepted_batches`. +- NULL-safe `trg_enforce_nonce_contiguity` (missing-parent arm). +- Add the `frames` immutability triggers (`fee`, `safe_block`) — symmetric + with `batches`, clearly allowed under R5. +- Decide the `convert.rs` philosophy: the saturating converters silently + launder corrupt rows while `decode_l2_tx_row` panics; R5 says fail loud. + Converting the saturations to checked-or-panic is a small, behavior-visible + change (corrupt DB: silently-wrong → crash) — its own commit with tests. +- Drop or mark audit-only the write-only columns + `safe_accepted_batches.{first_frame_safe_block, inclusion_block}`. + +### S5 — Worker plumbing (bundle with WP9 ops items) + +- Reader/submitter hold one `Storage` per worker instead of opening per tick. +- `partition.rs`: both callers collapse `Vec` to the first + error — return one error; match the JSON-RPC error *code* structurally + instead of substring-matching `format!("{err:?}")`. +- `runtime/workers.rs`: replace the string coupling between + `WorkerExit::component_name()` and the components array with a structural + key (a mismatch today means re-awaiting a consumed JoinHandle). +- Misclassified errors into `RunError::Io` (invalid key, provider build) → + `BootstrapError` variants, so the R4 exit-code projection stays clean. +- Lane: collapse the overlapping `DrainSummary`/`ChunkOutcome` tri-states; + precompute the byte target as an op count per head transition. +- Egress (declined-for-now items listed at the end stay declined). + +### S6 — Canonical-app side polish (touch with extra care: it is the reference) + +- Unify `force_execute_overdue` / `drain_directs_safe_at` into one + `drain_while(predicate)` helper; pop-before-execute removes the payload + clone. +- ~~Remove the duplicated in-frame `max_fee < fee_price` guard~~ **done + (trait-pruning wave, 2026-06-11)** — the guard now lives only in the + `validate_and_execute_user_op` free function (no longer an overridable + trait default), which is always in the call path; duality tests pass. +- Drop the `ProcessResult`↔`ProcessOutcome` cross-`PartialEq` impls + (test-only ergonomics; `result.outcome == X` is clearer). +- `to_user_op()` called twice per op in `execute_frame_user_ops` — call once. +- app-core: replace side-effecting match guards in the wallet with explicit + decode-then-match; tidy `decode_portal_erc20_deposit`'s parameter. + +### S7 — Fifteen-minute comment batch + +One-liners that prevent future confusion, no behavior: `WriteHead. +max_batch_user_op_bytes` is resampled from *current* policy on restart; +`decide_submit_start` deliberately folds mined-but-unsafe nonces without +simulating staleness (transient submitter idle on a stale landing is expected); +the byte target can overshoot by one max-size op (and the schema CHECK only +bounds the target, not target + 1 op); the lane's "read together" comment on +head + drain cursor (two read txs — combine or soften); `executed_input_count`'s +real consumer is the snapshot byte comparison; `WalletConfig::devnet()` reusing +the Sepolia portal address is a deterministic-deploy assumption. + +## Do-not-simplify list + +Things that *look* like cleanup and would break registered invariants — the +refactorer-facing mirror of [`docs/invariants.md`](../invariants.md): + +1. **Don't move filesystem work into `storage/snapshot_dumps.rs`** — the + module boundary *is* the GC crash-ordering guarantee (I13). +2. **Don't reorder `check_danger`'s arms** or merge its two `find_*` helpers + into one that consults the Tip first — the closed-frontier-first order + keeps the dispatch table's meaning (since the F9 scoped clear it is no + longer load-bearing for the pending clear; see I4's updated entry). +3. **Don't "deduplicate" promotion out of the drain transaction** — a + standalone promotion re-opens the `lifecycle.md` §6 crash-loop wedge (I6). +4. **Don't filter own-batch rows out of `valid_sequenced_l2_txs`** (tempting + to replace the three consumer-side sender checks with one view filter): + the drain cursor is `MAX(safe_input_index)+1` over those very rows — the + filter would rewind it and re-drain. Sender filtering must stay at the + consumers (I11). +5. **Don't replace the rowid offset with count-based pagination** or add + AUTOINCREMENT semantics assumptions — invalidated-batch holes and the + 0-sentinel both depend on current behavior (I10). +6. **Don't move GC back to the idle path or a dedicated worker** — promotion- + coupled GC is starvation-proof and single-writer by design (lifecycle §7). +7. **Don't add internal retry loops to the flusher/submitter for provider + errors** — the orchestrator respawn is the retry mechanism; internal + retries would mask exactly the failures the danger machinery routes on. +8. **Don't unify the two staleness references** (inclusion vs current) — they + are deliberately different formulas for different questions. + +## Declined / deferred (with reasons) + +- **Egress single-poller fan-out** (vs per-subscriber 20ms polling): correct + scale-up path, no need at current subscriber counts. Revisit with load. +- **`LeaseGuard` shared release channel**: per-release writer connections are + fine at current snapshot-polling traffic. +- **`finalized_state` ETag on `l2_tx_index`** instead of inclusion_block: + structurally collision-proof but no reachable collision today. +- **Runtime exit-mapping generic** (the ~150 lines of five identical + `From`/`from_shutdown`/waiter shapes): the module header explicitly defends + the explicitness; the detector is genuinely special. Declined — fix only the + string coupling (S5). +- **Stale-skip on-chain report**: would help forensics but is a scheduler + protocol change; queue behind the scheduler-library extraction. +- **`Storage::read` commit-vs-rollback**: no behavioral difference; not worth + churn. +- **TLA state-count freshness** (`157M` in the README): verify and update the + number only alongside an actual TLC re-run; pin the TLC version then. + +## Suggested combined order (S-items ∪ work packages) + +1. **WP1** durability flip + benchmarks (one line; unblocks WP2) +2. **WP8** trait defaults required (tiny, independent — PLAN §7 pulled forward) +3. **S1** surface shrink (makes everything after smaller) +4. **S3** recovery-tail refactor → **WP6** scoped clear + CHECK drop on top +5. **WP2** wallet-nonce watermark (needs WP1) +6. **WP3** content-identity check + `CanonicalDivergence` (independent) +7. **S2** scheduler-mirror colocation (sets up PLAN PR2) +8. **S4** fail-loud batch (WP11) +9. **WP4** reader hardening, **WP5** feed generation id, **WP10** exit codes, + **WP9 + S5** ops/plumbing bundle, **S6/S7** opportunistic +10. PLAN **PR1** (setup/run split) and **PR2** (scheduler library) on the + cleaned base. diff --git a/docs/review/2026-06-10-test-coverage.md b/docs/review/2026-06-10-test-coverage.md new file mode 100644 index 0000000..72f4171 --- /dev/null +++ b/docs/review/2026-06-10-test-coverage.md @@ -0,0 +1,211 @@ +# Test Coverage Map — 2026-06 review + +Third companion to [`2026-06-10-correctness-review.md`](2026-06-10-correctness-review.md) +(findings) and [`2026-06-10-simplification.md`](2026-06-10-simplification.md) +(refactor queue). This document answers: what does the suite pin, what does it +fail to pin, which tests will go red *by design* as the work packages land, and +which test-infrastructure levers exist or are missing. + +## Verdict + +**Recovery is the best-tested subsystem in the repo** — the startup dispatch +matrix is covered at both unit and e2e level (all five danger arms, threshold +boundaries on both sides, multi-generation cascades, nonce reuse across +generations, torn states, restart-loop convergence via `respawn_until_stable`, +and a post-test schema-invariant hook on every passing e2e scenario). The e2e +harness is also substantially stronger than its absence of documentation +suggests: libfaketime with *mid-run* wall-clock jumps, real process +spawn/respawn loops, TCP-proxy outage injection, Anvil mempool control +(`set_automine(false)` + `drop_all_pending_txs` — the delayed-inclusion +recipe), and DB surgery helpers. + +**The one structural hole is the one the review predicted: the duality has no +direct mechanism.** Both sides are strongly tested *separately* — the canonical +side against the real RISC-V machine, the sequencer side against hand-written +expectations — but no test anywhere feeds **sequencer-produced batch bytes** +through **any** scheduler implementation and compares end states. Agreement is +transitive through two sets of hand-built fixtures. Details below; the fix is +two cheap tests (A1, A2). + +## The duality scoreboard + +Per AGENTS.md Testing Guidance ("any invariant the two sides share should have +at least one test that exercises both"): + +**Two-sided today:** +- EIP-712 domain: `v1_regression_*` (host) ↔ `scheduler_emits_transfer_notice_from_guest` (guest signature recovery). +- Nonce exact-equality / reject-without-consume: guest `scheduler_rejected_batch_does_not_consume_nonce`, `scheduler_reports_wrong_nonce_batch_from_guest` ↔ host `advance_expected_batch_nonce_matches_scheduler_nonce_rule`, `populate_safe_accepted_batches_handles_large_nonce_gap` / `_out_of_order`. +- Staleness skip without nonce consumption: guest `scheduler_stale_batch_is_skipped_without_consuming_nonce` ↔ host `safe_accepted_frontier_skips_stale_payloads`. +- The structural-reject **asymmetry**, pinned on both sides: guest rejects (`scheduler_reports_non_monotonic_safe_blocks` / `_safe_block_above_inclusion`) ↔ host deliberately accepts (`frontier_accepts_future_safe_block_batch_by_design`). +- A partial end-to-end round trip exists: `nonce_zero_recovery_invalidates_then_accepts_at_nonce_zero_test` lands a *sequencer-produced* recovery batch in `safe_accepted_batches` — sequencer bytes through the **simulation**, not through the scheduler. + +**One-sided or absent:** +- **Sequencer-produced bytes through a real scheduler implementation: absent** (the most direct I1 test). +- **Guest fee path: never exercised** — every canonical-test batch uses `fee_price = 0`, `max_fee = 0`. The `max_fee < fee_price` skip and gas charging never run on the guest. +- **Host==guest fee arithmetic: no cross-check** — canonical-test never asserts post-gas balances; a `fee_to_linear` divergence on the guest would be silent state divergence (and review §5 pins this as load-bearing for PLAN §2). +- Drain attribution / flattened replay order on the *same* batch bytes: each side separately only. +- The native `Scheduler` fold is used by **zero tests outside canonical-app's own suite**. + +## Owed tests (the gap ledger) + +### A — writable today, no new infrastructure + +- [ ] **A1. Native duality agreement test** (the I1 mechanism): run the + sequencer (or seed storage directly), take its actual `pending_batches` SSZ + bytes + the same direct-input stream, fold them through the native + `Scheduler`, and compare final balances/nonces against the + sequencer's app state (snapshot or replayed feed). Closes "enforced by + review + tests only. No mechanism." +- [ ] **A2. Guest fee-path test** in canonical-test: nonzero `fee_price`, + user ops above/below it, **assert post-gas balances**. Gates S6's removal of + the duplicated max-fee guard ("verify with the duality tests" — that test + doesn't exist yet) and gives the first host==guest fee-arithmetic check. +- [ ] **A3. I4 arm-ordering dedicated test**: both `ClosedBatchInDanger` and + `TipInDanger` genuinely in danger, assert Closed wins. Today the ordering is + pinned only *incidentally* (a fixture whose Tip happens to be equally old). +- [ ] **A4. F9 coupling regression pair** (owed by WP6): (a) a gold-accepted- + during-startup-sync but never-promoted pending survives recovery / is + handled; (b) the promote-after-clear wedge state is unrepresentable. +- [ ] **A5. I2's second half**: `close_frame_only` at an *advanced* safe head + — assert the new frame row and the encoded wire frame carry the NEW + safe_block together with the drained directs. (Membership is pinned; + the stamp is not.) +- [ ] **A6. Fail-loud halves**: `CatchUpError::NoSnapshot` (no test references + it), `InclusionLaneError::NoOpenTip`, I7's tx-failure half (recipe: + pre-insert a dumps row with a colliding prefix so the seal tx fails on + UNIQUE → batch must stay the open Tip), F4's dangling-row *state* + (delete the directory under a referenced row → assert the loud failure + shape; the WAL-rewind cause stays unsimulable). +- [ ] **A7. Wallet pins before S6**: insufficient-balance transfer/withdrawal + silent no-op (gas charged, nonce bumped, no output — the side-effecting + match-guard arm S6 refactors, currently unpinned), the + `AppError::Internal` cannot-pay-gas arm, and a wallet-level + replay-determinism test (execute → dump → replay same sequence into fresh + app → state equality). +- [ ] **A8. EstimatedBatchInDanger e2e** — the one dispatch-matrix row without + an e2e. Recipe (verified writable): mine ~800 blocks (observed age below + the 900 stale gate), `set_faketime_offset(+30min)` without mining, respawn + → assert refusal + `invalidated == 0`. +- [ ] **A9. PLAN Q1/Q2 unit tests** against the existing `Scheduler` + (construct the fridge at B, feed (B,C], assert N and the drain set) — no Y + CLI needed; the e2e variants wait for PLAN PR4/PR5. +- [ ] **A10. Fourth-shape cascade policy pin**: a *young* never-submitted + closed batch is cascaded anyway (convergence-over-preservation — now a + normative README claim with no test). +- [ ] **A11. `recover_aging_tip` torn/no-Tip entry** (only the post-flush + variant is tested) — protects the S3 unification from dropping the reopen + on one path. +- [ ] **A12. F8 reproduction/fix pin**: backward-stepped clock + `seal_batch` + / cascade against the cross-column CHECKs (today: reproducible, unwitnessed; + after WP6 drops them: "cascade succeeds with backward clock" is the pin). + +### B — blocked on the feature (land with the WP) + +- [ ] **B1 (WP2)**: watermark positives — flush submits no-ops covering + `[latest, watermark+1)` even when `pending == safe`; post-flush + `safe > watermark` assert. Writable with existing levers once the feature + exists. +- [ ] **B2 (WP3)**: content-mismatch → persisted `CanonicalDivergence` → + Refuse ranked ahead of every arm (probe with `respawn_until_stable`: + a diverged node must never route into Proceed/FlushAndCascade); foreign + batch at expected nonce → divergence. +- [ ] **B3 (WP5)**: the new feed contract (generation id / invalidation + signal) — note the current suite *pins the divergent behavior* (see churn). +- [ ] **B4 (WP10)**: per-variant exit-code assertions — every current e2e + asserts only `!success()`; `ExitStatus::code()` is already available from + `wait_for_exit`. SIGTERM→0 additionally needs the §4 SIGTERM fix (today + every e2e teardown's "graceful stop" is an unhandled-signal death). +- [ ] **B5 (WP4)**: F6 fallback-boot wrong-chain detection; F5 input-index + reconciliation loud-failure (unit-level, reader). + +### C — blocked on a missing harness lever (build the lever first) + +| Lever | Unblocks | Effort | +|---|---|---| +| Pending-tx capture + re-inject (`txpool_content`/raw-tx before `drop_all_pending_txs`, `eth_sendRawTransaction` later) | **F1 zombie e2e** — the review's headline scenario; WP2's adversarial case | small | +| Snapshot observability (DB readers for `pending_snapshots`/`finalized_snapshot`/`dumps`, `/latest_snapshot` client, dump-dir inspection) | A4/F9 e2e, lifecycle e2e (currently **zero** e2e coverage of take/promote/GC/leases; warm-resume-from-dump is exercised by every restart test but never asserted — a silent fallback to genesis replay would pass everything, just slower) | small | +| Bare second-Anvil spawn (`--chain-id `) + `set_l1_endpoint_override` mid-run | F6 e2e | small | +| Kill-at-log-marker (deterministic crash between flush completion and cascade commit) | the crash-window case analysis in `run_flush_and_cascade` (the Proceed-with-noop'd-Pending branch has no witness) | medium | +| Split-view / response-rewriting L7 proxy (TcpProxy is byte-level) | F2 coherence e2e, F5 clamped-`get_logs` e2e — **realistically: unit-level provider mocks instead**, e2e validates only the passing path | medium–large | +| SQLITE_BUSY injection | §4 BUSY items (submitter fatal, WS teardown) | medium | +| fsync/power-loss/WAL-rewind fault injection | true F3/F4 reproductions | **out of scope** — accept; the state-construction variants (A6) cover the detectable halves | + +### D — explicitly accepted as untested + +Power-loss WAL rewind (above); the flusher's spurious retry log (lands +test-free with WP9); detector L1ViewStale-arm runtime exit (single code path +treats all non-Safe alike; a log-field assertion would do if wanted). + +## Churn forecast (consult before each WP — reds listed here are by design) + +- **WP2 (watermark)**: `flush_is_noop_when_no_pending_nonces` **pins the F1 + hole as correct behavior** — red by design. The other flusher tests are + signature churn. E2e: `delayed_inclusion_cascades_on_restart_test` and + `nonce_zero_recovery_…` rely on the early-return for fast recovery boots — + the widened flush adds no-ops + safe-wait; expect timeout tuning (and the + harness may need a block ticker during recovery boots). The three + `respawn_until_stable` convergence tests get timing exposure (flaky-red + risk, not hard-red). +- **WP3 (content check)**: a *family* of fixtures fabricates accepted + own-sender payloads that don't content-match local batches + (`submitter_frontier_tracks_accepted_prefix`, + `populate_safe_accepted_batches_resumes_from_latest_row`, + `frontier_accepts_future_safe_block_batch_by_design`, two `check_danger_*` + tests, four `tick_once_*` worker tests) — these will trip + `CanonicalDivergence` by design; fixtures must seed payloads through the + real encode path or seed matching closed batches. The stale/duplicate/gap + tests **survive** (the check gates on full acceptance). Also: + `check_danger_refuses_when_l1_view_is_stale`'s "first arm" claim becomes + second place (marker outranks it). +- **WP5 (feed contract)**: `ws_reconnect_at_invalidated_offset_skips_cleanly_test` + and `ws_subscribe_from_future_offset_waits_silently_test` pin exactly the + behavior being replaced — red by design. If the envelope changes, every e2e + fails at parse until `WsClient`/`ReplayWalletApp` update (mechanical). + First-frame assertions in `ws_broadcaster.rs` are conditional churn. +- **WP6 (scoped clear)**: `aging_tip_cascade_clears_pending_dumps` red by + design (scoped clear deletes nothing on the RecoverTip path); + `post_flush_cascade_clears_pending_dumps` passes by coincidence (pivot + nonce 0 ⇒ scoped == blanket) — **must gain the discriminating case**; + `clear_pending_removes_all_pending_rows` + a lease-test staging call need + mechanical updates. F8's CHECK drop is test-silent (nothing pins the + CHECKs). +- **WP8 (required trait methods)**: `SweepTestApp` (3 workers.rs tests) and + any stub relying on the defaults — compile-break; add the two explicit + methods. +- **S1 (surface shrink)**: see the correction below — the integration-test + seeds compile-break if gated as originally written. +- **S3 (recovery-tail extraction)**: **anti-churn** — the entire + `recover_post_flush` / `tip_staleness` / pending-clear suites must stay + green; any red there means the refactor changed behavior. +- **S5 (error taxonomy)**: `invalid_private_key_error_does_not_echo_key_material` + retargets variant, keeps the no-echo assertion. +- **S6 (wallet match guards)**: claimed behavior-neutral but the + insufficient-balance no-op arm has **no pin** — land A7 first. +- **PLAN PR1 (setup/run split)**: `first_boot_no_identity_l1_unreachable_…` + and `chain_id_mismatch_via_live_rpc_…` test bootstrap paths that move into + `setup`; rewrite as setup-phase tests; harness spawn flow changes. + +## Corrections to earlier review claims (recorded for honesty) + +1. **S1's "verified all callers are in test modules" was wrong.** Integration + tests compile as separate crates and cannot see `#[cfg(test)]` items — and + `sequencer/tests/` *does* call `initialize_open_state` (3 files), + bare `close_frame_and_batch` (batch_submitter_integration), + `safe_input_end_exclusive` (ws_broadcaster), and `promote_finalized` + (snapshot_endpoints). Revised S1: gate those four behind a `test-support` + cargo feature (or provide public seed helpers); outright deletion remains + correct for `ordered_l2_txs_for_batch` and `latest_batch_index` (their only + callers are their own unit tests). +2. **The old "T2 Anvil --no-mining is the big unbuilt lever" note is stale**: + `set_automine(false)` + `drop_all_pending_txs` exist and power the + delayed-inclusion e2e today. + +## On resurrecting TEST_PLAN.md + +Recommendation: **don't resurrect the full scenario matrix** — it rotted once +and its job is better served by three things that already exist or now exist: +the e2e suite's compile-time zone-math asserts and post-test schema-invariant +hook ("plan as code"), the per-WP test obligations in the review ledger, and +this document's owed-test checkboxes (a dated, finite list — not an +ever-growing matrix). If a standing artifact is wanted later, graduate the +"Owed tests" section into `docs/testing.md` once the A-list is burned down. diff --git a/docs/review/2026-06-25-cockroach-recovery-rooting.md b/docs/review/2026-06-25-cockroach-recovery-rooting.md new file mode 100644 index 0000000..9a188c6 --- /dev/null +++ b/docs/review/2026-06-25-cockroach-recovery-rooting.md @@ -0,0 +1,139 @@ +# Settled design — Cockroach-recovery batch-tree rooting (2026-06-25) + +PR5 (`setup --recovery`) rebuilds a wiped DB and must resume submitting at nonce +`N'` without replaying history, so the rebuilt batch tree is rooted at `N'`, not +0. `run`'s submitter submits `valid_closed_batches` with `nonce >= frontier` +(`= N'` after recovery), and a tip's nonce is structural (`parent.nonce + 1`), +so the tree **must** have a valid batch at `N'`. `trg_enforce_nonce_contiguity` +ABORTed any parentless root with `nonce != 0`, blocking this. + +Settled with Gabriel (free rein on schema — no migration/back-compat concern), +after an adversarial design panel (run `wuiha00k9`). + +## Decision: the batch-tree **anchor**, not a sealed sentinel batch + +A `batch_tree_anchor` singleton holds the nonce the single parentless root +carries (default `0`; `setup --recovery` writes `N'` before the +`setup_complete` marker). This generalizes the rule the code already had — +`parent = None ⇒ nonce 0`, present in both `compute_next_nonce` and the trigger +— to `parent = None ⇒ nonce == anchor`. `run`'s first tip **is** the anchored +root; there is no sentinel batch row. + +Schema delta (`0001_schema.sql`): +- New `batch_tree_anchor` singleton (default 0). +- `trg_enforce_nonce_contiguity` parentless arm: `nonce == (SELECT nonce FROM + batch_tree_anchor)` — an **exact** match, *tighter* than the old "must be 0" + (a buggy root at any wrong nonce, incl. 0 on a recovered deployment, ABORTs) + — **plus** an at-most-one-**valid**-parentless-root guard scoped to + `invalidated_at_ms IS NULL`. +- `trg_batch_tree_anchor_write_once`: the anchor is frozen once `setup_complete` + exists (re-anchoring a live deployment would strand its spine). +- `compute_next_nonce(None)` reads the anchor. + +Normal (anchor 0) deployments are **byte-identical** to pre-PR5. The invariant +is *strengthened*, not relaxed: see [I16](../invariants.md). + +### Why the single-root guard is scoped to valid rows + +A fully-torn cascade (every batch invalidated, e.g. the recovered root itself +goes bad) re-roots via the existing `open_fresh_tip_in_tx` `parent = None` path — +at the anchor `N'`. The old invalidated root is still a parentless *row*, so the +guard must count only `invalidated_at_ms IS NULL` roots, or the re-root would +ABORT. This mirrors the pre-existing "second parentless root on a fully-torn +cascade" shape (which the code already produced at nonce 0). + +## Runner-up: sealed `N'-1` sentinel batch — rejected + +Insert one sealed, payload-less batch at `N'-1` (the schema even anticipated it: +"`payload_hash` ... NULL ... on recovery sentinels"); `run`'s tip parents off it. + +**Fatal, unguarded flaw** (panel finding, verified): the sentinel is a *valid +closed* batch at `N'-1`. `first_non_gold_closed_batch` selects any +`valid_closed_batches WHERE nonce >= frontier_nonce` as a cascade pivot. Nothing +in the schema stops a runtime cascade from selecting and invalidating the +sentinel; once gone, `run`'s tip re-roots parentless at 0 and the **unchanged** +contiguity trigger ABORTs all of recovery. Its safety rested entirely on "the +frontier never drops to `N'-1`" — an unenforced assumption, exactly the +"defense-in-depth against our own bugs" posture the codebase prefers (R5). It +also relaxes the trigger *more loosely* ("any sealed parentless nonzero") and +needs a new INSERT-already-sealed write path. The anchor has none of these. + +## Two map findings that simplified the fill + +- **The gold frontier auto-populates.** `populate_safe_accepted_batches` + simulates scheduler acceptance over `safe_inputs` from genesis on *every* sync + (cheap — nonce/staleness only, no app exec), so after recovery's sync-through-C + `frontier_nonce` already equals `N'`. No synthetic frontier row, no FK-referent + problem. ~~Recovery uses it as a fail-loud **cross-check**: `fold N' == + frontier_nonce` or abort.~~ **(Superseded — see the 2026-06-26 update and the + Resolution below: recovery *defers* frontier population, so there is no + cross-check; `N` is trusted metadata.)** +- **"Pre-burn" is harmless.** A batch's wire payload is frames (`user_ops` + + `safe_block`) with no direct list; on-chain drains are driven by `safe_block`. + So the locally-sequenced `≤ C` directs are pure local bookkeeping (cursor + + catch-up skip) and never reach L1. `run`'s first batch (frame `safe_block ≥ C`) + re-drains them on-chain exactly once, matching `S'`. + +## Idempotency / re-run + +`setup --recovery` is a **strict one-shot**: it refuses (terminal, +`SetupRecoveryError::AlreadySetUp`) on a DB that is already set up — the model is +"wipe the data dir and re-run". A crash *before* the `setup_complete` marker +leaves no marker and re-runs cleanly: `fill_recovery_state` is guarded by +finalized-snapshot existence and uses a unique-per-attempt dump dir. (This +resolves the PR3-review checkpoint-vs-marker note — chosen over the +persist-checkpoint-and-re-detect alternative.) + +## Tests owed (beyond the landed unit tests) + +The fold/fill components are unit-tested (anchor root/reject/single-root/freeze; +`fill_recovery_state` roots at `N'` + skips pre-executed directs + idempotent +re-run; exit-code mapping). Still owed: a storage-level **full-tear cascade on a +recovered (anchor = `N'`) tree** re-rooting at `N'`. + +## Update (2026-06-26) — e2e landed, and it caught a real I15 bug + +The affirmative **e2e recovery → run round-trip** (`setup_recovery_round_trip_test`, +rollups-e2e) now exists and **passes on the devnet** (39/39): build state → +promote a real finalized checkpoint → wipe → `setup --recovery` → `run` boots → +a post-recovery transfer at the continuing nonce is accepted (proving `S'` +preserved the sender's nonce + balance) → the rebuilt tree's anchoring at `N'` +is validated structurally. (Driving a *promoted* checkpoint on the devnet +needed a new `SEQ_MAX_BATCH_OPEN_SECONDS` knob — no prior test sealed + promoted +a batch.) + +It caught a genuine bug the rooting panel's "the frontier auto-populates" +assumption missed: **the content-identity check (I15) self-diverged during +recovery.** The recovered tree has no local batches below `N'` (folded into +`S'`), so `populate_safe_accepted_batches` saw every L1 batch as "foreign" and +froze the frontier, poisoning the rebuilt DB. Fixed (commit a37708e) with the +**anchor-aware frontier**: the frontier begins at the anchor, so `< N'` landings +are trusted collapsed history (skipped by nonce-mismatch, never content-checked) +and `setup --recovery` defers frontier population to `run`'s first sync. +Adversarially reviewed (4 lenses, all "i15-preserved", no blockers); runtime +foreign/zombie detection is byte-identical for genesis/running deployments. See +[I15](../invariants.md) (anchor-aware-frontier note). + +## Resolution — `N` is trusted; no recovery-time verifier + +The recovery-time `fold N' == frontier_nonce` cross-check was dropped (the +frontier isn't populated during recovery) and is **not owed back**. Two reasons, +established by a follow-up review of the external feedback: + +1. **The "content-free" replacement is circular.** Recomputing `N'` from the + synced `(B, C]` batch nonces via `protocol::advance_expected_batch_nonce` + seeds the fold from the very `N` it would check, so a wrong-high `N` baked + into a quiet `(B, C]` re-derives itself and passes. The only check + independent of `N` is a *from-genesis* scheduler-nonce replay — re-fetching + all of L1 — which we deliberately do not do. +2. **A legitimate checkpoint can't be wrong.** A sequencer-produced finalized + dump's `next_batch_nonce` is self-consistent by construction (closing nonce + + 1, finalized only once promoted on L1), so `N` is trusted the same way `S` + is. See [`cockroach.md`](../recovery/cockroach.md#data-dictionary). + +Correction to the earlier note: "a wrong checkpoint nonce is still caught loudly +at `run`" was **too strong**. Only a wrong-**low** `N` is caught at `run` (the +rebuilt root collides with live L1 history → content-identity check). A +wrong-**high** `N` silently diverges if the checkpoint trust boundary is +violated. The dead `SetupRecoveryError::ResumeNonceMismatch` variant was +removed. diff --git a/docs/review/2026-06-26-branch-deep-review.md b/docs/review/2026-06-26-branch-deep-review.md new file mode 100644 index 0000000..fcfa004 --- /dev/null +++ b/docs/review/2026-06-26-branch-deep-review.md @@ -0,0 +1,116 @@ +# Deep branch review — setup/run split → cockroach recovery (2026-06-26) + +A whole-branch review of the `feature/cockroach-recovery` stack (the setup/run +split, the scheduler-library extraction, the fold engine, and `setup +--recovery`). Two targets, in priority order: **(1) correctness** — design and +implementation bugs; **(2) simplification** — reduce weight via good +docs/specs and sharper abstractions, without an architectural restructure. + +This ledger records outcomes. The rooting design is settled separately in +[`2026-06-25-cockroach-recovery-rooting.md`](2026-06-25-cockroach-recovery-rooting.md); +the standing simplification backlog is +[`2026-06-10-simplification.md`](2026-06-10-simplification.md). + +## Target 1 — correctness: 4 findings, all fixed + +All four landed in commit `79311d0`; revalidated on devnet (e2e 39/39, workspace +green, clippy/fmt clean). + +| # | Sev | Finding | Fix | +|---|---|---|---| +| B1 | critical | `setup --recovery` partial-recovery re-run could silently re-anchor a tree whose durable root already carried a *different* `N'` (crash after the root tip, before `setup_complete`). | `open_tip_nonce()` re-entry guard in `fill_recovery_state`: a tip at a different nonce → fail-loud `SetupRecoveryError::PartialRecoveryMismatch` (exit 30). | +| B2 | high | A plain `setup` (genesis) run over recovery residue (anchor `≠ 0`) would root genesis at the recovery nonce. | Assert `anchor == 0` before `register_genesis_finalized_snapshot`; else `SetupRecoveryError::GenesisOverRecoveryResidue`. | +| B3 | medium | Review flagged the seed filter (`sender != submitter`) as a divergence bug. | **Re-examined → NON-bug**: the filter mirrors the scheduler's own sender-based classification (submitter inputs are batches, never directs-with-effect). Resolved by documenting the contract in `source_fold_inputs`, not by an over-firing assert. | +| B4 | medium | The fold's `(A,B]`/`(B,C]` disjointness assert used `>=`, admitting a replay input sharing the last seed block — looser than the disjoint-ranges docstring. | Tightened to `>` ("replay must start strictly after the last seeded direct"); added `replay_sharing_the_last_seed_block_panics`. | + +## Target 2 — simplification: verdict and queue + +**Verdict: no architectural restructure.** The branch's weight is best reduced +by naming contracts and consolidating duplicated semantics — consistent with the +standing simplification doc's verdict. The queue (P-items), and disposition: + +| # | Item | Status | +|---|---|---| +| P1 | `docs/recovery/cockroach.md` — the recovery spec (flush → fold → fill, data dictionary, code map). | **Done** (`4e25a9d`). | +| P3 | Replace the `populate_frontier: bool` with a `FrontierMode` enum (`Populate` / `DeferUntilAnchorSet`), making the I15 deferral self-documenting at every call site. | **Done** (`8e7a134`). | +| P2 | Name the recovery checkpoint (`struct Checkpoint` + `load`); extract `source_fold_inputs`; collapse `recover()` to six labelled steps. | **Done** (`dde8fa9`). | +| §2.2 | `docs/protocol/scheduler-semantics.md` — the authoritative I1 acceptance algorithm. | **Done** (`6006108`). | +| §2.3 | `docs/protocol/application-contract.md` — the authoritative `Application` FFI trait contract. | **Done** (`6006108`). | +| P4 | A `FoldInputSource` abstraction over seed/replay sourcing. | **Deferred — substantially addressed.** After P2 landed (`Checkpoint` + `source_fold_inputs`) and B4 tightened the boundary assert, the residual delta is a thin wrapper over a single call site. Adding it would *increase* surface for no readability gain — exactly the over-abstraction the simplification verdict warns against. Revisit only if a second fold-input source (e.g. a non-L1 checkpoint stream) ever appears. | + +### Drift corrected alongside the specs (in `6006108`) + +- Canonical-fold path `examples/canonical-app/src/scheduler/core.rs` → + `sequencer-core/src/scheduler/mod.rs` (stale since the PR2 move), in AGENTS.md + and invariants.md. +- invariants.md I1 cited a non-existent test + (`frontier_accepts_future_safe_block_batch_by_design`) as pinning the + `scheduler_accepts` structural-reject omission. The omission is sound by + self-trust and is now documented in `scheduler-semantics.md`; the false + citation was removed. +- The expected-nonce fold is no longer "slated for colocation": it is homed in + `protocol.rs` as `advance_expected_batch_nonce` (consumed by + `decide_submit_start`); `populate_safe_accepted_batches` keeps a deliberate, + documented inline copy. AGENTS.md/invariants.md updated to match. + +## Open / owed (non-blocking) + +- **~~Content-free recovery-time nonce cross-check~~ — resolved: `N` is + trusted.** Settled after the external-review follow-up: recovery does not + re-verify `N`, and the dead `ResumeNonceMismatch` variant was removed. The + once-proposed `advance_expected_batch_nonce` cross-check is circular (it seeds + from the `N` it would check); only a from-genesis replay is independent, and + that is deliberately not built. The earlier "a wrong checkpoint nonce is still + caught loudly at `run`" was too strong — only wrong-low `N` is caught at run, + wrong-high is not, which is acceptable because a sequencer-produced finalized + dump cannot carry a wrong `N`. See the + [rooting-doc Resolution](2026-06-25-cockroach-recovery-rooting.md#resolution--n-is-trusted-no-recovery-time-verifier) + and [`cockroach.md`](../recovery/cockroach.md#data-dictionary). +- **Frontier-`nonce`/anchor asymmetry (external review)** — **fixed**: + `frontier_nonce` now defaults to the batch-tree anchor (not 0) on an empty + accepted table, so a recovered deployment's submitter starts at `N'` instead + of re-submitting its first post-recovery batch each tick until the first + accepted row lands. The anchor-read query is consolidated into one + `&Connection` helper shared by `frontier_nonce`, `populate_safe_accepted_batches`, + and `compute_next_nonce`. +- **Full-tear cascade on a recovered (anchor = `N'`) tree** — a storage-level + test of re-rooting at `N'` after every batch is invalidated. Unit coverage + exists for the anchor mechanics; this end-to-end shape is still owed. + +## Follow-on multi-agent adversarial sweep (2026-06) + +A whole-branch adversarial review (8 subsystem finders → 3 perspective-diverse +skeptics per finding: code-accuracy / reachability / already-mitigated). +**4 confirmed, 3 refuted.** All four fixed: + +- **[P1, fixed] `setup --recovery` dropped deposits in the `(C, H1]` window.** The + post-flush resync runs to the live safe head `H1` (normally `> C`), but the + fold folds only `≤ C` into `S'`. `fill_recovery_state` used the generic + `ensure_open_tip`, which drains the **whole** synced table — sequencing the + `(C, H1]` directs as already-executed (cursor + snapshot `l2_tx_index` past + them) while `S'` never executed them. They were then skipped by catch-up and + never re-led → vanished locally while the scheduler drains them on-chain + (divergence / lost deposit). A genuinely new class, distinct from the three + prior re-entry-guard gaps; the round-trip e2e never injected a `(C, H1]` + deposit. **Fix:** `open_recovery_tip` caps the drain at `C` (frame + `safe_block = C`), leaving `(C, H1]` undrained so `run` leads + executes them + once as the frontier advances `C → H1`. Test: + `fill_recovery_state_leaves_post_c_directs_undrained`. cockroach.md steps + 3/6 + constraints updated. +- **[P2, fixed] Wrong-chain RPC during a startup-recovery sync looped instead of + failing terminal.** The reader's `ChainIdMismatch` surfaced as + `BootstrapError::Recovery(RecoveryError::InputReader(..))` → the `Recovery(_)` + catch-all → exit 1 (unclassified) → restart loop on the wrong chain. **Fix:** + explicit terminal arm in `bootstrap_exit_code`, mirroring the worker/boot + arms; exit-code test added. +- **[P3, fixed] cockroach.md "Flush note" link** pointed at README.md; the + section is in PLAN.md. Retargeted. +- **[P3, fixed] format.md Versioning section** still named `WalletSnapshotV1` + current / `V2` hypothetical; this branch shipped `V2`. Updated (current = V2, + example future = V3). + +Refuted (correctly): the `(A, B]` seed-boundary "unstated dependency" +(self-conceded non-bug), the early-return F5-witness skip (premise impossible — +inputs cannot precede the InputBox deployment block), and an `assert_tree_invariants` +unit-test helper hard-coding `nonce 0` (a test-only coverage gap, not a runtime +bug — a fair minor cleanup if revisited). diff --git a/sequencer/src/storage/convert.rs b/sequencer/src/storage/convert.rs index be26c28..99a2f35 100644 --- a/sequencer/src/storage/convert.rs +++ b/sequencer/src/storage/convert.rs @@ -1,21 +1,36 @@ // (c) Cartesi and individual authors (see AUTHORS) // SPDX-License-Identifier: Apache-2.0 (see LICENSE) -//! Saturating width conversions between Rust and SQLite integer types, plus -//! `SystemTime` ↔ `i64` Unix-ms conversions. +//! Integer conversions between Rust domain types and SQLite `INTEGER` columns, +//! plus `SystemTime` ↔ `i64` Unix-ms conversions. //! -//! SQLite stores integers as `INTEGER` (signed 64-bit). Rust domain types use -//! narrower unsigned widths (`u16`, `u32`, `u64`). The conversions here are -//! load-bearing glue that the rest of the storage module calls pervasively. +//! Two conversion families with opposite postures, per the fail-loud check +//! policy in `docs/invariants.md`: //! -//! All conversions saturate rather than panic — the domain values we persist -//! are always non-negative and well within `i64::MAX`, but saturation keeps -//! corrupted or malicious DB rows from crashing the process. +//! - **Contract-bound conversions fail loud.** Domain values (indices, nonces, +//! block numbers, offsets, fees) are non-negative and far below `i64::MAX` +//! by schema `CHECK`s, triggers, or writer-side types. An out-of-range value +//! can only mean DB corruption, tampering, or a sequencer bug; saturating it +//! would fabricate a plausible value and let the divergence externalize (a +//! signed batch, a feed event, a wrong recovery pivot). These panic — a loud +//! crash is recoverable by design (orchestrator respawn + startup recovery), +//! silent divergence is not. (2026-07 decode-policy audit: every non-test +//! width-conversion call site is contract-bound except the ones below.) +//! - **Clock and query-bound conversions saturate.** Wall-clock time is +//! environmental, not an invariant (review F8): a far-future clock clamps to +//! `i64::MAX` rather than aborting. Untrusted or config-sourced SQL bounds +//! go through [`saturating_query_bound`], where clamping preserves the +//! comparison semantics exactly. Sign remains contract-bound even for clock +//! *columns*: every timestamp writer is u64-clock-sourced (floored at 0), so +//! [`from_unix_ms`] fail-louds on a negative stored value — a sign check is +//! a real invariant, unlike the ordering checks F8 warns about. use std::time::{Duration, SystemTime, UNIX_EPOCH}; // ── Time helpers ────────────────────────────────────────────────────────── +/// Saturating: a pre-epoch clock clamps to 0, a far-future clock to +/// `i64::MAX`. Wall-clock state is environmental (F8), never an invariant. pub(super) fn to_unix_ms(time: SystemTime) -> i64 { time.duration_since(UNIX_EPOCH) .unwrap_or_default() @@ -24,12 +39,19 @@ pub(super) fn to_unix_ms(time: SystemTime) -> i64 { .unwrap_or(i64::MAX) } +/// Fail-loud on sign: every timestamp writer is u64-clock-sourced, so a +/// negative stored Unix-ms value is contract-impossible. Ordering and +/// monotonicity are deliberately NOT checked here (review F8: wall-clock +/// regression is legitimate). pub(super) fn from_unix_ms(ms: i64) -> SystemTime { - let clamped_ms = ms.max(0) as u64; - UNIX_EPOCH + Duration::from_millis(clamped_ms) + let ms = u64::try_from(ms).unwrap_or_else(|_| { + panic!("stored unix-ms timestamp {ms} is negative: contract-impossible") + }); + UNIX_EPOCH + Duration::from_millis(ms) } -/// Current wall-clock time as an `i64` SQLite timestamp. +/// Current wall-clock time as an `i64` SQLite timestamp. Saturating (see +/// [`to_unix_ms`]). /// /// Delegates to [`crate::runtime::clock::unix_now_ms`] so the whole crate goes /// through one clock entry point. @@ -37,24 +59,40 @@ pub(super) fn now_unix_ms() -> i64 { i64::try_from(crate::runtime::clock::unix_now_ms()).unwrap_or(i64::MAX) } -// ── Width conversions ───────────────────────────────────────────────────── +// ── Contract-bound width conversions (fail loud) ────────────────────────── pub(super) fn u64_to_i64(value: u64) -> i64 { - i64::try_from(value).unwrap_or(i64::MAX) -} - -pub(super) fn usize_to_i64(value: usize) -> i64 { - i64::try_from(value).unwrap_or(i64::MAX) + i64::try_from(value) + .unwrap_or_else(|_| panic!("domain value {value} exceeds i64::MAX: contract-impossible")) } pub(super) fn i64_to_u64(value: i64) -> u64 { - value.max(0) as u64 + u64::try_from(value) + .unwrap_or_else(|_| panic!("stored value {value} is negative: contract-impossible")) } pub(super) fn i64_to_u16(value: i64) -> u16 { - u16::try_from(value.max(0)).unwrap_or(u16::MAX) + u16::try_from(value).unwrap_or_else(|_| { + panic!("stored value {value} is outside u16 range: contract-impossible") + }) } pub(super) fn i64_to_u32(value: i64) -> u32 { - u32::try_from(value.max(0)).unwrap_or(u32::MAX) + u32::try_from(value).unwrap_or_else(|_| { + panic!("stored value {value} is outside u32 range: contract-impossible") + }) +} + +// ── Query-bound conversion (saturating, by design) ──────────────────────── + +/// Saturating clamp for **untrusted or config-sourced** SQL query bounds: +/// WS `from_offset` cursors and page/count `LIMIT`s. The full `u64` range is +/// legal input here, and clamping to `i64::MAX` preserves the comparison +/// exactly — no rowid exceeds `i64::MAX`, so a past-the-end bound matches +/// zero rows and a clamped `LIMIT` is effectively unbounded. +/// +/// Never use this for domain values read from or written to columns; those +/// go through the fail-loud converters above. +pub(super) fn saturating_query_bound(value: u64) -> i64 { + i64::try_from(value).unwrap_or(i64::MAX) } diff --git a/sequencer/src/storage/egress.rs b/sequencer/src/storage/egress.rs index d2692e6..df4d9ab 100644 --- a/sequencer/src/storage/egress.rs +++ b/sequencer/src/storage/egress.rs @@ -11,7 +11,7 @@ use alloy_primitives::{Address, B256}; use rusqlite::{Result, params}; use super::Storage; -use super::convert::{i64_to_u64, u64_to_i64, usize_to_i64}; +use super::convert::{i64_to_u32, i64_to_u64, saturating_query_bound}; use super::queries::decode_l2_tx_row; use sequencer_core::l2_tx::{DirectInput, SequencedL2Tx, ValidUserOp}; @@ -128,40 +128,47 @@ impl Storage { LIMIT ?2 "; let mut stmt = self.conn.prepare_cached(SQL)?; - let rows = stmt.query_map(params![u64_to_i64(offset), usize_to_i64(limit)], |row| { - let db_offset: i64 = row.get(0)?; - let tx = decode_l2_tx_row( - row.get(1)?, - row.get(2)?, - row.get(3)?, - row.get(4)?, - row.get(5)?, - row.get(6)?, - ); - // Non-NULL for every sequenced row: batches and frames exist before - // anything can be sequenced into them. - let safe_block = i64_to_u64(row.get(7)?); - let batch_nonce = i64_to_u64(row.get(8)?); - match tx { - SequencedL2Tx::UserOp(tx) => Ok(OrderedL2TxRow::UserOp { - offset: i64_to_u64(db_offset), - tx, - nonce: u32::try_from(row.get::<_, i64>(10)?) - .expect("persisted user op nonce must fit u32"), - safe_block, - batch_nonce, - }), - SequencedL2Tx::Direct(tx) => Ok(OrderedL2TxRow::DirectInput { - offset: i64_to_u64(db_offset), - tx, - input_index: i64_to_u64(row.get(9)?), - safe_block, - batch_nonce, - block_timestamp: i64_to_u64(row.get(11)?), - transaction_hash: B256::from_slice(row.get::<_, Vec>(12)?.as_slice()), - }), - } - })?; + // Query bounds saturate by design: `offset` can be a client-supplied + // WS cursor and `limit` is config-sourced (see `saturating_query_bound`). + let rows = stmt.query_map( + params![ + saturating_query_bound(offset), + saturating_query_bound(limit as u64) + ], + |row| { + let db_offset: i64 = row.get(0)?; + let tx = decode_l2_tx_row( + row.get(1)?, + row.get(2)?, + row.get(3)?, + row.get(4)?, + row.get(5)?, + row.get(6)?, + ); + // Non-NULL for every sequenced row: batches and frames exist before + // anything can be sequenced into them. + let safe_block = i64_to_u64(row.get(7)?); + let batch_nonce = i64_to_u64(row.get(8)?); + match tx { + SequencedL2Tx::UserOp(tx) => Ok(OrderedL2TxRow::UserOp { + offset: i64_to_u64(db_offset), + tx, + nonce: i64_to_u32(row.get(10)?), + safe_block, + batch_nonce, + }), + SequencedL2Tx::Direct(tx) => Ok(OrderedL2TxRow::DirectInput { + offset: i64_to_u64(db_offset), + tx, + input_index: i64_to_u64(row.get(9)?), + safe_block, + batch_nonce, + block_timestamp: i64_to_u64(row.get(11)?), + transaction_hash: B256::from_slice(row.get::<_, Vec>(12)?.as_slice()), + }), + } + }, + )?; rows.collect::>>() } @@ -202,7 +209,11 @@ impl Storage { )"; self.conn.query_row( SQL, - params![u64_to_i64(from_offset), addr.as_slice(), u64_to_i64(limit)], + params![ + saturating_query_bound(from_offset), + addr.as_slice(), + saturating_query_bound(limit) + ], |row| row.get(0), )? } @@ -215,7 +226,10 @@ impl Storage { )"; self.conn.query_row( SQL, - params![u64_to_i64(from_offset), u64_to_i64(limit)], + params![ + saturating_query_bound(from_offset), + saturating_query_bound(limit) + ], |row| row.get(0), )? } diff --git a/sequencer/src/storage/l1_inputs.rs b/sequencer/src/storage/l1_inputs.rs index 944e374..4565218 100644 --- a/sequencer/src/storage/l1_inputs.rs +++ b/sequencer/src/storage/l1_inputs.rs @@ -419,7 +419,11 @@ fn insert_safe_inputs_batch( )?; for (offset, input) in inputs.iter().enumerate() { stmt.execute(params![ - u64_to_i64(start_index.saturating_add(offset as u64)), + u64_to_i64( + start_index + .checked_add(offset as u64) + .expect("safe-input index overflow: contract-impossible"), + ), input.sender().as_slice(), input.payload(), u64_to_i64(input.block_number()), diff --git a/sequencer/src/storage/migrations/0001_schema.sql b/sequencer/src/storage/migrations/0001_schema.sql index 49bd6f5..9faf01e 100644 --- a/sequencer/src/storage/migrations/0001_schema.sql +++ b/sequencer/src/storage/migrations/0001_schema.sql @@ -328,7 +328,9 @@ WHERE batch_index NOT IN (SELECT batch_index FROM batches WHERE invalidated_at_m -- acceptance logic over new safe_inputs rows. CREATE TABLE IF NOT EXISTS safe_accepted_batches ( safe_input_index INTEGER PRIMARY KEY REFERENCES safe_inputs(safe_input_index), - nonce INTEGER NOT NULL, + -- CHECK aligns this column with its siblings (batches.nonce, anchor nonce); + -- the writer is u64-sourced, so a negative value is corruption. + nonce INTEGER NOT NULL CHECK (nonce >= 0), first_frame_safe_block INTEGER NOT NULL, inclusion_block INTEGER NOT NULL ); diff --git a/sequencer/src/storage/mutations.rs b/sequencer/src/storage/mutations.rs index 88dab7d..dc77e36 100644 --- a/sequencer/src/storage/mutations.rs +++ b/sequencer/src/storage/mutations.rs @@ -72,7 +72,9 @@ fn compute_next_nonce(tx: &Transaction<'_>, parent_batch_index: Option) -> params![u64_to_i64(parent_bi)], |row| row.get(0), )?; - Ok(i64_to_u64(parent_nonce).saturating_add(1)) + Ok(i64_to_u64(parent_nonce) + .checked_add(1) + .expect("batch nonce overflow: contract-impossible")) } } } diff --git a/sequencer/src/storage/queries.rs b/sequencer/src/storage/queries.rs index e7abb82..d73c151 100644 --- a/sequencer/src/storage/queries.rs +++ b/sequencer/src/storage/queries.rs @@ -156,10 +156,18 @@ pub(super) fn query_batch_policy(conn: &Connection) -> Result { |row| Ok((row.get(0)?, row.get(1)?)), )?; let max_exp = sequencer_core::fee::MAX_EXPONENT; + // Deliberate saturating clamps, NOT fail-loud decodes: both columns are + // policy-derived log-space sums whose range is set by admin writes + // (`set_log_gas_price`, `set_alpha`), which can legally push + // log_recommended_fee past u16::MAX and log_batch_size_target below 0. + // Clamping into [0, MAX_EXPONENT] absorbs legal-range inputs (tested in + // admin.rs) and prevents panics in fee_to_linear — not corruption. + let clamp_log = |value: i64| -> u16 { + u16::try_from(value.clamp(0, i64::from(max_exp))).expect("clamped into u16 range") + }; Ok(BatchPolicy { - // Clamp to MAX_EXPONENT to prevent panics in fee_to_linear. - recommended_fee: i64_to_u16(log_recommended_fee).min(max_exp), - batch_size_target: i64_to_u16(log_batch_size_target).min(max_exp), + recommended_fee: clamp_log(log_recommended_fee), + batch_size_target: clamp_log(log_batch_size_target), }) }