Skip to content
This repository has been archived by the owner on Nov 6, 2020. It is now read-only.

Extract snapshot to own crate #11010

Merged
merged 18 commits into from
Sep 3, 2019
Merged

Extract snapshot to own crate #11010

merged 18 commits into from
Sep 3, 2019

Conversation

dvdplm
Copy link
Collaborator

@dvdplm dvdplm commented Aug 29, 2019

Extract snapshot to its own crate. Nothing particularly interesting to see here: shuffling files around and move types&traits to where they need to be. Adds a few more generics to the snapshot crate to disentangle it from ethcore::Client.
One needed refactoring is not present here: putting back SnapshotClient and SnapshotWriter in snapshot. This is taken care of in #11012 as it turned out to be more involved than I thought.

I have warp synced mainnet and I have observed snapshot chunks being created with this branch.

@niklasad1 Maybe this enables snapshots for the light client?

@dvdplm dvdplm self-assigned this Aug 29, 2019
@dvdplm dvdplm added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels Aug 29, 2019
@dvdplm dvdplm added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Aug 30, 2019
@dvdplm dvdplm marked this pull request as ready for review August 30, 2019 14:06
@dvdplm dvdplm added this to the 2.7 milestone Aug 30, 2019
ethcore/snapshot/Cargo.toml Outdated Show resolved Hide resolved
Copy link
Collaborator

@niklasad1 niklasad1 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, it introduces some additional trait bounds in a few of traits and types. I'm assuming this forced by removing the concrete type in favor of type parameters/ trait objects?!

Copy link
Collaborator

@ordian ordian left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you elaborate why the Send + Senc requirement wasn't needed before in several places and is now?

ethcore/snapshot/src/watcher.rs Show resolved Hide resolved
@dvdplm
Copy link
Collaborator Author

dvdplm commented Sep 2, 2019

Can you elaborate why the Send + Senc requirement wasn't needed before in several places and is now?

I wish I had something intelligent to say here, but tbh I followed the compilers' orders and she asked for Send + Sync (and 'static in some places) and I obeyed.
I explained it to myself by thinking that the concrete type Client was Send + Sync (à la "if a type is composed entirely of Send or Sync types, then it is Send or Sync") and so when replacing it with a generic, it needs the extra restrictions whenever the compiler sees that it is shared between threads.

@ordian ordian added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 3, 2019
@dvdplm dvdplm merged commit d193ddd into master Sep 3, 2019
dvdplm added a commit that referenced this pull request Sep 3, 2019
…he-right-place

* master:
  Extract snapshot to own crate (#11010)
  Edit publish-onchain.sh to use https (#11016)
  EIP 1108: Reduce alt_bn128 precompile gas costs (#11008)
  Fix deadlock in `network-devp2p` (#11013)
  Implement EIP-1283 reenable transition, EIP-1706 and EIP-2200 (#10191)
  EIP 1884 Re-pricing of trie-size dependent operations (#10992)
@niklasad1 niklasad1 deleted the dp/chore/extract-snapshot branch September 3, 2019 09:40
dvdplm added a commit that referenced this pull request Sep 3, 2019
* master:
  Extract snapshot to own crate (#11010)
  Edit publish-onchain.sh to use https (#11016)
ordian added a commit that referenced this pull request Sep 3, 2019
* master:
  Extract snapshot to own crate (#11010)
  Edit publish-onchain.sh to use https (#11016)
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
A8-looksgood 🦄 Pull request is reviewed well. M4-core ⛓ Core client code / Rust.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants