fix(l2): save last committed batch as current checkpoint #5152
Conversation
There was a problem hiding this comment.
Pull Request Overview
This PR refactors the batch production logic in the L1 committer by extracting the batch creation code into a separate produce_batch method. The refactoring simplifies the main commit flow and changes how checkpoints are managed during batch production.
Key changes:
- Extracted batch production logic into a new
produce_batchmethod - Changed from temporary checkpoints to permanent checkpoints created during batch production
- Updated checkpoint management to assign checkpoints directly rather than creating them separately
- Introduced a
batch_checkpoint_namehelper function to standardize checkpoint naming
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| async fn produce_batch(&mut self, batch_number: u64) -> Result<Option<Batch>, CommitterError> { | ||
| let last_committed_blocks = self | ||
| .rollup_store | ||
| .get_block_numbers_by_batch(batch_number-1) |
There was a problem hiding this comment.
Missing spaces around the subtraction operator. Should be batch_number - 1 for consistency with Rust formatting conventions.
| .get_block_numbers_by_batch(batch_number-1) | |
| .get_block_numbers_by_batch(batch_number - 1) |
| .get_block_numbers_by_batch(batch_number-1) | ||
| .await? | ||
| .ok_or( | ||
| CommitterError::RetrievalError(format!("Failed to get batch with batch number {}. Batch is missing when it should be present. This is a bug", batch_number)) |
There was a problem hiding this comment.
The error message refers to getting a batch but is actually fetching block numbers by batch. The message should say 'Failed to get blocks for batch number {}' to accurately reflect what failed.
| CommitterError::RetrievalError(format!("Failed to get batch with batch number {}. Batch is missing when it should be present. This is a bug", batch_number)) | |
| CommitterError::RetrievalError(format!("Failed to get blocks for batch number {}. Block numbers are missing when they should be present. This is a bug", batch_number)) |
crates/l2/sequencer/l1_committer.rs
Outdated
| batch.number | ||
| )); | ||
| // We need to create a one-time checkpoint copy because if witness generation fails the checkpoint would be modified | ||
| info!(batch = batch.number, "WITNESS OTC"); |
There was a problem hiding this comment.
Debug log message 'WITNESS OTC' appears to be temporary debugging code that should be removed before merging to production.
| info!(batch = batch.number, "WITNESS OTC"); |
Lines of code reportTotal lines added: Detailed view |
**Motivation** <!-- Why does this pull request exist? What are its goals? --> We are saving the latest `Store` as the "current" one. This means the next time a batch is constructed, it might use a latter checkpoint than the needed. This can occur if the batch is sealed with less blocks than the available (e.g., if there's no more available space). **Description** <!-- A clear and concise general description of the changes this PR introduces --> Save a checkpoint from the batch execution instead <!-- Link to issues: Resolves #111, Resolves #222 -->
Motivation
We are saving the latest
Storeas the "current" one. This means the next time a batch is constructed, it might use a latter checkpoint than the needed. This can occur if the batch is sealed with less blocks than the available (e.g., if there's no more available space).Description
Save a checkpoint from the batch execution instead