Parachain Consensus abstractions #329

bkchr · 2021-02-15T20:01:01Z

This pr introduces abstractions around the parachain consensus that is driven by the collator. It moves the current implementation into the RelayChainConsensus as a consensus that is entirely dependent on the relay chain.

Fixes: #36

…sensus

burdges · 2021-02-15T21:08:59Z

client/consensus/relay-chain/src/lib.rs

+//! total number of parachain validators.
+//!
+//! 4. The parachain candidate with the highest number of approvals is choosen by the relay-chain
+//! block producer to be backed.


Approvals means supporting parachain validators I guess.

Yeah, prefer the term backing as approval means something else in parachain consensus.

rphmeier · 2021-02-15T21:55:52Z

client/consensus/common/src/lib.rs

+	///
+	/// # NOTE
+	///
+	/// It is expected that the block is


pepyakin

Looks good overall. I am not sure how it fixes #36 though

pepyakin · 2021-02-16T10:13:37Z

client/collator/src/lib.rs

@@ -343,14 +260,15 @@ where
 		relay_parent: PHash,
 		validation_data: PersistedValidationData,
 	) -> Option<CollationResult> {
-		trace!(target: LOG_TARGET, "Producing candidate");
+		tracing::trace!(target: LOG_TARGET, "Producing candidate");


worth adding relay_parent tag here?

pepyakin · 2021-02-16T10:19:06Z

client/collator/src/lib.rs

 }

-impl<Block: BlockT, PF, BI, BS, Backend, PBackend, PClient, PBackend2> Clone
-	for Collator<Block, PF, BI, BS, Backend, PBackend, PClient, PBackend2>


bkchr · 2021-02-16T11:10:46Z

Looks good overall. I am not sure how it fixes #36 though

#36 was a rather shitty issue. In the end I settled with myself on requiring that people will need to implement ParachainConsensus for each consensus. Maybe I will provide an implementation of ParachainConsensus for T where T: Slots, but for that I will first need to have some examples :D

JoshOrndorff

I was a little late here, but I'll submit this anyway. Thanks for this PR.

JoshOrndorff · 2021-02-16T02:07:13Z

client/consensus/common/src/lib.rs

+/// implementation could for example check if this specific collator is part of the validator.
+#[async_trait::async_trait]
+pub trait ParachainConsensus<B: BlockT>: Send + Sync + dyn_clone::DynClone {
+	/// Produce a new candidate at the given parent block.


Suggested change

/// Produce a new candidate at the given parent block.

/// Produce a new candidate at the given parent and relay-parent blocks.

JoshOrndorff · 2021-02-16T02:07:47Z

client/consensus/common/src/lib.rs

+/// The collator will call [`Self::produce_candidate`] every time there is a free core for the parachain
+/// this collator is collating for. It is the job of the consensus implementation to decide if this
+/// specific collator should build candidate for the given relay chain block. The consensus
+/// implementation could for example check if this specific collator is part of the validator.


Suggested change

/// The collator will call [`Self::produce_candidate`] every time there is a free core for the parachain

/// this collator is collating for. It is the job of the consensus implementation to decide if this

/// specific collator should build candidate for the given relay chain block. The consensus

/// implementation could for example check if this specific collator is part of the validator.

/// The collator will call [`Self::produce_candidate`] every time there is a free core for the parachain

/// this collator is collating for. It is the job of the consensus implementation to decide whether this

/// specific collator should build a candidate for the given relay chain block. The consensus

/// implementation could, for example, check whether this specific collator is part of a staked set.

JoshOrndorff · 2021-02-16T02:08:10Z

client/consensus/common/src/lib.rs

@@ -511,6 +510,52 @@ where
 	}
 }

+/// The result of [`ParachainConsensus::produce_candidate`].
+pub struct ParachainCandidate<B> {
+	/// The block that was build for this candidate.


Suggested change

/// The block that was build for this candidate.

/// The block that was built for this candidate.

JoshOrndorff · 2021-02-16T02:11:57Z

client/consensus/relay-chain/src/lib.rs

+//! 3. The parachain validators validate at most X different parachain candidates, where X is the
+//! total number of parachain validators.


Suggested change

//! 3. The parachain validators validate at most X different parachain candidates, where X is the

//! total number of parachain validators.

//! 3. The validators assigned to this parachain validate at most X different parachain candidates, where X is the

//! total number of validators assigned to this parachain.

Mostly suggested this one because I worry that people who haven't read all the docs might think that "parachain validators" means collators.

bkchr · 2021-02-16T14:11:28Z

@JoshOrndorff could you open a pr with your suggestions please?

JoshOrndorff · 2021-02-26T20:44:44Z

Could you explain a little bit more about the RelayChainConsensusBuilder?

Why is it necessary? I don't quite understand the comment:

/// Builds a [RelayChainConsensus] for a parachain. As this requires
/// a concrete relay chain client instance, the builder takes a [polkadot_service::Client]
/// that wraps this concrete instanace. By using [polkadot_service::ExecuteWithClient]
/// the builder gets access to this concrete instance.

I mostly understand the explanation, but I don't understand why the builder needs a concrete relay chain client instance. Is this pattern used somewhere else? This seems more complex than the analogous code in sc_consensus_aura

JoshOrndorff · 2021-02-26T21:17:11Z

I guess this is the part that is really bending my brain:

	/// Build the relay chain consensus.
	fn build(self) -> Box<dyn ParachainConsensus<Block>> {
		self.relay_chain_client.clone().execute_with(self)
	}

bkchr · 2021-02-26T23:18:34Z

This works around some missing type system features :P

A polkadot client can either be Kusama/Polkadot/Rococo/Test and all use some small different types. Actually most of them are equal, but the compiler can not resolve this. To circumvent this problem on the polkadot side, there is this Client enum that combines all of them into one enum. Than we use this machinery here to get access to the real type inside this Client enum that implements all the required traits etc.

JoshOrndorff · 2021-03-01T13:42:00Z

Thanks for explaining. I see the part you mean in Polkadot now.

https://github.com/paritytech/polkadot/blob/71d46a027be3a8435c9500491e9fa0ee2c6944b3/node/service/src/client.rs#L97-L163

bkchr added 9 commits February 11, 2021 16:07

Move consensus to consensus-common crate

2934371

Move the parachain consensus out of the collator

b9a3832

Add first relay chain consensus stuff

b0ffe4c

Remove some warnings

2ba1466

Fix more stuff

7aa6978

Fix collator test

c3b086c

Change ParachainConsensus to take a mutable self

58190b0

Merge remote-tracking branch 'origin/master' into bkchr-parachain-con…

91a97fa

…sensus

Make everything compile

8078384

bkchr requested a review from pepyakin February 15, 2021 20:01

burdges reviewed Feb 15, 2021

View reviewed changes

rphmeier reviewed Feb 15, 2021

View reviewed changes

client/consensus/common/src/lib.rs Outdated

///

/// # NOTE

///

/// It is expected that the block is

Copy link

Contributor

rphmeier Feb 15, 2021

Choose a reason for hiding this comment

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

trailing

pepyakin approved these changes Feb 16, 2021

View reviewed changes

Feedback

6839c6c

bkchr merged commit 4820fa1 into master Feb 16, 2021

bkchr deleted the bkchr-parachain-consensus branch February 16, 2021 11:45

JoshOrndorff reviewed Feb 16, 2021

View reviewed changes

JoshOrndorff mentioned this pull request Feb 18, 2021

Minor improvements to consensus docs #333

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Parachain Consensus abstractions #329

Parachain Consensus abstractions #329

bkchr commented Feb 15, 2021

burdges Feb 15, 2021

rphmeier Feb 15, 2021

rphmeier Feb 15, 2021

pepyakin left a comment

pepyakin Feb 16, 2021

pepyakin Feb 16, 2021

bkchr commented Feb 16, 2021

JoshOrndorff left a comment

JoshOrndorff Feb 16, 2021

JoshOrndorff Feb 16, 2021

JoshOrndorff Feb 16, 2021

JoshOrndorff Feb 16, 2021

JoshOrndorff Feb 16, 2021

bkchr commented Feb 16, 2021

JoshOrndorff commented Feb 26, 2021

JoshOrndorff commented Feb 26, 2021

bkchr commented Feb 26, 2021

JoshOrndorff commented Mar 1, 2021

	/// Produce a new candidate at the given parent block.
	/// Produce a new candidate at the given parent and relay-parent blocks.

	/// The block that was build for this candidate.
	/// The block that was built for this candidate.

		//! 3. The parachain validators validate at most X different parachain candidates, where X is the
		//! total number of parachain validators.

Parachain Consensus abstractions #329

Parachain Consensus abstractions #329

Conversation

bkchr commented Feb 15, 2021

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

bkchr commented Feb 16, 2021

JoshOrndorff 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

bkchr commented Feb 16, 2021

JoshOrndorff commented Feb 26, 2021

JoshOrndorff commented Feb 26, 2021

bkchr commented Feb 26, 2021

JoshOrndorff commented Mar 1, 2021