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

ethcore sync decodes rlp less often #9264

Merged
merged 9 commits into from
Aug 8, 2018
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions ethcore/src/verification/queue/kind.rs
Original file line number Diff line number Diff line change
Expand Up @@ -113,6 +113,7 @@ pub mod blocks {
}

/// An unverified block.
#[derive(PartialEq, Debug)]
pub struct Unverified {
/// Unverified block header.
pub header: Header,
Expand Down
54 changes: 17 additions & 37 deletions ethcore/sync/src/block_sync.rs
Original file line number Diff line number Diff line change
Expand Up @@ -23,12 +23,11 @@ use std::cmp;
use heapsize::HeapSizeOf;
use ethereum_types::H256;
use rlp::{self, Rlp};
use ethcore::header::{BlockNumber, Header as BlockHeader};
use ethcore::header::BlockNumber;
use ethcore::client::{BlockStatus, BlockId, BlockImportError, BlockImportErrorKind};
use ethcore::error::{ImportErrorKind, BlockError};
use ethcore::verification::queue::kind::blocks::Unverified;
use sync_io::SyncIo;
use blocks::BlockCollection;
use blocks::{BlockCollection, SyncBody, SyncHeader};

const MAX_HEADERS_TO_REQUEST: usize = 128;
const MAX_BODIES_TO_REQUEST: usize = 32;
Expand Down Expand Up @@ -236,45 +235,39 @@ impl BlockDownloader {
let mut valid_response = item_count == 0; //empty response is valid
let mut any_known = false;
for i in 0..item_count {
let info: BlockHeader = r.val_at(i).map_err(|e| {
trace!(target: "sync", "Error decoding block header RLP: {:?}", e);
BlockDownloaderImportError::Invalid
})?;
let number = BlockNumber::from(info.number());
let info = SyncHeader::from_rlp(r.at(i)?.as_raw().to_vec())?;
Copy link
Contributor

Choose a reason for hiding this comment

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

Do you think it's important to keep the logging message? Or will this be logged elsewhere?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it's already logged upstream :)

let number = BlockNumber::from(info.header.number());
let hash = info.header.hash();
// Check if any of the headers matches the hash we requested
if !valid_response {
if let Some(expected) = expected_hash {
valid_response = expected == info.hash()
valid_response = expected == hash;
}
}
any_known = any_known || self.blocks.contains_head(&info.hash());
if self.blocks.contains(&info.hash()) {
trace!(target: "sync", "Skipping existing block header {} ({:?})", number, info.hash());
any_known = any_known || self.blocks.contains_head(&hash);
if self.blocks.contains(&hash) {
trace!(target: "sync", "Skipping existing block header {} ({:?})", number, hash);
continue;
}

if self.highest_block.as_ref().map_or(true, |n| number > *n) {
self.highest_block = Some(number);
}
let hash = info.hash();
let hdr = r.at(i).map_err(|e| {
Copy link
Contributor

Choose a reason for hiding this comment

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

We were decoding this twice in the same function just because... 😱

trace!(target: "sync", "Error decoding block header RLP: {:?}", e);
BlockDownloaderImportError::Invalid
})?;

match io.chain().block_status(BlockId::Hash(hash.clone())) {
BlockStatus::InChain | BlockStatus::Queued => {
match self.state {
State::Blocks => trace!(target: "sync", "Header already in chain {} ({})", number, hash),
_ => trace!(target: "sync", "Header already in chain {} ({}), state = {:?}", number, hash, self.state),
}
headers.push(hdr.as_raw().to_vec());
headers.push(info);
hashes.push(hash);
},
BlockStatus::Bad => {
return Err(BlockDownloaderImportError::Invalid);
},
BlockStatus::Unknown | BlockStatus::Pending => {
headers.push(hdr.as_raw().to_vec());
headers.push(info);
hashes.push(hash);
}
}
Expand Down Expand Up @@ -325,19 +318,15 @@ impl BlockDownloader {
let item_count = r.item_count().unwrap_or(0);
if item_count == 0 {
return Err(BlockDownloaderImportError::Useless);
}
else if self.state != State::Blocks {
} else if self.state != State::Blocks {
trace!(target: "sync", "Ignored unexpected block bodies");
}
else {
} else {
let mut bodies = Vec::with_capacity(item_count);
for i in 0..item_count {
let body = r.at(i).map_err(|e| {
trace!(target: "sync", "Error decoding block boides RLP: {:?}", e);
BlockDownloaderImportError::Invalid
})?;
bodies.push(body.as_raw().to_vec());
let body = SyncBody::from_rlp(r.at(i)?.as_raw())?;
bodies.push(body);
}

if self.blocks.insert_bodies(bodies) != item_count {
trace!(target: "sync", "Deactivating peer for giving invalid block bodies");
return Err(BlockDownloaderImportError::Invalid);
Expand Down Expand Up @@ -483,15 +472,6 @@ impl BlockDownloader {
let block = block_and_receipts.block;
let receipts = block_and_receipts.receipts;

let block = match Unverified::from_rlp(block) {
Ok(block) => block,
Err(_) => {
debug!(target: "sync", "Bad block rlp");
bad = true;
break;
}
};

let h = block.header.hash();
let number = block.header.number();
let parent = *block.header.parent_hash();
Expand Down
Loading