Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Closed
Show file tree
Hide file tree
Changes from 3 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 3 additions & 0 deletions Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

3 changes: 3 additions & 0 deletions client/beefy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ log = "0.4"
parking_lot = "0.11"
thiserror = "1.0"
wasm-timer = "0.2.5"
async-trait = "0.1.50"

codec = { version = "2.2.0", package = "parity-scale-codec", features = ["derive"] }
prometheus = { version = "0.10.0-dev", package = "substrate-prometheus-endpoint", path = "../../utils/prometheus" }
Expand All @@ -32,6 +33,8 @@ sc-client-api = { version = "4.0.0-dev", path = "../api" }
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" }

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

nit:

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

sc-consensus = { version = "0.10.0-dev", path = "../consensus/common" }

beefy-primitives = { version = "4.0.0-dev", path = "../../primitives/beefy" }

Expand Down
163 changes: 163 additions & 0 deletions client/beefy/src/import.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,163 @@
use std::{collections::HashMap, marker::PhantomData, sync::Arc};

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

No preamble


use sc_consensus::{
BlockCheckParams, BlockImport, BlockImportParams, ImportResult, JustificationImport,
};

use sc_client_api::backend::Backend;
use sp_api::{ProvideRuntimeApi, TransactionFor};
use sp_blockchain::well_known_cache_keys;
use sp_consensus::Error as ConsensusError;
use sp_runtime::{
generic::BlockId,
traits::{Block as BlockT, Header as HeaderT, NumberFor},
Justification,
};

use beefy_primitives::{crypto::Signature, BeefyApi, BEEFY_ENGINE_ID};

use crate::{
justification::decode_and_verify_justification, notification::BeefyJustificationSender,
Client as BeefyClient,
};

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.

_phantom: PhantomData<Backend>,
}

impl<BE, Block: BlockT, Client, I: Clone> Clone for BeefyBlockImport<BE, Block, Client, I> {
fn clone(&self) -> Self {
BeefyBlockImport {
client: self.client.clone(),
inner: self.inner.clone(),
justification_sender: self.justification_sender.clone(),
_phantom: PhantomData,
}
}
}

#[async_trait::async_trait]
impl<BE, Block: BlockT, Client, I> BlockImport<Block> for BeefyBlockImport<BE, Block, Client, I>
where
BE: Backend<Block>,
I: BlockImport<
Block,
Error = ConsensusError,
Transaction = sp_api::TransactionFor<Client, Block>,
> + Send
+ Sync,
Client: BeefyClient<Block, BE>,
Client::Api: BeefyApi<Block>,
for<'a> &'a Client:
BlockImport<Block, Error = ConsensusError, Transaction = TransactionFor<Client, Block>>,
TransactionFor<Client, Block>: 'static,
{
type Error = ConsensusError;
type Transaction = TransactionFor<Client, Block>;

async fn import_block(
&mut self,
block: BlockImportParams<Block, Self::Transaction>,
new_cache: HashMap<well_known_cache_keys::Id, Vec<u8>>,
) -> Result<ImportResult, Self::Error> {
let hash = block.post_hash();
let number = *block.header.number();
let justifications = block.justifications.clone();
// Run inner block import
let import_result = self.inner.import_block(block, new_cache).await?;
// Try importing beefy justification
let beefy_justification =
justifications.and_then(|just| just.into_justification(BEEFY_ENGINE_ID));
if let Some(beefy_justification) = beefy_justification {
self.import_justification(hash, number, (BEEFY_ENGINE_ID, beefy_justification))?;
}
Comment thread
acatangiu marked this conversation as resolved.
Ok(import_result)
}

async fn check_block(
&mut self,
block: BlockCheckParams<Block>,
) -> Result<ImportResult, Self::Error> {
self.inner.check_block(block.clone()).await
}
}

#[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
}
}
Comment on lines +121 to +147

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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)?


impl<BE, Block: BlockT, Client, I> BeefyBlockImport<BE, Block, Client, I>
where
BE: Backend<Block>,
Client: BeefyClient<Block, BE> + ProvideRuntimeApi<Block>,
Client::Api: BeefyApi<Block>,
{
/// Import a block justification.
fn import_justification(
&mut self,
hash: Block::Hash,
_number: NumberFor<Block>,
justification: Justification,
) -> Result<(), ConsensusError> {
if justification.0 != BEEFY_ENGINE_ID {
return Ok(())
}

// This function assumes the Block should have already been imported
let at = BlockId::hash(hash);
let validator_set = self
.client
.runtime_api()
.validator_set(&at)
.map_err(|e| ConsensusError::ClientImport(e.to_string()))?;
Comment on lines +168 to +172

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


if let Some(validator_set) = validator_set {
let encoded_proof = justification.1;
let _proof =
decode_and_verify_justification::<Block>(&encoded_proof[..], &validator_set)?;
} else {
return Err(ConsensusError::ClientImport("Empty validator set".to_string()))
}
Ok(())
}
}

impl<BE, Block: BlockT, Client, I> BeefyBlockImport<BE, Block, Client, I> {
/// Create a new BeefyBlockImport
pub(crate) fn new(
client: Arc<Client>,
inner: I,
justification_sender: BeefyJustificationSender<NumberFor<Block>, Signature>,
) -> BeefyBlockImport<BE, Block, Client, I> {
BeefyBlockImport { inner, client, justification_sender, _phantom: PhantomData }
}
}
57 changes: 57 additions & 0 deletions client/beefy/src/justification.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,57 @@
use crate::keystore::BeefyKeystore;

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Preamble missing.

use beefy_primitives::{
crypto::{AuthorityId, Signature},
ValidatorSet, VersionedFinalityProof,
};
use codec::{Decode, Encode};
use sp_consensus::Error as ConsensusError;
use sp_runtime::traits::{Block as BlockT, NumberFor};

/// Decodes a Beefy justification and verifies it
pub(crate) fn decode_and_verify_justification<Block: BlockT>(
encoded: &[u8],
validator_set: &ValidatorSet<AuthorityId>,
) -> Result<VersionedFinalityProof<NumberFor<Block>, Signature>, ConsensusError> {
let finality_proof =
<VersionedFinalityProof<NumberFor<Block>, Signature>>::decode(&mut &*encoded)
.map_err(|_| ConsensusError::InvalidJustification)?;

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

if res {
return Ok(finality_proof)
}

Err(ConsensusError::InvalidJustification)
}

/// Verify the Beefy provided finality proof
/// against the validtor set at the block it was generated
pub(crate) fn verify_with_validator_set<Block: BlockT>(
validator_set: &ValidatorSet<AuthorityId>,
proof: VersionedFinalityProof<NumberFor<Block>, Signature>,
) -> Result<bool, ConsensusError> {
Comment on lines +29 to +33

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

let result = match proof {
VersionedFinalityProof::V1(signed_commitment) => {
if validator_set.len() != signed_commitment.signatures.len() ||
signed_commitment.commitment.validator_set_id != validator_set.id()
{
return Err(ConsensusError::InvalidJustification)
}

// Arrangement of signatures in the commitment should be in the same order as validators
// for that set
let message = signed_commitment.commitment.encode();
validator_set
.validators()
.into_iter()
.zip(signed_commitment.signatures.into_iter())
.filter(|(.., sig)| sig.is_some())
.all(|(id, signature)| {
BeefyKeystore::verify(id, signature.as_ref().unwrap(), &message[..])
})
},
};
Comment thread
acatangiu marked this conversation as resolved.

Ok(result)
}
44 changes: 37 additions & 7 deletions client/beefy/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,30 +19,48 @@
use std::sync::Arc;

use log::debug;
use notification::BeefyJustificationStream;
use prometheus::Registry;

use sc_client_api::{Backend, BlockchainEvents, Finalizer};
use sc_network_gossip::{GossipEngine, Network as GossipNetwork};

use sp_api::ProvideRuntimeApi;
use sp_api::{NumberFor, ProvideRuntimeApi};
use sp_blockchain::HeaderBackend;
use sp_keystore::SyncCryptoStorePtr;
use sp_runtime::traits::Block;
use sp_runtime::traits::Block as BlockT;

use beefy_primitives::BeefyApi;
use beefy_primitives::{crypto::Signature, BeefyApi};

use crate::notification::{BeefyBestBlockSender, BeefySignedCommitmentSender};

mod error;
mod gossip;
mod import;
mod justification;
mod keystore;
mod metrics;
mod round;
mod worker;

use import::BeefyBlockImport;

pub mod notification;
pub use beefy_protocol_name::standard_name as protocol_standard_name;

/// 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()
}
}

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

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.


pub(crate) mod beefy_protocol_name {
use sc_chain_spec::ChainSpec;

Expand Down Expand Up @@ -78,22 +96,34 @@ pub fn beefy_peers_set_config(
cfg
}

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

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

pub fn block_import<BE, Client, Block: BlockT, I>(
Comment on lines +86 to +87

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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

wrapped_block_import: I,
client: Arc<Client>,
) -> (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());

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

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


(import, LinkHalf { justification_stream })
}

/// A convenience BEEFY client trait that defines all the type bounds a BEEFY client
/// has to satisfy. Ideally that should actually be a trait alias. Unfortunately as
/// of today, Rust does not allow a type alias to be used as a trait bound. Tracking
/// issue is <https://github.com/rust-lang/rust/issues/41517>.
pub trait Client<B, BE>:
BlockchainEvents<B> + HeaderBackend<B> + Finalizer<B, BE> + ProvideRuntimeApi<B> + Send + Sync
where
B: Block,
B: BlockT,
BE: Backend<B>,
{
// empty
}

impl<B, BE, T> Client<B, BE> for T
where
B: Block,
B: BlockT,
BE: Backend<B>,
T: BlockchainEvents<B>
+ HeaderBackend<B>
Expand All @@ -108,7 +138,7 @@ where
/// BEEFY gadget initialization parameters.
pub struct BeefyParams<B, BE, C, N>
where
B: Block,
B: BlockT,
BE: Backend<B>,
C: Client<B, BE>,
C::Api: BeefyApi<B>,
Expand Down Expand Up @@ -139,7 +169,7 @@ where
/// This is a thin shim around running and awaiting a BEEFY worker.
pub async fn start_beefy_gadget<B, BE, C, N>(beefy_params: BeefyParams<B, BE, C, N>)
where
B: Block,
B: BlockT,
BE: Backend<B>,
C: Client<B, BE>,
C::Api: BeefyApi<B>,
Expand Down
Loading