diff --git a/packages/contracts/contracts/optimistic-ethereum/ovm/StateTransitioner.sol b/packages/contracts/contracts/optimistic-ethereum/ovm/StateTransitioner.sol index 45e9fa7c912eb..763cc089a9b5a 100644 --- a/packages/contracts/contracts/optimistic-ethereum/ovm/StateTransitioner.sol +++ b/packages/contracts/contracts/optimistic-ethereum/ovm/StateTransitioner.sol @@ -35,6 +35,8 @@ contract StateTransitioner is IStateTransitioner, ContractResolver { bytes32 constant private BYTES32_NULL = bytes32(''); uint256 constant private UINT256_NULL = uint256(0); + // Max gas overhead that executeTransaction(...) can consume outside of the metered code contract execution. TODO: parameterize + uint constant private MAX_EXECUTE_TRANSACTION_GAS_OVERHEAD = 500000; /* @@ -229,8 +231,15 @@ contract StateTransitioner is IStateTransitioner, ContractResolver { stateManager.initNewTransactionExecution(); executionManager.setStateManager(address(stateManager)); + // Make sure we were given sufficient gas, accounting for EIP 150, +10000 to account for gas cost of prepping executeTransaction calldata + uint gasLeft = gasleft(); + require( + gasLeft > ((_transactionData.gasLimit + MAX_EXECUTE_TRANSACTION_GAS_OVERHEAD + 10000) * 64 / 63), + "Insufficient gas supplied to ensure L1 execution will not run out of gas before OVM transaction gas limit." + ); + // Execute the transaction via the execution manager. - executionManager.executeTransaction( + executionManager.executeTransaction.gas(gasLeft)( _transactionData.timestamp, _transactionData.blockNumber, _transactionData.queueOrigin, diff --git a/packages/contracts/test/contracts/ovm/StateTransitioner.spec.ts b/packages/contracts/test/contracts/ovm/StateTransitioner.spec.ts index 4e675dcdfac6c..b833335a1bd1c 100644 --- a/packages/contracts/test/contracts/ovm/StateTransitioner.spec.ts +++ b/packages/contracts/test/contracts/ovm/StateTransitioner.spec.ts @@ -51,6 +51,8 @@ const DUMMY_ACCOUNT_ADDRESSES = [ // gas metering always causes some storage slots to be updated const DEFAULT_TX_NUM_STORAGE_UPDATES: number = 4 +const SUFFICIENT_APPLY_TRANSACTION_GAS = GAS_LIMIT * 2 + interface OVMTransactionData { timestamp: number blockNumber: number @@ -492,6 +494,10 @@ describe('StateTransitioner', () => { let stateTrie: any let test: AccountStorageProofTest + let stateTransitioner: Contract + let stateManager: Contract + let dummyTransactionData: OVMTransactionData + let initializedDummyTxSnapshot before(async () => { stateTrie = makeInitialStateTrie( fraudTester.address, @@ -509,19 +515,29 @@ describe('StateTransitioner', () => { fraudTester.address, DUMMY_ACCOUNT_STORAGE()[0].key ) - }) - - let stateTransitioner: Contract - let stateManager: Contract - let transactionData: OVMTransactionData - beforeEach(async () => { - ;[stateTransitioner, stateManager] = await initStateTransitioner( + ;[ + stateTransitioner, + stateManager, + dummyTransactionData, + ] = await initStateTransitioner( StateTransitioner, StateManager, resolver.addressResolver, stateTrie, makeDummyTransaction('0x00') ) + initializedDummyTxSnapshot = await ethers.provider.send('evm_snapshot', []) + }) + + const revertToDummyTxSnapshot = async () => { + await ethers.provider.send('evm_revert', [initializedDummyTxSnapshot]) + // evm_revert deletes the snapshot so reset it right after + initializedDummyTxSnapshot = await ethers.provider.send('evm_snapshot', []) + } + + let transactionData: OVMTransactionData + beforeEach(async () => { + await revertToDummyTxSnapshot() }) describe('Initialization', async () => { @@ -547,16 +563,18 @@ describe('StateTransitioner', () => { }) it('should correctly reject inclusion of a contract with an invalid nonce', async () => { - try { - await stateTransitioner.proveContractInclusion( - fraudTester.address, - fraudTester.address, - 123, // Wrong nonce. - test.stateTrieWitness - ) - } catch (e) { - expect(e.toString()).to.contain('Invalid account state provided.') - } + await ethers.provider.send('evm_revert', [initializedDummyTxSnapshot]) + await TestUtils.assertRevertsAsync( + 'Invalid account state provided.', + async () => { + await stateTransitioner.proveContractInclusion( + fraudTester.address, + fraudTester.address, + 123, // Wrong nonce. + test.stateTrieWitness + ) + } + ) expect( await stateManager.isVerifiedContract(fraudTester.address) @@ -597,17 +615,18 @@ describe('StateTransitioner', () => { test.stateTrieWitness ) - try { - await stateTransitioner.proveStorageSlotInclusion( - fraudTester.address, - DUMMY_ACCOUNT_STORAGE()[0].key, - DUMMY_ACCOUNT_STORAGE()[1].val, // Different value. - test.stateTrieWitness, - test.storageTrieWitness - ) - } catch (e) { - expect(e.toString()).to.contain('Invalid account state provided.') - } + await TestUtils.assertRevertsAsync( + 'Invalid account state provided.', + async () => { + await stateTransitioner.proveStorageSlotInclusion( + fraudTester.address, + DUMMY_ACCOUNT_STORAGE()[0].key, + DUMMY_ACCOUNT_STORAGE()[1].val, // Different value. + test.stateTrieWitness, + test.storageTrieWitness + ) + } + ) expect( await stateManager.isVerifiedStorage( @@ -620,6 +639,16 @@ describe('StateTransitioner', () => { }) describe('applyTransaction(...)', async () => { + it('should fail if supplied less gas than might be needed to executeTransaction(...)', async () => { + TestUtils.assertRevertsAsync( + 'Insufficient gas supplied to ensure L1 execution will not run out of gas before OVM transaction gas limit.', + async () => { + await stateTransitioner.applyTransaction(dummyTransactionData, { + gasLimit: GAS_LIMIT / 2, + }) + } + ) + }) it('should succeed if no state is accessed', async () => { ;[ stateTransitioner, @@ -646,7 +675,9 @@ describe('StateTransitioner', () => { test.stateTrieWitness ) - await stateTransitioner.applyTransaction(transactionData) + await stateTransitioner.applyTransaction(transactionData, { + gasLimit: SUFFICIENT_APPLY_TRANSACTION_GAS, + }) expect(await stateTransitioner.currentTransitionPhase()).to.equal( STATE_TRANSITIONER_PHASES.POST_EXECUTION ) @@ -707,7 +738,9 @@ describe('StateTransitioner', () => { accessTest.storageTrieWitness ) - await stateTransitioner.applyTransaction(transactionData) + await stateTransitioner.applyTransaction(transactionData, { + gasLimit: SUFFICIENT_APPLY_TRANSACTION_GAS, + }) expect(await stateTransitioner.currentTransitionPhase()).to.equal( STATE_TRANSITIONER_PHASES.POST_EXECUTION ) @@ -740,7 +773,9 @@ describe('StateTransitioner', () => { test.stateTrieWitness ) - await stateTransitioner.applyTransaction(transactionData) + await stateTransitioner.applyTransaction(transactionData, { + gasLimit: SUFFICIENT_APPLY_TRANSACTION_GAS, + }) expect(await stateTransitioner.currentTransitionPhase()).to.equal( STATE_TRANSITIONER_PHASES.POST_EXECUTION ) @@ -776,7 +811,9 @@ describe('StateTransitioner', () => { await TestUtils.assertRevertsAsync( 'Detected an invalid state access.', async () => { - await stateTransitioner.applyTransaction(transactionData) + await stateTransitioner.applyTransaction(transactionData, { + gasLimit: SUFFICIENT_APPLY_TRANSACTION_GAS, + }) } ) @@ -807,7 +844,9 @@ describe('StateTransitioner', () => { await TestUtils.assertRevertsAsync( 'Detected an invalid state access.', async () => { - await stateTransitioner.applyTransaction(transactionData) + await stateTransitioner.applyTransaction(transactionData, { + gasLimit: SUFFICIENT_APPLY_TRANSACTION_GAS, + }) } ) @@ -845,7 +884,9 @@ describe('StateTransitioner', () => { test.stateTrieWitness ) - await stateTransitioner.applyTransaction(transactionData) + await stateTransitioner.applyTransaction(transactionData, { + gasLimit: SUFFICIENT_APPLY_TRANSACTION_GAS, + }) expect(await stateManager.updatedStorageSlotCounter()).to.equal( DEFAULT_TX_NUM_STORAGE_UPDATES + 1 @@ -891,7 +932,9 @@ describe('StateTransitioner', () => { test.stateTrieWitness ) - await stateTransitioner.applyTransaction(transactionData) + await stateTransitioner.applyTransaction(transactionData, { + gasLimit: SUFFICIENT_APPLY_TRANSACTION_GAS, + }) expect(await stateManager.updatedStorageSlotCounter()).to.equal( DEFAULT_TX_NUM_STORAGE_UPDATES + 3 @@ -937,7 +980,9 @@ describe('StateTransitioner', () => { test.stateTrieWitness ) - await stateTransitioner.applyTransaction(transactionData) + await stateTransitioner.applyTransaction(transactionData, { + gasLimit: SUFFICIENT_APPLY_TRANSACTION_GAS, + }) expect(await stateManager.updatedStorageSlotCounter()).to.equal( DEFAULT_TX_NUM_STORAGE_UPDATES + 1 @@ -981,7 +1026,9 @@ describe('StateTransitioner', () => { test.stateTrieWitness ) - await stateTransitioner.applyTransaction(transactionData) + await stateTransitioner.applyTransaction(transactionData, { + gasLimit: SUFFICIENT_APPLY_TRANSACTION_GAS, + }) // One update for each new contract, plus one nonce update for the creating contract. expect(await stateManager.updatedContractsCounter()).to.equal(2) @@ -1025,7 +1072,9 @@ describe('StateTransitioner', () => { test.stateTrieWitness ) - await stateTransitioner.applyTransaction(transactionData) + await stateTransitioner.applyTransaction(transactionData, { + gasLimit: SUFFICIENT_APPLY_TRANSACTION_GAS, + }) // One update for each new contract, plus one nonce update for the creating contract. expect(await stateManager.updatedContractsCounter()).to.equal(4) @@ -1101,7 +1150,9 @@ describe('StateTransitioner', () => { accessTest.storageTrieWitness ) - await stateTransitioner.applyTransaction(transactionData) + await stateTransitioner.applyTransaction(transactionData, { + gasLimit: SUFFICIENT_APPLY_TRANSACTION_GAS, + }) expect(await stateManager.updatedStorageSlotCounter()).to.equal( DEFAULT_TX_NUM_STORAGE_UPDATES + 0 ) @@ -1140,7 +1191,9 @@ describe('StateTransitioner', () => { test.stateTrieWitness ) - await stateTransitioner.applyTransaction(transactionData) + await stateTransitioner.applyTransaction(transactionData, { + gasLimit: SUFFICIENT_APPLY_TRANSACTION_GAS, + }) expect(await stateManager.updatedStorageSlotCounter()).to.equal( DEFAULT_TX_NUM_STORAGE_UPDATES + 1 )