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

Commit

Permalink
Break circular dependency between Client and Engine (part 1) (#10833)
Browse files Browse the repository at this point in the history 
* First draft: pass the parent block header to on_close_block

* typos and cleanup

* whitespace

* Store parent in ClosedBlock as well so it can be reopen()'d

* Don't pass parent when reopening a block

* Remove the unused ancestry param
  • Loading branch information
@dvdplm
dvdplm authored Jul 4, 2019
1 parent 09edb94 commit 582a4ea
Show file tree
Hide file tree
Showing 15 changed files with 64 additions and 80 deletions.
49 changes: 21 additions & 28 deletions ethcore/src/block.rs
View file
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())?;
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
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
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
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
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

0 comments on commit 582a4ea

Please sign in to comment.