Consensus Engines Implementation: Aura #911

gnunicorn · 2018-10-16T12:07:09Z

This PR attempts to (re-)implement the pluggable consensus engine upon #883: Aura

Updated scope, Oct 25th, 2018:

And for later, following this

rphmeier · 2018-10-18T14:29:08Z

core/client/src/error.rs


 error_chain! {
+	links {
+		Consenensus(consensus::Error, consensus::ErrorKind);


rphmeier · 2018-10-18T14:31:54Z

core/consensus/aura/src/lib.rs

+			let environ = Arc::new(DummyFactory(client.clone()));
+			import_notifications.push(
+				client.import_notification_stream()
+					.take_while(|n| Ok(n.header.number() < &10))


it may be possible for this test to pass without each node getting the others' blocks. Perhaps we could add another non-authority node to the network

- move ImportBlock, BlockOrigin, ImportResult into shared sr-primitives - let Consensus provide and traits again - update consensus traits to latest development - implement traits on client::Client, test_client::TestClient - update RHD to use the new import_block API

core/consensus/rhd/src/error.rs

bkchr · 2018-10-25T14:19:04Z

core/consensus/aura/src/lib.rs

+	-> Result<Option<(AuthorityId, Vec<AuthorityId>)>, C::Error>
+{
+	let hash = header.hash();
+	match client.authorities(&BlockId::Hash(hash)) {


Maybe using let authorities = client.authorities(&BlockId::Hash(hash))?; would be better here?

bkchr · 2018-10-25T14:20:18Z

core/consensus/aura/src/lib.rs

+		Ok(dur) => Ok(dur),
+		Err(_) => {
+			warn!("Current time {:?} is before unix epoch. Something is wrong.", now);
+			return Err(())


If the error is just (), I would change the return type to an Option<Duration>.

And the return can be omitted here.

bkchr · 2018-10-25T14:21:27Z

core/consensus/aura/src/lib.rs

+}
+
+/// Get the slot for now.
+fn slot_now(slot_duration: u64) -> Result<u64, ()> {


Same here with the changing the return type to Option<u64>.

bkchr · 2018-10-25T14:53:40Z

core/consensus/common/src/lib.rs

-
-#![cfg(feature="rhd")]
+//! Consensus Basics and common features
+#![recursion_limit="128"]


Could you please add a comment, why we need the increased recursion limit?

@rphmeier !?!

it was there before AFAIK -- but it can go

turns out, the error_chain definition can blow up otherwise. this is a recommendation from the rust compiler. I'll add a comment.

core/service/src/lib.rs

core/state-machine/src/lib.rs

core/test-client/src/client_ext.rs

srml/balances/src/lib.rs

bkchr · 2018-10-25T16:10:16Z

srml/consensus/src/lib.rs

 		for i in &items {
 			storage::unhashed::put_raw(&i.0, &i.1);
 		}
 		Ok(())
 	}

+	/// Set the number of pages in the WebAssembly environment's heap.
+	pub fn set_heap_pages(pages: u64) -> Result {
+		storage::unhashed::put_raw(well_known_keys::HEAP_PAGES, &pages.encode());


If we don't have any result in this function, we do we need to return one here?

Just by looking around, how the other functions work in that file:

pub fn set_code(new: Vec<u8>) -> Result { storage::unhashed::put_raw(well_known_keys::CODE, &new); Ok(()) } /// Set some items of storage. pub fn set_storage(items: Vec<KeyValue>) -> Result { for i in &items { storage::unhashed::put_raw(&i.0, &i.1); } Ok(()) } /// Set the number of pages in the WebAssembly environment's heap. pub fn set_heap_pages(pages: u64) -> Result { storage::unhashed::put_raw(well_known_keys::HEAP_PAGES, &pages.encode()); Ok(()) }

I can assume this is the current convention. Not sure, if there a better reason. @gavofyork ?

pepyakin · 2018-10-26T09:18:42Z

core/client/src/call_executor.rs

@@ -150,7 +150,7 @@ where
 		let heap_pages = state.storage(well_known_keys::HEAP_PAGES)
 			.map_err(|e| error::ErrorKind::Execution(Box::new(e)))?
 			.and_then(|v| u64::decode(&mut &v[..]))
-			.unwrap_or(8) as usize;
+			.unwrap_or(1024) as usize;


Why was this changed?

@gavofyork can you say something about this?

8 is way too low to do a chain upgrade. as i understand it, memory is unallocated until used anyway so it shouldn't cause a performance degradation?

pepyakin · 2018-10-26T09:22:40Z

core/client/src/client.rs

+			body,
+			finalized,
+			_aux, // TODO: write this to DB also
+		) = import_block.into_inner();


I wonder why this uses into_inner and not plain destruction, like:

let ImportBlock { origin, pre_header, ... } = import_block;

pepyakin · 2018-10-26T13:01:32Z

core/consensus/aura/src/lib.rs

+	Error: ::std::error::Error + Send + 'static + From<::consensus_common::Error>,
+{
+
+	// wait until the next full slot has started before authoring anything


This comment looks misplaced, doesn't it?

core/consensus/aura/src/lib.rs

pepyakin · 2018-10-26T13:08:51Z

core/consensus/aura/src/lib.rs

+
+		let idx = slot_num % (authorities.len() as u64);
+
+		let expected_author = *authorities.get(idx as usize)


why do we check

assert!(idx <= usize::max_value() as u64, "It is impossible to have a vector with length beyond the address space; qed");

in the slot_author but not here?

(also one extra indent level)

pepyakin · 2018-10-26T13:49:47Z

core/network/src/sync.rs

@@ -18,7 +18,8 @@ use std::collections::HashMap;
 use std::sync::Arc;
 use protocol::Context;
 use network_libp2p::{Severity, NodeIndex};
-use client::{BlockStatus, BlockOrigin, ClientInfo};
+use client::{BlockStatus, ClientInfo};
+use consensus::{BlockOrigin};


Suggested change

use consensus::{BlockOrigin};

use consensus::BlockOrigin;

core/network/src/test/mod.rs

pepyakin · 2018-10-26T13:52:57Z

core/consensus/rhd/src/lib.rs

+				signature: Some((node_runtime::RawAddress::Id(local_id), signature, payload.0, Era::immortal())),
+				function: payload.1,
+			};
+			let uxt: <<C as AuthoringApi>::Block as BlockT>::Extrinsic = Decode::decode(&mut extrinsic.encode().as_slice()).expect("Encoded extrinsic is valid");


It looks like this code is wider than 120 maximum. Can we wrap this code?

pepyakin · 2018-10-26T13:53:21Z

core/consensus/rhd/src/lib.rs

+						=> MisbehaviorKind::BftDoubleCommit(round as u32, (h1.into(), s1.signature), (h2.into(), s2.signature)),
+				}
+			};
+			let payload = (next_index, Call::Consensus(ConsensusCall::report_misbehavior(report)), Era::immortal(), self.client.genesis_hash());


It looks like this code is wider than 120 maximum. Can we wrap this code?

pepyakin · 2018-10-26T14:00:05Z

core/consensus/rhd/src/lib.rs

+
+fn current_timestamp() -> u64 {
+	time::SystemTime::now().duration_since(time::UNIX_EPOCH)
+		.expect("now always later than unix epoch; qed")


Why do we claim here that now is always later than UNIX epoch but in aura code we try to handle it?

This has been moved here from the old code base. I am not actually sure this or the other approach is really that different, after I changed the unwrap_or to also fail now.

Bumps [h2](https://github.com/hyperium/h2) from 0.3.16 to 0.3.17. - [Release notes](https://github.com/hyperium/h2/releases) - [Changelog](https://github.com/hyperium/h2/blob/master/CHANGELOG.md) - [Commits](hyperium/h2@v0.3.16...v0.3.17) --- updated-dependencies: - dependency-name: h2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

* add partial_fee estimation to submittable extrinsic * add integration test * make functions immune to doctest * add doc test * inline encoded_with_len, fix tests * fix test fmt * remove unused imoort * Bump h2 from 0.3.16 to 0.3.17 (paritytech#911) Bumps [h2](https://github.com/hyperium/h2) from 0.3.16 to 0.3.17. - [Release notes](https://github.com/hyperium/h2/releases) - [Changelog](https://github.com/hyperium/h2/blob/master/CHANGELOG.md) - [Commits](hyperium/h2@v0.3.16...v0.3.17) --- updated-dependencies: - dependency-name: h2 dependency-type: indirect ... Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com> * call_raw returns Res: Decode * remove import * remove struct --------- Signed-off-by: dependabot[bot] <support@github.com> Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>

gnunicorn added the A3-in_progress Pull request is in progress. No review needed at this stage. label Oct 16, 2018

gnunicorn assigned gnunicorn and rphmeier Oct 16, 2018

gnunicorn force-pushed the rh-aura branch 2 times, most recently from 990f378 to 733e826 Compare October 17, 2018 16:18

rphmeier reviewed Oct 18, 2018

View reviewed changes

core/client/src/error.rs Outdated

error_chain! {

links {

Consenensus(consensus::Error, consensus::ErrorKind);

Copy link

Contributor

rphmeier Oct 18, 2018

Choose a reason for hiding this comment

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

typo :)

rphmeier reviewed Oct 18, 2018

View reviewed changes

gnunicorn force-pushed the rh-aura branch 2 times, most recently from 43943d7 to cad30e8 Compare October 19, 2018 15:21

rphmeier and others added 10 commits October 19, 2018 18:25

begin to shuffle some more stuff over to consensus-common

e333350

initial aura block production

9bd7495

move node consensus into rhd

2a9268e

Move ImportBlock into consensus-common

de5a15e

Added block notification after import

65f8ad6

re-enable tests

822a0d6

Send import notification in aura tests

c1b9387

split into light and full, attempt to use aura

962bc0a

Integrating aura into service

51ace82

gnunicorn force-pushed the rh-aura branch from 59a180f to 51ace82 Compare October 19, 2018 22:39

gnunicorn and others added 10 commits October 20, 2018 01:27

Make Signatures more generic

c1f9f19

Aura Block Production with the given key

dd085e5

Block Proposing like 912 intended

bf1c7a4

also adapt to 924

1dbcfce

match up inherentdata struct

122c633

Merge remote-tracking branch 'origin/master' into rh-aura

8138a0b

Fix

ffe5a14

update slot duration and secs_per_block to not conflict

e177ae9

run aura on the thread pool

02d6fe0

start at exact step start in aura

a766d52

gnunicorn requested review from arkpar and pepyakin October 25, 2018 15:31

rphmeier mentioned this pull request Oct 25, 2018

Dynamic authority sets in GRANDPA #1014

Merged

5 tasks

bkchr reviewed Oct 25, 2018

View reviewed changes

gnunicorn mentioned this pull request Aug 24, 2023

Report Aura Equivocations to runtime paritytech/polkadot-sdk#70

Open

gnunicorn added 2 commits October 26, 2018 12:27

Cleaning up consensus-rhd

ded4fe9

parity-codec version bump

39d6e50

gnunicorn mentioned this pull request Oct 26, 2018

Clean up Consensus Service traits and infrastructure #1021

Open

gnunicorn and others added 5 commits October 26, 2018 13:09

addressing grumbles

c4a8acd

Reform Result<T, ()> into Option<T> returns

005110e

Attempt to merge remote-tracking branch 'origin/master' into rh-aura

30fc26c

Fixes bugs with the decl_module! macro

b4f0f64

Fixes last errors after merge

60ff69c

bkchr approved these changes Oct 26, 2018

View reviewed changes

Some nitpicking

63cbe77

rphmeier mentioned this pull request Oct 26, 2018

Write provided auxiliary data changes to DB in client when importing block. #1023

Closed

pepyakin reviewed Oct 26, 2018

View reviewed changes

rphmeier and others added 4 commits October 26, 2018 16:11

kill some TODOs

075f80a

Simplify and reuse slot_author

4c2955f

Addressing grumbles

17be798

Merge remote-tracking branch 'origin/master' into rh-aura

9ab87a6

gnunicorn merged commit 101d673 into master Oct 27, 2018

gnunicorn deleted the rh-aura branch October 27, 2018 13:59

This was referenced Oct 29, 2018

Generalize Consensus Setup for Node #1032

Closed

clean up Justification struct #1033

Closed

gnunicorn mentioned this pull request Nov 27, 2018

Aura: allow deferring of headers in verification #1019

Open

gnunicorn mentioned this pull request Jan 8, 2019

Reactivate RHD #1020

Open

lamafab pushed a commit to lamafab/substrate that referenced this pull request Jun 16, 2020

remove hardcoded scheduled GRANDPA pause (paritytech#911)

42c04e4

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consensus Engines Implementation: Aura #911

Consensus Engines Implementation: Aura #911

gnunicorn commented Oct 16, 2018 •

edited by shawntabrizi

Loading

rphmeier Oct 18, 2018

rphmeier Oct 18, 2018

bkchr Oct 25, 2018

bkchr Oct 25, 2018

bkchr Oct 25, 2018

bkchr Oct 25, 2018

bkchr Oct 25, 2018

gnunicorn Oct 26, 2018

rphmeier Oct 26, 2018

gnunicorn Oct 26, 2018

bkchr Oct 25, 2018

gnunicorn Oct 26, 2018

pepyakin Oct 26, 2018

gnunicorn Oct 26, 2018

gavofyork Oct 26, 2018

pepyakin Oct 26, 2018

pepyakin Oct 26, 2018

pepyakin Oct 26, 2018

gavofyork Oct 26, 2018

pepyakin Oct 26, 2018

pepyakin Oct 26, 2018

pepyakin Oct 26, 2018

pepyakin Oct 26, 2018

gnunicorn Oct 26, 2018


		let idx = slot_num % (authorities.len() as u64);

		let expected_author = *authorities.get(idx as usize)

Consensus Engines Implementation: Aura #911

Consensus Engines Implementation: Aura #911

Conversation

gnunicorn commented Oct 16, 2018 • edited by shawntabrizi Loading

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

gnunicorn commented Oct 16, 2018 •

edited by shawntabrizi

Loading