status-im
/
nimbus-eth1
mirror of https://github.com/status-im/nimbus-eth1.git
2
0
Fork 0
Code Issues Projects Releases Wiki Activity

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 6548ff98 "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.

   d117c8f3fd/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 <jamie@shareable.org>
This commit is contained in:
Jamie Lokier 2021-12-02 20:44:51 +01:00 committed by jangko
parent b3f21bf40c
commit 4b89ca3215
No known key found for this signature in database
GPG Key ID: 31702AE10541E6B9
5 changed files with 101 additions and 51 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

69
nimbus/vm/computation.nim
View File

@ -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
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)
let contractCode = c.output
if contractCode.len == 0: return
# 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
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
# 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
if fork >= FkLondon and contractCode[0] == 0xEF.byte:
debug "Contract code can't start with 0xEF byte"
return false
# 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()

10
nimbus/vm/interpreter/opcodes_impl.nim
View File

@ -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)

59
nimbus/vm2/computation.nim
View File

@ -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
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)
let contractCode = c.output
if contractCode.len == 0: return
# 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
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
# 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
if fork >= FkLondon and contractCode[0] == 0xEF.byte:
debug "Contract code can't start with 0xEF byte"
return false
# 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

3
nimbus/vm2/interpreter/op_handlers/oph_create.nim
View File

@ -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
# ------------------------------------------------------------------------------

11
nimbus/vm2/interpreter_dispatch.nim
View File

@ -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()