-
Notifications
You must be signed in to change notification settings - Fork 7
test: add test cases for handling l1 events and l1 reorg #428
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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))
};| /// This loads the file only once and keeps all transactions in memory for efficient access | ||
| /// throughout the test suite. The cache is organized as a nested map: | ||
| /// `tx_type -> index -> raw_bytes` | ||
| static TEST_TRANSACTIONS: LazyLock<HashMap<String, HashMap<String, Bytes>>> = LazyLock::new(|| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit:
| static TEST_TRANSACTIONS: LazyLock<HashMap<String, HashMap<String, Bytes>>> = LazyLock::new(|| { | |
| static TEST_TRANSACTIONS: LazyLock<HashMap<String, HashMap<usize, Bytes>>> = LazyLock::new(|| { |
| 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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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
| /// Consumes the reconciliation result and produces the consolidated chain by combining | |
| /// non-reorg block info with the reorg block results. | |
| pub(crate) async fn into_batch_consolidation_outcome( | |
| self, | |
| reorg_results: Vec<L2BlockInfoWithL1Messages>, | |
| ) -> Result<BatchConsolidationOutcome, ChainOrchestratorError> { | |
| let mut consolidate_chain = | |
| BatchConsolidationOutcome::new(self.batch_info, self.target_status); | |
| // First append all non-reorg results to the consolidated chain. | |
| self.actions.into_iter().filter(|action| !action.is_reorg()).for_each(|action| { | |
| consolidate_chain.push_block(action.into_block_info().expect("must have block info")); | |
| }); | |
| // Append the reorg results at the end of the consolidated chain. | |
| for block in reorg_results { | |
| consolidate_chain.push_block(block); | |
| } | |
| Ok(consolidate_chain) | |
| } |
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