From 1cb0bbc48b873ea0feed20c32423614b426ac216 Mon Sep 17 00:00:00 2001 From: Robert Habermeier Date: Thu, 15 Nov 2018 16:36:28 +0100 Subject: [PATCH] start addressing basti comments --- substrate/core/client/src/client.rs | 3 +- .../finality-grandpa/primitives/src/lib.rs | 95 +++++++++++++++---- substrate/core/finality-grandpa/src/lib.rs | 55 +++-------- 3 files changed, 88 insertions(+), 65 deletions(-) diff --git a/substrate/core/client/src/client.rs b/substrate/core/client/src/client.rs index 4d4a438a08..f889a7045f 100644 --- a/substrate/core/client/src/client.rs +++ b/substrate/core/client/src/client.rs @@ -916,7 +916,8 @@ impl CallApiAt for Client where B: backend::Backend, E: CallExecutor + Clone + Send + Sync, Block: BlockT, - RA: Send + Sync, + RA: CoreAPI, // not strictly necessary at the moment + // but we want to bound to make sure the API is actually available. { fn call_api_at( &self, diff --git a/substrate/core/finality-grandpa/primitives/src/lib.rs b/substrate/core/finality-grandpa/primitives/src/lib.rs index 2f53523555..ed8ae408b7 100644 --- a/substrate/core/finality-grandpa/primitives/src/lib.rs +++ b/substrate/core/finality-grandpa/primitives/src/lib.rs @@ -54,27 +54,82 @@ pub mod id { pub const GRANDPA_API: ApiId = *b"fgrandpa"; } -/// APIs for integrating the GRANDPA finality gadget into runtimes. -/// This should be implemented on the runtime side. -/// -/// This is primarily used for negotiating authority-set changes for the -/// gadget. GRANDPA uses a signalling model of changing authority sets: -/// changes should be signalled with a delay of N blocks, and then automatically -/// applied in the runtime after those N blocks have passed. -/// -/// The consensus protocol will coordinate the handoff externally. -pub trait GrandpaApi { - /// Check a digest for pending changes. - /// Return `None` if there are no pending changes. +decl_runtime_apis! { + /// APIs for integrating the GRANDPA finality gadget into runtimes. + /// This should be implemented on the runtime side. /// - /// Precedence towards earlier or later digest items can be given - /// based on the rules of the chain. + /// This is primarily used for negotiating authority-set changes for the + /// gadget. GRANDPA uses a signalling model of changing authority sets: + /// changes should be signalled with a delay of N blocks, and then automatically + /// applied in the runtime after those N blocks have passed. /// - /// No change should be scheduled if one is already and the delay has not - /// passed completely. - fn grandpa_pending_change(digest: DigestFor) -> Option>>; + /// The consensus protocol will coordinate the handoff externally. + pub trait GrandpaApi { + /// Check a digest for pending changes. + /// Return `None` if there are no pending changes. + /// + /// Precedence towards earlier or later digest items can be given + /// based on the rules of the chain. + /// + /// No change should be scheduled if one is already and the delay has not + /// passed completely. + fn grandpa_pending_change(digest: DigestFor) + -> Option>>; - /// Get the current GRANDPA authorities and weights. This should not change except - /// for when changes are scheduled and the corresponding delay has passed. - fn grandpa_authorities() -> Vec<(AuthorityId, u64)>; + /// Get the current GRANDPA authorities and weights. This should not change except + /// for when changes are scheduled and the corresponding delay has passed. + fn grandpa_authorities() -> Vec<(AuthorityId, u64)>; + } +} + +#[cfg(feature = "std")] +mod implementation { + use super::{GrandpaApi, ScheduledChange}; + use sr_primitives::traits::{Block as BlockT, DigestFor, NumberFor}; + use sr_primitives::generic::BlockId; + use parity_codec::{Encode, Decode}; + use client::{Client, error::Error as ClientError, backend::Backend, CallExecutor}; + use client::runtime_api::{CallApiAt, Core as CoreAPI}; + use substrate_primitives::{AuthorityId, H256, Blake2Hasher}; + + // TODO [basti]: do this implementation in runtime. + impl, RA> GrandpaApi for Client where + B: Backend + 'static, + E: CallExecutor + 'static + Clone + Send + Sync, + DigestFor: Encode, + RA: CoreAPI, + { + fn grandpa_authorities(&self, at: &BlockId) -> Result, ClientError> { + let raw = self.call_api_at( + &at, + ::AUTHORITIES_CALL, + Encode::encode(&()), + &mut Default::default(), + &mut None, + ); + + // TODO [basti]: implement this in runtime with macro. + match Decode::decode(&mut &raw[..]) { + Some(x) => Ok(x), + None => Err(::client::error::ErrorKind::CallResultDecode(::AUTHORITIES_CALL).into()), + } + } + + fn grandpa_pending_change(&self, at: &BlockId, digest: DigestFor) + -> Result>>, ClientError> + { + let raw = self.call_api_at( + at, + ::PENDING_CHANGE_CALL, + digest.encode(), + &mut Default::default(), + &mut None, + ); + + match Decode::decode(&mut &raw[..]) { + Some(x) => Ok(x), + None => Err(::client::error::ErrorKind::CallResultDecode(::PENDING_CHANGE_CALL).into()), + } + } + } } diff --git a/substrate/core/finality-grandpa/src/lib.rs b/substrate/core/finality-grandpa/src/lib.rs index ba23383db2..1a0f10334a 100644 --- a/substrate/core/finality-grandpa/src/lib.rs +++ b/substrate/core/finality-grandpa/src/lib.rs @@ -84,12 +84,13 @@ use futures::stream::Fuse; use futures::sync::mpsc; use client::{Client, error::Error as ClientError, ImportNotifications, backend::Backend, CallExecutor}; use client::blockchain::HeaderBackend; -use client::runtime_api::{CallApiAt, TaggedTransactionQueue}; +use client::runtime_api::{CallApiAt, TaggedTransactionQueue, Core as CoreAPI}; use codec::{Encode, Decode}; use consensus_common::{BlockImport, ImportBlock, ImportResult}; use runtime_primitives::traits::{ NumberFor, Block as BlockT, Header as HeaderT, DigestFor, }; +use fg_primitives::GrandpaApi; use runtime_primitives::generic::BlockId; use substrate_primitives::{ed25519, H256, AuthorityId, Blake2Hasher}; use tokio::timer::Interval; @@ -765,46 +766,7 @@ impl, N, RA> voter::Environment> { - /// Get the genesis authorities for GRANDPA. - fn genesis_authorities(&self) -> Result, ClientError>; - /// Check a header's digest for a scheduled change. - fn scheduled_change(&self, header: &Block::Header) - -> Result>>, ClientError>; -} - -impl, RA> ApiClient for Arc> where - B: Backend + 'static, - E: CallExecutor + 'static + Clone + Send + Sync, - DigestFor: Encode, - RA: Send + Sync, -{ - fn genesis_authorities(&self) -> Result, ClientError> { - use runtime_primitives::traits::Zero; - - self.call_api_at_strong( - &BlockId::Number(NumberFor::::zero()), - fg_primitives::AUTHORITIES_CALL, - &(), - &mut Default::default(), - &mut None, - ) - } - - fn scheduled_change(&self, header: &Block::Header) - -> Result>>, ClientError> - { - self.call_api_at_strong( - &BlockId::hash(header.parent_hash().clone()), - fg_primitives::PENDING_CHANGE_CALL, - header.digest(), - &mut Default::default(), - &mut None, - ) - } -} /// A block-import handler for GRANDPA. /// @@ -821,7 +783,7 @@ impl, Api, RA> BlockImport for GrandpaBloc B: Backend + 'static, E: CallExecutor + 'static + Clone + Send + Sync, DigestFor: Encode, - Api: ApiClient, + Api: GrandpaApi, RA: TaggedTransactionQueue + Send + Sync, // necessary for client to import `BlockImport`. { type Error = ClientError; @@ -831,7 +793,10 @@ impl, Api, RA> BlockImport for GrandpaBloc { use authorities::PendingChange; - let maybe_change = self.api_client.scheduled_change(&block.header)?; + let maybe_change = self.api_client.grandpa_pending_change( + &BlockId::hash(*block.header.parent_hash()), + &block.header.digest().clone(), + )?; // when we update the authorities, we need to hold the lock // until the block is written to prevent a race if we need to restore @@ -879,9 +844,10 @@ pub fn block_import, Api, RA>(client: Arc + 'static, E: CallExecutor + 'static, - Api: ApiClient, + Api: GrandpaApi, RA: Send + Sync, { + use runtime_primitives::traits::Zero; let authority_set = match client.backend().get_aux(AUTHORITY_SET_KEY)? { None => { info!(target: "afg", "Loading GRANDPA authorities \ @@ -890,7 +856,8 @@ pub fn block_import, Api, RA>(client: Arc = api_client.genesis_authorities()?; + let genesis_authorities = api_client + .grandpa_authorities(&BlockId::number(Zero::zero()))?; let authority_set = SharedAuthoritySet::genesis(genesis_authorities); let encoded = authority_set.inner().read().encode();