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>
In the unusual case where log data is zero-length, `data[0].addr` is invalid
and Nim thoughtfully raises `IndexOutOfBounds`, a `Defect` so it's not even
in `CatchableError`.
This is done in the EVMC host services to handle `LOG*` ops, and it made one
of the EVM tests silently fail with no error message. The fix is obvious.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Show calls from the host into the EVM. Shows the call, `evmc_message` fields,
and `evmc_result` fields when the call returns.
(When `show_tx_calls` is manually set to true.)
Signed-off-by: Jamie Lokier <jamie@shareable.org>
When `show_tx_calls` is manually set to true, show all the calls from the EVM
to the host, including name, arguments and results.
For example this shows each call to `setStorage`, the key, value and storage
result. This output allows the externally-visible activity of an EVM to be
seen, and it's been useful for guessing what went wrong when a test fails.
In theory, if two EVMs show the same activity in this log, they should have the
same effect on account states, gas, etc. and the same final `roothash`
(which is the only value some tests check).
ps. Ideally we'd use `{.push show.}`...`{.pop.}`, just like with `inline`.
But we can't: https://github.com/nim-lang/Nim/issues/12867
Signed-off-by: Jamie Lokier <jamie@shareable.org>
New pragma `{.show.}` on a proc definition turns on tracing for that proc.
Every call to it shows the name, arguments and results, if `show_tx_calls` is
manually set to true. This is to trace calls from EVM to host.
This started as a template which took a block expression, but the closure it
used led to illegal capture errors. It was easier to write a macro.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
1. Send all EVM executions through the EVMC `execute` function.
It leads to the same place in the end as calling `Computation` before, but
`execute` is the API function used by all EVMC implementations, and it is
very explicit what data is passed back and forth.
2. As a consequence this starts using the new `host_services` code from EVM, so
this is a significant change to the paths used for account state processing.
3. Because we will have to remove the `newComputation` call on the host side,
anticipating that the contract code is now saved in `host` instead of being
copied around. As it's saved in `host`, there is no need to pass it
separately to `evmcExecComputation`.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
We'll re-enable endian conversions based on a negotiated run-time option later,
but for now let's remove one complication to testing the new EVMC paths, and
also gain a little performance.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Even though `evmc_create_nimbus_evm` is called, it fails at link time because
the definition of that function isn't included unless it is pulled in
explicitly.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
This provides the functions a loadable VM must provide for a host to use it.
The main access to a loaded EVM is `evmc_create_nimbus_evm`, and this meant to
be the only exported function the caller starts with.
That provides access to other functions, also defined in this patch, to
configure the EVM and then the key interesting function is `execute`.
`execute` runs a full computation, here using Nimbus EVM `Computation`.
(Note, even though everything is EVMC binary-compatible, there is a small
dependency on `TransactionHost` in `execute` here, which prevents this being
used by a host that is not Nimbus at the moment. It is necessary for some
tests, and will eventually go away.)
Although this provides the VM-side functionality needed by the host, it does
not contain the glue functions for `Computation` to call the host, which are
already part of the Nimbus EVM in `nimbus/vm/evmc_api.nim`.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
1. This provides the necessary type adjustments for host services to be
(optionally) callable via the EVMC binary-compatible interface. This layer
is stashed away in a glue module so the host services continue to use
appropriate Nim types, and are still callable directly.
Inlining is used to ensure there should be no real overhead, including stack
frame size for the `call` function. Note, `import` must be used for
`{.inline.}` to work effectively.
2. This also provides a key call in the other direction, the version of host to
EVM `execute` that is called on the host side.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
This provides "host services", functions provided by the application to an EVM.
They are a key part of EVMC compatibility, but we will switch to using these
with "native" EVM as well.
These are functions like `getStorage`, `setStorage` and `emitLog` for accessing
the account state, because the EVM is not allowed direct access to the database.
This code is adapted from `nimbus/vm/evmc_host.nim` and other places, but there
is more emphasis on being host-side only, no dependency on the EVM or
`Computation` type. It uses `TransactionHost` and types in `host_types`.
These host services have two goals: To be compatible with EVMC, and to be a
good way for the Nimbus EVM to access the data it needs. In our new Nimbus
internal architecture, the EVM will only access the databases and other
application state via these host service functions.
The reason for containing the EVM like this, even "native" EVM, is that having
one good interface to the data makes it a lot easier to change how the database
works, which is on the roadmap.
These functions almost have EVMC signatures, but they are not binary compatible
with EVMC. (Binary compatibility is provided by another module). It would be
fine for Nimbus EVM to call these functions directly when linked directly.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Block validation failure isn't an error, it's correct rejection of a bad block
from the network. All conditions that lead to block rejection return a simple
boolean.
When a block is rejected, most reasons log at `debug` level. Only `stateRoot`
mismatch shouts a loud, highlighted, multi-line error message with big red
`error` alert.
Historically this was to assist EVM development, because it was more likely to
be a Nimbus EVM bug than a real bad block. But now the EVM is in good shape,
has a large and thorough testsuite, and `stateRoot` mismatch is more likely to
be a real bad block that should be rejected with less fuss.
If there's a genuine EVM bug, we'll still get an alert: Consensus failure will
quickly become obvious, and the block where it happens is easily fetched.
So a big, loud error is no longer useful, and it became a problem during tests.
Recently a few hundred tests were added that trigger it, and now successful
test output is filled with attention-grabbing errors which aren't really errors
or particularly useful.
Since it's not really an error, the original motivation is now backwards, and
other reasons warn at `debug` level, make this like the others.
Signed-off-by: Jamie Lokier <jamie@shareable.org>
Jamie Lokier
Jordan Hrycaj
Kim De Mey
jangko
KonradStaniec