Skip to content

Ensuring sufficient gas for L1 determinism #232

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
Jinglan merged 3 commits into from
Aug 27, 2020
Merged

Ensuring sufficient gas for L1 determinism #232

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
0e9d16b
speed up ST tests
ben-chain Aug 26, 2020
61ebe4a
add test and fix existing
ben-chain Aug 26, 2020
bbdf51e
linting
ben-chain Aug 26, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 10 additions & 1 deletion packages/contracts/contracts/optimistic-ethereum/ovm/StateTransitioner.sol
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;


/*
Expand Down Expand Up @@ -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."
Copy link

@willmeister willmeister Aug 26, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would change the error message or add a comment specifically saying why the 63/64 logic is in there (maybe with a link?)

);

// Execute the transaction via the execution manager.
executionManager.executeTransaction(
executionManager.executeTransaction.gas(gasLeft)(
_transactionData.timestamp,
_transactionData.blockNumber,
_transactionData.queueOrigin,
Expand Down
133 changes: 93 additions & 40 deletions packages/contracts/test/contracts/ovm/StateTransitioner.spec.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand Down Expand Up @@ -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,
Expand All @@ -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 () => {
Expand All @@ -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)
Expand Down Expand Up @@ -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(
Expand All @@ -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,
Expand All @@ -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
)
Expand Down Expand Up @@ -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
)
Expand Down Expand Up @@ -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
)
Expand Down Expand Up @@ -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,
})
}
)

Expand Down Expand Up @@ -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,
})
}
)

Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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)
Expand Down Expand Up @@ -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
)
Expand Down Expand Up @@ -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
)
Expand Down