From 2e840bc89ce071cf66f887e930f4ccfa283cbe8f Mon Sep 17 00:00:00 2001 From: Anton Gavrilov Date: Wed, 2 Aug 2017 17:10:06 +0200 Subject: [PATCH] Fixed comments after the review --- ethcore/res/null_morden.json | 3 +- ethcore/res/null_morden_with_reward.json | 2 +- ethcore/src/block.rs | 23 ++++--------- ethcore/src/engines/authority_round/mod.rs | 14 ++++---- ethcore/src/engines/null_engine.rs | 21 +++++++----- ethcore/src/engines/tendermint/mod.rs | 15 +++++---- ethcore/src/ethereum/ethash.rs | 20 +++++++----- ethcore/src/spec/spec.rs | 5 +-- ethcore/src/trace/db.rs | 38 ++++++---------------- ethcore/src/trace/executive_tracer.rs | 4 +-- ethcore/src/verification/verification.rs | 1 - 11 files changed, 61 insertions(+), 85 deletions(-) diff --git a/ethcore/res/null_morden.json b/ethcore/res/null_morden.json index 6958951ef..b615cdc29 100644 --- a/ethcore/res/null_morden.json +++ b/ethcore/res/null_morden.json @@ -8,8 +8,7 @@ "accountStartNonce": "0x0", "maximumExtraDataSize": "0x20", "minGasLimit": "0x1388", - "networkID" : "0x2", - "applyReward": false + "networkID" : "0x2" }, "genesis": { "seal": { diff --git a/ethcore/res/null_morden_with_reward.json b/ethcore/res/null_morden_with_reward.json index 1ddcad1a8..b7b1c9a0d 100644 --- a/ethcore/res/null_morden_with_reward.json +++ b/ethcore/res/null_morden_with_reward.json @@ -9,7 +9,7 @@ "maximumExtraDataSize": "0x20", "minGasLimit": "0x1388", "networkID" : "0x2", - "applyReward": true + "blockReward": "0x4563918244F40000" }, "genesis": { "seal": { diff --git a/ethcore/src/block.rs b/ethcore/src/block.rs index d23bdbc6f..d031f49a0 100644 --- a/ethcore/src/block.rs +++ b/ethcore/src/block.rs @@ -21,7 +21,7 @@ use std::sync::Arc; use std::collections::HashSet; use rlp::{UntrustedRlp, RlpStream, Encodable, Decodable, DecoderError}; -use util::{Bytes, Address, Hashable, U256, H256, ordered_trie_root, SHA3_NULL_RLP}; +use util::{Bytes, Address, Hashable, U256, H256, ordered_trie_root, SHA3_NULL_RLP, SHA3_EMPTY_LIST_RLP}; use util::error::{Mismatch, OutOfBounds}; use basic_types::{LogBloom, Seal}; @@ -91,8 +91,7 @@ pub struct ExecutedBlock { receipts: Vec, transactions_set: HashSet, state: State, - traces: Option>>, - tracing_enabled: bool, + traces: Option>> } /// A set of references to `ExecutedBlock` fields that are publicly accessible. @@ -109,8 +108,6 @@ pub struct BlockRefMut<'a> { pub state: &'a mut State, /// Traces. pub traces: &'a Option>>, - /// Tracing enabled flag. - pub tracing_enabled: &'a mut bool, } /// A set of immutable references to `ExecutedBlock` fields that are publicly accessible. @@ -127,8 +124,6 @@ pub struct BlockRef<'a> { pub state: &'a State, /// Traces. pub traces: &'a Option>>, - /// Tracing enabled flag. - pub tracing_enabled: &'a bool, } impl ExecutedBlock { @@ -142,7 +137,6 @@ impl ExecutedBlock { transactions_set: Default::default(), state: state, traces: if tracing {Some(Vec::new())} else {None}, - tracing_enabled: tracing, } } @@ -155,7 +149,6 @@ impl ExecutedBlock { state: &mut self.state, receipts: &self.receipts, traces: &self.traces, - tracing_enabled: &mut self.tracing_enabled, } } @@ -168,7 +161,6 @@ impl ExecutedBlock { state: &self.state, receipts: &self.receipts, traces: &self.traces, - tracing_enabled: &self.tracing_enabled, } } } @@ -206,7 +198,7 @@ pub trait IsBlock { fn uncles(&self) -> &[Header] { &self.block().uncles } /// Get tracing enabled flag for this block. - fn tracing_enabled(&self) -> bool { self.block().tracing_enabled } + fn tracing_enabled(&self) -> bool { self.block().traces.is_some() } } /// Trait for a object that has a state database. @@ -404,11 +396,8 @@ impl<'x> OpenBlock<'x> { let unclosed_state = s.block.state.clone(); match s.engine.on_close_block(&mut s.block) { - Ok(outcome) => match outcome.trace { - Some(t) => { - s.block.traces.as_mut().map(|traces| traces.push(t)); - }, - None => {}, + Ok(outcome) => if let Some(t) = outcome.trace { + s.block.traces.as_mut().map(|traces| traces.push(t)); }, Err(e) => warn!("Encountered error on closing the block: {}", e), } @@ -453,7 +442,7 @@ impl<'x> OpenBlock<'x> { s.block.header.set_transactions_root(ordered_trie_root(s.block.transactions.iter().map(|e| e.rlp_bytes().into_vec()))); } let uncle_bytes = s.block.uncles.iter().fold(RlpStream::new_list(s.block.uncles.len()), |mut s, u| {s.append_raw(&u.rlp(Seal::With), 1); s} ).out(); - if s.block.header.uncles_hash().is_zero() || s.block.header.receipts_root() == &SHA3_NULL_RLP { + if s.block.header.uncles_hash().is_zero() || s.block.header.uncles_hash() == &SHA3_EMPTY_LIST_RLP { s.block.header.set_uncles_hash(uncle_bytes.sha3()); } if s.block.header.receipts_root().is_zero() || s.block.header.receipts_root() == &SHA3_NULL_RLP { diff --git a/ethcore/src/engines/authority_round/mod.rs b/ethcore/src/engines/authority_round/mod.rs index 981cc45d2..d52e05cb6 100644 --- a/ethcore/src/engines/authority_round/mod.rs +++ b/ethcore/src/engines/authority_round/mod.rs @@ -545,6 +545,7 @@ impl Engine for AuthorityRound { /// Apply the block reward on finalisation of the block. fn on_close_block(&self, block: &mut ExecutedBlock) -> Result { + let tracing_enabled = block.tracing_enabled(); let fields = block.fields_mut(); let mut tracer = ExecutiveTracer::default(); // Bestow block reward @@ -553,18 +554,19 @@ impl Engine for AuthorityRound { .map_err(::error::Error::from) .and_then(|_| fields.state.commit()); - // Trace it - let block_miner = fields.header.author().clone(); - tracer.trace_reward(block_miner, self.params().block_reward, RewardType::Block); + if tracing_enabled { + let block_author = fields.header.author().clone(); + tracer.trace_reward(block_author, self.params().block_reward, RewardType::Block); + } // Commit state so that we can actually figure out the state root. if let Err(ref e) = res { warn!("Encountered error on closing block: {}", e); } match res { - Ok(_) => match *fields.tracing_enabled { - true => Ok(CloseOutcome{trace: Some(tracer.traces())}), - false => Ok(CloseOutcome{trace: None}) + Ok(_) => match tracing_enabled { + true => Ok(CloseOutcome { trace: Some(tracer.traces()) } ), + false => Ok(CloseOutcome { trace: None } ) }, Err(e) => Err(e) } diff --git a/ethcore/src/engines/null_engine.rs b/ethcore/src/engines/null_engine.rs index cb22e7661..3f29f5b41 100644 --- a/ethcore/src/engines/null_engine.rs +++ b/ethcore/src/engines/null_engine.rs @@ -17,7 +17,7 @@ use std::collections::BTreeMap; use util::Address; use builtin::Builtin; -use block::{ExecutedBlock, }; +use block::{ExecutedBlock, IsBlock}; use util::U256; use engines::{Engine, CloseOutcome}; use spec::CommonParams; @@ -71,11 +71,12 @@ impl Engine for NullEngine { } fn on_close_block(&self, block: &mut ExecutedBlock) -> Result { - if !self.params.apply_reward { + if self.params.block_reward == U256::zero() { return Ok(CloseOutcome{trace: None}); } /// Block reward + let tracing_enabled = block.tracing_enabled(); let fields = block.fields_mut(); let mut tracer = ExecutiveTracer::default(); @@ -86,25 +87,27 @@ impl Engine for NullEngine { CleanupMode::NoEmpty )?; - let block_miner = fields.header.author().clone(); - tracer.trace_reward(block_miner, result_block_reward, RewardType::Block); + if tracing_enabled { + let block_author = fields.header.author().clone(); + tracer.trace_reward(block_author, result_block_reward, RewardType::Block); + } /// Uncle rewards let result_uncle_reward = U256::from(10000000); for u in fields.uncles.iter() { - let uncle_miner = u.author().clone(); + let uncle_author = u.author().clone(); fields.state.add_balance( u.author(), &(result_uncle_reward), CleanupMode::NoEmpty )?; - tracer.trace_reward(uncle_miner, result_uncle_reward, RewardType::Uncle); + tracer.trace_reward(uncle_author, result_uncle_reward, RewardType::Uncle); } fields.state.commit()?; - match *fields.tracing_enabled { - true => Ok(CloseOutcome{trace: Some(tracer.traces())}), - false => Ok(CloseOutcome{trace: None}) + match tracing_enabled { + true => Ok(CloseOutcome { trace: Some(tracer.traces()) } ), + false => Ok(CloseOutcome { trace: None } ) } } } diff --git a/ethcore/src/engines/tendermint/mod.rs b/ethcore/src/engines/tendermint/mod.rs index 2d40c0314..cbe472a18 100644 --- a/ethcore/src/engines/tendermint/mod.rs +++ b/ethcore/src/engines/tendermint/mod.rs @@ -540,6 +540,7 @@ impl Engine for Tendermint { /// Apply the block reward on finalisation of the block. fn on_close_block(&self, block: &mut ExecutedBlock) -> Result{ + let tracing_enabled = block.tracing_enabled(); let fields = block.fields_mut(); let mut tracer = ExecutiveTracer::default(); // Bestow block reward @@ -548,22 +549,22 @@ impl Engine for Tendermint { .map_err(::error::Error::from) .and_then(|_| fields.state.commit()); - // Trace it - let block_miner = fields.header.author().clone(); - tracer.trace_reward(block_miner, self.params().block_reward, RewardType::Block); + if tracing_enabled { + let block_author = fields.header.author().clone(); + tracer.trace_reward(block_author, self.params().block_reward, RewardType::Block); + } // Commit state so that we can actually figure out the state root. if let Err(ref e) = res { warn!("Encountered error on closing block: {}", e); } match res { - Ok(_) => match *fields.tracing_enabled { - true => Ok(CloseOutcome{trace: Some(tracer.traces())}), - false => Ok(CloseOutcome{trace: None}) + Ok(_) => match tracing_enabled { + true => Ok(CloseOutcome { trace: Some(tracer.traces()) } ), + false => Ok(CloseOutcome { trace: None } ) }, Err(e) => Err(e) } - } fn verify_block_basic(&self, header: &Header, _block: Option<&[u8]>) -> Result<(), Error> { diff --git a/ethcore/src/ethereum/ethash.rs b/ethcore/src/ethereum/ethash.rs index 95a7554aa..195e39dbf 100644 --- a/ethcore/src/ethereum/ethash.rs +++ b/ethcore/src/ethereum/ethash.rs @@ -274,6 +274,7 @@ impl Engine for Arc { /// This assumes that all uncles are valid uncles (i.e. of at least one generation before the current). fn on_close_block(&self, block: &mut ExecutedBlock) -> Result { let reward = self.params().block_reward; + let tracing_enabled = block.tracing_enabled(); let fields = block.fields_mut(); let eras_rounds = self.ethash_params.ecip1017_era_rounds; let (eras, reward) = ecip1017_eras_block_reward(eras_rounds, reward, fields.header.number()); @@ -287,14 +288,15 @@ impl Engine for Arc { CleanupMode::NoEmpty )?; - // Trace it - let block_miner = fields.header.author().clone(); - tracer.trace_reward(block_miner, result_block_reward, RewardType::Block); + if tracing_enabled { + let block_author = fields.header.author().clone(); + tracer.trace_reward(block_author, result_block_reward, RewardType::Block); + } // Bestow uncle rewards let current_number = fields.header.number(); for u in fields.uncles.iter() { - let uncle_miner = u.author().clone(); + let uncle_author = u.author().clone(); let result_uncle_reward: U256; if eras == 0 { @@ -314,14 +316,16 @@ impl Engine for Arc { }?; // Trace uncle rewards - tracer.trace_reward(uncle_miner, result_uncle_reward, RewardType::Uncle); + if tracing_enabled { + tracer.trace_reward(uncle_author, result_uncle_reward, RewardType::Uncle); + } } // Commit state so that we can actually figure out the state root. fields.state.commit()?; - match *fields.tracing_enabled { - true => Ok(CloseOutcome{trace: Some(tracer.traces())}), - false => Ok(CloseOutcome{trace: None}) + match tracing_enabled { + true => Ok(CloseOutcome { trace: Some(tracer.traces()) } ), + false => Ok(CloseOutcome { trace: None } ) } } diff --git a/ethcore/src/spec/spec.rs b/ethcore/src/spec/spec.rs index b50b81095..a53db1edd 100644 --- a/ethcore/src/spec/spec.rs +++ b/ethcore/src/spec/spec.rs @@ -98,8 +98,6 @@ pub struct CommonParams { pub block_reward: U256, /// Registrar contract address. pub registrar: Address, - /// Apply reward - pub apply_reward: bool, } impl CommonParams { @@ -170,8 +168,7 @@ impl From for CommonParams { wasm: p.wasm.unwrap_or(false), gas_limit_bound_divisor: p.gas_limit_bound_divisor.into(), block_reward: p.block_reward.map_or_else(U256::zero, Into::into), - registrar: p.registrar.map_or_else(Address::new, Into::into), - apply_reward: p.apply_reward.unwrap_or(true), + registrar: p.registrar.map_or_else(Address::new, Into::into) } } } diff --git a/ethcore/src/trace/db.rs b/ethcore/src/trace/db.rs index 504648d38..5199fe0cd 100644 --- a/ethcore/src/trace/db.rs +++ b/ethcore/src/trace/db.rs @@ -215,20 +215,11 @@ impl TraceDB where T: DatabaseExtras { block_number: BlockNumber, tx_number: usize ) -> Vec { - let trace_tx_number; - let trace_tx_hash; - - match self.extras.transaction_hash(block_number, tx_number) { - Some(hash) => { - trace_tx_hash = Some(hash.clone()); - trace_tx_number = Some(tx_number); - }, - None => { - //None means trace without transaction (reward) - trace_tx_hash = None; - trace_tx_number = None; - } - } + let (trace_tx_number, trace_tx_hash) = match self.extras.transaction_hash(block_number, tx_number) { + Some(hash) => (Some(tx_number), Some(hash.clone())), + //None means trace without transaction (reward) + None => (None, None), + }; let flat_traces: Vec = traces.into(); flat_traces.into_iter() @@ -375,20 +366,11 @@ impl TraceDatabase for TraceDB where T: DatabaseExtras { .map(Into::>::into) .enumerate() .flat_map(|(tx_position, traces)| { - let trace_tx_number; - let trace_tx_hash; - - match self.extras.transaction_hash(block_number, tx_position) { - Some(hash) => { - trace_tx_hash = Some(hash.clone()); - trace_tx_number = Some(tx_position); - }, - None => { - //None means trace without transaction (reward) - trace_tx_hash = None; - trace_tx_number = None; - } - } + let (trace_tx_number, trace_tx_hash) = match self.extras.transaction_hash(block_number, tx_position) { + Some(hash) => (Some(tx_position), Some(hash.clone())), + //None means trace without transaction (reward) + None => (None, None), + }; traces.into_iter() .map(|trace| LocalizedTrace { diff --git a/ethcore/src/trace/executive_tracer.rs b/ethcore/src/trace/executive_tracer.rs index a6a5a95ea..61e2bd38b 100644 --- a/ethcore/src/trace/executive_tracer.rs +++ b/ethcore/src/trace/executive_tracer.rs @@ -159,7 +159,7 @@ impl Tracer for ExecutiveTracer { result: Res::None, trace_address: Default::default(), }; - debug!(target: "trace", "Traced failed suicide {:?}", trace); + debug!(target: "trace", "Traced suicide {:?}", trace); self.traces.push(trace); } @@ -174,7 +174,7 @@ impl Tracer for ExecutiveTracer { result: Res::None, trace_address: Default::default(), }; - debug!(target: "trace", "Traced failed reward {:?}", trace); + debug!(target: "trace", "Traced reward {:?}", trace); self.traces.push(trace); } diff --git a/ethcore/src/verification/verification.rs b/ethcore/src/verification/verification.rs index 6980a21d1..823d2ef70 100644 --- a/ethcore/src/verification/verification.rs +++ b/ethcore/src/verification/verification.rs @@ -591,7 +591,6 @@ mod tests { let mut params = CommonParams::default(); params.dust_protection_transition = 0; params.nonce_cap_increment = 2; - params.apply_reward = false; let mut header = Header::default(); header.set_number(1);