op-node/rollup/derive: Implement upgrade gas & upgrade block user tx omission#14797
op-node/rollup/derive: Implement upgrade gas & upgrade block user tx omission#14797sebastianst wants to merge 2 commits intodevelopfrom
Conversation
sebastianst
requested review from
ControlCplusControlV,
ajsutton,
bitwiseguy and
protolambda
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## develop #14797 +/- ##
============================================
+ Coverage 46.95% 81.71% +34.75%
============================================
Files 1049 174 -875
Lines 90854 10165 -80689
============================================
- Hits 42660 8306 -34354
+ Misses 45063 1691 -43372
+ Partials 3131 168 -2963
Flags with carried forward coverage won't be shown. Click here to find out more. 🚀 New features to boost your workflow:
|
| } | ||
|
|
||
| func (s *L2Sequencer) ActBuildL2ToFork(t Testing, fork rollup.ForkName) { | ||
| // TODO: add check that fork is set in rollup config |
There was a problem hiding this comment.
We should either do this or reference an issue for it. Probably a simple require could be added here since we have t already.
| dp.DeployConfig.ActivateForkAtGenesis(rollup.Holocene) | ||
| case Isthmus: | ||
| // Isthmus usually runs on a Prague L1 network | ||
| dp.DeployConfig.L1PragueTimeOffset = &genesisBlock |
There was a problem hiding this comment.
Is there a helper method we already have that would ensure all the Hardforks before Prague are also enabled?
There was a problem hiding this comment.
L1 Cancun is enabled by default. So that's currently the only L1 fork that needs to be enabled manually.
| Name: "transactions in Interop upgrade block", | ||
| L1Blocks: []eth.L1BlockRef{l1A, l1B, l1C}, |
There was a problem hiding this comment.
I wonder if we can create these automatically so we pick up new forks. We have the list of hard forks already and we could iterate through them, then for each check that they forbid user transactions unless they're in a list that explicitly allows it. Then if we add a new fork, as long as we add it to the list of forks this test will remind us to update the if statement in batches.go and prevent user transactions (or add it to the allow list if it doesn't have upgrade tx). Otherwise I can see us forgetting that.
There was a problem hiding this comment.
Good idea, I was thinking of actually running the whole upgrade block action tests for all forks from Isthmus. But then, as you can see with the commented out code, it just failed for Interop, so I didn't follow up with this idea. Will do this in the batches tests and track it for Interop.
| ) //.AddDefaultTestCases( | ||
| // upgradeBlockTestCfg{fork: rollup.Interop, numUpgradeTxs: 0}, | ||
| // helpers.NewForkMatrix(helpers.Isthmus), | ||
| // testUpgradeBlock, | ||
| // ) |
There was a problem hiding this comment.
nit: Can probably remove the commented out code.
| ) //.AddDefaultTestCases( | ||
| // upgradeBlockTestCfg{fork: rollup.Interop, numUpgradeTxs: 0}, | ||
| // helpers.NewForkMatrix(helpers.Isthmus), | ||
| // testUpgradeBlockTxOmission, | ||
| // ) |
There was a problem hiding this comment.
nit: Uncomment or remove.
| }, | ||
| Expected: BatchDrop, // dropped because it could have advanced the epoch to B | ||
| }, | ||
| { |
There was a problem hiding this comment.
Haven't looked at all batches tests, but in case you don't, I think it'd be worthwhile to add a test here that covers the happy path when there are no txs in the activation block to confirm this still works as intended.
|
Moved back into draft as we'll probably move this out of Isthmus. |
sebastianst
changed the title
|
There is general consensus that this is not going into isthmus |
opgitgovernance
added
the
S-stale
|
This pr has been automatically marked as stale and will be closed in 5 days if no updates |
|
We ultimately decided to not take this approach |
There was a problem hiding this comment.
Commenting here to have an easy to link for "how to increase gas for a single deposit block", as discussed in the L2 upgrades design doc.
6 participants
Description
This PR implements Upgrade Gas and the omission of user transactions in upgrade blocks per the specs at ethereum-optimism/specs#616
Tests
Added proofs action test of both features, plus unit tests for invalid batches. The omission of user tx by the sequencer is also covered by the action test.
Additional context
Metadata
Closes #14649