fix[l2geth]: have successful deposits return a status of 1 (#937)

* fix[l2geth]: have successful deposits return a status of 1

* chore: add changeset

* Update evm.go

* correctly handle reverts and add extra tests

* various tweaks and have watcher handle failed txs

* chore: add changeset

* add tests for 42 and dead addresses

.changeset/afraid-trainers-wonder.md

+---
+'@eth-optimism/core-utils': patch
+---
+
+Have watcher correctly handle failed L1 => L2 messages

.changeset/blue-fishes-sniff.md

+---
+'@eth-optimism/integration-tests': patch
+'@eth-optimism/l2geth': patch
+---
+
+Fix to ensure that L1 => L2 success status is reflected correctly in receipts

integration-tests/test/basic-l1-l2-communication.spec.ts

 import { expect } from 'chai'
 
 /* Imports: External */
-import { Contract, ContractFactory, utils } from 'ethers'
+import { Contract, ContractFactory } from 'ethers'
+import { predeploys, getContractInterface } from '@eth-optimism/contracts'
 import { Direction } from './shared/watcher-utils'
 
 /* Imports: Internal */
 import l1SimpleStorageJson from '../artifacts/contracts/SimpleStorage.sol/SimpleStorage.json'
 import l2SimpleStorageJson from '../artifacts-ovm/contracts/SimpleStorage.sol/SimpleStorage.json'
+import l2ReverterJson from '../artifacts-ovm/contracts/Reverter.sol/Reverter.json'
 import { OptimismEnv } from './shared/env'
 
 describe('Basic L1<>L2 Communication', async () => {
   let Factory__L1SimpleStorage: ContractFactory
   let Factory__L2SimpleStorage: ContractFactory
+  let Factory__L2Reverter: ContractFactory
   let L1SimpleStorage: Contract
   let L2SimpleStorage: Contract
+  let L2Reverter: Contract
   let env: OptimismEnv
 
   before(async () => {
@@ -28,6 +32,11 @@ describe('Basic L1<>L2 Communication', async () => {
       l2SimpleStorageJson.bytecode,
       env.l2Wallet
     )
+    Factory__L2Reverter = new ContractFactory(
+      l2ReverterJson.abi,
+      l2ReverterJson.bytecode,
+      env.l2Wallet
+    )
   })
 
   beforeEach(async () => {
@@ -35,41 +44,122 @@ describe('Basic L1<>L2 Communication', async () => {
     await L1SimpleStorage.deployTransaction.wait()
     L2SimpleStorage = await Factory__L2SimpleStorage.deploy()
     await L2SimpleStorage.deployTransaction.wait()
+    L2Reverter = await Factory__L2Reverter.deploy()
+    await L2Reverter.deployTransaction.wait()
   })
 
-  it('should withdraw from L2 -> L1', async () => {
-    const value = `0x${'77'.repeat(32)}`
+  describe('L2 => L1', () => {
+    it('should be able to perform a withdrawal from L2 -> L1', async () => {
+      const value = `0x${'77'.repeat(32)}`
 
-    // Send L2 -> L1 message.
-    const transaction = await env.l2Messenger.sendMessage(
-      L1SimpleStorage.address,
-      L1SimpleStorage.interface.encodeFunctionData('setValue', [value]),
-      5000000
-    )
+      // Send L2 -> L1 message.
+      const transaction = await env.l2Messenger.sendMessage(
+        L1SimpleStorage.address,
+        L1SimpleStorage.interface.encodeFunctionData('setValue', [value]),
+        5000000
+      )
 
-    await env.waitForXDomainTransaction(transaction, Direction.L2ToL1)
+      await env.waitForXDomainTransaction(transaction, Direction.L2ToL1)
 
-    expect(await L1SimpleStorage.msgSender()).to.equal(env.l1Messenger.address)
-    expect(await L1SimpleStorage.xDomainSender()).to.equal(env.l2Wallet.address)
-    expect(await L1SimpleStorage.value()).to.equal(value)
-    expect((await L1SimpleStorage.totalCount()).toNumber()).to.equal(1)
+      expect(await L1SimpleStorage.msgSender()).to.equal(
+        env.l1Messenger.address
+      )
+      expect(await L1SimpleStorage.xDomainSender()).to.equal(
+        env.l2Wallet.address
+      )
+      expect(await L1SimpleStorage.value()).to.equal(value)
+      expect((await L1SimpleStorage.totalCount()).toNumber()).to.equal(1)
+    })
   })
 
-  it('should deposit from L1 -> L2', async () => {
-    const value = `0x${'42'.repeat(32)}`
+  describe('L1 => L2', () => {
+    it('should deposit from L1 -> L2', async () => {
+      const value = `0x${'42'.repeat(32)}`
 
-    // Send L1 -> L2 message.
-    const transaction = await env.l1Messenger.sendMessage(
-      L2SimpleStorage.address,
-      L2SimpleStorage.interface.encodeFunctionData('setValue', [value]),
-      5000000
-    )
+      // Send L1 -> L2 message.
+      const transaction = await env.l1Messenger.sendMessage(
+        L2SimpleStorage.address,
+        L2SimpleStorage.interface.encodeFunctionData('setValue', [value]),
+        5000000
+      )
+
+      await env.waitForXDomainTransaction(transaction, Direction.L1ToL2)
+
+      expect(await L2SimpleStorage.msgSender()).to.equal(
+        env.l2Messenger.address
+      )
+      expect(await L2SimpleStorage.xDomainSender()).to.equal(
+        env.l1Wallet.address
+      )
+      expect(await L2SimpleStorage.value()).to.equal(value)
+      expect((await L2SimpleStorage.totalCount()).toNumber()).to.equal(1)
+    })
+
+    it('should have a receipt with a status of 1 for a successful message', async () => {
+      const value = `0x${'42'.repeat(32)}`
+
+      // Send L1 -> L2 message.
+      const transaction = await env.l1Messenger.sendMessage(
+        L2SimpleStorage.address,
+        L2SimpleStorage.interface.encodeFunctionData('setValue', [value]),
+        5000000
+      )
+
+      const { remoteReceipt } = await env.waitForXDomainTransaction(
+        transaction,
+        Direction.L1ToL2
+      )
+
+      expect(remoteReceipt.status).to.equal(1)
+    })
+
+    it('should have a receipt with a status of 0 for a failed message', async () => {
+      // Send L1 -> L2 message.
+      const transaction = await env.l1Messenger.sendMessage(
+        L2Reverter.address,
+        L2Reverter.interface.encodeFunctionData('doRevert', []),
+        5000000
+      )
+
+      const { remoteReceipt } = await env.waitForXDomainTransaction(
+        transaction,
+        Direction.L1ToL2
+      )
+
+      expect(remoteReceipt.status).to.equal(0)
+    })
+
+    it('should have a receipt with a status of 0 for messages sent to 0x42... addresses', async () => {
+      // This call is fine but will give a status of 0.
+      const transaction = await env.l1Messenger.sendMessage(
+        predeploys.Lib_AddressManager,
+        getContractInterface(
+          'Lib_AddressManager'
+        ).encodeFunctionData('getAddress', ['whatever']),
+        5000000
+      )
+
+      const { remoteReceipt } = await env.waitForXDomainTransaction(
+        transaction,
+        Direction.L1ToL2
+      )
+
+      expect(remoteReceipt.status).to.equal(0)
+    })
+
+    it('should have a receipt with a status of 0 for messages sent to 0xdead... addresses', async () => {
+      const transaction = await env.l1Messenger.sendMessage(
+        '0xDeadDeAddeAddEAddeadDEaDDEAdDeaDDeAD0000',
+        '0x',
+        5000000
+      )
 
-    await env.waitForXDomainTransaction(transaction, Direction.L1ToL2)
+      const { remoteReceipt } = await env.waitForXDomainTransaction(
+        transaction,
+        Direction.L1ToL2
+      )
 
-    expect(await L2SimpleStorage.msgSender()).to.equal(env.l2Messenger.address)
-    expect(await L2SimpleStorage.xDomainSender()).to.equal(env.l1Wallet.address)
-    expect(await L2SimpleStorage.value()).to.equal(value)
-    expect((await L2SimpleStorage.totalCount()).toNumber()).to.equal(1)
+      expect(remoteReceipt.status).to.equal(0)
+    })
   })
 })

l2geth/core/vm/evm.go

@@ -17,6 +17,7 @@
 package vm
 
 import (
+	"bytes"
 	"crypto/rand"
 	"encoding/hex"
 	"fmt"
@@ -191,10 +192,14 @@ type Context struct {
 	Difficulty  *big.Int       // Provides information for DIFFICULTY
 
 	// OVM_ADDITION
-	EthCallSender       *common.Address
-	OvmExecutionManager dump.OvmDumpAccount
-	OvmStateManager     dump.OvmDumpAccount
-	OvmSafetyChecker    dump.OvmDumpAccount
+	EthCallSender             *common.Address
+	IsL1ToL2Message           bool
+	IsSuccessfulL1ToL2Message bool
+	OvmExecutionManager       dump.OvmDumpAccount
+	OvmStateManager           dump.OvmDumpAccount
+	OvmSafetyChecker          dump.OvmDumpAccount
+	OvmL2CrossDomainMessenger dump.OvmDumpAccount
+	OvmETH                    dump.OvmDumpAccount
 }
 
 // EVM is the Ethereum Virtual Machine base object and provides
@@ -245,6 +250,8 @@ func NewEVM(ctx Context, statedb StateDB, chainConfig *params.ChainConfig, vmCon
 		ctx.OvmExecutionManager = chainConfig.StateDump.Accounts["OVM_ExecutionManager"]
 		ctx.OvmStateManager = chainConfig.StateDump.Accounts["OVM_StateManager"]
 		ctx.OvmSafetyChecker = chainConfig.StateDump.Accounts["OVM_SafetyChecker"]
+		ctx.OvmL2CrossDomainMessenger = chainConfig.StateDump.Accounts["OVM_L2CrossDomainMessenger"]
+		ctx.OvmETH = chainConfig.StateDump.Accounts["OVM_ETH"]
 	}
 
 	id := make([]byte, 4)
@@ -323,6 +330,19 @@ func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas
 		}
 	}
 
+	// We need to be able to show a status of 1 ("no error") for successful L1 => L2 messages.
+	// The current way we figure out the success of the transaction (by parsing the
+	// returned data) would require an upgrade to the contracts that we likely won't make for a
+	// while. As a result, we'll use this mechanism where we set IsL1ToL2Message = true if
+	// we detect an L1 => L2 message and then IsSuccessfulL1ToL2Message = true if the message is
+	// successfully executed.
+	// Just to be safe (if the evm object ever gets reused), we set both values to false on the
+	// start of a new EVM execution.
+	if evm.depth == 0 {
+		evm.Context.IsL1ToL2Message = false
+		evm.Context.IsSuccessfulL1ToL2Message = false
+	}
+
 	var (
 		to       = AccountRef(addr)
 		snapshot = evm.StateDB.Snapshot()
@@ -369,8 +389,35 @@ func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas
 		}()
 	}
 
+	// Here's where we detect L1 => L2 messages. Based on the current contracts, we're guaranteed
+	// that the target address will only be the OVM_L2CrossDomainMessenger at a depth of 1 if this
+	// is an L1 => L2 message.
+	if evm.depth == 1 && addr == evm.Context.OvmL2CrossDomainMessenger.Address {
+		evm.Context.IsL1ToL2Message = true
+	}
+
 	ret, err = run(evm, contract, input, false)
 
+	// If all of these very particular conditions hold then we're guaranteed to be in a successful
+	// L1 => L2 message. It's not pretty, but it works. Broke this out into a series of checks to
+	// make it a bit more legible.
+	if evm.Context.IsL1ToL2Message && evm.depth == 3 {
+		var isValidMessageTarget = true
+		// 0x420... addresses are not valid targets except for the ETH predeploy.
+		if bytes.HasPrefix(addr.Bytes(), fortyTwoPrefix) && addr != evm.Context.OvmETH.Address {
+			isValidMessageTarget = false
+		}
+		// 0xdead... addresses are not valid targets.
+		if bytes.HasPrefix(addr.Bytes(), deadPrefix) {
+			isValidMessageTarget = false
+		}
+		// As long as this is a valid target and the message didn't revert then we can consider
+		// this a successful L1 => L2 message.
+		if isValidMessageTarget && err == nil {
+			evm.Context.IsSuccessfulL1ToL2Message = true
+		}
+	}
+
 	// When an error was returned by the EVM or when setting the creation code
 	// above we revert to the snapshot and consume any gas remaining. Additionally
 	// when we're in homestead this also counts for code storage gas errors.
@@ -387,30 +434,35 @@ func (evm *EVM) Call(caller ContractRef, addr common.Address, input []byte, gas
 			// We're back at the root-level message call, so we'll need to modify the return data
 			// sent to us by the OVM_ExecutionManager to instead be the intended return data.
 
-			// Attempt to decode the returndata as as ExecutionManager.run when
-			// it is not an `eth_call` and as ExecutionManager.simulateMessage
-			// when it is an `eth_call`. If the data is not decodable as ABI
-			// encoded bytes, then return nothing. If the data is able to be
-			// decoded as bytes, then attempt to decode as (bool, bytes)
-			isDecodable := true
-			returnData := runReturnData{}
-			if err := codec.Unpack(&returnData, "blob", ret); err != nil {
-				isDecodable = false
-			}
+			// We skip the parsing step if this was a successful L1 => L2 message. Note that if err
+			// is set (perhaps because of an error in ExecutionManager.run) we'll still return that
+			// error.
+			if !evm.Context.IsSuccessfulL1ToL2Message {
+				// Attempt to decode the returndata as as ExecutionManager.run when
+				// it is not an `eth_call` and as ExecutionManager.simulateMessage
+				// when it is an `eth_call`. If the data is not decodable as ABI
+				// encoded bytes, then return nothing. If the data is able to be
+				// decoded as bytes, then attempt to decode as (bool, bytes)
+				isDecodable := true
+				returnData := runReturnData{}
+				if err := codec.Unpack(&returnData, "blob", ret); err != nil {
+					isDecodable = false
+				}
 
-			switch isDecodable {
-			case true:
-				inner := innerData{}
-				// If this fails to decode, the nil values will be set in
-				// `inner`, meaning that it will be interpreted as reverted
-				// execution with empty returndata
-				_ = codec.Unpack(&inner, "call", returnData.ReturnData)
-				if !inner.Success {
-					err = errExecutionReverted
+				switch isDecodable {
+				case true:
+					inner := innerData{}
+					// If this fails to decode, the nil values will be set in
+					// `inner`, meaning that it will be interpreted as reverted
+					// execution with empty returndata
+					_ = codec.Unpack(&inner, "call", returnData.ReturnData)
+					if !inner.Success {
+						err = errExecutionReverted
+					}
+					ret = inner.ReturnData
+				case false:
+					ret = []byte{}
 				}
-				ret = inner.ReturnData
-			case false:
-				ret = []byte{}
 			}
 		}
 

packages/core-utils/src/watcher.ts

@@ -75,12 +75,19 @@ export class Watcher {
   ): Promise<TransactionReceipt> {
     const blockNumber = await layer.provider.getBlockNumber()
     const startingBlock = Math.max(blockNumber - this.NUM_BLOCKS_TO_FETCH, 0)
-    const filter = {
+    const successFilter = {
       address: layer.messengerAddress,
       topics: [ethers.utils.id(`RelayedMessage(bytes32)`)],
       fromBlock: startingBlock,
     }
-    const logs = await layer.provider.getLogs(filter)
+    const failureFilter = {
+      address: layer.messengerAddress,
+      topics: [ethers.utils.id(`FailedRelayedMessage(bytes32)`)],
+      fromBlock: startingBlock,
+    }
+    const successLogs = await layer.provider.getLogs(successFilter)
+    const failureLogs = await layer.provider.getLogs(failureFilter)
+    const logs = successLogs.concat(failureLogs)
     const matches = logs.filter((log: any) => log.data === msgHash)
 
     // Message was relayed in the past
@@ -98,19 +105,23 @@ export class Watcher {
 
     // Message has yet to be relayed, poll until it is found
     return new Promise(async (resolve, reject) => {
-      layer.provider.on(filter, async (log: any) => {
+      const handleEvent = async (log: any) => {
         if (log.data === msgHash) {
           try {
             const txReceipt = await layer.provider.getTransactionReceipt(
               log.transactionHash
             )
-            layer.provider.off(filter)
+            layer.provider.off(successFilter)
+            layer.provider.off(failureFilter)
             resolve(txReceipt)
           } catch (e) {
             reject(e)
           }
         }
-      })
+      }
+
+      layer.provider.on(successFilter, handleEvent)
+      layer.provider.on(failureFilter, handleEvent)
     })
   }
 }