Skip to content

Conversation

@yiweichi
Copy link
Member

@yiweichi yiweichi commented Nov 18, 2025

This PR adds test cases for handling l1 events and l1 reorg.

  • added test cases to test the behavior of RN when handling BatchCommit, BatchFinalized and BatchRevertRange events in different sync mode.
  • added test cases to test the behavior of RN when handling L1 block reorg with BatchCommit, BatchFinalized and BatchRevertRange events.

Fixes #420, #141

@codspeed-hq
Copy link

codspeed-hq bot commented Nov 18, 2025

CodSpeed Performance Report

Merging this PR will not alter performance

Comparing morty-add-anvil-to-test-fixture (f396937) with main (c00bd01)

Summary

✅ 2 untouched benchmarks

@yiweichi yiweichi force-pushed the morty-add-anvil-to-test-fixture branch from 200259b to 3db8192 Compare January 6, 2026 01:35
Copy link
Collaborator

@frisitano frisitano left a 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;
Copy link
Collaborator

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?

Comment on lines +476 to +477
#[cfg_attr(not(feature = "test-utils"), allow(unused_mut))]
let (chain_orchestrator, mut handle) = ChainOrchestrator::new(
Copy link
Collaborator

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.

Suggested change
#[cfg_attr(not(feature = "test-utils"), allow(unused_mut))]
let (chain_orchestrator, mut handle) = ChainOrchestrator::new(
let (chain_orchestrator, handle) = ChainOrchestrator::new(

Comment on lines +493 to +500
{
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));
}
Copy link
Collaborator

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(|| {
Copy link
Collaborator

Choose a reason for hiding this comment

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

nit:

Suggested change
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"
);

Copy link
Collaborator

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:

/// 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(())
    }

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Testing] Test multi-mode L1

5 participants