BEEFY-specific import_justification #10699

Wizdave97 · 2022-01-19T18:30:54Z

Fixes paritytech/grandpa-bridge-gadget#183
polkadot companion: paritytech/polkadot#4791

… david/beefy-block-import

seunlanlege · 2022-01-19T18:42:48Z

you beauty 😍

client/beefy/src/justification.rs

… david/beefy-block-import

acatangiu · 2022-01-25T15:13:48Z

client/beefy/Cargo.toml

 sc-keystore = { version = "4.0.0-dev", path = "../keystore" }
 sc-network = { version = "0.10.0-dev", path = "../network" }
 sc-network-gossip = { version = "0.10.0-dev", path = "../network-gossip" }
+sp-consensus = { version = "0.10.0-dev", path = "../../primitives/consensus/common" }


nit:

please move this above with the other sp-* and ideally keep alphabetical ordering for easy reading/parsing

acatangiu · 2022-01-25T15:20:14Z

client/beefy/src/lib.rs

+/// Link between the block importer and the beefy client.
+/// It provides access to the justification stream
+pub struct LinkHalf<Block: BlockT> {
+	justification_stream: BeefyJustificationStream<NumberFor<Block>, Signature>,
+}
+
+impl<Block: BlockT> LinkHalf<Block> {
+	/// Get the receiving end of justification notifications.
+	pub fn justification_stream(&self) -> BeefyJustificationStream<NumberFor<Block>, Signature> {
+		self.justification_stream.clone()
+	}
+}


can't this be a simple type definition?

Suggested change

/// Link between the block importer and the beefy client.

/// It provides access to the justification stream

pub struct LinkHalf<Block: BlockT> {

justification_stream: BeefyJustificationStream<NumberFor<Block>, Signature>,

}

impl<Block: BlockT> LinkHalf<Block> {

/// Get the receiving end of justification notifications.

pub fn justification_stream(&self) -> BeefyJustificationStream<NumberFor<Block>, Signature> {

self.justification_stream.clone()

}

}

/// Link between the block importer and the beefy client, the justification stream.

pub type LinkHalf<Block> = BeefyJustificationStream<NumberFor<Block>, Signature>;

or (even better) just use BeefyJustificationStream directly for clarity.

Okay, I reasoned we might share more state between the block import worker and the beefy worker in the future like in grandpa, that's why I created this struct, I guess the just the justification stream is good for now though.

acatangiu · 2022-01-25T15:22:19Z

client/beefy/src/lib.rs

+) -> (BeefyBlockImport<BE, Block, Client, I>, LinkHalf<Block>) {
+	let (justification_sender, justification_stream) = BeefyJustificationStream::channel();
+	let import =
+		BeefyBlockImport::new(client.clone(), wrapped_block_import, justification_sender.clone());


Suggested change

BeefyBlockImport::new(client.clone(), wrapped_block_import, justification_sender.clone());

BeefyBlockImport::new(client.clone(), wrapped_block_import, justification_sender);

acatangiu · 2022-01-25T15:28:40Z

client/beefy/src/lib.rs

+/// Produce a BEEFY block import object and a link half for tying it to the client
+pub fn block_import<BE, Client, Block: BlockT, I>(


Pls add a companion PR that uses this in Polkadot (Rococo chain).

client/beefy/src/import.rs

acatangiu · 2022-01-26T13:31:16Z

client/beefy/src/import.rs

+#[async_trait::async_trait]
+impl<BE, Block: BlockT, Client, I> JustificationImport<Block>
+	for BeefyBlockImport<BE, Block, Client, I>
+where
+	BE: Backend<Block>,
+	Client: BeefyClient<Block, BE> + ProvideRuntimeApi<Block>,
+	Client::Api: BeefyApi<Block>,
+	I: JustificationImport<Block, Error = ConsensusError> + Send + Sync,
+{
+	type Error = ConsensusError;
+
+	async fn on_start(&mut self) -> Vec<(Block::Hash, NumberFor<Block>)> {
+		self.inner.on_start().await
+	}
+
+	async fn import_justification(
+		&mut self,
+		hash: Block::Hash,
+		number: NumberFor<Block>,
+		justification: Justification,
+	) -> Result<(), Self::Error> {
+		// Try Importing Beefy justification
+		BeefyBlockImport::import_justification(self, hash, number, justification.clone())?;
+		// Importing for inner BlockImport
+		self.inner.import_justification(hash, number, justification).await
+	}
+}


Where would this be used? As I understand it, we're manually importing justifications on BlockImport.

Is JustificationImport::import_justification ever going to be called for beefy justifications? If yes, where from (I'm new to this area myself)?

acatangiu · 2022-01-26T13:36:37Z

client/beefy/src/justification.rs

+	let res = verify_with_validator_set::<Block>(number, validator_set, finality_proof.clone())?;
+
+	if res {
+		return Ok(finality_proof)
+	}
+
+	Err(ConsensusError::InvalidJustification)


nit:

Suggested change

let res = verify_with_validator_set::<Block>(number, validator_set, finality_proof.clone())?;

if res {

return Ok(finality_proof)

}

Err(ConsensusError::InvalidJustification)

if verify_with_validator_set::<Block>(number, validator_set, finality_proof.clone())? {

Ok(finality_proof)

} else {

Err(ConsensusError::InvalidJustification)

}

acatangiu · 2022-01-26T13:42:45Z

client/beefy/src/import.rs

+/// BeefyBlockImport
+/// Wraps a type `inner` that implements [`BlockImport`]


Suggested change

/// BeefyBlockImport

/// Wraps a type `inner` that implements [`BlockImport`]

/// A block-import handler for BEEFY.

///

/// This scans each imported block for BEEFY justifications and verifies them.

/// Wraps a type `inner` that implements [`BlockImport`] and ultimately defers to it.

acatangiu · 2022-01-26T13:43:46Z

client/beefy/src/import.rs

+
+/// Checks the given header for a consensus digest signalling a beefy authority set change
+/// and extracts it.
+pub fn find_beefy_authority_set_change<B: BlockT>(


no reason for this helper fn to be public

Suggested change

pub fn find_beefy_authority_set_change<B: BlockT>(

fn find_beefy_authority_set_change<B: BlockT>(

acatangiu · 2022-01-26T15:56:10Z

client/beefy/src/import.rs

+pub struct BeefyBlockImport<Backend, Block: BlockT, Client, I> {
+	client: Arc<Client>,
+	inner: I,
+	justification_sender: BeefyJustificationSender<NumberFor<Block>, Signature>,


Oh, and I'd forgot about this, sorry.

The code in the PR only validates BEEFY justifications in block imports and nothing else.

I am guessing there is a 2nd part or a future PR that uses this justification_sender to forward justifications from block import to the beefy gadget, right?

Yeah, there should definitely be a second PR that makes use of this , but I don't understand fully how it would look like, was hoping to get insight on that after this was reviewed, but this just verifies beefy justfications if present and ensures there are justifications for authority set change blocks.

… david/beefy-block-import

tomusdrw

The PR makes many low-level design/architectural decisions that I feel were not well thought through or discussed with the responsible team, also some of that stuff is just incorrect. IMHO there is too much to change here to be worth continuing this discussion as a PR review.

I appreciate your willingness to help and note that this is mostly failure on our side, since the design docs and proper issue details are missing.
Please consult a working draft of BEEFY spec & implementation notes for now and until the docs are fully fleshed-out the best way forward is to discuss implementation details of a particular issue with the responsible team beforehand.

tomusdrw · 2022-01-27T10:21:02Z

client/beefy/src/import.rs

+		// If this block contains a beefy authority set change then it must have a beefy
+		// justification
+		let change = find_beefy_authority_set_change::<Block>(&block.header);


What is this assumption based on?

This was based on the mandatory blocks requirement in the docs

tomusdrw · 2022-01-27T10:21:23Z

client/beefy/src/import.rs

+	let id = OpaqueDigestItemId::Consensus(&BEEFY_ENGINE_ID);
+
+	let filter_log = |log: ConsensusLog<AuthorityId>| match log {
+		ConsensusLog::AuthoritiesChange(change) => Some(change),


This logic is duplicated in gadget code.

tomusdrw · 2022-01-27T10:21:29Z

client/beefy/src/import.rs

@@ -0,0 +1,197 @@
+use std::{collections::HashMap, marker::PhantomData, sync::Arc};


No preamble

tomusdrw · 2022-01-27T10:23:50Z

client/beefy/src/import.rs

+		let validator_set = self
+			.client
+			.runtime_api()
+			.validator_set(&at)
+			.map_err(|e| ConsensusError::ClientImport(e.to_string()))?;


We should be verifying that the previous authority set has signed off on the hand-off instead.

tomusdrw · 2022-01-27T10:24:02Z

client/beefy/src/justification.rs

@@ -0,0 +1,58 @@
+use crate::keystore::BeefyKeystore;


Preamble missing.

tomusdrw · 2022-01-27T10:25:13Z

client/beefy/src/justification.rs

+pub(crate) fn verify_with_validator_set<Block: BlockT>(
+	number: NumberFor<Block>,
+	validator_set: &ValidatorSet<AuthorityId>,
+	proof: VersionedFinalityProof<NumberFor<Block>, Signature>,
+) -> Result<bool, ConsensusError> {


This logic should most likely be part of the gadget code not hardcoded into client/beefy

tomusdrw · 2022-01-27T10:25:52Z

client/beefy/src/lib.rs

 	cfg
 }

+/// Produce a BEEFY block import object and a link half for tying it to the client


This is misleading. The gadget will be appending justifications as well and notifications will not be triggered here.

Wizdave97 · 2022-01-27T10:51:37Z

The PR makes many low-level design/architectural decisions that I feel were not well thought through or discussed with the responsible team, also some of that stuff is just incorrect. IMHO there is too much to change here to be worth continuing this discussion as a PR review.

I appreciate your willingness to help and note that this is mostly failure on our side, since the design docs and proper issue details are missing. Please consult a working draft of BEEFY spec & implementation notes for now and until the docs are fully fleshed-out the best way forward is to discuss implementation details of a particular issue with the responsible team beforehand.

@tomusdrw could you please help detail how this issue could be solved correctly? I’ll really like to get it across the finish line.

stale · 2022-02-26T14:25:02Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

Wizdave97 · 2022-02-26T19:52:49Z

Bump

stale · 2022-03-28T23:26:14Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

Wizdave97 · 2022-03-29T08:36:18Z

bump

stale · 2022-04-28T11:01:10Z

Hey, is anyone still working on this? Due to the inactivity this issue has been automatically marked as stale. It will be closed if no further activity occurs. Thank you for your contributions.

acatangiu · 2022-04-28T11:55:41Z

We need to settle on a design first (paritytech/grandpa-bridge-gadget#183 (comment)). Closing this for now.

Wizdave97 added 2 commits January 19, 2022 19:29

create a beefy blockimport type

01fc5f3

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

c7fd128

… david/beefy-block-import

Wizdave97 requested a review from acatangiu as a code owner January 19, 2022 18:30

minor fix

73ed135

seunlanlege reviewed Jan 19, 2022

View reviewed changes

client/beefy/src/justification.rs Show resolved Hide resolved

Wizdave97 and others added 5 commits January 19, 2022 20:07

verify block number of commitment

85cd4b1

enforce beefy justification when importing authority set change blocks

fb79ff2

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

c72c6b0

… david/beefy-block-import

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

a06eea1

… david/beefy-block-import

Merge branch 'paritytech:master' into david/beefy-block-import

164466c

acatangiu requested review from acatangiu and removed request for acatangiu January 25, 2022 11:48

Wizdave97 added 2 commits January 25, 2022 15:23

Merge branch 'paritytech:master' into david/beefy-block-import

0c01619

Merge branch 'paritytech:master' into david/beefy-block-import

7d58fa3

acatangiu reviewed Jan 26, 2022

View reviewed changes

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

abbe972

… david/beefy-block-import

Wizdave97 mentioned this pull request Jan 27, 2022

Implement beefy block import worker paritytech/polkadot#4791

Closed

Wizdave97 added 2 commits January 27, 2022 10:32

implemented review suggestions

34b67fc

cargo fmt

0559087

tomusdrw suggested changes Jan 27, 2022

View reviewed changes

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 26, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Feb 26, 2022

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 28, 2022

stale bot removed the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Mar 29, 2022

stale bot added the A5-stale Pull request did not receive any updates in a long time. No review needed at this stage. Close it. label Apr 28, 2022

acatangiu closed this Apr 28, 2022

	BeefyBlockImport::new(client.clone(), wrapped_block_import, justification_sender.clone());
	BeefyBlockImport::new(client.clone(), wrapped_block_import, justification_sender);

		/// Produce a BEEFY block import object and a link half for tying it to the client
		pub fn block_import<BE, Client, Block: BlockT, I>(

		/// BeefyBlockImport
		/// Wraps a type `inner` that implements [`BlockImport`]

-/// BeefyBlockImport
-/// Wraps a type `inner` that implements [`BlockImport`]
+/// A block-import handler for BEEFY.
+///
+/// This scans each imported block for BEEFY justifications and verifies them.
+/// Wraps a type `inner` that implements [`BlockImport`] and ultimately defers to it.

	pub fn find_beefy_authority_set_change<B: BlockT>(
	fn find_beefy_authority_set_change<B: BlockT>(

		@@ -0,0 +1,197 @@
		use std::{collections::HashMap, marker::PhantomData, sync::Arc};

BEEFY-specific import_justification #10699

BEEFY-specific import_justification #10699

Uh oh!

Conversation

Wizdave97 commented Jan 19, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

seunlanlege commented Jan 19, 2022

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tomusdrw left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Wizdave97 commented Jan 27, 2022

Uh oh!

stale bot commented Feb 26, 2022

Uh oh!

Wizdave97 commented Feb 26, 2022

Uh oh!

stale bot commented Mar 28, 2022

Uh oh!

Wizdave97 commented Mar 29, 2022

Uh oh!

stale bot commented Apr 28, 2022

Uh oh!

acatangiu commented Apr 28, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Wizdave97 commented Jan 19, 2022 •

edited

Loading