From 7b27e159dec0fb4803a00ddc9ec3bb66a1357734 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Sun, 6 Jan 2019 21:37:54 +0100 Subject: [PATCH 1/8] Make sure parent block is not in importing queue when importing ancient blocks --- ethcore/src/client/client.rs | 8 ++++++++ ethcore/src/verification/queue/mod.rs | 1 + 2 files changed, 9 insertions(+) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 1b49d009225..78ccfda6dfd 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -77,6 +77,7 @@ use types::filter::Filter; use types::ancestry_action::AncestryAction; use verification; use verification::{PreverifiedBlock, Verifier, BlockQueue}; +use verification::queue::Status as QueueStatus; use verification::queue::kind::blocks::Unverified; use verification::queue::kind::BlockLike; @@ -2193,6 +2194,13 @@ impl IoClient for Client { let first = queued.write().1.pop_front(); if let Some((unverified, receipts_bytes)) = first { let hash = unverified.hash(); + let parent_hash = unverified.parent_hash(); + let status = client.importer.block_queue.status(&parent_hash); + + if status == QueueStatus::Queued { + break; + } + let result = client.importer.import_old_block( unverified, &receipts_bytes, diff --git a/ethcore/src/verification/queue/mod.rs b/ethcore/src/verification/queue/mod.rs index b1ab7a13008..213ab77cffe 100644 --- a/ethcore/src/verification/queue/mod.rs +++ b/ethcore/src/verification/queue/mod.rs @@ -108,6 +108,7 @@ impl HeapSizeOf for Verifying { } /// Status of items in the queue. +#[derive(PartialEq, Eq)] pub enum Status { /// Currently queued. Queued, From 9f4bd11ee5494c60349176e5e18f6e8d7cfa6702 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Sun, 6 Jan 2019 23:41:36 +0100 Subject: [PATCH 2/8] Clear queue when an ancient import fails --- ethcore/src/client/client.rs | 2 ++ 1 file changed, 2 insertions(+) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 936769678a5..d2f3e068077 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -2207,6 +2207,8 @@ impl IoClient for Client { ); if let Err(e) = result { error!(target: "client", "Error importing ancient block: {}", e); + queued.write().1.clear(); + queued.write().0.clear(); } // remove from pending queued.write().0.remove(&hash); From e3120735b4dbd9ea879693f209d498002b2eea9a Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Sun, 6 Jan 2019 23:51:42 +0100 Subject: [PATCH 3/8] Lock only once in clear --- ethcore/src/client/client.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index d2f3e068077..952c66bd4ec 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -2207,8 +2207,10 @@ impl IoClient for Client { ); if let Err(e) = result { error!(target: "client", "Error importing ancient block: {}", e); - queued.write().1.clear(); - queued.write().0.clear(); + + let mut queued = queued.write(); + queued.0.clear(); + queued.1.clear(); } // remove from pending queued.write().0.remove(&hash); From 18fdcb429e514dc64f1ce0b16399768432a8b235 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 7 Jan 2019 00:21:33 +0100 Subject: [PATCH 4/8] Add comments why queued check is needed --- ethcore/src/client/client.rs | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 952c66bd4ec..c771eeb6df4 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -2192,9 +2192,15 @@ impl IoClient for Client { let first = queued.write().1.pop_front(); if let Some((unverified, receipts_bytes)) = first { let hash = unverified.hash(); + + // In the situation where we try to import a normal block A, and right + // after it the network situation vastly changed (for example, receiving + // a far-ahead best block from another peer) and we try to import an + // ancient block B that is a child of A, if A is still queued and + // haven't finished importing, we return early here and retry later. + // Otherwise `import_old_block` will panic. let parent_hash = unverified.parent_hash(); let status = client.importer.block_queue.status(&parent_hash); - if status == QueueStatus::Queued { break; } From d9f1c7d186de8bf92c3949c931ae62b2b3e39e22 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 7 Jan 2019 09:57:51 +0100 Subject: [PATCH 5/8] Should push the value back to the queue --- ethcore/src/client/client.rs | 1 + 1 file changed, 1 insertion(+) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index c771eeb6df4..342f50ac2bc 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -2202,6 +2202,7 @@ impl IoClient for Client { let parent_hash = unverified.parent_hash(); let status = client.importer.block_queue.status(&parent_hash); if status == QueueStatus::Queued { + queued.write().1.push_front((unverified, receipts_bytes)); break; } From 44ad6e1e3e7141c6029588148f8b3b4d540756a4 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 7 Jan 2019 10:02:27 +0100 Subject: [PATCH 6/8] Directly check in chain.read() --- ethcore/src/client/client.rs | 21 ++------------------- 1 file changed, 2 insertions(+), 19 deletions(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 342f50ac2bc..6ad48ce96a8 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -73,7 +73,6 @@ use state::{self, State}; use state_db::StateDB; use trace::{self, TraceDB, ImportRequest as TraceImportRequest, LocalizedTrace, Database as TraceDatabase}; use transaction_ext::Transaction; -use verification::queue::Status as QueueStatus; use verification::queue::kind::BlockLike; use verification::queue::kind::blocks::Unverified; use verification::{PreverifiedBlock, Verifier, BlockQueue}; @@ -2166,11 +2165,8 @@ impl IoClient for Client { // NOTE To prevent race condition with import, make sure to check queued blocks first // (and attempt to acquire lock) let is_parent_pending = self.queued_ancient_blocks.read().0.contains(&parent_hash); - if !is_parent_pending { - let status = self.block_status(BlockId::Hash(parent_hash)); - if status == BlockStatus::Unknown { - bail!(EthcoreErrorKind::Block(BlockError::UnknownParent(parent_hash))); - } + if !is_parent_pending && !self.chain.read().is_known(&parent_hash) { + bail!(EthcoreErrorKind::Block(BlockError::UnknownParent(parent_hash))); } } @@ -2193,19 +2189,6 @@ impl IoClient for Client { if let Some((unverified, receipts_bytes)) = first { let hash = unverified.hash(); - // In the situation where we try to import a normal block A, and right - // after it the network situation vastly changed (for example, receiving - // a far-ahead best block from another peer) and we try to import an - // ancient block B that is a child of A, if A is still queued and - // haven't finished importing, we return early here and retry later. - // Otherwise `import_old_block` will panic. - let parent_hash = unverified.parent_hash(); - let status = client.importer.block_queue.status(&parent_hash); - if status == QueueStatus::Queued { - queued.write().1.push_front((unverified, receipts_bytes)); - break; - } - let result = client.importer.import_old_block( unverified, &receipts_bytes, From 76cecb66406d9a05ddb2a56603fcc75e160ef232 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 7 Jan 2019 10:04:17 +0100 Subject: [PATCH 7/8] Remove extra empty line --- ethcore/src/client/client.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/ethcore/src/client/client.rs b/ethcore/src/client/client.rs index 6ad48ce96a8..b152ed16cdd 100644 --- a/ethcore/src/client/client.rs +++ b/ethcore/src/client/client.rs @@ -2188,7 +2188,6 @@ impl IoClient for Client { let first = queued.write().1.pop_front(); if let Some((unverified, receipts_bytes)) = first { let hash = unverified.hash(); - let result = client.importer.import_old_block( unverified, &receipts_bytes, From 28fa60fa32c87e9d076b1ac5432e615d1526c220 Mon Sep 17 00:00:00 2001 From: Wei Tang Date: Mon, 7 Jan 2019 10:04:56 +0100 Subject: [PATCH 8/8] Revert unused verification change --- ethcore/src/verification/queue/mod.rs | 1 - 1 file changed, 1 deletion(-) diff --git a/ethcore/src/verification/queue/mod.rs b/ethcore/src/verification/queue/mod.rs index 7d131535f3b..9541ecc1ad1 100644 --- a/ethcore/src/verification/queue/mod.rs +++ b/ethcore/src/verification/queue/mod.rs @@ -108,7 +108,6 @@ impl HeapSizeOf for Verifying { } /// Status of items in the queue. -#[derive(PartialEq, Eq)] pub enum Status { /// Currently queued. Queued,