Commit Graph

460 Commits

Author SHA1 Message Date
jangko f2f204293e
first step into styleCheck fixes 2022-04-14 08:39:50 +07:00
Jordan Hrycaj 534fb528a4
Update terminal total difficulty handling (#992)
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.
2022-03-15 17:21:41 +00:00
jangko e461248e86
remove cruft 2022-03-09 21:43:51 +07:00
jangko f782327fcf
reimplement engine API rpc kiln spec v2 2022-03-03 11:28:18 +07:00
jangko 73e28694b5
move ttd from vm/state to chain_db 2022-03-03 08:58:55 +07:00
jangko 667cb6d750
EIP 4399 changes: random -> prevRandao 2022-02-27 14:37:35 +07:00
jangko 6374c9f66d
fixes regression caused by #962 2022-02-14 21:22:39 +07:00
jangko b4283aeb1f
fix nonce overflow related to CREATE/CREATE2
if the caller nonce == u64.high, the contract creation cannot
go on.
2022-02-10 15:48:38 +07:00
jangko 28cdfcaf6b
fix EIP-4399 'random' opcode
- fix previous implementation of EIP-4399
- now `random` opcode can be used with evmc_enabled
2022-02-08 20:23:40 +07:00
jangko d3fbe1d94d
fixes related to EIP-4399/EIP-3675
- fix `RANDOM` opcode export
- fix `difficulty` return value in VMState
2022-02-05 16:15:50 +07:00
jangko 71aa7e4b5c EIP-4399 implementation of nim-vm
what's new:
- `RANDOM` OPCODE
- `random` field of BlockHeader(previously `mixDigest`)
- `PostMerge` temporary name
2022-02-01 18:11:14 +02:00
Jordan Hrycaj 261c0b51a7
Redesign of BaseVMState descriptor (#923)
* 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.
2022-01-18 16:19:32 +00:00
Jamie Lokier 4b89ca3215
EVM: `writeContract` fixes, never return contract code as `RETURNDATA`
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>
2021-12-12 16:34:13 +07:00
Jamie Lokier 6ef9bfd21b
EVMC: Byte-endian conversions for 256-bit numeric values
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>
2021-12-10 16:23:27 +00:00
jangko baf508f6ae
move stateDB from VMState to chainDB
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.
2021-10-28 18:57:08 +07:00
jangko cec628e620
cleanup: remove unused accessLogs code from vm_state
they are not used anywhere at present, nor in the future
2021-10-28 11:30:18 +07:00
Jamie Lokier 5a5edb392a Bugfix: Incorrect processing of self-destructed, new contract
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>
2021-10-19 14:24:46 +01:00
Jamie Lokier 242dfdd5ac
Bugfix: Off by 1 in EIP-170 code size checks in `stateless`
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>
2021-10-19 10:30:53 +01:00
jangko 908dc21478
evm: fixes EIP2929 opcodes
op balanceEIP2929, extCodeHashEIP2929, extCodeSizeEIP2929, and
extCodeCopyEIP2929 are fixed due to their wrong gasConsume
position
2021-09-22 11:58:06 +07:00
jangko 69f2a0f95a
config: replace stdlib parseOpt with nim-confutils
fixes #581
2021-09-18 17:34:46 +07:00
bmoo b09ad5cacb
code cleanup removed unused imports 2021-08-18 10:35:36 +07:00
Jamie Lokier a7b40b0762
EVM: Use the EVMC calls for EIP-2929 access-list and refactor in EVM
Signed-off-by: Jamie Lokier <jamie@shareable.org>
2021-08-11 19:47:38 +07:00
Jamie Lokier 74f53c7761
EVMC: Add missing EIP-2929 (Berlin) functions to EVMC host
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>
2021-08-11 19:47:34 +07:00
Jamie Lokier 11f03a1846
Transaction: EVMC fix, `CREATE2` salt is a 256-bit blob not a number
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>
2021-08-05 10:35:52 +01:00
Jordan Hrycaj dc8ef09727
fix CI failing 2021-08-05 12:27:14 +07:00
Jordan Hrycaj 4713bd4cf4
#768 Moved/re-implemented ecRecover() from Clique sources to utils/ec_recover
why:
  The same functionality was differently implemented in one or the
  other form.

details:
  Caching and non-caching variants available
2021-08-05 12:27:10 +07:00
Jamie Lokier ab9067133c
Tracing: Remove some trace messages that occur a lot during sync
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>
2021-07-27 14:12:55 +01:00
jangko 8482cb3ed3
EIP-3541: Fixes typo, 0xFE -> 0xEF 2021-06-30 20:44:34 +07:00
jangko db8988fe64
EIP-1559: Fee market change for ETH 1.0 chain
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
2021-06-30 20:30:39 +07:00
jangko 05d905b136
EIP-3529: Replace SSTORE_CLEARS_SCHEDULE
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
2021-06-29 07:37:17 +07:00
jangko 8982e6c649
EIP-3529: Remove the SELFDESTRUCT refund.
- remove it from both nim-evm and nim-evm2
2021-06-29 07:37:17 +07:00
jangko e08c9ef2d9
EIP-3541: Reject new contracts starting with the 0xEF byte 2021-06-29 07:36:56 +07:00
jangko 05e9b891f0
EIP-3198: add baseFee op code in nim-evm 2021-06-29 07:35:16 +07:00
jangko 5159ad7aac
preparation for London hard fork
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
2021-06-29 07:34:45 +07:00
Jamie Lokier df71c8bec9
EVMC: Disable byte-endian conversion of 256-bit values on EVM side
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>
2021-06-08 18:29:39 +01:00
Jamie Lokier 7c90d8de70
EVM: Remove `vm_forks` everywhere, use common forks list instead
The common forks list was already used, redirected via `vm_forks` for
historical compatibility.  Remove the old `vm_forks` now and divert all imports
to the common forks list outside the EVM.

Signed-off-by: Jamie Lokier <jamie@shareable.org>
2021-06-08 15:36:31 +01:00
Jamie Lokier 05bc174bef
Forks: Use a common fork list outside the EVMs
Many places outside the EVM use `Fork` and the fork list, and in general we
want progressively fewer dependencies on EVM internal types and files.

This may prove to be a temporary location, especially when we implement
issue #640.  But it's a fine temporary location if so.

Signed-off-by: Jamie Lokier <jamie@shareable.org>
2021-06-08 15:36:31 +01:00
Jamie Lokier b3a788c7ce
Transaction: Move contract address generation outside the EVM
The current EVM generates its own new contract addresses, and this is why there
are separate `msg.contractAddress` and `msg.codeAddress` fields in the
computation start message.

In EVMC, account updates are only allowed on the host side, including contract
generation, and the start message has one destination field, `msg.destination`.
The EVM cannot select addresses, only use them.  It's a sensible design.

The difference makes the current EVM incompatible with EVMC and its message
format, so this patch corrects the difference.  It moves contract address
generation to the host side.  This simplifies the EVM and its API a little.

(As an API change, this is incompatible with vm2, so it's guarded under
`evmc_enabled` to allow vm2 to continue to build and run at this time.  This is
also why there are fewer deletions than would otherwise be expected.)

Signed-off-by: Jamie Lokier <jamie@shareable.org>
2021-06-08 15:36:30 +01:00
Jamie Lokier 775231eef1
EVM: Apply EIP-6 in the code (affects both vm and vm2)
The rationale in EIP-6[1] for changing names to `selfDestruct` applies to code
as much as it does to specs.  Also, Ethereum uses the new names consistently,
so it's useful for our code to match the terms used in later EIP specs and
testsuite entries.

This change is straightforward, and is a prerequisite for patches to come that
do things with the `selfDestruct` fields.

[1] https://eips.ethereum.org/EIPS/eip-6
Hudson Jameson, "EIP-6: Renaming SUICIDE opcode," Ethereum Improvement
Proposals, no. 6, November 2015.

Signed-off-by: Jamie Lokier <jamie@shareable.org>
2021-06-08 15:36:30 +01:00
Jamie Lokier 537cac1bf5
EVM: Move where `continuation` is cleared to fix a potential stall
This fixes a bug spotted by @mjfh that was introduced by commit 2a7ccceb:

    try:
      if not c.continuation.isNil:
        (c.continuation)()
        c.continuation = nil
      c.selectVM(fork)
    except CatchableError as e:
      ...

The call to `(c.continuation)()` was moved by 2a7ccceb inside the `try` so
that, like all the Op functions do already, if the continuation raises, the
interpreter's general catch turns the exception into a an error status result.

But if the continuation raises an exception, `continuation` is not cleared in
the next line, and at the next resumption the continuation is called again.
It may loop doing this.

This doesn't currently happen because the continuations don't really raise, but
it's still a correctness issue.

This fix also allows a continuation to spawn a second continuation, if it
encounters a second suspension point.  This also doesn't happen currently,
but the pattern will become useful with async EVM.

Signed-off-by: Jamie Lokier <jamie@shareable.org>
2021-05-27 12:16:37 +01:00
jangko a0d10f5728
drop PublicNetwork enum usage and replace it with NetworkId
we cannot limit the `--networkid` switch to values available in
`PublicNetwork` enum. it should able to accept very wide range of
custom NetworkId.
2021-05-20 14:04:16 +07:00
jangko 76543da456
disable EIP-2537: Precompile for BLS12-381 curve operations
reason: not included in berlin hard fork

but we keep the code around, for future inclusion
2021-05-17 01:29:03 +07:00
jangko 3ccc4642f2
disable EIP-2315: Simple Subroutines for the EVM
reason: not included in berlin hard fork
2021-05-17 01:29:03 +07:00
jangko 6fc3df637c
reenable EIP-2565: modExp gas cost
now it's officially included in berlin hard fork
2021-05-17 01:28:31 +07:00
jangko 79044f1e92
eip2718: test_blockchain_json pass test 2021-05-15 18:09:35 +07:00
jangko f6a0e4bcbd
fixes wrong usage of `chainId` in places where it should be networkId
fixes #643
2021-05-12 09:45:09 +07:00
Jamie Lokier 4187eb1959
Transaction: Prepare txRefundGas to support txCallEvm
There's only one call left to `refundGas(Transaction, ...)`, and the
similarity to the tail of `rpcEstimateGas` is obvious.

Gather this into `call_evm`: `refundGas` -> `txRefundGas`.

Signed-off-by: Jamie Lokier <jamie@shareable.org>
2021-05-03 19:51:20 +01:00
Jamie Lokier 52fd8b8129
Transaction: Prepare txSetupComputation to support txCallEvm
After recent changes, there's only one call left to `setupComputation`, and
it's just a variant like `rpcSetupComputation` but for transaction processing.
The similarity to `rpcSetupComputation` is obvious.

Gather this into `call_evm`: `setupComputation` -> `txSetupComputation`.

Signed-off-by: Jamie Lokier <jamie@shareable.org>
2021-05-03 19:51:20 +01:00
Jamie Lokier 2a7ccceb3e
EVM: Make continuation exceptions behave as they did before
The account database code is not supposed to raise exceptions in the EVM, and
the behaviour is not well defined if it does.  It isn't compliant with EVMC
spec either.  But that will be dealt with properly when the account state-cache
is dealt with, as there is some work to be done on it.

Meanwhile, if it raises in code under `chainTo` and then `(continuation)()`,
the behaviour was changed slightly by the stack-shrink patches.

Before those patches, an exception after the recursion-point was converted to
`c.setError` "Opcode Dispatch Error" in `executeOpcodes.  After, it would
propagate out, a different behaviour.  (It still correctly walked the chain of
`c.dispose()` calls to clean up.)

It's easy to restore the original behaviour just by moving the continuation
call, so let's do that.

Signed-off-by: Jamie Lokier <jamie@shareable.org>
2021-04-30 11:32:42 +07:00
Jamie Lokier a3c8a5c3f3
EVMC: Small stacks when using EVMC, closes #575 (segfaults)
This patch reduces stack space used with EVM in ENABLE_EVMC=1 mode, from 13 MB
worst case to 550 kB, a 24x reduction.

This completes fixing the "stack problem" and closes #575 (`EVM: Different
segmentation faults when running the test suite with EVMC`).

It also closes #256 (`recursive EVM call trigger unrecoverable stack overflow`).

After this patch, it is possible to re-enable the CI targets which had to be
disabled due to #575.

This change is also a required precursor for switching over to "nearly EVMC" as
the clean and focused Nimbus-internal API between EVM and sync/database
processes, and is also key to the use of Chronos `async` in those processes
when calling the EVM.

(The motivation is the internal interface has to be substantially changed
_anyway_ for the parallel sync and database processes, and EVMC turns out to be
well-designed and well-suited for this.  It provides good separation between
modules, and suits our needs better than our other current interface.  Might as
well use a good one designed by someone else.  EVMC is 98% done in Nimbus
thanks to great work done before by @jangko, and we can use Nimbus-specific
extensions where we need flexibility, including for performance.  Being aligned
with the ecosystem is a useful bonus feature.)

All tests below were run on Ubuntu 20.04 LTS server, x86-64.  This matches one
of the targets that has been disabled for a while in CI in EVMC mode due to
stack overflow crashing the tests, so it's a good choice.

Measurements before
===================

Testing commit `e76e0144 2021-04-22 11:29:42 +0700 add submodules: graphql and
toml-serialization`.

    $ rm -f build/all_tests && make ENABLE_EVMC=1 test
    $ ulimit -S -s 16384 # Requires larger stack than default to avoid crash.
    $ ./build/all_tests 9 | tee tlog
    [Suite] persist block json tests
    ...
    Stack range 38416 depthHigh 3
    ...
    Stack range 13074720 depthHigh 1024
    [OK] tests/fixtures/PersistBlockTests/block1431916.json

These tests use 13.07 MB of stack to run, and so crash with the default stack
limit on Ubuntu Server 20.04 (8MB).  Exactly 12768 bytes per EVM call stack
frame.

    $ rm -f build/all_tests && make ENABLE_EVMC=1 test
    $ ulimit -S -s 16384 # Requires larger stack than default.
    $ ./build/all_tests 7 | tee tlog
    [Suite] new generalstate json tests
        ...
    Stack range 14384 depthHigh 2
        ...
    Stack range 3495456 depthHigh 457
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest639.json
    ...
    Stack range 3709600 depthHigh 485
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest458.json
        ...
    Stack range 7831600 depthHigh 1024
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stCreate2/Create2OnDepth1024.json

These tests use 7.83MB of stack to run.  About 7648 bytes per EVM call stack
frame.  It _only just_ avoids crashing with the default Ubuntu Server stack
limit of 8 MB.  However, it still crashes on Windows x86-64, which is why the
Windows CI EVMC target is currently disabled.

On Linux where this passes, this is so borderline that it affects work and
testing of the complex storage code, because that's called from the EVM.

Also, this greatly exceeds the default thread stack size.

Measurements after
==================

    $ rm -f build/all_tests && make ENABLE_EVMC=1 test
    $ ulimit -S -s 600 # Because we can!  600k stack.
    $ ./build/all_tests 9 | tee tlog
    [Suite] persist block json tests
    ...
    Stack range 1936 depthHigh 3
    ...
        Stack range 556272 depthHigh 1022
        Stack range 556512 depthHigh 1023
        Stack range 556816 depthHigh 1023
        Stack range 557056 depthHigh 1024
        Stack range 557360 depthHigh 1024
        [OK] tests/fixtures/PersistBlockTests/block1431916.json

    $ rm -f build/all_tests && make ENABLE_EVMC=1 test
    $ ulimit -S -s 600 # Because we can!  600k stack.
    $ ./build/all_tests 7 | tee tlog
    [Suite] new generalstate json tests
        ...
    Stack range 1392 depthHigh 2
        ...
    Stack range 248912 depthHigh 457
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest639.json
    ...
    Stack range 264144 depthHigh 485
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stRandom2/randomStatetest458.json
        ...
        Stack range 557360 depthHigh 1024
    [OK] tests/fixtures/eth_tests/GeneralStateTests/stStaticCall/static_CallRecursiveBombPreCall.json

For both tests, a satisfying *544 bytes* per EVM call stack frame, and EVM
takes less than 600 kB total.  With other overheads, both tests run in 600 kB
stack total at maximum EVM depth.

We must add some headroom on this for database activity called from the EVM,
and different compile targets.  But it means the EVM itself is no longer a
stack burden.

This is much smaller than the default thread stack size on Linux (2MB), with
plenty of margin.  (Just fyi, it isn't smaller than a _small_ thread stack on
Linux from a long time ago (128kB), and some small embedded C targets.)

This size is well suited to running EVMs in threads.

Further reduction
=================

This patch solves the stack problem.  Windows and Linux 64-bit EVMC CI targets
can be re-enabled, and there is no longer a problem with stack usage.

We can reduce further to ~340 bytes per frame and 350 kB total, while still
complying with EVMC.  But as this involves changing how errors are handled to
comply fully with EVMC, and removing `dispose` calls, it's not worth doing now
while there are other EVMC changes in progress that will have the same effect.

A Nimbus-specific extension will allow us to avoid recursion with EVMC anyway,
bringing bytes per frame to zero.  We need the extension anyway, to support
Chronos `async` which parallel transaction processing is built around.

Interop with non-Nimbus over EVMC won't let us avoid recursion, but then we
can't control the stack frame size either.  To prevent stack overflow in
interop I anticipate using (this method in Aleth)
[6e96ce34e3/libethereum/ExtVM.cpp (L61)].

Smoke test other versions of GCC and Clang/LLVM
===============================================

As all builds including Windows use GCC or Apple's Clang/LLVM, this is just to
verify we're in the right ballpark on all targets.  I've only checked `x86_64`
though, not 32-bit, and not ARM.

It's interesting to see GCC 10 uses less stack.  This is because it optimises
`struct` returns better, sometimes skipping an intermediate copy.  Here it
benefits the EVMC API, but I found GCC 10 also improves the larger stack usage
of the rest of `nimbus-eth1` as well.

Apple clang 12.0.0 (clang-1200.0.26.2) on MacOS 10.15:

- 544 bytes per EVM call stack frame

GCC 10.3.0 (Ubuntu 10.3.0-1ubuntu1) on Ubuntu 21.04:

- 464 bytes per EVM call stack frame

GCC 10.2.0 (Ubuntu 10.2.0-5ubuntu1~20.04) on Ubuntu 20.04 LTS:

- 464 bytes per EVM call stack frame

GCC 11.0.1 20210417 (experimental; Ubuntu 11-20210417-1ubuntu1) on Ubuntu 21.04:

- 8 bytes per EVM call stack frame

GCC 9.3.0 (Ubuntu 9.3.0-17ubuntu1~20.04) on Ubuntu 20.04 LTS:

- 544 bytes per EVM call stack frame

GCC 8.4.0 (Ubuntu 8.4.0-3ubuntu2) on Ubuntu 20.04 LTS:

- 544 bytes per EVM call stack frame

GCC 7.5.0 (Ubuntu 7.5.0-6ubuntu2) on Ubuntu 20.04 LTS:

- 544 bytes per EVM call stack frame

GCC 9.2.1 20191008 (Ubuntu 9.2.1-9ubuntu2) on Ubuntu 19.10:

- 528 bytes per EVM call stack frame

Signed-off-by: Jamie Lokier <jamie@shareable.org>
2021-04-27 05:53:32 +01:00