* Activate wire protocol eth/66
and:
Disentangle protocol_eth66.nim from import sections
why:
Importing the protocol_eth66 module is not necessary. There is
no need to know too many details of the underlying wire protocol. All
that is needed will be exported by blockchain_sync.nim.
* fixes, and rebase
* Update nimbus/p2p/blockchain_sync.nim
Co-authored-by: Kim De Mey <kim.demey@gmail.com>
* Fixes and rebase
Co-authored-by: Kim De Mey <kim.demey@gmail.com>
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.
- prevRandao/mixDigest = payloadAttrs.prevRandao
- timestamp = payloadAttrs.timestamp
- coinbase = payloadAttrs.suggestedFeeRecipient
- extraData = it can be anything 0-32 bytes, but currently set to empty bytes.
* Rearrange/rename test_kintsugu => test_custom_network
why:
Debug, fix and test more general problems related to running
nimbus on a custom network.
* Update UInt265/Json parser for --custom-network command line option
why:
As found out with the Kintsugi configuration, block number and balance
have the same Nim type which led to misunderstandings. This patch makes
sure that UInt265 encoded string values "0x11" decodes to 17, and "b"
and "11" to 11.
* Refactored genesis.toBlock() => genesis.toBlockHeader()
why:
The function toBlock(g,db) may return different results depending on
whether the db descriptor argument is nil, or initialised. This is due
to the db.config data sub-descriptor which may give various outcomes
for the baseFee field of the genesis header.
Also, the version where db is non-nil initialised is used internally
only. So the public rewrite toBlockHeader() that replaces the toBlock()
function expects a full set of NetworkParams.
* update comments
* Rename toBlockHeader() => toGenesisHeader()
why:
Polymorphic prototype used for BaseChainDB or NetworkParams argument.
With a BaseChainDB descriptor argument, the name shall imply that the
header is generated from the config fields rather than fetched from
the database.
* Added command line option --static-peers-file
why:
Handy feature to keep peer nodes in a file, similar to the
--bootstrap-file option.
Any transaction where tx.sender has a CODEHASH != EMPTYCODEHASH MUST
be rejected as invalid, where EMPTYCODEHASH =
0xc5d2460186f7233c927e7db2dcc703c0e500b653ca82273b7bfad8045d85a470.
The invalid transaction MUST be rejected by the client and not be included
in a block. A block containing such a transaction MUST be considered invalid.
because EIP-4399 override mixDigest validation rule,
there is no need to check mixDigest == ZERO_HASH
`mixDigest` will carry POS block randomness value
* Kludge needed for setting up custom network
why:
Some non-features in the persistent hexary trie DB produce an assert
error when initiating the Kinsugi network.
details:
This fix should be temporary, only.
* Fix OS detection
why:
directive detectOs() bails out on Windows if checking for Ubuntu
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.
Prior to this patch, top-level EVM executions and nested EVM executions did
their `getStorage` and other requests using a completely different set of host
functions. It was just unfinished, to get top-level "new" EVMC working.
This finishes the job - it stops using the old methods. Effect:
- Functionality added at the EVMC host level will be used by all EVM calls.
(The target here is Beam Sync).
- The old set of functions are no longer used, so they can be removed.
- When EVMC host call tracing is enabled (`showTxCalls = true`), it traces
the calls from nested EVM executions as well as top-level.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
We've been filling a "vtable"-like at run time, but it's not necessary.
The new object is a global `let x = evmc_host_interface(...)`, we assume it's
initialised before the first use, and we take its address with `.unsafeAddr`.
(If we use `ref evmc_host_interface`, Nim decides (correctly) that the
functions which use it aren't GC-safe because it's a global.)
Signed-off-by: Jamie Lokier <jamie@shareable.org>
This combines two things, a C stack usage change with EVM nested calls
via EVMC, and changes to host call tracing.
Feature-wise, the tracing is improved:
- Storage keys and values are make more sense.
- The message/result/context objects are shown with all relevant fields.
- `call` trace is split into entry/exit, so these can be shown around the
called contract's operations, instead of only showing the `call` parameters
after the nested call is finished.
- Nested calls are indented, which helps to highlight the flow.
- C stack usage considerably reduced in nested calls when more functionality
is enabled (either tracing here, or other things to come).
This will seem like a minor patch, but C stack usage was the real motivation,
after plenty of time in the debugger.
Nobody cares about stack when `showTxCalls` (you can just use a big stack when
debugging). But these subtle changes around the `call` path were found to be
necessary for passing all tests when the EVMC nested call code is completed,
and that's a prerequisite for many things: async EVM, dynamic EVM, Beam Sync,
and to fix https://github.com/status-im/nimbus-eth1/issues/345.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
The update for London (EIP-1559) in 1cdb30df ("bump nim-emvc with evmc revision
8.0.0 to 9.0.0") really bumped EVMC ABI version from 7.5 up to 9.
In other words, it skipped Berlin, going direct from Istanbul to London.
That was accompanied by EVMC changes in 05e9b891 ("EIP-3198: add baseFee op
code in nim-evm"), which added the API changes needed for London.
But the missing Berlin functions weren't added in the move to London.
As a result, our EVMC host became incompatible with Berlin, London, and really
all revisions of the ABI, and if a third party EVM was loaded, it crashed.
This commit adds the missing Berlin host support, and makes our ABI
binary-compatible with real EVMC again.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
although they are technically different, but in reality,
many networks are using the same id for ChainId dan NetworkId.
in this commit, we set networkid from config file's chainId.
- allow clique period and epoch to be configured via config file
- this also activate poaEngine mode
- remove clique period configuration from cli to reduce confusion
- fix#786
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>
This changes fixes a bug in `CREATE2` ops when used with EVMC.
Because it changes the salt type, it affects non-EVMC code as well.
The salt was passed through EVMC with the wrong byte order, although this went
unnoticed as the Nimbus host flipped the byte order before using it.
This was found when running Nimbus with third-party EVM,
["evmone"](https://github.com/ethereum/evmone).
There are different ways to remedy this.
If treated as a number, Nimbus EVM would byte-flip the value when calling EVMC,
then Nimbus host would flip the received value. Finally, it would be flipped a
third time when generating the address in `generateSafeAddress`. The first two
flips can be eliminated by negotiation (like other numbers), but there would
always be one flip.
As a bit pattern, Nimbus EVM would flip the same way it does when dealing with
hashes on the stack (e.g. with `getBlockHash`). Nimbus host wouldn't flip at
all - and when using third-party EVMs there would be no flips in Nimbus.
Because this value is not for arithmetic, any bit pattern is valid, and there
shouldn't be any flips when using a third-party EVM, the bit-pattern
interpretation is favoured. The only flip is done in Nimbus EVM (and might be
eliminated in an optimised version).
As suggested, we'll define a new "opaque 256 bits" type to hold this value.
(Similar to `Hash256`, but the salt isn't necessarily a hash.)
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Nimbus types generally use the bit count not the byte count, e.g. `UInt256`,
`Hash256`, so make `ZERO_HASH256` (which has type `Hash256`) fit this pattern.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
* Provide PoA voting header generator
why:
Handy for hive/smoke test
details:
Header generator is a re-implementation of the generator previously
used for the canonical reference tests.
* try fixing ci out-of-mem condition
why:
for some reason, the ci began behaving like a real win7/i386 machine
where gcc is limited to 64k optimiser space
* fix comments, typos ..
new command line options:
--ws Enable the Websocket JSON-RPC server
--wsbind:<value> Set address:port pair(s) (comma-separated) Websocket JSON-RPC server will bind to (default: localhost:8546)
--wsapi:<value> Enable specific set of Websocket RPC API from list (comma-separated) (available: eth, debug)
fixes#770
new command line options:
--clique-period:<value> Enables clique support. value is block time in seconds
--engine-signer:<value> Enables mining. value is EthAddress in hex
--import-key:<path> Import unencrypted 32 bytes hex private key file
fixes#771
* Provide API
details:
API is bundled via clique.nim.
* Set extraValidation as default for PoA chains
why:
This triggers consensus verification and an update of the list
of authorised signers. These signers are integral part of the
PoA block chain.
todo:
Option argument to control validation for the nimbus binary.
* Fix snapshot state block number
why:
Using sub-sequence here, so the len() function was wrong.
* Optional start where block verification begins
why:
Can speed up time building loading initial parts of block chain. For
PoA, this allows to prove & test that authorised signers can be
(correctly) calculated starting at any point on the block chain.
todo:
On Goerli around blocks #193537..#197568, processing time increases
disproportionally -- needs to be understand
* For Clique test, get old grouping back (7 transactions per log entry)
why:
Forgot to change back after troubleshooting
* Fix field/function/module-name misunderstanding
why:
Make compilation work
* Use eth_types.blockHash() rather than utils.hash() in Clique modules
why:
Prefer lib module
* Dissolve snapshot_misc.nim
details:
.. into clique_verify.nim (the other source file clique_unused.nim
is inactive)
* Hide unused AsyncLock in Clique descriptor
details:
Unused here but was part of the Go reference implementation
* Remove fakeDiff flag from Clique descriptor
details:
This flag was a kludge in the Go reference implementation used for the
canonical tests. The tests have been adapted so there is no need for
the fakeDiff flag and its implementation.
* Not observing minimum distance from epoch sync point
why:
For compiling PoA state, the go implementation will walk back to the
epoch header with at least 90000 blocks apart from the current header
in the absence of other synchronisation points.
Here just the nearest epoch header is used. The assumption is that all
the checkpoints before have been vetted already regardless of the
current branch.
details:
The behaviour of using the nearest vs the minimum distance epoch is
controlled by a flag and can be changed at run time.
* Analysing processing time (patch adds some debugging/visualisation support)
why:
At the first half million blocks of the Goerli replay, blocks on the
interval #194854..#196224 take exceptionally long to process, but not
due to PoA processing.
details:
It turns out that much time is spent in p2p/excecutor.processBlock()
where the elapsed transaction execution time is significantly greater
for many of these blocks.
Between the 1371 blocks #194854..#196224 there are 223 blocks with more
than 1/2 seconds execution time whereas there are only 4 such blocks
before and 13 such after this range up to #504192.
* fix debugging symbol in clique_desc (causes CI failing)
* Fixing canonical reference tests
why:
Two errors were introduced earlier but ovelooked:
1. "Remove fakeDiff flag .." patch was incomplete
2. "Not observing minimum distance .." introduced problem w/tests 23/24
details:
Fixing 2. needed to revert the behaviour by setting the
applySnapsMinBacklog flag for the Clique descriptor. Also a new
test was added to lock the new behaviour.
* Remove cruft
why:
Clique/PoA processing was intended to take place somewhere in
executor/process_block.processBlock() but was decided later to run
from chain/persist_block.persistBlock() instead.
* Update API comment
* ditto
Using the same packet tracing format to match `protocol_eth65`.
There aren't many calls, and this makes them clear.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
The format is reasonably useful and not too large, when looking at the
behaviour of sync processes. It doesn't try to show all the details of
packets, just something at a useful level of detail to see what's going on.
The consistent presentation has proven helpful too, e.g. when grepping.
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>
Move `blockchain_sync.nim` from `nim-eth` to `nimbus-eth1`.
This lets `blockchain_sync` use the `eth/65` protocol to synchronise with more
modern peers than before.
Practically, the effect is the sync process runs more quickly and reliably than
before. It finds usable peers, and they are up to date.
Note, this is mostly old code, and it mostly performs "classic sync", the
original Ethereum method. Here's a summary of this code:
- It decides on a blockchain canonical head by sampling a few peers.
- Starting from block 0 (genesis), it downloads each block header and
block, mostly in order.
- After it downloads each block, it executes the EVM transactions in that block
and updates state trie from that, before going to the next block.
- This way the database state is updated by EVM executions in block order,
and new state is persisted to the trie database after each block.
Even though it mentions Geth "fast sync" (comments near end of file), and has
some elements, it isn't really. The most obvious missing part is this code
_doesn't download a state trie_, it calculates all state from block 0.
Geth "fast sync" has several parts:
1. Find an agreed common chain among several peers to treat as probably secure,
and a sufficiently long suffix to provide "statistical economic consensus"
when it is validated.
2. Perform a subset of PoW calculations, skipping forward over a segment to
verify some of the PoWs according to a pattern in the relevant paper.
3. Download the state trie from the block at the start of that last segment.
4. Execute only the blocks/transactions in that last segment, using the
downloaded state trie, to fill out the later states and properly validate the
blocks in the last segment.
Some other issues with `blockchain_sync` code:
- If it ever reaches the head of the chain, it doesn't follow new blocks with
increasing block numbers, at least not rapidly.
- If the chain undergoes a reorg, this code won't fetch a block number it has
already fetched, so it can't accept the reorg. It will end up conflicted
with peers. This hasn't mattered because the development focus has been on
the bulk of the catching up process, not the real-time head and reorgs.
- So it probably doesn't work correctly when it gets close to the head due to
many small reorgs, though it might for subtle reasons.
- Some of the network message handling isn't sufficiently robust, and it
discards some replies that have valid data according to specification.
- On rare occasions the initial query mapping block hash to number can
fail (because the peer's state changes).
- It makes some assumptions about the state of peers based on their responses
which may not be valid (I'm not convinced they are). The method for working
out "trusted" peers that agree a common chain prefix is clever. It compares
peers by asking each peer if it has the header matching another peer's
canonical head block by hash. But it's not clear that merely knowing about a
block constitutes agreement about the canonical chain. (If it did, query by
block number would give the same answer more authoritatively.)
Nonetheless, being able to run this sync process on `eth/65` is useful.
<# interactive rebase in progress; onto 66532e8a
Signed-off-by: Jamie Lokier <jamie@shareable.org>
This constant shouldn't be used outside `protocol_eth65`.
When we support multiple `eth/NN` versions side by side, or even just have
multiple code files, there's a risk some code would import just one of the
files (e.g. `protocol_eth65`), use `protocolVersion`, and incorrectly act as
though that version is the one active on the node.
In fact that happened, and now it can't happen. Other code needs to query the
`EtheruemNode` to find what versions are really active.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
In RPC `eth_protocolVersion`, look at the live `EthereumNode` to find which
version of `eth/NN` protocol is active, instead of trusting a compile-time
constant. It's better to check dynamically. GraphQL already does this.
As a result, the RPC code doesn't depend on `eth_protocol` any more.
To make sure there are no more accidental users of the old constant,
`protocolVersion` is no longer exported from `protocol_eth65`.
(The simplest way to support `eth/65` was to make `eth_protocolVersion` use
`protocol_eth65.protocolVersion`, to get 65. But that's silly. More
seriously, when we add another version (`eth/66`) running alongside `eth/65`,
that expression would still compile ok yet return the wrong value, while still
passing the RPC test suite.)
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Turns out `{.rlpInline.}` doesn't do anything.
It's documented but not implemented.
Due to this, whenever a peer sent us a `NewBlock` message, we had an RLP
decoding error processing it, and disconnected the peer thinking it was the
peer's error.
These messages are sent often by good peers, so whenever we connected to a
really good peer, we'd end up disconnecting from it due to this.
Because a block body is a list of transactions, the parse errors looked
suspiciously like EIP-2718/2976/2930/1559 typed transaction RLP errors.
But it was a failure to parse `BlockBody` inline.
Conveniently, the `EthBlock` type defined for another reason is encoded exactly
the way `NewBlockAnnounce` needs to be, so we can reuse that type.
This didn't stand out before updating to `eth/65`, because with old protocols
we tend to only connect to old peers, which may be out of date themselves and
have no new blocks to send. Also, we didn't really investigate occasional
disconnects before, we assumed they're just part of P2P life.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
This patch adds the `eth/65` protocol, documented at
https://github.com/ethereum/devp2p/blob/master/caps/eth.md.
This is an intentionally simple patch, designed to not break, change or depend
on other functionality much, so that the "_old_ sync" methods can be run
usefully again and observed. This patch isn't "new sync" (a different set of
sync algorithms), but it is one of the foundations.
For a while now Nimbus Eth1 only supported protocol `eth/63`. But that's
obsolete, and very few nodes still support it. This meant Nimbus Eth1 would
make slow progress trying to sync, as most up to date clients rejected it.
The current specification is `eth/66`, and the functionality we really need is
in `eth/64`.
So why `eth/65`?
- `eth/64` is essential because of the `forkId` feature. But `eth/64` is on
its way out as well. Many clients, especially the most up to date Geth
running the current hard-forks (Berlin/London) don't talk `eth/64` any more.
- `eth/66` is the current specification, but some clients don't talk `eth/66`
yet. We'd like to have the best peer connectivity during tests, and
currently everything that talks `eth/66` also talks `eth/65`.
- Nimbus Eth1 RLPx only talks one version at a time. (Without changes to the
RLPx module. When those go in we'll add `eth/64..eth/66` for greater peer
reach and testing the `eth/66` behaviour. For simplicity and decoupling,
this patch contains just one version, the most useful.)
What are `eth/64` and `eth/65`?
- `eth/64` (EIP-2364) added `forkId` which allows nodes to distinguish between
Ethereum (ETH) and Ethereum Classic (ETC) blockchains, which share the same
genesis block. `forkId` also protects the system when a new hard fork is
going to be rolled out, by blocking interaction with out of date nodes. The
feature is required nowadays.
We send the right details to allow connection (this has been tested a lot),
but don't apply the full validation rules of EIP-2124/EIP-2364 in this patch.
It's to keep this patch simple (in its effects) and because those rules have
consequences best tested separately. In practice the other node will reject
us when we would reject it, so this is ok for testing, as long as it doesn't
get seriously deployed.
- `eth/65` added more efficient transaction pool methods.
- Then a later version of `eth/65` (without a new number) added typed
transactions, described in [EIP-2976](https://eips.ethereum.org/EIPS/eip-2976).
Why it's moved to `nimbus-eth1`:
- Mainly because `eth/64` onwards depend on the current state of block
synchronisation, as well as the blockchain's sequence of hard-fork block
numbers, both of which are part of `nimbus-eth1` run-time state. These
aren't available to pure `nim-eth` code. Although it would be possible to
add an API to let `nimbus-eth1` set these numbers, there isn't any point
because the protocol would still only be useful to `nimbus-eth1`.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
* Renamed source file clique_utils => clique_helpers
why:
New name is more in line with other modules where local libraries
are named similarly.
* re-implemented PoA verification module as clique_verify.nim
details:
The verification code was ported from the go sources and provisionally
stored in the clique_misc.nim source file.
todo:
Bring it to life.
* re-design Snapshot descriptor as: ref object
why:
Avoids some copying descriptor objects
details:
The snapshot management in clique_snapshot.nim has been cleaned up.
todo:
There is a lot of unnecessary copying & sub-list manipulation of
seq[BlockHeader] lists which needs to be simplified by managing
index intervals.
* optimised sequence handling for Clique/PoA
why:
To much ado about nothing
details:
* Working with shallow sequences inside PoA processing avoids
unnecessary copying.
* Using degenerate lists in the cliqueVerify() batch where only the
parent (and no other ancestor) is needed.
todo:
Expose only functions that are needed, shallow sequences should be
handles with care.
* fix var-parameter function argument
* Activate PoA engine -- currently proof of concept
details:
PoA engine is activated with newChain(extraValidation = true) applied
to a PoA network.
status and todo:
The extraValidation flag on the Chain object can be set at a later
state which allows to pre-load parts of the block chain without
verification. Setting it later will only go back the block chain to
the latest epoch checkpoint. This is inherent to the Clique protocol,
needs testing though.
PoA engine works in fine weather mode on Goerli replay. With the
canonical eip-225 tests, there are quite a few fringe conditions
that fail. These can easily fudged over to make things work but need
some more work to understand and correct properly.
* Make the last offending verification header available
why:
Makes some fringe case tests work.
details:
Within a failed transaction comprising several blocks, this
feature help to identify the offending block if there was a
PoA verification error.
* Make PoA header verifier store the final snapshot
why:
The last snapshot needed by the verifier is the one of the parent but
the list of authorised signer is derived from the current snapshot. So
updating to the latest snapshot provides the latest signers list.
details:
Also, PoA processing has been implemented as transaction in
persistBlocks() with Clique state rollback.
Clique tests succeed now.
* Avoiding double yields in iterator => replaced by template
why:
Tanks to Andri who observed it (see #762)
* Calibrate logging interval and fix logging event detection
why:
Logging interval as copied from Go implementation was too large and
needed re-calibration. Elapsed time calculation was bonkers, negative
the wrong way round.
* re-shuffled Clique functions
why:
Due to the port from the go-sources, the interface logic is not optimal
for nimbus. The main visible function is currently snapshot() and most
of the _procurement_ of this function result has been moved to a
sub-directory.
* run eip-225 Clique test against p2p/chain.persistBlocks()
why:
Previously, loading the test block chains was fugdged with the purpose
only to fill the database. As it is now clear how nimbus works on
Goerli, the same can be achieved with a more realistic scenario.
details:
Eventually these tests will be pre-cursor to the reply tests for the
Goerli chain supporting TDD approach with more simple cases.
* fix exception annotations for executor module
why:
needed for exception tracking
details:
main annoyance are vmState methods (in state.nim) which potentially
throw a base level Exception (a proc would only throws CatchableError)
* split p2p/chain into sub-modules and fix exception annotations
why:
make space for implementing PoA stuff
* provide over-loadable Clique PRNG
why:
There is a PRNG provided for generating reproducible number sequences.
The functions which employ the PRNG to generate time slots were ported
ported from the go-implementation. They are currently unused.
* implement trusted signer assembly in p2p/chain.persistBlocks()
details:
* PoA processing moved there at the end of a transaction. Currently,
there is no action (eg. transaction rollback) if this fails.
* The unit tests with staged blocks work ok. In particular, there should
be tests with to-be-rejected blocks.
* TODO: 1.Optimise throughput/cache handling; 2.Verify headers
* fix statement cast in pool.nim
* added table features to LRU cache
why:
Clique uses the LRU cache using a mixture of volatile online items
from the LRU cache and database checkpoints for hard synchronisation.
For performance, Clique needs more table like features.
details:
First, last, and query key added, as well as efficient random delete
added. Also key-item pair iterator added for debugging.
* re-factored LRU snapshot caching
why:
Caching was sub-optimal (aka. bonkers) in that it skipped over memory
caches in many cases and so mostly rebuild the snapshot from the
last on-disk checkpoint.
details;
The LRU snapshot toValue() handler has been moved into the module
clique_snapshot. This is for the fact that toValue() is not supposed
to see the whole LRU cache database. So there must be a higher layer
working with the the whole LRU cache and the on-disk checkpoint
database.
also:
some clean up
todo:
The code still assumes that the block headers are valid in itself. This
is particular important when an epoch header (aka re-sync header) is
processed as it must contain the PoA result of all previous headers.
So blocks need to be verified when they come in before used for PoA
processing.
* fix some snapshot cache fringe cases
why:
Must not index empty sequences in clique_snapshot module
* extract unused clique/mining support into separate file
why:
mining is currently unsupported by nimbus
* Replay first 51840 transactions from Goerli block chain
why:
Currently Goerli is loaded but the block headers are not verified.
Replaying allows real data PoA development.
details:
Simple stupid gzipped dump/undump layer for debugging based on
the zlib module (no nim-faststream support.)
This is a replay running against p2p/chain.persistBlocks() where
the data were captured from.
* prepare stubs for PoA engine
* split executor source into sup-modules
why:
make room for updates, clique integration should go into
executor/update_poastate.nim
* Simplify p2p/executor.processBlock() function prototype
why:
vmState argument always wraps basicChainDB
* split processBlock() into sub-functions
why:
isolate the part where it will support clique/poa
* provided additional processTransaction() function prototype without _fork_ argument
why:
with the exception of some tests, the _fork_ argument is always derived
from the other prototype argument _vmState_
details:
similar situation with makeReceipt()
* provide new processBlock() version explicitly supporting PoA
details:
The new processBlock() version supporting PoA is the general one also
supporting non-PoA networks, it needs an additional _Clique_ descriptor
function argument for PoA state (if any.)
The old processBlock() function without the _Clique_ descriptor argument
retorns an error on PoA networgs (e.g. Goerli.)
* re-implemented Clique descriptor as _ref object_
why:
gives more flexibility when moving around the descriptor object
details:
also cleaned up a bit the clique sources
* comments for clarifying handling of Clique/PoA state descriptor
Transaction and BlockHeader already updated in nim-eth repo
to support EIP-1559
EIP-1559 header validation and gasLimit validation
already implemented in previous commit
This commit deals with block validation:
- Effective gasPrice per EIP-1559
- new miner reward based on priorityFee
Previously max gas refunded was defined as gas_used div 2.
Here we name the constant 2 as MAX_REFUND_QUOTIENT and
change its value to 5.
The new equation will be: gas_used div MAX_REFUND_QUOTIENT
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
* continue importing rlp blocks
why:
a chain of blocks to be imported might have legit blocks
after rejected blocks
details:
import loop only stops if the import list is exhausted or if there
was a decoding error. this adds another four to the count of successful
no-hive tests.
* verify DAO marked extra data field in block header
why:
was ignored, scores another two no-hive tests
* verify minimum required difficulty in header validator
why:
two more nohive tests to succeed
details:
* subsumed extended header tests under validateKinship() and renamed it
more appropriately validateHeaderAndKinship()
* enhanced readability of p2p/chain.nim
* cleaned up test_blockchain_json.nim
* verify positive gasUsed unless no transactions
why:
solves another to nohive tests
details:
straightened test_blockchain_json chech so there is no unconditional
rejection anymore (based on the input test scenario)
* Re-adjust canonical head to parent of block to be inserted
why:
of the failing tests that remain to be solved, 30 of those will succeed
if the canonical database chain head is cleverly adjusted -- yes, it
looks like a hack, indeed.
details:
at the moment, this hack works for the non-hive tests only and is
triggered by a boolean argument passed on to the chain.persistBlocks()
method.
* Use parent instead of canonical head for block to be inserted
why:
side chains need to be inserted typically somewhere before the
canonical head.
details:
the previous _hack_ was unnecessary and removed, it was inspired by
some verification in persistBlocks() which explicitly referenced the
canonical head (which now might or might not refer to the newly inserted
header.)
* remove unnecessary code + comment
why:
some handy features were intended to support the unit test from
the clique/clique_test.go source (the other one is from
clique/snapshot_test.go.)
as this test cannot realistically be implemented without the full
api (includes mining support), it is left as that
Proper nested call functionality is being skipped in this iteration of new EVMC
host code to keep it simpler, to allow testing and architecture to be built
around the less complicated non-nested cases first.
Instead, nested calls use the old `Computation` path, and bypass any
third-party EVM that may be loaded. The results are the same, and mixing
different EVMs in this way is actually permitted in the EVMC specification.
This approach also means third-party EVMs we test don't need to support
precompiles and we don't need to specially handle those cases.
(E.g. "evmone" doesn't support precompiles, just EVM1 opcodes).
(These before/after scope actions are approximately copy-pasted from
`nimbus/vm/evmc_host.nim`, making their detailed behaviour "obviously correct".
Of course they are subject to tests as well. The small stack property of
a3c8a5c3 "EVMC: Small stacks when using EVMC, closes#575 (segfaults)" is
carefully retained.)
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Make the host service `setStorage` calculate the gas refund itself, instead of
depending on EVM internals.
In EVMC, host `setStorage` is responsible for adding the gas refund using the
rules of EIP-1283 (Constantinople), EIP-2200 (Istanbul) or EIP-2929 (Berlin).
It is not obvious that the host must do it from EVMC documentation, but it's
how it has to be. Note, this is very different from the gas _cost_, which the
host does not calculate.
Gas refund doesn't affect computation. It is applied after the whole
transaction is complete, so it can be tracked on the host or EVM side. But
`setStorage` doesn't return enough information for the EVM to calculate the
refund, so the host must do it when `setStorage` is used.
For now, this continues using Nimbus `Computation` just to hold the gas refund,
to fit around existing structures and get new EVMC working. But the host can't
keep using `Computation`, so gas refund will be moved out of it in due course.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
When processing log operations on the EVMC host side, it causes incorrect
`rootHash` results in some tests. This patch fixes the results.
The cause of these results is known: `Computation` is still doing parts of
contract scope entry/exit which need to be moved to the host. For now, as a
temporary workaround, update logs in `Computation` as it did before.
This makes test pass when using Nimbus EVM. (It breaks third-party EVMs when
`LOG*` ops are used, although most other tests pass.)
We can't keep this as it prevents complete host/EVM separation, but it's useful
in the current code, and it's fine to develop other functionality on top.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
When processing self destructs on the EVMC host side, it causes incorrect
`rootHash` results in some tests. This patch fixes the results.
The cause of these results is known: `Computation` is still doing parts of
contract scope entry/exit which need to be moved to the host. For now, as a
temporary workaround, update self destructs in `Computation` as it did before.
This makes test pass when using Nimbus EVM. (It breaks third-party EVMs when
`SELFDESTRUCT` ops are used, although most other tests pass.)
We can't keep this as it prevents complete host/EVM separation, but it's useful
in the current code, and it's fine to develop other functionality on top.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
jangko
Jordan Hrycaj
Kim De Mey
Zahary Karadjov
Jamie Lokier
bmoo
KonradStaniec