why:
Testing against a replay unit test for Devnet4 made it necessary to
adjust the TTD handling. Without updated, importing fails at block #5646
which is the parent of the terminal PoW block. Similar considerations
apply for Devnet5 and Kiln.
* Redesign of BaseVMState descriptor
why:
BaseVMState provides an environment for executing transactions. The
current descriptor also provides data that cannot generally be known
within the execution environment, e.g. the total gasUsed which is
available not before after all transactions have finished.
Also, the BaseVMState constructor has been replaced by a constructor
that does not need pre-initialised input of the account database.
also:
Previous constructor and some fields are provided with a deprecated
annotation (producing a lot of noise.)
* Replace legacy directives in production sources
* Replace legacy directives in unit test sources
* fix CI (missing premix update)
* Remove legacy directives
* chase CI problem
* rebased
* Re-introduce 'AccountsCache' constructor optimisation for 'BaseVmState' re-initialisation
why:
Constructing a new 'AccountsCache' descriptor can be avoided sometimes
when the current state root is properly positioned already. Such a
feature existed already as the update function 'initStateDB()' for the
'BaseChanDB' where the accounts cache was linked into this desctiptor.
The function 'initStateDB()' was removed and re-implemented into the
'BaseVmState' constructor without optimisation. The old version was of
restricted use as a wrong accounts cache state would unconditionally
throw an exception rather than conceptually ask for a remedy.
The optimised 'BaseVmState' re-initialisation has been implemented for
the 'persistBlocks()' function.
also:
moved some test helpers to 'test/replay' folder
* Remove unused & undocumented fields from Chain descriptor
why:
Reduces attack surface in general & improves reading the code.
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>
previously, every time the VMState was created, it will also create
new stateDB, and this action will nullify the advantages of cached accounts.
the new changes will conserve the accounts cache if the executed blocks
are contiguous. if not the stateDB need to be reinited.
this changes also allow rpcCallEvm and rpcEstimateGas executed properly
using current stateDB instead of creating new one each time they are called.
Fixes#868 "Gas usage consensus error at Mainnet block 6001128", and equivalent
on other networks. Mainnet sync is able to continue past 6001128 after this.
Here's a trace:
```
TRC 2021-09-29 15:13:21.532+01:00 Persisting blocks file=persist_blocks.nim:43 fromBlock=6000961 toBlock=6001152
...
DBG 2021-09-29 15:14:35.925+01:00 gasUsed neq cumulativeGasUsed file=process_block.nim:68 gasUsed=7999726 cumulativeGasUsed=7989726
TRC 2021-09-29 15:14:35.925+01:00 peer disconnected file=blockchain_sync.nim:407 peer=<PEER:IP>
```
Similar output is seen at many blocks in the range 6001128..6001204.
The bug is when handling a combination of `CREATE` or `CREATE2`, along with
`SELFDESTRUCT` applied to the new contract address.
Init code for a contract can't return non-empty code and do `SELFDESTRUCT` at
the same time, because `SELFDESTRUCT` returns empty data.
But it is possible to return non-empty code in a newly created, self-destructed
account if the init code calls `DELEGATECALL` or `CALLCODE` to other code which
uses `SELFDESTRUCT`.
In this case we must still charge gas and write the code. This shows on
Mainnet blocks 6001128..6001204, where the gas difference matters. The code
must be written because the new code can be called later in the transaction
too, before self-destruction wipes the account at the end.
There are actually three semantic changes here for a self-destructed, new
contract:
- Gas is charged.
- The code is written to the account.
- It can fail due to insufficient gas.
This patch almost exactly reverts a15805e4 "fix applyCreateMessage" from
2019-02-28. I wonder what that fixed.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Fixes an off by 1 error where `EIP170_CODE_SIZE_LIMIT` was being treated as the
lowest invalid value by EVM code, but the highest valid value by witness code.
To remove confusion, this is renamed to `EIP170_MAX_CODE_SIZE` with value
0x6000, which matches the name (`MAX_CODE_SIZE`) and value used for this limit
in [EIP-170](https://eips.ethereum.org/EIPS/eip-170).
Signed-off-by: Jamie Lokier <jamie@shareable.org>
As this branch of vm2 doesn't support EVMC, this EVMC-motivated change is only
required here for internal compatibility.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
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>
SSTORE_CLEARS_SCHEDULE or FeeSchedule[RefundsClear] in evm
have initial value of 15_000 when introduced by EIP-2200.
EIP-2200 also set new value for SSTORE_RESET_GAS
from 5000 to to 5000 - COLD_SLOAD_COST
Now with EIP-3529, SSTORE_CLEARS_SCHEDULE beecome
SSTORE_RESET_GAS + ACCESS_LIST_STORAGE_KEY_COST
or 5000 - COLD_SLOAD_COST + ACCESS_LIST_STORAGE_KEY_COST
of 5000 - 2100 + 1900 = 4800
This preparation is needed for subsequent
EIPs included in London.
- Add London to Fork enum
- Block number to fork
- Parsing London fork in chain config
- Prepare gas costs table for London
- Prepare EVM opcode dispatcher for London
- Block rewards for London
- Prepare hive script for London
Many places outside the EVM use `Fork` and the fork list, and in general we
want progressively fewer dependencies on EVM internal types and files.
This may prove to be a temporary location, especially when we implement
issue #640. But it's a fine temporary location if so.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
The rationale in EIP-6[1] for changing names to `selfDestruct` applies to code
as much as it does to specs. Also, Ethereum uses the new names consistently,
so it's useful for our code to match the terms used in later EIP specs and
testsuite entries.
This change is straightforward, and is a prerequisite for patches to come that
do things with the `selfDestruct` fields.
[1] https://eips.ethereum.org/EIPS/eip-6
Hudson Jameson, "EIP-6: Renaming SUICIDE opcode," Ethereum Improvement
Proposals, no. 6, November 2015.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
There's no need for macro `toSymbolName` to convert fork enum values to their
presentation texts (logging etc) then re-parse them back to a fork enum value.
`asFork` is already used in the same function and works without these steps,
so use it consistently.
Same applies to `op.toSymbolName` and `asOp`.
This makes the code simpler, and removes a text pattern-matching requirement.
The patch has been checked to confirm it doesn't change the compiled code.
Motivation: The forks list will be removed from VM because it is used outside
the VM as well. Doing so highlighted vm2's `toSymbolName`. It's not needed,
and it's best if the VM doesn't constrain text strings used outside the VM
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.
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.)
Jordan Hrycaj
jangko
Jamie Lokier
bmoo