Use GrandpaJustification instead of Vec<u8> in Pallet API (#847)

* Stop passing raw encoded justifications to pallet API

By having the API accept a struct-ified justification we are able to
better utilize the justifications fields for weight calculations.

* Update relayer code to use decoded justifications

* Add justification to `expect()` statement

* Fix some imports

* Make justification wrapper contain decoded justification

* Rename some fields

* Get rid of warnings

* Appease Clippy

* Only decode justification once at init time

* Remove unnecessary method

* Remove justification wrapper

This became kinda unnecessary since we could implement the FinalityProof
trait on GrandpaJustification directly.
This commit is contained in:
Hernando Castano
2021-04-01 07:08:28 -04:00
committed by Bastian Köcher
parent 904b9f4da5
commit 67cdca8aa4
14 changed files with 92 additions and 117 deletions
+15 -16
View File
@@ -36,6 +36,7 @@
// Runtime-generated enums
#![allow(clippy::large_enum_variant)]
use bp_header_chain::justification::GrandpaJustification;
use bp_runtime::{BlockNumberOf, Chain, HashOf, HasherOf, HeaderOf};
use codec::{Decode, Encode};
use finality_grandpa::voter_set::VoterSet;
@@ -46,7 +47,6 @@ use serde::{Deserialize, Serialize};
use sp_finality_grandpa::{ConsensusLog, GRANDPA_ENGINE_ID};
use sp_runtime::traits::{BadOrigin, Header as HeaderT, Zero};
use sp_runtime::RuntimeDebug;
use sp_std::vec::Vec;
#[cfg(test)]
mod mock;
@@ -111,7 +111,7 @@ pub mod pallet {
pub fn submit_finality_proof(
origin: OriginFor<T>,
finality_target: BridgedHeader<T, I>,
justification: Vec<u8>,
justification: GrandpaJustification<BridgedHeader<T, I>>,
) -> DispatchResultWithPostInfo {
ensure_operational::<T, I>()?;
let _ = ensure_signed(origin)?;
@@ -359,7 +359,7 @@ pub mod pallet {
///
/// Will use the GRANDPA current authorities known to the pallet.
pub(crate) fn verify_justification<T: Config<I>, I: 'static>(
justification: &[u8],
justification: &GrandpaJustification<BridgedHeader<T, I>>,
hash: BridgedBlockHash<T, I>,
number: BridgedBlockNumber<T, I>,
) -> Result<(), sp_runtime::DispatchError> {
@@ -515,7 +515,7 @@ pub(crate) fn find_forced_change<H: HeaderT>(
pub fn initialize_for_benchmarks<T: Config<I>, I: 'static>(header: BridgedHeader<T, I>) {
initialize_bridge::<T, I>(InitializationData {
header,
authority_list: Vec::new(), // we don't verify any proofs in external benchmarks
authority_list: sp_std::vec::Vec::new(), // we don't verify any proofs in external benchmarks
set_id: 0,
is_halted: false,
});
@@ -555,7 +555,7 @@ mod tests {
fn submit_finality_proof(header: u8) -> frame_support::dispatch::DispatchResultWithPostInfo {
let header = test_header(header.into());
let justification = make_default_justification(&header).encode();
let justification = make_default_justification(&header);
Module::<TestRuntime>::submit_finality_proof(Origin::signed(1), header, justification)
}
@@ -701,10 +701,7 @@ mod tests {
run_test(|| {
<IsHalted<TestRuntime>>::put(true);
assert_noop!(
Module::<TestRuntime>::submit_finality_proof(Origin::signed(1), test_header(1), vec![]),
Error::<TestRuntime>::Halted,
);
assert_noop!(submit_finality_proof(1), Error::<TestRuntime>::Halted,);
})
}
@@ -731,7 +728,7 @@ mod tests {
set_id: 2,
..Default::default()
};
let justification = make_justification_for_header(params).encode();
let justification = make_justification_for_header(params);
assert_err!(
Module::<TestRuntime>::submit_finality_proof(Origin::signed(1), header, justification,),
@@ -746,7 +743,8 @@ mod tests {
initialize_substrate_bridge();
let header = test_header(1);
let justification = [1u8; 32].encode();
let mut justification = make_default_justification(&header);
justification.round = 42;
assert_err!(
Module::<TestRuntime>::submit_finality_proof(Origin::signed(1), header, justification,),
@@ -771,7 +769,7 @@ mod tests {
assert_ok!(Module::<TestRuntime>::initialize(Origin::root(), init_data));
let header = test_header(1);
let justification = [1u8; 32].encode();
let justification = make_default_justification(&header);
assert_err!(
Module::<TestRuntime>::submit_finality_proof(Origin::signed(1), header, justification,),
@@ -805,7 +803,7 @@ mod tests {
header.digest = change_log(0);
// Create a valid justification for the header
let justification = make_default_justification(&header).encode();
let justification = make_default_justification(&header);
// Let's import our test header
assert_ok!(Module::<TestRuntime>::submit_finality_proof(
@@ -837,7 +835,7 @@ mod tests {
header.digest = change_log(1);
// Create a valid justification for the header
let justification = make_default_justification(&header).encode();
let justification = make_default_justification(&header);
// Should not be allowed to import this header
assert_err!(
@@ -858,7 +856,7 @@ mod tests {
header.digest = forced_change_log(0);
// Create a valid justification for the header
let justification = make_default_justification(&header).encode();
let justification = make_default_justification(&header);
// Should not be allowed to import this header
assert_err!(
@@ -917,7 +915,8 @@ mod tests {
run_test(|| {
let submit_invalid_request = || {
let header = test_header(1);
let invalid_justification = vec![4, 2, 4, 2].encode();
let mut invalid_justification = make_default_justification(&header);
invalid_justification.round = 42;
Module::<TestRuntime>::submit_finality_proof(Origin::signed(1), header, invalid_justification)
};
+7 -1
View File
@@ -215,6 +215,8 @@ where
/// proof. If the header enacts an authority set change the change will be applied once the
/// header has been finalized.
pub fn import_finality_proof(&mut self, hash: H::Hash, proof: FinalityProof) -> Result<(), FinalizationError> {
use codec::Decode;
// Make sure that we've previously imported this header
let header = self
.storage
@@ -233,11 +235,15 @@ where
"We verified the correctness of the authority list during header import,
before writing them to storage. This must always be valid.",
);
let justification = bp_header_chain::justification::GrandpaJustification::<H>::decode(&mut proof.0.as_slice())
.map_err(|_| FinalizationError::InvalidJustification)?;
verify_justification::<H>(
(hash, *header.number()),
current_authority_set.set_id,
&voter_set,
&proof.0,
&justification,
)
.map_err(|_| FinalizationError::InvalidJustification)?;
log::trace!("Received valid justification for {:?}", header);
@@ -25,7 +25,7 @@ use frame_support::RuntimeDebug;
use sp_finality_grandpa::{AuthorityId, AuthoritySignature, SetId};
use sp_runtime::traits::Header as HeaderT;
use sp_std::collections::{btree_map::BTreeMap, btree_set::BTreeSet};
use sp_std::prelude::Vec;
use sp_std::prelude::*;
/// Justification verification error.
#[derive(RuntimeDebug, PartialEq)]
@@ -58,15 +58,11 @@ pub fn verify_justification<Header: HeaderT>(
finalized_target: (Header::Hash, Header::Number),
authorities_set_id: SetId,
authorities_set: &VoterSet<AuthorityId>,
raw_justification: &[u8],
justification: &GrandpaJustification<Header>,
) -> Result<(), Error>
where
Header::Number: finality_grandpa::BlockNumberOps,
{
// Decode justification first
let justification =
GrandpaJustification::<Header>::decode(&mut &*raw_justification).map_err(|_| Error::JustificationDecode)?;
// Ensure that it is justification for the expected header
if (justification.commit.target_hash, justification.commit.target_number) != finalized_target {
return Err(Error::InvalidJustificationTarget);
@@ -130,7 +126,7 @@ where
///
/// This particular proof is used to prove that headers on a bridged chain
/// (so not our chain) have been finalized correctly.
#[derive(Encode, Decode, RuntimeDebug)]
#[derive(Encode, Decode, RuntimeDebug, Clone, PartialEq, Eq)]
pub struct GrandpaJustification<Header: HeaderT> {
/// The round (voting period) this justification is valid for.
pub round: u64,
@@ -140,6 +136,12 @@ pub struct GrandpaJustification<Header: HeaderT> {
pub votes_ancestries: Vec<Header>,
}
impl<H: HeaderT> crate::FinalityProof<H::Number> for GrandpaJustification<H> {
fn target_header_number(&self) -> H::Number {
self.commit.target_number
}
}
/// A utility trait implementing `finality_grandpa::Chain` using a given set of headers.
#[derive(RuntimeDebug)]
struct AncestryChain<Header: HeaderT> {
@@ -94,6 +94,12 @@ impl<H: Default, E> HeaderChain<H, E> for () {
}
}
/// Abstract finality proof that is justifying block finality.
pub trait FinalityProof<Number>: Clone + Send + Sync + Debug {
/// Return number of header that this proof is generated for.
fn target_header_number(&self) -> Number;
}
/// Find header digest that schedules next GRANDPA authorities set.
pub fn find_grandpa_authorities_scheduled_change<H: HeaderT>(
header: &H,
@@ -18,7 +18,6 @@
use bp_header_chain::justification::{verify_justification, Error};
use bp_test_utils::*;
use codec::Encode;
type TestHeader = sp_runtime::testing::Header;
@@ -38,7 +37,7 @@ fn valid_justification_accepted() {
header_id::<TestHeader>(1),
TEST_GRANDPA_SET_ID,
&voter_set(),
&make_justification_for_header::<TestHeader>(params).encode()
&make_justification_for_header::<TestHeader>(params)
),
Ok(()),
);
@@ -60,7 +59,7 @@ fn valid_justification_accepted_with_single_fork() {
header_id::<TestHeader>(1),
TEST_GRANDPA_SET_ID,
&voter_set(),
&make_justification_for_header::<TestHeader>(params).encode()
&make_justification_for_header::<TestHeader>(params)
),
Ok(()),
);
@@ -94,20 +93,12 @@ fn valid_justification_accepted_with_arbitrary_number_of_authorities() {
header_id::<TestHeader>(1),
TEST_GRANDPA_SET_ID,
&voter_set,
&make_justification_for_header::<TestHeader>(params).encode()
&make_justification_for_header::<TestHeader>(params)
),
Ok(()),
);
}
#[test]
fn justification_with_invalid_encoding_rejected() {
assert_eq!(
verify_justification::<TestHeader>(header_id::<TestHeader>(1), TEST_GRANDPA_SET_ID, &voter_set(), &[],),
Err(Error::JustificationDecode),
);
}
#[test]
fn justification_with_invalid_target_rejected() {
assert_eq!(
@@ -115,7 +106,7 @@ fn justification_with_invalid_target_rejected() {
header_id::<TestHeader>(2),
TEST_GRANDPA_SET_ID,
&voter_set(),
&make_default_justification::<TestHeader>(&test_header(1)).encode(),
&make_default_justification::<TestHeader>(&test_header(1)),
),
Err(Error::InvalidJustificationTarget),
);
@@ -131,7 +122,7 @@ fn justification_with_invalid_commit_rejected() {
header_id::<TestHeader>(1),
TEST_GRANDPA_SET_ID,
&voter_set(),
&justification.encode(),
&justification,
),
Err(Error::InvalidJustificationCommit),
);
@@ -147,7 +138,7 @@ fn justification_with_invalid_authority_signature_rejected() {
header_id::<TestHeader>(1),
TEST_GRANDPA_SET_ID,
&voter_set(),
&justification.encode(),
&justification,
),
Err(Error::InvalidAuthoritySignature),
);
@@ -163,7 +154,7 @@ fn justification_with_invalid_precommit_ancestry() {
header_id::<TestHeader>(1),
TEST_GRANDPA_SET_ID,
&voter_set(),
&justification.encode(),
&justification,
),
Err(Error::InvalidPrecommitAncestries),
);
@@ -186,7 +177,7 @@ fn justification_is_invalid_if_we_dont_meet_threshold() {
header_id::<TestHeader>(1),
TEST_GRANDPA_SET_ID,
&voter_set(),
&make_justification_for_header::<TestHeader>(params).encode()
&make_justification_for_header::<TestHeader>(params)
),
Err(Error::InvalidJustificationCommit),
);
@@ -17,7 +17,6 @@
//! Logic for checking Substrate storage proofs.
use hash_db::{HashDB, Hasher, EMPTY_PREFIX};
use sp_core::H256;
use sp_runtime::RuntimeDebug;
use sp_std::vec::Vec;
use sp_trie::{read_trie_value, Layout, MemoryDB, StorageProof};
@@ -67,7 +66,7 @@ pub enum Error {
///
/// NOTE: This should only be used for **testing**.
#[cfg(feature = "std")]
pub fn craft_valid_storage_proof() -> (H256, StorageProof) {
pub fn craft_valid_storage_proof() -> (sp_core::H256, StorageProof) {
use sp_state_machine::{backend::Backend, prove_read, InMemoryBackend};
// construct storage proof
@@ -106,7 +105,7 @@ pub mod tests {
// checking proof against invalid commitment fails
assert_eq!(
<StorageProofChecker<sp_core::Blake2Hasher>>::new(H256::random(), proof).err(),
<StorageProofChecker<sp_core::Blake2Hasher>>::new(sp_core::H256::random(), proof).err(),
Some(Error::StorageRootMismatch)
);
}
@@ -18,11 +18,9 @@
use crate::finality_target::SubstrateFinalityTarget;
use bp_header_chain::justification::GrandpaJustification;
use finality_relay::{FinalitySyncParams, FinalitySyncPipeline};
use relay_substrate_client::{
finality_source::{FinalitySource, Justification},
BlockNumberOf, Chain, Client, HashOf, SyncHeader,
};
use relay_substrate_client::{finality_source::FinalitySource, BlockNumberOf, Chain, Client, HashOf, SyncHeader};
use relay_utils::BlockNumberBase;
use sp_core::Bytes;
use std::{fmt::Debug, marker::PhantomData, time::Duration};
@@ -101,7 +99,7 @@ where
type Hash = HashOf<SourceChain>;
type Number = BlockNumberOf<SourceChain>;
type Header = SyncHeader<SourceChain::Header>;
type FinalityProof = Justification<SourceChain::BlockNumber>;
type FinalityProof = GrandpaJustification<SourceChain::Header>;
}
/// Run Substrate-to-Substrate finality sync.
@@ -116,7 +114,7 @@ where
Hash = HashOf<SourceChain>,
Number = BlockNumberOf<SourceChain>,
Header = SyncHeader<SourceChain::Header>,
FinalityProof = Justification<SourceChain::BlockNumber>,
FinalityProof = GrandpaJustification<SourceChain::Header>,
TargetChain = TargetChain,
>,
SourceChain: Clone + Chain,
@@ -23,7 +23,7 @@
use bp_header_chain::{
find_grandpa_authorities_scheduled_change,
justification::{decode_justification_target, verify_justification},
justification::{verify_justification, GrandpaJustification},
};
use codec::Decode;
use finality_grandpa::voter_set::VoterSet;
@@ -116,9 +116,12 @@ async fn prepare_initialization_data<SourceChain: Chain>(
})?;
// Read initial header.
let justification: GrandpaJustification<SourceChain::Header> = Decode::decode(&mut &justification.0[..])
.map_err(|err| format!("Failed to decode {} justification: {:?}", SourceChain::NAME, err))?;
let (initial_header_hash, initial_header_number) =
decode_justification_target::<SourceChain::Header>(&justification.0)
.map_err(|err| format!("Failed to decode {} justification: {:?}", SourceChain::NAME, err))?;
(justification.commit.target_hash, justification.commit.target_number);
let initial_header = source_header(&source_client, initial_header_hash).await?;
log::trace!(target: "bridge", "Selected {} initial header: {}/{}",
SourceChain::NAME,
@@ -176,9 +179,10 @@ async fn prepare_initialization_data<SourceChain: Chain>(
(initial_header_hash, initial_header_number),
initial_authorities_set_id,
&authorities_for_verification,
&justification.0,
&justification,
)
.is_ok();
if is_valid_set_id {
break;
}
@@ -18,10 +18,11 @@
use crate::finality_pipeline::{SubstrateFinalitySyncPipeline, SubstrateFinalityToSubstrate};
use bp_header_chain::justification::GrandpaJustification;
use codec::Encode;
use relay_millau_client::{Millau, SyncHeader as MillauSyncHeader};
use relay_rialto_client::{Rialto, SigningParams as RialtoSigningParams};
use relay_substrate_client::{finality_source::Justification, Chain, TransactionSignScheme};
use relay_substrate_client::{Chain, TransactionSignScheme};
use sp_core::{Bytes, Pair};
/// Millau-to-Rialto finality sync pipeline.
@@ -40,11 +41,9 @@ impl SubstrateFinalitySyncPipeline for MillauFinalityToRialto {
&self,
transaction_nonce: <Rialto as Chain>::Index,
header: MillauSyncHeader,
proof: Justification<bp_millau::BlockNumber>,
proof: GrandpaJustification<bp_millau::Header>,
) -> Bytes {
let call =
rialto_runtime::BridgeGrandpaMillauCall::submit_finality_proof(header.into_inner(), proof.into_inner())
.into();
let call = rialto_runtime::BridgeGrandpaMillauCall::submit_finality_proof(header.into_inner(), proof).into();
let genesis_hash = *self.target_client.genesis_hash();
let transaction = Rialto::sign_transaction(genesis_hash, &self.target_sign, transaction_nonce, call);
@@ -18,10 +18,11 @@
use crate::finality_pipeline::{SubstrateFinalitySyncPipeline, SubstrateFinalityToSubstrate};
use bp_header_chain::justification::GrandpaJustification;
use codec::Encode;
use relay_millau_client::{Millau, SigningParams as MillauSigningParams};
use relay_rialto_client::{Rialto, SyncHeader as RialtoSyncHeader};
use relay_substrate_client::{finality_source::Justification, Chain, TransactionSignScheme};
use relay_substrate_client::{Chain, TransactionSignScheme};
use sp_core::{Bytes, Pair};
/// Rialto-to-Millau finality sync pipeline.
@@ -40,12 +41,12 @@ impl SubstrateFinalitySyncPipeline for RialtoFinalityToMillau {
&self,
transaction_nonce: <Millau as Chain>::Index,
header: RialtoSyncHeader,
proof: Justification<bp_rialto::BlockNumber>,
proof: GrandpaJustification<bp_rialto::Header>,
) -> Bytes {
let call = millau_runtime::BridgeGrandpaRialtoCall::<
millau_runtime::Runtime,
millau_runtime::RialtoGrandpaInstance,
>::submit_finality_proof(header.into_inner(), proof.into_inner())
>::submit_finality_proof(header.into_inner(), proof)
.into();
let genesis_hash = *self.target_client.genesis_hash();
@@ -18,9 +18,10 @@
use crate::finality_pipeline::{SubstrateFinalitySyncPipeline, SubstrateFinalityToSubstrate};
use bp_header_chain::justification::GrandpaJustification;
use codec::Encode;
use relay_millau_client::{Millau, SigningParams as MillauSigningParams};
use relay_substrate_client::{finality_source::Justification, Chain, TransactionSignScheme};
use relay_substrate_client::{Chain, TransactionSignScheme};
use relay_westend_client::{SyncHeader as WestendSyncHeader, Westend};
use sp_core::{Bytes, Pair};
@@ -40,12 +41,12 @@ impl SubstrateFinalitySyncPipeline for WestendFinalityToMillau {
&self,
transaction_nonce: <Millau as Chain>::Index,
header: WestendSyncHeader,
proof: Justification<bp_westend::BlockNumber>,
proof: GrandpaJustification<bp_westend::Header>,
) -> Bytes {
let call = millau_runtime::BridgeGrandpaWestendCall::<
millau_runtime::Runtime,
millau_runtime::WestendGrandpaInstance,
>::submit_finality_proof(header.into_inner(), proof.into_inner())
>::submit_finality_proof(header.into_inner(), proof)
.into();
let genesis_hash = *self.target_client.genesis_hash();
@@ -22,35 +22,14 @@ use crate::error::Error;
use crate::sync_header::SyncHeader;
use async_trait::async_trait;
use bp_header_chain::justification::decode_justification_target;
use finality_relay::{FinalityProof, FinalitySyncPipeline, SourceClient, SourceHeader};
use bp_header_chain::justification::GrandpaJustification;
use codec::Decode;
use finality_relay::{FinalitySyncPipeline, SourceClient, SourceHeader};
use futures::stream::{unfold, Stream, StreamExt};
use relay_utils::relay_loop::Client as RelayClient;
use sp_runtime::traits::Header as HeaderT;
use std::{marker::PhantomData, pin::Pin};
/// Wrapped raw Justification.
#[derive(Debug, Clone)]
pub struct Justification<Number> {
/// Header number decoded from the [`raw_justification`].
target_header_number: Number,
/// Raw, encoded justification bytes.
raw_justification: sp_runtime::Justification,
}
impl<Number> Justification<Number> {
/// Extract raw justification.
pub fn into_inner(self) -> sp_runtime::Justification {
self.raw_justification
}
}
impl<Number: relay_utils::BlockNumberBase> FinalityProof<Number> for Justification<Number> {
fn target_header_number(&self) -> Number {
self.target_header_number
}
}
/// Substrate node as finality source.
pub struct FinalitySource<C: Chain, P> {
client: Client<C>,
@@ -94,11 +73,11 @@ where
Hash = C::Hash,
Number = C::BlockNumber,
Header = SyncHeader<C::Header>,
FinalityProof = Justification<C::BlockNumber>,
FinalityProof = GrandpaJustification<C::Header>,
>,
P::Header: SourceHeader<C::BlockNumber>,
{
type FinalityProofsStream = Pin<Box<dyn Stream<Item = Justification<C::BlockNumber>> + Send>>;
type FinalityProofsStream = Pin<Box<dyn Stream<Item = GrandpaJustification<C::Header>> + Send>>;
async fn best_finalized_block_number(&self) -> Result<P::Number, Error> {
// we **CAN** continue to relay finality proofs if source node is out of sync, because
@@ -114,16 +93,14 @@ where
) -> Result<(P::Header, Option<P::FinalityProof>), Error> {
let header_hash = self.client.block_hash_by_number(number).await?;
let signed_block = self.client.get_block(Some(header_hash)).await?;
Ok((
signed_block.header().into(),
signed_block
.justification()
.cloned()
.map(|raw_justification| Justification {
target_header_number: number,
raw_justification,
}),
))
let justification = signed_block
.justification()
.map(|raw_justification| GrandpaJustification::<C::Header>::decode(&mut raw_justification.as_slice()))
.transpose()
.map_err(Error::ResponseParseFailed)?;
Ok((signed_block.header().into(), justification))
}
async fn finality_proofs(&self) -> Result<Self::FinalityProofsStream, Error> {
@@ -132,9 +109,11 @@ where
move |mut subscription| async move {
loop {
let next_justification = subscription.next().await?;
let decoded_target = decode_justification_target::<C::Header>(&next_justification.0);
let target_header_number = match decoded_target {
Ok((_, number)) => number,
let decoded_justification =
GrandpaJustification::<C::Header>::decode(&mut &next_justification.0[..]);
let justification = match decoded_justification {
Ok(j) => j,
Err(err) => {
log::error!(
target: "bridge",
@@ -147,13 +126,7 @@ where
}
};
return Some((
Justification {
target_header_number,
raw_justification: next_justification.0,
},
subscription,
));
return Some((justification, subscription));
}
},
)
+1
View File
@@ -10,6 +10,7 @@ description = "Finality proofs relay"
async-std = "1.6.5"
async-trait = "0.1.40"
backoff = "0.2"
bp-header-chain = { path = "../../primitives/header-chain" }
futures = "0.3.5"
headers-relay = { path = "../headers" }
log = "0.4.11"
+1 -6
View File
@@ -21,6 +21,7 @@
pub use crate::finality_loop::{run, FinalitySyncParams, SourceClient, TargetClient};
use bp_header_chain::FinalityProof;
use std::fmt::Debug;
mod finality_loop;
@@ -50,9 +51,3 @@ pub trait SourceHeader<Number>: Clone + Debug + PartialEq + Send + Sync {
/// Returns true if this header needs to be submitted to target node.
fn is_mandatory(&self) -> bool;
}
/// Abstract finality proof that is justifying block finality.
pub trait FinalityProof<Number>: Clone + Send + Sync + Debug {
/// Return number of header that this proof is generated for.
fn target_header_number(&self) -> Number;
}