Snapshot sync part 2 #2098

arkpar · 2016-09-14T17:11:23Z

In this PR:

Refactored blockchain sync logic into a block_sync module. Now two of these run in parallel - one for the head of the chain and another one to download missing blocks for the blockchain gap.
Added --warp CLI option
Snapshot sync starts if the chain is more than 100000 blocks behind

Run with --warp --bootnodes enode://cc92c4c40d612a10c877ca023ef0496c843fbc92b6c6c0d55ce0b863d51d821c4bd70daebb54324a6086374e6dc05708fed39862b275f169cb678e655da9d07d@136.243.154.246:30303 to test

still TODO:

Protocol documentation
Check PoW for blocks in the gap.
More tests
Download ancient blocks in reverse

coveralls · 2016-09-14T17:58:01Z

Coverage decreased (-0.5%) to 84.616% when pulling 555b3e0 on snapshot-sync into c8533a3 on master.

rphmeier

first pass mostly LGTM, just the concerns about importing invalid ancient blocks. i'll take a more in-depth look tomorrow.

I'm really excited to try this out! Once it's merged we ought to spin up a couple servers and do a field test :)

rphmeier · 2016-09-14T18:48:15Z

ethcore/src/blockchain/blockchain.rs

+		let result = self.insert_unordered_block(&mut batch, bytes, receipts, parent_td, is_best, false);
+		self.db.write(batch).unwrap();
+		result
+	}


this function ought to just be removed and its usages replaced with insert_unordered_block.

rphmeier · 2016-09-14T19:39:37Z

ethcore/src/client/client.rs

@@ -415,6 +415,26 @@ impl Client {
 		imported
 	}

+	fn import_old_block(&self, block_bytes: Bytes, receipts_bytes: Bytes) -> H256 {


i notice this doesn't do any block verification. are these blocks guaranteed to be part of a continuous chain to the first snapshot block at this point?

see comments below

rphmeier · 2016-09-14T19:40:52Z

ethcore/src/types/blockchain_info.rs

+	/// First block on the best sequence.
+	pub first_block_hash: Option<H256>,
+	/// Number of the first block on the best sequence.
+	pub first_block_number: Option<BlockNumber>,


first block is guaranteed to exist. what does None signify here?

I'd prefer None to indicate that there's no gap in the blockchain. Blockchain always sets the value though.

that seems fair, I see that's implemented now

rphmeier · 2016-09-14T19:57:17Z

sync/src/block_sync.rs

+			}
+
+			let result = if let Some(receipts) = receipts {
+				io.chain().import_block_with_receipts(block, receipts)


is it safe to import directly here? we have no idea if the transactions contained within are valid

That's not much of an attack vector. These historical blocks are only used to be served to other peers. The attacker can't affect node's state with providing invalid data here.

while it can't affect the state, it's probably best not to ever propagate potentially invalid blocks. other peers might detect that and label the offending node as bad

rphmeier · 2016-09-14T20:06:11Z

sync/src/chain.rs

+		if let (Some(ancient_block_hash), Some(ancient_block_number)) = (chain.ancient_block_hash, chain.ancient_block_number) {
+
+			trace!(target: "sync", "Downloading old blocks from {:?} (#{}) till {:?} (#{:?})", ancient_block_hash, ancient_block_number, chain.first_block_hash, chain.first_block_number);
+			let mut downloader = BlockDownloader::new(true, &ancient_block_hash, ancient_block_number);


The BlockDownloader here has no way of knowing whether the blocks it's getting from ancient_block_hash onwards are actually going to lead up to the first block. Although these blocks never get enacted, it's probably best not to import any potentially invalid blocks.

For correctness, I would suggest having the block downloader proceed in two stages:

Find a sparse hash-chain from the first block to the ancient block, built by walking in reverse (by 256? 512? 1024?) from the first block until the ancient block is reached.

Walk this hash chain forward, fetching all necessary headers and ensuring that they have the right hash before importing.

I agree that it is better to download the ancient chain in reverse, but will leave it for a future PR

Ok, another PR sounds good. It should be a blocker for the 1.4 release, IMO.

Note that if the blocks are being downloaded in reverse, no additional verification is needed.

coveralls · 2016-09-14T20:21:12Z

Coverage decreased (-0.5%) to 84.606% when pulling 86aeab3 on snapshot-sync into c8533a3 on master.

coveralls · 2016-09-14T21:34:07Z

Coverage decreased (-0.5%) to 84.577% when pulling f326b59 on snapshot-sync into c8533a3 on master.

coveralls · 2016-09-14T22:24:51Z

Coverage decreased (-0.5%) to 84.614% when pulling e3d6ee3 on snapshot-sync into c8533a3 on master.

coveralls · 2016-09-15T09:09:41Z

Coverage decreased (-0.5%) to 84.613% when pulling 10fea19 on snapshot-sync into c8533a3 on master.

coveralls · 2016-09-15T10:47:41Z

Coverage decreased (-0.5%) to 84.616% when pulling 398fcc3 on snapshot-sync into c8533a3 on master.

rphmeier · 2016-09-16T09:14:57Z

the ancient block downloading is quite slow on my machine -- it keeps losing all its peers. it will need investigation.

coveralls · 2016-09-19T06:10:02Z

Coverage decreased (-0.5%) to 84.776% when pulling 5d67bbf on snapshot-sync into 7f7e748 on master.

arkpar · 2016-09-20T09:22:54Z

ethcore/res/ethereum/frontier.json

@@ -160,7 +160,8 @@
 		"enode://a979fb575495b8d6db44f750317d0f4622bf4c2aa3365d6af7c284339968eef29b69ad0dce72a4d8db5ebb4968de0e3bec910127f134779fbcb0cb6d3331163c@52.16.188.185:30303",
 		"enode://de471bccee3d042261d52e9bff31458daecc406142b401d4cd848f677479f73104b9fdeb090af9583d3391b7f10cb2ba9e26865dd5fca4fcdc0fb1e3b723c786@54.94.239.50:30303",
 		"enode://1118980bf48b0a3640bdba04e0fe78b1add18e1cd99bf22d53daac1fd9972ad650df52176e7c7d89d1114cfef2bc23a2959aa54998a46afcf7d91809f0855082@52.74.57.123:30303",
-		"enode://248f12bc8b18d5289358085520ac78cd8076485211e6d96ab0bc93d6cd25442db0ce3a937dc404f64f207b0b9aed50e25e98ce32af5ac7cb321ff285b97de485@zero.parity.io:30303"
+		"enode://4cd540b2c3292e17cff39922e864094bf8b0741fcc8c5dcea14957e389d7944c70278d872902e3d0345927f621547efa659013c400865485ab4bfa0c6596936f@zero.parity.io:30303",
+		"enode://cc92c4c40d612a10c877ca023ef0496c843fbc92b6c6c0d55ce0b863d51d821c4bd70daebb54324a6086374e6dc05708fed39862b275f169cb678e655da9d07d@136.243.154.246:30303"


aka antares.ethcore.io

arkpar · 2016-09-20T09:23:37Z

ethcore/src/blockchain/blockchain.rs

@@ -1192,6 +1241,24 @@ impl BlockChain {
 		body.append_raw(block_rlp.at(2).as_raw(), 1);
 		body.out()
 	}
+
+	/// Returns general blockchain information
+	pub fn chain_info(&self) -> BlockChainInfo {


This is moved to blockchain.rs to make it atomic

coveralls · 2016-09-20T09:57:39Z

Coverage decreased (-0.5%) to 84.74% when pulling 9aece51 on snapshot-sync into 48be609 on master.

coveralls · 2016-09-20T09:59:47Z

Coverage decreased (-0.5%) to 84.776% when pulling 9aece51 on snapshot-sync into 48be609 on master.

rphmeier · 2016-09-20T10:15:08Z

sync/src/chain.rs

-				BlockStatus::Unknown => {
-					headers.push(try!(r.at(i)).as_raw().to_vec());
-					hashes.push(hash);
+					self.old_blocks.as_mut().expect("Checked with condition above; qed")


why not just match on self.old_blocks.as_mut()?

rphmeier · 2016-09-20T10:16:27Z

sync/src/chain.rs

+							self.continue_sync(io);
+							return Ok(());
+						}
+						self.old_blocks.as_mut().expect("Checked with condition above; qed")


could also be rewritten with match

rphmeier · 2016-09-20T10:16:58Z

sync/src/chain.rs

+							self.continue_sync(io);
+							return Ok(());
+						}
+						self.old_blocks.as_mut().expect("Checked with condition above; qed")


here as well

rphmeier · 2016-09-20T10:47:36Z

looks mostly good to me, just a couple minor grumbles.

coveralls · 2016-09-20T16:59:10Z

Coverage decreased (-0.5%) to 84.776% when pulling d9e232b on snapshot-sync into 9b6d30f on master.

gavofyork · 2016-10-08T15:31:30Z

haven't done a thorough review of the logic in the sync module. code in ethcore module could use some additional documentation.

i think the full sync module would benefit from some high-level documentation to comprehend what all the moving parts are. can be in another PR though.

arkpar · 2016-10-10T17:10:47Z

The core syncing logic was not changed and is documented in sync/chain.rs. It was just refactored into a separate module and an additional background sync was added, using the same algorithm as the main sync.

coveralls · 2016-10-10T17:38:25Z

Coverage decreased (-0.6%) to 85.782% when pulling 9bf6ae4 on snapshot-sync into 64f6f83 on master.

coveralls · 2016-10-11T08:40:09Z

Changes Unknown when pulling 544d20e on snapshot-sync into * on master*.

coveralls · 2016-10-14T09:56:07Z

Coverage decreased (-0.6%) to 85.951% when pulling aee487d on snapshot-sync into 4581469 on master.

[ci:skip]

arkpar added the A0-pleasereview 🤓 Pull request needs code review. label Sep 14, 2016

rphmeier reviewed Sep 14, 2016

View reviewed changes

rphmeier added the M4-core ⛓ Core client code / Rust. label Sep 15, 2016

arkpar force-pushed the snapshot-sync branch from 398fcc3 to 5d67bbf Compare September 19, 2016 05:27

arkpar force-pushed the snapshot-sync branch from 38eae61 to 9aece51 Compare September 20, 2016 09:15

arkpar commented Sep 20, 2016

View reviewed changes

rphmeier reviewed Sep 20, 2016

View reviewed changes

rphmeier added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Sep 20, 2016

arkpar mentioned this pull request Sep 20, 2016

Fix syncing with pv63 peers #2204

Merged

arkpar force-pushed the snapshot-sync branch from 9aece51 to d9e232b Compare September 20, 2016 16:13

rphmeier mentioned this pull request Sep 28, 2016

peers problem #2314

Closed

arkpar force-pushed the snapshot-sync branch from d9e232b to 41018fe Compare October 4, 2016 22:15

arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Oct 4, 2016

arkpar force-pushed the snapshot-sync branch from 41018fe to d1dadb7 Compare October 8, 2016 11:25

gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 8, 2016

arkpar force-pushed the snapshot-sync branch from d1dadb7 to 9bf6ae4 Compare October 10, 2016 17:08

arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Oct 10, 2016

arkpar force-pushed the snapshot-sync branch from 9bf6ae4 to 544d20e Compare October 11, 2016 08:04

arkpar added 11 commits October 14, 2016 10:48

Split block downloader into a module

d17cb9c

Snapshot sync progress

5d43604

Warp sync CLI option

d5390f2

Increased snapshot chunk and ping timeouts

2c7cec9

Fixed an issue with delayed writes

0726f7d

Updated bootnodes

48f33f8

Don't run pending IO tasks on shutdown

f1d973a

Optional first_block; removed insert_snapshot_block

c01538b

Fixed stalled sync

8869887

Fixing expect calls

2dc1ee5

style and docs

aee487d

arkpar force-pushed the snapshot-sync branch from 544d20e to aee487d Compare October 14, 2016 09:12

gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Oct 18, 2016

Update block_sync.rs

078c9c6

[ci:skip]

arkpar merged commit 487dfb0 into master Oct 18, 2016

tomusdrw deleted the snapshot-sync branch October 24, 2016 09:52

tomusdrw restored the snapshot-sync branch October 24, 2016 09:52

tomusdrw deleted the snapshot-sync branch November 2, 2016 09:06

Snapshot sync part 2 #2098

Snapshot sync part 2 #2098

Conversation

arkpar commented Sep 14, 2016 • edited Loading

coveralls commented Sep 14, 2016

rphmeier 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

arkpar Sep 15, 2016 • edited Loading

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

coveralls commented Sep 14, 2016

coveralls commented Sep 14, 2016

coveralls commented Sep 14, 2016

coveralls commented Sep 15, 2016

coveralls commented Sep 15, 2016

rphmeier commented Sep 16, 2016

coveralls commented Sep 19, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

coveralls commented Sep 20, 2016

coveralls commented Sep 20, 2016

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

rphmeier commented Sep 20, 2016

coveralls commented Sep 20, 2016

gavofyork commented Oct 8, 2016

arkpar commented Oct 10, 2016

coveralls commented Oct 10, 2016

coveralls commented Oct 11, 2016

coveralls commented Oct 14, 2016

arkpar commented Sep 14, 2016 •

edited

Loading

arkpar Sep 15, 2016 •

edited

Loading