Add block reward contract config to ethash and allow off-chain contracts #9312

sorpaas · 2018-08-08T12:02:36Z

This adds block reward contract config to ethash. A new config blockRewardContractCode is also added to both Aura and ethash. When specified, it will execute the code directly and overrides any blockRewardContractAddress config. Having this blockRewardContractCode config allows chains to deploy hard fork by simply replacing the current config value, without the need from us to support any multi block reward scheme.

One interface issue I faced was RewardKind::Uncle. Most ethash uncles reward are calculated according to its depth, but we didn't have that information. So I added RewardKind::UncleWithDepth(u8) for this purpose. To keep backward compatibility, removing RewardKind::Uncle might not be an option. UncleWithDepth uses the reward kind index from 100 to 355, where 101 represents uncle with depth 1, and 102 represents uncle with depth 2, etc. Technically, we only use 101-106 right now, and all others are invalid. cc @andresilva. Not sure whether there're better ways to do this.

cc @eosclassicteam (#9163) @yograterol (#9283). If you could rewrite your PR on_close_block using a reward contract, that would certainly speed up merging it! Examples of block reward contracts can be found here https://github.com/parity-contracts/block-reward/tree/master/contracts

phahulin · 2018-08-18T19:44:32Z

Hi @sorpaas !
could you please explain in a bit more details how will blockRewardContractCode work, especially in AuRa chains.
As I understand, new parameter blockRewardContractCode will be part of genesis.json chain spec file, so it will go somewhere in "engine" > "authorityRound" > "params" and contain the bytecode to be executed every round, correct? Will it support the same ABI as contract at blockRewardContractAddress?

I don't understand, how will it help to avoid use of "mutil" option. If blockRewardContractCode is part of genesis and we issue a fork, replacing bytecode in genesis with a new version, won't it make all previous blocks invalid?

Thanks!

sorpaas · 2018-08-21T02:59:18Z

@phahulin So when deploying a hard fork, you just replace blockRewardContractCode with the new one. Previously you may have logic like:

Set block reward to 5 ether.

For the new one, you add if statement to check the block number:

If block number is less than FORK_BLOCK,
  Set block reward to 5 ether.
Otherwise,
  Set block reward to 3 ether.

Note that this parameter is entirely optional, and blockRewardContractCode is never written to state (nor it will appear anywhere in genesis block).

dvdplm

Code LGTM, logic also (but I'm not familiar so don't take my word for it).
Added a few questions and suggestions.

dvdplm · 2018-08-21T06:19:14Z

ethcore/src/engines/authority_round/mod.rs

 					result.map_err(|e| format!("{}", e))
 				};

-				let rewards = c.reward(&benefactors, &mut call)?;
+				let rewards = c.reward(&beneficiaries, &mut call)?;


+1 at this.

I think the wiki also uses "benefactors" instead of "beneficiaries" which was very confusing to me when I read it.

dvdplm · 2018-08-21T06:23:07Z

ethcore/src/engines/authority_round/mod.rs

-					let result = self.machine.execute_as_system(
-						block,
-						to,
-						U256::max_value(), // unbounded gas? maybe make configurable.


Is this comment not relevant anymore?

I think the original comment didn't make sense in this case (I copied it from some other usage of execute_code_as_system 😛). It probably makes sense to execute the block reward contract with unbounded gas. Otherwise, if we cap it and it goes out of gas what do we default to? No rewards for that block? The block reward contract is a critical part of consensus so I think the framework should assume it is correct, in this case we should assume that it will terminate, if it doesn't your chain is broken.

dvdplm · 2018-08-21T06:46:11Z

ethcore/src/engines/block_reward.rs

@@ -139,7 +163,7 @@ pub fn apply_block_rewards<M: Machine + WithBalances + WithRewards>(
 	}

 	let rewards: Vec<_> = rewards.into_iter().map(|&(a, k, r)| (a, k.into(), r)).collect();


This line is a little awkward. It would be nice to do this work only if tracing is enabled. I'm not sure what the reason is for having both RewardKind and RewardType – is it possible to unify them? That way this line could go away entirely I think?

It's probably possible to unify them, although now they're a bit different since the tracing type doesn't distinguish between uncle depth. Although maybe we can change note_rewards to take RewardKind and handle the conversion itself (only performing it if tracing is enabled)?

I created a new issue on this #9399

(I would hope that we get the above done after #9360, otherwise it'll be a lot of duplicate interface changes.)

dvdplm · 2018-08-21T06:53:33Z

ethcore/src/ethereum/ethash.rs

@@ -531,6 +586,8 @@ mod tests {
 			eip649_reward: None,
 			expip2_transition: u64::max_value(),
 			expip2_duration_limit: 30,
+			block_reward_contract: None,
+			block_reward_contract_transition: 0,
 		}
 	}


This is a pretty big function. Perhaps it's time to split it up a little?

Not sure how we can split it though. I would love some ideas. The issue is just that ethash engine is full of configs/patches and all kinds of different hard forks.

dvdplm · 2018-08-21T06:59:11Z

json/src/spec/authority_round.rs

@@ -16,8 +16,9 @@

 //! Authority params deserialization.

-use ethereum_types::Address;
+use hash::Address;


Why this rename?

Previously it was not using the right type -- in json, we should use the json serialization/serialization type ($crate::hash::Address) rather than from ethereum_types.

andresilva

Overall LGTM, some minor comments. We should also update the wiki on this.

andresilva · 2018-08-21T14:32:41Z

ethcore/src/engines/authority_round/mod.rs

-					let result = self.machine.execute_as_system(
-						block,
-						to,
-						U256::max_value(), // unbounded gas? maybe make configurable.


I think the original comment didn't make sense in this case (I copied it from some other usage of execute_code_as_system 😛). It probably makes sense to execute the block reward contract with unbounded gas. Otherwise, if we cap it and it goes out of gas what do we default to? No rewards for that block? The block reward contract is a critical part of consensus so I think the framework should assume it is correct, in this case we should assume that it will terminate, if it doesn't your chain is broken.

andresilva · 2018-08-21T14:54:02Z

ethcore/src/engines/block_reward.rs

@@ -139,7 +163,7 @@ pub fn apply_block_rewards<M: Machine + WithBalances + WithRewards>(
 	}

 	let rewards: Vec<_> = rewards.into_iter().map(|&(a, k, r)| (a, k.into(), r)).collect();


It's probably possible to unify them, although now they're a bit different since the tracing type doesn't distinguish between uncle depth. Although maybe we can change note_rewards to take RewardKind and handle the conversion itself (only performing it if tracing is enabled)?

andresilva · 2018-08-21T14:59:06Z

ethcore/src/machine.rs

-		if let Err(e) = ex.call(params, &mut substate, BytesRef::Flexible(&mut output), &mut NoopTracer, &mut NoopVMTracer) {
-			warn!("Encountered error on making system call: {}", e);
-		}
+		ex.call(params, &mut substate, BytesRef::Flexible(&mut output), &mut NoopTracer, &mut NoopVMTracer).map_err(|e| ::engines::EngineError::FailedSystemCall(format!("{}", e)))?;


Should we still keep the warning? Code that's executed as system is usually consensus critical so it's probably helpful to have failures logged (helps with debugging issues like borked contracts).

After #9085, we can make this call error directly throw, and miner/validator would handle emitting the warnings. :)

andresilva · 2018-08-21T15:09:35Z

ethcore/src/engines/mod.rs

@@ -133,6 +133,18 @@ pub enum Seal {
 /// A system-calling closure. Enacts calls on a block's state from the system address.
 pub type SystemCall<'a> = FnMut(Address, Vec<u8>) -> Result<Vec<u8>, String> + 'a;


Do you think it makes sense to remove this type? It seems like everywhere SystemCall is supported SystemOrCodeCall should also be? (even if it's not exposed by any parameter).

I think we are still using SystemCall in several other cases, but I do agree that we may eventually want to clean up this.

andresilva · 2018-08-21T15:10:29Z

ethcore/src/ethereum/ethash.rs

-			rewards.push((author, RewardKind::Author, result_block_reward));
-			rewards.push((ubi_contract, RewardKind::External, ubi_reward));
-			rewards.push((dev_contract, RewardKind::External, dev_reward));
+				let mut call = |to, data| {


We can probably refactor this to share the code with AuRa's on_close_block.

andresilva · 2018-08-21T15:18:24Z

ethcore/src/engines/block_reward.rs

+	External,
+
+	/// Reward attributed to the block uncle(s) without any reference of block difference. This is used by all except Ethash engine's reward contract.
+	Uncle,


I think it's safe to remove this since the block reward contract was only supported in Aura and it doesn't produce uncles, so I don't think anyone is relying on this. Even if their contract code did handle the uncle type, it would never be reached.

andresilva · 2018-08-21T15:22:43Z

After this is merged it would also be nice to be able to extract the musicoin block reward logic to a contract (which maybe can be used as an example for other people).

ghost · 2018-08-22T06:59:13Z

@andresilva @sorpaas ACK on this. we will move our logic if there is an example for musicoin

varasev · 2018-08-22T07:09:12Z

Hi @sorpaas,

Please clarify the next thing: will it work fine if we use blockRewardContractAddress and blockRewardContractTransition and then we switch to blockRewardContractCode after some time (and after blockRewardContractTransition block is created)?

…-block-reward

sorpaas · 2018-08-22T11:04:17Z

@varasev Yes it will work fine. In that case, you would want to keep blockRewardContractTransition to be the original value, and in the new contract defined by blockRewardContractCode, keep the behavior of reward contracts for old blocks unchanged.

varasev · 2018-08-22T12:35:33Z

@sorpaas Thank you for the clarification. What if the new contract code in blockRewardContractCode differs from that old contract defined in blockRewardContractAddress? Would it be possible?

sorpaas · 2018-08-22T12:46:22Z

@varasev Yes! As long as it evaluates to the same reward results for pre-fork blocks.

andresilva

LGTM! Wiki https://wiki.parity.io/Block-Reward-Contract posting it here so we don't forget to update it before we release this feature.

* master: evmbin: Fix gas_used issue in state root mismatch and handle output better (#9418) Update hardcoded sync (#9421) Add block reward contract config to ethash and allow off-chain contracts (#9312) Private packets verification and queue refactoring (#8715) Update tobalaba.json (#9419) docs: add parity ethereum logo to readme (#9415) build: update rocksdb crate (#9414) Updating the CI system (#8765) Better support for eth_getLogs in light mode (#9186) Add update docs script to CI (#9219) `gasleft` extern implemented for WASM runtime (kip-6) (#9357) block view! removal in progress (#9397) Prevent sync restart if import queue full (#9381) nonroot CentOS Docker image (#9280) ethcore: kovan: delay activation of strict score validation (#9406)

sorpaas added 6 commits August 8, 2018 18:36

Add execute_code_as_system and throw error if system call fails

cf6135b

Add SystemOrCodeCallKind and change block reward contract to use it

88d1eae

Add block_reward_contract_code param

d09d031

Add reward contract support for ethash

6c4d2c4

Fix json crate compile

6c03980

Be more graceful on uncle with depth calculation

5c379d4

sorpaas added A0-pleasereview 🤓 Pull request needs code review. M4-core ⛓ Core client code / Rust. labels Aug 8, 2018

sorpaas added this to the 2.1 milestone Aug 8, 2018

sorpaas requested a review from andresilva August 8, 2018 12:02

benefactors -> beneficiaries

0ea2236

dvdplm approved these changes Aug 21, 2018

View reviewed changes

andresilva reviewed Aug 21, 2018

View reviewed changes

sorpaas added A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. and removed A0-pleasereview 🤓 Pull request needs code review. labels Aug 22, 2018

sorpaas added 3 commits August 22, 2018 18:10

Merge branch 'master' of https://github.com/paritytech/parity into sp…

ac0d3e1

…-block-reward

Fix linting

56a409c

Refactor duplicate system_or_code_call

0dd53bf

sorpaas mentioned this pull request Aug 22, 2018

Convert rewards to tracing struct only when tracing is enabled #9399

Closed

Remove old RewardKind::Uncle

c494b79

sorpaas added A0-pleasereview 🤓 Pull request needs code review. and removed A3-inprogress ⏳ Pull request is in progress. No review needed at this stage. labels Aug 22, 2018

andresilva approved these changes Aug 29, 2018

View reviewed changes

andresilva merged commit 74ce0f7 into master Aug 29, 2018

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

sorpaas mentioned this pull request Aug 29, 2018

Add Callisto Mainnet and Testnet #9283

Closed

5chdn deleted the sp-block-reward branch August 30, 2018 19:02

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add block reward contract config to ethash and allow off-chain contracts #9312

Add block reward contract config to ethash and allow off-chain contracts #9312

sorpaas commented Aug 8, 2018

phahulin commented Aug 18, 2018

sorpaas commented Aug 21, 2018

dvdplm left a comment

dvdplm Aug 21, 2018

dvdplm Aug 21, 2018

andresilva Aug 21, 2018

dvdplm Aug 21, 2018

andresilva Aug 21, 2018

sorpaas Aug 22, 2018 •

edited

dvdplm Aug 21, 2018

sorpaas Aug 22, 2018

dvdplm Aug 21, 2018

sorpaas Aug 22, 2018

andresilva left a comment

andresilva Aug 21, 2018

andresilva Aug 21, 2018

andresilva Aug 21, 2018

sorpaas Aug 22, 2018

andresilva Aug 21, 2018

sorpaas Aug 22, 2018

andresilva Aug 21, 2018

andresilva Aug 21, 2018

andresilva commented Aug 21, 2018

ghost commented Aug 22, 2018

varasev commented Aug 22, 2018

sorpaas commented Aug 22, 2018

varasev commented Aug 22, 2018

sorpaas commented Aug 22, 2018

andresilva left a comment

		@@ -139,7 +163,7 @@ pub fn apply_block_rewards<M: Machine + WithBalances + WithRewards>(
		}

		let rewards: Vec<_> = rewards.into_iter().map(\|&(a, k, r)\| (a, k.into(), r)).collect();

		@@ -133,6 +133,18 @@ pub enum Seal {
		/// A system-calling closure. Enacts calls on a block's state from the system address.
		pub type SystemCall<'a> = FnMut(Address, Vec<u8>) -> Result<Vec<u8>, String> + 'a;

Add block reward contract config to ethash and allow off-chain contracts #9312

Add block reward contract config to ethash and allow off-chain contracts #9312

Conversation

sorpaas commented Aug 8, 2018

phahulin commented Aug 18, 2018

sorpaas commented Aug 21, 2018

dvdplm left a 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

sorpaas Aug 22, 2018 • edited

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

andresilva left a 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

andresilva commented Aug 21, 2018

ghost commented Aug 22, 2018

varasev commented Aug 22, 2018

sorpaas commented Aug 22, 2018

varasev commented Aug 22, 2018

sorpaas commented Aug 22, 2018

andresilva left a comment

Choose a reason for hiding this comment

sorpaas Aug 22, 2018 •

edited