Skip to content

Add Interop Upgrade Txs to op-node#15517

Merged
sebastianst merged 1 commit intodevelopfrom
interop-upgrade-txs
May 5, 2025
Merged

Add Interop Upgrade Txs to op-node#15517
sebastianst merged 1 commit intodevelopfrom
interop-upgrade-txs

Conversation

@teddyknox
Copy link
Contributor

@teddyknox teddyknox commented Apr 22, 2025

Description

Adds interop upgrade txs in accordance to ethereum-optimism/specs#672

Necessary second-order changes:

  • Update the op-chain-ops/interopgen test framework to support activating interop after network setup rather than at genesis.
  • Adding ActBuildL2ToInterop() to L2Sequencer
  • actors.PrepareChainState(t) has been renamed to actors.PrepareAndVerifyInitialState(t), and logic within that function has been factored into two new functions:
    • actors.PrepareChainState(t), which performs the preparation steps (adopting the previous name of actors.PrepareAndVerifyInitialState(t))
    • actors.VerifyInitialState(t), which verifies that the chain has been setup correctly assuming a genesis timestamp interop activation.
      This makes it possible to use the logic in the new actors.PrepareChainState(t) function without the verify step, which hardcodes the genesis timestamp activation assumption.

Tests

  • Adds TestInteropUpgradeTransactions in op-e2e/actions/upgrades/interop_fork_test.go which verifies that the transactions match the spec.
  • Adds TestInteropUpgradeTransactions in op-e2e/actions/upgrades/interop_fork_test.go which verifies that the upgrade transactions are applied appropriately.

@teddyknox teddyknox requested review from a team as code owners April 22, 2025 21:48
@teddyknox teddyknox requested a review from ajsutton April 22, 2025 21:48
@codecov
Copy link

codecov bot commented Apr 22, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 92.35%. Comparing base (92cb5cd) to head (ab7e3b0).
Report is 4 commits behind head on develop.

Additional details and impacted files
@@             Coverage Diff              @@
##           develop   #15517       +/-   ##
============================================
+ Coverage    46.10%   92.35%   +46.25%     
============================================
  Files         1281      122     -1159     
  Lines       105461     5850    -99611     
============================================
- Hits         48626     5403    -43223     
+ Misses       53332      435    -52897     
+ Partials      3503       12     -3491     
Flag Coverage Δ
cannon-go-tests-64 ?
contracts-bedrock-tests 95.26% <ø> (ø)

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

see 1162 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.

@tynes tynes added the H-interop Hardfork: change planned for interop upgrade label Apr 22, 2025
@tynes tynes added this to the Interop RC Beta milestone Apr 22, 2025
@teddyknox teddyknox requested a review from sebastianst April 23, 2025 18:18
@sebastianst sebastianst self-assigned this Apr 23, 2025
@teddyknox teddyknox force-pushed the interop-upgrade-txs branch from 9d786a9 to 443af90 Compare April 29, 2025 18:12
Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

It seems to match the specs (but using the correct updateTo call argument). But there's only so much we can verify statically. We need an end2end test of the transition block, ideally an actions/proof test, to verify that the upgrade transactions work as intended.

@teddyknox teddyknox force-pushed the interop-upgrade-txs branch from 443af90 to 6c1afd2 Compare April 29, 2025 21:39
@teddyknox teddyknox requested a review from sebastianst April 29, 2025 21:40
@teddyknox teddyknox force-pushed the interop-upgrade-txs branch from 6c1afd2 to c719370 Compare April 30, 2025 20:31
@teddyknox teddyknox requested a review from a team as a code owner April 30, 2025 20:31
@teddyknox teddyknox requested a review from tynes April 30, 2025 20:57
@teddyknox teddyknox force-pushed the interop-upgrade-txs branch from c719370 to c6fd3b7 Compare May 1, 2025 03:42
@teddyknox teddyknox requested a review from sebastianst May 1, 2025 03:45
@teddyknox teddyknox force-pushed the interop-upgrade-txs branch from c6fd3b7 to ac81069 Compare May 2, 2025 03:56
Copy link
Contributor

@geoknee geoknee left a comment

Choose a reason for hiding this comment

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

Update all previous calls to VerifyInitialState() to also feature a call to actors.PrepareChainState(t).
Did you mean to say "update all calls to prepare with a call to verify"?
Tests

Adds TestInteropUpgradeTransactions in op-e2e/actions/upgrades/interop_fork_test.go which verifies that the transactions match the spec.
Adds TestInteropUpgradeTransactions in op-e2e/actions/upgrades/interop_fork_test.go which verifies that the upgrade transactions are applied appropriately.

Nit: this doesn't seem accurate. NBD but it can cause some confusion.

@teddyknox teddyknox force-pushed the interop-upgrade-txs branch 2 times, most recently from 7aa1ff8 to a58d566 Compare May 5, 2025 12:12
@teddyknox
Copy link
Contributor Author

@geoknee thanks for the thorough review.

Nit: this doesn't seem accurate. NBD but it can cause some confusion.

I updated the description to be more accurate. Apologies, it had fallen out of date.

Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

One critical check should be changed at the end of the action test, then we can merge it and work on remaining open discussions in the follow-up PR.

Remove rescheduled upgrade transactions
@teddyknox teddyknox force-pushed the interop-upgrade-txs branch from a58d566 to ab7e3b0 Compare May 5, 2025 13:16
Copy link
Member

@sebastianst sebastianst left a comment

Choose a reason for hiding this comment

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

Approving and merging to get this in, but all open comments need to be addressed in a follow-up soon. I've resolved all open comments with the comment "TODO in follow-up".

@sebastianst sebastianst added this pull request to the merge queue May 5, 2025
Merged via the queue into develop with commit 12cb0c5 May 5, 2025
53 checks passed
@sebastianst sebastianst deleted the interop-upgrade-txs branch May 5, 2025 13:48
iquidus pushed a commit to Layr-Labs/optimism that referenced this pull request Jul 24, 2025
Remove rescheduled upgrade transactions
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

H-interop Hardfork: change planned for interop upgrade

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants