test: add test cases for handling l1 events and l1 reorg#428
test: add test cases for handling l1 events and l1 reorg#428
Conversation
CodSpeed Performance ReportMerging this PR will not alter performanceComparing Summary
|
* feat: add reboot to testFixture * feat: add reboot * feat: add reboot * feat: add more test * fix: db * fix: ci
200259b to
3db8192
Compare
* shutdown refactor * clean up * use unused ports of anvil and run tests concurrently
frisitano
left a comment
There was a problem hiding this comment.
Looks good! Added some comments inline.
| tracing::info!(target: "scroll::node::args", ?l1_block_startup_info, "Starting L1 watcher"); | ||
|
|
||
| #[cfg(feature = "test-utils")] | ||
| let skip_synced = self.test_args.test && self.test_args.skip_l1_synced; |
There was a problem hiding this comment.
Do we need both or can we just use self.test_args.skip_l1_synced?
| #[cfg_attr(not(feature = "test-utils"), allow(unused_mut))] | ||
| let (chain_orchestrator, mut handle) = ChainOrchestrator::new( |
There was a problem hiding this comment.
combine this suggestion with the one below.
| #[cfg_attr(not(feature = "test-utils"), allow(unused_mut))] | |
| let (chain_orchestrator, mut handle) = ChainOrchestrator::new( | |
| let (chain_orchestrator, handle) = ChainOrchestrator::new( |
| { | ||
| let command_rx = _l1_command_rx.map(|rx| Arc::new(tokio::sync::Mutex::new(rx))); | ||
| let l1_watcher_mock = rollup_node_watcher::test_utils::L1WatcherMock { | ||
| command_rx, | ||
| notification_tx: _l1_notification_tx.expect("L1 notification sender should be set"), | ||
| }; | ||
| handle = handle.with_l1_watcher_mock(Some(l1_watcher_mock)); | ||
| } |
There was a problem hiding this comment.
update
#[cfg(feature = "test-utils")]
let handle = {
let command_rx = _l1_command_rx.map(|rx| Arc::new(tokio::sync::Mutex::new(rx)));
let l1_watcher_mock = rollup_node_watcher::test_utils::L1WatcherMock {
command_rx,
notification_tx: _l1_notification_tx.expect("L1 notification sender should be set"),
};
handle.with_l1_watcher_mock(Some(l1_watcher_mock))
};
| new_status.l2.fcs.safe_block_info().number > initial_safe, | ||
| "Safe head should advance after L1Synced when processing buffered BatchCommit events" | ||
| ); | ||
|
|
There was a problem hiding this comment.
We should also add a check here to assert that the head block number has been updated in the database as well. get_l2_head_block_number. I believe there is a bug here because we don't update the head after consolidation. I suggest we add l2_head_updated to get:
/// The outcome of consolidating a batch with the L2 chain.
#[derive(Debug, Clone, PartialEq, Eq)]
pub struct BatchConsolidationOutcome {
/// The batch info for the consolidated batch.
pub batch_info: BatchInfo,
/// The consolidation outcomes for each block in the batch.
pub blocks: Vec<L2BlockInfoWithL1Messages>,
/// The list of skipped L1 messages index.
pub skipped_l1_messages: Vec<u64>,
/// The target status of the batch after consolidation.
pub target_status: BatchStatus,
/// Is the l2 head block number updated.
pub l2_head_updated: bool,
}
We set this to true if reorg_results.is_empty() == false:
rollup-node/crates/chain-orchestrator/src/consolidation.rs
Lines 97 to 117 in f396937
We then use this to set the head in the insert_batch_consolidation_outcome function:
async fn insert_batch_consolidation_outcome(
&self,
outcome: BatchConsolidationOutcome,
) -> Result<(), DatabaseError> {
self.insert_blocks(
outcome.blocks.iter().map(|b| b.block_info).collect(),
outcome.batch_info,
)
.await?;
self.update_l1_messages_with_l2_blocks(outcome.blocks).await?;
self.update_skipped_l1_messages(outcome.skipped_l1_messages).await?;
self.update_batch_status(outcome.batch_info.hash, outcome.target_status).await?;
if outcome.l2_head_updated {
// This is psuedocode - please fix it
self.set_l2_head_block_number(outcome.blocks.last().unwrap().number)
}
Ok(())
}
This PR adds test cases for handling l1 events and l1 reorg.
BatchCommit,BatchFinalizedandBatchRevertRangeevents in different sync mode.BatchCommit,BatchFinalizedandBatchRevertRangeevents.Fixes #420, #141