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

decode block rlp less often #9252

Merged
merged 7 commits into from
Aug 2, 2018
Merged

decode block rlp less often #9252

Show file tree
Hide file tree
Changes from 4 commits
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
eb78076
deserialize block only once during verification
debris Jul 18, 2018
91edc42
ethcore-sync uses Unverified
debris Jul 18, 2018
0efdf0f
Merge branch 'master' into use_unverified
debris Jul 30, 2018
8290fb7
ethcore-sync uses Unverified
debris Jul 30, 2018
35fcb81
fixed build error
debris Jul 30, 2018
a19faf0
removed Block::is_good
debris Jul 30, 2018
32f7b38
applied review suggestions
debris Jul 31, 2018
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
44 changes: 9 additions & 35 deletions ethcore/src/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ use engines::EthEngine;
use error::{Error, BlockError};
use ethereum_types::{H256, U256, Address, Bloom};
use factory::Factories;
use hash::{keccak, KECCAK_NULL_RLP, KECCAK_EMPTY_LIST_RLP};
use hash::keccak;
use header::{Header, ExtendedHeader};
use receipt::{Receipt, TransactionOutcome};
use rlp::{Rlp, RlpStream, Encodable, Decodable, DecoderError, encode_list};
Expand All @@ -51,7 +51,6 @@ use transaction::{UnverifiedTransaction, SignedTransaction, Error as Transaction
use triehash::ordered_trie_root;
use unexpected::{Mismatch, OutOfBounds};
use verification::PreverifiedBlock;
use views::BlockView;
use vm::{EnvInfo, LastHashes};

/// A block, encoded as it is on the block chain.
Expand Down Expand Up @@ -424,31 +423,9 @@ impl<'x> OpenBlock<'x> {

/// Turn this into a `LockedBlock`.
pub fn close_and_lock(self) -> Result<LockedBlock, Error> {
let mut s = self;

s.engine.on_close_block(&mut s.block)?;
s.block.state.commit()?;

if s.block.header.transactions_root().is_zero() || s.block.header.transactions_root() == &KECCAK_NULL_RLP {
s.block.header.set_transactions_root(ordered_trie_root(s.block.transactions.iter().map(|e| e.rlp_bytes())));
}
if s.block.header.uncles_hash().is_zero() || s.block.header.uncles_hash() == &KECCAK_EMPTY_LIST_RLP {
let uncle_bytes = encode_list(&s.block.uncles);
s.block.header.set_uncles_hash(keccak(&uncle_bytes));
}
if s.block.header.receipts_root().is_zero() || s.block.header.receipts_root() == &KECCAK_NULL_RLP {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

as mentioned before, this checks could lead to invalid block being locked, so I removed them

s.block.header.set_receipts_root(ordered_trie_root(s.block.receipts.iter().map(|r| r.rlp_bytes())));
}

s.block.header.set_state_root(s.block.state.root().clone());
s.block.header.set_log_bloom(s.block.receipts.iter().fold(Bloom::zero(), |mut b, r| {
b.accrue_bloom(&r.log_bloom);
b
}));
s.block.header.set_gas_used(s.block.receipts.last().map_or_else(U256::zero, |r| r.gas_used));

let closed = self.close()?;
Copy link
Collaborator

Choose a reason for hiding this comment

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

Just want to note that this will have one additional clone of unclosed_state (https://github.com/paritytech/parity-ethereum/blob/a19faf01f347e7f485a1e1a82c47e7715e132dba/ethcore/src/block.rs#L397), which is immediately dropped. Not sure whether it's optimized away by compiler.

Copy link
Collaborator

Choose a reason for hiding this comment

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

Or maybe we can do it the other way around:

pub fn close(self) -> Result<ClosedBlock, Error> {
  let unclosed_state = self.block.state.clone();
  let locked = self.close_and_lock()?;
  
  Ok(ClosedBlock {
    block: locked.block,
    unclosed_state,
  })
}

Ok(LockedBlock {
block: s.block,
block: closed.block,
})
}

Expand Down Expand Up @@ -537,18 +514,16 @@ impl LockedBlock {
self,
engine: &EthEngine,
seal: Vec<Bytes>,
) -> Result<SealedBlock, (Error, LockedBlock)> {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

LockedBlock returned in Err was never used

) -> Result<SealedBlock, Error> {
let mut s = self;
s.block.header.set_seal(seal);
s.block.header.compute_hash();

// TODO: passing state context to avoid engines owning it?
match engine.verify_local_seal(&s.block.header) {
Err(e) => Err((e, s)),
_ => Ok(SealedBlock {
block: s.block
}),
}
engine.verify_local_seal(&s.block.header)?;
Ok(SealedBlock {
block: s.block
})
}
}

Expand Down Expand Up @@ -637,12 +612,11 @@ pub fn enact_verified(
is_epoch_begin: bool,
ancestry: &mut Iterator<Item=ExtendedHeader>,
) -> Result<LockedBlock, Error> {
let view = view!(BlockView, &block.bytes);

enact(
block.header,
block.transactions,
view.uncles(),
block.uncles,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed redundant decoding of uncles

engine,
tracing,
db,
Expand Down
55 changes: 24 additions & 31 deletions ethcore/src/client/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -74,6 +74,8 @@ use types::filter::Filter;
use types::ancestry_action::AncestryAction;
use verification;
use verification::{PreverifiedBlock, Verifier, BlockQueue};
use verification::queue::kind::blocks::Unverified;
use verification::queue::kind::BlockLike;

// re-export
pub use types::blockchain_info::BlockChainInfo;
Expand Down Expand Up @@ -209,7 +211,7 @@ pub struct Client {
/// Queued ancient blocks, make sure they are imported in order.
queued_ancient_blocks: Arc<RwLock<(
HashSet<H256>,
VecDeque<(Header, encoded::Block, Bytes)>
VecDeque<(Unverified, Bytes)>
)>>,
ancient_blocks_import_lock: Arc<Mutex<()>>,
/// Consensus messages import queue
Expand Down Expand Up @@ -429,19 +431,19 @@ impl Importer {
///
/// The block is guaranteed to be the next best blocks in the
/// first block sequence. Does no sealing or transaction validation.
fn import_old_block(&self, header: &Header, block: encoded::Block, receipts_bytes: &[u8], db: &KeyValueDB, chain: &BlockChain) -> Result<(), ::error::Error> {
fn import_old_block(&self, unverified: Unverified, receipts_bytes: &[u8], db: &KeyValueDB, chain: &BlockChain) -> Result<(), ::error::Error> {
let receipts = ::rlp::decode_list(receipts_bytes);
let _import_lock = self.import_lock.lock();

{
trace_time!("import_old_block");
// verify the block, passing the chain for updating the epoch verifier.
let mut rng = OsRng::new()?;
self.ancient_verifier.verify(&mut rng, &header, &chain)?;
self.ancient_verifier.verify(&mut rng, &unverified.header, &chain)?;

// Commit results
let mut batch = DBTransaction::new();
chain.insert_unordered_block(&mut batch, block, receipts, None, false, true);
chain.insert_unordered_block(&mut batch, encoded::Block::new(unverified.bytes), receipts, None, false, true);
Copy link
Collaborator

Choose a reason for hiding this comment

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

So where does the verification actually happen?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

4 lines above, we verify only header for old blocks. No logic has been changed here

// Final commit to the DB
db.write_buffered(batch);
chain.commit();
Expand Down Expand Up @@ -1382,22 +1384,15 @@ impl CallContract for Client {
}

impl ImportBlock for Client {
fn import_block(&self, bytes: Bytes) -> Result<H256, BlockImportError> {
use verification::queue::kind::BlockLike;
use verification::queue::kind::blocks::Unverified;

// create unverified block here so the `keccak` calculation can be cached.
let unverified = Unverified::from_rlp(bytes)?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed redundant block deserialization


{
if self.chain.read().is_known(&unverified.hash()) {
bail!(BlockImportErrorKind::Import(ImportErrorKind::AlreadyInChain));
}
let status = self.block_status(BlockId::Hash(unverified.parent_hash()));
if status == BlockStatus::Unknown || status == BlockStatus::Pending {
bail!(BlockImportErrorKind::Block(BlockError::UnknownParent(unverified.parent_hash())));
}
fn import_block(&self, unverified: Unverified) -> Result<H256, BlockImportError> {
if self.chain.read().is_known(&unverified.hash()) {
bail!(BlockImportErrorKind::Import(ImportErrorKind::AlreadyInChain));
}
let status = self.block_status(BlockId::Hash(unverified.parent_hash()));
if status == BlockStatus::Unknown || status == BlockStatus::Pending {
bail!(BlockImportErrorKind::Block(BlockError::UnknownParent(unverified.parent_hash())));
}

Ok(self.importer.block_queue.import(unverified)?)
}
}
Expand Down Expand Up @@ -2022,24 +2017,23 @@ impl IoClient for Client {
});
}

fn queue_ancient_block(&self, block_bytes: Bytes, receipts_bytes: Bytes) -> Result<H256, BlockImportError> {
fn queue_ancient_block(&self, unverified: Unverified, receipts_bytes: Bytes) -> Result<H256, BlockImportError> {
trace_time!("queue_ancient_block");
let header: Header = ::rlp::Rlp::new(&block_bytes).val_at(0)?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

removed redundant header deserialization

let hash = header.hash();

let hash = unverified.hash();
{
// check block order
if self.chain.read().is_known(&hash) {
bail!(BlockImportErrorKind::Import(ImportErrorKind::AlreadyInChain));
}
let parent_hash = header.parent_hash();
let parent_hash = unverified.parent_hash();
// 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);
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));
let status = self.block_status(BlockId::Hash(parent_hash));
if status == BlockStatus::Unknown || status == BlockStatus::Pending {
bail!(BlockImportErrorKind::Block(BlockError::UnknownParent(*parent_hash)));
bail!(BlockImportErrorKind::Block(BlockError::UnknownParent(parent_hash)));
}
}
}
Expand All @@ -2048,7 +2042,7 @@ impl IoClient for Client {
{
let mut queued = self.queued_ancient_blocks.write();
queued.0.insert(hash);
queued.1.push_back((header, encoded::Block::new(block_bytes), receipts_bytes));
queued.1.push_back((unverified, receipts_bytes));
}

let queued = self.queued_ancient_blocks.clone();
Expand All @@ -2060,11 +2054,10 @@ impl IoClient for Client {
let _lock = lock.lock();
for _i in 0..MAX_ANCIENT_BLOCKS_TO_IMPORT {
let first = queued.write().1.pop_front();
if let Some((header, block_bytes, receipts_bytes)) = first {
let hash = header.hash();
if let Some((unverified, receipts_bytes)) = first {
let hash = unverified.hash();
let result = client.importer.import_old_block(
&header,
block_bytes,
unverified,
&receipts_bytes,
&**client.db.read().key_value(),
&*client.chain.read(),
Expand Down
16 changes: 9 additions & 7 deletions ethcore/src/client/test_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -53,6 +53,7 @@ use spec::Spec;
use types::basic_account::BasicAccount;
use types::pruning_info::PruningInfo;
use verification::queue::QueueInfo;
use verification::queue::kind::blocks::Unverified;
use block::{OpenBlock, SealedBlock, ClosedBlock};
use executive::Executed;
use error::CallError;
Expand Down Expand Up @@ -280,7 +281,8 @@ impl TestBlockChainClient {
rlp.append(&header);
rlp.append_raw(&txs, 1);
rlp.append_raw(uncles.as_raw(), 1);
self.import_block(rlp.as_raw().to_vec()).unwrap();
let unverified = Unverified::from_rlp(rlp.out()).unwrap();
self.import_block(unverified).unwrap();
}
}

Expand Down Expand Up @@ -512,8 +514,8 @@ impl RegistryInfo for TestBlockChainClient {
}

impl ImportBlock for TestBlockChainClient {
fn import_block(&self, b: Bytes) -> Result<H256, BlockImportError> {
let header = view!(BlockView, &b).header();
fn import_block(&self, unverified: Unverified) -> Result<H256, BlockImportError> {
let header = unverified.header;
let h = header.hash();
let number: usize = header.number() as usize;
if number > self.blocks.read().len() {
Expand All @@ -539,7 +541,7 @@ impl ImportBlock for TestBlockChainClient {
*difficulty = *difficulty + header.difficulty().clone();
}
mem::replace(&mut *self.last_hash.write(), h.clone());
self.blocks.write().insert(h.clone(), b);
self.blocks.write().insert(h.clone(), unverified.bytes);
self.numbers.write().insert(number, h.clone());
let mut parent_hash = header.parent_hash().clone();
if number > 0 {
Expand All @@ -552,7 +554,7 @@ impl ImportBlock for TestBlockChainClient {
}
}
else {
self.blocks.write().insert(h.clone(), b.to_vec());
self.blocks.write().insert(h.clone(), unverified.bytes);
}
Ok(h)
}
Expand Down Expand Up @@ -856,8 +858,8 @@ impl IoClient for TestBlockChainClient {
self.miner.import_external_transactions(self, txs);
}

fn queue_ancient_block(&self, b: Bytes, _r: Bytes) -> Result<H256, BlockImportError> {
self.import_block(b)
fn queue_ancient_block(&self, unverified: Unverified, _r: Bytes) -> Result<H256, BlockImportError> {
self.import_block(unverified)
}

fn queue_consensus_message(&self, message: Bytes) {
Expand Down
5 changes: 3 additions & 2 deletions ethcore/src/client/traits.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -34,6 +34,7 @@ use receipt::LocalizedReceipt;
use trace::LocalizedTrace;
use transaction::{self, LocalizedTransaction, SignedTransaction};
use verification::queue::QueueInfo as BlockQueueInfo;
use verification::queue::kind::blocks::Unverified;
use state::StateInfo;
use header::Header;
use engines::EthEngine;
Expand Down Expand Up @@ -167,7 +168,7 @@ pub trait RegistryInfo {
/// Provides methods to import block into blockchain
pub trait ImportBlock {
/// Import a block into the blockchain.
fn import_block(&self, bytes: Bytes) -> Result<H256, BlockImportError>;
fn import_block(&self, block: Unverified) -> Result<H256, BlockImportError>;
}

/// Provides `call_contract` method
Expand Down Expand Up @@ -204,7 +205,7 @@ pub trait IoClient: Sync + Send {
fn queue_transactions(&self, transactions: Vec<Bytes>, peer_id: usize);

/// Queue block import with transaction receipts. Does no sealing and transaction validation.
fn queue_ancient_block(&self, block_bytes: Bytes, receipts_bytes: Bytes) -> Result<H256, BlockImportError>;
fn queue_ancient_block(&self, block_bytes: Unverified, receipts_bytes: Bytes) -> Result<H256, BlockImportError>;

/// Queue conensus engine message.
fn queue_consensus_message(&self, message: Bytes);
Expand Down
3 changes: 2 additions & 1 deletion ethcore/src/engines/validator_set/multi.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -158,6 +158,7 @@ mod tests {
use test_helpers::{generate_dummy_client_with_spec_and_accounts, generate_dummy_client_with_spec_and_data};
use types::ids::BlockId;
use ethereum_types::Address;
use verification::queue::kind::blocks::Unverified;

use super::Multi;

Expand Down Expand Up @@ -198,7 +199,7 @@ mod tests {
let sync_client = generate_dummy_client_with_spec_and_data(Spec::new_validator_multi, 0, 0, &[]);
sync_client.engine().register_client(Arc::downgrade(&sync_client) as _);
for i in 1..4 {
sync_client.import_block(client.block(BlockId::Number(i)).unwrap().into_inner()).unwrap();
sync_client.import_block(Unverified::from_rlp(client.block(BlockId::Number(i)).unwrap().into_inner()).unwrap()).unwrap();
Copy link
Collaborator

Choose a reason for hiding this comment

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

This is a very long and noisy line...

}
sync_client.flush_queue();
assert_eq!(sync_client.chain_info().best_block_number, 3);
Expand Down
3 changes: 2 additions & 1 deletion ethcore/src/engines/validator_set/safe_contract.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -458,6 +458,7 @@ mod tests {
use test_helpers::{generate_dummy_client_with_spec_and_accounts, generate_dummy_client_with_spec_and_data};
use super::super::ValidatorSet;
use super::{ValidatorSafeContract, EVENT_NAME_HASH};
use verification::queue::kind::blocks::Unverified;

#[test]
fn fetches_validators() {
Expand Down Expand Up @@ -530,7 +531,7 @@ mod tests {
let sync_client = generate_dummy_client_with_spec_and_data(Spec::new_validator_safe_contract, 0, 0, &[]);
sync_client.engine().register_client(Arc::downgrade(&sync_client) as _);
for i in 1..4 {
sync_client.import_block(client.block(BlockId::Number(i)).unwrap().into_inner()).unwrap();
sync_client.import_block(Unverified::from_rlp(client.block(BlockId::Number(i)).unwrap().into_inner()).unwrap()).unwrap();
}
sync_client.flush_queue();
assert_eq!(sync_client.chain_info().best_block_number, 3);
Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/miner/miner.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1148,7 +1148,7 @@ impl miner::MinerService for Miner {
|b| &b.hash() == &block_hash
) {
trace!(target: "miner", "Submitted block {}={}={} with seal {:?}", block_hash, b.hash(), b.header().bare_hash(), seal);
b.lock().try_seal(&*self.engine, seal).or_else(|(e, _)| {
b.lock().try_seal(&*self.engine, seal).or_else(|e| {
warn!(target: "miner", "Mined solution rejected: {}", e);
Err(ErrorKind::PowInvalid.into())
})
Expand Down
9 changes: 5 additions & 4 deletions ethcore/src/test_helpers.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -43,6 +43,7 @@ use blooms_db;
use kvdb::KeyValueDB;
use kvdb_rocksdb;
use tempdir::TempDir;
use verification::queue::kind::blocks::Unverified;
use encoded;

/// Creates test block with corresponding header
Expand Down Expand Up @@ -175,7 +176,7 @@ pub fn generate_dummy_client_with_spec_accounts_and_data<F>(test_spec: F, accoun

let b = b.close_and_lock().unwrap().seal(test_engine, vec![]).unwrap();

if let Err(e) = client.import_block(b.rlp_bytes()) {
if let Err(e) = client.import_block(Unverified::from_rlp(b.rlp_bytes()).unwrap()) {
panic!("error importing block which is valid by definition: {:?}", e);
}

Expand Down Expand Up @@ -211,7 +212,7 @@ pub fn push_blocks_to_client(client: &Arc<Client>, timestamp_salt: u64, starting
rolling_block_number = rolling_block_number + 1;
rolling_timestamp = rolling_timestamp + 10;

if let Err(e) = client.import_block(create_test_block(&header)) {
if let Err(e) = client.import_block(Unverified::from_rlp(create_test_block(&header)).unwrap()) {
panic!("error importing block which is valid by definition: {:?}", e);
}
}
Expand All @@ -231,7 +232,7 @@ pub fn push_block_with_transactions(client: &Arc<Client>, transactions: &[Signed
}
let b = b.close_and_lock().unwrap().seal(test_engine, vec![]).unwrap();

if let Err(e) = client.import_block(b.rlp_bytes()) {
if let Err(e) = client.import_block(Unverified::from_rlp(b.rlp_bytes()).unwrap()) {
panic!("error importing block which is valid by definition: {:?}", e);
}

Expand All @@ -253,7 +254,7 @@ pub fn get_test_client_with_blocks(blocks: Vec<Bytes>) -> Arc<Client> {
).unwrap();

for block in blocks {
if let Err(e) = client.import_block(block) {
if let Err(e) = client.import_block(Unverified::from_rlp(block).unwrap()) {
panic!("error importing block which is well-formed: {:?}", e);
}
}
Expand Down
Loading