Repository navigation
fix(prover): preserve checkpoint work across restarts (STR-4562) - #2390
Conversation
|
Commit: 5cb0c1c SP1 Execution Results
|
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. |
🔒 AI Security Review (claude-opus-5-5)✅ No security issues found. |
|
@codex review |
![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: e52eb8a3fb
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
voidash
left a comment
voidash
left a comment
There was a problem hiding this comment.
LGTM but we might want to delete the dead code related to storage manager
Codecov Report❌ Patch coverage is @@ Coverage Diff @@
## main #2390 +/- ##
==========================================
+ Coverage 89.14% 89.21% +0.06%
==========================================
Files 441 440 -1
Lines 75737 75930 +193
==========================================
+ Hits 67515 67740 +225
+ Misses 8222 8190 -32
Flags with carried forward coverage won't be shown. Click here to find out more.
... and 14 files with indirect coverage changes 🚀 New features to boost your workflow:
|
storopoli
left a comment
storopoli
left a comment
There was a problem hiding this comment.
The P1 is real and we should address them before anything else. This is a good start but it's not ready to merge yet.
Remove startup cleanup of local checkpoint payloads, proofs, and tasks beyond ASM's verified tip. Preserve saved remote requests and existing artifact and epoch-spec validation. Cover unaccepted checkpoint restart retention, non-destructive artifact-check failures, and persisted task recovery metadata. Refs: STR-4562
Signal BlockAccepted after FCM finishes block processing and persists Valid. Let chain worker inspect the terminal flag, complete epoch processing, and store and notify the summary. Keep summary creation failures outside the block rejection path. Cover acceptance ordering, rejected blocks, and ordinary blocks in regression tests. Refs: STR-4562
e52eb8a to
f9ffffe
Compare
🔒 AI Security Review (claude-opus-5-5)✅ No security issues found. |
🔒 AI Security Review (claude-opus-5-5)✅ No security issues found. |
krsnapaudel
left a comment
krsnapaudel
left a comment
There was a problem hiding this comment.
LGTM. Good cleanup for epoch summary creation path. We could revisit FCM setting status multiple times later if needed.
Description
Restarting a prover-enabled node deleted checkpoint payloads, proofs, and tasks beyond ASM's verified tip, including saved remote request IDs and signing records. Remove that startup cleanup so completed proof work is reused and unfinished tasks remain available to the existing recovery path.
Document the retention assumptions and add coverage for unaccepted checkpoints surviving restart, failed artifact validation preserving stored work, and task metadata surviving database reopening.
Type of Change
Notes to Reviewers
OL only incorporates buried ASM manifests, and protocol upgrades assign rules per epoch without invalidating completed epochs. ASM acceptance lag is therefore not a reason to delete proof work. Existing artifact validation, spec-scoped recovery, and epoch commitment/spec checks remain in place. Deep-reorg recovery and writer-intent cancellation are outside this change.
Validation:
./run_tests.sh -t test_checkpoint_restart test_checkpoint_artifact_availabilitypassed.just test-unit: 2,468 passed, 2 skipped.just lint-check-wspassed.just pr-litepassed in a clean checkout of this exact patch, including all lint, documentation, doctest, and unit-test stages (2,468 passed, 2 skipped). The clean checkout excludes unrelated ignored local investigation files.Remote-request preservation is tested through persisted task metadata; these tests do not submit live remote proof jobs.
Is this PR addressing any specification, design doc or external reference document?
Relevant link: STR-4562. The implementation follows the clarified scope: preserve work across restart without introducing automatic deletion for upgrades or supported shallow reorgs.
AI assistance: Codex authored the implementation, documentation, and tests, performed reviews, ran validation, and drafted this PR body with human direction and review.
Checklist
Related Issues
Closes STR-4562