Make ClientIoMessage generic over the Client #10981

dvdplm · 2019-08-19T20:20:14Z

The one big remaining issue in ethcore is the ClientIoMessage type which has an Execute variant that embeds a Client. This is a blocker for several things, e.g. making a freestanding ethcore/light, extracting verification etc.
This PR attempts to disentangle this type by making it generic and extracts verification from ethcore.

There are some questionable changes made here, in particular wrt to the traits used/modified. Careful review of these areas is much appreciated.

the ImportBlock trait gains a method despite the FIXME that seems to indicate that the trait should not even exist
the new Tick trait
hijacking the SnapshotClient trait (maybe the SnapshotWriter trait should be combined with this?)
~~making the light client's fetcher type param 'static – better way?~~

After this PR we can build the eth light client without depending on Client.

Based on #10978.

I have synced mainnet with this branch.

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

ordian

This introduces some changes that need to be reverted later, but the ability to build the light client w/o ethcore is cool!

ethcore/verification/src/lib.rs

ethcore/client-traits/src/io_message.rs

ethcore/client-traits/src/lib.rs

ethcore/src/client/client.rs

ethcore/client-traits/src/lib.rs

* master: Fix compilation on recent nightlies (#10991) [ipfs] Convert to edition 2018 (#10979)

* master: fix compile warnings (#10993)

niklasad1 · 2019-08-27T16:47:07Z

ethcore/light/src/client/service.rs

 	client: Arc<Client<T>>,
-	io_service: IoService<ClientIoMessage>,
+	io_service: IoService<ClientIoMessage<()>>,


why is this ()?

does it work because we only care about ClientIoMessage::BlockVerified for the light client?

Great question. So before this PR light depends on ethcore essentially only for ClientIoMessage, and then only because it has a Execute variant that embeds a Callback which is a Fn that takes an ethcore::Client as a param… So the dependency was actually not "real" but merely ceremonial, to satisfy the types as they were set up.
Once I made the Callback generic over the Client (and all the other types using it) @ordian spotted that it's actually not necessary to use anything other than () in light.

Now, we all agree that ClientIoMessage<()> looks plenty odd and hopefully someone will come along and refactor this further. ;)
In particular I know that @ngotchac is working on changes to the IO code that may, as a desired side-effect, result in a better message passing infrastructure, one that does not need a Client (or whatever stand-in we use).

Ok, I see.

In particular I know that @ngotchac is working on changes to the IO code that may, as a desired side-effect, result in a better message passing infrastructure, one that does not need a Client (or whatever stand-in we use).

Cool 👍

niklasad1

nice work.

I have one remaining question regarding the light-client when that is answered you'll get my final approval!

niklasad1

LGTM

* master: Make ClientIoMessage generic over the Client (#10981) bump spin to 0.5.2 (#10996) fix compile warnings (#10993) Fix compilation on recent nightlies (#10991) [ipfs] Convert to edition 2018 (#10979)

…/eip-1884-reprice-trie-size-dependent-ops * dp/feature/eip-1344-add-ChainID-opcode: more merge fallout Update ethcore/vm/src/schedule.rs Fix a few warnings merge conflict error Make ClientIoMessage generic over the Client (#10981) bump spin to 0.5.2 (#10996) fix compile warnings (#10993) Fix compilation on recent nightlies (#10991) [ipfs] Convert to edition 2018 (#10979)

* master: EIP 1884 Re-pricing of trie-size dependent operations (openethereum#10992) xDai chain support and nodes list update (openethereum#10989) [trace] check mem diff within range (openethereum#11002) EIP-1344 Add CHAINID op-code (openethereum#10983) Make ClientIoMessage generic over the Client (openethereum#10981) bump spin to 0.5.2 (openethereum#10996) fix compile warnings (openethereum#10993)

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

cleanup

fa9cf5d

ordian approved these changes Aug 26, 2019

View reviewed changes

ethcore/verification/src/lib.rs Show resolved Hide resolved

ordian added this to the 2.7 milestone Aug 26, 2019

dvdplm added 2 commits August 26, 2019 14:22

Parametrise ClientIoMessage with () for the light client

7f4c7e5

Add impl Tick for ()

0c8cb9e

dvdplm requested a review from sorpaas August 26, 2019 18:04

niklasad1 reviewed Aug 27, 2019

View reviewed changes

ethcore/client-traits/src/io_message.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Aug 27, 2019

View reviewed changes

ethcore/client-traits/src/lib.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Aug 27, 2019

View reviewed changes

ethcore/client-traits/src/lib.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Aug 27, 2019

View reviewed changes

ethcore/client-traits/src/lib.rs Show resolved Hide resolved

niklasad1 reviewed Aug 27, 2019

View reviewed changes

ethcore/client-traits/src/lib.rs Show resolved Hide resolved

niklasad1 reviewed Aug 27, 2019

View reviewed changes

ethcore/src/client/client.rs Outdated Show resolved Hide resolved

niklasad1 reviewed Aug 27, 2019

View reviewed changes

ethcore/client-traits/src/lib.rs Outdated Show resolved Hide resolved

dvdplm added 5 commits August 27, 2019 17:07

Address review feedback

23a832c

Move ClientIoMessage to common-types

0dfe3c9

Merge branch 'master' into dp/chore/sort-out-ClientIoMessage

1e9f93a

* master: Fix compilation on recent nightlies (#10991) [ipfs] Convert to edition 2018 (#10979)

remove superseeded fixme

4e8ab46

Merge branch 'master' into dp/chore/sort-out-ClientIoMessage

22df32b

* master: fix compile warnings (#10993)

dvdplm requested a review from niklasad1 August 27, 2019 16:02

fix merge conflict errors

ad1999f

niklasad1 reviewed Aug 27, 2019

View reviewed changes

niklasad1 approved these changes Aug 28, 2019

View reviewed changes

niklasad1 added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 28, 2019

dvdplm merged commit cd26526 into master Aug 28, 2019

niklasad1 deleted the dp/chore/sort-out-ClientIoMessage branch August 28, 2019 08:29

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Make ClientIoMessage generic over the Client #10981

Make ClientIoMessage generic over the Client #10981

dvdplm commented Aug 19, 2019 •

edited

Loading

ordian left a comment •

edited

Loading

niklasad1 Aug 27, 2019

dvdplm Aug 27, 2019 •

edited

Loading

niklasad1 Aug 28, 2019

niklasad1 left a comment •

edited

Loading

niklasad1 left a comment

Make ClientIoMessage generic over the Client #10981

Make ClientIoMessage generic over the Client #10981

Conversation

dvdplm commented Aug 19, 2019 • edited Loading

ordian left a comment • edited Loading

Choose a reason for hiding this comment

niklasad1 Aug 27, 2019

Choose a reason for hiding this comment

dvdplm Aug 27, 2019 • edited Loading

Choose a reason for hiding this comment

niklasad1 Aug 28, 2019

Choose a reason for hiding this comment

niklasad1 left a comment • edited Loading

Choose a reason for hiding this comment

niklasad1 left a comment

Choose a reason for hiding this comment

dvdplm commented Aug 19, 2019 •

edited

Loading

ordian left a comment •

edited

Loading

dvdplm Aug 27, 2019 •

edited

Loading

niklasad1 left a comment •

edited

Loading