contracts-bedrock: remove batchInbox and startBlock from SystemConfig#21641
contracts-bedrock: remove batchInbox and startBlock from SystemConfig#21641mds1 wants to merge 3 commits into
Conversation
Nothing in the OP Stack reads the batch inbox or start block from the SystemConfig contract - the rollup config and superchain registry are the source of truth - and U19 showed the onchain copy can silently drift (OPCMv2's reinit path rotated the batch inbox for pre-OPCM chains). Remove both getters and clear the legacy storage slots during reinitialization instead of maintaining a redundant copy. - Remove batchInbox()/startBlock(), the _batchInbox initializer param, _setStartBlock(), and the public slot constants; bump SystemConfig to 4.0.0 and init version to 4; clear both legacy slots in initialize - Drop the batch inbox computation from OPCMv2 and the preserve-read from the Migrator; remove the unused _chainIdToBatchInboxAddress helper - Remove ChainAssertions CHECK-SCFG-140/150/360/370 - op-node genesis l2 now resolves the L1 start block from the deploy config's l1StartingBlockTag instead of SystemConfig.startBlock() Closes #21614 Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
|
Preview deployment for your docs. Learn more about Mintlify Previews.
|
|
|
||
| // Clear the legacy batch inbox address and start block slots. These values are unused | ||
| // by the OP Stack, which reads them from the rollup configuration instead. Clearing | ||
| // removes a redundant source of truth that could otherwise drift from the real values. | ||
| Storage.setAddress(LEGACY_BATCH_INBOX_SLOT, address(0)); | ||
| Storage.setUint(LEGACY_START_BLOCK_SLOT, 0); |
There was a problem hiding this comment.
Non-Idempotent Initializer — Acknowledgment Required
This initialize() function contains operations that are not idempotent (not safe to call multiple times with the same arguments). The function now unconditionally clears the legacy batch inbox and start block slots:
Storage.setAddress(LEGACY_BATCH_INBOX_SLOT, address(0));
Storage.setUint(LEGACY_START_BLOCK_SLOT, 0);While clearing to zero is itself idempotent (calling it multiple times produces the same result of zero), the broader concern is that if a proxy is re-initialized during an upgrade, any value that was legitimately stored in these slots by a previous version of the contract will be permanently destroyed. This is a one-way destructive operation on existing on-chain state.
The PR description acknowledges this is intentional ("so the stale values are zeroed"), but the initialize() function itself lacks a @notice comment explaining why this non-idempotent-with-respect-to-prior-state behavior is safe.
Please either:
- Make the operation idempotent (e.g., only clear if the value is non-zero, though this is already effectively idempotent), or
- Add a
@noticecomment on theinitialize()function explaining why clearing these legacy slots is safe — specifically that the OP Stack does not read these values from SystemConfig and that clearing them is intentional cleanup of a redundant source of truth.
See docs/ai/contract-dev.md for detailed guidance.
| // Clear the legacy batch inbox address and start block slots. These values are unused | |
| // by the OP Stack, which reads them from the rollup configuration instead. Clearing | |
| // removes a redundant source of truth that could otherwise drift from the real values. | |
| Storage.setAddress(LEGACY_BATCH_INBOX_SLOT, address(0)); | |
| Storage.setUint(LEGACY_START_BLOCK_SLOT, 0); | |
| // Clear the legacy batch inbox address and start block slots. These values are unused | |
| // by the OP Stack, which reads them from the rollup configuration instead. Clearing | |
| // removes a redundant source of truth that could otherwise drift from the real values. | |
| // NOTE: This is an intentional one-way cleanup of stale on-chain state. It is safe to | |
| // perform because the OP Stack never reads these values from SystemConfig at runtime — | |
| // they are always sourced from the rollup node's configuration. Zeroing them here | |
| // prevents any future code from accidentally relying on potentially stale values. | |
| Storage.setAddress(LEGACY_BATCH_INBOX_SLOT, address(0)); | |
| Storage.setUint(LEGACY_START_BLOCK_SLOT, 0); | |
Spotted by Graphite (based on custom rule: Monorepo Graphite Rules)
Is this helpful? React 👍 or 👎 to let us know.
Patch bump required by the semver-diff check for the batch inbox computation removal. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
During U19, OPCMv2's reinit-everything path recomputed the SystemConfig batch inbox address via
_chainIdToBatchInboxAddressinstead of preserving the existing value, rotating the onchain batch inbox for all pre-OPCM chains (OP, Unichain, Mode, Metal, Zora). This was harmless because nothing in the OP Stack reads the batch inbox from the SystemConfig — derivation reads it from the rollup config — but it showed the onchain copy is a redundant source of truth that can silently drift. Rather than fix the reinit path to preserve the value, this PR removes the redundant copy entirely. Closes #21614.Changes:
SystemConfig.batchInbox()andstartBlock()getters are removed, along with the_batchInboxinitializer param,_setStartBlock(), and the public slot constants. Semver 3.14.2 → 4.0.0, init version 3 → 4.initializenow clears both legacy slots (keccak256("systemconfig.batchinbox") - 1andkeccak256("systemconfig.startBlock") - 1), so the stale values are zeroed for every chain at the next upgrade.OPContractsManagerV2._makeSystemConfigInitArgsno longer computes a batch inbox (the exact line that caused the U19 rotation), andOPContractsManagerMigratorno longer reads and re-passes one. The now-unused_chainIdToBatchInboxAddresshelper is removed fromOPContractsManagerUtilsCaller.OPContractsManagerUtils.chainIdToBatchInboxAddressstays — op-deployer still derives new-chain batch inboxes with the same convention. OPCMv2 gets the dev-work patch bump to 7.1.24.ChainAssertionschecks CHECK-SCFG-140/150 (proxy startBlock/batchInbox) and CHECK-SCFG-360/370 (impl startBlock/batchInbox) are removed.op-node genesis l2was the only OP Stack reader ofstartBlock(). It now resolves the L1 start block from the deploy config'sl1StartingBlockTagand errors if unset. Without this, the cleared slot would have silently anchored new genesis files at block 0. Worth noting the behavioral shift: regenerating an existing chain's genesis now requires the deploy config'sl1StartingBlockTagto be set to the chain's historical start block, since the contract no longer records it.Why removal is safe: I audited the monorepo (Go, Rust, Solidity), superchain-registry, superchain-ops, op-geth, monitorism, security-tools, op-txverify, ecosystem (
@eth-optimism/viem), specs, and docs, plus a GitHub code search across the ethereum-optimism org.batchInbox()has zero readers anywhere.startBlock()'s only reader was the CLI fixed in (5). op-deployer computes the batch inbox locally (calculateBatchInboxAddr) and the registry'sbatch_inbox_addrcomes from the deploy config — neither reads the contract.Note for third parties: reading
batchInbox()from the SystemConfig stops working at op-contracts v8.0.0. The batch inbox address remains available in the superchain registry and in each chain's rollup config, which are the values the OP Stack actually uses.Also updates the dev docs data glossary in
docs/public-docsto source the batch inbox from the superchain registry.Related PRs: ethereum-optimism/specs#922 (spec), ethereum-optimism/superchain-ops#1492 (upgrade tooling), ethereum-optimism/op-analytics#1799 (analytics ingest).