feat(ctb): allow multiple output deletions

Updates the L2OutputOracle to allow deletion of multiple outputs at the same time. This is a safety mechanism that prevents a compromised sequencer from forcing the multisig into deleting potentially thousands of bad outputs one by one.
ethereum-optimism · Nov 22, 2022 · c71500a · c71500a
1 parent 49685f8
commit c71500a
Show file tree

Hide file tree

Showing 6 changed files with 188 additions and 178 deletions.
diff --git a/.changeset/lovely-sheep-wash.md b/.changeset/lovely-sheep-wash.md
@@ -0,0 +1,5 @@
+---
+'@eth-optimism/contracts-bedrock': patch
+---
+
+Updates L2OutputOracle to easily delete multiple outputs at once
diff --git a/op-bindings/bindings/l2outputoracle.go b/op-bindings/bindings/l2outputoracle.go
diff --git a/packages/contracts-bedrock/.gas-snapshot b/packages/contracts-bedrock/.gas-snapshot
@@ -85,29 +85,30 @@ L2CrossDomainMessenger_Test:test_L2MessengerSendMessage() (gas: 122423)
 L2CrossDomainMessenger_Test:test_L2MessengerTwiceSendMessage() (gas: 134632)
 L2CrossDomainMessenger_Test:test_L2MessengerXDomainSenderReverts() (gas: 10568)
 L2CrossDomainMessenger_Test:test_L2MessengerxDomainMessageSenderResets() (gas: 52615)
-L2OutputOracleTest:testCannot_ProposeWithUnmatchedBlockhash() (gas: 26798)
-L2OutputOracleTest:testCannot_constructWithBadTimestamp() (gas: 50365)
-L2OutputOracleTest:testCannot_deleteL2Output_ifNotOwner() (gas: 25064)
-L2OutputOracleTest:testCannot_deleteL2Output_withWrongRoot() (gas: 91319)
-L2OutputOracleTest:testCannot_deleteL2Output_withWrongTime() (gas: 87302)
-L2OutputOracleTest:testCannot_proposeEmptyOutput() (gas: 24074)
+L2OutputOracleTest:testCannot_ProposeWithUnmatchedBlockhash() (gas: 26776)
+L2OutputOracleTest:testCannot_constructWithBadTimestamp() (gas: 50351)
+L2OutputOracleTest:testCannot_deleteL2Outputs_afterLatest() (gas: 194584)
+L2OutputOracleTest:testCannot_deleteL2Outputs_ifNotOwner() (gas: 18871)
+L2OutputOracleTest:testCannot_deleteL2Outputs_nonExistent() (gas: 86711)
+L2OutputOracleTest:testCannot_proposeEmptyOutput() (gas: 24085)
 L2OutputOracleTest:testCannot_proposeFutureTimetamp() (gas: 26043)
-L2OutputOracleTest:testCannot_proposeL2OutputIfNotProposer() (gas: 23537)
-L2OutputOracleTest:testCannot_proposeOnWrongFork() (gas: 26316)
-L2OutputOracleTest:testCannot_proposeUnexpectedBlockNumber() (gas: 25977)
-L2OutputOracleTest:test_changeProposer() (gas: 47047)
-L2OutputOracleTest:test_computeL2Timestamp() (gas: 30483)
-L2OutputOracleTest:test_constructor() (gas: 45586)
-L2OutputOracleTest:test_deleteOutput() (gas: 77092)
-L2OutputOracleTest:test_getL2Output() (gas: 88702)
-L2OutputOracleTest:test_latestBlockNumber() (gas: 76186)
-L2OutputOracleTest:test_nextBlockNumber() (gas: 15187)
-L2OutputOracleTest:test_proposeWithBlockhashAndHeight() (gas: 74990)
-L2OutputOracleTest:test_proposingAnotherOutput() (gas: 76815)
+L2OutputOracleTest:testCannot_proposeL2OutputIfNotProposer() (gas: 23515)
+L2OutputOracleTest:testCannot_proposeOnWrongFork() (gas: 26371)
+L2OutputOracleTest:testCannot_proposeUnexpectedBlockNumber() (gas: 25955)
+L2OutputOracleTest:test_changeProposer() (gas: 47212)
+L2OutputOracleTest:test_computeL2Timestamp() (gas: 30430)
+L2OutputOracleTest:test_constructor() (gas: 45691)
+L2OutputOracleTest:test_deleteOutputs_multipleOutputs() (gas: 203841)
+L2OutputOracleTest:test_deleteOutputs_singleOutput() (gas: 94524)
+L2OutputOracleTest:test_getL2Output() (gas: 84525)
+L2OutputOracleTest:test_latestBlockNumber() (gas: 76229)
+L2OutputOracleTest:test_nextBlockNumber() (gas: 15120)
+L2OutputOracleTest:test_proposeWithBlockhashAndHeight() (gas: 75035)
+L2OutputOracleTest:test_proposingAnotherOutput() (gas: 76837)
 L2OutputOracleTest:test_updateOwner() (gas: 36063)
-L2OutputOracleUpgradeable_Test:test_cannotInitImpl() (gas: 17403)
-L2OutputOracleUpgradeable_Test:test_cannotInitProxy() (gas: 22398)
-L2OutputOracleUpgradeable_Test:test_initValuesOnProxy() (gas: 36068)
+L2OutputOracleUpgradeable_Test:test_cannotInitImpl() (gas: 17381)
+L2OutputOracleUpgradeable_Test:test_cannotInitProxy() (gas: 22376)
+L2OutputOracleUpgradeable_Test:test_initValuesOnProxy() (gas: 38217)
 L2OutputOracleUpgradeable_Test:test_upgrading() (gas: 180457)
 L2StandardBridge_Test:test_cannotWithdrawEthWithoutSendingIt() (gas: 21749)
 L2StandardBridge_Test:test_finalizeBridgeETH_incorrectValueReverts() (gas: 23733)
@@ -149,21 +150,21 @@ OptimismPortalUpgradeable_Test:test_initialize_cannotInitImpl_reverts() (gas: 10
 OptimismPortalUpgradeable_Test:test_initialize_cannotInitProxy_reverts() (gas: 15767)
 OptimismPortalUpgradeable_Test:test_params_initValuesOnProxy_success() (gas: 16010)
 OptimismPortalUpgradeable_Test:test_upgradeToAndCall_upgrading_success() (gas: 180435)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputRootChanges_reverts() (gas: 192770)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 195058)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputRootChanges_reverts() (gas: 194874)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifOutputTimestampIsNotFinalized_reverts() (gas: 197162)
 OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalNotProven_reverts() (gas: 39589)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 190512)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 192953)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 173010)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 233210)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 232604)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_success() (gas: 224624)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 327261)
-OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 191268)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 81307)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_ifWithdrawalProofNotOldEnough_reverts() (gas: 192616)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onInsufficientGas_reverts() (gas: 192931)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onRecentWithdrawal_reverts() (gas: 172988)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReentrancy_reverts() (gas: 233188)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_onReplay_reverts() (gas: 234938)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_provenWithdrawalHash_success() (gas: 226854)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_targetFails_fails() (gas: 329491)
+OptimismPortal_FinalizeWithdrawal_Test:test_finalizeWithdrawalTransaction_timestampLessThanL2OracleStart_reverts() (gas: 193394)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onInvalidOutputRootProof_reverts() (gas: 83433)
 OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_onSelfCall_reverts() (gas: 50776)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_oninvalidWithdrawalProof_reverts() (gas: 130079)
-OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_validWithdrawalProof_success() (gas: 176821)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_oninvalidWithdrawalProof_reverts() (gas: 132205)
+OptimismPortal_FinalizeWithdrawal_Test:test_proveWithdrawalTransaction_validWithdrawalProof_success() (gas: 178947)
 OptimismPortal_Test:test_OptimismPortalConstructor() (gas: 17298)
 OptimismPortal_Test:test_OptimismPortalReceiveEth_success() (gas: 127483)
 OptimismPortal_Test:test_depositTransaction_NoValueContract_success() (gas: 76706)
@@ -175,7 +176,7 @@ OptimismPortal_Test:test_depositTransaction_withEthValueAndContractContractCreat
 OptimismPortal_Test:test_depositTransaction_withEthValueAndEOAContractCreation_success() (gas: 75852)
 OptimismPortal_Test:test_depositTransaction_withEthValueFromContract_success() (gas: 83370)
 OptimismPortal_Test:test_depositTransaction_withEthValueFromEOA_success() (gas: 83964)
-OptimismPortal_Test:test_isBlockFinalized_success() (gas: 113327)
+OptimismPortal_Test:test_isBlockFinalized_success() (gas: 104802)
 OptimismPortal_Test:test_simple_isBlockFinalized_success() (gas: 24142)
 Proxy_Test:test_clashingFunctionSignatures() (gas: 101347)
 Proxy_Test:test_implementationKey() (gas: 20887)

diff --git a/packages/contracts-bedrock/contracts/L1/L2OutputOracle.sol b/packages/contracts-bedrock/contracts/L1/L2OutputOracle.sol
@@ -67,17 +67,11 @@ contract L2OutputOracle is OwnableUpgradeable, Semver {
     );
 
     /**
-     * @notice Emitted when an output is deleted.
+     * @notice Emitted when outputs are deleted.
      *
-     * @param outputRoot    The output root.
-     * @param l1Timestamp   The L1 timestamp when proposed.
-     * @param l2BlockNumber The L2 block number of the output root.
+     * @param l2BlockNumber First L2 block number deleted.
      */
-    event OutputDeleted(
-        bytes32 indexed outputRoot,
-        uint256 indexed l1Timestamp,
-        uint256 indexed l2BlockNumber
-    );
+    event OutputsDeleted(uint256 indexed l2BlockNumber);
 
     /**
      * @notice Emitted when the proposer address is changed.
@@ -148,32 +142,35 @@ contract L2OutputOracle is OwnableUpgradeable, Semver {
     }
 
     /**
-     * @notice Deletes the most recent output. This is used to remove the most recent output in the
-     *         event that an erreneous output is submitted. It can only be called by the contract's
-     *         owner, not the proposer. Longer term, this should be replaced with a more robust
-     *         mechanism which will allow deletion of proposals shown to be invalid by a fault
-     *         proof.
+     * @notice Deletes all output proposals after and including the proposal that corresponds to
+     *         the given block number. Can only be called by the owner, but will be replaced with
+     *         a mechanism that allows a challenger contract to delete proposals.
      *
-     * @param _proposal Represents the output proposal to delete
+     * @param _l2BlockNumber L2 block number of the first output root to delete.
      */
     // solhint-disable-next-line ordering
-    function deleteL2Output(Types.OutputProposal memory _proposal) external onlyOwner {
-        Types.OutputProposal memory outputToDelete = l2Outputs[latestBlockNumber];
-
+    function deleteL2Outputs(uint256 _l2BlockNumber) external onlyOwner {
+        // Simple check that accomplishes two things:
+        //   1. Prevents deleting anything from before the genesis block.
+        //   2. Prevents deleting anything other than a checkpoint block.
         require(
-            _proposal.outputRoot == outputToDelete.outputRoot,
-            "L2OutputOracle: output root to delete does not match the latest output proposal"
+            l2Outputs[_l2BlockNumber].outputRoot != bytes32(0),
+            "L2OutputOracle: cannot delete a non-existent output"
         );
 
+        // Prevent deleting beyond latest block number. Above check will miss this case if we
+        // already deleted an output and then the user tries to delete a later output.
         require(
-            _proposal.timestamp == outputToDelete.timestamp,
-            "L2OutputOracle: timestamp to delete does not match the latest output proposal"
+            _l2BlockNumber <= latestBlockNumber,
+            "L2OutputOracle: cannot delete outputs after the latest block number"
         );
 
-        emit OutputDeleted(outputToDelete.outputRoot, outputToDelete.timestamp, latestBlockNumber);
+        // We're setting the latest block number back to the checkpoint block before the given L2
+        // block number. Next proposal will overwrite the deleted output and following proposals
+        // will delete any outputs after that.
+        latestBlockNumber = _l2BlockNumber - SUBMISSION_INTERVAL;
 
-        delete l2Outputs[latestBlockNumber];
-        latestBlockNumber = latestBlockNumber - SUBMISSION_INTERVAL;
+        emit OutputsDeleted(_l2BlockNumber);
     }
 
     /**
@@ -242,6 +239,11 @@ contract L2OutputOracle is OwnableUpgradeable, Semver {
         view
         returns (Types.OutputProposal memory)
     {
+        require(
+            _l2BlockNumber <= latestBlockNumber,
+            "L2OutputOracle: block number cannot be greater than the latest block number"
+        );
+
         // Find the distance between _l2BlockNumber, and the checkpoint block before it.
         uint256 offset = (_l2BlockNumber - STARTING_BLOCK_NUMBER) % SUBMISSION_INTERVAL;
 

diff --git a/packages/contracts-bedrock/contracts/test/L2OutputOracle.t.sol b/packages/contracts-bedrock/contracts/test/L2OutputOracle.t.sol
@@ -79,7 +79,9 @@ contract L2OutputOracleTest is L2OutputOracle_Initializer {
         oracle.getL2Output(0);
 
         // The block number is larger than the latest proposed output:
-        vm.expectRevert("L2OutputOracle: no output found for the given block number");
+        vm.expectRevert(
+            "L2OutputOracle: block number cannot be greater than the latest block number"
+        );
         oracle.getL2Output(nextBlockNumber + 1);
     }
 
@@ -269,36 +271,52 @@ contract L2OutputOracleTest is L2OutputOracle_Initializer {
      * Delete Tests - Happy Path *
      *****************************/
 
-    event OutputDeleted(
-        bytes32 indexed l2Output,
-        uint256 indexed l1Timestamp,
-        uint256 indexed l2BlockNumber
-    );
+    event OutputsDeleted(uint256 indexed l2BlockNumber);
 
-    function test_deleteOutput() external {
+    function test_deleteOutputs_singleOutput() external {
         test_proposingAnotherOutput();
 
         uint256 latestBlockNumber = oracle.latestBlockNumber();
-        Types.OutputProposal memory proposalToDelete = oracle.getL2Output(latestBlockNumber);
         Types.OutputProposal memory newLatestOutput = oracle.getL2Output(
             latestBlockNumber - submissionInterval
         );
 
         vm.prank(owner);
         vm.expectEmit(true, true, false, false);
-        emit OutputDeleted(
-            proposalToDelete.outputRoot,
-            proposalToDelete.timestamp,
-            latestBlockNumber
-        );
-        oracle.deleteL2Output(proposalToDelete);
+        emit OutputsDeleted(latestBlockNumber);
+        oracle.deleteL2Outputs(latestBlockNumber);
 
         // validate latestBlockNumber has been reduced
         uint256 latestBlockNumberAfter = oracle.latestBlockNumber();
         assertEq(latestBlockNumber - submissionInterval, latestBlockNumberAfter);
 
+        // validate that the new latest output is as expected.
         Types.OutputProposal memory proposal = oracle.getL2Output(latestBlockNumberAfter);
+        assertEq(newLatestOutput.outputRoot, proposal.outputRoot);
+        assertEq(newLatestOutput.timestamp, proposal.timestamp);
+    }
+
+    function test_deleteOutputs_multipleOutputs() external {
+        test_proposingAnotherOutput();
+        test_proposingAnotherOutput();
+        test_proposingAnotherOutput();
+
+        uint256 latestBlockNumber = oracle.latestBlockNumber();
+        Types.OutputProposal memory newLatestOutput = oracle.getL2Output(
+            latestBlockNumber - submissionInterval * 3
+        );
+
+        vm.prank(owner);
+        vm.expectEmit(true, true, false, false);
+        emit OutputsDeleted(latestBlockNumber - submissionInterval * 2);
+        oracle.deleteL2Outputs(latestBlockNumber - submissionInterval * 2);
+
+        // validate latestBlockNumber has been reduced
+        uint256 latestBlockNumberAfter = oracle.latestBlockNumber();
+        assertEq(latestBlockNumber - submissionInterval * 3, latestBlockNumberAfter);
+
         // validate that the new latest output is as expected.
+        Types.OutputProposal memory proposal = oracle.getL2Output(latestBlockNumberAfter);
         assertEq(newLatestOutput.outputRoot, proposal.outputRoot);
         assertEq(newLatestOutput.timestamp, proposal.timestamp);
     }
@@ -307,40 +325,38 @@ contract L2OutputOracleTest is L2OutputOracle_Initializer {
      * Delete Tests - Sad Path *
      ***************************/
 
-    function testCannot_deleteL2Output_ifNotOwner() external {
+    function testCannot_deleteL2Outputs_ifNotOwner() external {
         uint256 latestBlockNumber = oracle.latestBlockNumber();
-        Types.OutputProposal memory proposal = oracle.getL2Output(latestBlockNumber);
 
         vm.expectRevert("Ownable: caller is not the owner");
-        oracle.deleteL2Output(proposal);
+        oracle.deleteL2Outputs(latestBlockNumber);
     }
 
-    function testCannot_deleteL2Output_withWrongRoot() external {
+    function testCannot_deleteL2Outputs_nonExistent() external {
         test_proposingAnotherOutput();
 
-        uint256 previousBlockNumber = oracle.latestBlockNumber() - submissionInterval;
-        Types.OutputProposal memory proposalToDelete = oracle.getL2Output(previousBlockNumber);
+        uint256 latestBlockNumber = oracle.latestBlockNumber();
 
         vm.prank(owner);
-        vm.expectRevert(
-            "L2OutputOracle: output root to delete does not match the latest output proposal"
-        );
-        oracle.deleteL2Output(proposalToDelete);
+        vm.expectRevert("L2OutputOracle: cannot delete a non-existent output");
+        oracle.deleteL2Outputs(latestBlockNumber + 1);
     }
 
-    function testCannot_deleteL2Output_withWrongTime() external {
+    function testCannot_deleteL2Outputs_afterLatest() external {
+        // Start by proposing three outputs
+        test_proposingAnotherOutput();
+        test_proposingAnotherOutput();
         test_proposingAnotherOutput();
 
+        // Delete the latest two outputs
         uint256 latestBlockNumber = oracle.latestBlockNumber();
-        Types.OutputProposal memory proposalToDelete = oracle.getL2Output(latestBlockNumber);
+        vm.prank(owner);
+        oracle.deleteL2Outputs(latestBlockNumber - submissionInterval * 2);
 
-        // Modify the timestamp so that it does not match.
-        proposalToDelete.timestamp -= 1;
+        // Now try to delete the same output again
         vm.prank(owner);
-        vm.expectRevert(
-            "L2OutputOracle: timestamp to delete does not match the latest output proposal"
-        );
-        oracle.deleteL2Output(proposalToDelete);
+        vm.expectRevert("L2OutputOracle: cannot delete outputs after the latest block number");
+        oracle.deleteL2Outputs(latestBlockNumber - submissionInterval * 2);
     }
 }
 

diff --git a/packages/contracts-bedrock/contracts/test/OptimismPortal.t.sol b/packages/contracts-bedrock/contracts/test/OptimismPortal.t.sol
@@ -242,7 +242,9 @@ contract OptimismPortal_Test is Portal_Initializer {
         // The checkpointed block should not be finalized until 1 second from now.
         assertEq(op.isBlockFinalized(checkpoint), false);
         // Nor should a block after it
-        vm.expectRevert("L2OutputOracle: no output found for the given block number");
+        vm.expectRevert(
+            "L2OutputOracle: block number cannot be greater than the latest block number"
+        );
         assertEq(op.isBlockFinalized(checkpoint + 1), false);
         // Nor a block before it, even though the finalization period has passed, there is
         // not yet a checkpoint block on top of it for which that is true.
@@ -255,7 +257,9 @@ contract OptimismPortal_Test is Portal_Initializer {
         // So should the block before it.
         assertEq(op.isBlockFinalized(checkpoint - 1), true);
         // But not the block after it.
-        vm.expectRevert("L2OutputOracle: no output found for the given block number");
+        vm.expectRevert(
+            "L2OutputOracle: block number cannot be greater than the latest block number"
+        );
         assertEq(op.isBlockFinalized(checkpoint + 1), false);
     }
 }