contracts-bedrock: Resolve shared contract pause state via ETHLockbox - #22095
contracts-bedrock: Resolve shared contract pause state via ETHLockbox#22095ajsutton wants to merge 1 commit into
Conversation
64ebb50 to
98a5a02
Compare
98a5a02 to
0973cab
Compare
| /// shared by every chain authorized on that lockbox. | ||
| interface IPauseSource { | ||
| function paused() external view returns (bool); | ||
| function guardian() external view returns (address); |
There was a problem hiding this comment.
AFAICT the ETHLockbox doesn't have guardian(), and I don't think it's needed on this interface.
There was a problem hiding this comment.
Oh, I see that is being added in this PR.
| IPauseSource pauseSource = | ||
| isEthLockboxEnabled ? IPauseSource(address(_cts.ethLockbox)) : IPauseSource(address(_cts.systemConfig)); |
There was a problem hiding this comment.
This is set on the AnchorStateRegistry and DelayedWETH below, but not other pausable contracts like OptimismPortal2, L1CrossDomainMessenger, and L1StandardBridge.
There was a problem hiding this comment.
Correct, non-shared contracts continue to use the SystemConfig because they're always chain specific. I think the ideal case for simplifying things would be to enable the lockbox on all chains, then all chains would use the lockbox as their pause source. I'm already worried this is more work than I can actually commit to and should be planned - that expanded scope definitely should be planned.
| ISystemConfig public systemConfig; | ||
| /// @notice The address of the SuperchainConfig contract. The lockbox is its own pause | ||
| /// identifier, so every contract that resolves its pause state through this lockbox | ||
| /// shares one pause state, regardless of how many chains are authorized on it. | ||
| ISuperchainConfig public superchainConfig; |
There was a problem hiding this comment.
I can't think of what could go wrong, but I think we should follow our convention of having a bytes spacer_slot_offset_length here and adding the new variable to the end of the layout.
There was a problem hiding this comment.
It looks like the implementation of SystemConfig.paused() has a lot of complexity in it that could also be removed here.
Fixes #22093.
The shared
ETHLockbox,AnchorStateRegistryandDelayedWETHof an interop set each stored a pointer to chain 0'sSystemConfig— a per-chain contract — and used it forpaused(),guardian()andsuperchainConfig(). That makes one member the governance root of the whole set, and it makes per-chainupgrade()non-idempotent:_upgradeChainre-initialises all three with_cts.systemConfig, so upgrading chain N silently repoints the set at chain N, and the validator's_asr.systemConfig() == _sysCfgpredicate can only hold for one member at a time. That is a direct obstacle to the "all chains always upgrade the shared components" resolution in #21731.Each contract now points at an
IPauseSource—paused()/guardian()/superchainConfig()— whose scope matches its own:ETHLockboxholds theSuperchainConfigdirectly,paused()keys onaddress(this), and it gains aguardian()passthrough. This also collapses the old 4-hop pause lookup (lockbox → systemConfig → portal → its own address → superchainConfig) to one call on theunlockETHpath.AnchorStateRegistryandDelayedWETHhold anIPauseSource: the sharedETHLockboxwhen the chain uses one, otherwise the chain's ownSystemConfig. Both satisfy the interface, so chains without a lockbox keep exactly today's behaviour.migrate()now also repoints the reused chain-0DelayedWETHat the shared lockbox, andsetInteropDisputeGames()re-derives the pause source from the portal's lockbox, so a set migrated by an older release is fixed up there._asr.pauseSource() == expectedPauseSource(sysCfg), which is true for every member of a migrated set simultaneously.ETHLockbox1.3.1 → 2.0.0,AnchorStateRegistry3.9.0 → 4.0.0 andDelayedWETH1.5.1 → 2.0.0 are major bumps: thesystemConfig()getter is gone, andinitializekeeps its selector (ISystemConfig,ISuperchainConfigandIPauseSourceall encode asaddress) while the meaning of its first argument changes. Anything calling these initializers must be updated deliberately.Storage-layout neutral: the field is a plain 20-byte address slot in all three contracts, so only the label and type change in
snapshots/storageLayout.Two migration-validator tests gained extra expected error codes: with the lockbox missing or
ETH_LOCKBOXdisabled, the pause-source checks now also fire. That is intended — a shared registry pointing at a lockbox the chain does not acknowledge is a real misconfiguration.Not addressed here: the shared proxies are still administered by chain 0's
ProxyAdmin, tracked in #21731.Test plan
just test-devgreen in three configurations: default,DEV_FEATURE__OPTIMISM_PORTAL_INTEROP+DEV_FEATURE__ZK_DISPUTE_GAME, andSYS_FEATURE__CUSTOM_GAS_TOKEN.test_migrate_sharedContractsUseLockboxAsPauseSource_succeedsasserts that aftermigrate()the shared ASR and DelayedWETH point at the shared lockbox, that both chains resolve that same lockbox, and that pausing it pauses both chains at once.