Add randomness contract support to AuthorityRound. #10946

afck · 2019-08-06T13:25:54Z

This allows AuthorityRound validators to participate in a RANDAO-like RNG smart contract. We implemented it as part of POSDAO, but it would work as a standalone feature to provide on-chain randomness, too.

See the description below: #10946 (comment)

Original implementation by @mbr: poanetwork#52

I'm not really happy with the fact that we can only call e.g. isCommitPhase on the "latest" block, i.e. the parent of the one we're currently creating, and we're including our transactions in. This means authors of randomness contracts need to check in commitHash whether _isCommitPhase(block.number - 1).
For a different feature, we actually added a Client::call_contract_before method that allows you to make calls at the current pending block: https://github.com/poanetwork/parity-ethereum/pull/140/files#diff-40c7c65265e511d07158f62712f69573R1460
I.e. the call is made at the initial state of the pending block, but with the correct new block number. Do you think it would make sense to add that to Client and use it here instead?

Another issue is that without --force-sealing, we're not producing new blocks, even if it would be our turn to send a value to the randomness contract. I'm not sure where the right place in the code is to put a warning in that case; or where I'd even have access to both force_sealing and randomness_contract_address?

Is there any place where I should add documentation for the contract API?

Please let me know your thoughts!

parity-cla-bot · 2019-08-06T13:25:58Z

It looks like @afck signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

afck · 2019-08-27T19:01:23Z

~~I can't reproduce that failure locally:~~ (fixed)

$ cargo test should_check_status_of_request_when_its_resolved
    Finished dev [unoptimized + debuginfo] target(s) in 0.21s
     Running /home/andreas/git/parity-ethereum/target/debug/deps/parity_rpc-36b54cdf58daa398

running 1 test
test v1::tests::mocked::signing::should_check_status_of_request_when_its_resolved ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured; 297 filtered out

afck · 2019-10-17T10:28:24Z

Hi! I'm still interested in merging this, and happy address any concerns about the implementation.

ethcore/res/contracts/authority_round_random.json

ethcore/engines/authority-round/src/lib.rs

afck · 2019-11-27T11:40:39Z

Tabs and spaces are mixed here ☝️

~~Where? I can't find it.~~
(Ah, sorry, you did point out which line. Fixing it now.)

@mbr

Changes have been cherry-picked from poanetwork's aura-pos branch. Most of the work has been done by @mbr.

Co-Authored-By: David <[email protected]>

ordian · 2019-12-03T15:15:27Z

ethcore/engines/authority-round/src/lib.rs

+		// Creates and signs a transaction with the given contract call.
+		let mut make_transaction = |to: Address, data: Bytes| -> Result<SignedTransaction, Error> {
+			let nonce = Some(tx_nonce);
+			tx_nonce += U256::one(); // Increment the nonce for the next transaction.


is it okay to panic here on overflow? or is it not feasible to get an overflow here?

Is there a way to artificially increase account nonces somehow?
Otherwise I'd say it's not feasible: You'd have to execute 2^256 transactions to get there.
Not sure whether the yellow paper even defines what should happen in that case? At a glance, I couldn't find it.

dvdplm

LGTM

dvdplm · 2019-12-05T09:01:02Z

ethcore/engines/authority-round/src/lib.rs

@@ -1430,6 +1452,46 @@ impl Engine for AuthorityRound {
 		block_reward::apply_block_rewards(&rewards, block, &self.machine)
 	}

+	/// Make calls to the randomness contract.
+	fn on_prepare_block(&self, block: &ExecutedBlock) -> Result<Vec<SignedTransaction>, Error> {


Is it still guaranteed at this point that the client's latest nonce is the same, i.e. that the latest block as seen by the client is the parent of this block?

I honestly do not know. @tomusdrw maybe?

dvdplm · 2019-12-05T09:04:39Z

ethcore/engines/authority-round/src/randomness.rs

+	/// Waiting for the next phase.
+	///
+	/// This state indicates either the successful revelation in this round or having missed the
+	/// window to make a commitment.


Maybe dumb q: what is the window? A range of blocks? Where do we set this? In the aura_random contract? (If yes, I think it's a good idea to add notes for any piece of data that is defined in the contract as opposed to the rust code).

That's up to the contract. In POSDAO it's a range of blocks, yes.
I'll add a comment.

The comment was added in poanetwork@4799aeb.

dvdplm · 2019-12-05T09:08:32Z

ethcore/engines/authority-round/src/randomness.rs

+	///
+	/// Returns the encoded contract call necessary to advance the randomness contract's state.
+	///
+	/// **Warning**: The `advance()` function should be called only once per block state; otherwise


Not sure what you mean by "block state"? Can you elaborate?

I think the intent of this warning was to make sure we don't call advance() twice in the same block. I'll reword it.

ethcore/engines/authority-round/src/util.rs

dvdplm · 2019-12-05T09:12:30Z

ethcore/engines/authority-round/src/util.rs

+
+	/// Perform a function call to an ethereum machine that doesn't create a transaction or change the state.
+	///
+	/// Runs a constant function call on `client`. The `call` value can be serialized by calling any


I think I've asked this somewhere else already but I do not remember the answer, apologies. What is meant by a "constant function" in this context? Is it "constant" because it does not create txs and thus does not alter state? If yes, I suggest being explicit: "Runs a non-state altering function call…" (or something to that effect).

Yes, exactly. I'll expand the comment.

The comment was expanded in poanetwork@4799aeb#diff-a4faad7e2fe2f519b612d48dfa0f8607.

dvdplm · 2019-12-05T09:24:35Z

json/src/spec/authority_round.rs

+#[derive(Debug, PartialEq, Deserialize)]
+#[serde(deny_unknown_fields)]
+#[serde(untagged)]
+pub enum TransitionMap<T> {


I agree that it'd be nice to let knowledgeable users to have a short-form for the single-item use case. However I also think that it's worth more to have less rust code to maintain: configuration mistakes are non-catastrophic (the node won't start; bugs in rust might be more subtle) and probably easy to mitigate with docs. So yeah, unless you disagree strongly I think it's good to mandate a map here.

stale

afck · 2019-12-05T10:49:59Z

I addressed most comments, except the error-related ones (also #10946 (comment) — see my reply; happy to work on this once we make a decision).

tomusdrw

Looks good overall, but I suggested couple of renames and we need to address the sealing.lock() being held during the entire time when we push transactions to the block.

tomusdrw · 2019-10-22T08:07:57Z

ethcore/client-traits/src/lib.rs

+		self.transact(Action::Call(address), data, None, None, None)
+	}
+
+	/// Returns a signed transaction. If gas limit, gas price, or nonce are not


Signed by what key?

tomusdrw · 2019-10-22T08:09:27Z

ethcore/client-traits/src/lib.rs

+	}
+
+	/// Returns a signed transaction. If gas limit, gas price, or nonce are not
+	/// specified, the defaults are used.


the defaults are used.

I like the comment from transact better, cause you really can't have a default nonce - it's a bit nonsense.

tomusdrw · 2019-10-22T08:10:30Z

ethcore/client-traits/src/lib.rs

+		&self,
+		action: Action,
+		data: Bytes,
+		gas: Option<U256>,


I was expressing my need for a builder in such long calls in some other PR already :) I really think it would make the callers life easier and the code more readable, especially if we are passing 3 Nones.

Something like:

client.create_transaction(TransactionBuilder::call(address).gas(5).nonce(4).build())

Then the caller would configure only the stuff they really need and we could simplify the interface to:

fn create_transaction(&self, TransactionRequest) -> Result<SignedTransaction>; fn submit_transaction(&self, SignedTransaction) -> Result<()>; fn transact(&self, r: TransactionRequest) -> Result<()> { self.submit_transaction(self.create_transaction(r)?) }

instead of having a random transact_contract.

Done…
I didn't separate builder from request, though; let me know if you're okay with that approach.

tomusdrw · 2019-12-11T12:23:22Z

ethcore/engines/authority-round/src/lib.rs

 	snapshot::Snapshotting,
+    transaction::{Action, SignedTransaction},


funky indentation

Fixed. (Sorry, somehow sometimes my Vim doesn't realize it's tabs.)

tomusdrw · 2019-12-11T12:45:30Z

ethcore/engines/authority-round/src/lib.rs

+			EngineError::RequiresClient
+		})?;
+		let full_client = client.as_full_client()
+			.ok_or(EngineError::FailedSystemCall("Failed to upgrade to BlockchainClient.".to_string()))?;


Suggested change

.ok_or(EngineError::FailedSystemCall("Failed to upgrade to BlockchainClient.".to_string()))?;

.ok_or_else(|| EngineError::FailedSystemCall("Failed to upgrade to BlockchainClient.".to_string()))?;

tomusdrw · 2019-12-11T13:20:49Z

ethcore/src/miner/miner.rs

+	/// Creates new instance of miner with given spec and accounts.
+	///
+	/// NOTE This should be only used for tests.
+	pub fn new_for_tests_force_sealing(spec: &Spec, accounts: Option<HashSet<Address>>, force_sealing: bool) -> Miner {


I would rather keep only one function that is exposed for tests (especially that it's not feature-flagged). Is this new function really necessary? Can't you enable force_sealing for all tests?

There seem to be two tests relying on it:

failures: miner::miner::tests::should_not_mine_if_internal_sealing_is_disabled miner::miner::tests::should_not_mine_if_no_fetch_work_request

We can add the parameter to new_for_tests, but that will touch a lot of files.

Okay, would be best to have some way for them to alter the options, without introducing those additional methods, but I'm fine as-is.

tomusdrw · 2019-12-11T13:38:55Z

ethcore/engines/authority-round/src/util.rs

+
+impl<'a> BoundContract<'a> {
+	/// Create a new `BoundContract`.
+	#[inline]


Suggested change

#[inline]

Should be left to compiler to decide that.

tomusdrw · 2019-12-11T13:48:12Z

ethcore/engines/authority-round/src/randomness.rs

+				let number: RandNumber = rng.gen();
+				let number_hash: Hash = keccak(number.as_bytes());
+				let public = signer.public().ok_or(PhaseError::MissingPublicKey)?;
+				let cipher = ecies::encrypt(&public, &number_hash.0, number.as_bytes())?;


Are there any other reasons to store the encrypted number on-chain other than avoiding validator nodes being stateful (i.e. having to store it off-chain to reveal in the future)?

We think it would be more reliable to store the cipher on-chain, and, since we call the commitHash function anyway, we pass the cipher to it at the same time. So storing it on-chain doesn't require making a separate transaction.

Also, if your node crashes somehow and you start another one, it can just continue, and won't fail to reveal the secret.
If we stored that locally, the backup node wouldn't have the required information.

Would be good to check with some cryptographer if we don't leak some information everytime we encode the same number. I think it's unlikely, but still.

You mean because we publish multiple pairs of plaintext + ciphertext for the same key?
Since ECIES is supposed to be secure even against chosen-plaintext and chosen-ciphertext attacks, I believe that shouldn't be a problem. But I'm not a cryptographer either.

tomusdrw · 2019-12-11T14:13:18Z

ethcore/engines/authority-round/src/lib.rs

@@ -1430,6 +1452,46 @@ impl Engine for AuthorityRound {
 		block_reward::apply_block_rewards(&rewards, block, &self.machine)
 	}

+	/// Make calls to the randomness contract.
+	fn on_prepare_block(&self, block: &ExecutedBlock) -> Result<Vec<SignedTransaction>, Error> {


Yes, the nonce should be the same since we are only calling this function before any transaction is applied. I think we should actually consider renaming this function to fn generate_engine_transactions(&self, block: &ExecutedBlock) -> Result<Vec<_>> and document it properly. Having on_prepare_block return a list of transactions doesn't seem intuitive for me.

tomusdrw · 2019-12-11T14:20:35Z

ethcore/types/src/errors/engine_error.rs

@@ -35,8 +35,16 @@ pub enum EngineError {
 	BadSealFieldSize(OutOfBounds<usize>),
 	/// Validation proof insufficient.
 	InsufficientProof(String),
+	/// Randomness error in load method
+	RandomnessLoadError(String),


Seems that the AuRa internal randomness contract is leaking to a generic EngineError type, I think we should rather keep these under some more generic variant, especially that we never really handle them explicitly.

Yes, I'm really unsure about how we should organize these errors (also see #10946 (comment)).

Should we call this Other(String) then? Or Other<Box<dyn Error>>?

Yeah, either of those. I think I'm more in favor of simpler Other(String). Ideally would be if Engine had an associated type of it's custom error type, but that's way beyond the scope of this PR.

@dvdplm are you fine with Other(String) or do you like Other<Box<dyn Error>> more?

Actually, there's already a Custom(String).

Let's go with the existing Custom(String). Not ideal, but I agree with Tomek here: improving the error handling here is non-trivial and not in scope for this PR.

Great! I used Custom in ab169e2.

tomusdrw

LGTM! Tiny thing left (remove transact_contract), but I'm good to merge as is.

tomusdrw · 2019-12-13T13:08:41Z

ethcore/client-traits/src/lib.rs

@@ -397,30 +397,69 @@ pub trait BlockChainClient:

 	/// Schedule state-altering transaction to be executed on the next pending block.
 	fn transact_contract(&self, address: Address, data: Bytes) -> Result<(), transaction::Error> {


I'd remove that method now, it's really easy to use transact instead.

I removed it. 👍

Perfect, thanks!

* master: Add Nat PMP method to P2P module (#11210) Add randomness contract support to AuthorityRound. (#10946) ethcore/res: activate ecip-1061 on kotti and mordor (#11338)

jam10o-new added A0-pleasereview 🤓 Pull request needs code review. F8-enhancement 🎊 An additional feature request. M4-core ⛓ Core client code / Rust. labels Aug 6, 2019

jam10o-new added this to the 2.7 milestone Aug 6, 2019

afck force-pushed the afck-rand branch 2 times, most recently from 73fca41 to 98c6827 Compare August 13, 2019 12:30

afck force-pushed the afck-rand branch 2 times, most recently from 64ae88d to cfad2b7 Compare August 22, 2019 07:33

afck force-pushed the afck-rand branch 3 times, most recently from d1af62f to 565c31e Compare August 27, 2019 14:57

afck force-pushed the afck-rand branch from 565c31e to 0218adc Compare August 29, 2019 09:20

afck force-pushed the afck-rand branch 3 times, most recently from b93995f to fd89a98 Compare September 17, 2019 08:24

afck force-pushed the afck-rand branch 2 times, most recently from ed2b734 to 3e42c94 Compare September 30, 2019 08:20

afck force-pushed the afck-rand branch 2 times, most recently from e7d4815 to 11efb01 Compare October 10, 2019 08:32

dvdplm requested review from tomusdrw and ordian October 17, 2019 11:26

afck force-pushed the afck-rand branch 3 times, most recently from 8998882 to 245d71c Compare October 29, 2019 09:30

afck mentioned this pull request Oct 30, 2019

Add POSDAO transition; call emitInitiateChange. #11220

Closed

varasev reviewed Oct 30, 2019

View reviewed changes

ethcore/res/contracts/authority_round_random.json Outdated Show resolved Hide resolved

varasev reviewed Oct 30, 2019

View reviewed changes

ethcore/engines/authority-round/src/lib.rs Outdated Show resolved Hide resolved

afck force-pushed the afck-rand branch from 7340833 to e69adec Compare November 22, 2019 16:00

afck force-pushed the afck-rand branch from 7cb5843 to a5791db Compare November 27, 2019 12:09

afck and others added 7 commits December 3, 2019 15:00

Add randomness contract support to Authority Round.

6961143

Changes have been cherry-picked from poanetwork's aura-pos branch. Most of the work has been done by @mbr.

Address review comments for randomness contract.

7b997c7

Co-Authored-By: David <[email protected]>

Rename revealSecret to revealNumber

620020d

Update Randomness contract bytecode

84fa200

Use H256, rename secret to random number.

b19a2d4

Use get_commit_and_cipher

624ce47

Clean up Miner::prepare_block.

727fce3

afck force-pushed the afck-rand branch from a5791db to 727fce3 Compare December 3, 2019 14:15

ordian reviewed Dec 3, 2019

View reviewed changes

Remove is_reveal_phase call.

b51249b

dvdplm approved these changes Dec 5, 2019

View reviewed changes

Add more comments, require randomness contract map.

4799aeb

Simplify run_randomness_phase

0d7d0c8

tomusdrw suggested changes Dec 11, 2019

View reviewed changes

Address review comments.

ab169e2

tomusdrw approved these changes Dec 13, 2019

View reviewed changes

Remove Client::transact_contract.

ce5cc1a

dvdplm merged commit 2b1d148 into openethereum:master Dec 17, 2019

afck deleted the afck-rand branch December 17, 2019 12:35

varasev mentioned this pull request Jan 6, 2020

Error(Engine(NotProposer(Mismatch and Block(InvalidGasLimit(OutOfBounds poanetwork/parity-ethereum#203

Open

LukaszRozmej mentioned this pull request Feb 7, 2020

Add randomness contract support to AuRa NethermindEth/nethermind#1415

Closed

varasev mentioned this pull request Feb 20, 2020

Activate on-chain randomness in POA Sokol #11505

Merged

varasev mentioned this pull request Apr 7, 2020

Activate on-chain randomness in POA Core and update bootnodes #11609

Merged

rakita mentioned this pull request Dec 7, 2020

Backport xDAI support openethereum/openethereum#118

Closed

		snapshot::Snapshotting,
		transaction::{Action, SignedTransaction},

	.ok_or(EngineError::FailedSystemCall("Failed to upgrade to BlockchainClient.".to_string()))?;
	.ok_or_else(\|\| EngineError::FailedSystemCall("Failed to upgrade to BlockchainClient.".to_string()))?;

		@@ -397,30 +397,69 @@ pub trait BlockChainClient:

		/// Schedule state-altering transaction to be executed on the next pending block.
		fn transact_contract(&self, address: Address, data: Bytes) -> Result<(), transaction::Error> {

Add randomness contract support to AuthorityRound. #10946

Add randomness contract support to AuthorityRound. #10946

Conversation

afck commented Aug 6, 2019 • edited Loading

parity-cla-bot commented Aug 6, 2019

afck commented Aug 27, 2019 • edited Loading

afck commented Oct 17, 2019

afck commented Nov 27, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

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

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

afck commented Dec 5, 2019

tomusdrw 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

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

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

tomusdrw 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

afck commented Aug 6, 2019 •

edited

Loading

afck commented Aug 27, 2019 •

edited

Loading

afck commented Nov 27, 2019 •

edited

Loading