Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 11 commits
Commits
Show all changes
25 commits
Select commit Hold shift + click to select a range
c4efa5f
client/beefy: don't accept vote for older rounds
acatangiu Jul 8, 2022
934f14c
client/beefy: clean up and reorg the worker struct
acatangiu Jul 7, 2022
2f1f221
client/beefy: first step towards Full BEEFY
acatangiu Jul 11, 2022
d732f82
client/beefy: sketch idea for block import and sync
acatangiu Jul 12, 2022
b70b9be
client/beefy: add BEEFY block import
acatangiu Jul 12, 2022
1d0298c
client/beefy: process justifications from block import
acatangiu Jul 13, 2022
6d381db
client/beefy: add TODOs for sync protocol
acatangiu Jul 13, 2022
739898a
client/beefy: add more docs and comments
acatangiu Jul 13, 2022
abfd074
client/beefy-rpc: fix RPC error
acatangiu Jul 13, 2022
46de909
client/beefy: verify justification validity on block import
acatangiu Jul 14, 2022
efe0157
client/beefy: more tests
acatangiu Jul 14, 2022
e129702
client/beefy: small fixes
acatangiu Jul 15, 2022
0540091
client/beefy: remove invalid justifications at block import
acatangiu Jul 15, 2022
2c16427
todo: beefy block import tests
acatangiu Jul 18, 2022
8256fb0
RFC: ideas for multiple justifications per block
acatangiu Jul 18, 2022
d5b53ae
Revert "RFC: ideas for multiple justifications per block"
acatangiu Jul 19, 2022
bc6a187
client/beefy: append justif to backend on block import
acatangiu Jul 19, 2022
73355d0
client/beefy: groundwork for block import test
acatangiu Jul 18, 2022
d5f9f1a
client/beefy: groundwork2 for block import test
acatangiu Jul 19, 2022
ff02b1a
client/beefy: groundwork3 for block import test
acatangiu Jul 19, 2022
1c49f49
client/beefy: add block import test
acatangiu Jul 19, 2022
9761f89
Merge branch 'master' of github.com:paritytech/substrate into beefy-i…
acatangiu Jul 20, 2022
ecf625b
client/beefy: add required trait bounds to block import builder
acatangiu Jul 20, 2022
0d843e0
remove client from beefy block import, backend gets the job done
acatangiu Jul 29, 2022
581b5ab
Merge branch 'master' of github.com:paritytech/substrate into beefy-i…
acatangiu Jul 29, 2022
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
1 change: 1 addition & 0 deletions Cargo.lock

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

3 changes: 2 additions & 1 deletion client/beefy/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -9,6 +9,7 @@ description = "BEEFY Client gadget for substrate"
homepage = "https://substrate.io"

[dependencies]
async-trait = "0.1.50"
codec = { package = "parity-scale-codec", version = "3.0.0", features = ["derive"] }
fnv = "1.0.6"
futures = "0.3"
Expand All @@ -22,6 +23,7 @@ beefy-primitives = { version = "4.0.0-dev", path = "../../primitives/beefy" }
prometheus = { package = "substrate-prometheus-endpoint", version = "0.10.0-dev", path = "../../utils/prometheus" }
sc-chain-spec = { version = "4.0.0-dev", path = "../../client/chain-spec" }
sc-client-api = { version = "4.0.0-dev", path = "../api" }
sc-consensus = { version = "0.10.0-dev", path = "../consensus/common" }
sc-finality-grandpa = { version = "0.10.0-dev", path = "../../client/finality-grandpa" }
sc-keystore = { version = "4.0.0-dev", path = "../keystore" }
sc-network = { version = "0.10.0-dev", path = "../network" }
Expand All @@ -42,7 +44,6 @@ serde = "1.0.136"
strum = { version = "0.23", features = ["derive"] }
tempfile = "3.1.0"
tokio = "1.17.0"
sc-consensus = { version = "0.10.0-dev", path = "../consensus/common" }
sc-network-test = { version = "0.8.0", path = "../network/test" }
sp-finality-grandpa = { version = "4.0.0-dev", path = "../../primitives/finality-grandpa" }
sp-keyring = { version = "6.0.0", path = "../../primitives/keyring" }
Expand Down
5 changes: 3 additions & 2 deletions client/beefy/rpc/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -164,8 +164,9 @@ where
mod tests {
use super::*;

use beefy_gadget::notification::{
BeefyBestBlockStream, BeefySignedCommitment, BeefySignedCommitmentSender,
use beefy_gadget::{
justification::BeefySignedCommitment,
notification::{BeefyBestBlockStream, BeefySignedCommitmentSender},
};
use beefy_primitives::{known_payload_ids, Payload};
use codec::{Decode, Encode};
Expand Down
2 changes: 1 addition & 1 deletion client/beefy/rpc/src/notification.rs
Original file line number Diff line number Diff line change
Expand Up @@ -29,7 +29,7 @@ pub struct EncodedSignedCommitment(sp_core::Bytes);

impl EncodedSignedCommitment {
pub fn new<Block>(
signed_commitment: beefy_gadget::notification::BeefySignedCommitment<Block>,
signed_commitment: beefy_gadget::justification::BeefySignedCommitment<Block>,
) -> Self
where
Block: BlockT,
Expand Down
4 changes: 4 additions & 0 deletions client/beefy/src/error.rs
Original file line number Diff line number Diff line change
Expand Up @@ -24,8 +24,12 @@ use std::fmt::Debug;

#[derive(Debug, thiserror::Error, PartialEq)]
pub enum Error {
#[error("Backend: {0}")]
Backend(String),
#[error("Keystore error: {0}")]
Keystore(String),
#[error("Signature error: {0}")]
Signature(String),
#[error("Session uninitialized")]
UninitSession,
}
141 changes: 141 additions & 0 deletions client/beefy/src/import.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
// This file is part of Substrate.

// Copyright (C) 2021-2022 Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0

// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use std::{collections::HashMap, marker::PhantomData, sync::Arc};

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},
};

use sc_client_api::backend::Backend;
use sc_consensus::{BlockCheckParams, BlockImport, BlockImportParams, ImportResult};

use crate::{
justification::decode_and_verify_commitment, notification::BeefySignedCommitmentSender,
Client as BeefyClient,
};
use beefy_primitives::{BeefyApi, VersionedFinalityProof, BEEFY_ENGINE_ID};

/// A block-import handler for BEEFY.
///
/// This scans each imported block for BEEFY justifications and verifies them.
/// Wraps a `inner: BlockImport` and ultimately defers to it.
///
/// When using BEEFY, the block import worker should be using this block import object.
pub struct BeefyBlockImport<Backend, Block: BlockT, Client, I> {
client: Arc<Client>,
inner: I,
justification_sender: BeefySignedCommitmentSender<Block>,
_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,
}
}
}

impl<BE, Block: BlockT, Client, I> BeefyBlockImport<BE, Block, Client, I> {
/// Create a new BeefyBlockImport.
pub fn new(
client: Arc<Client>,
inner: I,
justification_sender: BeefySignedCommitmentSender<Block>,
) -> BeefyBlockImport<BE, Block, Client, I> {
BeefyBlockImport { inner, client, justification_sender, _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> + ProvideRuntimeApi<Block>,
Client::Api: BeefyApi<Block>,
{
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 beefy_justification = block
.justifications
.as_ref()
.and_then(|just| just.get(BEEFY_ENGINE_ID).cloned());

// Run inner block import.
let import_result = self.inner.import_block(block, new_cache).await?;

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.

The inner here would be (or include) grandpa block import, right? If so, then iiuc in its current implementation (here, here and here) it won't pass BEEFY justification to the client => it won't be stored in the database. Am I missing something, or it isn't a part of this PR?

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.

Another q: if justification is supposed to be saved in this call, then what if it is invalid? Since it is checked after this line, it may be invalid, but it still be saved to the db during this call, 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.

Yes, inner here will be (or in the future include) grandpa block import.

it won't pass BEEFY justification to the client => it won't be stored in the database

I think you're right, I didn't realize the client will need it as well for storing in the db.
I cloned the beefy just before passing it down the line exactly because I saw that grandpa can take/consume all of them, so the BeefyWorker does get the justification, but the client backend+db might not.

Let's see what @andresilva thinks here before I start hacking the grandpa code.

if justification is supposed to be saved in this call, then what if it is invalid?

Also good catch! I guess as long as we filter before BeefyWorker it still works, but filtering out invalid justif early avoids us caching and forwarding invalid justifications. I will fix this.

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.

filtering out invalid justif early avoids us caching and forwarding invalid justifications. I will fix this.

done

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.

because we don't want to tie BEEFY with GRANDPA logic:

  1. the BEEFY justification is removed from list going downstream,
  2. once all inner finish successfully, justification is manually appended to backend if it is valid.


if let Some(justification) = beefy_justification {
if let ImportResult::Imported(_) = &import_result {
let block_id = BlockId::hash(hash);
let validator_set = self
.client
.runtime_api()
.validator_set(&block_id)
.map_err(|e| ConsensusError::ClientImport(e.to_string()))?
// Deploying `BeefyBlockImport` on chains with dummy BeefyApi will

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.

Dumb q: is it really rejecting such blocks, or just ignore BEEFY justifications? I'm asking because inner.import_block() has been (successfully) called already and here we just returning error here. Sorry - many things have changed in block import since I've last checked it, so maybe there's some transactional mechanism here, but I haven't found it.

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.

Also (I believe you've said about that yesterday) - you're going to implement mandatory justifications requests in follow-up PRs, right? There's still needs_justification field in the import result - it won't be used in BEEFY pipeline, 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.

I thought it actually rejected the block, even if inner components have accepted/imported it. I'll look into it.

you're going to implement mandatory justifications requests in follow-up PRs, right? There's still needs_justification field in the import result - it won't be used in BEEFY pipeline, right?

Right, we'll have our own on-demand justifications protocol, so we won't be using needs_justification (or any of the client-sync code I believe). I believe @andresilva has more details here.

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.

So the block is still imported, only an error log and event are emitted:

Err(err) => {
warn!(
target: logging_target,
"Error with block built on {:?}: {}", parent_hash, err,
);
telemetry!(
telemetry;
CONSENSUS_WARN;
"slots.err_with_block_built_on";
"hash" => ?parent_hash,
"err" => ?err,
);
},

I'll remove the incorrect comment in beefy block import.

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.

Reworked the block import logic now to:

  • check validity early
  • remove BEEFY justification before passing into inner, and manually add it to backend if all layers successful
  • pass justif to worker only if all inner block imports are successful
  • removed the incorrect comment around block not being imported in case of bad justification

I tried to split work in self-sufficient commits for easier review.

// effectively reject all blocks with BEEFY justifications.
.ok_or_else(|| {
ConsensusError::ClientImport("Unknown validator set".to_string())
})?;

match decode_and_verify_commitment::<Block>(
&justification[..],
number,
&validator_set,
)? {
// TODO: these channels should also use `VersionedFinalityProof`.
VersionedFinalityProof::V1(signed_commitment) => self
.justification_sender
.notify(|| Ok::<_, ()>(signed_commitment))
.expect("forwards closure result; the closure always returns Ok; qed."),
};
}
}

Ok(import_result)
}

async fn check_block(
&mut self,
block: BlockCheckParams<Block>,
) -> Result<ImportResult, Self::Error> {
self.inner.check_block(block).await
}
}
177 changes: 177 additions & 0 deletions client/beefy/src/justification.rs
Original file line number Diff line number Diff line change
@@ -0,0 +1,177 @@
// This file is part of Substrate.

// Copyright (C) 2021-2022 Parity Technologies (UK) Ltd.
// SPDX-License-Identifier: GPL-3.0-or-later WITH Classpath-exception-2.0

// This program is free software: you can redistribute it and/or modify
// it under the terms of the GNU General Public License as published by
// the Free Software Foundation, either version 3 of the License, or
// (at your option) any later version.

// This program is distributed in the hope that it will be useful,
// but WITHOUT ANY WARRANTY; without even the implied warranty of
// MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
// GNU General Public License for more details.

// You should have received a copy of the GNU General Public License
// along with this program. If not, see <https://www.gnu.org/licenses/>.

use crate::keystore::BeefyKeystore;
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};

/// A commitment with matching BEEFY authorities' signatures.
pub type BeefySignedCommitment<Block> =
beefy_primitives::SignedCommitment<NumberFor<Block>, beefy_primitives::crypto::Signature>;

/// Decode and verify a Beefy SignedCommitment.
pub(crate) fn decode_and_verify_commitment<Block: BlockT>(
encoded: &[u8],
target_number: NumberFor<Block>,
validator_set: &ValidatorSet<AuthorityId>,
) -> Result<VersionedFinalityProof<NumberFor<Block>, Signature>, ConsensusError> {
let proof = <VersionedFinalityProof<NumberFor<Block>, Signature>>::decode(&mut &*encoded)
.map_err(|_| ConsensusError::InvalidJustification)?;
verify_with_validator_set::<Block>(target_number, validator_set, &proof).map(|_| proof)
}

/// Verify the Beefy finality proof against the validator set at the block it was generated.
fn verify_with_validator_set<Block: BlockT>(
target_number: NumberFor<Block>,
validator_set: &ValidatorSet<AuthorityId>,
proof: &VersionedFinalityProof<NumberFor<Block>, Signature>,
) -> Result<(), ConsensusError> {
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 != target_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();
let valid_signatures = validator_set
.validators()
.into_iter()
.zip(signed_commitment.signatures.iter())
.filter(|(id, signature)| {
signature
.as_ref()
.map(|sig| BeefyKeystore::verify(id, sig, &message[..]))
.unwrap_or(false)
})
.count();
if valid_signatures >= crate::round::threshold(validator_set.len()) {
Ok(())
} else {
Err(ConsensusError::InvalidJustification)
}
},
}
}

#[cfg(test)]
pub(crate) mod tests {
use beefy_primitives::{known_payload_ids, Commitment, Payload, SignedCommitment};
use substrate_test_runtime_client::runtime::Block;

use super::*;
use crate::{keystore::tests::Keyring, tests::make_beefy_ids};

pub(crate) fn new_signed_commitment(
block_num: NumberFor<Block>,
validator_set: &ValidatorSet<AuthorityId>,
keys: &[Keyring],
) -> BeefySignedCommitment<Block> {
let commitment = Commitment {
payload: Payload::new(known_payload_ids::MMR_ROOT_ID, vec![]),
block_number: block_num,
validator_set_id: validator_set.id(),
};
let message = commitment.encode();
let signatures = keys.iter().map(|key| Some(key.sign(&message))).collect();
SignedCommitment { commitment, signatures }
}

#[test]
fn should_verify_with_validator_set() {
let keys = &[Keyring::Alice, Keyring::Bob, Keyring::Charlie];
let validator_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap();

// build valid justification
let block_num = 42;
let proof = new_signed_commitment(block_num, &validator_set, keys);

let good_proof = proof.clone().into();
// should verify successfully
verify_with_validator_set::<Block>(block_num, &validator_set, &good_proof).unwrap();

// wrong block number -> should fail verification
let good_proof = proof.clone().into();
match verify_with_validator_set::<Block>(block_num + 1, &validator_set, &good_proof) {
Err(ConsensusError::InvalidJustification) => (),
_ => assert!(false, "Expected Err(ConsensusError::InvalidJustification)"),
};

// wrong validator set id -> should fail verification
let good_proof = proof.clone().into();
let other = ValidatorSet::new(make_beefy_ids(keys), 1).unwrap();
match verify_with_validator_set::<Block>(block_num, &other, &good_proof) {
Err(ConsensusError::InvalidJustification) => (),
_ => assert!(false, "Expected Err(ConsensusError::InvalidJustification)"),
};

// wrong signatures length -> should fail verification
let mut bad_proof = proof.clone();
// change length of signatures
bad_proof.signatures.pop().flatten().unwrap();
match verify_with_validator_set::<Block>(block_num + 1, &validator_set, &bad_proof.into()) {
Err(ConsensusError::InvalidJustification) => (),
_ => assert!(false, "Expected Err(ConsensusError::InvalidJustification)"),
};

// not enough signatures -> should fail verification
let mut bad_proof = proof.clone();
// remove a signature (but same length)
*bad_proof.signatures.first_mut().unwrap() = None;
match verify_with_validator_set::<Block>(block_num + 1, &validator_set, &bad_proof.into()) {
Err(ConsensusError::InvalidJustification) => (),
_ => assert!(false, "Expected Err(ConsensusError::InvalidJustification)"),
};

// not enough _correct_ signatures -> should fail verification
let mut bad_proof = proof.clone();
// change a signature to a different key
*bad_proof.signatures.first_mut().unwrap() =
Some(Keyring::Dave.sign(&proof.commitment.encode()));
match verify_with_validator_set::<Block>(block_num + 1, &validator_set, &bad_proof.into()) {
Err(ConsensusError::InvalidJustification) => (),
_ => assert!(false, "Expected Err(ConsensusError::InvalidJustification)"),
};
}

#[test]
fn should_decode_and_verify_commitment() {
let keys = &[Keyring::Alice, Keyring::Bob];
let validator_set = ValidatorSet::new(make_beefy_ids(keys), 0).unwrap();
let block_num = 1;

// build valid justification
let proof = new_signed_commitment(block_num, &validator_set, keys);
let versioned_proof: VersionedFinalityProof<NumberFor<Block>, Signature> = proof.into();
let encoded = versioned_proof.encode();

// should successfully decode and verify
let verified =
decode_and_verify_commitment::<Block>(&encoded, block_num, &validator_set).unwrap();
assert_eq!(verified, versioned_proof);
}
}
Loading