Skip to content

op-node/rollup/interop: do not ignore local unsafe reset #16052

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

Merged
merged 1 commit into from
Jun 3, 2025

Conversation

nonsense
Copy link
Contributor

@nonsense nonsense commented May 21, 2025

Description

This PR is:

  1. Fixing a bug with L2 unsafe head not reorging after a L1 reorg
  2. Fixing a config with deployment of interop network in sysgo tests -- SequencerConfDepth
  3. Fixing the wait times of a few tests due to the change in 2.

In managed-mode in interop networks, it is the op-supervisor that monitors the L1 chain, and is the source-of-truth for the networks view for the op-node. At the moment when it issues a reset, the op-node is ignoring the LocalUnsafe field within the reset event.

This means that if the L2 chain has progressed on an incorrect chain due to L1 reorg, its unsafe ref is not rewound back to a block using a valid L1 ref after the reorg.

Tests

This functionality is tested with TestL2ReorgAfterL1Reorg from #16105

cd op-acceptance-tests/tests/interop/reorgs
go test -count=1 -timeout=30m -failfast -v -run TestL2ReorgAfterL1Reorg ./...

If you comment out the update to LocalUnsafe field, the test will fail, and you can inspect the logs and see that L1Origin ref for the L2 chain blocks is stuck on a reorged L1 block.

I haven't added the test to this PR, as the op-test-sequencer L1-mode needs more work in order to be ready for review, but this bugfix and the test itself are ready.

Follow-up

In a separate PR, as a follow-up:

  • improve op-test-sequencer so that we can run the test multiple times. when running with -count=2, no reorg on L1 is triggered at the moment
  • use op-supervisor as source-of-truth for L1 origin selector (op-node.rollup.sequencing.L1OriginSelector)

Additional context

Found as part of #15331

Copy link

codecov bot commented May 21, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 96.24%. Comparing base (dc1cca7) to head (957c538).
Report is 5 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop   #16052       +/-   ##
============================================
+ Coverage    81.55%   96.24%   +14.68%     
============================================
  Files          161      106       -55     
  Lines         8866     4583     -4283     
============================================
- Hits          7231     4411     -2820     
+ Misses        1502      172     -1330     
+ Partials       133        0      -133     
Flag Coverage Δ
cannon-go-tests-64 ?
contracts-bedrock-tests 96.24% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

see 55 files with indirect coverage changes

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@nonsense nonsense force-pushed the nonsense/reset-local-unsafe branch 2 times, most recently from 07aed9d to 8b710d5 Compare May 21, 2025 17:40
@tynes tynes requested a review from Copilot May 21, 2025 17:41
Copy link
Contributor

@Copilot Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR fixes an issue in managed-mode interop where the op-node was ignoring the local unsafe field during a reset, which could lead to inconsistencies after L1 reorganizations.

  • Update the Reset method to retrieve the latest valid local unsafe block instead of using an empty reference.
  • Introduce a new function (scanL2ForLatestLocalUnsafe) to scan the L2 chain and validate the L1 origins.
  • Add new methods to the L2Source and L1Source interfaces to support block retrieval by label and by number.

@nonsense nonsense force-pushed the nonsense/reset-local-unsafe branch from 8b710d5 to 7b632f4 Compare May 21, 2025 18:33
@nonsense nonsense marked this pull request as ready for review May 21, 2025 18:46
@nonsense nonsense requested review from a team as code owners May 21, 2025 18:46
@teddyknox teddyknox moved this to In Progress in Protocol Team May 22, 2025
@nonsense nonsense force-pushed the nonsense/reset-local-unsafe branch from 7b632f4 to 590a420 Compare May 23, 2025 14:26
@nonsense nonsense requested a review from a team as a code owner May 23, 2025 14:26
@nonsense
Copy link
Contributor Author

Due to SequencerConfDepth: 2 change, a few tests' waits had to be bumped up with at least 2*blockTime (12sec. in tests).

@nonsense nonsense force-pushed the nonsense/reset-local-unsafe branch 7 times, most recently from 0b9e712 to 965af03 Compare May 30, 2025 13:29
@nonsense nonsense force-pushed the nonsense/reset-local-unsafe branch 3 times, most recently from c3360e3 to 8cc0668 Compare June 2, 2025 11:26
@nonsense nonsense force-pushed the nonsense/reset-local-unsafe branch from 8cc0668 to 957c538 Compare June 3, 2025 10:26
@github-project-automation github-project-automation bot moved this from In Progress to In Review in Protocol Team Jun 3, 2025
@nonsense nonsense added this pull request to the merge queue Jun 3, 2025
Merged via the queue into develop with commit 39dd9b8 Jun 3, 2025
60 checks passed
@nonsense nonsense deleted the nonsense/reset-local-unsafe branch June 3, 2025 15:48
@github-project-automation github-project-automation bot moved this from In Review to Done in Protocol Team Jun 3, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.

2 participants