From 5c8a882f29738eda2975dd1e2bdf43a59e27953a Mon Sep 17 00:00:00 2001 From: Serban Iorga Date: Fri, 28 Oct 2022 13:12:37 +0300 Subject: [PATCH] Finality loop: get block justification and authorities change by consensus engine ID (#1619) * SignedBlock: get justification by consensus engine id * Define ConsensusLogReader Making the check for authority changes more generic * cod review changes --- primitives/header-chain/src/lib.rs | 39 ++++++++++----- relays/client-substrate/src/chain.rs | 10 ++-- relays/client-substrate/src/sync_header.rs | 8 +-- relays/finality/src/finality_loop_tests.rs | 6 ++- relays/finality/src/lib.rs | 8 +-- .../src/finality/engine.rs | 50 +++++++++++-------- .../lib-substrate-relay/src/finality/mod.rs | 3 +- .../src/finality/source.rs | 13 ++--- .../src/on_demand/headers.rs | 15 +++--- 9 files changed, 88 insertions(+), 64 deletions(-) diff --git a/primitives/header-chain/src/lib.rs b/primitives/header-chain/src/lib.rs index f5590ce0a5a2b..21a42874b719d 100644 --- a/primitives/header-chain/src/lib.rs +++ b/primitives/header-chain/src/lib.rs @@ -26,7 +26,7 @@ use scale_info::TypeInfo; #[cfg(feature = "std")] use serde::{Deserialize, Serialize}; use sp_finality_grandpa::{AuthorityList, ConsensusLog, SetId, GRANDPA_ENGINE_ID}; -use sp_runtime::{generic::OpaqueDigestItemId, traits::Header as HeaderT, RuntimeDebug}; +use sp_runtime::{traits::Header as HeaderT, Digest, RuntimeDebug}; use sp_std::boxed::Box; pub mod justification; @@ -77,18 +77,31 @@ pub trait FinalityProof: Clone + Send + Sync + Debug { fn target_header_number(&self) -> Number; } -/// Find header digest that schedules next GRANDPA authorities set. -pub fn find_grandpa_authorities_scheduled_change( - header: &H, -) -> Option> { - let id = OpaqueDigestItemId::Consensus(&GRANDPA_ENGINE_ID); +/// A trait that provides helper methods for querying the consensus log. +pub trait ConsensusLogReader { + fn schedules_authorities_change(digest: &Digest) -> bool; +} - let filter_log = |log: ConsensusLog| match log { - ConsensusLog::ScheduledChange(change) => Some(change), - _ => None, - }; +/// A struct that provides helper methods for querying the GRANDPA consensus log. +pub struct GrandpaConsensusLogReader(sp_std::marker::PhantomData); - // 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)) +impl GrandpaConsensusLogReader { + pub fn find_authorities_change( + digest: &Digest, + ) -> Option> { + // find the first consensus digest with the right ID which converts to + // the right kind of consensus log. + digest + .convert_first(|log| log.consensus_try_to(&GRANDPA_ENGINE_ID)) + .and_then(|log| match log { + ConsensusLog::ScheduledChange(change) => Some(change), + _ => None, + }) + } +} + +impl ConsensusLogReader for GrandpaConsensusLogReader { + fn schedules_authorities_change(digest: &Digest) -> bool { + GrandpaConsensusLogReader::::find_authorities_change(digest).is_some() + } } diff --git a/relays/client-substrate/src/chain.rs b/relays/client-substrate/src/chain.rs index 3f483078cac79..97e3b0f0fc21e 100644 --- a/relays/client-substrate/src/chain.rs +++ b/relays/client-substrate/src/chain.rs @@ -27,7 +27,7 @@ use sp_core::{storage::StorageKey, Pair}; use sp_runtime::{ generic::SignedBlock, traits::{Block as BlockT, Dispatchable, Member}, - EncodedJustification, + ConsensusEngineId, EncodedJustification, }; use std::{fmt::Debug, time::Duration}; @@ -147,7 +147,7 @@ pub trait BlockWithJustification
{ /// Return encoded block extrinsics. fn extrinsics(&self) -> Vec; /// Return block justification, if known. - fn justification(&self) -> Option<&EncodedJustification>; + fn justification(&self, engine_id: ConsensusEngineId) -> Option<&EncodedJustification>; } /// Transaction before it is signed. @@ -237,9 +237,7 @@ impl BlockWithJustification for SignedBlock self.block.extrinsics().iter().map(Encode::encode).collect() } - fn justification(&self) -> Option<&EncodedJustification> { - self.justifications - .as_ref() - .and_then(|j| j.get(sp_finality_grandpa::GRANDPA_ENGINE_ID)) + fn justification(&self, engine_id: ConsensusEngineId) -> Option<&EncodedJustification> { + self.justifications.as_ref().and_then(|j| j.get(engine_id)) } } diff --git a/relays/client-substrate/src/sync_header.rs b/relays/client-substrate/src/sync_header.rs index e45e6b4197abb..fdfd1f22ce9ed 100644 --- a/relays/client-substrate/src/sync_header.rs +++ b/relays/client-substrate/src/sync_header.rs @@ -14,7 +14,7 @@ // You should have received a copy of the GNU General Public License // along with Parity Bridges Common. If not, see . -use bp_header_chain::find_grandpa_authorities_scheduled_change; +use bp_header_chain::ConsensusLogReader; use finality_relay::SourceHeader as FinalitySourceHeader; use sp_runtime::traits::Header as HeaderT; @@ -44,7 +44,9 @@ impl
From
for SyncHeader
{ } } -impl FinalitySourceHeader for SyncHeader
{ +impl FinalitySourceHeader + for SyncHeader
+{ fn hash(&self) -> Header::Hash { self.0.hash() } @@ -54,6 +56,6 @@ impl FinalitySourceHeader for Syn } fn is_mandatory(&self) -> bool { - find_grandpa_authorities_scheduled_change(&self.0).is_some() + R::schedules_authorities_change(self.digest()) } } diff --git a/relays/finality/src/finality_loop_tests.rs b/relays/finality/src/finality_loop_tests.rs index a6e97c0770f99..1853c095f703f 100644 --- a/relays/finality/src/finality_loop_tests.rs +++ b/relays/finality/src/finality_loop_tests.rs @@ -30,6 +30,7 @@ use crate::{ }; use async_trait::async_trait; +use bp_header_chain::GrandpaConsensusLogReader; use futures::{FutureExt, Stream, StreamExt}; use parking_lot::Mutex; use relay_utils::{ @@ -85,6 +86,7 @@ impl FinalitySyncPipeline for TestFinalitySyncPipeline { type Hash = TestHash; type Number = TestNumber; + type ConsensusLogReader = GrandpaConsensusLogReader; type Header = TestSourceHeader; type FinalityProof = TestFinalityProof; } @@ -92,7 +94,9 @@ impl FinalitySyncPipeline for TestFinalitySyncPipeline { #[derive(Debug, Clone, PartialEq, Eq)] struct TestSourceHeader(IsMandatory, TestNumber, TestHash); -impl SourceHeader for TestSourceHeader { +impl SourceHeader> + for TestSourceHeader +{ fn hash(&self) -> TestHash { self.2 } diff --git a/relays/finality/src/lib.rs b/relays/finality/src/lib.rs index 49be64ff74db2..dca47c6a572cf 100644 --- a/relays/finality/src/lib.rs +++ b/relays/finality/src/lib.rs @@ -24,7 +24,7 @@ pub use crate::{ sync_loop_metrics::SyncLoopMetrics, }; -use bp_header_chain::FinalityProof; +use bp_header_chain::{ConsensusLogReader, FinalityProof}; use std::fmt::Debug; mod finality_loop; @@ -42,14 +42,16 @@ pub trait FinalitySyncPipeline: 'static + Clone + Debug + Send + Sync { type Hash: Eq + Clone + Copy + Send + Sync + Debug; /// Headers we're syncing are identified by this number. type Number: relay_utils::BlockNumberBase; + /// A reader that can extract the consensus log from the header digest and interpret it. + type ConsensusLogReader: ConsensusLogReader; /// Type of header that we're syncing. - type Header: SourceHeader; + type Header: SourceHeader; /// Finality proof type. type FinalityProof: FinalityProof; } /// Header that we're receiving from source node. -pub trait SourceHeader: Clone + Debug + PartialEq + Send + Sync { +pub trait SourceHeader: Clone + Debug + PartialEq + Send + Sync { /// Returns hash of header. fn hash(&self) -> Hash; /// Returns number of header. diff --git a/relays/lib-substrate-relay/src/finality/engine.rs b/relays/lib-substrate-relay/src/finality/engine.rs index 83ea074e93d0b..4c2da5a53195d 100644 --- a/relays/lib-substrate-relay/src/finality/engine.rs +++ b/relays/lib-substrate-relay/src/finality/engine.rs @@ -19,9 +19,8 @@ use crate::error::Error; use async_trait::async_trait; use bp_header_chain::{ - find_grandpa_authorities_scheduled_change, justification::{verify_justification, GrandpaJustification}, - FinalityProof, + ConsensusLogReader, FinalityProof, GrandpaConsensusLogReader, }; use bp_runtime::{BasicOperatingMode, OperatingMode}; use codec::{Decode, Encode}; @@ -32,7 +31,7 @@ use relay_substrate_client::{ Subscription, SubstrateFinalityClient, SubstrateGrandpaFinalityClient, }; use sp_core::{storage::StorageKey, Bytes}; -use sp_finality_grandpa::AuthorityList as GrandpaAuthoritiesSet; +use sp_finality_grandpa::{AuthorityList as GrandpaAuthoritiesSet, GRANDPA_ENGINE_ID}; use sp_runtime::{traits::Header, ConsensusEngineId}; use std::marker::PhantomData; @@ -41,6 +40,8 @@ use std::marker::PhantomData; pub trait Engine: Send { /// Unique consensus engine identifier. const ID: ConsensusEngineId; + /// A reader that can extract the consensus log from the header digest and interpret it. + type ConsensusLogReader: ConsensusLogReader; /// Type of Finality RPC client used by this engine. type FinalityClient: SubstrateFinalityClient; /// Type of finality proofs, used by consensus engine. @@ -50,22 +51,11 @@ pub trait Engine: Send { /// Type of bridge pallet operating mode. type OperatingMode: OperatingMode + 'static; - /// Returns storage key at the bridged (target) chain that corresponds to the variable - /// that holds the operating mode of the pallet. - fn pallet_operating_mode_key() -> StorageKey; /// Returns storage at the bridged (target) chain that corresponds to some value that is /// missing from the storage until bridge pallet is initialized. /// /// Note that we don't care about type of the value - just if it present or not. fn is_initialized_key() -> StorageKey; - /// A method to subscribe to encoded finality proofs, given source client. - async fn finality_proofs(client: &Client) -> Result, SubstrateError> { - client.subscribe_finality_justifications::().await - } - /// Prepare initialization data for the finality bridge pallet. - async fn prepare_initialization_data( - client: Client, - ) -> Result, BlockNumberOf>>; /// Returns `Ok(true)` if finality pallet at the bridged chain has already been initialized. async fn is_initialized( @@ -77,6 +67,10 @@ pub trait Engine: Send { .is_some()) } + /// Returns storage key at the bridged (target) chain that corresponds to the variable + /// that holds the operating mode of the pallet. + fn pallet_operating_mode_key() -> StorageKey; + /// Returns `Ok(true)` if finality pallet at the bridged chain is halted. async fn is_halted( target_client: &Client, @@ -87,6 +81,16 @@ pub trait Engine: Send { .map(|operating_mode| operating_mode.is_halted()) .unwrap_or(false)) } + + /// A method to subscribe to encoded finality proofs, given source client. + async fn finality_proofs(client: &Client) -> Result, SubstrateError> { + client.subscribe_finality_justifications::().await + } + + /// Prepare initialization data for the finality bridge pallet. + async fn prepare_initialization_data( + client: Client, + ) -> Result, BlockNumberOf>>; } /// GRANDPA finality engine. @@ -120,20 +124,21 @@ impl Grandpa { #[async_trait] impl Engine for Grandpa { - const ID: ConsensusEngineId = sp_finality_grandpa::GRANDPA_ENGINE_ID; + const ID: ConsensusEngineId = GRANDPA_ENGINE_ID; + type ConsensusLogReader = GrandpaConsensusLogReader<::Number>; type FinalityClient = SubstrateGrandpaFinalityClient; type FinalityProof = GrandpaJustification>; type InitializationData = bp_header_chain::InitializationData; type OperatingMode = BasicOperatingMode; - fn pallet_operating_mode_key() -> StorageKey { - bp_header_chain::storage_keys::pallet_operating_mode_key(C::WITH_CHAIN_GRANDPA_PALLET_NAME) - } - fn is_initialized_key() -> StorageKey { bp_header_chain::storage_keys::best_finalized_key(C::WITH_CHAIN_GRANDPA_PALLET_NAME) } + fn pallet_operating_mode_key() -> StorageKey { + bp_header_chain::storage_keys::pallet_operating_mode_key(C::WITH_CHAIN_GRANDPA_PALLET_NAME) + } + /// Prepare initialization data for the GRANDPA verifier pallet. async fn prepare_initialization_data( source_client: Client, @@ -183,11 +188,14 @@ impl Engine for Grandpa { // If initial header changes the GRANDPA authorities set, then we need previous authorities // to verify justification. let mut authorities_for_verification = initial_authorities_set.clone(); - let scheduled_change = find_grandpa_authorities_scheduled_change(&initial_header); + let scheduled_change = + GrandpaConsensusLogReader::>::find_authorities_change( + initial_header.digest(), + ); assert!( scheduled_change.as_ref().map(|c| c.delay.is_zero()).unwrap_or(true), "GRANDPA authorities change at {} scheduled to happen in {:?} blocks. We expect\ - regular hange to have zero delay", + regular change to have zero delay", initial_header_hash, scheduled_change.as_ref().map(|c| c.delay), ); diff --git a/relays/lib-substrate-relay/src/finality/mod.rs b/relays/lib-substrate-relay/src/finality/mod.rs index 3144a1016ea5c..aefcd21722525 100644 --- a/relays/lib-substrate-relay/src/finality/mod.rs +++ b/relays/lib-substrate-relay/src/finality/mod.rs @@ -87,7 +87,8 @@ impl FinalitySyncPipeline for FinalitySyncPipe type Hash = HashOf; type Number = BlockNumberOf; - type Header = relay_substrate_client::SyncHeader>; + type ConsensusLogReader = >::ConsensusLogReader; + type Header = SyncHeader>; type FinalityProof = SubstrateFinalityProof

; } diff --git a/relays/lib-substrate-relay/src/finality/source.rs b/relays/lib-substrate-relay/src/finality/source.rs index 430a83eb43ca7..e75862a8227bc 100644 --- a/relays/lib-substrate-relay/src/finality/source.rs +++ b/relays/lib-substrate-relay/src/finality/source.rs @@ -33,15 +33,8 @@ use std::pin::Pin; pub type RequiredHeaderNumberRef = Arc::BlockNumber>>; /// Substrate finality proofs stream. -pub type SubstrateFinalityProofsStream

= Pin< - Box< - dyn Stream< - Item = <

::FinalityEngine as Engine< -

::SourceChain, - >>::FinalityProof, - > + Send, - >, ->; +pub type SubstrateFinalityProofsStream

= + Pin> + Send>>; /// Substrate finality proof. Specific to the used `FinalityEngine`. pub type SubstrateFinalityProof

= @@ -130,7 +123,7 @@ impl SourceClient::decode(&mut raw_justification.as_slice()) }) diff --git a/relays/lib-substrate-relay/src/on_demand/headers.rs b/relays/lib-substrate-relay/src/on_demand/headers.rs index 87f1e012cc57d..3a54e6bb00cba 100644 --- a/relays/lib-substrate-relay/src/on_demand/headers.rs +++ b/relays/lib-substrate-relay/src/on_demand/headers.rs @@ -18,13 +18,14 @@ use async_std::sync::{Arc, Mutex}; use async_trait::async_trait; +use bp_header_chain::ConsensusLogReader; use futures::{select, FutureExt}; use num_traits::{One, Zero}; +use sp_runtime::traits::Header; -use finality_relay::{FinalitySyncParams, SourceHeader, TargetClient as FinalityTargetClient}; +use finality_relay::{FinalitySyncParams, TargetClient as FinalityTargetClient}; use relay_substrate_client::{ - AccountIdOf, AccountKeyPairOf, BlockNumberOf, Chain, Client, HeaderOf, SyncHeader, - TransactionSignScheme, + AccountIdOf, AccountKeyPairOf, BlockNumberOf, Chain, Client, TransactionSignScheme, }; use relay_utils::{ metrics::MetricsParams, relay_loop::Client as RelayClient, FailedClient, MaybeConnectionError, @@ -33,6 +34,7 @@ use relay_utils::{ use crate::{ finality::{ + engine::Engine, source::{RequiredHeaderNumberRef, SubstrateFinalitySource}, target::SubstrateFinalityTarget, SubstrateFinalitySyncPipeline, RECENT_FINALITY_PROOFS_LIMIT, @@ -416,9 +418,10 @@ async fn find_mandatory_header_in_range( ) -> Result>, relay_substrate_client::Error> { let mut current = range.0; while current <= range.1 { - let header: SyncHeader> = - finality_source.client().header_by_number(current).await?.into(); - if header.is_mandatory() { + let header = finality_source.client().header_by_number(current).await?; + if >::ConsensusLogReader::schedules_authorities_change( + header.digest(), + ) { return Ok(Some(current)) }