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

Break circular dependency between Client and Engine (part 1) #10833

Merged
merged 6 commits into from
Jul 4, 2019
Merged

Break circular dependency between Client and Engine (part 1) #10833

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
5b00859
First draft: pass the parent block header to on_close_block
dvdplm Jul 2, 2019
98f3cd9
typos and cleanup
dvdplm Jul 2, 2019
2f11a9c
whitespace
dvdplm Jul 2, 2019
91f81ac
Store parent in ClosedBlock as well so it can be reopen()'d
dvdplm Jul 2, 2019
6fa99ef
Don't pass parent when reopening a block
dvdplm Jul 2, 2019
8d3a6fe
Remove the unused ancestry param
dvdplm Jul 2, 2019
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
49 changes: 21 additions & 28 deletions ethcore/src/block.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -22,13 +22,13 @@
//! and can be appended to with transactions and uncles.
//!
//! When ready, `OpenBlock` can be closed and turned into a `ClosedBlock`. A `ClosedBlock` can
//! be reopend again by a miner under certain circumstances. On block close, state commit is
//! be re-opend again by a miner under certain circumstances. On block close, state commit is
//! performed.
//!
//! `LockedBlock` is a version of a `ClosedBlock` that cannot be reopened. It can be sealed
//! using an engine.
//!
//! `ExecutedBlock` is an underlaying data structure used by all structs above to store block
//! `ExecutedBlock` is an underlying data structure used by all structs above to store block
//! related info.

use std::{cmp, ops};
Expand All @@ -52,7 +52,7 @@ use vm::{EnvInfo, LastHashes};
use hash::keccak;
use rlp::{RlpStream, Encodable, encode_list};
use types::transaction::{SignedTransaction, Error as TransactionError};
use types::header::{Header, ExtendedHeader};
use types::header::Header;
use types::receipt::{Receipt, TransactionOutcome};

/// Block that is ready for transactions to be added.
Expand All @@ -62,6 +62,7 @@ use types::receipt::{Receipt, TransactionOutcome};
pub struct OpenBlock<'x> {
block: ExecutedBlock,
engine: &'x dyn Engine,
parent: Header,
}

/// Just like `OpenBlock`, except that we've applied `Engine::on_close_block`, finished up the non-seal header fields,
Expand All @@ -72,6 +73,7 @@ pub struct OpenBlock<'x> {
pub struct ClosedBlock {
block: ExecutedBlock,
unclosed_state: State<StateDB>,
parent: Header,
}

/// Just like `ClosedBlock` except that we can't reopen it and it's faster.
Expand Down Expand Up @@ -102,7 +104,7 @@ pub struct ExecutedBlock {
pub receipts: Vec<Receipt>,
/// Hashes of already executed transactions.
pub transactions_set: HashSet<H256>,
/// Underlaying state.
/// Underlying state.
pub state: State<StateDB>,
/// Transaction traces.
pub traces: Tracing,
Expand All @@ -119,13 +121,13 @@ impl ExecutedBlock {
uncles: Default::default(),
receipts: Default::default(),
transactions_set: Default::default(),
state: state,
state,
traces: if tracing {
Tracing::enabled()
} else {
Tracing::Disabled
},
last_hashes: last_hashes,
last_hashes,
}
}

Expand Down Expand Up @@ -162,7 +164,7 @@ pub trait Drain {

impl<'x> OpenBlock<'x> {
/// Create a new `OpenBlock` ready for transaction pushing.
pub fn new<'a, I: IntoIterator<Item = ExtendedHeader>>(
pub fn new<'a>(
engine: &'x dyn Engine,
factories: Factories,
tracing: bool,
Expand All @@ -173,14 +175,11 @@ impl<'x> OpenBlock<'x> {
gas_range_target: (U256, U256),
extra_data: Bytes,
is_epoch_begin: bool,
ancestry: I,
) -> Result<Self, Error> {
let number = parent.number() + 1;
let state = State::from_existing(db, parent.state_root().clone(), engine.account_start_nonce(number), factories)?;
let mut r = OpenBlock {
block: ExecutedBlock::new(state, last_hashes, tracing),
engine: engine,
};

let mut r = OpenBlock { block: ExecutedBlock::new(state, last_hashes, tracing), engine, parent: parent.clone() };

r.block.header.set_parent_hash(parent.hash());
r.block.header.set_number(number);
Expand All @@ -195,7 +194,7 @@ impl<'x> OpenBlock<'x> {
engine.populate_from_parent(&mut r.block.header, parent);

engine.machine().on_new_block(&mut r.block)?;
engine.on_new_block(&mut r.block, is_epoch_begin, &mut ancestry.into_iter())?;
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The ancestry was never used so I'm not sure what it was used for tbh.

Copy link
Collaborator

Choose a reason for hiding this comment

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

good catch!

Copy link
Collaborator

Choose a reason for hiding this comment

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

This was for engines that need to compute additional information regarding finalisation (for example, finalise blocks based on ancestor finalisation information).

Looks like we at least don’t need it right now!

engine.on_new_block(&mut r.block, is_epoch_begin)?;

Ok(r)
}
Expand Down Expand Up @@ -297,19 +296,20 @@ impl<'x> OpenBlock<'x> {
/// Turn this into a `ClosedBlock`.
pub fn close(self) -> Result<ClosedBlock, Error> {
let unclosed_state = self.block.state.clone();
let parent = self.parent.clone();
let locked = self.close_and_lock()?;

Ok(ClosedBlock {
block: locked.block,
unclosed_state,
parent,
})
}

/// 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.engine.on_close_block(&mut s.block, &s.parent)?;
s.block.state.commit()?;

s.block.header.set_transactions_root(ordered_trie_root(s.block.transactions.iter().map(|e| e.rlp_bytes())));
Expand Down Expand Up @@ -378,10 +378,8 @@ impl ClosedBlock {
// revert rewards (i.e. set state back at last transaction's state).
let mut block = self.block;
block.state = self.unclosed_state;
OpenBlock {
block: block,
engine: engine,
}
let parent = self.parent;
OpenBlock { block, engine, parent }
}
}

Expand Down Expand Up @@ -479,7 +477,6 @@ pub(crate) fn enact(
last_hashes: Arc<LastHashes>,
factories: Factories,
is_epoch_begin: bool,
ancestry: &mut dyn Iterator<Item=ExtendedHeader>,
) -> Result<LockedBlock, Error> {
// For trace log
let trace_state = if log_enabled!(target: "enact", ::log::Level::Trace) {
Expand All @@ -501,7 +498,6 @@ pub(crate) fn enact(
(3141562.into(), 31415620.into()),
vec![],
is_epoch_begin,
ancestry,
)?;

if let Some(ref s) = trace_state {
Expand All @@ -522,7 +518,7 @@ pub(crate) fn enact(
b.close_and_lock()
}

/// Enact the block given by `block_bytes` using `engine` on the database `db` with given `parent` block header
/// Enact the block given by `block_bytes` using `engine` on the database `db` with the given `parent` block header
pub fn enact_verified(
block: PreverifiedBlock,
engine: &dyn Engine,
Expand All @@ -532,7 +528,6 @@ pub fn enact_verified(
last_hashes: Arc<LastHashes>,
factories: Factories,
is_epoch_begin: bool,
ancestry: &mut dyn Iterator<Item=ExtendedHeader>,
) -> Result<LockedBlock, Error> {

enact(
Expand All @@ -546,7 +541,6 @@ pub fn enact_verified(
last_hashes,
factories,
is_epoch_begin,
ancestry,
)
}

Expand Down Expand Up @@ -608,7 +602,6 @@ mod tests {
(3141562.into(), 31415620.into()),
vec![],
false,
None,
)?;

b.populate_from(&header);
Expand Down Expand Up @@ -643,7 +636,7 @@ mod tests {
let genesis_header = spec.genesis_header();
let db = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let last_hashes = Arc::new(vec![genesis_header.hash()]);
let b = OpenBlock::new(&*spec.engine, Default::default(), false, db, &genesis_header, last_hashes, Address::zero(), (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b = OpenBlock::new(&*spec.engine, Default::default(), false, db, &genesis_header, last_hashes, Address::zero(), (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b = b.close_and_lock().unwrap();
let _ = b.seal(&*spec.engine, vec![]);
}
Expand All @@ -657,7 +650,7 @@ mod tests {

let db = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let last_hashes = Arc::new(vec![genesis_header.hash()]);
let b = OpenBlock::new(engine, Default::default(), false, db, &genesis_header, last_hashes.clone(), Address::zero(), (3141562.into(), 31415620.into()), vec![], false, None).unwrap()
let b = OpenBlock::new(engine, Default::default(), false, db, &genesis_header, last_hashes.clone(), Address::zero(), (3141562.into(), 31415620.into()), vec![], false).unwrap()
.close_and_lock().unwrap().seal(engine, vec![]).unwrap();
let orig_bytes = b.rlp_bytes();
let orig_db = b.drain().state.drop().1;
Expand All @@ -682,7 +675,7 @@ mod tests {

let db = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let last_hashes = Arc::new(vec![genesis_header.hash()]);
let mut open_block = OpenBlock::new(engine, Default::default(), false, db, &genesis_header, last_hashes.clone(), Address::zero(), (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let mut open_block = OpenBlock::new(engine, Default::default(), false, db, &genesis_header, last_hashes.clone(), Address::zero(), (3141562.into(), 31415620.into()), vec![], false).unwrap();
let mut uncle1_header = Header::new();
uncle1_header.set_extra_data(b"uncle1".to_vec());
let mut uncle2_header = Header::new();
Expand Down
2 changes: 0 additions & 2 deletions ethcore/src/client/client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -407,7 +407,6 @@ impl Importer {
last_hashes,
client.factories.clone(),
is_epoch_begin,
&mut chain.ancestry_with_metadata_iter(*header.parent_hash()),
);

let mut locked_block = match enact_result {
Expand Down Expand Up @@ -2361,7 +2360,6 @@ impl PrepareOpenBlock for Client {
gas_range_target,
extra_data,
is_epoch_begin,
chain.ancestry_with_metadata_iter(best_header.hash()),
)?;

// Add uncles
Expand Down
1 change: 0 additions & 1 deletion ethcore/src/client/test_client.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -417,7 +417,6 @@ impl PrepareOpenBlock for TestBlockChainClient {
gas_range_target,
extra_data,
false,
None,
)?;
// TODO [todr] Override timestamp for predictability
open_block.set_timestamp(*self.latest_block_timestamp.read());
Expand Down
49 changes: 19 additions & 30 deletions ethcore/src/engines/authority_round/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1211,7 +1211,6 @@ impl Engine for AuthorityRound {
&self,
block: &mut ExecutedBlock,
epoch_begin: bool,
_ancestry: &mut dyn Iterator<Item=ExtendedHeader>,
) -> Result<(), Error> {
// with immediate transitions, we don't use the epoch mechanism anyway.
// the genesis is always considered an epoch, but we ignore it intentionally.
Expand All @@ -1236,26 +1235,18 @@ impl Engine for AuthorityRound {
}

/// Apply the block reward on finalisation of the block.
fn on_close_block(&self, block: &mut ExecutedBlock) -> Result<(), Error> {
fn on_close_block(
&self,
block: &mut ExecutedBlock,
parent: &Header,
) -> Result<(), Error> {
let mut beneficiaries = Vec::new();
if block.header.number() >= self.empty_steps_transition {
let empty_steps = if block.header.seal().is_empty() {
// this is a new block, calculate rewards based on the empty steps messages we have accumulated
let client = match self.client.read().as_ref().and_then(|weak| weak.upgrade()) {
Some(client) => client,
None => {
debug!(target: "engine", "Unable to close block: missing client ref.");
return Err(EngineError::RequiresClient.into())
},
};

let parent = client.block_header(::client::BlockId::Hash(*block.header.parent_hash()))
.expect("hash is from parent; parent header must exist; qed")
.decode()?;

let parent_step = header_step(&parent, self.empty_steps_transition)?;
let parent_step = header_step(parent, self.empty_steps_transition)?;
let current_step = self.step.inner.load();
self.empty_steps(parent_step.into(), current_step.into(), parent.hash())
self.empty_steps(parent_step, current_step, parent.hash())
} else {
// we're verifying a block, extract empty steps from the seal
header_empty_steps(&block.header)?
Expand Down Expand Up @@ -1707,9 +1698,9 @@ mod tests {
let db1 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let db2 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let last_hashes = Arc::new(vec![genesis_header.hash()]);
let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b1 = b1.close_and_lock().unwrap();
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes, addr2, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes, addr2, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b2 = b2.close_and_lock().unwrap();

engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
Expand Down Expand Up @@ -1741,9 +1732,9 @@ mod tests {
let db2 = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let last_hashes = Arc::new(vec![genesis_header.hash()]);

let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b1 = b1.close_and_lock().unwrap();
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes, addr2, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes, addr2, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b2 = b2.close_and_lock().unwrap();

engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
Expand Down Expand Up @@ -1991,7 +1982,7 @@ mod tests {

engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));

let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b1 = b1.close_and_lock().unwrap();

// the block is empty so we don't seal and instead broadcast an empty step message
Expand Down Expand Up @@ -2029,7 +2020,7 @@ mod tests {
engine.register_client(Arc::downgrade(&client) as _);

// step 2
let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b1 = b1.close_and_lock().unwrap();

// since the block is empty it isn't sealed and we generate empty steps
Expand All @@ -2038,7 +2029,7 @@ mod tests {
engine.step();

// step 3
let mut b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes.clone(), addr2, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let mut b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes.clone(), addr2, (3141562.into(), 31415620.into()), vec![], false).unwrap();
b2.push_transaction(Transaction {
action: Action::Create,
nonce: U256::from(0),
Expand Down Expand Up @@ -2082,7 +2073,7 @@ mod tests {
engine.register_client(Arc::downgrade(&client) as _);

// step 2
let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b1 = b1.close_and_lock().unwrap();

// since the block is empty it isn't sealed and we generate empty steps
Expand All @@ -2091,15 +2082,15 @@ mod tests {
engine.step();

// step 3
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes.clone(), addr2, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes.clone(), addr2, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b2 = b2.close_and_lock().unwrap();
engine.set_signer(Box::new((tap.clone(), addr2, "0".into())));
assert_eq!(engine.generate_seal(&b2, &genesis_header), Seal::None);
engine.step();

// step 4
// the spec sets the maximum_empty_steps to 2 so we will now seal an empty block and include the empty step messages
let b3 = OpenBlock::new(engine, Default::default(), false, db3, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b3 = OpenBlock::new(engine, Default::default(), false, db3, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b3 = b3.close_and_lock().unwrap();

engine.set_signer(Box::new((tap.clone(), addr1, "1".into())));
Expand Down Expand Up @@ -2132,7 +2123,7 @@ mod tests {
engine.register_client(Arc::downgrade(&client) as _);

// step 2
let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b1 = OpenBlock::new(engine, Default::default(), false, db1, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b1 = b1.close_and_lock().unwrap();

// since the block is empty it isn't sealed and we generate empty steps
Expand All @@ -2142,7 +2133,7 @@ mod tests {

// step 3
// the signer of the accumulated empty step message should be rewarded
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b2 = OpenBlock::new(engine, Default::default(), false, db2, &genesis_header, last_hashes.clone(), addr1, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let addr1_balance = b2.state.balance(&addr1).unwrap();

// after closing the block `addr1` should be reward twice, one for the included empty step message and another for block creation
Expand Down Expand Up @@ -2242,7 +2233,6 @@ mod tests {
(3141562.into(), 31415620.into()),
vec![],
false,
None,
).unwrap();
let b1 = b1.close_and_lock().unwrap();

Expand All @@ -2264,7 +2254,6 @@ mod tests {
(3141562.into(), 31415620.into()),
vec![],
false,
None,
).unwrap();
let addr1_balance = b2.state.balance(&addr1).unwrap();

Expand Down
2 changes: 1 addition & 1 deletion ethcore/src/engines/basic_authority.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -268,7 +268,7 @@ mod tests {
let genesis_header = spec.genesis_header();
let db = spec.ensure_db_good(get_temp_state_db(), &Default::default()).unwrap();
let last_hashes = Arc::new(vec![genesis_header.hash()]);
let b = OpenBlock::new(engine, Default::default(), false, db, &genesis_header, last_hashes, addr, (3141562.into(), 31415620.into()), vec![], false, None).unwrap();
let b = OpenBlock::new(engine, Default::default(), false, db, &genesis_header, last_hashes, addr, (3141562.into(), 31415620.into()), vec![], false).unwrap();
let b = b.close_and_lock().unwrap();
if let Seal::Regular(seal) = engine.generate_seal(&b, &genesis_header) {
assert!(b.try_seal(engine, seal).is_ok());
Expand Down
Loading