Commit Graph

19 Commits

Author SHA1 Message Date
jangko b4283aeb1f
fix nonce overflow related to CREATE/CREATE2
if the caller nonce == u64.high, the contract creation cannot
go on.
2022-02-10 15:48:38 +07:00
Jamie Lokier 4b89ca3215
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>
2021-12-12 16:34:13 +07:00
Jamie Lokier ab9067133c
Tracing: Remove some trace messages that occur a lot during sync
Disable some trace messages which appeared a lot in the output and probably
aren't so useful any more, when block processing is functioning well at high
speed.

Turning on the trace level globally is useful to get a feel for what's
happening, but only if each category is kept to a reasonable amount.

As well as overwhelming the output so that it's hard to see general activity,
some of these messages happen so much they severely slow down processing.  Ones
called every time an EVM opcode uses some gas are particularly extreme.

These messages have all been chosen as things which are probably not useful any
more (the relevant functionality has been debugged and is tested plenty).

These have been commented out rather than removed.  It may be that turning
trace topics on/off, or other selection, is a better longer term solution, but
that will require better command line options and good defaults for sure.
(I think higher levels `tracev` and `tracevv` levels (extra verbose) would be
more useful for this sort of deep tracing on request.)

For now, enabling `--log-level:TRACE` on the command line is quite useful as
long as we keep each category reasonable, and this patch tries to keep that
balance.

- Don't show "has transactions" on virtually every block imported.
- Don't show "Sender" and "txHash" lines on every transaction processed.
- Don't show "GAS CONSUMPTION" on every opcode executed", this is way too much.
- Don't show "GAS RETURNED" and "GAS REFUND" on each contract call.
- Don't show "op: Stop" on every Stop opcode, which means every transaction.
- Don't show "Insufficient funds" whenever a contract can't call another.
- Don't show "ECRecover", "SHA256 precompile", "RIPEMD160", "Identity"
  or even "Call precompile" every time a precompile is called.  These are
  very well tested now.
- Don't show "executeOpcodes error" whenever a contract returns an error.
  (This is changed to `trace` too, it's a normal event that is well tested.)

Signed-off-by: Jamie Lokier <jamie@shareable.org>
2021-07-27 14:12:55 +01:00
Jordan Hrycaj 5d0d44c38f re-named compu_helper.nim => computation.nim
why:
  exports all except one of the original computation.nim functional
  objects
2021-04-28 15:24:14 +03:00
Jordan Hrycaj a86308c079 merged contents of computations.nim int interpreter_dispatch.nim
why:
  only two public functions left: executeOpcodes() and execCallOrCreate()
  where the former one was originally in interpreter_dispatch.nim and
  the latter one calls this one.

  improves maintainability
2021-04-28 15:24:14 +03:00
Jordan Hrycaj 49afac46b7 move dispatcher case switch from interpterer_dispatcher.nim into separate file
why:
  insulate for improving maintenance
2021-04-28 15:24:14 +03:00
Jordan Hrycaj caabc9c292 removed kludge and simplified sources oph_call.nim and oph_create.nim
why:
  kludge not needed anymore for oph_handlers.nim sub-sources and sources
  that rely on oph_handlers.nim (but not state_transactions.nim which
  relies on computation.nim.)
2021-04-28 15:24:14 +03:00
Jordan Hrycaj ff6921eb1a re-named some v2*.nim sources to its original name *.nim (without the v2)
why:
  the v2 prefix of the file name was used as a visual aid when
  comparing vm2 against vm sources

details:
  all renamed v2*.nim sources compile locally with the -d:kludge:1 flag
  set or without (some work with either)

  only sources not renamed yet: v2state_transactions.nim
2021-04-28 15:24:14 +03:00
Jordan Hrycaj bca6e791aa provide experimental op handler switch -d:lowmem:1 for low memory C compiler
why:
  on 32bit windows 7, there seems to be a 64k memory ceiling for the gcc
  compiler which was exceeded on some test platform.

details:
  compiling VM2 for low memory C compiler can be triggered with
  "make ENABLE_VM2LOWMEM". this comes with a ~24% longer execution time
  of the test suite against old VM and optimised VM2.
2021-04-28 15:24:14 +03:00
Jordan Hrycaj 1b3117edbd re-implemented handler-call statement by doubly nested case statement
why:
  the new implementation lost more then 25% execution time on the test
  suite when compared to the original VM. so the handler call and the
  surrounding statements have been wrapped in a big case statement similar
  to the original VM implementation. on Linux/x64, the execution time of
  the new VM2 seems to be on par with the old VM.

details:
  on Linux/x64, computed goto works and is activated with the -d:release
  flag. here the execution time of the new VM2 was tested short of 0.02%
  better than the old VM. without the computed goto, it is short of
  0.4% slower than the old VM.
2021-04-28 15:24:14 +03:00
Jordan Hrycaj 3ed234e0a1 clean up cyclic-import-breaker function stubs where possible for op handlers
why:
  using function stubs made it possible to check the syntax of an op
  handler source file by compiling this very file. this was previously
  impossible due cyclic import/include mechanism.

details:
  only oph_call.nim, oph_create.nim and subsequently op_handlers.nim
  still need the -d:kludge:1 flag for syntax check compiling. this flag
  also works with interpreter_dispatch.nim which imports op_handlers.nim.
2021-04-28 15:24:14 +03:00
Jordan Hrycaj a86bcefc7a re-named v2gas_costs.nim to its original name v2gas_costs.nim
why:
  the v2 prefix of the file name was used as a visual aid when
  comparing vm2 against vm sources
2021-04-28 15:24:14 +03:00
Jordan Hrycaj b2ce6d9e70 re-arrange functions from v2computation.nim and interpreter_dispatch.nim
why:
  step towards breaking circular dependency

details:
  some functions from v2computation.nim have been extracted into
  compu_helper.nim which does not explicitly back-import
  v2computation.nim. all non recursive op handlers now import this source
  file rather than v2computation.nim.

  recursive call/create op handler still need to import v2computation.nim.

  the executeOpcodes() function from interpreter_dispatch.nim has been
  moved to v2computation.nim which allows for <import> rather than
  <include> the interpreter_dispatch.nim source.
2021-04-28 15:24:14 +03:00
Jordan Hrycaj 9b70ab5f8f update handler prototype using call-by-reference argument
why:
  this allows for passing back information which can eventually be
  used for reducing use of exceptions

caveat:
  call/create currently needs to un-capture the call-by-reference
  (wrapper) argument using the Computation reference inside
2021-04-28 15:24:14 +03:00
Jordan Hrycaj b388e966cc simplify interpreter_dispatch.nim code
details:
  replace generated macro loop/switch by explicit call using the
  fork/op handler matrix (encapsulated via opHandlersRun() function)
2021-04-28 15:24:14 +03:00
Jordan Hrycaj 2fb18bf88c install new op handlers
details:
  in the source file interpreter_dispatch.nim, op handlers imported from
  opcodes_impl.nim are replaced by table entries from op_handlers.nim
2021-04-28 15:24:14 +03:00
Jordan Hrycaj 2ca9621799 renamed message.nim, precompiles.nim, gas_costs.nim => v2*.nim
why:
  these files provide part of the externally accessible interface
  provided by vm_message.nim, vm_precompile.nim, vm_gas_cost.nim. so the
  new filename indicates that the source code belongs to vm2 (rather
  than vm).
2021-04-23 14:04:06 +03:00
Jordan Hrycaj 7b6767c4a3 renamed types.nim, vm_fork.nim, opcode_values.nim => v2*.nim
why:
  these files provide part of the externally accessible interface
  provided by vm_types*.nim. so the new filename indicates that the
  source code belongs to vm2 (rather than vm).
2021-04-23 14:04:06 +03:00
Jordan Hrycaj b4f8450968 provide identical copy of vm folder => vm2, activated by make flag ENABLE_VM2=1
why:
  vm2 enabled by ENABLE_VM2=1 behaves as vm without ENABLE_EVMC=1 until
  it doesn't in some future fatch set. this leaves some wiggle room
  to work on a vm copy without degrading the original implementation.

details:
  + additional make flag ENABLE_VM2=1 (or ENABLE_VM2=0 to explicitely disable)
  + when both flags ENABLE_EVMC=1 and ENABLE_VM2=1 are present, the former
    flag ENABLE_EVMC=1 takes precedence, this is implemented at the NIM
    compiler level for -d:evmc_enabled and -d:vm2_enabled
2021-04-23 14:04:06 +03:00