Merge pull request #7555 from ethereum-optimism/fix/ctb-sys-config

contracts-bedrock: SystemConfig reinit fix

packages/contracts-bedrock/.gas-snapshot

@@ -661,11 +661,11 @@ StandardBridge_Stateless_Test:test_isOptimismMintableERC20_succeeds() (gas: 3307
 Storage_Roundtrip_Test:test_setGetAddress_succeeds(bytes32,address) (runs: 64, μ: 31799, ~: 31799)
 Storage_Roundtrip_Test:test_setGetBytes32_succeeds(bytes32,bytes32) (runs: 64, μ: 31643, ~: 31643)
 Storage_Roundtrip_Test:test_setGetUint_succeeds(bytes32,uint256) (runs: 64, μ: 30998, ~: 31620)
-SystemConfig_Initialize_Test:test_initialize_events_succeeds() (gas: 71950)
-SystemConfig_Initialize_Test:test_initialize_startBlockOverride_succeeds() (gas: 65132)
-SystemConfig_Initialize_Test:test_initialize_values_succeeds() (gas: 64946)
+SystemConfig_Initialize_Test:test_initialize_events_succeeds() (gas: 71972)
+SystemConfig_Initialize_Test:test_initialize_startBlockNoop_reverts() (gas: 81247)
+SystemConfig_Initialize_Test:test_initialize_startBlockOverride_succeeds() (gas: 65143)
+SystemConfig_Initialize_Test:test_initialize_values_succeeds() (gas: 64968)
 SystemConfig_Initialize_TestFail:test_initialize_lowGasLimit_reverts() (gas: 55589)
-SystemConfig_Initialize_TestFail:test_initialize_startBlock_reverts() (gas: 78214)
 SystemConfig_Setters_TestFail:test_setBatcherHash_notOwner_reverts() (gas: 15607)
 SystemConfig_Setters_TestFail:test_setGasConfig_notOwner_reverts() (gas: 15577)
 SystemConfig_Setters_TestFail:test_setGasLimit_notOwner_reverts() (gas: 15676)

packages/contracts-bedrock/semver-lock.json

@@ -8,7 +8,7 @@
   "src/L1/L2OutputOracle.sol": "0x1cfa6e41f3449896d2b0f5fba85d759acefe7012d6bffbcdf0bba883a78b50ee",
   "src/L1/OptimismPortal.sol": "0xc4753409d6aca7f7b8a48b9a8ac9542a5c5e0b7e7cb71ca3b6b35dd714806c73",
   "src/L1/ProtocolVersions.sol": "0xfc92dfc0e4518470c48c74fa9c5b15347da2a791273afd1968ac3eada7c45dd6",
-  "src/L1/SystemConfig.sol": "0x9ebc7ab20ea65301a477f45169225ac2141c7be9654102befcd448aaab271c0c",
+  "src/L1/SystemConfig.sol": "0x9b1c9225283d4944ad722d11d5650a0d84809156cfb1bcab8a7288bc3e5f247f",
   "src/L2/BaseFeeVault.sol": "0xc347c1aebe69178e72d2b1d3e700bbf84e39975319465bb85d69fd0d60fc1759",
   "src/L2/GasPriceOracle.sol": "0x88efffbd40f8d012d700a5d7fde0d92266f65e9d7006cd8f034bacaa036d0eb2",
   "src/L2/L1Block.sol": "0x1ed9aa36036ded00a0383692eca81a22f668d64e22af973559d2ccefc86825c0",

packages/contracts-bedrock/src/L1/SystemConfig.sol

@@ -101,8 +101,8 @@ contract SystemConfig is OwnableUpgradeable, ISemver {
     uint256 public startBlock;
 
     /// @notice Semantic version.
-    /// @custom:semver 1.9.0
-    string public constant version = "1.9.0";
+    /// @custom:semver 1.10.0
+    string public constant version = "1.10.0";
 
     /// @notice Constructs the SystemConfig contract. Cannot set
     ///         the owner to `address(0)` due to the Ownable contract's
@@ -249,18 +249,18 @@ contract SystemConfig is OwnableUpgradeable, ISemver {
     ///         can have their start block set by a user provided override.
     ///         A start block of 0 indicates that there is no override and the
     ///         start block will be set by `block.number`.
-    /// @dev    This logic is used to patch legacy with new storage values. In the
-    ///         next version, it should remove the override and set the start block
-    ///         to `block.number` if the value in storage is 0. This will allow it
-    ///         to be reinitialized again and also work for fresh deployments.
+    /// @dev    This logic is used to patch legacy deployments with new storage values.
+    ///         Use the override if it is provided as a non zero value and the value
+    ///         has not already been set in storage. Use `block.number` if the value
+    ///         has already been set in storage
     /// @param  _startBlock The start block override to set in storage.
     function _setStartBlock(uint256 _startBlock) internal {
-        require(startBlock == 0, "SystemConfig: cannot override an already set start block");
-        if (_startBlock != 0) {
-            // There is an override, it cannot already be set.
+        if (_startBlock != 0 && startBlock == 0) {
+            // There is an override and it is not already set, this is for legacy chains.
             startBlock = _startBlock;
-        } else {
+        } else if (startBlock == 0) {
             // There is no override and it is not set in storage. Set it to the block number.
+            // This is for newly deployed chains.
             startBlock = block.number;
         }
     }

packages/contracts-bedrock/test/SystemConfig.t.sol

@@ -141,6 +141,49 @@ contract SystemConfig_Initialize_Test is SystemConfig_Init {
         assertEq(sysConf.startBlock(), startBlock);
     }
 
+    /// @dev Tests that initialization with start block already set is a noop.
+    function test_initialize_startBlockNoop_reverts() external {
+        // wipe out initialized slot so we can initialize again
+        vm.store(address(sysConf), bytes32(0), bytes32(0));
+        // the startBlock slot is 106, set it to something non zero
+        vm.store(address(sysConf), bytes32(uint256(106)), bytes32(uint256(0xff)));
+
+        // Initialize with a non zero start block, should see a revert
+        vm.prank(multisig);
+        // The call to initialize reverts due to: "SystemConfig: cannot override an already set start block"
+        // but the proxy revert message bubbles up.
+        Proxy(payable(address(sysConf))).upgradeToAndCall(
+            address(systemConfigImpl),
+            abi.encodeCall(
+                SystemConfig.initialize,
+                (
+                    alice, // _owner,
+                    overhead, // _overhead,
+                    scalar, // _scalar,
+                    batcherHash, // _batcherHash
+                    gasLimit, // _gasLimit,
+                    unsafeBlockSigner, // _unsafeBlockSigner,
+                    Constants.DEFAULT_RESOURCE_CONFIG(), // _config,
+                    1, // _startBlock
+                    batchInbox, // _batchInbox
+                    SystemConfig.Addresses({ // _addresses
+                        l1CrossDomainMessenger: l1CrossDomainMessenger,
+                        l1ERC721Bridge: l1ERC721Bridge,
+                        l1StandardBridge: l1StandardBridge,
+                        l2OutputOracle: l2OutputOracle,
+                        optimismPortal: optimismPortal,
+                        optimismMintableERC20Factory: optimismMintableERC20Factory
+                    })
+                )
+            )
+        );
+
+        // It was initialized with 1 but it was already set so the override
+        // should be ignored.
+        uint256 startBlock = sysConf.startBlock();
+        assertEq(startBlock, 0xff);
+    }
+
     /// @dev Ensures that the events are emitted during initialization.
     function test_initialize_events_succeeds() external {
         // Wipe out the initialized slot so the proxy can be initialized again
@@ -224,46 +267,6 @@ contract SystemConfig_Initialize_TestFail is SystemConfig_Init {
             )
         );
     }
-
-    /// @dev Tests that initialization fails when the start block override is used
-    ///      when the start block has already been set.
-    function test_initialize_startBlock_reverts() external {
-        // wipe out initialized slot so we can initialize again
-        vm.store(address(sysConf), bytes32(0), bytes32(0));
-        // the startBlock slot is 106, set it to something non zero
-        vm.store(address(sysConf), bytes32(uint256(106)), bytes32(uint256(block.number)));
-
-        // Initialize with a non zero start block, should see a revert
-        vm.prank(multisig);
-        // The call to initialize reverts due to: "SystemConfig: cannot override an already set start block"
-        // but the proxy revert message bubbles up.
-        vm.expectRevert("Proxy: delegatecall to new implementation contract failed");
-        Proxy(payable(address(sysConf))).upgradeToAndCall(
-            address(systemConfigImpl),
-            abi.encodeCall(
-                SystemConfig.initialize,
-                (
-                    alice, // _owner,
-                    overhead, // _overhead,
-                    scalar, // _scalar,
-                    batcherHash, // _batcherHash
-                    gasLimit, // _gasLimit,
-                    unsafeBlockSigner, // _unsafeBlockSigner,
-                    Constants.DEFAULT_RESOURCE_CONFIG(), // _config,
-                    1, // _startBlock
-                    batchInbox, // _batchInbox
-                    SystemConfig.Addresses({ // _addresses
-                        l1CrossDomainMessenger: l1CrossDomainMessenger,
-                        l1ERC721Bridge: l1ERC721Bridge,
-                        l1StandardBridge: l1StandardBridge,
-                        l2OutputOracle: l2OutputOracle,
-                        optimismPortal: optimismPortal,
-                        optimismMintableERC20Factory: optimismMintableERC20Factory
-                    })
-                )
-            )
-        );
-    }
 }
 
 contract SystemConfig_Setters_TestFail is SystemConfig_Init {