From 4b89ca3215b63b69bdadfd2ab4d8d2916def1122 Mon Sep 17 00:00:00 2001 From: Jamie Lokier Date: Thu, 2 Dec 2021 20:44:51 +0100 Subject: [PATCH] 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= ``` 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 --- nimbus/vm/computation.nim | 69 +++++++++++++------ nimbus/vm/interpreter/opcodes_impl.nim | 10 +-- nimbus/vm2/computation.nim | 59 +++++++++++----- .../interpreter/op_handlers/oph_create.nim | 3 +- nimbus/vm2/interpreter_dispatch.nim | 11 +-- 5 files changed, 101 insertions(+), 51 deletions(-) diff --git a/nimbus/vm/computation.nim b/nimbus/vm/computation.nim index ca77d27cf..aa34f8695 100644 --- a/nimbus/vm/computation.nim +++ b/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 +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() diff --git a/nimbus/vm/interpreter/opcodes_impl.nim b/nimbus/vm/interpreter/opcodes_impl.nim index ddfb3c885..9b4a8622d 100644 --- a/nimbus/vm/interpreter/opcodes_impl.nim +++ b/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) diff --git a/nimbus/vm2/computation.nim b/nimbus/vm2/computation.nim index a4c50e3d4..0e4b1efce 100644 --- a/nimbus/vm2/computation.nim +++ b/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 +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 diff --git a/nimbus/vm2/interpreter/op_handlers/oph_create.nim b/nimbus/vm2/interpreter/op_handlers/oph_create.nim index 4e76c623a..d0599ba42 100644 --- a/nimbus/vm2/interpreter/op_handlers/oph_create.nim +++ b/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 # ------------------------------------------------------------------------------ diff --git a/nimbus/vm2/interpreter_dispatch.nim b/nimbus/vm2/interpreter_dispatch.nim index 653d2025c..5f791a864 100644 --- a/nimbus/vm2/interpreter_dispatch.nim +++ b/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()