Rewards on closing blocks #6194

grbIzl · 2017-07-31T10:39:56Z

Added tracing of rewards on closing blocks. The following major changes made:

Add new action Reward with reward type (Block or Uncle)
Extended engine's API in order to return traces on closing block.
New parameter for engine's spec params added (apply_reward). By default it's true
New engine spec null_morden_with_reward created (in order to distinguish with default null_morden without reward)
Db code of trace filtering was modified in order to reflect, that traces can be block-related not only transaction-related
Localisation trace (passed outside by rpc) modified. Transaction id and hash in trace made optional. So it can be null now. This should be expressed in release notes somehow.
Integration and unit tests added

The work was done in terms of #4858

…races

NikVolf · 2017-07-31T10:55:19Z

needs a rebase to master

…races

keorn · 2017-07-31T12:04:33Z

ethcore/src/spec/spec.rs

@@ -166,9 +168,17 @@ impl From<ethjson::spec::Params> for CommonParams {
 			nonce_cap_increment: p.nonce_cap_increment.map_or(64, Into::into),
 			remove_dust_contracts: p.remove_dust_contracts.unwrap_or(false),
 			wasm: p.wasm.unwrap_or(false),
+<<<<<<< HEAD


rphmeier · 2017-07-31T13:04:28Z

ethcore/src/block.rs

@@ -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 }


could just return bool, it's actually smaller than the ref type

rphmeier · 2017-07-31T13:06:35Z

ethcore/src/engines/null_engine.rs

-use engines::Engine;
+use block::*;
+use util::*;
+use engines::{Engine, CloseOutcome};


Better not to use glob imports where avoidable

rphmeier · 2017-07-31T13:12:04Z

ethcore/src/ethereum/ethash.rs

 				fields.state.add_balance(
 					u.author(),
-					&(reward * U256::from(8 + u.number() - current_number) / U256::from(8)),
+					&(result_uncle_reward),


parentheses no longer necessary

grbIzl · 2017-08-01T09:55:25Z

Add relnotes label in order to reflect, that transaction id and hash for trace can be null after this change

gavofyork · 2017-08-09T16:48:17Z

ethcore/src/block.rs

@@ -419,7 +423,7 @@ impl<'x> OpenBlock<'x> {
 		let mut s = self;

 		if let Err(e) = s.engine.on_close_block(&mut s.block) {
-			warn!("Encountered error on closing the block: {}", e);
+ 			warn!("Encountered error on closing the block: {}", e);


dodgy space added (beginning of line)

gavofyork · 2017-08-09T16:48:46Z

ethcore/src/block.rs

@@ -429,7 +433,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.uncles_hash() == &SHA3_EMPTY_LIST_RLP {


any particular reason for this change?

Yes. From my point of view it's a logical flaw. If block is initialized using default construction, uncle_hash won't be zero and won't be set in this method. I assume, that original intent was to set it in this case. You can see the same pattern (checking default values and zero) in this function for other fields. As a result simple integration test with close_and_seal for block failed.

ok that's fair enough.

gavofyork · 2017-08-09T16:49:06Z

ethcore/src/engines/mod.rs

@@ -229,7 +229,7 @@ pub trait Engine : Sync + Send {

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


dodgy space added

gavofyork · 2017-08-09T16:50:26Z

ethcore/src/ethereum/ethash.rs

@@ -24,6 +24,7 @@ use block::*;
 use builtin::Builtin;
 use vm::EnvInfo;
 use error::{BlockError, Error, TransactionError};
+use trace::{Tracer, ExecutiveTracer,RewardType};


inconsistent spaces.

gavofyork · 2017-08-09T16:52:58Z

ethcore/src/ethereum/ethash.rs

 					CleanupMode::NoEmpty
-				)
+				)	


trailing whitespace

gavofyork · 2017-08-09T16:57:55Z

ethcore/src/spec/spec.rs

@@ -170,7 +170,7 @@ impl From<ethjson::spec::Params> 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),
+			registrar: p.registrar.map_or_else(Address::new, Into::into)


don't kill the trailing ,

gavofyork · 2017-08-09T16:58:25Z

ethcore/src/tests/trace.rs

+
+	// Add root block first
+	let mut root_block = OpenBlock::new(
+			engine,


only one indentation needed

gavofyork · 2017-08-09T16:58:41Z

ethcore/src/tests/trace.rs

+
+	// Add parent block
+	let mut parent_block = OpenBlock::new(
+			engine,


only one indentation needed

gavofyork · 2017-08-09T16:58:58Z

ethcore/src/tests/trace.rs

+		(3141562.into(), 31415620.into()), 
+		vec![],
+		false
+		).unwrap();


should be one indent fewer

gavofyork · 2017-08-09T16:59:14Z

ethcore/src/tests/trace.rs

+
+	// Test0. Check overall filter
+	let filter = TraceFilter {
+			range: (BlockId::Number(1)..BlockId::Number(3)),


should be one indentation fewer

gavofyork · 2017-08-09T17:01:22Z

ethcore/src/trace/types/filter.rs

@@ -128,6 +128,10 @@ impl Filter {
 				let from_matches = self.from_address.matches(&suicide.address);
 				let to_matches = self.to_address.matches(&suicide.refund_address);
 				from_matches && to_matches
+			},
+			Action::Reward(ref reward) => {
+				let to_matches = self.to_address.matches(&reward.author);


what's the point of let X = ...; X ? why not just ...?

gavofyork · 2017-08-09T17:01:42Z

ethcore/src/trace/types/flat.rs

 	use evm::CallType;
+    use trace::RewardType;


tabs! not spaces.

gavofyork · 2017-08-09T17:05:38Z

more just did a style review. quite a lot of issues. some were probably just misconfigured editor, others misunderstanding about our house style.

one or two, however, were carelessness, which is a bad omen given this is utterly consensus-critical code that you're hacking on.

the logic here should be positively and deeply reviewed by at least three experienced core devs before merge.

grbIzl · 2017-08-09T17:55:25Z

@gavofyork I provided the reasoning in the question about logic's change. All other style comments will be fixed with the next commit.

debris · 2017-08-30T12:26:51Z

I will fix build failures and merge to master

grbIzl · 2017-08-30T12:30:15Z

Thanks @debris !

debris · 2017-08-30T12:37:53Z

Done. I'll merge once CI is green 👍

grbIzl added 9 commits July 27, 2017 18:19

Tracing for rewards added. Without tests for now

ab61538

Warnings removed

16f3948

Working test with block reward added

7312803

Complete version of tracing test with reward

53c3b77

Unit tests for tracing reward added

af1fbb3

Fixed errors after merge with master

b193534

Transaction ids made optional in order to reflect not transactional t…

1f3f911

…races

Miner field renamed to author

94efa3a

Tabs corrected

bb043ba

grbIzl added the A0-pleasereview 🤓 Pull request needs code review. label Jul 31, 2017

NikVolf added the M4-core ⛓ Core client code / Rust. label Jul 31, 2017

grbIzl added 10 commits July 31, 2017 13:40

Tracing for rewards added. Without tests for now

06862c7

Warnings removed

204a63a

Working test with block reward added

5086dc3

Complete version of tracing test with reward

3fdb912

Unit tests for tracing reward added

f2d12ae

Fixed errors after merge with master

45043c3

Transaction ids made optional in order to reflect not transactional t…

1a3f3ff

…races

Miner field renamed to author

9374e31

Tabs corrected

e809582

Merge with head

8826fdb

keorn reviewed Jul 31, 2017

View reviewed changes

rphmeier reviewed Jul 31, 2017

View reviewed changes

Fixed comments after review and test after rebase

141c2fd

grbIzl added the B7-releasenotes 📜 Changes should be mentioned in the release notes of the next minor version release. label Aug 1, 2017

tomusdrw changed the title ~~Fix 4858~~ Tracing of rewards on closing blocks Aug 2, 2017

gavofyork reviewed Aug 9, 2017

View reviewed changes

ethcore/src/ethereum/ethash.rs Outdated

CleanupMode::NoEmpty

)

)

Copy link

Contributor

gavofyork Aug 9, 2017

Choose a reason for hiding this comment

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

trailing whitespace

gavofyork reviewed Aug 9, 2017

View reviewed changes

ethcore/src/tests/trace.rs

(3141562.into(), 31415620.into()),

vec![],

false

).unwrap();

Copy link

Contributor

gavofyork Aug 9, 2017

Choose a reason for hiding this comment

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

should be one indent fewer

gavofyork reviewed Aug 9, 2017

View reviewed changes

ethcore/src/trace/types/flat.rs Outdated

use evm::CallType;

use trace::RewardType;

Copy link

Contributor

gavofyork Aug 9, 2017

Choose a reason for hiding this comment

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

tabs! not spaces.

gavofyork added A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. and removed A8-looksgood 🦄 Pull request is reviewed well. labels Aug 9, 2017

Style fixed after review

bfd238e

arkpar added A0-pleasereview 🤓 Pull request needs code review. and removed A5-grumble 🔥 Pull request has minor issues that must be addressed before merging. labels Aug 28, 2017

gavofyork added 2 commits August 29, 2017 15:11

Merge branch 'master' into Fix-4858

50495c6

Consistent use of ,s

e366645

gavofyork added A8-looksgood 🦄 Pull request is reviewed well. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 29, 2017

debris added 2 commits August 30, 2017 14:30

Merge branch 'master' into Fix-4858

88200a1

fixed merge

8a420d6

debris merged commit f7e15f2 into master Aug 30, 2017

debris deleted the Fix-4858 branch August 30, 2017 13:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rewards on closing blocks #6194

Rewards on closing blocks #6194

grbIzl commented Jul 31, 2017

NikVolf commented Jul 31, 2017

keorn Jul 31, 2017

rphmeier Jul 31, 2017

rphmeier Jul 31, 2017

rphmeier Jul 31, 2017

grbIzl commented Aug 1, 2017

gavofyork Aug 9, 2017

gavofyork Aug 9, 2017

grbIzl Aug 9, 2017

gavofyork Aug 10, 2017

gavofyork Aug 9, 2017

gavofyork Aug 9, 2017

gavofyork Aug 9, 2017

gavofyork Aug 9, 2017

gavofyork Aug 9, 2017

gavofyork Aug 9, 2017

gavofyork Aug 9, 2017

gavofyork Aug 9, 2017

gavofyork Aug 9, 2017

gavofyork Aug 9, 2017

gavofyork commented Aug 9, 2017

grbIzl commented Aug 9, 2017

debris commented Aug 30, 2017

grbIzl commented Aug 30, 2017

debris commented Aug 30, 2017

Rewards on closing blocks #6194

Rewards on closing blocks #6194

Conversation

grbIzl commented Jul 31, 2017

NikVolf commented Jul 31, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

grbIzl commented Aug 1, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gavofyork commented Aug 9, 2017

grbIzl commented Aug 9, 2017

debris commented Aug 30, 2017

grbIzl commented Aug 30, 2017

debris commented Aug 30, 2017