op-node: managed mode pre-interop #16071
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## develop #16071 +/- ##
============================================
- Coverage 81.40% 45.79% -35.61%
============================================
Files 161 1313 +1152
Lines 8812 105791 +96979
============================================
+ Hits 7173 48444 +41271
- Misses 1506 53895 +52389
- Partials 133 3452 +3319
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
protolambda
force-pushed
the
op-node-managed-start
branch
from
0890f39 to
3c6f3ce
Compare
There was a problem hiding this comment.
LGTM. I think the question around how to setup challenger could be ignored for now - there's some separate work to fix the setup and actually call migrate so it's using a real interop system. We can then hopefully update the WithUnscheduledInterop to also skip calling migrate and it will be on the pre-interop fault proof system. We're not actually testing the fault proof system in devstack yet anyway so its fine as-is.
| t.Require().LessOrEqual(uint64(5), status.Chains[sys.L2ChainA.ChainID()].Finalized.Number, "must have synced A") | ||
| t.Require().LessOrEqual(uint64(5), status.Chains[sys.L2ChainB.ChainID()].Finalized.Number, "must have synced B") |
There was a problem hiding this comment.
I'm unclear why this is LessOrEqual - shouldn't we be expecting Finalized to be GreaterOrEqual than 5 (ie it should have progressed to at least 5 and it's possible it will have progressed further before this check actually runs since the chain is progressing in the background).
| interopTime := wb.outL2Genesis[id].Config.InteropTime | ||
| if interopTime == nil { | ||
| continue | ||
| } |
There was a problem hiding this comment.
Probably not for this PR but we should be able to get rid of this method and always use the dependency set from op-deployer. It now always generates one and there's no activation time anymore. I can take a look at that.
There was a problem hiding this comment.
Yep, can be removed: #16138
| @@ -82,7 +82,8 @@ func WithL2Challenger(challengerID stack.L2ChallengerID, l1ELID stack.L1ELNodeID | |||
|
|
|||
| dir := p.TempDir() | |||
| var cfg *config.Config | |||
| if interopScheduled { | |||
| // If interop is scheduled, or if we cannot do the pre-interop connection, then set up with supervisor | |||
| if interopScheduled || l2CLID == nil { | |||
There was a problem hiding this comment.
I'm not entirely sure this is the right framing. Really the decision of which way to configure things is based on which dispute game we're using (FaultDisputeGame vs SuperDisputeGame). It used to be that interopScheduled meant we would be using SuperDisputeGame but that's not necessarily the case.
That said, the protocol fork schedule and contract deployments aren't in sync right now. We're never calling migrate so the dispute games aren't shared, but I think we used addGameType somewhere to deploy the SuperDisputeGame. And the new test is only changing the rollup config to unschedule interop but not ensuring the contracts are setup for pre-interop.
I don't think that needs to block this landing but I think we've managed to make a mess of our test setup...
Description
Implements some simple changes to configure op-node in managed-mode before an interop activation time is set.
The test-setup needed some changes: to create the interop composition, without configuring interop itself.
Tests
Adds a new test that runs the interop-style infra configuration with managed mode and multiple chains, before any interop hardfork time is scheduled. And tests that it finalizes new L2 blocks.
Additional context
Metadata
Fix #15756