diff --git a/packages/ovm/src/app/utils.ts b/packages/ovm/src/app/utils.ts index 41cc618597ec4..c000794aa4095 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' @@ -124,7 +121,12 @@ export const getOvmTransactionMetadata = ( if (callingWithEoaLog) { ovmFrom = callingWithEoaLog.values._ovmFromAddress + ovmTo = callingWithEoaLog.values._ovmToAddress } + + const eoaContractCreatedLog = logs.find( + (log) => log.name === 'EOACreatedContract' + ) if (eoaContractCreatedLog) { ovmCreatedContractAddress = eoaContractCreatedLog.values._ovmContractAddress ovmTo = ovmCreatedContractAddress @@ -174,20 +176,25 @@ 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..5ca2170e75aa4 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,17 @@ 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 +65,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/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 bf608c471ad34..e325e904a391b 100644 --- a/packages/rollup-full-node/src/app/web3-rpc-handler.ts +++ b/packages/rollup-full-node/src/app/web3-rpc-handler.ts @@ -41,8 +41,9 @@ import { UnsupportedMethodError, Web3Handler, 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') @@ -241,10 +242,16 @@ 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)}` ) + 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 } @@ -375,6 +382,9 @@ export class DefaultWeb3Handler ? ovmTx[key].toNumber() : ovmTx[key] } + if (typeof transaction[key] === 'number') { + transaction[key] = numberToHexString(transaction[key]) + } }) return transaction @@ -536,18 +546,45 @@ 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 @@ -614,16 +651,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 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}.`) - - return ovmTxHash + throw e } if (remove0x(internalTxHash) !== remove0x(returnedInternalTxHash)) { @@ -704,7 +745,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( @@ -804,7 +845,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 )}` ) @@ -898,7 +939,7 @@ export class DefaultWeb3Handler ovmTx.data, ovmFrom, ZERO_ADDRESS, - false + true ) log.debug(`EOA calldata: [${internalCalldata}]`) 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..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 { getOvmTransactionMetadata } 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 a5076874c41db..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 @@ -11,7 +11,14 @@ import { } from '@eth-optimism/core-utils' import { CHAIN_ID } 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' @@ -21,6 +28,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) @@ -32,6 +40,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) => { @@ -115,6 +125,25 @@ 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 +180,87 @@ describe('Web3Handler', () => { }) }) + describe('EVM reversion handling', async () => { + let wallet + let simpleReversion + const solidityRevertMessage = 'trolololo' + 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 eth_sendRawTransaction', async () => { + await assertAsyncThrowsWithMessage(async () => { + await simpleReversion.doRevert() + }, EVM_REVERT_MSG) + }) + it('Should propogate solidity require messages upwards for eth_sendRawTransaction', async () => { + await assertAsyncThrowsWithMessage(async () => { + 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(), + 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 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(txHash) + 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', () => { 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..95fbfa1c49e44 --- /dev/null +++ b/packages/rollup-full-node/test/contracts/transpiled/SimpleReversion.sol @@ -0,0 +1,16 @@ +pragma solidity ^0.5.0; + +contract SimpleReversion { + function doRevert() public { + revert(); + } + function doRevertWithMessage(string memory _message) public { + require(false, _message); + } + function doRevertPure() public pure { + revert(); + } + function doRevertWithMessagePure(string memory _message) public pure { + require(false, _message); + } +}