Generalize the Consensus Infrastructure #883

gnunicorn · 2018-10-05T22:25:36Z

In our efforts to allow pluggable consensus engines in substrate, this PR attempts to generalize consensus messages over the network, blocks and the client.

Generalize NetworkMessages for Consensus as an opaque Vec<u8>
Generalize Justification as a Consensus-specific opaque Vec<u8>
Generalize ConsensusGossip over any consensus internal information
Remove all rhododendron-dependencies (and everything bft:*) from core, sr-* and the srml
Restructure Import-Procedure as described in Pluggable Consensus: Import Queue #784
- Decouple import_block from client
- generalize client over cli to allow substrate to decide on consensus engine
- let allow cli to block-import again
disable current rhd-code-base; make it all compile

gavofyork · 2018-10-06T15:08:29Z

because GRANDPA and AURA have a higher priority at the moment, we might post-pone fixing rhd for later and merge this after the restructure itself finished, breaking consensus for node on master...

tht's fine - bbq birch will be decommissioned as soon as shaft(grandpa)/aura are operational.

gnunicorn · 2018-10-08T14:33:40Z

Updated the comment to reflect the latest design and process decisions, specifically:

fix block-import for cli by providing allow substrate/node to provide a specialization to decide on consensus engines
drop attempt to fix RHD; instead disable and focus on getting CI pass without it for now

Previously, this also attempted to, but won't be part of this

Re-activate rhododendron (substrate-consensus-rhd)*

simplify message handling by directly supporting for parity-codec

move MisbehaviourCheck into rhd-crate (as it is rhd specific)

refactor rhodendron to use new ImportQueue-System

re-implement rhd in node

ensure rhd-messages are backwards-compatible

Move all common consensus into a shared substrate-consensus-common-crate

OfflineTracker

Generic Consensus Service (from node)

rphmeier · 2018-10-10T17:31:45Z

core/network/src/import_queue.rs

+}
+
+#[cfg(any(test, feature = "test-helpers"))]
+unsafe impl Send for PassThroughVerifier {}


PassThroughVerifier stores only a bool -- are these necessary?

nope. this stems from a previous version, where it stored more than a bool. removed.

svyatonik · 2018-10-15T08:53:56Z

core/client/src/client.rs

-		body: Option<Vec<<Block as BlockT>::Extrinsic>>,
-		finalized: bool,
+		import_block: ImportBlock<Block>,
+	    new_authorities: Option<Vec<AuthorityId>>,


This value (named new_authorities) is actually cached under parent header Id - previously it meant 'authorities that have justified this block'. So probably rename back to authorities, since it is not necessary the new authorities set? Or the meaning has changed?

@svyatonik the meaning has changed. While you described how it was previously, all actual checks are done within the Verifier now and this only communicates the changes to import: the Block itself and optionally a new Set of Authorities - as changes of authority set will also be consensus engine driven. This is a temporary hack until we have the signaling in place, in which case this wouldn't be passed here anymore but instead a NewAuthorities-Signal will be triggered at import time instead.

OK, thanks. Then better to wait for actual Verifier implementation && see if some changes are required here (and in the authorities cache).

core/client/src/client.rs

bkchr · 2018-10-15T08:53:41Z

core/client/src/client.rs

+    pub header: Block::Header,
+    /// Justification provided for this block from the outside
+    pub external_justification: Justification,
+    /// internal Justification for the block


Nitpick: Capitalize Internal.

bkchr · 2018-10-15T09:06:50Z

core/consensus/rhd/src/lib.rs

-
-/// Sign a BFT message with the given key.
-pub fn sign_message<B: Block + Clone>(message: Message<B>, key: &ed25519::Pair, parent_hash: B::Hash) -> LocalizedMessage<B> {
+// fn check_justification_signed_message<H>(


Why did you need this code entirely? If you need this code anytime in the future, you can just use git to get it back^^

pepyakin

Looks good! a few minor nits.

pepyakin · 2018-10-15T15:37:59Z

core/client/src/client.rs

-		body: Option<Vec<<Block as BlockT>::Extrinsic>>,
-		finalized: bool,
+		import_block: ImportBlock<Block>,
+	    new_authorities: Option<Vec<AuthorityId>>,


Stray spaces

pepyakin · 2018-10-15T16:21:21Z

node/service/src/lib.rs

 /// A collection of type to generalise specific components over full / light client.
 pub trait Components: service::Components {
 	/// Demo API.
-	type Api: 'static + AuthoringApi + Send + Sync;
+	type Api: 'static +  Send + Sync;


Double space

pepyakin · 2018-10-15T17:38:08Z

core/network/src/protocol.rs

+			sync.on_block_data(&mut ProtocolContext::new(&self.context_data, io), peer, request, response)
+		};
+
+		if let Some((origin, new_blocks)) =  new_blocks {


Double space

pepyakin · 2018-10-15T17:47:45Z

core/service/src/components.rs

+			}
+
+		} else {
+			Err("Chain Specification doesn't containg any consensus_engine name".into())


containg → contain?

pepyakin · 2018-10-15T17:47:55Z

core/service/src/components.rs

+			}
+
+		} else {
+			Err("Chain Specification doesn't containg any consensus_engine name".into())


containg → contain?

pepyakin · 2018-10-15T17:50:59Z

core/service/src/chain_ops.rs

+				receipt: None,
+				message_queue: None
+			};
+			// import queue handels verification and importing it into the client


handels → handles

gavofyork · 2018-10-16T09:43:28Z

node/service/src/lib.rs

@@ -71,10 +60,33 @@ pub type ChainSpec = service::ChainSpec<GenesisConfig>;
 pub type ComponentClient<C> = Client<<C as Components>::Backend, <C as Components>::Executor, Block>;
 pub type NetworkService = network::Service<Block, <Factory as service::ServiceFactory>::NetworkProtocol, Hash>;

+/// A verifier that doesn't actually do any checks
+pub struct NoneVerifier;


Using () for "null" implementations of traits by convention.

Can't be done here: impl<B: BlockT> Verifier<B> for () can only be done within the crate that defines Verifier, not in node-service but in substrate-network . However we don't want to provide such a bad implementation in substrate-network.

Leaving it as is for now, as this is only temporarily to make the compiler happy and will be replaced in the next chunk of work with an actual Verifier implementation.

gavofyork

Minor grumble that might be nice/reasonable to address

- simplify ImportQueue.import_blocks - remove Deadlock on import_block - Adding Verifier-Trait - Implement import_queue provisioning in service; allow cli to import

Co-authored-by: cheme <emericchevalier.pro@gmail.com>

Bumps [regex](https://github.com/rust-lang/regex) from 1.7.1 to 1.7.3. - [Release notes](https://github.com/rust-lang/regex/releases) - [Changelog](https://github.com/rust-lang/regex/blob/master/CHANGELOG.md) - [Commits](rust-lang/regex@1.7.1...1.7.3) --- updated-dependencies: - dependency-name: regex dependency-type: direct:production update-type: version-update:semver-patch ... 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 5, 2018

gnunicorn force-pushed the ben-consensus-restructure branch from 404c543 to bfb31c0 Compare October 8, 2018 14:28

rphmeier reviewed Oct 10, 2018

View reviewed changes

gnunicorn force-pushed the ben-consensus-restructure branch 3 times, most recently from 294d9a1 to 21d394e Compare October 12, 2018 22:47

svyatonik reviewed Oct 15, 2018

View reviewed changes

bkchr reviewed Oct 15, 2018

View reviewed changes

gnunicorn requested review from arkpar and pepyakin October 15, 2018 10:21

gnunicorn changed the title ~~[WIP] Generalize the Consensus Infrastructure~~ Generalize the Consensus Infrastructure Oct 15, 2018

gnunicorn added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Oct 15, 2018

pepyakin approved these changes Oct 15, 2018

View reviewed changes

gavofyork reviewed Oct 16, 2018

View reviewed changes

gnunicorn added 8 commits October 16, 2018 12:52

split out consensus

f54619e

Supply ImportQueue through network-service

49a2ef8

- simplify ImportQueue.import_blocks - remove Deadlock on import_block - Adding Verifier-Trait - Implement import_queue provisioning in service; allow cli to import

Allow to actually customize import queue

14d5283

Addressing grumbles

ec51642

Consensus Gossip: Cache Message hash per Topic

f89841b

fix consensus gossip tests

1cd6d9c

Tests: Blocks aren't finalized yet; fixes grandpa tests

5c254fe

Addressing grumbles

dc0826b

gnunicorn force-pushed the ben-consensus-restructure branch 2 times, most recently from 0e77cc7 to dc0826b Compare October 16, 2018 11:27

gnunicorn merged commit 14ff8f9 into master Oct 16, 2018

gnunicorn mentioned this pull request Oct 16, 2018

Consensus Engines Implementation: Aura #911

Merged

21 tasks

arkpar deleted the ben-consensus-restructure branch October 18, 2018 20:45

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

Use jmalloc, align kvdb crate version. (paritytech#883)

27fc65f

Co-authored-by: cheme <emericchevalier.pro@gmail.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Generalize the Consensus Infrastructure #883

Generalize the Consensus Infrastructure #883

gnunicorn commented Oct 5, 2018 •

edited

Loading

gavofyork commented Oct 6, 2018

gnunicorn commented Oct 8, 2018 •

edited

Loading

rphmeier Oct 10, 2018

gnunicorn Oct 11, 2018

svyatonik Oct 15, 2018

gnunicorn Oct 15, 2018

svyatonik Oct 15, 2018

bkchr Oct 15, 2018

bkchr Oct 15, 2018

pepyakin left a comment

pepyakin Oct 15, 2018

pepyakin Oct 15, 2018

pepyakin Oct 15, 2018

pepyakin Oct 15, 2018

pepyakin Oct 15, 2018

pepyakin Oct 15, 2018

gavofyork Oct 16, 2018

gnunicorn Oct 16, 2018 •

edited

Loading

gavofyork left a comment

Generalize the Consensus Infrastructure #883

Generalize the Consensus Infrastructure #883

Conversation

gnunicorn commented Oct 5, 2018 • edited Loading

gavofyork commented Oct 6, 2018

gnunicorn commented Oct 8, 2018 • edited 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

pepyakin 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

gnunicorn Oct 16, 2018 • edited Loading

Choose a reason for hiding this comment

gavofyork left a comment

Choose a reason for hiding this comment

gnunicorn commented Oct 5, 2018 •

edited

Loading

gnunicorn commented Oct 8, 2018 •

edited

Loading

gnunicorn Oct 16, 2018 •

edited

Loading