From 1b968b1fbf6cc3ee3a1653660ad43341be7ffa98 Mon Sep 17 00:00:00 2001 From: Maurelian Date: Tue, 28 Sep 2021 21:29:01 -0400 Subject: [PATCH] refactor(contracts): remove unused code from CTC --- .changeset/few-students-pretend.md | 5 + packages/contracts/bin/deploy.ts | 5 - .../L1/rollup/CanonicalTransactionChain.sol | 67 +------------- .../L1/rollup/ICanonicalTransactionChain.sol | 9 -- ...04-OVM_CanonicalTransactionChain.deploy.ts | 2 - packages/contracts/tasks/deploy.ts | 11 --- .../messaging/L1CrossDomainMessenger.spec.ts | 4 - .../L1/messaging/deposit.gas.spec.ts | 4 - .../CanonicalTransactionChain.gas.spec.ts | 7 +- .../rollup/CanonicalTransactionChain.spec.ts | 91 ------------------- 10 files changed, 9 insertions(+), 196 deletions(-) create mode 100644 .changeset/few-students-pretend.md diff --git a/.changeset/few-students-pretend.md b/.changeset/few-students-pretend.md new file mode 100644 index 000000000000..c0c585f8905a --- /dev/null +++ b/.changeset/few-students-pretend.md @@ -0,0 +1,5 @@ +--- +'@eth-optimism/contracts': minor +--- + +Remove unused code from CTC diff --git a/packages/contracts/bin/deploy.ts b/packages/contracts/bin/deploy.ts index db9f11a54053..b39fdb052331 100755 --- a/packages/contracts/bin/deploy.ts +++ b/packages/contracts/bin/deploy.ts @@ -31,10 +31,6 @@ const parseEnv = () => { return { l1BlockTimeSeconds: ensure('BLOCK_TIME_SECONDS', 'number'), - ctcForceInclusionPeriodSeconds: ensure( - 'FORCE_INCLUSION_PERIOD_SECONDS', - 'number' - ), ctcMaxTransactionGasLimit: ensure('MAX_TRANSACTION_GAS_LIMIT', 'number'), ctcL2GasDiscountDivisor: ensure('L2_GAS_DISCOUNT_DIVISOR', 'number'), ctcEnqueueGasCost: ensure('ENQUEUE_GAS_COST', 'number'), @@ -51,7 +47,6 @@ const main = async () => { await hre.run('deploy', { l1BlockTimeSeconds: config.l1BlockTimeSeconds, - ctcForceInclusionPeriodSeconds: config.ctcForceInclusionPeriodSeconds, ctcMaxTransactionGasLimit: config.ctcMaxTransactionGasLimit, ctcL2GasDiscountDivisor: config.ctcL2GasDiscountDivisor, ctcEnqueueGasCost: config.ctcEnqueueGasCost, diff --git a/packages/contracts/contracts/L1/rollup/CanonicalTransactionChain.sol b/packages/contracts/contracts/L1/rollup/CanonicalTransactionChain.sol index 5b92c158268f..1131be2b56eb 100644 --- a/packages/contracts/contracts/L1/rollup/CanonicalTransactionChain.sol +++ b/packages/contracts/contracts/L1/rollup/CanonicalTransactionChain.sol @@ -20,8 +20,6 @@ import { Math } from "@openzeppelin/contracts/utils/math/Math.sol"; * writing them to the 'CTC:batches' instance of the Chain Storage Container. * The CTC also allows any account to 'enqueue' an L2 transaction, which will require that the * Sequencer will eventually append it to the rollup state. - * If the Sequencer does not include an enqueued transaction within the 'force inclusion period', - * then any account may force it to be included by calling appendQueueBatch(). * * Runtime target: EVM */ @@ -50,8 +48,6 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes * Variables * *************/ - uint256 public forceInclusionPeriodSeconds; - uint256 public forceInclusionPeriodBlocks; uint256 public maxTransactionGasLimit; @@ -61,16 +57,12 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes constructor( address _libAddressManager, - uint256 _forceInclusionPeriodSeconds, - uint256 _forceInclusionPeriodBlocks, uint256 _maxTransactionGasLimit, uint256 _l2GasDiscountDivisor, uint256 _enqueueGasCost ) Lib_AddressResolver(_libAddressManager) { - forceInclusionPeriodSeconds = _forceInclusionPeriodSeconds; - forceInclusionPeriodBlocks = _forceInclusionPeriodBlocks; maxTransactionGasLimit = _maxTransactionGasLimit; L2_GAS_DISCOUNT_DIVISOR = _l2GasDiscountDivisor; ENQUEUE_GAS_COST = _enqueueGasCost; @@ -250,7 +242,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes uint256 _gasLimit, bytes memory _data ) - public + external { require( _data.length <= MAX_ROLLUP_TX_SIZE, @@ -325,59 +317,6 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes ); } - /** - * Appends a given number of queued transactions as a single batch. - * param _numQueuedTransactions Number of transactions to append. - */ - function appendQueueBatch( - uint256 // _numQueuedTransactions - ) - public - pure - { - // TEMPORARY: Disable `appendQueueBatch` for minnet - revert("appendQueueBatch is currently disabled."); - - // solhint-disable max-line-length - // _numQueuedTransactions = Math.min(_numQueuedTransactions, getNumPendingQueueElements()); - // require( - // _numQueuedTransactions > 0, - // "Must append more than zero transactions." - // ); - - // bytes32[] memory leaves = new bytes32[](_numQueuedTransactions); - // uint40 nextQueueIndex = getNextQueueIndex(); - - // for (uint256 i = 0; i < _numQueuedTransactions; i++) { - // if (msg.sender != resolve("OVM_Sequencer")) { - // Lib_OVMCodec.QueueElement memory el = getQueueElement(nextQueueIndex); - // require( - // el.timestamp + forceInclusionPeriodSeconds < block.timestamp, - // "Queue transactions cannot be submitted during the sequencer inclusion period." - // ); - // } - // leaves[i] = _getQueueLeafHash(nextQueueIndex); - // nextQueueIndex++; - // } - - // Lib_OVMCodec.QueueElement memory lastElement = getQueueElement(nextQueueIndex - 1); - - // _appendBatch( - // Lib_MerkleTree.getMerkleRoot(leaves), - // _numQueuedTransactions, - // _numQueuedTransactions, - // lastElement.timestamp, - // lastElement.blockNumber - // ); - - // emit QueueBatchAppended( - // nextQueueIndex - _numQueuedTransactions, - // _numQueuedTransactions, - // getTotalElements() - // ); - // solhint-enable max-line-length - } - /** * Allows the sequencer to append a batch of transactions. * @dev This function uses a custom encoding scheme for efficiency reasons. @@ -387,7 +326,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes * .param _transactionDataFields Array of raw transaction data. */ function appendSequencerBatch() - public + external { uint40 shouldStartAtElement; uint24 totalElementsToAppend; @@ -540,7 +479,7 @@ contract CanonicalTransactionChain is ICanonicalTransactionChain, Lib_AddressRes Lib_OVMCodec.ChainBatchHeader memory _batchHeader, Lib_OVMCodec.ChainInclusionProof memory _inclusionProof ) - public + external view returns ( bool diff --git a/packages/contracts/contracts/L1/rollup/ICanonicalTransactionChain.sol b/packages/contracts/contracts/L1/rollup/ICanonicalTransactionChain.sol index ce5ad6d04fae..95db3b353ef7 100644 --- a/packages/contracts/contracts/L1/rollup/ICanonicalTransactionChain.sol +++ b/packages/contracts/contracts/L1/rollup/ICanonicalTransactionChain.sol @@ -191,15 +191,6 @@ interface ICanonicalTransactionChain { ) external; - /** - * Appends a given number of queued transactions as a single batch. - * @param _numQueuedTransactions Number of transactions to append. - */ - function appendQueueBatch( - uint256 _numQueuedTransactions - ) - external; - /** * Allows the sequencer to append a batch of transactions. * @dev This function uses a custom encoding scheme for efficiency reasons. diff --git a/packages/contracts/deploy/004-OVM_CanonicalTransactionChain.deploy.ts b/packages/contracts/deploy/004-OVM_CanonicalTransactionChain.deploy.ts index 7758b6d46a34..9b7eca45c72d 100644 --- a/packages/contracts/deploy/004-OVM_CanonicalTransactionChain.deploy.ts +++ b/packages/contracts/deploy/004-OVM_CanonicalTransactionChain.deploy.ts @@ -18,8 +18,6 @@ const deployFn: DeployFunction = async (hre) => { name: 'CanonicalTransactionChain', args: [ Lib_AddressManager.address, - (hre as any).deployConfig.ctcForceInclusionPeriodSeconds, - (hre as any).deployConfig.ctcForceInclusionPeriodBlocks, (hre as any).deployConfig.ctcMaxTransactionGasLimit, (hre as any).deployConfig.ctcL2GasDiscountDivisor, (hre as any).deployConfig.ctcEnqueueGasCost, diff --git a/packages/contracts/tasks/deploy.ts b/packages/contracts/tasks/deploy.ts index 00eee160d351..ac0e9b6b0384 100644 --- a/packages/contracts/tasks/deploy.ts +++ b/packages/contracts/tasks/deploy.ts @@ -4,7 +4,6 @@ import { task } from 'hardhat/config' import * as types from 'hardhat/internal/core/params/argumentTypes' const DEFAULT_L1_BLOCK_TIME_SECONDS = 15 -const DEFAULT_CTC_FORCE_INCLUSION_PERIOD_SECONDS = 60 * 60 * 24 * 30 // 30 days const DEFAULT_CTC_MAX_TRANSACTION_GAS_LIMIT = 11_000_000 const DEFAULT_CTC_L2_GAS_DISCOUNT_DIVISOR = 32 const DEFAULT_CTC_ENQUEUE_GAS_COST = 60_000 @@ -18,12 +17,6 @@ task('deploy') DEFAULT_L1_BLOCK_TIME_SECONDS, types.int ) - .addOptionalParam( - 'ctcForceInclusionPeriodSeconds', - 'Number of seconds that the sequencer has to include transactions before the L1 queue.', - DEFAULT_CTC_FORCE_INCLUSION_PERIOD_SECONDS, - types.int - ) .addOptionalParam( 'ctcMaxTransactionGasLimit', 'Max gas limit for L1 queue transactions.', @@ -91,10 +84,6 @@ task('deploy') validateAddressArg('ovmProposerAddress') validateAddressArg('ovmAddressManagerOwner') - args.ctcForceInclusionPeriodBlocks = Math.floor( - args.ctcForceInclusionPeriodSeconds / args.l1BlockTimeSeconds - ) - hre.deployConfig = args return runSuper(args) }) diff --git a/packages/contracts/test/contracts/L1/messaging/L1CrossDomainMessenger.spec.ts b/packages/contracts/test/contracts/L1/messaging/L1CrossDomainMessenger.spec.ts index 1f077f8052bf..00f36be9a569 100644 --- a/packages/contracts/test/contracts/L1/messaging/L1CrossDomainMessenger.spec.ts +++ b/packages/contracts/test/contracts/L1/messaging/L1CrossDomainMessenger.spec.ts @@ -14,8 +14,6 @@ import { NON_ZERO_ADDRESS, DUMMY_BATCH_HEADERS, DUMMY_BATCH_PROOFS, - FORCE_INCLUSION_PERIOD_SECONDS, - FORCE_INCLUSION_PERIOD_BLOCKS, L2_GAS_DISCOUNT_DIVISOR, ENQUEUE_GAS_COST, TrieTestGenerator, @@ -100,8 +98,6 @@ describe('L1CrossDomainMessenger', () => { ) CanonicalTransactionChain = await Factory__CanonicalTransactionChain.deploy( AddressManager.address, - FORCE_INCLUSION_PERIOD_SECONDS, - FORCE_INCLUSION_PERIOD_BLOCKS, MAX_GAS_LIMIT, L2_GAS_DISCOUNT_DIVISOR, ENQUEUE_GAS_COST diff --git a/packages/contracts/test/contracts/L1/messaging/deposit.gas.spec.ts b/packages/contracts/test/contracts/L1/messaging/deposit.gas.spec.ts index 973ed107ec16..a540edb8f668 100644 --- a/packages/contracts/test/contracts/L1/messaging/deposit.gas.spec.ts +++ b/packages/contracts/test/contracts/L1/messaging/deposit.gas.spec.ts @@ -6,8 +6,6 @@ import { smoddit } from '@eth-optimism/smock' /* Internal Imports */ import { makeAddressManager, - FORCE_INCLUSION_PERIOD_SECONDS, - FORCE_INCLUSION_PERIOD_BLOCKS, L2_GAS_DISCOUNT_DIVISOR, ENQUEUE_GAS_COST, NON_ZERO_ADDRESS, @@ -46,8 +44,6 @@ describe('[GAS BENCHMARK] Depositing via the standard bridge', () => { await ethers.getContractFactory('CanonicalTransactionChain') ).deploy( AddressManager.address, - FORCE_INCLUSION_PERIOD_SECONDS, - FORCE_INCLUSION_PERIOD_BLOCKS, MAX_GAS_LIMIT, L2_GAS_DISCOUNT_DIVISOR, ENQUEUE_GAS_COST diff --git a/packages/contracts/test/contracts/L1/rollup/CanonicalTransactionChain.gas.spec.ts b/packages/contracts/test/contracts/L1/rollup/CanonicalTransactionChain.gas.spec.ts index f702ba324db9..4aaa7410fa4d 100644 --- a/packages/contracts/test/contracts/L1/rollup/CanonicalTransactionChain.gas.spec.ts +++ b/packages/contracts/test/contracts/L1/rollup/CanonicalTransactionChain.gas.spec.ts @@ -81,13 +81,8 @@ describe('[GAS BENCHMARK] CanonicalTransactionChain', () => { let CanonicalTransactionChain: Contract beforeEach(async () => { - // Use a larger FIP for blocks so that we can send a large number of - // enqueue() transactions without having to manipulate the block number. - const forceInclusionPeriodBlocks = 101 CanonicalTransactionChain = await Factory__CanonicalTransactionChain.deploy( AddressManager.address, - FORCE_INCLUSION_PERIOD_SECONDS, - forceInclusionPeriodBlocks, MAX_GAS_LIMIT, L2_GAS_DISCOUNT_DIVISOR, ENQUEUE_GAS_COST @@ -269,7 +264,7 @@ describe('[GAS BENCHMARK] CanonicalTransactionChain', () => { 'Non-calldata overhead gas cost per transaction:', (gasUsed - fixedCalldataCost) / numTxs ) - expectApprox(gasUsed, 1_293_111, { upperPercentDeviation: 0 }) + expectApprox(gasUsed, 1_293_611, { upperPercentDeviation: 0 }) }).timeout(10_000_000) }) diff --git a/packages/contracts/test/contracts/L1/rollup/CanonicalTransactionChain.spec.ts b/packages/contracts/test/contracts/L1/rollup/CanonicalTransactionChain.spec.ts index 26576bee4fcf..d93a51a08b57 100644 --- a/packages/contracts/test/contracts/L1/rollup/CanonicalTransactionChain.spec.ts +++ b/packages/contracts/test/contracts/L1/rollup/CanonicalTransactionChain.spec.ts @@ -17,17 +17,12 @@ import _ from 'lodash' import { makeAddressManager, setProxyTarget, - FORCE_INCLUSION_PERIOD_SECONDS, - FORCE_INCLUSION_PERIOD_BLOCKS, L2_GAS_DISCOUNT_DIVISOR, ENQUEUE_GAS_COST, setEthTime, NON_ZERO_ADDRESS, getEthTime, getNextBlockNumber, - increaseEthTime, - getBlockTime, - mineBlock, } from '../../../helpers' import { predeploys } from '../../../../src' @@ -132,8 +127,6 @@ describe('CanonicalTransactionChain', () => { beforeEach(async () => { CanonicalTransactionChain = await Factory__CanonicalTransactionChain.deploy( AddressManager.address, - FORCE_INCLUSION_PERIOD_SECONDS, - FORCE_INCLUSION_PERIOD_BLOCKS, MAX_GAS_LIMIT, L2_GAS_DISCOUNT_DIVISOR, ENQUEUE_GAS_COST @@ -424,89 +417,6 @@ describe('CanonicalTransactionChain', () => { }) }) - describe('appendQueueBatch disabled', () => { - it('should revert', async () => { - await expect( - CanonicalTransactionChain.appendQueueBatch(0) - ).to.be.revertedWith('appendQueueBatch is currently disabled.') - }) - }) - - describe.skip('appendQueueBatch', () => { - it('should revert if trying to append zero transactions', async () => { - await expect( - CanonicalTransactionChain.appendQueueBatch(0) - ).to.be.revertedWith('Must append more than zero transactions.') - }) - - it('should revert if the queue is empty', async () => { - await expect( - CanonicalTransactionChain.appendQueueBatch(1) - ).to.be.revertedWith('Must append more than zero transactions.') - }) - - describe('when the queue is not empty', () => { - const target = NON_ZERO_ADDRESS - const gasLimit = 500_000 - const data = '0x' + '12'.repeat(1234) - - for (const size of ELEMENT_TEST_SIZES) { - describe(`when the queue has ${size} elements`, () => { - beforeEach(async () => { - for (let i = 0; i < size; i++) { - await CanonicalTransactionChain.enqueue(target, gasLimit, data) - } - }) - - describe('when the sequencer inclusion period has not passed', () => { - it('should revert if not called by the sequencer', async () => { - await expect( - CanonicalTransactionChain.connect(signer).appendQueueBatch(1) - ).to.be.revertedWith( - 'Queue transactions cannot be submitted during the sequencer inclusion period.' - ) - }) - - it('should succeed if called by the sequencer', async () => { - await expect( - CanonicalTransactionChain.connect(sequencer).appendQueueBatch(1) - ) - .to.emit(CanonicalTransactionChain, 'QueueBatchAppended') - .withArgs(0, 1, 1) - }) - }) - - describe('when the sequencer inclusion period has passed', () => { - beforeEach(async () => { - await increaseEthTime( - ethers.provider, - FORCE_INCLUSION_PERIOD_SECONDS * 2 - ) - }) - - it('should be able to append a single element', async () => { - await expect(CanonicalTransactionChain.appendQueueBatch(1)) - .to.emit(CanonicalTransactionChain, 'QueueBatchAppended') - .withArgs(0, 1, 1) - }) - - it(`should be able to append ${size} elements`, async () => { - await expect(CanonicalTransactionChain.appendQueueBatch(size)) - .to.emit(CanonicalTransactionChain, 'QueueBatchAppended') - .withArgs(0, size, size) - }) - - it(`should be able to append ${size} elements even if attempting to append ${size} + 1 elements`, async () => { - await expect(CanonicalTransactionChain.appendQueueBatch(size + 1)) - .to.emit(CanonicalTransactionChain, 'QueueBatchAppended') - .withArgs(0, size, size) - }) - }) - }) - } - }) - }) - describe('verifyTransaction', () => { it('should successfully verify against a valid queue transaction appended by the sequencer', async () => { const entrypoint = NON_ZERO_ADDRESS @@ -576,7 +486,6 @@ describe('CanonicalTransactionChain', () => { await CanonicalTransactionChain.enqueue(entrypoint, gasLimit, data) const blockNumber = await ethers.provider.getBlockNumber() - await increaseEthTime(ethers.provider, FORCE_INCLUSION_PERIOD_SECONDS * 2) await CanonicalTransactionChain.appendQueueBatch(1)