instead of using stdlib/json, now we switch to json_serialization
the result is much tidier code and more robust when parsing
optional fields.
fixes#635
although this is not part of EIP 1767
but the hive test cases derived from besu
test cases contains this.
we add this now to pass more test hive.graphql cases
Move the EVM setup and call in precompile tests to `fixtureCallEvm` in
`call_evm`. Extra return values needed for testing are returned specially, and
the convention for reporting gas used is changed to match `asmCallEvm`.
Although the precompile tests used `execPrecompiles` before, `executeOpcodes`
does perfectly well as a substitute, allowing `fixtureCallEvm` to be shared.
_Significantly, this patch also makes `Computation` more or less an internal
type of the EVM now._
Nothing outside the EVM (except `call_evm`) needs access any more to
`Computation`, `execComputation`, `executeOpcodes` or `execPrecompiles`.
Many imports can be trimmed, some files removed, and EVMC is much closer.
(As a bonus, the functions in `call_evm` reveal what capabilities parts of the
program have needed over time, makes certain bugs and inconsistencies clearer,
and suggests how to refactor into a more useful shared entry point.)
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Allow the fork to be specified consistently through an `option[Fork]` instead
of varying inconsistencies depending on which call. When fork is not
specified, the `BaseVMState` code picks the correct fork by default for the
block number and chain.
This change actually deletes code, because a number of functions (RPC etc) had
redundant code to pick the fork, which always resolved to same as default.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Move the EVM setup and call in fixtures "vm json tests" to new function
`fixtureCallEvm` in `call_evm`. Extra return values needed for testing are
returned specially.
This entry point is different from all other `..CallEvm` type functions,
because it uses `executeOpcodes` instead of `execComputation`, so it doesn't
update the account balance or nonce on entry and exit from the EVM.
The new code is a bit redundant and simplistic intentionally, as the purpose is
to move functionality to `call_evm` with high confidence nothing really
changed. The calls will be jointly refactored afterwards to merge differences.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
In the `text_vm_json` ("fixtures") test code, there is another variant of
`rpcSetupComputation` and `txSetupComputation` with slightly different
paremeters. The similarity is obvious.
It is a special setup for testing, though, as it requires slightly different
parameters.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
usually, there is always a sender around `getRecipient` call.
no need to recalculate sender. and more important, in some of
JSON-RPC/GraphQL call, the sender is come from `rpcCallData`,
not from `tx.getSender`. or in ohter situation when the tx is
an unsigned tx, without `r,s,v` fields to calculate sender.
Move the EVM setup and call in `macro_assembler` (`runVM`) entirely to new
function `asmCallEvm` in `call_evm`. Extra return values needed for
testing are returned specially from `asmCallEvm`.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
The second `asmSetupComputation looks up state by block number and preceding
block number, modifies the first transaction with code for testing, and uses
some parts of that transaction to setup an an EVM test.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
In the `macro_assembler` test code, `initComputation` is another variant of
`rpcSetupComputation` and `txSetupComputation` with slightly different
paremeters. The similarity is obvious.
It is a special setup for testing, though, as it requires a contract-creation
transaction for parameters, but sets up a `CALL` execution not `CREATE`.
Gather this into `call_evm`: `initComputation` -> `asmSetupComputation`.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
The point of the `call_vm` exercise is to allow `Computation` to become an
internal type of the EVM, not used as API by the rest of the program. So
`rpcSetupComputation` should be private. It was left exported by mistake.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Split out and move the EVM setup and call in `processTransaction` to
`call_evm`. This is the last part of the main program which calls the EVM
to be moved. (There's still test code.)
While we're here, move the EIP2929 access list setup too, as the similarity
to `rpcInitialAccessListEIP2929` is obvious.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
There's only one call left to `refundGas(Transaction, ...)`, and the
similarity to the tail of `rpcEstimateGas` is obvious.
Gather this into `call_evm`: `refundGas` -> `txRefundGas`.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
After recent changes, there's only one call left to `setupComputation`, and
it's just a variant like `rpcSetupComputation` but for transaction processing.
The similarity to `rpcSetupComputation` is obvious.
Gather this into `call_evm`: `setupComputation` -> `txSetupComputation`.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
It's possible for `tx.value` in the transaction to have a deliberately
constructed large 256-bit value, such that adding `gasLimit * gasPrice` to it
overflows to a small value.
Prior to this patch, the code would allow such a transaction to pass
validation, even though such a large transfer cannot be valid.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Change `rpcEstimateGas` to setup and execute a computation directly, in a
similar way to `rpcDoCall` and `rpcMakeCall`, instead of constructing a fake
transaction and then validating it.
This patch does not (or should not) change any behaviour.
Although this looks a bit messy as it duplicates parts of `validateTransaction`
and `processTransaction`, proc names have been used to hopefully keep the
meanings clear, and it's just a stepping stone as those transaction functions
will be changed next. Also the behaviour of RPC `estimateGas` may not be
correct (although this patch is careful not to change it), so it's good to make
it explicit so we can see how it differs from other RPCs.
Doing this change exposed some interesting behaviour differences between the
`call` RPC and `estimateGas` RPC, which may be bugs, or may be intentional.
These differences are now obvious and explicit.
The unclear areas are not well documented by any of the clients, even Infura
which says a bit more than the others. So to find out if they are intended,
we'll have to run tests against other Ethereum services.
Guessing, on the face of it, it looks likely that RPC `call` should:
- Setup EIP2929 access lists
- Account for intrinsic gas (maybe not because zero-gas transactions are ok)
And it looks likely that RPC `estimateGas` should:
- Not return zero when an account has insufficient balance
- Maybe use a different gas cost estimate when one isn't supplied in the RPC
Signed-off-by: Jamie Lokier <jamie@shareable.org>
The RPC `estimateGas` behaves differently from RPC `call` in a number of ways.
These differences may be bugs due to `rpcEstimateGas` calling the EVM in a very
different way than `rpcDoCall`, or they may be intentional. To be sure, we'll
need to test behaviour with Geth, Infura etc to find out (their documentation
isn't enough.) For now, though, we'll keep the same behaviour as we always had.
`rpcEstimateGas` cannot use `rpcSetupComputation` as it is, because
`estimateGas` accounts for "intrinsic gas", and `call` does not.
This patch changes `rpcSetupComputation` to accomodate both behaviours.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
`makeCall` used by GraphQL is another way to setup and call the EVM.
Move it to `transaction/call_evm`.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
`estimateGas` used by JSON-RPC is another way to setup and call the EVM,
also used by GraphQL. Move it to `transaction/call_evm`.
This function has too much direct knowledge of details that shouldn't be used
outside transaction handling code, details we need to change when changing the
db and transaction memory layer.
Moving this one exposed quite a bit of abstraction leakage, as it calls
directly to the hexary trie db around `processTransaction`.
It looks like the _intended_ functionality of `estimateGas` is similar to
`rpcDoCall` with the only real difference being to not store the final state.
It looks like the extra stuff in `estimateGas` compared with `doCall` is a
messy workaround for computation not exposing the right API ("don't save final
state") for RPC to use.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
`doCall` used by JSON-RPC is another way to setup and call the EVM.
Move it to `transaction/call_evm`.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Start gathering the functions that call the EVM into one place,
`transaction/call_evm.nim`.
This is first of a series of changes to gather all ways the EVM is called to
one place. Duplicate, slightly different setup functions have accumulated over
time, each with some knowledge of EVM internals. When they are brought
together, these methods will be changed to use a single entry point to the EVM,
allowing the entry point to be refactored, EVMC to be completed, and async
concurrency to be implemented on top. This also simplifies the callers.
First, a helper function used by RPC and GraphQL to make EVM calls without
permanently modifying the account state. `setupComputation` ->
`rpcSetupComputation`.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
the `processArguments` now have overloaded proc, one with opt param and one without.
the OptParser now can be passed to `opt` param.
this is useful in scenario where in test code we need to simulate something
without using real command line arguments.
rather than initialize it to 0, those block numbers
are initialized to high(BlockNumber). this will fix
issue when imported genesis.json doesn't contains all
forks' blockNumber.
The account database code is not supposed to raise exceptions in the EVM, and
the behaviour is not well defined if it does. It isn't compliant with EVMC
spec either. But that will be dealt with properly when the account state-cache
is dealt with, as there is some work to be done on it.
Meanwhile, if it raises in code under `chainTo` and then `(continuation)()`,
the behaviour was changed slightly by the stack-shrink patches.
Before those patches, an exception after the recursion-point was converted to
`c.setError` "Opcode Dispatch Error" in `executeOpcodes. After, it would
propagate out, a different behaviour. (It still correctly walked the chain of
`c.dispose()` calls to clean up.)
It's easy to restore the original behaviour just by moving the continuation
call, so let's do that.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
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
overview:
can be verified by running "make check_vm2 X=0" in the nimbus directory
(be patient when running it.) the X=0 flag is necessary if there is a
native NIM compiler which may bail out at some vendor imports.
details:
when compiling state_transaction.nim, the nim flag vm2_enabled must
be set in order to avoid implicit import of native VM definitions.
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.)
also:
re-integrated stack_defs.nim back into stack.nim
why:
the v2 prefix of the file name was used as a visual aid when
comparing vm2 against vm sources
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
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.
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.
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.
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.
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
why:
the previous approach was replacing the function-lets in
opcode_impl.nim by the particulate table handlers. the test
functions will verify the the handler functions are sort of
correct but not the assignments in the fork tables.
the handler names of old and new for tables are checked here.
caveat:
verifying tables currently takes a while at compile time.
details:
the op handler table is accessible via op_handlers.nim module
op handler function implementations are found in the op_handlers/
sub-directory
kludge:
for development and pre-testing, any new module can be individually
compiled setting the kludge flag using -d:kludge:1. this causes some
proc/func replacements in turn allowing for omitting imports that would
otherwise cause a circular dependency. otherwise individual compilation
would fail.
in order to prove the overall correctness of the code, the
op_handlers.nim is imported by opcodes_impl.nim when compiling all,
nimbus or test.
why:
subsequent development will compile sources as main without setting
the vm2_enabled flag. also, the doc generator would fail an vm2 without
setting the flag for the vm2 files.
why:
generally, there is no role for libbacktrace when docs are generated
for vm2, undo settings of config.nim and provide the "kludge" flag, so
circular import/include dependencies can be taken care of (not only)
for generating docs
why:
new name forks_list.nim file name matches additional documentation
file names.
details:
v2forks.nim remains a hollowed out shell serving as interface file.
why:
activate NIM comments needed re-write. as there is no advantage in using
the macro replacing a few missing op-codes by "Nop##" name symbols, the
macro wrapper has been removed.
details:
when explicitly accessed by numeric value ##, missing Op enum entries
result in a symbol name something like "Op ##".
rather than implicitly using a macro to fix the op-codes list, missing
entries are detected at compile time when a fatal exception is thrown.
the static compile time check verifies that
all op-codes 0 .. 255 are defined
op code name/mnemonic has at least 2 chars and starts with a capital
op code name/mnemonic is not NIM auto-generated (i.e. has a space)
also, original '#' comments are exposed as doc comments '##'
Capitalisation:
- The option is lower case `--logmetrics` but help said `--logMetrics`
- Same for `--logmetricsiterval`
- Same for `--metricsserver` and `--metricsserverport`
Ethereum network selection:
- Moved out into their own, cleaner help section
- Added help for `--mainnet`, `--goerli` and `--kovan`
- Moved `--networkid` and `--customnetwork` to this section as well
Other:
- Reworded or formatted some help lines for clarity and consistency
Changed options:
- Renamed `--metricserver` to `--metrics`
- Renamed `--matricsserverport` to `--metricsport`
- Removed Morden network; this didn't have an option, but could be
selected with `--networkid:2` and then fail to work
Signed-off-by: Jamie Lokier <jamie@shareable.org>
This patch reduces stack space used with EVM in ENABLE_EVMC=1 mode, from 13 MB
worst case to 550 kB, a 24x reduction.
This completes fixing the "stack problem" and closes#575 (`EVM: Different
segmentation faults when running the test suite with EVMC`).
It also closes#256 (`recursive EVM call trigger unrecoverable stack overflow`).
After this patch, it is possible to re-enable the CI targets which had to be
disabled due to #575.
This change is also a required precursor for switching over to "nearly EVMC" as
the clean and focused Nimbus-internal API between EVM and sync/database
processes, and is also key to the use of Chronos `async` in those processes
when calling the EVM.
(The motivation is the internal interface has to be substantially changed
_anyway_ for the parallel sync and database processes, and EVMC turns out to be
well-designed and well-suited for this. It provides good separation between
modules, and suits our needs better than our other current interface. Might as
well use a good one designed by someone else. EVMC is 98% done in Nimbus
thanks to great work done before by @jangko, and we can use Nimbus-specific
extensions where we need flexibility, including for performance. Being aligned
with the ecosystem is a useful bonus feature.)
All tests below were run on Ubuntu 20.04 LTS server, x86-64. This matches one
of the targets that has been disabled for a while in CI in EVMC mode due to
stack overflow crashing the tests, so it's a good choice.
Measurements before
===================
Testing commit `e76e0144 2021-04-22 11:29:42 +0700 add submodules: graphql and
toml-serialization`.
$ rm -f build/all_tests && make ENABLE_EVMC=1 test
$ ulimit -S -s 16384 # Requires larger stack than default to avoid crash.
$ ./build/all_tests 9 | tee tlog
[Suite] persist block json tests
...
Stack range 38416 depthHigh 3
...
Stack range 13074720 depthHigh 1024
[OK] tests/fixtures/PersistBlockTests/block1431916.json
These tests use 13.07 MB of stack to run, and so crash with the default stack
limit on Ubuntu Server 20.04 (8MB). Exactly 12768 bytes per EVM call stack
frame.
$ rm -f build/all_tests && make ENABLE_EVMC=1 test
$ ulimit -S -s 16384 # Requires larger stack than default.
$ ./build/all_tests 7 | tee tlog
[Suite] new generalstate json tests
...
Stack range 14384 depthHigh 2
...
Stack range 3495456 depthHigh 457
[OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest639.json
...
Stack range 3709600 depthHigh 485
[OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest458.json
...
Stack range 7831600 depthHigh 1024
[OK] tests/fixtures/eth_tests/GeneralStateTests/stCreate2/Create2OnDepth1024.json
These tests use 7.83MB of stack to run. About 7648 bytes per EVM call stack
frame. It _only just_ avoids crashing with the default Ubuntu Server stack
limit of 8 MB. However, it still crashes on Windows x86-64, which is why the
Windows CI EVMC target is currently disabled.
On Linux where this passes, this is so borderline that it affects work and
testing of the complex storage code, because that's called from the EVM.
Also, this greatly exceeds the default thread stack size.
Measurements after
==================
$ rm -f build/all_tests && make ENABLE_EVMC=1 test
$ ulimit -S -s 600 # Because we can! 600k stack.
$ ./build/all_tests 9 | tee tlog
[Suite] persist block json tests
...
Stack range 1936 depthHigh 3
...
Stack range 556272 depthHigh 1022
Stack range 556512 depthHigh 1023
Stack range 556816 depthHigh 1023
Stack range 557056 depthHigh 1024
Stack range 557360 depthHigh 1024
[OK] tests/fixtures/PersistBlockTests/block1431916.json
$ rm -f build/all_tests && make ENABLE_EVMC=1 test
$ ulimit -S -s 600 # Because we can! 600k stack.
$ ./build/all_tests 7 | tee tlog
[Suite] new generalstate json tests
...
Stack range 1392 depthHigh 2
...
Stack range 248912 depthHigh 457
[OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest639.json
...
Stack range 264144 depthHigh 485
[OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest458.json
...
Stack range 557360 depthHigh 1024
[OK] tests/fixtures/eth_tests/GeneralStateTests/stStaticCall/static_CallRecursiveBombPreCall.json
For both tests, a satisfying *544 bytes* per EVM call stack frame, and EVM
takes less than 600 kB total. With other overheads, both tests run in 600 kB
stack total at maximum EVM depth.
We must add some headroom on this for database activity called from the EVM,
and different compile targets. But it means the EVM itself is no longer a
stack burden.
This is much smaller than the default thread stack size on Linux (2MB), with
plenty of margin. (Just fyi, it isn't smaller than a _small_ thread stack on
Linux from a long time ago (128kB), and some small embedded C targets.)
This size is well suited to running EVMs in threads.
Further reduction
=================
This patch solves the stack problem. Windows and Linux 64-bit EVMC CI targets
can be re-enabled, and there is no longer a problem with stack usage.
We can reduce further to ~340 bytes per frame and 350 kB total, while still
complying with EVMC. But as this involves changing how errors are handled to
comply fully with EVMC, and removing `dispose` calls, it's not worth doing now
while there are other EVMC changes in progress that will have the same effect.
A Nimbus-specific extension will allow us to avoid recursion with EVMC anyway,
bringing bytes per frame to zero. We need the extension anyway, to support
Chronos `async` which parallel transaction processing is built around.
Interop with non-Nimbus over EVMC won't let us avoid recursion, but then we
can't control the stack frame size either. To prevent stack overflow in
interop I anticipate using (this method in Aleth)
[6e96ce34e3/libethereum/ExtVM.cpp (L61)].
Smoke test other versions of GCC and Clang/LLVM
===============================================
As all builds including Windows use GCC or Apple's Clang/LLVM, this is just to
verify we're in the right ballpark on all targets. I've only checked `x86_64`
though, not 32-bit, and not ARM.
It's interesting to see GCC 10 uses less stack. This is because it optimises
`struct` returns better, sometimes skipping an intermediate copy. Here it
benefits the EVMC API, but I found GCC 10 also improves the larger stack usage
of the rest of `nimbus-eth1` as well.
Apple clang 12.0.0 (clang-1200.0.26.2) on MacOS 10.15:
- 544 bytes per EVM call stack frame
GCC 10.3.0 (Ubuntu 10.3.0-1ubuntu1) on Ubuntu 21.04:
- 464 bytes per EVM call stack frame
GCC 10.2.0 (Ubuntu 10.2.0-5ubuntu1~20.04) on Ubuntu 20.04 LTS:
- 464 bytes per EVM call stack frame
GCC 11.0.1 20210417 (experimental; Ubuntu 11-20210417-1ubuntu1) on Ubuntu 21.04:
- 8 bytes per EVM call stack frame
GCC 9.3.0 (Ubuntu 9.3.0-17ubuntu1~20.04) on Ubuntu 20.04 LTS:
- 544 bytes per EVM call stack frame
GCC 8.4.0 (Ubuntu 8.4.0-3ubuntu2) on Ubuntu 20.04 LTS:
- 544 bytes per EVM call stack frame
GCC 7.5.0 (Ubuntu 7.5.0-6ubuntu2) on Ubuntu 20.04 LTS:
- 544 bytes per EVM call stack frame
GCC 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2) on Ubuntu 19.10:
- 528 bytes per EVM call stack frame
Signed-off-by: Jamie Lokier <jamie@shareable.org>
instead of using header as input param, now getReceipts using
receiptRoot hash, the intention is clearer and less data passed around
when we only using receiptRoot instead of whole block header.
why:
these files provide part of the externally accessible interface
provided by vm_cpmputation.nim, vm_internals.nim. so the
new filename indicates that the source code belongs to vm2 (rather
than vm).
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).
why:
these files provide part of the externally accessible interface
provided by vm_state*.nim. so the new filename indicates that the
source code belongs to vm2 (rather than vm).
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).
why:
making sure that deep links into vm2 sources are configured properly. it
is intended that only the vm_*.nim interface headers are allowed to
source files in vm2. the sentinels just protect from coding errors.
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
jangko
Jamie Lokier
Jordan Hrycaj