Extract the Engine trait #10958

dvdplm · 2019-08-10T14:09:12Z

Another big, noisy PR. This time the Engine trait and associated types are extracted to an engine crate. Along with this many of the client traits have been moved from ethcore/client/traits.rs into the client_traits crate.

To some degree the PR makes arbitrary decisions on which types/traits to move. The criteria used is not so much about what belongs where, but rather given that we want a free standing Engine, what else has to move? One example of such unclean division is how the engine crate now hosts two traits related to snapshotting.

The only relevant logic change is in Ethash: the epoch_verifier() method requires a type that implements EpochVerifier that can be stuck inside a ConstructedVerifier enum and before this PR this was achieved by an impl on Arc<Ethash>. When the Engine trait is no longer local, this is not possible. Instead there is now a local EpochVerifier struct that impls the EpochVerifier trait, much like the other engines do. This leads to somewhat convoluted code but otoh it saves us the doubly-arc-wrapped setup we had before.

Based on #10949

Move the BlockInfo trait to new crate

Contains code extracted from ethcore that defines `Machine`, `Externalities` and other execution related code.

…ropriate

* master: Allow default block parameter to be blockHash (#10932) Enable sealing when engine is ready (#10938) Fix some warnings and typos. (#10941)

* master: unify loading spec && further spec cleanups (#10948) refactor: Refactor evmbin CLI (#10742) journaldb changes (#10929)

* master: Fix compiler warnings in util/io and upgrade to edition 2018 Upgrade mio to latest (#10953)

Initial version of extracted Engine trait

Cleanup Executed as exported from machine crate

Cleanup

Sort out default impls for EpochVerifier

Fix Histogram

Don't export ethcore::client::*

Use types from the engine crate Explicit exports from engine::engine

…, ScheduleInfo, StateClient

tomusdrw

🎉 Awesome work! A bunch of tiny grumbles.

ethcore/Cargo.toml

tomusdrw · 2019-08-13T10:07:02Z

ethcore/client-traits/Cargo.toml

+vm = { path = "../vm" }
+trace = { path = "../trace" }
+ethcore-miner = { path = "../../miner" }
+kvdb = "0.1.0"


ethcore/engine/Cargo.toml

ethcore/engine/src/snapshot.rs

tomusdrw · 2019-08-13T10:15:11Z

ethcore/src/engines/authority_round/mod.rs

-use block::*;
-use client::EngineClient;
-use engines::{Engine, Seal, ConstructedVerifier};
+use client_traits::EngineClient;


I guess the next step would be to move specific engines implementations into it's own crates? ;)

You nailed it. :)

We're actually already doing that with our WIP Honey Badger engine:
https://github.com/poanetwork/parity-ethereum/tree/hbbft/ethcore/hbbft_engine
Saves a lot of compile time!

@afck See #10966 – once you get rid of the ethcore dependency compilation times are pretty great.

ethcore/src/engines/ethash.rs

tomusdrw · 2019-08-13T10:16:30Z

ethcore/src/engines/ethash.rs

@@ -207,17 +241,18 @@ impl Ethash {
 // for any block in the chain.
 // in the future, we might move the Ethash epoch
 // caching onto this mechanism as well.
-// NOTE[dvdplm]: the reason we impl this for Arc<Ethash> and not plain Ethash is the


ethcore/src/engines/ethash.rs

ethcore/src/engines/mod.rs

* master: Extract Machine from ethcore (#10949) removed redundant state_root function from spec, improve spec error types (#10955) Add support for Energy Web Foundation's new chains (#10957) [evmbin] add more tests to main.rs (#10956)

* master: Verify transaction against its block during import (#10954) [evmbin] fix compilation (#10976) Update to latest trie version. (#10972) [blooms-db] Fix benchmarks (#10974) Fix ethcore/benches build. (#10964) tx-pool: accept local tx with higher gas price when pool full (#10901) Disable unsyncable expanse chain (#10926)

debris

lgtm!

* master: [.gitlab.yml] cargo check ethcore benches (#10965)

* master: Extract the Engine trait (#10958) Better error message for rpc gas price errors (#10931) [.gitlab.yml] cargo check ethcore benches (#10965) Verify transaction against its block during import (#10954) [evmbin] fix compilation (#10976) Update to latest trie version. (#10972) [blooms-db] Fix benchmarks (#10974) Fix ethcore/benches build. (#10964) tx-pool: accept local tx with higher gas price when pool full (#10901) Disable unsyncable expanse chain (#10926)

dvdplm added 30 commits August 6, 2019 18:33

Add client-traits crate

ddeef45

Move the BlockInfo trait to new crate

New crate machine

0d49c45

Contains code extracted from ethcore that defines `Machine`, `Externalities` and other execution related code.

Use new machine and client-traits crates in ethcore

0465e6b

Use new crates machine and client-traits instead of ethcore where app…

50ca45a

…ropriate

Fix tests

29d4095

Don't re-export so many types from ethcore::client

cd6e0e2

Fixing more fallout from removing re-export

2477cbd

Merge branch 'master' into dp/chore/move-machine-to-own-crate

51ba2f4

* master: Allow default block parameter to be blockHash (#10932) Enable sealing when engine is ready (#10938) Fix some warnings and typos. (#10941)

fix test

c84a01f

More fallout from not re-exporting types

f2497f4

Add some docs

1e43534

cleanup

6d83cf2

import the macro edition style

e04d188

Merge branch 'master' into dp/chore/move-machine-to-own-crate

b6e91de

* master: unify loading spec && further spec cleanups (#10948) refactor: Refactor evmbin CLI (#10742) journaldb changes (#10929)

Tweak docs

bac7268

Add missing import

978eb88

Merge branch 'master' into dp/chore/move-machine-to-own-crate

d6273a7

* master: Fix compiler warnings in util/io and upgrade to edition 2018 Upgrade mio to latest (#10953)

remove unused ethabi_derive imports

18d6a94

Use latest ethabi-contract

d5700d9

Move many traits from ethcore/client/traits to client-traits crate

83bb140

Initial version of extracted Engine trait

Move snapshot related traits to the engine crate (eew)

d96fff3

Move a few snapshot related types to common_types

33aa152

Cleanup Executed as exported from machine crate

fix warning

9b70856

Gradually introduce new engine crate: snapshot

f76e5ed

ethcore typechecks with new engine crate

5b3a8ca

Sort out types outside ethcore

cecd69c

Add an EpochVerifier to ethash and use that in Engine.epoch_verifier()

c5ab534

Cleanup

Document pub members

45c0497

Sort out tests

b11a21f

Sort out default impls for EpochVerifier

Add test-helpers feature and move EngineSigner impl to the right place

af485cc

dvdplm added 2 commits August 10, 2019 15:20

More missing traits

881b71f

Fix Histogram

Fix tests and cleanup

ddca9ee

dvdplm requested review from debris and tomusdrw August 10, 2019 14:09

dvdplm self-assigned this Aug 10, 2019

dvdplm added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 10, 2019

dvdplm added 8 commits August 10, 2019 16:13

cleanup

d7d3f4b

Put back needed logger import

b2f8be6

Don't rexport common_types from ethcore/src/client

8ab2a02

Don't export ethcore::client::*

Remove files no longer used

4aef5f2

Use types from the engine crate Explicit exports from engine::engine

Get rid of itertools

951f63e

Move a few more traits from ethcore to client-traits: BlockChainReset…

1c42cc1

…, ScheduleInfo, StateClient

Move ProvingBlockChainClient to client-traits

07b1c2d

Don't re-export ForkChoice and Transition from ethcore

7e5e4bc

tomusdrw approved these changes Aug 13, 2019

View reviewed changes

dvdplm added 3 commits August 13, 2019 12:34

Address grumbles: sort imports, remove commented out code

b244bfb

Fix merge resolution error

88cfa7f

dvdplm mentioned this pull request Aug 13, 2019

Extract engines to own crates #10966

Merged

debris approved these changes Aug 15, 2019

View reviewed changes

dvdplm added 2 commits August 15, 2019 16:41

merge failure

60a03b2

Merge branch 'master' into dp/chore/extract-engine-trait

c75738e

* master: [.gitlab.yml] cargo check ethcore benches (#10965)

dvdplm merged commit 6a9de9b into master Aug 15, 2019

dvdplm deleted the dp/chore/extract-engine-trait branch August 15, 2019 15:59

This was referenced Nov 5, 2019

Backport code structure changes #11241

Closed

Backport code structure changes #11242

Closed

Backport code structure changes #11243

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extract the Engine trait #10958

Extract the Engine trait #10958

dvdplm commented Aug 10, 2019 •

edited

Loading

tomusdrw left a comment

tomusdrw Aug 13, 2019

tomusdrw Aug 13, 2019

dvdplm Aug 13, 2019

afck Aug 20, 2019

dvdplm Aug 20, 2019

tomusdrw Aug 13, 2019

debris left a comment

Extract the Engine trait #10958

Extract the Engine trait #10958

Conversation

dvdplm commented Aug 10, 2019 • edited Loading

tomusdrw left a comment

Choose a reason for hiding this comment

tomusdrw Aug 13, 2019

Choose a reason for hiding this comment

tomusdrw Aug 13, 2019

Choose a reason for hiding this comment

dvdplm Aug 13, 2019

Choose a reason for hiding this comment

afck Aug 20, 2019

Choose a reason for hiding this comment

dvdplm Aug 20, 2019

Choose a reason for hiding this comment

tomusdrw Aug 13, 2019

Choose a reason for hiding this comment

debris left a comment

Choose a reason for hiding this comment

dvdplm commented Aug 10, 2019 •

edited

Loading