Includes a simple test harness for the merge interop M1 milestone
This aims to enable connecting nimbus-eth2 to nimbus-eth1 within
the testing protocol described here:
https://github.com/status-im/nimbus-eth2/blob/amphora-merge-interop/docs/interop_merge.md
To execute the work-in-progress test, please run:
In terminal 1:
tests/amphora/launch-nimbus.sh
In terminal 2:
tests/amphora/check-merge-test-vectors.sh
details:
For documentation, see comments in the file tx_pool.nim.
For prettified manual pages run 'make docs' in the nimbus directory and
point your web browser to the newly created 'docs' directory.
why:
Some helper file will not be generated by the nim document gereator,
so they have been stashed from a later nim version to be provided when
missing.
This problem was known with an earlier nim version (see here
https://github.com/nim-lang/Nim/issues/8952) but was reported solved.
Maybe we need a second look into that.
why:
Previously, the function 'snapshot_desc.loadSnapshot()' contained the
equivalent of 'eth.decode(@[],SnapshotData)' for some type 'SnapshotData'
which should result in an exception of type 'RlpTypeMismatch'.
Before mid October, this worked for all systems on the Github CI. Since
then, a segfault message in the Github CI can be reproduced on all 64bit
Windows wuns when running 'build/all_tests <id-of-test_txpool>' after the
failed 'make test' directive (the latter one needs to be extended by
'|| true'.) This error cannot be reproduced on my local Win7/64 system
with the same MSYS2 and gcc 11.2.0 compiler.
The fix is, rather than catching an exception, to explicitly check the
first argument of 'eth.decode(@[],SnapshotData)' and act if it is empty.
also:
removed some obsolete {.inline.} annotations.
why:
Previous version was based on lru_cache which is ugly. This module is
based on the stew/keyed_queue library module.
other:
There are still some other modules rely on lru_cache which should be
removed.
* 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.
details:
1. The check for cumulativeGasUsed + tx.gasLimit <= header.gasLimit
makes neither sense nor is it part of the Eip1559 specs. Nevertheless
a check tx.gasLimit <= header.gasLimit is added to satisfy some
unit test (see comments in validateTransaction() body.)
2. As a replacement check for the one removed in 1, a check for
cumulativeGasUsed + gasBurned <= header.gasLimit has been added
(see comments in processTransactionImpl() body.)
3. Prototypes for processTransaction() variants have been cleaned up and
commented.
why:
Detail 1. in particular produces an error for tightly packed blocks when
the last tx in the list has a generous gasLimit.
- Remove the `--evm` option on non-EVMC builds.
`when` around an option doesn't work with confutils; it fails to compile.
Workaround that by setting the `ignore` pragma on EVMC-specific options.
(Thanks @jangko for that new pragma). I prefer this to a solution which
moves the whole option's pragma elsewhere, especially if we add more options.
- Improve the help text, so that it shows the standard library extension on
each target platform (or none if on another platform).
- Undo b3f21bf4 "add missing evmc_enabled conditional compilation in
evmc_dynamic_loader". Move the conditional to `nimbus.nim`, and take more
care there to only use the loader function in EVMC builds.
It's ok to just not include this EVMC-only module (like some other EVMC
modules), rather than making the module itself a bit broken: Without this
change, it references a function that's not imported or linked to, and it
only links because there is no call sequence reaching that function.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
- `network` and `discovery` got additional longDesc,
the help text now become more descriptive.
- `networkId` and `networkParams` now is ignored by confutils
and become ordinary fields of NimbusConf.
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 patch adds:
- Load and use a third-party EVM in a shared library, instead of Nimbus EVM.
- New option `--evm` to specify which library to load.
- The library and this loader conforms to the [EVMC]
(https://evmc.ethereum.org/) 9.x specification.
Any third-party EVM which is compatible with EVMC version 9.x and supports EVM1
contract code will be accepted. The operating system's shared library format
applies. These are `.so*` files on Linux, `.dll` files on Windows and `.dylib`
files on Mac.
The alternative EVM can be selected in two ways:
- Nimbus command line option `--evm:<path>`.
- Environment variable `NIMBUS_EVM=<path>`.
The reason for an environment variable is this allows all the test programs to
run with a third-party EVM as well. Some don't parse command line options.
There are some limitations to be aware of:
- The third-party EVM must use EVMC version 9.x, no other major version.
EVMC 9.x supports EIP-1559 / London fork and older transactions.
- Nested `*CALL` and `CREATE*` operations don't use the third-party EVM yet.
These call the built-in Nimbus EVM. This mixing of different EVMs between
levels is explicitly allowed in specs, so there is no problem doing it.
- The third-party EVM doesn't need to support precompiles, because those are
nested calls, which use the built-in Nimbus EVM.
- Third-party EVMs execute contracts correctly, but fail the final `rootHash`
match. The reason is that some account state changes, which are correct, are
currently inside the Nimbus EVM and need to be moved to EVMC host logic.
*This is a known work in progress*. The EVM execution itself is fine.
Test results using "evmone" third-party EVM:
- [evmone](https://github.com/ethereum/evmone) has been tested. Only on
Linux but it "should" work on Windows and Mac equally well.
- [Version 0.8.1](https://github.com/ethereum/evmone/releases/tag/v0.8.1) was
used because it is compatible with EVMC 9.x, which is required for the
EIP-1559 / London fork, which Nimbus supports. Version 0.8.0 could be used
but it looks like an important bug was fixed in 0.8.1.
- evmone runs fine and the trace output looks good. The calls and arguments
are the same as the built-in Nimbus EVM for tests that have been checked
manually, except evmone skips some calls that can be safely skipped.
- The final `rootHash` is incorrect, due to the *work in progress* mentioned
above which is not part of the evmone execution. Due to this, it's possible
to try evmone and verify expected behaviours, which also validates our own
EVMC implementation, but it can't be used as a full substitute yet.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
This missing part of EVMC processing allows third-party EVMs to work.
It fixes EVMC result processing (at the top-level of calls, not nested calls)
to use the EVMC result object, instead of reading so much internal state of the
Nimbus `Computation` object.
It has been tested by calling [`evmone`](https://github.com/ethereum/evmone)
and getting useful results with tracing enabled (`showTxCalls = true`). It's
even able to run parts of the fixtures test suite.
There are other issues with account balances, etc that need to be worked on to
get the correct _final_ results, but the EVM execution is correct with this.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Perform byte-endian conversion for 256-bit numeric values, but not 256-bit
hashes. These conversions are necessary for EVMC binary compatibility.
In new EVMC, all host-side conversions are explicit, calling `flip256`.
These conversions are performed in the EVMC "glue" code, which deals with the
binary interface, so the host services aren't aware of conversions.
We intend to skip these conversions when Nimbus host calls Nimbus EVM, even
when it's a shared library, using a negotiated EVMC extension. But for now
we're focused on correctness and cross-validation with third party EVMs.
The overhead of endian conversion is not too high because most EVMC host calls
access the database anyway. `getTxContext` does not, so the conversions from
that are cached here. Also, well-optimised EVMs don't call it often.
It is arguable whether endian conversion should occur for storage slots (`key`).
In favour of no conversion: Slot keys are 32-byte blobs, and this is clear in
the EVMC definition where slot keys are `evmc_bytes32` (not `evmc_uint256be`),
meaning treating as a number is _not_ expected by EVMC. Although they are
often small numbers, sometimes they are a hash from the contract code plus a
number. Slot keys are hashed on the host side with Keccak256 before any
database calls, so the host side does not look at them numerically.
In favour of conversion: They are often small numbers and it is helpful to log
them as such, rather than a long string of zero digits with 1-2 non-zero. The
representation in JSON has leading zeros removed, like a number rather than a
32-byte blob. There is also an interesting space optimisation when the keys
are used unhashed in storage.
Nimbus currently treats slot keys on the host side as numbers, and the tests
pass when endian conversion is done. So to remain consistent with other parts
of Nimbus we convert slot keys.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Add the new [Arrow Glacier fork](https://eips.ethereum.org/EIPS/eip-4345).
Only the difficulty calculation is changed, but as a new fork it still affects
a number of places in the code.
To the best of my knowledge the change is only scheduled on Mainnet.
In addition:
- The fork date comments in `chain_config.nim` have been checked against the
real networks, set consistently in UTC instead of random timezones, and made
neater. Maybe we'll keep these when transferring config to a file someday.
- It's added to forkid hash tests (EIP-2124/EIP-2364), of course.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
* PoW wrapper for verification & mining
why:
It eases data management of per-Epoch lookup tables. Also some unit
tests show limits of usefulness on non-specialised machines for
mining besides developing tests.
details:
For PoW verification, this patch provides a pretty wrapper hiding the
details of the ethash/Hashimoto lookup cache management.
For mining on my development system without special hardware, the
underlying ethash functions are prohibitively slow. It takes
* ~20 minutes to prepare the full ethash/Hashimoto lookup dataset
* a second to run ~25k nonce tests (in the mining loop)
The mining part might be of some use for generating test data for
the tx-pool, though.
* Using PowRef as replacement for EpochHashCache + hashimotoLight()
* Fix typo (CI failed)
why:
was below log level when testing locally
* fix canonical naming
detected when running hive consensus simulator.
when processing an invalid block header and then
a new valid block header with the same block number,
the state root of the stateDB object should be updated
or reverted to parent stateRoot.
using intermediate stateRoot will trigger the hexary trie assertion.
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>
Fixes#864 "Sync progress stops at Goerli block 4494913", and equivalent on
other networks.
The block body fetcher in `blockchain_sync.nim` had an incorrect assumption
about how peers respond to `GetBlockBodies`. It was issuing requests for N
block bodies and incorrectly handling replies which contained fewer than N
bodies.
Having received up to 192 headers in a batch, it split the range into smaller
`GetBlockBodies` requests, fetched each reply, then combined replies. The
effect was Nimbus requested batches of 128+64 block bodies, received gaps in
the reply sequence, then aborted.
That meant it repeatedly fetched data, then discarded it, and fetched it again,
dropping good peers in the process.
Aborted and restarted batches occurred with earlier blocks too, but this became
more pronounced until there were no suitable peers at batch 4494913..4495104.
Here's a trace:
```
TRC 2021-09-29 02:40:24.977+01:00 Requesting block headers file=blockchain_sync.nim:224 start=4494913 count=192 peer=<ENODE>
TRC 2021-09-29 02:40:24.977+01:00 >> Sending eth.GetBlockHeaders (0x03) file=protocol_eth65.nim:51 peer=<PEER> startBlock=4494913 max=192
TRC 2021-09-29 02:40:25.005+01:00 << Got reply eth.BlockHeaders (0x04) file=protocol_eth65.nim:51 peer=<PEER> count=192
TRC 2021-09-29 02:40:25.007+01:00 >> Sending eth.GetBlockBodies (0x05) file=protocol_eth65.nim:51 peer=<PEER> count=128
TRC 2021-09-29 02:40:25.209+01:00 << Got reply eth.BlockBodies (0x06) file=protocol_eth65.nim:51 peer=<PEER> count=13
TRC 2021-09-29 02:40:25.210+01:00 >> Sending eth.GetBlockBodies (0x05) file=protocol_eth65.nim:51 peer=<PEER> count=64
TRC 2021-09-29 02:40:25.290+01:00 << Got reply eth.BlockBodies (0x06) file=protocol_eth65.nim:51 peer=<PEER> count=64
WRN 2021-09-29 02:40:25.306+01:00 Bodies len != headers.len file=blockchain_sync.nim:276 bodies=77 headers=192
TRC 2021-09-29 02:40:25.306+01:00 peer disconnected file=blockchain_sync.nim:403 peer=<PEER>
TRC 2021-09-29 02:40:25.306+01:00 Finished obtaining blocks file=blockchain_sync.nim:303 peer=<PEER>
```
In practice, for modern peers, Nimbus received shorter replies than it assumed
depending on the block sizes on the chain. Geth/Erigon has 2MiB `BlockBodies`
response size soft limit. OpenEthereum has 4MiB.
Up to Berlin (EIP-2929), Nimbus's fetcher failed often, but there were still
some peers serving what Nimbus needed.
Just after the start of Berlin, at batch 4494913..4495104 on Goerli, zero peers
responded with full size replies for the whole batch, so Nimbus couldn't
progress past that point. But there was already a problem happening before
that for large blocks, dropping good peers and repeatedly fetching the same
block data.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
pre EIP1559 max(gasCost) is tx.gasLimit * tx.gasPrice
the new EIP1559 max(gasCost) before the transaction can be executed is
tx.gasLimit * tx.maxFeePerGas
EIP-2718:
- chainID: Long! of Query
- chainID: Long of Transaction
EIP-1559:
- baseFeePerGas: BigInt of Block
- effectiveGasPrice: BigInt of Transaction
- maxFeePerGas: BigInt of Transaction
- maxPriorityFeePerGas: BigInt of Transaction
this is a preparation for migration to confutils based config
although there is still some getConfiguration usage in tests code
it will be removed after new config arrived
both clique epoch and clique period already checked in
newClique and will use default configuration they are not set.
this redundant check in sealing engine also failed with
some configuration where only one of them is set and the
other one not set.
Zahary Karadjov
jangko
Jordan Hrycaj
Jamie Lokier
Kim De Mey