Skip to content

Conversation

@gianbelinche
Copy link
Contributor

@gianbelinche gianbelinche commented Oct 30, 2025

Motivation
Checkpoints are never deleted

Description
Delete checkpoints once we verify the corresponding batch

Copilot AI review requested due to automatic review settings October 30, 2025 13:18
@gianbelinche gianbelinche requested a review from a team as a code owner October 30, 2025 13:18
@github-actions github-actions bot added the L2 Rollup client label Oct 30, 2025
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull Request Overview

This PR adds automatic cleanup of checkpoint directories after batch proofs are successfully verified on L1. The L1ProofSender now receives the checkpoints_dir path and removes old checkpoint directories once they are no longer needed.

  • Passes checkpoints_dir to L1ProofSender for checkpoint cleanup
  • Adds logic to delete checkpoint directories after batch proof verification
  • Uses PathBuf cloning to allow sharing the path between L1Committer and L1ProofSender

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.

File Description
crates/l2/sequencer/mod.rs Clones checkpoints_dir when spawning L1Committer and passes the original to L1ProofSender
crates/l2/sequencer/l1_proof_sender.rs Adds checkpoints_dir field, updates constructor/spawn signatures, and implements checkpoint deletion logic after proof verification

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

.checkpoints_dir
.join(format!("checkpoint_batch_{}", batch_to_send - 1));
if checkpoint_path.exists() {
let _ = remove_dir_all(&checkpoint_path).inspect_err(|e| {
Copy link

Copilot AI Oct 30, 2025

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using std::fs::remove_dir_all is a blocking file system operation being called in an async context. This can block the async executor thread. Consider using tokio::fs::remove_dir_all instead for non-blocking async file operations.

Copilot uses AI. Check for mistakes.
@github-actions
Copy link

github-actions bot commented Oct 30, 2025

Lines of code report

Total lines added: 24
Total lines removed: 2
Total lines changed: 26

Detailed view
+-----------------------------------------------+-------+------+
| File                                          | Lines | Diff |
+-----------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/l1_committer.rs    | 1032  | -2   |
+-----------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/l1_proof_sender.rs | 459   | +20  |
+-----------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/mod.rs             | 226   | +1   |
+-----------------------------------------------+-------+------+
| ethrex/crates/l2/sequencer/utils.rs           | 170   | +3   |
+-----------------------------------------------+-------+------+

Comment on lines 214 to 216
let checkpoint_path = self
.checkpoints_dir
.join(format!("checkpoint_batch_{}", batch_to_send - 1));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Document why batch_to_send - 1.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@ManuelBilbao ManuelBilbao added this pull request to the merge queue Nov 1, 2025
Merged via the queue into main with commit 02c4468 Nov 1, 2025
37 checks passed
@ManuelBilbao ManuelBilbao deleted the remov-used-checkpoints branch November 1, 2025 04:47
@github-project-automation github-project-automation bot moved this to Done in ethrex_l2 Nov 1, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

L2 Rollup client

Projects

Status: Done

Development

Successfully merging this pull request may close these issues.

5 participants