-
Notifications
You must be signed in to change notification settings - Fork 2.7k
BEEFY-specific import_justification #10699
Changes from all commits
01fc5f3
c7fd128
73ed135
85cd4b1
fb79ff2
c72c6b0
a06eea1
164466c
0c01619
7d58fa3
abbe972
34b67fc
0559087
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,197 @@ | ||
| use std::{collections::HashMap, marker::PhantomData, sync::Arc}; | ||
|
|
||
| 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, OpaqueDigestItemId}, | ||
| traits::{Block as BlockT, Header as HeaderT, NumberFor}, | ||
| Justification, | ||
| }; | ||
|
|
||
| use beefy_primitives::{ | ||
| crypto::{AuthorityId, Signature}, | ||
| BeefyApi, ConsensusLog, ValidatorSet, BEEFY_ENGINE_ID, | ||
| }; | ||
|
|
||
| use crate::{ | ||
| justification::decode_and_verify_justification, notification::BeefyJustificationSender, | ||
| Client as BeefyClient, | ||
| }; | ||
|
|
||
| /// Checks the given header for a consensus digest signalling a beefy authority set change | ||
| /// and extracts it. | ||
| fn find_beefy_authority_set_change<B: BlockT>( | ||
| header: &B::Header, | ||
| ) -> Option<ValidatorSet<AuthorityId>> { | ||
| let id = OpaqueDigestItemId::Consensus(&BEEFY_ENGINE_ID); | ||
|
|
||
| let filter_log = |log: ConsensusLog<AuthorityId>| match log { | ||
| ConsensusLog::AuthoritiesChange(change) => Some(change), | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This logic is duplicated in gadget code. |
||
| _ => None, | ||
| }; | ||
|
|
||
| // find the first consensus digest with the right ID which converts to | ||
| // the right kind of consensus log. | ||
| header.digest().convert_first(|l| l.try_to(id).and_then(filter_log)) | ||
| } | ||
|
|
||
| /// 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>, | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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(); | ||
| // 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); | ||
|
Comment on lines
+93
to
+95
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. What is this assumption based on?
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This was based on the mandatory blocks requirement in the docs |
||
| // 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 change.is_some() && beefy_justification.is_none() { | ||
| return Err(ConsensusError::ClientImport( | ||
| "Missing beefy justification in authority set change block".to_string(), | ||
| )) | ||
| } | ||
| if let Some(beefy_justification) = beefy_justification { | ||
| self.import_justification(hash, number, (BEEFY_ENGINE_ID, beefy_justification))?; | ||
| } | ||
acatangiu marked this conversation as resolved.
Show resolved
Hide 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 Is |
||
|
|
||
| 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
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>( | ||
| number, | ||
| &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 } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,58 @@ | ||
| use crate::keystore::BeefyKeystore; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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>( | ||
| number: NumberFor<Block>, | ||
| 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)?; | ||
|
|
||
| if verify_with_validator_set::<Block>(number, validator_set, finality_proof.clone())? { | ||
| 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>( | ||
| number: NumberFor<Block>, | ||
| validator_set: &ValidatorSet<AuthorityId>, | ||
| proof: VersionedFinalityProof<NumberFor<Block>, Signature>, | ||
| ) -> Result<bool, ConsensusError> { | ||
|
Comment on lines
+29
to
+33
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 |
||
| 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() || | ||
| signed_commitment.commitment.block_number != number | ||
| { | ||
| 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[..]) | ||
| }) | ||
| }, | ||
| }; | ||
acatangiu marked this conversation as resolved.
Show resolved
Hide resolved
|
||
|
|
||
| Ok(result) | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No preamble