[part 1/2] Warp sync on light client #8723

ordian · 2018-05-28T15:01:01Z

Part of #6010.

This part doesn't include background old header sync.

parity-cla-bot · 2018-05-28T15:01:04Z

It looks like @ordian signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

dvdplm · 2018-05-28T18:50:09Z

ethcore/light/src/client/mod.rs

 			proof: proof,
 		});
 		self.db.write_buffered(batch);
 		Ok(())
 	}
 }

+/// An ungly borrow-checker workaround to allow iterating over a block and its ancestry.


s/ungly/ugly/ (?) – and also, was this meant to be a doc-comment?

dvdplm · 2018-05-28T18:56:49Z

ethcore/light/src/client/mod.rs

+	fn restore_db(&self, new_db: &str, spec: &Spec, hs: HardcodedSync) -> Result<(), Error> {
+		trace!(target: "snapshot", "Replacing light client database with {:?}", new_db);
+
+		let _import_lock = self.import_lock.lock();


Out of curiosity, why not try_lock_for() here?

try_lock_for() is a timed lock. What should we do if the timeout is reached? If try again, then this is no better than lock().

Well, perhaps exit with an error message?

dvdplm · 2018-05-28T19:05:46Z

ethcore/light/src/client/snapshot/chain.rs

+		};
+		match result {
+			Ok(pending) => *self.pending.write() = Some(pending),
+			// TODO: panic? abort snapshot sync?


Why not return error upstream and have?

fn insert_epoch_transition(… …) -> Result<…> { if header.number() == 0 { let td = self.chain.genesis_header().difficulty(); self.chain.insert_with_td(batch, header, td, Some(transition.proof)) } else { self.chain.insert(batch, header, Some(transition.proof)) } }

EDIT I see: the new trait wraps previous code that doesn't use Return much. Is it worthwhile to change that perhaps, so that new code can use more idiomatic error handling?

rphmeier · 2018-05-29T19:45:20Z

ethcore/light/src/client/header_chain.rs

@@ -564,6 +510,67 @@ impl HeaderChain {
 		Ok(pending)
 	}

+	fn produce_next_cht_root(
+		&self, from_era: u64, candidates: &mut BTreeMap<u64, Entry>,


style: each param should get its own line if there are too many for one line

rphmeier · 2018-05-29T19:46:26Z

ethcore/light/src/client/header_chain.rs

+		self.cache.clone()
+	}
+
+	/// Get the database column.


would be good to elaborate on what this column is used for

rphmeier · 2018-05-29T19:49:07Z

ethcore/light/src/client/mod.rs

 			proof: proof,
 		});
 		self.db.write_buffered(batch);
 		Ok(())
 	}
 }

+// An ugly borrow-checker workaround to allow iterating over a block and its ancestry.
+/// An iterator over a block and its ancestry.
+pub struct AncestryIter<'a>(RwLockReadGuard<'a, HeaderChain>);


why is this necessary?

rphmeier · 2018-05-29T19:52:23Z

ethcore/src/engines/mod.rs

@@ -342,9 +342,15 @@ pub trait Engine<M: Machine>: Sync + Send {
 		None
 	}

+	/// Create a factory for restoring from snapshot chunks.
+	/// Returning `None` indicates that this engine doesn't support restoring from a snapshot.
+	fn snapshot_restoration(&self) -> Option<Box<RebuilderFactory>> {


Should be unified with SnapshotComponents

ordian

Tested locally, basically it worked, although there are a couple of unresolved questions left as comments. Also it would be nice to have tests for this and reduce code duplication with the full chain.

ordian · 2018-06-27T11:53:13Z

ethcore/light/src/client/snapshot/chain.rs

+	}
+
+	fn add_child(&self, _batch: &mut DBTransaction, _block_hash: H256, _child_hash: H256) {
+		// TODO


Should we store this parent<->child relationship in the database similar to a full node
https://github.com/paritytech/parity/blob/0bed5976e368e948566f5a445f5fa17088674f3d/ethcore/src/blockchain/blockchain.rs#L934-L947?

yes, something like that

ordian · 2018-06-27T11:55:22Z

ethcore/light/src/client/header_chain.rs

-					.expect("blocks are only inserted if parent is present; or this is the block we just added; qed");
+				let canon_pos = match entry.candidates.iter().position(|x| x.hash == canon_hash) {
+					Some(pos) => pos,
+					None => break,


Not sure about correctness here.

Why not return an Err?

5chdn · 2018-07-02T13:04:14Z

Is this to be reviewed? "[WIP] [1/2]"?

niklasad1 · 2018-07-09T12:39:53Z

ethcore/light/Cargo.toml

@@ -36,10 +36,13 @@ triehash = { path = "../../util/triehash" }
 kvdb = { path = "../../util/kvdb" }
 memory-cache = { path = "../../util/memory_cache" }
 error-chain = { version = "0.12", default-features = false }
+util-error = { path = "../../util/error" }


This was crate was removed in #9054, do you need it?

Please rebase/merge to master

dvdplm

Code looks good to me modulo minor grumbles. Can't say I fully understand it all though.

dvdplm · 2018-07-09T11:28:03Z

ethcore/light/Cargo.toml

@@ -36,10 +36,13 @@ triehash = { path = "../../util/triehash" }
 kvdb = { path = "../../util/kvdb" }
 memory-cache = { path = "../../util/memory_cache" }
 error-chain = { version = "0.12", default-features = false }
+util-error = { path = "../../util/error" }


This crate was recently removed.

dvdplm · 2018-07-09T11:47:38Z

ethcore/light/src/client/header_chain.rs

@@ -359,7 +360,7 @@ impl HeaderChain {
 		header: Header,
 		transition_proof: Option<Vec<u8>>,
 	) -> Result<PendingChanges, BlockImportError> {
-		self.insert_inner(transaction, header, None, transition_proof)
+		self.insert_inner(transaction, header, None, transition_proof, false)
 	}

 	/// Insert a pre-verified header, with a known total difficulty. Similary to `insert`.


There's a typo here: s/Similary/Similar/ maybe?

dvdplm · 2018-07-09T12:09:40Z

ethcore/light/src/client/header_chain.rs

-					.expect("blocks are only inserted if parent is present; or this is the block we just added; qed");
+				let canon_pos = match entry.candidates.iter().position(|x| x.hash == canon_hash) {
+					Some(pos) => pos,
+					None => break,


Why not return an Err?

dvdplm · 2018-07-09T12:11:05Z

ethcore/light/src/client/header_chain.rs

-				if let Some((epoch_transition, header)) = last_canonical_transition {
-					let x = encode_canonical_transition(&header, &epoch_transition.proof);
-					transaction.put_vec(self.col, LAST_CANONICAL_TRANSITION, x);
+			if !dont_produce_cht {


I'm not a great fan of double-negatives like this. Would you consider flipping the boolean, so you'd have if produce_cht { … … }?

dvdplm · 2018-07-09T12:36:35Z

ethcore/light/src/client/header_chain.rs

 		&self,
 		transaction: &mut DBTransaction,
 		header: Header,
 		total_difficulty: Option<U256>,
 		transition_proof: Option<Vec<u8>>,
+		dont_produce_cht: bool,


As mentioned, flipping this bool would make it easier to read imo: product_cht.

dvdplm · 2018-07-09T13:06:35Z

ethcore/src/snapshot/consensus/mod.rs

+	fn commit(&self);
+}
+
+/// A factory producing `Rebuilder` necessary for snapshot restoration.


s/producing Rebuilder/producing a Rebuilder/

dvdplm · 2018-07-09T13:07:06Z

ethcore/src/snapshot/consensus/mod.rs

+
+/// A factory producing `Rebuilder` necessary for snapshot restoration.
+pub trait RebuilderFactory: Send {
+	/// Create a rebuilder, which will have chunks fed into it in arbitrary


s/rebuilder/Rebuilder/

dvdplm · 2018-07-09T13:25:08Z

ethcore/sync/src/light_sync/warp_sync.rs

+	}
+
+	/// Returns the largest group of peers with the same snapshot hash.
+	/// If we already  have manifest, prefer peers with that manifest.


Double spaces between "already" and "have"

dvdplm · 2018-07-09T13:33:06Z

ethcore/sync/src/light_sync/warp_sync.rs

+		let protocol_version: u8 = rlp.val_at(0)?;
+		let network_id: u64 = rlp.val_at(1)?;
+		let _difficulty: U256 = rlp.val_at(2)?;
+		let _latest_hash: H256 = rlp.val_at(3)?;


Why fetch these two if we don't need them? (and why bail on error?) Is it faster maybe?

To ensure that the status message that we receive from a full node is well formed.

dvdplm · 2018-07-09T13:36:17Z

parity/export_hardcoded_sync.rs

+		cmd.compaction,
+		cmd.wal,
+		VMType::Interpreter,
+		"".into(),


Perhaps annotate these hard-to-guess items to help future readers?

rphmeier · 2018-07-09T15:49:47Z

ethcore/light/src/client/mod.rs

+
+impl<'a> AncestryIter<'a> {
+	/// Get an iterator over a block and its ancestry.
+	pub fn iter(&'a self, start: BlockId) -> Box<Iterator<Item=encoded::Header> + 'a> {


could even be impl Iterator now

rphmeier · 2018-07-09T16:04:50Z

ethcore/light/src/client/service.rs

-struct ImportBlocks<T>(Arc<Client<T>>);
+impl<T> ClientIoHandler<T> {
+	pub fn new(client: Arc<Client<T>>, snapshot: Arc<SnapshotService>) -> Self {
+		Self {


favor ClientIoHandler

rphmeier · 2018-07-27T13:27:59Z

ethcore/light/src/client/snapshot/chain.rs

+
+	fn insert_unordered_block(
+		&self, batch: &mut DBTransaction, bytes: &[u8], _receipts: Vec<Receipt>,
+		parent_td: Option<U256>, _is_best: bool, _is_ancient: bool


rphmeier · 2018-07-27T13:30:59Z

ethcore/sync/src/chain/mod.rs


-const STATUS_PACKET: u8 = 0x00;
+pub const STATUS_PACKET: u8 = 0x00;


document all the public constants. Also, why have these become public?

Because currently, the warp sync handshake packet uses it: https://github.com/paritytech/parity-ethereum/blob/7ad556346eed03cfea2999f7f9daf864de6489af/ethcore/sync/src/chain/mod.rs#L890-L910

Actually, pub(crate) is enough for them.

rphmeier · 2018-07-27T13:31:19Z

ethcore/sync/src/light_sync/mod.rs

@@ -275,6 +300,13 @@ impl<L: AsLightClient + Send + Sync> Handler for LightSync<L> {
 	}

 	fn on_disconnect(&self, ctx: &EventContext, unfulfilled: &[ReqId]) {
+		match *self.state.lock() {
+			SyncState::Snapshot(_) => {
+				return;


extraneous braces

ethcore/sync/src/light_sync/mod.rs

* 'master' of https://github.com/paritytech/parity: removed client error (openethereum#9253) Implement EIP-1052 (EXTCODEHASH) and fix several issues in state account cache (openethereum#9234) Improve Tracer documentation (openethereum#9237) Update Dockerfile (openethereum#9242) block cleanup (openethereum#9117) Increase the number of sessions. (openethereum#9203) add changelog for 1.11.8 stable and 2.0.1 beta (openethereum#9230) fix typo (openethereum#9232) Fix potential as_usize overflow when casting from U256 in miner (openethereum#9221) Allow old blocks from peers with lower difficulty (openethereum#9226) Removes duplicate libudev-dev from Dockerfile (openethereum#9220) snap: remove ssl dependencies from snapcraft definition (openethereum#9222) remove ssl from dockerfiles, closes openethereum#8880 (openethereum#9195) Insert PROOF messages for some cases in blockchain (openethereum#9141) [Chain] Add more bootnodes (openethereum#9174) ethcore: update bn version (openethereum#9217) deserialize block only once during verification (openethereum#9161) Simple build instruction fix (openethereum#9215) Added --tx-queue-no-early-reject flag to disable early tx queue rejects (openethereum#9143)

dvdplm

lgtm

dvdplm · 2018-08-02T08:35:51Z

ethcore/light/src/client/header_chain.rs

@@ -451,15 +453,23 @@ impl HeaderChain {
 		// reorganize ancestors so canonical entries are first in their
 		// respective candidates vectors.
 		if is_new_best {
+			let prove = "blocks are only inserted if parent is present; or this is the block we just added; qed";


make const (and "proof" is a better name imo)

…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: Propagate transactions for next 4 blocks. (openethereum#9265) decode block rlp less often (openethereum#9252) Fix eternalities tests can_create (missing parameter) (openethereum#9270) Update ref to `parity-common` and update `seek` behaviour (openethereum#9257) Comply EIP-86 with the new definition (openethereum#9140) Check if synced when using eth_getWork (openethereum#9193) (openethereum#9210)

codecov-io · 2018-08-02T22:23:30Z

Codecov Report

Merging #8723 into master will increase coverage by 0.05%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #8723      +/-   ##
==========================================
+ Coverage   68.72%   68.77%   +0.05%     
==========================================
  Files         635      636       +1     
  Lines       90784    91288     +504     
==========================================
+ Hits        62394    62787     +393     
- Misses      28390    28501     +111

Impacted Files	Coverage Δ
parity/ethcore/vm/src/ext.rs	`50% <0%> (-30%)`	⬇️
parity/ethcore/src/account_provider/stores.rs	`88.51% <0%> (-7.15%)`	⬇️
parity/rpc/src/v1/metadata.rs	`44.44% <0%> (-5.56%)`	⬇️
parity/util/panic_hook/src/lib.rs	`18.75% <0%> (-4.33%)`	⬇️
parity/ethcore/wasm/src/env.rs	`81.69% <0%> (-4.03%)`	⬇️
parity/ethcore/sync/src/block_sync.rs	`67.55% <0%> (-2.62%)`	⬇️
parity/ethcore/src/block.rs	`88.69% <0%> (-2.14%)`	⬇️
parity/ethcore/src/blockchain/extras.rs	`84.76% <0%> (-2.11%)`	⬇️
parity/ethcore/light/src/net/context.rs	`20% <0%> (-1.67%)`	⬇️
parity/ethcore/src/verification/queue/kind.rs	`75.36% <0%> (-1.67%)`	⬇️
... and 97 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 1564fae...cfd6152. Read the comment docs.

…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: ethcore: add transition flag for transaction permission contract (openethereum#9275) Remove all dapp permissions related settings (openethereum#9120) Improve return data truncate logic (openethereum#9254) Update wasm-tests hash (openethereum#9295) Implement KIP4: create2 for wasm (openethereum#9277) Fix loop start value (openethereum#9285) Avoid using $HOME if not necessary (openethereum#9273) Fix path to parity.h (openethereum#9274)

…rp_sync_on_light_client * 'master' of https://github.com/paritytech/parity: Allow setting the panic hook with parity-clib (openethereum#9292) Prevent blockchain & miner racing when accessing pending block. (openethereum#9310) Docker alpine: use multi-stage concept (openethereum#9269) Update `log` -> 0.4, `env_logger` -> 0.5. (openethereum#9294) Update tobalaba.json (openethereum#9313) Allow tx pool to be Send (openethereum#9315) Fix codecov.io badge in README (openethereum#9327) Move ethereum-specific H256FastMap type to own crate (openethereum#9307) ethcore sync decodes rlp less often (openethereum#9264)

5chdn · 2018-08-24T19:45:57Z

Has some conflicts and needs a 2nd review.

5chdn · 2018-09-04T09:42:53Z

What's the status of this?

ordian · 2018-09-04T11:06:27Z

@5chdn the current implementation uses existing warp sync protocol (WARP_SYNC_PROTOCOL_ID), because it required less changes to implement. But because this protocol is tightly coupled with eth sync protocol, this approach has many drawbacks.
I started working on switching to a separate protocol, but it's not in a reviewable state yet, since I focused on higher priority light client bugs and also EIP#1218 may render it obsolete IIUC.

5chdn · 2018-09-08T22:47:36Z

Ok, let's just reopen this once we are continuing with this.

ordian added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. M4-core ⛓ Core client code / Rust. labels May 28, 2018

dvdplm reviewed May 28, 2018

View reviewed changes

ordian force-pushed the warp_sync_on_light_client branch 2 times, most recently from 86167a9 to 9afe71f Compare May 29, 2018 09:37

rphmeier reviewed May 29, 2018

View reviewed changes

5chdn added this to the 1.12 milestone May 31, 2018

ordian added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Jun 7, 2018

ordian commented Jun 27, 2018

View reviewed changes

ordian changed the title ~~[WIP] [1/2] Warp sync on light client~~ [part 1/2] Warp sync on light client Jul 2, 2018

5chdn requested review from dvdplm and rphmeier July 9, 2018 06:52

niklasad1 reviewed Jul 9, 2018

View reviewed changes

dvdplm approved these changes Jul 9, 2018

View reviewed changes

rphmeier reviewed Jul 9, 2018

View reviewed changes

5chdn added the A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. label Jul 10, 2018

5chdn modified the milestones: 2.0, 2.1 Jul 10, 2018

5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A1-onice 🌨 Pull request is reviewed well, but should not yet be merged. A0-pleasereview 🤓 Pull request needs code review. labels Jul 11, 2018

rphmeier reviewed Jul 27, 2018

View reviewed changes

ethcore/sync/src/light_sync/mod.rs Show resolved Hide resolved

review style issues addressed

52ab007

5chdn added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Jul 29, 2018

ordian added 2 commits July 31, 2018 14:07

make it compile

3d3991e

dvdplm approved these changes Aug 2, 2018

View reviewed changes

ordian added 2 commits August 2, 2018 17:36

refactor(warp_sync_light): const PROOF

062340b

ordian added 6 commits August 7, 2018 17:29

refactor(sync): reuse status packet creation

8424ad6

chore(docs): add a whitespace after semicolon

d77eaea

fix(warp_sync_light): insert_unordered_block !allow_producing_cht

dc78bd2

docs(warp_sync_light): fix a typo

f2e3c5d

5chdn requested a review from rphmeier August 24, 2018 19:46

5chdn added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 24, 2018

ordian added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Aug 27, 2018

5chdn added the A3-stale 🍃 Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Sep 4, 2018

5chdn closed this Sep 8, 2018


		const STATUS_PACKET: u8 = 0x00;
		pub const STATUS_PACKET: u8 = 0x00;

[part 1/2] Warp sync on light client #8723

[part 1/2] Warp sync on light client #8723

Conversation

ordian commented May 28, 2018

parity-cla-bot commented May 28, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

ordian left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

5chdn commented Jul 2, 2018

niklasad1 Jul 9, 2018 • edited Loading

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dvdplm left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

codecov-io commented Aug 2, 2018 • edited Loading

Codecov Report

5chdn commented Aug 24, 2018

5chdn commented Sep 4, 2018

ordian commented Sep 4, 2018

5chdn commented Sep 8, 2018

niklasad1 Jul 9, 2018 •

edited

Loading

codecov-io commented Aug 2, 2018 •

edited

Loading