feat(op-acceptance-tests): l2cm upgrade accaptance tests and fork tests on karst betanet#20668
Conversation
…imal upgrade with l2cm
… and add flags to support running against betanet
| // Skip if not L2 fork test | ||
| skipIfNotL2ForkTest("L2ForkUpgrade: not a fork test"); | ||
|
|
||
| // Skip if L2CM dev feature is not enabled |
There was a problem hiding this comment.
Shouldn't we also check for KARST_BETANET_L2_FORK_TEST before skipping L2 Forked tests? Setting KARST_BETANET_L2_FORK_TEST=true would still just silently skip these.
There was a problem hiding this comment.
wouldn't the L2CM feature flag be true on the karst betanet?
There was a problem hiding this comment.
Sorry, I meant the check above: skipIfNotL2ForkTest("L2ForkUpgrade: not a fork test");
There was a problem hiding this comment.
I see. Probably better to adjust the justfile so L2_FORK_TEST=true is set too for the test-karst-betanet-l2-fork-upgrade task? There are quite a few places were we rely on l2ForkTest / isL2ForkTest.
There was a problem hiding this comment.
actually it's already done, because test-karst-betanet-l2-fork-upgrade starts with prepare-l2-upgrade-env which does set L2_FORK_TEST. I removed a redundant check here 1971c09
| upgradeGas += nutGas | ||
| } | ||
|
|
||
| if ba.rollupCfg.IsCustomNUTActivationBlock(nextL2Time) { |
There was a problem hiding this comment.
I don't think we should add test-only code in here.
| function _executeCurrentBundle() internal virtual { | ||
| PastNUTBundles.ForkWrappers memory w = PastNUTBundles.wrappersForFork(currentFork); | ||
| PastNUTBundles.executeWithWrappers(executeScript, w.pre, _currentBundleTxns(), w.post); | ||
| if (!isKarstForkActivated()) { |
There was a problem hiding this comment.
Why do we need to skip if Karst is activated? In post-karst upgrades we still want to take chains that are in an "older" state up-to-date so we can execute the NUTs in the current bundle.
|
/ci authorize da9943e |
|
/ci authorize 331ce17 |
|
/ci authorize 96f15bc |
|
|
||
| /// @notice Executes the current generated NUT bundle with any fork-specific wrappers. | ||
| function _executeCurrentBundle() internal virtual { | ||
| PastNUTBundles.ForkWrappers memory w = PastNUTBundles.wrappersForFork(currentFork); |
There was a problem hiding this comment.
I think I see this more clearly here, it should be able to skip the current bundle if it's already applied not only if karst is applied:
if (_isCurrentBundleAlreadyApplied()) return;
…rrent bundle is applied
…with before/after blocknumbers and capture events from the activation block
| return vm.envString("KARST_BETANET_L2_FORK_RPC_URL"); | ||
| /// @notice Returns the L2 block after the fork. | ||
| function l2BlockAfterFork() internal view returns (uint256) { | ||
| return vm.envOr("L2_FORK_BLOCK_NUMBER", uint256(0)); |
There was a problem hiding this comment.
I would think it should be an error to either attempt to use this on a non-activation test or try to use it during a activation test while it is 0. wdyt?
|
|
||
| test-karst-betanet-l2-fork-upgrade-rerun *ARGS: | ||
| test-post-karst-betanet-l2-fork-upgrade-rerun *ARGS: | ||
| just test-karst-betanet-l2-fork-upgrade {{ARGS}} --rerun -vvvv |
There was a problem hiding this comment.
Shouldn't this be also updated to match test-post-karst-betanet-l2-fork-upgrade?
…terFork revert outside l2CMActivationTest
| /// @notice Returns the L2 block after the fork. | ||
| function l2BlockAfterFork() internal view returns (uint256) { | ||
| if (l2CMActivationTest()) { | ||
| return vm.envOr("L2_FORK_BLOCK_NUMBER", uint256(0)); |
There was a problem hiding this comment.
I think this needs to be fixed before merge.
The justfile documents L2_BLOCK_AFTER_FORK, but this reads L2_FORK_BLOCK_NUMBER and defaults to 0. I assume 0 is meant to preserve the existing "use latest" behaviour from the normal L2 fork setup. But activation mode passes this value directly to createSelectFork(url, block), so following the documented command makes the post upgrade fork select explicit block 0, not latest or the intended after fork block.
Could we do one of the following:
- Read a mandatory
L2_BLOCK_AFTER_FORK - Keep 0 as latest and have
_executeCurrentBundleOrSwitchFork()callcreateSelectFork(url)overload when this returns 0?
The pre/post activation flow can become:
- Fork at
L2_BLOCK_BEFORE_FORK - Capture pre upgrade state
- Fork at block 0
- Verify post upgrade state
…after activation block number
| address(0), | ||
| topics | ||
| ); | ||
| logs = _ethGetLogsToLogs(ethLogs); |
There was a problem hiding this comment.
Currently the L2CM activation path only checks the historical Upgraded(address) logs. That doesn't prove the activation block contained the expected NUT bundle txns.
Can we also assert that the activation block contains the expected activation bundle txns? Otherwise missing, reordered, or corrupted upgrade txns could still be missed if the expected upgrade logs are present.
|
/ci authorize e05e520 |
Description
Ability to run fork tests on karst betanet, add acceptance tests for withdrawal. L2 Fork test for checking the existence of the deterministic-deployment-proxy.
Tests
mise exec -- just test -run TestWithdrawal_Karst ./op-acceptance-tests/tests/karst/Running L2ForkUpgrade.t.sol with a betanet
Additional Context
Setting up a karst betanet is required for running the tests this way.
Metadata
fixes #18870