Repository navigation
Conversation
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
|
Commit: 1f48122 SP1 Execution Results
|
🔒 AI Security Review (claude-opus-5-5)✅ No security issues found. |
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #2369 +/- ##
==========================================
+ Coverage 89.21% 89.32% +0.10%
==========================================
Files 440 445 +5
Lines 75934 76666 +732
==========================================
+ Hits 67747 68481 +734
+ Misses 8187 8185 -2
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 7 files with indirect coverage changes 🚀 New features to boost your workflow:
|
|
@storopoli I see that you are adding checkpoint submission tracker here. I like the idea, but in the PR you do this for detecting checkpoints rejected by ASM. I feel the submission tracker could track checkpoint in the submission/publication path in a sequencer. Another observation tracker could take over after ASM emits a log: basically what CSM does now. Both could be part of one OL checkpoint tracker. I have written a rough draft. We can discuss the ideas in tomorrow's sync. We should also discuss whether ASM should emit a log for rejection. Otherwise detection of rejection is indirect. |
|
Thanks, I read the draft. I agree with splitting this into submission tracking on the sequencer and observation on every node, with checkpoint status kept in the OL checkpoint DB. STR-4084 is detection only, so this PR is narrower. It reads the writer's bundles and the broadcaster's tx status and records rejections of this sequencer's own checkpoints, without changing how the writer, broadcaster or CSM behave. It doesn't catch everything: if another transaction gets the epoch accepted before the rejected one is buried, the rejection isn't recorded. The ASM does log a tracing warning on rejection, but there's no rejection |
🔒 AI Security Review (claude-opus-5-5)✅ No security issues found. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: a6d7fc0299
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
🔒 AI Security Review (claude-opus-5-5)✅ No security issues found. |
1 similar comment
🔒 AI Security Review (claude-opus-5-5)✅ No security issues found. |
The ASM emits nothing when it rejects a checkpoint. Around a predicate rotation, a checkpoint proven with the wrong artifact can be mined and rejected while the broadcaster marks it finalized, and nothing reports it. Add a sequencer-side submission tracker to strata-ol-checkpoint. It follows the checkpoint state the CSM publishes, scans the L1 writer's checkpoint bundles from a persisted cursor, and resolves each reveal through the broadcaster's replacement chain. A checkpoint is recorded as confirmed-but-rejected once its inclusion block is buried at `l1_reorg_safe_depth` under the CSM tip while the ASM's verified epoch is still below the checkpoint's epoch. Rejections are committed together with the cursor, then logged at warn once and counted in `strata_checkpoint_confirmed_rejected_total`. The `strata_checkpoint_confirmed_rejected` gauge counts recorded rejections whose epoch the ASM has still not accepted. Writer bundles survive the startup artifact reconciliation, so a restart resumes from the cursor without losing or re-alerting a submission. Detection only: writer, broadcaster and prover state are untouched. Refs: STR-4084
A failed check used to warn on every 30 s retry for as long as the failure lasted. The tracker state now remembers the last failure: the first one is logged at warn, repeats of the same failure at debug, and the recovery once at info. Retries still run on the next update or tick. An undecodable checkpoint bundle had the same problem while an earlier unsettled bundle held the cursor. The context now reports it as `BundleContents::Undecodable` instead of logging, and the state warns once per bundle. Refs: STR-4084
The submission tracker's module doc said the ASM emits nothing on rejection. At v0.4.0-rc.6 the checkpoint handler logs a tracing warning but emits no ASM log, so the node still keeps no record of the rejection.
The unresolved-rejections gauge was only set at the end of a completed scan, so it kept a stale value while a check deferred or failed. It is now set after every check and renamed to strata_checkpoint_unresolved_rejections, so it no longer shares the counter's name for a different quantity. A deferred check (no genesis, or a CSM tip the node no longer considers canonical) no longer counts as recovering from a failure, which broke warn-once logging. Also: - ConfirmedRejected carries the mined reveal, so the verdict match is exhaustive. - SubmissionTrackerState::new, which reads storage, is now load. - Context methods that read storage have get_ names. - An undecodable checkpoint bundle's warning says why it does not decode.
Reverting the L1 canonical chain purges the height cache before deleting the rows, so the submission tracker's canonical lookup can briefly see a reverted block during a reorg. It cannot produce a false rejection record short of a reorg as deep as the reorg-safe depth; the fix belongs in L1BlockManager.
2480ffd to
a4bf8e1
Compare
🔒 AI Security Review (claude-opus-5-5)✅ No security issues found. |
Description
The ASM only logs a tracing warning when it rejects a checkpoint. It emits no ASM log, so the node keeps no record of the rejection. Around a predicate rotation, a checkpoint proven with the wrong artifact can be mined and rejected while the broadcaster marks it
Finalized.This PR adds a sequencer-only tracker in
strata-ol-checkpoint(submission_tracker/). There is no automatic recovery. On eachCheckpointStatethe CSM publishes, and on a 30 s tick, the tracker scans the L1 writer's checkpoint bundles from a persisted cursor and resolves each reveal through the broadcaster, following RBF replacements. A checkpoint counts as confirmed-but-rejected once its canonical inclusion block isbtcio.l1_reorg_safe_depthdeep under the CSM tip and the ASM's verified epoch is still below the checkpoint's epoch. The CSM already uses that depth for checkpoint finality, so there is no new config.Each rejection goes into a new
ol_checkpointtable (CBOR, keyed by txid), committed together with the cursor, then logged once atwarn(checkpoint confirmed on L1 but rejected by ASM) and counted instrata_checkpoint_confirmed_rejected_total. Thestrata_checkpoint_unresolved_rejectionsgauge counts rejections whose epoch is still unaccepted. A failed check retries on the next tick and warns once, with repeats atdebugand the recovery atinfo; an undecodable bundle also warns once, with the reason.#2370 adds the RPC and the functional test.
Type of Change
Notes to Reviewers
verified.epoch + 1, and depth is measured from the CSM tip, so the ASM has processed the inclusion block before the tracker decides. A rejected posting isn't reported if another posting gets its epoch accepted first.reconcile_unaccepted_checkpoint_artifacts.provers/sp1/guest-checkpoint/Cargo.lockis unchanged.strata-ol-checkpoint, used only bybin/strata, gainsstrata-btcio,strata-csm-types,strata-db-types,strata-codec,strata-codec-utilsandstrata-asm-proto-checkpoint-txs.Is this PR addressing any specification, design doc or external reference document?
Checklist
Testing
Ran
just fmt-ws,just lint-check-ws, the TOML, codespell, style and TODO lints, andcargo nexteston the touched crates. Unit tests cover the classifier and the worker against an in-memory context: repeated observations, a reorg before depth, an acceptance undone by a shallow reorg, restarts, an out-of-order rebuild, and an unmined bundle holding the cursor. Others cover warn-once logging, the sled table, and building the timer input outside the tokio runtime. CI passes, including functional tests. The end-to-end test is in #2370.AI use
Claude Code wrote the implementation, the tests and this description. OpenAI Codex reviewed the plan and the diff (read-only).
Related Issues
STR-4084