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

Rewards on closing blocks #6194

Merged
merged 31 commits into from
Aug 30, 2017
Merged
Show file tree
Hide file tree
Changes from 20 commits
Commits
Show all changes
31 commits
Select commit Hold shift + click to select a range
ab61538
Tracing for rewards added. Without tests for now
grbIzl Jul 18, 2017
16f3948
Warnings removed
grbIzl Jul 18, 2017
7312803
Working test with block reward added
grbIzl Jul 25, 2017
53c3b77
Complete version of tracing test with reward
grbIzl Jul 26, 2017
af1fbb3
Unit tests for tracing reward added
grbIzl Jul 27, 2017
b193534
Fixed errors after merge with master
grbIzl Jul 27, 2017
1f3f911
Transaction ids made optional in order to reflect not transactional t…
grbIzl Jul 28, 2017
94efa3a
Miner field renamed to author
grbIzl Jul 31, 2017
bb043ba
Tabs corrected
grbIzl Jul 31, 2017
06862c7
Tracing for rewards added. Without tests for now
grbIzl Jul 18, 2017
204a63a
Warnings removed
grbIzl Jul 18, 2017
5086dc3
Working test with block reward added
grbIzl Jul 25, 2017
3fdb912
Complete version of tracing test with reward
grbIzl Jul 26, 2017
f2d12ae
Unit tests for tracing reward added
grbIzl Jul 27, 2017
45043c3
Fixed errors after merge with master
grbIzl Jul 27, 2017
1a3f3ff
Transaction ids made optional in order to reflect not transactional t…
grbIzl Jul 28, 2017
9374e31
Miner field renamed to author
grbIzl Jul 31, 2017
e809582
Tabs corrected
grbIzl Jul 31, 2017
8826fdb
Merge with head
grbIzl Jul 31, 2017
141c2fd
Fixed comments after review and test after rebase
grbIzl Jul 31, 2017
2e840bc
Fixed comments after the review
grbIzl Aug 2, 2017
01ea968
Merge with master
grbIzl Aug 2, 2017
01a02a8
Wasm test link changed
grbIzl Aug 2, 2017
3043432
Modification of traces moved to engines
grbIzl Aug 3, 2017
655ed93
Common engine method for bestowing rewards created
grbIzl Aug 4, 2017
dd91121
Common method for tracing refactored due to comments on review
grbIzl Aug 4, 2017
bfd238e
Style fixed after review
grbIzl Aug 10, 2017
50495c6
Merge branch 'master' into Fix-4858
gavofyork Aug 29, 2017
e366645
Consistent use of `,`s
gavofyork Aug 29, 2017
88200a1
Merge branch 'master' into Fix-4858
debris Aug 30, 2017
8a420d6
fixed merge
debris Aug 30, 2017
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
3 changes: 2 additions & 1 deletion ethcore/res/null_morden.json
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,8 @@
"accountStartNonce": "0x0",
"maximumExtraDataSize": "0x20",
"minGasLimit": "0x1388",
"networkID" : "0x2"
"networkID" : "0x2",
"applyReward": false
},
"genesis": {
"seal": {
Expand Down
35 changes: 35 additions & 0 deletions ethcore/res/null_morden_with_reward.json
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
{
"name": "Morden",
"engine": {
"null": null
},
"params": {
"gasLimitBoundDivisor": "0x0400",
"accountStartNonce": "0x0",
"maximumExtraDataSize": "0x20",
"minGasLimit": "0x1388",
"networkID" : "0x2",
"applyReward": true
},
"genesis": {
"seal": {
"ethereum": {
"nonce": "0x00006d6f7264656e",
"mixHash": "0x00000000000000000000000000000000000000647572616c65787365646c6578"
}
},
"difficulty": "0x20000",
"author": "0x0000000000000000000000000000000000000000",
"timestamp": "0x00",
"parentHash": "0x0000000000000000000000000000000000000000000000000000000000000000",
"extraData": "0x",
"gasLimit": "0x2fefd8"
},
"accounts": {
"0000000000000000000000000000000000000001": { "balance": "1", "nonce": "1048576", "builtin": { "name": "ecrecover", "pricing": { "linear": { "base": 3000, "word": 0 } } } },
"0000000000000000000000000000000000000002": { "balance": "1", "nonce": "1048576", "builtin": { "name": "sha256", "pricing": { "linear": { "base": 60, "word": 12 } } } },
"0000000000000000000000000000000000000003": { "balance": "1", "nonce": "1048576", "builtin": { "name": "ripemd160", "pricing": { "linear": { "base": 600, "word": 120 } } } },
"0000000000000000000000000000000000000004": { "balance": "1", "nonce": "1048576", "builtin": { "name": "identity", "pricing": { "linear": { "base": 15, "word": 3 } } } },
"102e61f5d8f9bc71d0ad4a084df4e65e05ce0e1c": { "balance": "1606938044258990275541962092341162602522202993782792835301376", "nonce": "1048576" }
}
}
34 changes: 29 additions & 5 deletions ethcore/src/block.rs
Original file line number Diff line number Diff line change
Expand Up @@ -92,6 +92,7 @@ pub struct ExecutedBlock {
transactions_set: HashSet<H256>,
state: State<StateDB>,
traces: Option<Vec<Vec<FlatTrace>>>,
Copy link
Contributor

Choose a reason for hiding this comment

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

style: don't kill the trailing ,

tracing_enabled: bool,
}

/// A set of references to `ExecutedBlock` fields that are publicly accessible.
Expand All @@ -108,6 +109,8 @@ pub struct BlockRefMut<'a> {
pub state: &'a mut State<StateDB>,
/// Traces.
pub traces: &'a Option<Vec<Vec<FlatTrace>>>,
/// Tracing enabled flag.
pub tracing_enabled: &'a mut bool,
}

/// A set of immutable references to `ExecutedBlock` fields that are publicly accessible.
Expand All @@ -124,6 +127,8 @@ pub struct BlockRef<'a> {
pub state: &'a State<StateDB>,
/// Traces.
pub traces: &'a Option<Vec<Vec<FlatTrace>>>,
/// Tracing enabled flag.
pub tracing_enabled: &'a bool,
}

impl ExecutedBlock {
Expand All @@ -137,6 +142,7 @@ impl ExecutedBlock {
transactions_set: Default::default(),
state: state,
traces: if tracing {Some(Vec::new())} else {None},
tracing_enabled: tracing,
Copy link
Collaborator

Choose a reason for hiding this comment

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

Can't we just check if trace.is_some() instead of storing it here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, we can. I thought about it. But IMHO relying on the explicit flag is better than basing on the assumption. The logic of working with traces can be changed and this assumption may become false

Copy link
Collaborator

Choose a reason for hiding this comment

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

You could just add a member function that would return the flag instead of introducing another state variable.

}
}

Expand All @@ -149,6 +155,7 @@ impl ExecutedBlock {
state: &mut self.state,
receipts: &self.receipts,
traces: &self.traces,
tracing_enabled: &mut self.tracing_enabled,
}
}

Expand All @@ -161,6 +168,7 @@ impl ExecutedBlock {
state: &self.state,
receipts: &self.receipts,
traces: &self.traces,
tracing_enabled: &self.tracing_enabled,
}
}
}
Expand Down Expand Up @@ -196,6 +204,9 @@ pub trait IsBlock {

/// Get all uncles in this block.
fn uncles(&self) -> &[Header] { &self.block().uncles }

/// Get tracing enabled flag for this block.
fn tracing_enabled(&self) -> bool { self.block().tracing_enabled }
}

/// Trait for a object that has a state database.
Expand Down Expand Up @@ -392,9 +403,16 @@ impl<'x> OpenBlock<'x> {

let unclosed_state = s.block.state.clone();

if let Err(e) = s.engine.on_close_block(&mut s.block) {
warn!("Encountered error on closing the block: {}", e);
match s.engine.on_close_block(&mut s.block) {
Ok(outcome) => match outcome.trace {
Copy link
Collaborator

Choose a reason for hiding this comment

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

if let(Some(t)) = outcome.trace could be used here

Some(t) => {
s.block.traces.as_mut().map(|traces| traces.push(t));
},
None => {},
},
Err(e) => warn!("Encountered error on closing the block: {}", e),
}

if let Err(e) = s.block.state.commit() {
warn!("Encountered error on state commit: {}", e);
}
Expand All @@ -418,8 +436,14 @@ impl<'x> OpenBlock<'x> {
pub fn close_and_lock(self) -> LockedBlock {
let mut s = self;

if let Err(e) = s.engine.on_close_block(&mut s.block) {
warn!("Encountered error on closing the block: {}", e);
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 => {},
},
Err(e) => warn!("Encountered error on closing the block: {}", e),
}

if let Err(e) = s.block.state.commit() {
Expand All @@ -429,7 +453,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() {
if s.block.header.uncles_hash().is_zero() || s.block.header.receipts_root() == &SHA3_NULL_RLP {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why is this necessary and related?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I incorrectly fixed the bug with this check. Thanks! I missed it (((

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 {
Expand Down
19 changes: 16 additions & 3 deletions ethcore/src/engines/authority_round/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,8 @@ use account_provider::AccountProvider;
use block::*;
use builtin::Builtin;
use client::{Client, EngineClient};
use engines::{Call, Engine, Seal, EngineError, ConstructedVerifier};
use engines::{Call, Engine, Seal, EngineError, ConstructedVerifier, CloseOutcome};
use trace::{Tracer, ExecutiveTracer, RewardType};
use error::{Error, TransactionError, BlockError};
use ethjson;
use header::{Header, BlockNumber};
Expand Down Expand Up @@ -543,18 +544,30 @@ 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) -> Result<CloseOutcome, Error> {
let fields = block.fields_mut();
let mut tracer = ExecutiveTracer::default();
Copy link
Collaborator

Choose a reason for hiding this comment

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

nitpick: The tracing happens in Engine not Executive so we could have a separate struct for tracing the rewards.

// Bestow block reward
let reward = self.params().block_reward;
let res = fields.state.add_balance(fields.header.author(), &reward, CleanupMode::NoEmpty)
.map_err(::error::Error::from)
.and_then(|_| fields.state.commit());

// Trace it
let block_miner = fields.header.author().clone();
Copy link
Collaborator

Choose a reason for hiding this comment

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

block_author is a more generic name for the miner

tracer.trace_reward(block_miner, 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);
Copy link
Collaborator

Choose a reason for hiding this comment

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

This could be inside match block, but if we decide to add traces directly it won't be needed.

}
res
match res {
Ok(_) => match *fields.tracing_enabled {
true => Ok(CloseOutcome{trace: Some(tracer.traces())}),
false => Ok(CloseOutcome{trace: None})
},
Err(e) => Err(e)
}
}

/// Check the number of seal fields.
Expand Down
12 changes: 10 additions & 2 deletions ethcore/src/engines/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ use error::Error;
use evm::Schedule;
use header::{Header, BlockNumber};
use receipt::Receipt;
use trace::FlatTrace;
use snapshot::SnapshotComponents;
use spec::CommonParams;
use transaction::{UnverifiedTransaction, SignedTransaction};
Expand Down Expand Up @@ -81,6 +82,13 @@ pub enum EngineError {
RequiresClient,
}

/// Used to return information about close block operation.
#[derive(Debug)]
pub struct CloseOutcome {
/// The trace for the closing block, if None if tracing is disabled.
pub trace: Option<Vec<FlatTrace>>,
}

impl fmt::Display for EngineError {
fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result {
use self::EngineError::*;
Expand Down Expand Up @@ -228,8 +236,8 @@ pub trait Engine : Sync + Send {
}

/// Block transformation functions, after the transactions.
fn on_close_block(&self, _block: &mut ExecutedBlock) -> Result<(), Error> {
Ok(())
fn on_close_block(&self, _block: &mut ExecutedBlock) -> Result<CloseOutcome, Error> {
Ok(CloseOutcome{trace: None})
}

/// None means that it requires external input (e.g. PoW) to seal a block.
Expand Down
45 changes: 44 additions & 1 deletion ethcore/src/engines/null_engine.rs
Original file line number Diff line number Diff line change
Expand Up @@ -17,10 +17,15 @@
use std::collections::BTreeMap;
use util::Address;
use builtin::Builtin;
use engines::Engine;
use block::{ExecutedBlock, };
use util::U256;
use engines::{Engine, CloseOutcome};
Copy link
Contributor

Choose a reason for hiding this comment

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

Better not to use glob imports where avoidable

use spec::CommonParams;
use evm::Schedule;
use header::BlockNumber;
use error::Error;
use state::CleanupMode;
use trace::{Tracer, ExecutiveTracer, RewardType};

/// An engine which does not provide any consensus mechanism and does not seal blocks.
pub struct NullEngine {
Expand Down Expand Up @@ -64,4 +69,42 @@ impl Engine for NullEngine {
fn snapshot_components(&self) -> Option<Box<::snapshot::SnapshotComponents>> {
Some(Box::new(::snapshot::PowSnapshot(10000)))
}

fn on_close_block(&self, block: &mut ExecutedBlock) -> Result<CloseOutcome, Error> {
if !self.params.apply_reward {
return Ok(CloseOutcome{trace: None});
}

/// Block reward
let fields = block.fields_mut();
let mut tracer = ExecutiveTracer::default();

let result_block_reward = U256::from(1000000000);
fields.state.add_balance(
fields.header.author(),
&result_block_reward,
CleanupMode::NoEmpty
)?;

let block_miner = fields.header.author().clone();
tracer.trace_reward(block_miner, 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();
fields.state.add_balance(
u.author(),
&(result_uncle_reward),
CleanupMode::NoEmpty
)?;
tracer.trace_reward(uncle_miner, result_uncle_reward, RewardType::Uncle);
}

fields.state.commit()?;
match *fields.tracing_enabled {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Why not alter the block traces directly here? We modify the state anyway?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't know the reasoning for this but traces are not mutable from outside. Actually I've repeated the logic made for tracing transactions - traces are added inside block already

Copy link
Collaborator

Choose a reason for hiding this comment

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

I think it's because Executive operates only on state and transaction not on full blocks.
IMHO it would be cleaner to actually alter the traces directly here.

true => Ok(CloseOutcome{trace: Some(tracer.traces())}),
false => Ok(CloseOutcome{trace: None})
}
}
}
20 changes: 17 additions & 3 deletions ethcore/src/engines/tendermint/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,8 @@ use ethkey::{Message, public_to_address, recover, Signature};
use account_provider::AccountProvider;
use block::*;
use spec::CommonParams;
use engines::{Engine, Seal, EngineError, ConstructedVerifier};
use engines::{Engine, Seal, EngineError, CloseOutcome, ConstructedVerifier};
use trace::{Tracer, ExecutiveTracer, RewardType};
use state::CleanupMode;
use io::IoService;
use super::signer::EngineSigner;
Expand Down Expand Up @@ -538,18 +539,31 @@ impl Engine for Tendermint {
}

/// 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) -> Result<CloseOutcome, Error>{
let fields = block.fields_mut();
let mut tracer = ExecutiveTracer::default();
// Bestow block reward
let reward = self.params().block_reward;
let res = fields.state.add_balance(fields.header.author(), &reward, CleanupMode::NoEmpty)
.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);

// 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);
}
res
match res {
Ok(_) => match *fields.tracing_enabled {
true => Ok(CloseOutcome{trace: Some(tracer.traces())}),
false => Ok(CloseOutcome{trace: None})
Copy link
Collaborator

Choose a reason for hiding this comment

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

spaces around { here and in other places

},
Err(e) => Err(e)
}

}

fn verify_block_basic(&self, header: &Header, _block: Option<&[u8]>) -> Result<(), Error> {
Expand Down
Loading