feat: l1 reorg #409
feat: l1 reorg #409
Conversation
CodSpeed Performance ReportMerging #409 will not alter performanceComparing Summary
|
frisitano
left a comment
frisitano
left a comment
There was a problem hiding this comment.
Added some comments inline
| /// The finalized block info after finalizing the consolidated batches. | ||
| finalized_block_info: Option<BlockInfo>, |
There was a problem hiding this comment.
I question whether this is needed here. I must consider this case in more depth but I believe this will always be none.
There was a problem hiding this comment.
I think in practice this will never be needed. The reason is that we always wait for the BatchFinalized event to be finalized. As such, there should never be an impact on the finalized head from a BatchFinalized event.
crates/chain-orchestrator/src/lib.rs
Outdated
| async fn handle_l1_finalized( | ||
| &mut self, | ||
| block_number: u64, | ||
| block_info: BlockInfo, |
There was a problem hiding this comment.
It appears that we only ever use the finalized block number and not the hash. We should evaluate to see if we can simplify this by just using the block number.
crates/chain-orchestrator/src/lib.rs
Outdated
| batch_info: BatchInfo::new(batch_clone.index, batch_clone.hash), | ||
| l1_block_number: batch_clone.block_number, | ||
| safe_head: new_safe_head, | ||
| batch_info: BatchInfo::new(batch.index, batch.hash), |
There was a problem hiding this comment.
We can use (&batch).into()
crates/primitives/src/batch.rs
Outdated
| } | ||
|
|
||
| impl core::str::FromStr for BatchStatus { | ||
| type Err = (); |
There was a problem hiding this comment.
Add an error type
crates/watcher/src/lib.rs
Outdated
| head: BlockInfo, | ||
| finalized: BlockInfo, |
There was a problem hiding this comment.
Do we need BlockInfo in this context?
| // remove the L1 block infos greater than the provided l1 block number | ||
| self.remove_l1_block_info_gt(l1_block_number).await?; | ||
|
|
||
| // delete batch commits, l1 messages and batch finalization effects greater than the |
There was a problem hiding this comment.
If we configure the tables with ON DELETE CASCADE on a foreign key relationship we could minimize the code necessary here and let the DB take care of it once L1 blocks are deleted. While this introduces implicit behavior it should also guarantee that the DB is in a consistent state and reduce coding errors by forgetting to clean up.
There was a problem hiding this comment.
I had considered this, and there are a few considerations here:
- We have to maintain all L1 block info for every event, even after they are finalized, maybe not a huge deal (I estimate this would result in approximately 0.5GB additional storage requirement on mainnet)
- Using
ON DELETE CASCADEworks in some cases, for example, we use this to delete safe L2 blocks when batches are deleted. However, if we take the status field in the batch commit table, this is a bit more complex to handle using cascade logic. We may be able to combine cascade and database triggers to achieve the desired outcome.
I tend to agree with the general sentiment that delegating this to database rules is more robust. However, to be pragmatic, I would propose that we leave as is in this PR and open an issue and tackle this in a follow-up PR. However, we may need to consider this before a production release as foreign relationships / cascade logic is immutable.
There was a problem hiding this comment.
Yeah let's do that in a separate PR. We don't necessarily need to do this before production release. Worst case, we can write a migration that copies old data to new tables and then deletes the old ones for deep structural DB changes.
| .await?; | ||
|
|
||
| models::batch_commit::Entity::update_many() | ||
| .filter(models::batch_commit::Column::Hash.is_in(batch_hashes.iter().cloned())) |
There was a problem hiding this comment.
Shouldn't the batch status also be set to Reverted here?
No description provided.