EVM: writeContract fixes, never return contract code as RETURNDATA

This fixes #867 "EIP-170 related consensus error at Goerli block 5080941", and
equivalent on other networks.

This combines a change on the EVM-caller side with an EVM-side change from
@jangko 6548ff9 "fixes CREATE/CREATE2's `returndata` bug", making the caller
EVM ignore any data except from `REVERT`.

Either change works by itself.  The reason for both is to ensure we definitely
comply with ambiguous EVMC expectations from either side of that boundary, and
it makes the internal API clearer.

As well as fixing a specific consensus issue, there are some other EVM logic
changes too: Refactored `writeContract`, how `RETURNDATA` is handled inside the
EVM, and changed behaviour with quirks before EIP-2 (Homestead).

The fix allows sync to pass block 5080941 on Goerli, and probably equivalent on
other networks.  Here's a trace at batch 5080897..5081088:

```
TRC 2021-10-01 21:18:12.883+01:00 Persisting blocks                  file=persist_blocks.nim:43 fromBlock=5080897 toBlock=5081088
...
DBG 2021-10-01 21:18:13.270+01:00 Contract code size exceeds EIP170  topics="vm computation" file=computation.nim:236 limit=24577 actual=31411
DBG 2021-10-01 21:18:13.271+01:00 gasUsed neq cumulativeGasUsed      file=process_block.nim:68 block=5080941/0A3537BC5BDFC637349E1C77D9648F2F65E2BF973ABF7956618F854B769DF626 gasUsed=3129669 cumulativeGasUsed=3132615
TRC 2021-10-01 21:18:13.271+01:00 peer disconnected                  file=blockchain_sync.nim:407 peer=<IP:PORT>
```

Although it says "Contract code size" and "gasUsed", this bug is more general
than either contract size or gas.  It's due to incorrect behaviour of EVM
instructions `RETURNDATA` and `RETURNDATASIZE`.

Sometimes when `writeContract` decides to reject writing the contract for any
of several reasons (for example just insufficient gas), the unwritten contract
code was being used as the "return data", and given to the caller.  If the
caller used `RETURNDATA` or `RETURNDATASIZE` ops, those incorrectly reported
the contract code that didn't get written.

EIP-211 (https://eips.ethereum.org/EIPS/eip-211) describes `RETURNDATA`:
> "`CREATE` and `CREATE2` are considered to return the empty buffer in the
> success case and the failure data in the failure case".

The language is ambiguous.  In fact "failure case" means when the contract uses
`REVERT` to finish.  It doesn't mean other failures like out of gas, EIP-170
limit, EIP-3541, etc.

To be thorough, and to ensure we always do the right thing with real EVMC when
that's finalised, this patch fixes the `RETURNDATA` issue in two places, either
of which make Goerli block 5080941 pass.

`writeContract` has been refactored to be caller, and so has where it's called.
It sets an error in the usual way if contract writing is rejected -- that's
anticipating EVMC, where we'll use different error codes later.

Overall four behaviour changes:

1. On the callee side, it doesn't set `c.outputData` except for `REVERT`.
2. On the caller side, it doesn't read `child.outputData` except for `REVERT`.
3. There was a bug in processing before Homestead fork (EIP-2).  We did not
   match the spec or other implementations; now we do.  When there's
   insufficient gas, before Homestead it's treated as success but with an empty
   contract.

   https://github.com/ethereum/pyethereum/blob/d117c8f3fd93359fc641fd850fa799436f7c43b5/ethereum/processblock.py#L304
   https://github.com/ethereum/go-ethereum/blob/401354976bb4/core/vm/instructions.go#L586

4. The Byzantium check has been removed, as it's unnecessary.

Signed-off-by: Jamie Lokier <[email protected]>

nimbus/vm/computation.nim

@@ -226,20 +226,32 @@ proc rollback*(c: Computation) =
 proc setError*(c: Computation, msg: string, burnsGas = false) {.inline.} =
  c.error = Error(info: msg, burnsGas: burnsGas)
 
-proc writeContract*(c: Computation, fork: Fork): bool {.gcsafe.} =
- result = true
-
- let contractCode = c.output
- if contractCode.len == 0: return
-
- if fork >= FkSpurious and contractCode.len > EIP170_MAX_CODE_SIZE:
- debug "Contract code size exceeds EIP170",
- max = EIP170_MAX_CODE_SIZE, actual = contractCode.len
- return false
-
- if fork >= FkLondon and contractCode[0] == 0xEF.byte:
- debug "Contract code can't start with 0xEF byte"
- return false
+proc writeContract*(c: Computation) =
+ template withExtra(tracer: untyped, args: varargs[untyped]) =
+ tracer args, newContract=($c.msg.contractAddress),
+ blockNumber=c.vmState.blockNumber, blockHash=($c.vmState.blockHash)
+
+ # In each check below, they are guarded by `len > 0`. This includes writing
+ # out the code, because the account already has zero-length code to handle
+ # nested calls (this is specified). May as well simplify other branches.
+ let (len, fork) = (c.output.len, c.fork)
+ if len == 0:
+ return
+
+ # EIP-3541 constraint (https://eips.ethereum.org/EIPS/eip-3541).
+ if fork >= FkLondon and c.output[0] == 0xEF.byte:
+ withExtra trace, "New contract code starts with 0xEF byte, not allowed by EIP-3541"
+ # TODO: Return `EVMC_CONTRACT_VALIDATION_FAILURE` (like Silkworm).
+ c.setError("EVMC_CONTRACT_VALIDATION_FAILURE", true)
+ return
+
+ # EIP-170 constraint (https://eips.ethereum.org/EIPS/eip-3541).
+ if fork >= FkSpurious and len > EIP170_MAX_CODE_SIZE:
+ withExtra trace, "New contract code exceeds EIP-170 limit",
+ codeSize=len, maxSize=EIP170_MAX_CODE_SIZE
+ # TODO: Return `EVMC_OUT_OF_GAS` (like Silkworm).
+ c.setError("EVMC_OUT_OF_GAS", true)
+ return
 
  # Charge gas and write the code even if the code address is self-destructed.
  # Non-empty code in a newly created, self-destructed account is possible if
@@ -248,16 +260,26 @@ proc writeContract*(c: Computation, fork: Fork): bool {.gcsafe.} =
  # gas difference matters. The new code can be called later in the
  # transaction too, before self-destruction wipes the account at the end.
 
- let gasParams = GasParams(kind: Create, cr_memLength: contractCode.len)
+ let gasParams = GasParams(kind: Create, cr_memLength: len)
  let codeCost = c.gasCosts[Create].c_handler(0.u256, gasParams).gasCost
- if c.gasMeter.gasRemaining >= codeCost:
- c.gasMeter.consumeGas(codeCost, reason = "Write contract code for CREATE")
+ if codeCost <= c.gasMeter.gasRemaining:
+ c.gasMeter.consumeGas(codeCost, reason = "Write new contract code")
  c.vmState.mutateStateDb:
- db.setCode(c.msg.contractAddress, contractCode)
- result = true
+ db.setCode(c.msg.contractAddress, c.output)
+ withExtra trace, "Writing new contract code"
+ return
+
+ if fork >= FkHomestead:
+ # EIP-2 (https://eips.ethereum.org/EIPS/eip-2).
+ # TODO: Return `EVMC_OUT_OF_GAS` (like Silkworm).
+ c.setError("EVMC_OUT_OF_GAS", true)
  else:
- if fork < FkHomestead or fork >= FkByzantium: c.output = @[]
- result = false
+ # Before EIP-2, when out of gas for code storage, the account ends up with
+ # zero-length code and no error. No gas is charged. Code cited in EIP-2:
+ # https://github.com/ethereum/pyethereum/blob/d117c8f3fd93/ethereum/processblock.py#L304
+ # https://github.com/ethereum/go-ethereum/blob/401354976bb4/core/vm/instructions.go#L586
+ # The account already has zero-length code to handle nested calls.
+ withExtra trace, "New contract given empty code by pre-Homestead rules"
 
 proc initAddress(x: int): EthAddress {.compileTime.} = result[19] = x.byte
 const ripemdAddr = initAddress(3)
@@ -315,10 +337,13 @@ proc beforeExecCreate(c: Computation): bool =
 
 proc afterExecCreate(c: Computation) =
  if c.isSuccess:
- let fork = c.fork
- let contractFailed = not c.writeContract(fork)
- if contractFailed and fork >= FkHomestead:
- c.setError(&"writeContract failed, depth={c.msg.depth}", true)
+ # This can change `c.isSuccess`.
+ c.writeContract()
+ # Contract code should never be returned to the caller. Only data from
+ # `REVERT` is returned after a create. Clearing in this branch covers the
+ # right cases, particularly important with EVMC where it must be cleared.
+ if c.output.len > 0:
+ c.output = @[]
 
  if c.isSuccess:
  c.commit()

nimbus/vm/interpreter/opcodes_impl.nim

@@ -695,12 +695,12 @@ template genCreate(callName: untyped, opCode: Op): untyped =
  )
 
  c.chainTo(msg):
- c.returnData = @(makeOpenArray(c.res.outputData, c.res.outputSize.int))
  c.gasMeter.returnGas(c.res.gas_left)
-
  if c.res.status_code == EVMC_SUCCESS:
  c.stack.top(c.res.create_address)
-
+ elif c.res.status_code == EVMC_REVERT:
+ # From create, only use `outputData` if child returned with `REVERT`.
+ c.returnData = @(makeOpenArray(c.res.outputData, c.res.outputSize.int))
  if not c.res.release.isNil:
  c.res.release(c.res)
  else:
@@ -717,11 +717,11 @@ template genCreate(callName: untyped, opCode: Op): untyped =
  c.chainTo(child):
  if not child.shouldBurnGas:
  c.gasMeter.returnGas(child.gasMeter.gasRemaining)
-
  if child.isSuccess:
  c.merge(child)
  c.stack.top child.msg.contractAddress
- else:
+ elif not child.error.burnsGas: # Means return was `REVERT`.
+ # From create, only use `outputData` if child returned with `REVERT`.
  c.returnData = child.output
 
 genCreate(create, Create)

nimbus/vm2/computation.nim

@@ -163,20 +163,32 @@ proc rollback*(c: Computation) =
 proc setError*(c: Computation, msg: string, burnsGas = false) {.inline.} =
  c.error = Error(info: msg, burnsGas: burnsGas)
 
-proc writeContract*(c: Computation, fork: Fork): bool {.gcsafe.} =
- result = true
-
- let contractCode = c.output
- if contractCode.len == 0: return
-
- if fork >= FkSpurious and contractCode.len > EIP170_MAX_CODE_SIZE:
- debug "Contract code size exceeds EIP170",
- max = EIP170_MAX_CODE_SIZE, actual = contractCode.len
- return false
-
- if fork >= FkLondon and contractCode[0] == 0xEF.byte:
- debug "Contract code can't start with 0xEF byte"
- return false
+proc writeContract*(c: Computation) =
+ template withExtra(tracer: untyped, args: varargs[untyped]) =
+ tracer args, newContract=($c.msg.contractAddress),
+ blockNumber=c.vmState.blockNumber, blockHash=($c.vmState.blockHash)
+
+ # In each check below, they are guarded by `len > 0`. This includes writing
+ # out the code, because the account already has zero-length code to handle
+ # nested calls (this is specified). May as well simplify other branches.
+ let (len, fork) = (c.output.len, c.fork)
+ if len == 0:
+ return
+
+ # EIP-3541 constraint (https://eips.ethereum.org/EIPS/eip-3541).
+ if fork >= FkLondon and c.output[0] == 0xEF.byte:
+ withExtra trace, "New contract code starts with 0xEF byte, not allowed by EIP-3541"
+ # TODO: Return `EVMC_CONTRACT_VALIDATION_FAILURE` (like Silkworm).
+ c.setError("EVMC_CONTRACT_VALIDATION_FAILURE", true)
+ return
+
+ # EIP-170 constraint (https://eips.ethereum.org/EIPS/eip-3541).
+ if fork >= FkSpurious and len > EIP170_MAX_CODE_SIZE:
+ withExtra trace, "New contract code exceeds EIP-170 limit",
+ codeSize=len, maxSize=EIP170_MAX_CODE_SIZE
+ # TODO: Return `EVMC_OUT_OF_GAS` (like Silkworm).
+ c.setError("EVMC_OUT_OF_GAS", true)
+ return
 
  # Charge gas and write the code even if the code address is self-destructed.
  # Non-empty code in a newly created, self-destructed account is possible if
@@ -185,17 +197,26 @@ proc writeContract*(c: Computation, fork: Fork): bool {.gcsafe.} =
  # gas difference matters. The new code can be called later in the
  # transaction too, before self-destruction wipes the account at the end.
 
- let gasParams = GasParams(kind: Create, cr_memLength: contractCode.len)
+ let gasParams = GasParams(kind: Create, cr_memLength: len)
  let codeCost = c.gasCosts[Create].c_handler(0.u256, gasParams).gasCost
- if c.gasMeter.gasRemaining >= codeCost:
- c.gasMeter.consumeGas(codeCost, reason = "Write contract code for CREATE")
+ if codeCost <= c.gasMeter.gasRemaining:
+ c.gasMeter.consumeGas(codeCost, reason = "Write new contract code")
  c.vmState.mutateStateDb:
- db.setCode(c.msg.contractAddress, contractCode)
- result = true
+ db.setCode(c.msg.contractAddress, c.output)
+ withExtra trace, "Writing new contract code"
+ return
+
+ if fork >= FkHomestead:
+ # EIP-2 (https://eips.ethereum.org/EIPS/eip-2).
+ # TODO: Return `EVMC_OUT_OF_GAS` (like Silkworm).
+ c.setError("EVMC_OUT_OF_GAS", true)
  else:
- if fork < FkHomestead or FkByzantium <= fork:
- c.output = @[]
- result = false
+ # Before EIP-2, when out of gas for code storage, the account ends up with
+ # zero-length code and no error. No gas is charged. Code cited in EIP-2:
+ # https://github.com/ethereum/pyethereum/blob/d117c8f3fd93/ethereum/processblock.py#L304
+ # https://github.com/ethereum/go-ethereum/blob/401354976bb4/core/vm/instructions.go#L586
+ # The account already has zero-length code to handle nested calls.
+ withExtra trace, "New contract given empty code by pre-Homestead rules"
 
 template chainTo*(c, toChild: Computation, after: untyped) =
  c.child = toChild

nimbus/vm2/interpreter/op_handlers/oph_create.nim

@@ -54,7 +54,8 @@ proc execSubCreate(k: var Vm2Ctx; childMsg: Message;
  if child.isSuccess:
  c.merge(child)
  c.stack.top child.msg.contractAddress
- else:
+ elif not child.error.burnsGas: # Means return was `REVERT`.
+ # From create, only use `outputData` if child returned with `REVERT`.
  c.returnData = child.output
 
 # ------------------------------------------------------------------------------

nimbus/vm2/interpreter_dispatch.nim

@@ -159,10 +159,13 @@ proc beforeExecCreate(c: Computation): bool =
 
 proc afterExecCreate(c: Computation) =
  if c.isSuccess:
- let fork = c.fork
- let contractFailed = not c.writeContract(fork)
- if contractFailed and fork >= FkHomestead:
- c.setError(&"writeContract failed, depth={c.msg.depth}", true)
+ # This can change `c.isSuccess`.
+ c.writeContract()
+ # Contract code should never be returned to the caller. Only data from
+ # `REVERT` is returned after a create. Clearing in this branch covers the
+ # right cases, particularly important with EVMC where it must be cleared.
+ if c.output.len > 0:
+ c.output = @[]
 
  if c.isSuccess:
  c.commit()