From 0e4fb48e8c5a9a987da5a67bae262fb9359bf3bd Mon Sep 17 00:00:00 2001 From: ben-chain Date: Fri, 24 Apr 2020 12:30:56 -0400 Subject: [PATCH 1/9] pass RevertErrors up in full node --- packages/rollup-full-node/src/app/web3-rpc-handler.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/packages/rollup-full-node/src/app/web3-rpc-handler.ts b/packages/rollup-full-node/src/app/web3-rpc-handler.ts index bf608c471ad34..4398a663a7553 100644 --- a/packages/rollup-full-node/src/app/web3-rpc-handler.ts +++ b/packages/rollup-full-node/src/app/web3-rpc-handler.ts @@ -41,6 +41,7 @@ import { UnsupportedMethodError, Web3Handler, Web3RpcMethods, + RevertError, } from '../types' import { initializeL2Node, getCurrentTime } from './utils' import { NoOpL2ToL1MessageSubmitter } from './message-submitter' @@ -616,14 +617,14 @@ export class DefaultWeb3Handler } catch (e) { logError( log, - `Error executing transaction!\n\nIncrementing nonce for sender (${ovmTx.from} and returning failed tx hash. Ovm tx hash: ${ovmTxHash}, internal hash: ${internalTxHash}.`, + `Error executing internal transaction!\n\nIncrementing nonce for sender (${ovmTx.from} and returning failed tx hash. Ovm tx hash: ${ovmTxHash}, internal hash: ${internalTxHash}.`, e ) await this.context.executionManager.incrementNonce(add0x(ovmTx.from)) log.debug(`Nonce incremented successfully for ${ovmTx.from}.`) - return ovmTxHash + throw new RevertError(e.message as string) } if (remove0x(internalTxHash) !== remove0x(returnedInternalTxHash)) { @@ -898,7 +899,7 @@ export class DefaultWeb3Handler ovmTx.data, ovmFrom, ZERO_ADDRESS, - false + true ) log.debug(`EOA calldata: [${internalCalldata}]`) From 413a98f28bac3c7a0b43bbaffebc914d622455c3 Mon Sep 17 00:00:00 2001 From: ben-chain Date: Fri, 24 Apr 2020 16:36:17 -0400 Subject: [PATCH 2/9] revert messages working, receipts not --- .../test/app/web-rpc-handler.spec.ts | 64 ++++++++++++++++++- .../contracts/transpiled/SimpleReversion.sol | 10 +++ 2 files changed, 73 insertions(+), 1 deletion(-) create mode 100644 packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol diff --git a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts index a5076874c41db..c1b24c3b98565 100644 --- a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts +++ b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts @@ -9,7 +9,7 @@ import { ZERO_ADDRESS, hexStrToBuf, } from '@eth-optimism/core-utils' -import { CHAIN_ID } from '@eth-optimism/ovm' +import { CHAIN_ID, convertInternalLogsToOvmLogs } from '@eth-optimism/ovm' import { ethers, ContractFactory, Wallet, Contract, utils } from 'ethers' import { resolve } from 'path' @@ -21,6 +21,7 @@ import assert from 'assert' import { FullnodeRpcServer, DefaultWeb3Handler } from '../../src/app' import * as SimpleStorage from '../contracts/build/untranspiled/SimpleStorage.json' import * as EventEmitter from '../contracts/build/untranspiled/EventEmitter.json' +import * as SimpleReversion from '../contracts/build/transpiled/SimpleReversion.json' import { Web3RpcMethods } from '../../src/types' const log = getLogger('web3-handler', true) @@ -115,6 +116,22 @@ export const getUnsignedTransactionCalldata = ( return contract.interface.functions[functionName].encode(args) } +const assertAsyncThrowsWithMessage = async ( + func: () => Promise, + message: string +): Promise => { + let succeeded = true + try { + await func() + succeeded = false + } catch (e) { + if (e.message !== message) { + succeeded = false + } + } + succeeded.should.equal(true, 'Function didn\'t throw as expected or threw with the wrong error message.' ) +} + /********* * TESTS * *********/ @@ -151,6 +168,51 @@ describe('Web3Handler', () => { }) }) + describe.only('EVM reversion handling', async () => { + let wallet + let simpleReversion + beforeEach(async () => { + wallet = getWallet(httpProvider) + const factory = new ContractFactory( + SimpleReversion.abi, + SimpleReversion.bytecode, + wallet + ) + simpleReversion = await factory.deploy() + }) + it('Should propogate generic internal EVM reverts upwards for sendRawTransaction', async () => { + await assertAsyncThrowsWithMessage( + async () => { + await simpleReversion.doRevert() + }, + 'VM Exception while processing transaction: revert' + ) + }) + it('Should propogate solidity require messages upwards for sendRawTransaction', async () => { + const solidityRevertMessage = 'trolololo' + await assertAsyncThrowsWithMessage( + async () => { + await simpleReversion.doRevertWithMessage(solidityRevertMessage) + }, + 'VM Exception while processing transaction: revert ' + solidityRevertMessage + ) + }) + it('Logs should acknowledge the reversion as well', async () => { + // this will fail, but that's fine, we want it to and then check its logs + try { + await simpleReversion.doRevert() + } catch {} + const receipt = ( + await httpProvider.getTransactionReceipt( + '0xade9e00b889b02e994f9ef2d652f3fdb6f34c5862c4a78e959b2b36c79142dc9' + ) + ) + log.debug(`the receipt is: ${JSON.stringify(receipt)}`) + // .map((x) => simpleReversion.interface.parseLog(x)) + }) + + }) + describe('the getBlockByNumber endpoint', () => { it('should return a block with the correct timestamp', async () => { const block = await httpProvider.getBlock('latest') diff --git a/packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol b/packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol new file mode 100644 index 0000000000000..39496a7c55a1d --- /dev/null +++ b/packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol @@ -0,0 +1,10 @@ +pragma solidity ^0.5.0; + +contract SimpleReversion { + function doRevert() public { + revert(); + } + function doRevertWithMessage(string memory _message) public { + require(false, _message); + } +} \ No newline at end of file From 291a4d531be22aecb937e27236c627bc7489a2fd Mon Sep 17 00:00:00 2001 From: ben-chain Date: Fri, 24 Apr 2020 17:54:17 -0400 Subject: [PATCH 3/9] receipts working for failed txs --- packages/ovm/src/app/utils.ts | 24 +++++++----- .../ovm/src/contracts/ExecutionManager.sol | 8 +++- packages/ovm/test/app/utils.spec.ts | 30 ++------------- .../src/app/web3-rpc-handler.ts | 37 +++++++++++++++---- .../test/app/test-web-rpc-handler.spec.ts | 2 +- .../test/app/web-rpc-handler.spec.ts | 30 ++++++--------- 6 files changed, 68 insertions(+), 63 deletions(-) diff --git a/packages/ovm/src/app/utils.ts b/packages/ovm/src/app/utils.ts index 41cc618597ec4..210c921c46a8a 100644 --- a/packages/ovm/src/app/utils.ts +++ b/packages/ovm/src/app/utils.ts @@ -97,7 +97,7 @@ export const convertInternalLogsToOvmLogs = (logs: Log[]): Log[] => { * @param internalTxReceipt the internal transaction receipt * @return ovm transaction metadata */ -export const getOvmTransactionMetadata = ( +export const getSuccessfulOvmTransactionMetadata = ( internalTxReceipt: TransactionReceipt ): OvmTransactionMetadata => { let ovmTo @@ -113,9 +113,6 @@ export const getOvmTransactionMetadata = ( .map((log) => executionManagerInterface.parseLog(log)) .filter((log) => log != null) const callingWithEoaLog = logs.find((log) => log.name === 'CallingWithEOA') - const eoaContractCreatedLog = logs.find( - (log) => log.name === 'EOACreatedContract' - ) const revertEvents: LogDescription[] = logs.filter( (x) => x.name === 'EOACallRevert' @@ -125,9 +122,15 @@ export const getOvmTransactionMetadata = ( if (callingWithEoaLog) { ovmFrom = callingWithEoaLog.values._ovmFromAddress } + + const eoaContractCreatedLog = logs.find( + (log) => log.name === 'EOACreatedContract' + ) if (eoaContractCreatedLog) { ovmCreatedContractAddress = eoaContractCreatedLog.values._ovmContractAddress ovmTo = ovmCreatedContractAddress + } else { + ovmTo = callingWithEoaLog.values._ovmToAddress } const metadata: OvmTransactionMetadata = { @@ -174,20 +177,23 @@ export const internalTxReceiptToOvmTxReceipt = async ( internalTxReceipt: TransactionReceipt, ovmTxHash?: string ): Promise => { - const ovmTransactionMetadata = getOvmTransactionMetadata(internalTxReceipt) + const ovmTransactionMetadata = getSuccessfulOvmTransactionMetadata(internalTxReceipt) // Construct a new receipt // // Start off with the internalTxReceipt const ovmTxReceipt: OvmTransactionReceipt = internalTxReceipt // Add the converted logs ovmTxReceipt.logs = convertInternalLogsToOvmLogs(internalTxReceipt.logs) - // Update the to and from fields - ovmTxReceipt.to = ovmTransactionMetadata.ovmTo + // Update the to and from fields if necessary + if (ovmTransactionMetadata.ovmTo) { + ovmTxReceipt.to = ovmTransactionMetadata.ovmTo + } // TODO: Update this to use some default account abstraction library potentially. ovmTxReceipt.from = ovmTransactionMetadata.ovmFrom // Also update the contractAddress in case we deployed a new contract - ovmTxReceipt.contractAddress = - ovmTransactionMetadata.ovmCreatedContractAddress + ovmTxReceipt.contractAddress = !!ovmTransactionMetadata.ovmCreatedContractAddress + ? ovmTransactionMetadata.ovmCreatedContractAddress + : null ovmTxReceipt.status = ovmTransactionMetadata.ovmTxSucceeded ? 1 : 0 diff --git a/packages/ovm/src/contracts/ExecutionManager.sol b/packages/ovm/src/contracts/ExecutionManager.sol index 0e7d97b35c105..14a92a387b8b1 100644 --- a/packages/ovm/src/contracts/ExecutionManager.sol +++ b/packages/ovm/src/contracts/ExecutionManager.sol @@ -47,7 +47,8 @@ contract ExecutionManager is FullStateManager { bytes32 _codeContractHash ); event CallingWithEOA( - address _ovmFromAddress + address _ovmFromAddress, + address _ovmToAddress ); event EOACreatedContract( address _ovmContractAddress @@ -137,7 +138,10 @@ contract ExecutionManager is FullStateManager { require(eoaAddress != ZERO_ADDRESS, "Failed to recover signature"); // Require nonce to be correct require(_nonce == getOvmContractNonce(eoaAddress), "Incorrect nonce!"); - emit CallingWithEOA(eoaAddress); + emit CallingWithEOA( + eoaAddress, + _ovmEntrypoint + ); // Make the EOA call for the account executeTransaction(_timestamp, _queueOrigin, _ovmEntrypoint, _callBytes, eoaAddress, ZERO_ADDRESS, false); } diff --git a/packages/ovm/test/app/utils.spec.ts b/packages/ovm/test/app/utils.spec.ts index 4c55d7ccaa60a..95b83878b1d64 100644 --- a/packages/ovm/test/app/utils.spec.ts +++ b/packages/ovm/test/app/utils.spec.ts @@ -10,7 +10,7 @@ import { import { TransactionReceipt } from 'ethers/providers' import { convertInternalLogsToOvmLogs, - getOvmTransactionMetadata, + getSuccessfulOvmTransactionMetadata, OvmTransactionMetadata, revertMessagePrefix, } from '../../src/app' @@ -43,13 +43,13 @@ describe('convertInternalLogsToOvmLogs', () => { }) }) -describe('getOvmTransactionMetadata', () => { +describe('getSuccessfulOvmTransactionMetadata', () => { it('should return transaction metadata from calls from externally owned accounts', async () => { const transactionReceipt: TransactionReceipt = { byzantium: true, logs: [ [EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [ALICE]], - [EXECUTION_MANAGER_ADDRESS, 'CallingWithEOA(address)', [ALICE]], + [EXECUTION_MANAGER_ADDRESS, 'CallingWithEOA(address,address)', [ALICE,CONTRACT]], [EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [ALICE]], [EXECUTION_MANAGER_ADDRESS, 'EOACreatedContract(address)', [CONTRACT]], [EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [CONTRACT]], @@ -61,33 +61,11 @@ describe('getOvmTransactionMetadata', () => { ].map((args) => buildLog.apply(null, args)), } - getOvmTransactionMetadata(transactionReceipt).should.deep.eq({ + getSuccessfulOvmTransactionMetadata(transactionReceipt).should.deep.eq({ ovmCreatedContractAddress: CONTRACT, ovmFrom: ALICE, ovmTo: CONTRACT, ovmTxSucceeded: true, }) }) - - it('should return with ovmTxSucceeded equal to false if the transaction reverted', async () => { - const revertMessage: string = 'The tx reverted!' - const msgHex: string = add0x( - Buffer.from(revertMessage, 'utf8').toString('hex') - ) - const encodedMessage: string = abi.encode(['bytes'], [msgHex]) - // needs 4 bytes of sighash - const eventData: string = add0x('ab'.repeat(4) + remove0x(encodedMessage)) - const transactionReceipt: TransactionReceipt = { - byzantium: true, - logs: [ - [EXECUTION_MANAGER_ADDRESS, 'EOACallRevert(bytes)', [eventData]], - ].map((args) => buildLog.apply(null, args)), - } - - const metadata: OvmTransactionMetadata = getOvmTransactionMetadata( - transactionReceipt - ) - metadata.ovmTxSucceeded.should.eq(false) - metadata.revertMessage.should.eq(revertMessagePrefix + revertMessage) - }) }) diff --git a/packages/rollup-full-node/src/app/web3-rpc-handler.ts b/packages/rollup-full-node/src/app/web3-rpc-handler.ts index 4398a663a7553..daab028f8db4f 100644 --- a/packages/rollup-full-node/src/app/web3-rpc-handler.ts +++ b/packages/rollup-full-node/src/app/web3-rpc-handler.ts @@ -376,6 +376,9 @@ export class DefaultWeb3Handler ? ovmTx[key].toNumber() : ovmTx[key] } + if (typeof transaction[key] === 'number') { + transaction[key] = numberToHexString(transaction[key]) + } }) return transaction @@ -537,18 +540,38 @@ export class DefaultWeb3Handler return null } - // Now let's parse the internal transaction reciept - const ovmTxReceipt: OvmTransactionReceipt = await internalTxReceiptToOvmTxReceipt( - internalTxReceipt, - ovmTxHash - ) + log.debug(`Converting internal tx receipt to ovm receipt, internal receipt is:`, internalTxReceipt) + + // if there are no logs, the tx must have failed, as the Execution Mgr always logs stuff + const txSucceeded: boolean = internalTxReceipt.logs.length !== 0 + let ovmTxReceipt + if (txSucceeded) { + log.debug(`The internal tx previously succeeded for this OVM tx, converting internal receipt to OVM receipt...`) + ovmTxReceipt = await internalTxReceiptToOvmTxReceipt( + internalTxReceipt, + ovmTxHash + ) + } else { + log.debug(`Internal tx previously failed for this OVM tx, creating receipt from the OVM tx itself.`) + const rawOvmTx = await this.getOvmTransactionByHash(ovmTxHash) + const ovmTx = utils.parseTransaction(rawOvmTx) + // for a failing tx, everything is identical between the internal and external receipts, except to and from + ovmTxReceipt = internalTxReceipt + ovmTxReceipt.from = ovmTx.from + ovmTxReceipt.to = ovmTx.to + } + if (ovmTxReceipt.revertMessage !== undefined && !includeRevertMessage) { delete ovmTxReceipt.revertMessage } + if (typeof ovmTxReceipt.status === 'number') { + ovmTxReceipt.status = numberToHexString(ovmTxReceipt.status) + } + log.debug( `Returning tx receipt for ovm tx hash [${ovmTxHash}]: [${JSON.stringify( - internalTxReceipt + ovmTxReceipt )}]` ) return ovmTxReceipt @@ -705,7 +728,7 @@ export class DefaultWeb3Handler ) throw e } - log.debug(`L1 to L2 Transaction mined. Tx hash: ${receipt.hash}`) + log.debug(`L1 to L2 Transaction applied to L2. Tx hash: ${receipt.hash}`) try { const ovmTxReceipt: OvmTransactionReceipt = await internalTxReceiptToOvmTxReceipt( diff --git a/packages/rollup-full-node/test/app/test-web-rpc-handler.spec.ts b/packages/rollup-full-node/test/app/test-web-rpc-handler.spec.ts index 459ab7d560b40..a46afb7052ec4 100644 --- a/packages/rollup-full-node/test/app/test-web-rpc-handler.spec.ts +++ b/packages/rollup-full-node/test/app/test-web-rpc-handler.spec.ts @@ -22,7 +22,7 @@ import { import * as SimpleStorage from '../contracts/build/untranspiled/SimpleStorage.json' import * as EmptyContract from '../contracts/build/untranspiled/EmptyContract.json' import * as CallerStorer from '../contracts/build/transpiled/CallerStorer.json' -import { getOvmTransactionMetadata } from '@eth-optimism/ovm' +import { getSuccessfulOvmTransactionMetadata } from '@eth-optimism/ovm' const log = getLogger('test-web3-handler', true) diff --git a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts index c1b24c3b98565..62ef188331147 100644 --- a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts +++ b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts @@ -129,7 +129,10 @@ const assertAsyncThrowsWithMessage = async ( succeeded = false } } - succeeded.should.equal(true, 'Function didn\'t throw as expected or threw with the wrong error message.' ) + succeeded.should.equal( + true, + "Function didn't throw as expected or threw with the wrong error message." + ) } /********* @@ -181,36 +184,27 @@ describe('Web3Handler', () => { simpleReversion = await factory.deploy() }) it('Should propogate generic internal EVM reverts upwards for sendRawTransaction', async () => { - await assertAsyncThrowsWithMessage( - async () => { - await simpleReversion.doRevert() - }, - 'VM Exception while processing transaction: revert' - ) + await assertAsyncThrowsWithMessage(async () => { + await simpleReversion.doRevert() + }, 'VM Exception while processing transaction: revert') }) it('Should propogate solidity require messages upwards for sendRawTransaction', async () => { const solidityRevertMessage = 'trolololo' - await assertAsyncThrowsWithMessage( - async () => { - await simpleReversion.doRevertWithMessage(solidityRevertMessage) - }, - 'VM Exception while processing transaction: revert ' + solidityRevertMessage - ) + await assertAsyncThrowsWithMessage(async () => { + await simpleReversion.doRevertWithMessage(solidityRevertMessage) + }, 'VM Exception while processing transaction: revert ' + solidityRevertMessage) }) it('Logs should acknowledge the reversion as well', async () => { // this will fail, but that's fine, we want it to and then check its logs try { await simpleReversion.doRevert() } catch {} - const receipt = ( - await httpProvider.getTransactionReceipt( - '0xade9e00b889b02e994f9ef2d652f3fdb6f34c5862c4a78e959b2b36c79142dc9' - ) + const receipt = await httpProvider.getTransactionReceipt( + '0xade9e00b889b02e994f9ef2d652f3fdb6f34c5862c4a78e959b2b36c79142dc9' ) log.debug(`the receipt is: ${JSON.stringify(receipt)}`) // .map((x) => simpleReversion.interface.parseLog(x)) }) - }) describe('the getBlockByNumber endpoint', () => { From f5cb2a01145257c9683b502f480cf01bf36c6feb Mon Sep 17 00:00:00 2001 From: ben-chain Date: Fri, 24 Apr 2020 18:50:27 -0400 Subject: [PATCH 4/9] test receipt generation for reverting txns' --- .../src/app/web3-rpc-handler.ts | 2 +- .../test/app/test-web-rpc-handler.spec.ts | 1 - .../test/app/web-rpc-handler.spec.ts | 40 ++++++++++++++----- 3 files changed, 30 insertions(+), 13 deletions(-) diff --git a/packages/rollup-full-node/src/app/web3-rpc-handler.ts b/packages/rollup-full-node/src/app/web3-rpc-handler.ts index daab028f8db4f..a7294d64e52d7 100644 --- a/packages/rollup-full-node/src/app/web3-rpc-handler.ts +++ b/packages/rollup-full-node/src/app/web3-rpc-handler.ts @@ -828,7 +828,7 @@ export class DefaultWeb3Handler .waitForTransaction(res.hash) .then((receipt) => { log.debug( - `Got receipt mapping ${ovmTxHash} to ${internalTxHash}: ${JSON.stringify( + `Got receipt mapping ovm tx hash ${ovmTxHash} to internal tx hash ${internalTxHash}: ${JSON.stringify( receipt )}` ) diff --git a/packages/rollup-full-node/test/app/test-web-rpc-handler.spec.ts b/packages/rollup-full-node/test/app/test-web-rpc-handler.spec.ts index a46afb7052ec4..10ae540a9864e 100644 --- a/packages/rollup-full-node/test/app/test-web-rpc-handler.spec.ts +++ b/packages/rollup-full-node/test/app/test-web-rpc-handler.spec.ts @@ -22,7 +22,6 @@ import { import * as SimpleStorage from '../contracts/build/untranspiled/SimpleStorage.json' import * as EmptyContract from '../contracts/build/untranspiled/EmptyContract.json' import * as CallerStorer from '../contracts/build/transpiled/CallerStorer.json' -import { getSuccessfulOvmTransactionMetadata } from '@eth-optimism/ovm' const log = getLogger('test-web3-handler', true) diff --git a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts index 62ef188331147..d2e9c3db0a6bd 100644 --- a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts +++ b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts @@ -11,7 +11,7 @@ import { } from '@eth-optimism/core-utils' import { CHAIN_ID, convertInternalLogsToOvmLogs } from '@eth-optimism/ovm' -import { ethers, ContractFactory, Wallet, Contract, utils } from 'ethers' +import { ethers, ContractFactory, Wallet, Contract, utils, providers } from 'ethers' import { resolve } from 'path' import * as rimraf from 'rimraf' import * as fs from 'fs' @@ -33,6 +33,8 @@ const port = 9999 const storageKey = '0x' + '01'.repeat(32) const storageValue = '0x' + '02'.repeat(32) +const EVM_REVERT_MSG = 'VM Exception while processing transaction: revert' + const tmpFilePath = resolve(__dirname, `./.test_db`) const getWallet = (httpProvider) => { @@ -171,7 +173,7 @@ describe('Web3Handler', () => { }) }) - describe.only('EVM reversion handling', async () => { + describe('EVM reversion handling', async () => { let wallet let simpleReversion beforeEach(async () => { @@ -186,24 +188,40 @@ describe('Web3Handler', () => { it('Should propogate generic internal EVM reverts upwards for sendRawTransaction', async () => { await assertAsyncThrowsWithMessage(async () => { await simpleReversion.doRevert() - }, 'VM Exception while processing transaction: revert') + }, EVM_REVERT_MSG) }) it('Should propogate solidity require messages upwards for sendRawTransaction', async () => { const solidityRevertMessage = 'trolololo' await assertAsyncThrowsWithMessage(async () => { await simpleReversion.doRevertWithMessage(solidityRevertMessage) - }, 'VM Exception while processing transaction: revert ' + solidityRevertMessage) + }, EVM_REVERT_MSG + ' ' + solidityRevertMessage) }) - it('Logs should acknowledge the reversion as well', async () => { - // this will fail, but that's fine, we want it to and then check its logs + it('Should serve receipts for reverting transactions', async () => { + const revertingTx = { + nonce: await wallet.getTransactionCount(), + gasPrice: 0, + gasLimit: 9999999999, + to: simpleReversion.address, + chainId: CHAIN_ID, + data: simpleReversion.interface.functions[ + 'doRevert' + ].encode([]) + } + const signedTx = await wallet.sign(revertingTx) + const txHash = ethers.utils.keccak256(signedTx) try { - await simpleReversion.doRevert() - } catch {} + await httpProvider.send( + 'eth_sendRawTransaction', + [signedTx] + ) + } catch(e) { + e.message.should.equal(EVM_REVERT_MSG, 'expected EVM revert but got some other error!') + } const receipt = await httpProvider.getTransactionReceipt( - '0xade9e00b889b02e994f9ef2d652f3fdb6f34c5862c4a78e959b2b36c79142dc9' + txHash ) - log.debug(`the receipt is: ${JSON.stringify(receipt)}`) - // .map((x) => simpleReversion.interface.parseLog(x)) + receipt.from.should.equal(wallet.address) + receipt.to.should.equal(simpleReversion.address) }) }) From 60afc7489af466078459a9890f67a16295f3fb63 Mon Sep 17 00:00:00 2001 From: ben-chain Date: Fri, 24 Apr 2020 19:15:58 -0400 Subject: [PATCH 5/9] bugfix, linting --- packages/ovm/src/app/utils.ts | 7 ++--- packages/ovm/test/app/utils.spec.ts | 6 ++++- .../src/app/web3-rpc-handler.ts | 15 ++++++++--- .../test/app/web-rpc-handler.spec.ts | 27 ++++++++++--------- 4 files changed, 35 insertions(+), 20 deletions(-) diff --git a/packages/ovm/src/app/utils.ts b/packages/ovm/src/app/utils.ts index 210c921c46a8a..c000794aa4095 100644 --- a/packages/ovm/src/app/utils.ts +++ b/packages/ovm/src/app/utils.ts @@ -121,6 +121,7 @@ export const getSuccessfulOvmTransactionMetadata = ( if (callingWithEoaLog) { ovmFrom = callingWithEoaLog.values._ovmFromAddress + ovmTo = callingWithEoaLog.values._ovmToAddress } const eoaContractCreatedLog = logs.find( @@ -129,8 +130,6 @@ export const getSuccessfulOvmTransactionMetadata = ( if (eoaContractCreatedLog) { ovmCreatedContractAddress = eoaContractCreatedLog.values._ovmContractAddress ovmTo = ovmCreatedContractAddress - } else { - ovmTo = callingWithEoaLog.values._ovmToAddress } const metadata: OvmTransactionMetadata = { @@ -177,7 +176,9 @@ export const internalTxReceiptToOvmTxReceipt = async ( internalTxReceipt: TransactionReceipt, ovmTxHash?: string ): Promise => { - const ovmTransactionMetadata = getSuccessfulOvmTransactionMetadata(internalTxReceipt) + const ovmTransactionMetadata = getSuccessfulOvmTransactionMetadata( + internalTxReceipt + ) // Construct a new receipt // // Start off with the internalTxReceipt diff --git a/packages/ovm/test/app/utils.spec.ts b/packages/ovm/test/app/utils.spec.ts index 95b83878b1d64..5ca2170e75aa4 100644 --- a/packages/ovm/test/app/utils.spec.ts +++ b/packages/ovm/test/app/utils.spec.ts @@ -49,7 +49,11 @@ describe('getSuccessfulOvmTransactionMetadata', () => { byzantium: true, logs: [ [EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [ALICE]], - [EXECUTION_MANAGER_ADDRESS, 'CallingWithEOA(address,address)', [ALICE,CONTRACT]], + [ + EXECUTION_MANAGER_ADDRESS, + 'CallingWithEOA(address,address)', + [ALICE, CONTRACT], + ], [EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [ALICE]], [EXECUTION_MANAGER_ADDRESS, 'EOACreatedContract(address)', [CONTRACT]], [EXECUTION_MANAGER_ADDRESS, 'ActiveContract(address)', [CONTRACT]], diff --git a/packages/rollup-full-node/src/app/web3-rpc-handler.ts b/packages/rollup-full-node/src/app/web3-rpc-handler.ts index a7294d64e52d7..cfa178ac15a53 100644 --- a/packages/rollup-full-node/src/app/web3-rpc-handler.ts +++ b/packages/rollup-full-node/src/app/web3-rpc-handler.ts @@ -540,19 +540,26 @@ export class DefaultWeb3Handler return null } - log.debug(`Converting internal tx receipt to ovm receipt, internal receipt is:`, internalTxReceipt) - + log.debug( + `Converting internal tx receipt to ovm receipt, internal receipt is:`, + internalTxReceipt + ) + // if there are no logs, the tx must have failed, as the Execution Mgr always logs stuff const txSucceeded: boolean = internalTxReceipt.logs.length !== 0 let ovmTxReceipt if (txSucceeded) { - log.debug(`The internal tx previously succeeded for this OVM tx, converting internal receipt to OVM receipt...`) + log.debug( + `The internal tx previously succeeded for this OVM tx, converting internal receipt to OVM receipt...` + ) ovmTxReceipt = await internalTxReceiptToOvmTxReceipt( internalTxReceipt, ovmTxHash ) } else { - log.debug(`Internal tx previously failed for this OVM tx, creating receipt from the OVM tx itself.`) + log.debug( + `Internal tx previously failed for this OVM tx, creating receipt from the OVM tx itself.` + ) const rawOvmTx = await this.getOvmTransactionByHash(ovmTxHash) const ovmTx = utils.parseTransaction(rawOvmTx) // for a failing tx, everything is identical between the internal and external receipts, except to and from diff --git a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts index d2e9c3db0a6bd..6b0f8cd920be8 100644 --- a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts +++ b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts @@ -11,7 +11,14 @@ import { } from '@eth-optimism/core-utils' import { CHAIN_ID, convertInternalLogsToOvmLogs } from '@eth-optimism/ovm' -import { ethers, ContractFactory, Wallet, Contract, utils, providers } from 'ethers' +import { + ethers, + ContractFactory, + Wallet, + Contract, + utils, + providers, +} from 'ethers' import { resolve } from 'path' import * as rimraf from 'rimraf' import * as fs from 'fs' @@ -203,23 +210,19 @@ describe('Web3Handler', () => { gasLimit: 9999999999, to: simpleReversion.address, chainId: CHAIN_ID, - data: simpleReversion.interface.functions[ - 'doRevert' - ].encode([]) + data: simpleReversion.interface.functions['doRevert'].encode([]), } const signedTx = await wallet.sign(revertingTx) const txHash = ethers.utils.keccak256(signedTx) try { - await httpProvider.send( - 'eth_sendRawTransaction', - [signedTx] + await httpProvider.send('eth_sendRawTransaction', [signedTx]) + } catch (e) { + e.message.should.equal( + EVM_REVERT_MSG, + 'expected EVM revert but got some other error!' ) - } catch(e) { - e.message.should.equal(EVM_REVERT_MSG, 'expected EVM revert but got some other error!') } - const receipt = await httpProvider.getTransactionReceipt( - txHash - ) + const receipt = await httpProvider.getTransactionReceipt(txHash) receipt.from.should.equal(wallet.address) receipt.to.should.equal(simpleReversion.address) }) From 038c2ff80eff5645ebcbf739ea386997d729693f Mon Sep 17 00:00:00 2001 From: ben-chain Date: Sat, 25 Apr 2020 14:59:11 -0400 Subject: [PATCH 6/9] fix eth_call reversion errors --- .../rollup-full-node/src/app/utils/l2-node.ts | 14 ++++++++++ .../src/app/web3-rpc-handler.ts | 27 +++++++++++++------ .../test/app/web-rpc-handler.spec.ts | 16 ++++++++--- .../contracts/transpiled/SimpleReversion.sol | 6 +++++ 4 files changed, 52 insertions(+), 11 deletions(-) diff --git a/packages/rollup-full-node/src/app/utils/l2-node.ts b/packages/rollup-full-node/src/app/utils/l2-node.ts index 0e5994cc82d1a..40d1849967b00 100644 --- a/packages/rollup-full-node/src/app/utils/l2-node.ts +++ b/packages/rollup-full-node/src/app/utils/l2-node.ts @@ -212,3 +212,17 @@ function getL2ToL1MessagePasserContract(wallet: Wallet): Contract { wallet ) } + +/** + * Detects whether an internal L2 node error is due to an EVM revert or some other error + * + * @param e The error message reterned from either eth_sendRawTransaction or eth_call on the internal L2 node + * @returns Whether the error is an EVM revert error or some other issue. + */ +export function isErrorEVMRevert(e: any): boolean { + return ( + !!e.results && + !!Object.keys(e.results)[0] && + e.results[Object.keys(e.results)[0]].error === 'revert' + ) +} diff --git a/packages/rollup-full-node/src/app/web3-rpc-handler.ts b/packages/rollup-full-node/src/app/web3-rpc-handler.ts index cfa178ac15a53..25e855b4c8e54 100644 --- a/packages/rollup-full-node/src/app/web3-rpc-handler.ts +++ b/packages/rollup-full-node/src/app/web3-rpc-handler.ts @@ -43,7 +43,7 @@ import { Web3RpcMethods, RevertError, } from '../types' -import { initializeL2Node, getCurrentTime } from './utils' +import { initializeL2Node, getCurrentTime, isErrorEVMRevert } from './utils' import { NoOpL2ToL1MessageSubmitter } from './message-submitter' const log = getLogger('web3-handler') @@ -242,10 +242,17 @@ export class DefaultWeb3Handler ]) } catch (e) { log.debug( - `Error executing call: ${JSON.stringify( + `Internal error executing call: ${JSON.stringify( txObject )}, default block: ${defaultBlock}, error: ${JSON.stringify(e)}` ) + console.log('here') + if (isErrorEVMRevert(e)) { + log.debug( + `Internal error appears to be an EVM revert, surfacing revert message up...` + ) + throw new RevertError(e.message as string) + } throw e } @@ -645,16 +652,20 @@ export class DefaultWeb3Handler [internalTx] ) } catch (e) { + if (isErrorEVMRevert(e)) { + log.debug( + `Internal EVM revert for Ovm tx hash: ${ovmTxHash} and internal hash: ${internalTxHash}. Incrementing nonce Incrementing nonce for sender (${ovmTx.from}) and surfacing revert message up...` + ) + await this.context.executionManager.incrementNonce(add0x(ovmTx.from)) + log.debug(`Nonce incremented successfully for ${ovmTx.from}.`) + throw new RevertError(e.message as string) + } logError( log, - `Error executing internal transaction!\n\nIncrementing nonce for sender (${ovmTx.from} and returning failed tx hash. Ovm tx hash: ${ovmTxHash}, internal hash: ${internalTxHash}.`, + `Non-revert error executing internal transaction! Ovm tx hash: ${ovmTxHash}, internal hash: ${internalTxHash}. Returning generic internal error.`, e ) - - await this.context.executionManager.incrementNonce(add0x(ovmTx.from)) - log.debug(`Nonce incremented successfully for ${ovmTx.from}.`) - - throw new RevertError(e.message as string) + throw e } if (remove0x(internalTxHash) !== remove0x(returnedInternalTxHash)) { diff --git a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts index 6b0f8cd920be8..d3454892c20be 100644 --- a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts +++ b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts @@ -183,6 +183,7 @@ describe('Web3Handler', () => { describe('EVM reversion handling', async () => { let wallet let simpleReversion + const solidityRevertMessage = 'trolololo' beforeEach(async () => { wallet = getWallet(httpProvider) const factory = new ContractFactory( @@ -192,13 +193,12 @@ describe('Web3Handler', () => { ) simpleReversion = await factory.deploy() }) - it('Should propogate generic internal EVM reverts upwards for sendRawTransaction', async () => { + it('Should propogate generic internal EVM reverts upwards for eth_sendRawTransaction', async () => { await assertAsyncThrowsWithMessage(async () => { await simpleReversion.doRevert() }, EVM_REVERT_MSG) }) - it('Should propogate solidity require messages upwards for sendRawTransaction', async () => { - const solidityRevertMessage = 'trolololo' + it('Should propogate solidity require messages upwards for eth_sendRawTransaction', async () => { await assertAsyncThrowsWithMessage(async () => { await simpleReversion.doRevertWithMessage(solidityRevertMessage) }, EVM_REVERT_MSG + ' ' + solidityRevertMessage) @@ -226,6 +226,16 @@ describe('Web3Handler', () => { receipt.from.should.equal(wallet.address) receipt.to.should.equal(simpleReversion.address) }) + it('Should propogate generic EVM reverts for eth_call', async () => { + await assertAsyncThrowsWithMessage(async () => { + await simpleReversion.doRevertPure() + }, EVM_REVERT_MSG) + }) + it('Should propogate custom message EVM reverts for eth_call', async () => { + await assertAsyncThrowsWithMessage(async () => { + await simpleReversion.doRevertWithMessagePure(solidityRevertMessage) + }, EVM_REVERT_MSG + ' ' + solidityRevertMessage) + }) }) describe('the getBlockByNumber endpoint', () => { diff --git a/packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol b/packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol index 39496a7c55a1d..df9d32d556da8 100644 --- a/packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol +++ b/packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol @@ -7,4 +7,10 @@ contract SimpleReversion { function doRevertWithMessage(string memory _message) public { require(false, _message); } + function doRevertPure() public pure { + revert(); + } + function doRevertWithMessagePure(string memory _message) public pure { + require(false, _message); + } } \ No newline at end of file From 9ce3e33729c257d40ce4e2c880a80a79f62cc3e4 Mon Sep 17 00:00:00 2001 From: ben-chain Date: Sat, 25 Apr 2020 15:04:21 -0400 Subject: [PATCH 7/9] linting --- packages/rollup-full-node/src/app/web3-rpc-handler.ts | 1 - 1 file changed, 1 deletion(-) diff --git a/packages/rollup-full-node/src/app/web3-rpc-handler.ts b/packages/rollup-full-node/src/app/web3-rpc-handler.ts index 25e855b4c8e54..e325e904a391b 100644 --- a/packages/rollup-full-node/src/app/web3-rpc-handler.ts +++ b/packages/rollup-full-node/src/app/web3-rpc-handler.ts @@ -246,7 +246,6 @@ export class DefaultWeb3Handler txObject )}, default block: ${defaultBlock}, error: ${JSON.stringify(e)}` ) - console.log('here') if (isErrorEVMRevert(e)) { log.debug( `Internal error appears to be an EVM revert, surfacing revert message up...` From 56da0020ccc02ee1f5d46dc587da38e0dfc1cd43 Mon Sep 17 00:00:00 2001 From: Karl Floersch Date: Sun, 26 Apr 2020 13:18:45 -0400 Subject: [PATCH 8/9] Add nonce incrementing test --- .../test/app/web-rpc-handler.spec.ts | 23 +++++++++++++++++++ 1 file changed, 23 insertions(+) diff --git a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts index d3454892c20be..68d2d020a902a 100644 --- a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts +++ b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts @@ -203,6 +203,29 @@ describe('Web3Handler', () => { await simpleReversion.doRevertWithMessage(solidityRevertMessage) }, EVM_REVERT_MSG + ' ' + solidityRevertMessage) }) + it('Should increment the nonce after a revert', async () => { + const beforeNonce = await httpProvider.getTransactionCount( + wallet.address + ) + let didError = false + try { + await simpleReversion.doRevertWithMessage(solidityRevertMessage) + } catch (e) { + didError = true + } + didError.should.equal( + true, + 'Expected doRevertWithMessage(...) to throw!' + ) + const afterNonce = await httpProvider.getTransactionCount( + wallet.address + ) + + afterNonce.should.equal( + beforeNonce + 1, + 'Expected the nonce to be incremented by 1!' + ) + }) it('Should serve receipts for reverting transactions', async () => { const revertingTx = { nonce: await wallet.getTransactionCount(), From cbc0ba9ba674351117c45d2112dce152cf75eabd Mon Sep 17 00:00:00 2001 From: Ben Jones Date: Sun, 26 Apr 2020 13:20:17 -0400 Subject: [PATCH 9/9] Clean up unused code / whitespace --- packages/rollup-full-node/test/app/web-rpc-handler.spec.ts | 2 +- .../test/contracts/transpiled/SimpleReversion.sol | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-) diff --git a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts index 68d2d020a902a..8be7f46d33035 100644 --- a/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts +++ b/packages/rollup-full-node/test/app/web-rpc-handler.spec.ts @@ -9,7 +9,7 @@ import { ZERO_ADDRESS, hexStrToBuf, } from '@eth-optimism/core-utils' -import { CHAIN_ID, convertInternalLogsToOvmLogs } from '@eth-optimism/ovm' +import { CHAIN_ID } from '@eth-optimism/ovm' import { ethers, diff --git a/packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol b/packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol index df9d32d556da8..95fbfa1c49e44 100644 --- a/packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol +++ b/packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol @@ -10,7 +10,7 @@ contract SimpleReversion { function doRevertPure() public pure { revert(); } - function doRevertWithMessagePure(string memory _message) public pure { + function doRevertWithMessagePure(string memory _message) public pure { require(false, _message); } -} \ No newline at end of file +}