From 2f5367f11a171920c0aa449668b8eeeacfe5b00b Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 8 Apr 2020 15:23:40 -0400 Subject: [PATCH 01/59] Rough skeleton for what I think the RPC should look like --- client/api/src/client.rs | 12 +++++++ client/rpc-api/src/chain/mod.rs | 20 +++++++++++ client/rpc/src/chain/mod.rs | 61 +++++++++++++++++++++++++++++++++ client/rpc/src/chain/tests.rs | 33 ++++++++++++++++++ client/src/client.rs | 7 ++++ 5 files changed, 133 insertions(+) diff --git a/client/api/src/client.rs b/client/api/src/client.rs index c855cd3a08328..9ec86e6cda67e 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -36,6 +36,9 @@ pub type ImportNotifications = TracingUnboundedReceiver = TracingUnboundedReceiver>; +/// A stream of Grandpa Justification notifications. +pub type JustificationNotifications = TracingUnboundedReceiver>; + /// Expected hashes of blocks at given heights. /// /// This may be used as chain spec extension to set trusted checkpoints, i.e. @@ -246,3 +249,12 @@ pub struct FinalityNotification { /// Imported block header. pub header: Block::Header, } + +/// Justification for a finalized block. +#[derive(Clone, Debug)] +pub struct JustificationNotification { + /// Highest finalized block header + pub header: Block::Header, + + // Should contain a GrandpaJustification +} diff --git a/client/rpc-api/src/chain/mod.rs b/client/rpc-api/src/chain/mod.rs index 2ab3851d37663..0bfacda2aebed 100644 --- a/client/rpc-api/src/chain/mod.rs +++ b/client/rpc-api/src/chain/mod.rs @@ -109,4 +109,24 @@ pub trait ChainApi { metadata: Option, id: SubscriptionId, ) -> RpcResult; + + /// Subscribe to finality events. + #[pubsub( + subscription = "grandpa_finality", + subscribe, + name = "grandpa_subscribeToFinality", + )] + fn subscribe_finality(&self, metadata: Self::Metadata, subscriber: Subscriber
); + + /// Unsubscribe to finality events. + #[pubsub( + subscription = "grandpa_finality", + unsubscribe, + name = "grandpa_subscribeToFinality", + )] + fn unsubscribe_finality( + &self, + metadata: Option, + id: SubscriptionId, + ) -> RpcResult; } diff --git a/client/rpc/src/chain/mod.rs b/client/rpc/src/chain/mod.rs index fa97e21886035..6a2aadb9e31e3 100644 --- a/client/rpc/src/chain/mod.rs +++ b/client/rpc/src/chain/mod.rs @@ -173,6 +173,58 @@ trait ChainBackend: Send + Sync + 'static ) -> RpcResult { Ok(self.subscriptions().cancel(id)) } + + /// Subscribe to finality events. + fn subscribe_finality( + &self, + metadata: crate::metadata::Metadata, + subscriber: Subscriber + ) { + let block_stream = match self.client().finality_notification_stream() { + Ok(stream) => stream, + Err(e) => { + todo!(); + } + }; + + let justification_stream = match self.client().justification_notification_stream() { + Ok(stream) => stream, + Err(e) => { + todo!(); + } + }; + + self.subscriptions().add(subscriber, |sink|{ + let header = self.client().header(BlockId::Hash(self.client().info().finalized_hash)) + .map_err(client_err) + .and_then(|header| { + header.ok_or_else(|| "Best header missing.".to_owned().into()) + }) + .map_err(Into::into); + + // How do I know that the right header is matched to the right justification? + // Should we be doing any filtering here? + // Probably depends on what we end up getting from the justification_stream + let stream = block_stream.zip(justification_stream); + + // stream::iter_result(vec![Ok(version)]) + // .chain(stream) + sink + .sink_map_err(|e| warn!("Error sending notifications: {:?}", e)) + .send_all(stream) + // we ignore the resulting Stream (if the first stream is over we are unsubscribed) + .map(|_| ()) + }); + } + + /// Unsubscribe to finality events. + fn unsubscribe_finality( + &self, + metadata: Option, + id: SubscriptionId, + ) -> RpcResult { + Ok(self.Subscriptions.cancel(id)) + } } /// Create new state API that works on full node. @@ -275,6 +327,15 @@ impl ChainApi, Block::Hash, Block::Header, Signe fn unsubscribe_finalized_heads(&self, metadata: Option, id: SubscriptionId) -> RpcResult { self.backend.unsubscribe_finalized_heads(metadata, id) } + + // TODO: Figure out what `T` should be for `Subscriber` + fn subscribe_finality(&self, metadata: Self::Metadata, subscriber: Subscriber) { + self.backend.subscribe_finality(metadata, subscriber) + } + + fn unsubscribe_finality(&self, metadata: Option, id: SubscriptionId) -> RpcResult { + self.backend.unsubscribe_finality(metadata, id) + } } /// Subscribe to new headers. diff --git a/client/rpc/src/chain/tests.rs b/client/rpc/src/chain/tests.rs index 68d904919b3d7..73f7044e23865 100644 --- a/client/rpc/src/chain/tests.rs +++ b/client/rpc/src/chain/tests.rs @@ -277,3 +277,36 @@ fn should_notify_about_finalized_block() { // no more notifications on this channel assert_eq!(core.block_on(next.into_future()).unwrap().0, None); } + +#[test] +fn should_notify_about_finality_event() { + let mut core = ::tokio::runtime::Runtime::new().unwrap(); + let remote = core.executor(); + let (subscriber, id, transport) = Subscriber::new_test("test"); + + { + let mut client = Arc::new(substrate_test_runtime_client::new()); + let api = new_full(client.clone(), Subscriptions::new(Arc::new(remote))); + + api.subscribe_finality(Default::default(), subscriber); + + // assert id assigned + assert_eq!(core.block_on(id), Ok(Ok(SubscriptionId::Number(1)))); + + let block = client.new_block(Default::default()).unwrap().build().unwrap().block; + client.import(BlockOrigin::Own, block).unwrap(); + // Do I need some way to trigger a "justification" event aside from + // simply finalizing a block? + // Maybe something to do with a voting round ending? + client.finalize_block(BlockId::number(1), None).unwrap(); + } + + // assert initial head sent. + let (notification, next) = core.block_on(transport.into_future()).unwrap(); + assert!(notification.is_some()); + // assert notification sent to transport + let (notification, next) = core.block_on(next.into_future()).unwrap(); + assert!(notification.is_some()); + // no more notifications on this channel + assert_eq!(core.block_on(next.into_future()).unwrap().0, None); +} diff --git a/client/src/client.rs b/client/src/client.rs index 2a8040febf3ef..b1bc4f46bf6a9 100644 --- a/client/src/client.rs +++ b/client/src/client.rs @@ -95,6 +95,7 @@ pub struct Client where Block: BlockT { storage_notifications: Mutex>, import_notification_sinks: Mutex>>>, finality_notification_sinks: Mutex>>>, + justification_notification_sinks: Mutex>>>, // holds the block hash currently being imported. TODO: replace this with block queue importing_block: RwLock>, block_rules: BlockRules, @@ -1777,6 +1778,12 @@ where stream } + fn justification_notification_stream(&self) -> JustificationNotifications { + let (sink, stream) = tracing_unbounded("mpsc_justification_notification_stream"); + self.justification_notification_sinks.lock().push(sink); + stream + } + /// Get storage changes event stream. fn storage_changes_notification_stream( &self, From 9d369ac0b67f9d71e7964b645b3ac5d810dd8ef3 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 10 Apr 2020 19:15:39 -0400 Subject: [PATCH 02/59] Create channel for sending justifications Sends finalized header and justification from Grandpa to the client. This lays the groundwork for hooking into the RPC module. --- client/api/src/backend.rs | 13 +++++ client/api/src/client.rs | 10 +++- client/finality-grandpa/src/environment.rs | 1 - client/src/client.rs | 57 ++++++++++++++++++++-- 4 files changed, 75 insertions(+), 6 deletions(-) diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index 33a370c7cb2c5..a036f98826676 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -69,6 +69,17 @@ pub struct ImportSummary { pub retracted: Vec, } +/// Summary of a Justification event. +/// +/// Contains the most recently finalized block as well +/// as a justification for its finalization. +pub struct JustificationSummary { + /// Most recently finalized header. + pub header: Block::Header, + /// A justification proving that the given block was finalized. + pub justification: Justification, +} + /// Import operation wrapper pub struct ClientImportOperation> { /// DB Operation. @@ -77,6 +88,8 @@ pub struct ClientImportOperation> { pub notify_imported: Option>, /// A list of hashes of blocks that got finalized. pub notify_finalized: Vec, + /// Best finalized block and accompanying justification. + pub notify_justified: Option>, } /// State of a new block. diff --git a/client/api/src/client.rs b/client/api/src/client.rs index 9ec86e6cda67e..88033d96cc672 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -67,6 +67,12 @@ pub trait BlockchainEvents { /// finalized block. fn finality_notification_stream(&self) -> FinalityNotifications; + /// Get a stream of Grandpa justifications alongside the best block that's + /// been finalized with that justification. + /// + /// Idk about the guarantees around the frequency of this yet + fn justification_notification_stream(&self) -> JustificationNotifications; + /// Get storage changes event stream. /// /// Passing `None` as `filter_keys` subscribes to all storage changes. @@ -255,6 +261,6 @@ pub struct FinalityNotification { pub struct JustificationNotification { /// Highest finalized block header pub header: Block::Header, - - // Should contain a GrandpaJustification + /// A justification that the given header has been finalized + pub justification: Justification, } diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index d3bbc1adb3cb9..8a02d39f36f76 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -387,7 +387,6 @@ impl Metrics { } } - /// The environment we run GRANDPA in. pub(crate) struct Environment, SC, VR> { pub(crate) client: Arc, diff --git a/client/src/client.rs b/client/src/client.rs index b1bc4f46bf6a9..ecfea68f5013e 100644 --- a/client/src/client.rs +++ b/client/src/client.rs @@ -64,12 +64,13 @@ use sc_block_builder::{BlockBuilderApi, BlockBuilderProvider}; pub use sc_client_api::{ backend::{ self, BlockImportOperation, PrunableStateChangesTrieStorage, - ClientImportOperation, Finalizer, ImportSummary, NewBlockState, + ClientImportOperation, Finalizer, ImportSummary, JustificationSummary, NewBlockState, changes_tries_state_at_block, StorageProvider, LockImportRun, }, client::{ ImportNotifications, FinalityNotification, FinalityNotifications, BlockImportNotification, + JustificationNotification, JustificationNotifications, ClientInfo, BlockchainEvents, BlockBackend, ProvideUncles, BadBlocks, ForkBlocks, BlockOf, }, @@ -205,15 +206,23 @@ impl LockImportRun for Client op: self.backend.begin_operation()?, notify_imported: None, notify_finalized: Vec::new(), + notify_justified: None, }; let r = f(&mut op)?; - let ClientImportOperation { op, notify_imported, notify_finalized } = op; + let ClientImportOperation { + op, + notify_imported, + notify_finalized, + notify_justified, + } = op; self.backend.commit_operation(op)?; + // Q: Does the order of these matter? self.notify_finalized(notify_finalized)?; self.notify_imported(notify_imported)?; + self.notify_justified(notify_justified)?; Ok(r) }; @@ -280,6 +289,7 @@ impl Client where storage_notifications: Default::default(), import_notification_sinks: Default::default(), finality_notification_sinks: Default::default(), + justification_notification_sinks: Default::default(), importing_block: Default::default(), block_rules: BlockRules::new(fork_blocks, bad_blocks), execution_extensions, @@ -885,7 +895,7 @@ impl Client where } assert_eq!(enacted.last().map(|e| e.hash), Some(block)); - operation.op.mark_finalized(BlockId::Hash(block), justification)?; + operation.op.mark_finalized(BlockId::Hash(block), justification.clone())?; if notify { // sometimes when syncing, tons of blocks can be finalized at once. @@ -896,6 +906,19 @@ impl Client where for finalized in &enacted[start..] { operation.notify_finalized.push(finalized.hash); } + + // Only want to notify if a block was "explicitly" finalized + // instead of one that was implicitly finlized + if let Some(justification) = justification { + // TODO: Check that this expect() holds true + let header = self.header(&BlockId::Hash(block))? + .expect("header already known to exist in DB because it is indicated in the tree route; qed"); + operation.notify_justified = Some(JustificationSummary { + header, + justification, + }); + } + } Ok(()) @@ -981,6 +1004,34 @@ impl Client where Ok(()) } + fn notify_justified( + &self, + notify_justified: Option>, + ) -> sp_blockchain::Result<()> { + let notify_justified = match notify_justified { + Some(notify_justified) => notify_justified, + None => { + // Clean up unused justification notification + // Is this neccesary though? + self.justification_notification_sinks + .lock() + .retain(|sink| !sink.is_closed()); + + return Ok(()); + } + }; + + let notification = JustificationNotification:: { + header: notify_justified.header, + justification: notify_justified.justification, + }; + + self.justification_notification_sinks.lock() + .retain(|sink| sink.unbounded_send(notification.clone()).is_ok()); + + Ok(()) + } + /// Attempts to revert the chain by `n` blocks guaranteeing that no block is /// reverted past the last finalized block. Returns the number of blocks /// that were successfully reverted. From 0095b78cbe83c66ba5313b708e67abe6439ec20e Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Sat, 18 Apr 2020 19:19:55 -0400 Subject: [PATCH 03/59] WIP: Add subscribers for justifications to Grandpa Adds the Sender end of a channel into Grandpa, through which notifications about block finality events can be sent. --- client/finality-grandpa/src/environment.rs | 22 ++++++++++++++++++++ client/finality-grandpa/src/import.rs | 7 ++++++- client/finality-grandpa/src/lib.rs | 24 +++++++++++++++++++++- client/finality-grandpa/src/observer.rs | 1 + 4 files changed, 52 insertions(+), 2 deletions(-) diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 8a02d39f36f76..227973bbfd7d1 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -44,6 +44,7 @@ use sc_telemetry::{telemetry, CONSENSUS_INFO}; use crate::{ CommandOrError, Commit, Config, Error, Precommit, Prevote, PrimaryPropose, SignedMessage, NewAuthoritySet, VoterCommand, + SharedFinalitySubscribers, JustificationNotification, }; use sp_consensus::SelectChain; @@ -400,6 +401,7 @@ pub(crate) struct Environment, SC, pub(crate) voter_set_state: SharedVoterSetState, pub(crate) voting_rule: VR, pub(crate) metrics: Option, + pub(crate) finality_subscribers: Option>, pub(crate) _phantom: PhantomData, } @@ -911,6 +913,7 @@ where number, (round, commit).into(), false, + self.finality_subscribers, ) } @@ -971,6 +974,7 @@ pub(crate) fn finalize_block( number: NumberFor, justification_or_commit: JustificationOrCommit, initial_sync: bool, + finality_subscribers: Option>, ) -> Result<(), CommandOrError>> where Block: BlockT, BE: Backend, @@ -982,6 +986,7 @@ pub(crate) fn finalize_block( let mut authority_set = authority_set.inner().write(); let status = client.info(); + if number <= status.finalized_number && client.hash(number)? == Some(hash) { // This can happen after a forced change (triggered by the finality tracker when finality is stalled), since // the voter will be restarted at the median last finalized block, which can be lower than the local best @@ -1076,6 +1081,23 @@ pub(crate) fn finalize_block( }, }; + if let Some(subscribers) = finality_subscribers { + // Q: We `finalized()` this at L37, so can I be sure + // that it's fine to unwrap here? + if let Some(justification) = justification { + let header = client.header(BlockId::Hash(hash))? + .expect(""); + let notification = JustificationNotification { + header, + justification, + }; + + for s in subscribers.iter() { + s.unbounded_send(notification); + } + } + } + debug!(target: "afg", "Finalizing blocks up to ({:?}, {})", number, hash); // ideally some handle to a synchronization oracle would be used diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index c1e32dfa6cc9e..7863ac1475ee0 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -37,7 +37,7 @@ use sp_runtime::traits::{ Block as BlockT, DigestFor, Header as HeaderT, NumberFor, Zero, }; -use crate::{Error, CommandOrError, NewAuthoritySet, VoterCommand}; +use crate::{Error, CommandOrError, NewAuthoritySet, VoterCommand, SharedFinalitySubscribers}; use crate::authorities::{AuthoritySet, SharedAuthoritySet, DelayKind, PendingChange}; use crate::consensus_changes::SharedConsensusChanges; use crate::environment::finalize_block; @@ -60,6 +60,7 @@ pub struct GrandpaBlockImport { send_voter_commands: TracingUnboundedSender>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: HashMap>>, + finality_subscribers: Option>, _phantom: PhantomData, } @@ -74,6 +75,7 @@ impl Clone for send_voter_commands: self.send_voter_commands.clone(), consensus_changes: self.consensus_changes.clone(), authority_set_hard_forks: self.authority_set_hard_forks.clone(), + finality_subscribers: self.finality_subscribers.clone(), _phantom: PhantomData, } } @@ -558,6 +560,7 @@ impl GrandpaBlockImport>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: Vec<(SetId, PendingChange>)>, + finality_subscribers: Option>, ) -> GrandpaBlockImport { // check for and apply any forced authority set hard fork that applies // to the *current* authority set. @@ -601,6 +604,7 @@ impl GrandpaBlockImport( } } +/// Justification for a finalized block. +pub struct JustificationNotification { + /// Highest finalized block header + pub header: Block::Header, + /// An encoded justification proving that the given header has been finalized + pub justification: Vec, +} + +// TODO: Figure out where to best have these types +type FinalitySubscriber = TracingUnboundedSender>; +pub type FinalitySubscribers = Vec>; + +pub type SharedFinalitySubscribers = Arc>; + /// Parameters used to run Grandpa. pub struct GrandpaParams { /// Configuration for the GRANDPA service. @@ -620,6 +635,8 @@ pub struct GrandpaParams { pub voting_rule: VR, /// The prometheus metrics registry. pub prometheus_registry: Option, + /// A list of subscribers to block justifications + pub finality_subscribers: Option>, } /// Run a GRANDPA voter as a task. Provide configuration and a link to a @@ -644,6 +661,7 @@ pub fn run_grandpa_voter( telemetry_on_connect, voting_rule, prometheus_registry, + finality_subscribers, } = grandpa_params; // NOTE: we have recently removed `run_grandpa_observer` from the public @@ -704,6 +722,7 @@ pub fn run_grandpa_voter( persistent_data, voter_commands_rx, prometheus_registry, + finality_subscribers, ); let voter_work = voter_work @@ -761,6 +780,7 @@ where persistent_data: PersistentData, voter_commands_rx: TracingUnboundedReceiver>>, prometheus_registry: Option, + finality_subscribers: Option>, ) -> Self { let metrics = match prometheus_registry.as_ref().map(Metrics::register) { Some(Ok(metrics)) => Some(metrics), @@ -784,6 +804,7 @@ where consensus_changes: persistent_data.consensus_changes.clone(), voter_set_state: persistent_data.set_state.clone(), metrics: metrics.as_ref().map(|m| m.environment.clone()), + finality_subscribers: finality_subscribers.clone(), _phantom: PhantomData, }); @@ -907,6 +928,7 @@ where network: self.env.network.clone(), voting_rule: self.env.voting_rule.clone(), metrics: self.env.metrics.clone(), + finality_subscribers: self.env.finality_subscribers.clone(), _phantom: PhantomData, }); diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index 1e6c8ddf18820..dcf76d29c43d8 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -125,6 +125,7 @@ fn grandpa_observer( finalized_number, (round, commit).into(), false, + None, // TODO: Should I include the finality_subscribers here? ) { Ok(_) => {}, Err(e) => return future::err(e), From 77192924356450604ba84bbf1e7c3cb1386dbe22 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 20 Apr 2020 23:15:01 -0400 Subject: [PATCH 04/59] WIP: Add a struct for managing subscriptions Slightly different approach from the last commit, but same basic idea. Still a rough sketch, very much doesn't compile yet. --- bin/node/cli/src/service.rs | 17 +++++-- client/finality-grandpa/src/finality_proof.rs | 47 +++++++++++++++++++ client/finality-grandpa/src/lib.rs | 25 +++------- 3 files changed, 67 insertions(+), 22 deletions(-) diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 0acd553ea0127..cdb93720df633 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -22,7 +22,12 @@ use std::sync::Arc; use sc_consensus_babe; use sc_client::{self, LongestChain}; -use grandpa::{self, FinalityProofProvider as GrandpaFinalityProofProvider, StorageAndProofProvider}; +use grandpa::{ + self, + FinalityProofProvider as GrandpaFinalityProofProvider, + StorageAndProofProvider, + FinalityProofSubscription, +}; use node_executor; use node_primitives::Block; use node_runtime::RuntimeApi; @@ -49,6 +54,7 @@ macro_rules! new_full_start { type RpcExtension = jsonrpc_core::IoHandler; let mut import_setup = None; let inherent_data_providers = sp_inherents::InherentDataProviders::new(); + let finality_proof_subscription = grandpa::FinalityProofSubscription::new(); let builder = sc_service::ServiceBuilder::new_full::< node_primitives::Block, node_runtime::RuntimeApi, node_executor::Executor @@ -89,6 +95,10 @@ macro_rules! new_full_start { Ok(import_queue) })? .with_rpc_extensions(|builder| -> std::result::Result { + + // TODO: Do something with this stream + let justification_stream = finality_proof_subscription.stream(); + let babe_link = import_setup.as_ref().map(|s| &s.2) .expect("BabeLink is present for full services or set up failed; qed."); let deps = node_rpc::FullDeps { @@ -105,7 +115,7 @@ macro_rules! new_full_start { Ok(node_rpc::create_full(deps)) })?; - (builder, import_setup, inherent_data_providers) + (builder, import_setup, inherent_data_providers, finality_tx) }} } @@ -131,7 +141,7 @@ macro_rules! new_full { $config.disable_grandpa, ); - let (builder, mut import_setup, inherent_data_providers) = new_full_start!($config); + let (builder, mut import_setup, inherent_data_providers, finality_tx) = new_full_start!($config); let service = builder .with_finality_proof_provider(|client, backend| { @@ -243,6 +253,7 @@ macro_rules! new_full { telemetry_on_connect: Some(service.telemetry_on_connect_stream()), voting_rule: grandpa::VotingRulesBuilder::default().build(), prometheus_registry: service.prometheus_registry(), + finality_subscribers: finality_proof_subscription.subscribers(), // Or I can pass a ref to the struct }; // the GRANDPA voter task is considered infallible, i.e. diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 2c85839b5e364..9b7e34452d7a3 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -52,6 +52,7 @@ use sp_runtime::{ use sp_core::storage::StorageKey; use sc_telemetry::{telemetry, CONSENSUS_INFO}; use sp_finality_grandpa::{AuthorityId, AuthorityList, VersionedAuthorityList, GRANDPA_AUTHORITIES_KEY}; +use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedSender, TracingUnboundedReceiver}; use crate::justification::GrandpaJustification; @@ -145,6 +146,52 @@ impl AuthoritySetForFinalityChecker for Arc { + /// Highest finalized block header + pub header: Block::Header, + /// An encoded justification proving that the given header has been finalized + pub justification: Vec, +} + +type JustificationStream = TracingUnboundedReceiver>; +type FinalitySubscriber = TracingUnboundedSender>; +type SharedFinalitySubscribers = Arc>>; + +pub struct FinalityProofSubscription { + subscribers: SharedFinalitySubscribers, +} + +impl FinalityProofSubscription { + pub fn new() -> Self { + Self { + subscribers: Arc::new(vec![]), + } + } + + pub fn subscribers(&self) -> SharedFinalitySubscribers { + self.subscribers.clone() + } + + pub fn notify(&self, notification: JustificationNotification) { + // TODO: Look at `notify_justified()` in `client` + for s in self.subscribers.iter() { + s.unbounded_send(notification); + } + + todo!() + } + + // This could also be called "subscribe()" + pub fn stream(&self) -> JustificationStream { + // TODO: I only want one channel to be created per instance + let (sink, stream) = tracing_unbounded("mpsc_justification_notification_stream"); + self.subscribers.push(sink); + stream + } +} + /// Finality proof provider for serving network requests. pub struct FinalityProofProvider { backend: Arc, diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index a36d4b001b9a5..cfcc4974c450d 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -127,6 +127,7 @@ use import::GrandpaBlockImport; use until_imported::UntilGlobalMessageBlocksImported; use communication::{NetworkBridge, Network as NetworkT}; use sp_finality_grandpa::{AuthorityList, AuthorityPair, AuthoritySignature, SetId}; +use crate::finality_proof::FinalityProofSubscription; // Re-export these two because it's just so damn convenient. pub use sp_finality_grandpa::{AuthorityId, ScheduledChange}; @@ -605,20 +606,6 @@ fn register_finality_tracker_inherent_data_provider( } } -/// Justification for a finalized block. -pub struct JustificationNotification { - /// Highest finalized block header - pub header: Block::Header, - /// An encoded justification proving that the given header has been finalized - pub justification: Vec, -} - -// TODO: Figure out where to best have these types -type FinalitySubscriber = TracingUnboundedSender>; -pub type FinalitySubscribers = Vec>; - -pub type SharedFinalitySubscribers = Arc>; - /// Parameters used to run Grandpa. pub struct GrandpaParams { /// Configuration for the GRANDPA service. @@ -635,8 +622,8 @@ pub struct GrandpaParams { pub voting_rule: VR, /// The prometheus metrics registry. pub prometheus_registry: Option, - /// A list of subscribers to block justifications - pub finality_subscribers: Option>, + /// A list of subscribers to block justifications -> TODO: Update + pub finality_subscription: Option>, } /// Run a GRANDPA voter as a task. Provide configuration and a link to a @@ -661,7 +648,7 @@ pub fn run_grandpa_voter( telemetry_on_connect, voting_rule, prometheus_registry, - finality_subscribers, + finality_subscription, } = grandpa_params; // NOTE: we have recently removed `run_grandpa_observer` from the public @@ -780,7 +767,7 @@ where persistent_data: PersistentData, voter_commands_rx: TracingUnboundedReceiver>>, prometheus_registry: Option, - finality_subscribers: Option>, + finality_subscription: Option>, ) -> Self { let metrics = match prometheus_registry.as_ref().map(Metrics::register) { Some(Ok(metrics)) => Some(metrics), @@ -804,7 +791,7 @@ where consensus_changes: persistent_data.consensus_changes.clone(), voter_set_state: persistent_data.set_state.clone(), metrics: metrics.as_ref().map(|m| m.environment.clone()), - finality_subscribers: finality_subscribers.clone(), + finality_subscribers: finality_subscription.subscribers(), _phantom: PhantomData, }); From 8c35b1e345cdedad772277548aa0e6f7d76b5d64 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 21 Apr 2020 15:30:53 -0400 Subject: [PATCH 05/59] Make naming more clear and lock data in Arc --- Cargo.lock | 8 +++---- bin/node/cli/src/service.rs | 2 +- client/finality-grandpa/src/environment.rs | 17 ++++++------- client/finality-grandpa/src/finality_proof.rs | 24 +++++++++++-------- client/finality-grandpa/src/import.rs | 13 +++++----- client/finality-grandpa/src/lib.rs | 20 ++++++++++------ 6 files changed, 48 insertions(+), 36 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 7b36a7d08a475..5aaf6dfa6d27d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -5538,18 +5538,18 @@ dependencies = [ [[package]] name = "ref-cast" -version = "1.0.0" +version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "077f197a31bfe7e4169145f9eca08d32705c6c6126c139c26793acdf163ac3ef" +checksum = "0a214c7875e1b63fc1618db7c80efc0954f6156c9ff07699fd9039e255accdd1" dependencies = [ "ref-cast-impl", ] [[package]] name = "ref-cast-impl" -version = "1.0.0" +version = "1.0.1" source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "c36eb52b69b87c9e3a07387f476c88fd0dba9a1713b38e56617ed66b45392c1f" +checksum = "602eb59cda66fcb9aec25841fb76bc01d2b34282dcdd705028da297db6f3eec8" dependencies = [ "proc-macro2", "quote 1.0.3", diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index cdb93720df633..5777876e006b1 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -253,7 +253,7 @@ macro_rules! new_full { telemetry_on_connect: Some(service.telemetry_on_connect_stream()), voting_rule: grandpa::VotingRulesBuilder::default().build(), prometheus_registry: service.prometheus_registry(), - finality_subscribers: finality_proof_subscription.subscribers(), // Or I can pass a ref to the struct + finality_subscribers: Some(finality_proof_subscription), }; // the GRANDPA voter task is considered infallible, i.e. diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 227973bbfd7d1..77c93fd089d74 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -44,7 +44,6 @@ use sc_telemetry::{telemetry, CONSENSUS_INFO}; use crate::{ CommandOrError, Commit, Config, Error, Precommit, Prevote, PrimaryPropose, SignedMessage, NewAuthoritySet, VoterCommand, - SharedFinalitySubscribers, JustificationNotification, }; use sp_consensus::SelectChain; @@ -52,6 +51,7 @@ use sp_consensus::SelectChain; use crate::authorities::{AuthoritySet, SharedAuthoritySet}; use crate::communication::Network as NetworkT; use crate::consensus_changes::SharedConsensusChanges; +use crate::finality_proof::{SharedFinalityNotifiers, JustificationNotification}; use crate::justification::GrandpaJustification; use crate::until_imported::UntilVoteTargetImported; use crate::voting_rule::VotingRule; @@ -401,7 +401,7 @@ pub(crate) struct Environment, SC, pub(crate) voter_set_state: SharedVoterSetState, pub(crate) voting_rule: VR, pub(crate) metrics: Option, - pub(crate) finality_subscribers: Option>, + pub(crate) finality_notifiers: Option>, pub(crate) _phantom: PhantomData, } @@ -913,7 +913,7 @@ where number, (round, commit).into(), false, - self.finality_subscribers, + self.finality_notifiers.clone(), ) } @@ -974,7 +974,7 @@ pub(crate) fn finalize_block( number: NumberFor, justification_or_commit: JustificationOrCommit, initial_sync: bool, - finality_subscribers: Option>, + finality_notifiers: Option>, ) -> Result<(), CommandOrError>> where Block: BlockT, BE: Backend, @@ -1081,10 +1081,10 @@ pub(crate) fn finalize_block( }, }; - if let Some(subscribers) = finality_subscribers { + if let Some(notifiers) = finality_notifiers { // Q: We `finalized()` this at L37, so can I be sure // that it's fine to unwrap here? - if let Some(justification) = justification { + if let Some(justification) = justification.clone() { let header = client.header(BlockId::Hash(hash))? .expect(""); let notification = JustificationNotification { @@ -1092,8 +1092,9 @@ pub(crate) fn finalize_block( justification, }; - for s in subscribers.iter() { - s.unbounded_send(notification); + for s in notifiers.read().iter() { + // TODO: Deal with Result + s.unbounded_send(notification.clone()); } } } diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 9b7e34452d7a3..066c879ebc7bd 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -36,6 +36,7 @@ use std::sync::Arc; use log::{trace, warn}; +use parking_lot::RwLock; use sp_blockchain::{Backend as BlockchainBackend, Error as ClientError, Result as ClientResult}; use sc_client_api::{ @@ -148,6 +149,7 @@ impl AuthoritySetForFinalityChecker for Arc { /// Highest finalized block header pub header: Block::Header, @@ -156,38 +158,40 @@ pub struct JustificationNotification { } type JustificationStream = TracingUnboundedReceiver>; -type FinalitySubscriber = TracingUnboundedSender>; -type SharedFinalitySubscribers = Arc>>; +pub type SharedFinalitySubscribers = Arc>>>; + +type FinalityNotifier = TracingUnboundedSender>; +pub type SharedFinalityNotifiers = Arc>>>; pub struct FinalityProofSubscription { - subscribers: SharedFinalitySubscribers, + notifiers: SharedFinalityNotifiers, } impl FinalityProofSubscription { pub fn new() -> Self { Self { - subscribers: Arc::new(vec![]), + notifiers: Arc::new(RwLock::new(vec![])), } } - pub fn subscribers(&self) -> SharedFinalitySubscribers { - self.subscribers.clone() + pub fn notifiers(&self) -> SharedFinalityNotifiers { + self.notifiers.clone() } + // Will notify subsribers (receivers) pub fn notify(&self, notification: JustificationNotification) { // TODO: Look at `notify_justified()` in `client` - for s in self.subscribers.iter() { - s.unbounded_send(notification); + for s in self.notifiers.read().iter() { + s.unbounded_send(notification.clone()); } todo!() } - // This could also be called "subscribe()" pub fn stream(&self) -> JustificationStream { // TODO: I only want one channel to be created per instance let (sink, stream) = tracing_unbounded("mpsc_justification_notification_stream"); - self.subscribers.push(sink); + self.notifiers.write().push(sink); stream } } diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index 7863ac1475ee0..0cfdb01326adf 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -37,11 +37,12 @@ use sp_runtime::traits::{ Block as BlockT, DigestFor, Header as HeaderT, NumberFor, Zero, }; -use crate::{Error, CommandOrError, NewAuthoritySet, VoterCommand, SharedFinalitySubscribers}; +use crate::{Error, CommandOrError, NewAuthoritySet, VoterCommand, }; use crate::authorities::{AuthoritySet, SharedAuthoritySet, DelayKind, PendingChange}; use crate::consensus_changes::SharedConsensusChanges; use crate::environment::finalize_block; use crate::justification::GrandpaJustification; +use crate::finality_proof::SharedFinalityNotifiers; use std::marker::PhantomData; /// A block-import handler for GRANDPA. @@ -60,7 +61,7 @@ pub struct GrandpaBlockImport { send_voter_commands: TracingUnboundedSender>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: HashMap>>, - finality_subscribers: Option>, + finality_notifiers: Option>, _phantom: PhantomData, } @@ -75,7 +76,7 @@ impl Clone for send_voter_commands: self.send_voter_commands.clone(), consensus_changes: self.consensus_changes.clone(), authority_set_hard_forks: self.authority_set_hard_forks.clone(), - finality_subscribers: self.finality_subscribers.clone(), + finality_notifiers: self.finality_notifiers.clone(), _phantom: PhantomData, } } @@ -560,7 +561,7 @@ impl GrandpaBlockImport>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: Vec<(SetId, PendingChange>)>, - finality_subscribers: Option>, + finality_notifiers: Option>, ) -> GrandpaBlockImport { // check for and apply any forced authority set hard fork that applies // to the *current* authority set. @@ -604,7 +605,7 @@ impl GrandpaBlockImport { pub voting_rule: VR, /// The prometheus metrics registry. pub prometheus_registry: Option, - /// A list of subscribers to block justifications -> TODO: Update + /// A subscription to new block justifications pub finality_subscription: Option>, } @@ -700,6 +700,12 @@ pub fn run_grandpa_voter( future::Either::Right(future::pending()) }; + let finality_notifiers = if let Some(s) = finality_subscription { + Some(s.notifiers()) + } else { + None + }; + let voter_work = VoterWork::new( client, config, @@ -709,7 +715,7 @@ pub fn run_grandpa_voter( persistent_data, voter_commands_rx, prometheus_registry, - finality_subscribers, + finality_notifiers, ); let voter_work = voter_work @@ -767,7 +773,7 @@ where persistent_data: PersistentData, voter_commands_rx: TracingUnboundedReceiver>>, prometheus_registry: Option, - finality_subscription: Option>, + finality_notifiers: Option>, ) -> Self { let metrics = match prometheus_registry.as_ref().map(Metrics::register) { Some(Ok(metrics)) => Some(metrics), @@ -791,7 +797,7 @@ where consensus_changes: persistent_data.consensus_changes.clone(), voter_set_state: persistent_data.set_state.clone(), metrics: metrics.as_ref().map(|m| m.environment.clone()), - finality_subscribers: finality_subscription.subscribers(), + finality_notifiers, _phantom: PhantomData, }); @@ -915,7 +921,7 @@ where network: self.env.network.clone(), voting_rule: self.env.voting_rule.clone(), metrics: self.env.metrics.clone(), - finality_subscribers: self.env.finality_subscribers.clone(), + finality_notifiers: self.env.finality_notifiers.clone(), _phantom: PhantomData, }); From 07ef5fa9e3439beb94f37d91c534f507bd898e7b Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 21 Apr 2020 15:40:11 -0400 Subject: [PATCH 06/59] Rough idea of what RPC would look like --- client/rpc/src/chain/mod.rs | 38 ++++++++----------------------------- 1 file changed, 8 insertions(+), 30 deletions(-) diff --git a/client/rpc/src/chain/mod.rs b/client/rpc/src/chain/mod.rs index 6a2aadb9e31e3..35122cec50db5 100644 --- a/client/rpc/src/chain/mod.rs +++ b/client/rpc/src/chain/mod.rs @@ -175,40 +175,18 @@ trait ChainBackend: Send + Sync + 'static } /// Subscribe to finality events. + /// + /// Informs subscribers about blocks with have beeen explicitly finalized + /// by Grandpa and thus contain a justification. fn subscribe_finality( &self, metadata: crate::metadata::Metadata, - subscriber: Subscriber + subscriber: Subscriber> ) { - let block_stream = match self.client().finality_notification_stream() { - Ok(stream) => stream, - Err(e) => { - todo!(); - } - }; - - let justification_stream = match self.client().justification_notification_stream() { - Ok(stream) => stream, - Err(e) => { - todo!(); - } - }; - - self.subscriptions().add(subscriber, |sink|{ - let header = self.client().header(BlockId::Hash(self.client().info().finalized_hash)) - .map_err(client_err) - .and_then(|header| { - header.ok_or_else(|| "Best header missing.".to_owned().into()) - }) - .map_err(Into::into); - - // How do I know that the right header is matched to the right justification? - // Should we be doing any filtering here? - // Probably depends on what we end up getting from the justification_stream - let stream = block_stream.zip(justification_stream); - - // stream::iter_result(vec![Ok(version)]) - // .chain(stream) + // TODO: Figure out how to get access to channel here + let justification_stream = self.grandpa_subscription().justification_notification_stream(); + + self.subscriptions().add(subscriber, |sink| { sink .sink_map_err(|e| warn!("Error sending notifications: {:?}", e)) .send_all(stream) From c9aa9e242d0ca36b846338266e47023e011ba89e Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 21 Apr 2020 23:20:16 -0400 Subject: [PATCH 07/59] Remove code from previous approach --- client/api/src/backend.rs | 13 --------- client/api/src/client.rs | 18 ------------ client/src/client.rs | 58 ++------------------------------------- 3 files changed, 2 insertions(+), 87 deletions(-) diff --git a/client/api/src/backend.rs b/client/api/src/backend.rs index a036f98826676..33a370c7cb2c5 100644 --- a/client/api/src/backend.rs +++ b/client/api/src/backend.rs @@ -69,17 +69,6 @@ pub struct ImportSummary { pub retracted: Vec, } -/// Summary of a Justification event. -/// -/// Contains the most recently finalized block as well -/// as a justification for its finalization. -pub struct JustificationSummary { - /// Most recently finalized header. - pub header: Block::Header, - /// A justification proving that the given block was finalized. - pub justification: Justification, -} - /// Import operation wrapper pub struct ClientImportOperation> { /// DB Operation. @@ -88,8 +77,6 @@ pub struct ClientImportOperation> { pub notify_imported: Option>, /// A list of hashes of blocks that got finalized. pub notify_finalized: Vec, - /// Best finalized block and accompanying justification. - pub notify_justified: Option>, } /// State of a new block. diff --git a/client/api/src/client.rs b/client/api/src/client.rs index 88033d96cc672..c855cd3a08328 100644 --- a/client/api/src/client.rs +++ b/client/api/src/client.rs @@ -36,9 +36,6 @@ pub type ImportNotifications = TracingUnboundedReceiver = TracingUnboundedReceiver>; -/// A stream of Grandpa Justification notifications. -pub type JustificationNotifications = TracingUnboundedReceiver>; - /// Expected hashes of blocks at given heights. /// /// This may be used as chain spec extension to set trusted checkpoints, i.e. @@ -67,12 +64,6 @@ pub trait BlockchainEvents { /// finalized block. fn finality_notification_stream(&self) -> FinalityNotifications; - /// Get a stream of Grandpa justifications alongside the best block that's - /// been finalized with that justification. - /// - /// Idk about the guarantees around the frequency of this yet - fn justification_notification_stream(&self) -> JustificationNotifications; - /// Get storage changes event stream. /// /// Passing `None` as `filter_keys` subscribes to all storage changes. @@ -255,12 +246,3 @@ pub struct FinalityNotification { /// Imported block header. pub header: Block::Header, } - -/// Justification for a finalized block. -#[derive(Clone, Debug)] -pub struct JustificationNotification { - /// Highest finalized block header - pub header: Block::Header, - /// A justification that the given header has been finalized - pub justification: Justification, -} diff --git a/client/src/client.rs b/client/src/client.rs index ecfea68f5013e..0be9ce5cd31b7 100644 --- a/client/src/client.rs +++ b/client/src/client.rs @@ -64,13 +64,12 @@ use sc_block_builder::{BlockBuilderApi, BlockBuilderProvider}; pub use sc_client_api::{ backend::{ self, BlockImportOperation, PrunableStateChangesTrieStorage, - ClientImportOperation, Finalizer, ImportSummary, JustificationSummary, NewBlockState, + ClientImportOperation, Finalizer, ImportSummary, NewBlockState, changes_tries_state_at_block, StorageProvider, LockImportRun, }, client::{ ImportNotifications, FinalityNotification, FinalityNotifications, BlockImportNotification, - JustificationNotification, JustificationNotifications, ClientInfo, BlockchainEvents, BlockBackend, ProvideUncles, BadBlocks, ForkBlocks, BlockOf, }, @@ -96,7 +95,6 @@ pub struct Client where Block: BlockT { storage_notifications: Mutex>, import_notification_sinks: Mutex>>>, finality_notification_sinks: Mutex>>>, - justification_notification_sinks: Mutex>>>, // holds the block hash currently being imported. TODO: replace this with block queue importing_block: RwLock>, block_rules: BlockRules, @@ -206,7 +204,6 @@ impl LockImportRun for Client op: self.backend.begin_operation()?, notify_imported: None, notify_finalized: Vec::new(), - notify_justified: None, }; let r = f(&mut op)?; @@ -215,14 +212,11 @@ impl LockImportRun for Client op, notify_imported, notify_finalized, - notify_justified, } = op; self.backend.commit_operation(op)?; - // Q: Does the order of these matter? self.notify_finalized(notify_finalized)?; self.notify_imported(notify_imported)?; - self.notify_justified(notify_justified)?; Ok(r) }; @@ -289,7 +283,6 @@ impl Client where storage_notifications: Default::default(), import_notification_sinks: Default::default(), finality_notification_sinks: Default::default(), - justification_notification_sinks: Default::default(), importing_block: Default::default(), block_rules: BlockRules::new(fork_blocks, bad_blocks), execution_extensions, @@ -625,7 +618,7 @@ impl Client where if let Ok(ImportResult::Imported(ref aux)) = result { if aux.is_new_best { use rand::Rng; - + // don't send telemetry block import events during initial sync for every // block to avoid spamming the telemetry server, these events will be randomly // sent at a rate of 1/10. @@ -906,19 +899,6 @@ impl Client where for finalized in &enacted[start..] { operation.notify_finalized.push(finalized.hash); } - - // Only want to notify if a block was "explicitly" finalized - // instead of one that was implicitly finlized - if let Some(justification) = justification { - // TODO: Check that this expect() holds true - let header = self.header(&BlockId::Hash(block))? - .expect("header already known to exist in DB because it is indicated in the tree route; qed"); - operation.notify_justified = Some(JustificationSummary { - header, - justification, - }); - } - } Ok(()) @@ -1004,34 +984,6 @@ impl Client where Ok(()) } - fn notify_justified( - &self, - notify_justified: Option>, - ) -> sp_blockchain::Result<()> { - let notify_justified = match notify_justified { - Some(notify_justified) => notify_justified, - None => { - // Clean up unused justification notification - // Is this neccesary though? - self.justification_notification_sinks - .lock() - .retain(|sink| !sink.is_closed()); - - return Ok(()); - } - }; - - let notification = JustificationNotification:: { - header: notify_justified.header, - justification: notify_justified.justification, - }; - - self.justification_notification_sinks.lock() - .retain(|sink| sink.unbounded_send(notification.clone()).is_ok()); - - Ok(()) - } - /// Attempts to revert the chain by `n` blocks guaranteeing that no block is /// reverted past the last finalized block. Returns the number of blocks /// that were successfully reverted. @@ -1829,12 +1781,6 @@ where stream } - fn justification_notification_stream(&self) -> JustificationNotifications { - let (sink, stream) = tracing_unbounded("mpsc_justification_notification_stream"); - self.justification_notification_sinks.lock().push(sink); - stream - } - /// Get storage changes event stream. fn storage_changes_notification_stream( &self, From 78250b774e1a9d261d0f6c1ed24a334a949fab37 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Tue, 21 Apr 2020 23:23:04 -0400 Subject: [PATCH 08/59] Missed some things --- client/src/client.rs | 10 +++------- 1 file changed, 3 insertions(+), 7 deletions(-) diff --git a/client/src/client.rs b/client/src/client.rs index 0be9ce5cd31b7..2a8040febf3ef 100644 --- a/client/src/client.rs +++ b/client/src/client.rs @@ -208,11 +208,7 @@ impl LockImportRun for Client let r = f(&mut op)?; - let ClientImportOperation { - op, - notify_imported, - notify_finalized, - } = op; + let ClientImportOperation { op, notify_imported, notify_finalized } = op; self.backend.commit_operation(op)?; self.notify_finalized(notify_finalized)?; @@ -618,7 +614,7 @@ impl Client where if let Ok(ImportResult::Imported(ref aux)) = result { if aux.is_new_best { use rand::Rng; - + // don't send telemetry block import events during initial sync for every // block to avoid spamming the telemetry server, these events will be randomly // sent at a rate of 1/10. @@ -888,7 +884,7 @@ impl Client where } assert_eq!(enacted.last().map(|e| e.hash), Some(block)); - operation.op.mark_finalized(BlockId::Hash(block), justification.clone())?; + operation.op.mark_finalized(BlockId::Hash(block), justification)?; if notify { // sometimes when syncing, tons of blocks can be finalized at once. From fe315005909a6ae26ef405ded29f0fd6fb31f809 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 22 Apr 2020 12:27:55 -0400 Subject: [PATCH 09/59] Update client/rpc-api/src/chain/mod.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Tomasz Drwięga --- client/rpc-api/src/chain/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/rpc-api/src/chain/mod.rs b/client/rpc-api/src/chain/mod.rs index 0bfacda2aebed..1af6a6b143f6b 100644 --- a/client/rpc-api/src/chain/mod.rs +++ b/client/rpc-api/src/chain/mod.rs @@ -122,7 +122,7 @@ pub trait ChainApi { #[pubsub( subscription = "grandpa_finality", unsubscribe, - name = "grandpa_subscribeToFinality", + name = "grandpa_unsubscribeJustifications", )] fn unsubscribe_finality( &self, From ba13ee6bca9b3727566699f81e40da18fe9a42a7 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 22 Apr 2020 12:28:12 -0400 Subject: [PATCH 10/59] Update client/rpc-api/src/chain/mod.rs MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Co-Authored-By: Tomasz Drwięga --- client/rpc-api/src/chain/mod.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/rpc-api/src/chain/mod.rs b/client/rpc-api/src/chain/mod.rs index 1af6a6b143f6b..58907eeb256c4 100644 --- a/client/rpc-api/src/chain/mod.rs +++ b/client/rpc-api/src/chain/mod.rs @@ -114,7 +114,7 @@ pub trait ChainApi { #[pubsub( subscription = "grandpa_finality", subscribe, - name = "grandpa_subscribeToFinality", + name = "grandpa_subscribeJustifications", )] fn subscribe_finality(&self, metadata: Self::Metadata, subscriber: Subscriber
); From fe360b9ac0f0fb428dba1bf8dbd23215365c4e5e Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Sun, 26 Apr 2020 13:49:16 -0400 Subject: [PATCH 11/59] Split justification subscription into sender and receiver halves --- client/finality-grandpa/src/environment.rs | 15 ++-- client/finality-grandpa/src/finality_proof.rs | 73 +++++++++++++------ client/finality-grandpa/src/import.rs | 12 +-- client/finality-grandpa/src/lib.rs | 20 ++--- client/finality-grandpa/src/observer.rs | 2 +- 5 files changed, 71 insertions(+), 51 deletions(-) diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 77c93fd089d74..0b16b9680619e 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -51,7 +51,7 @@ use sp_consensus::SelectChain; use crate::authorities::{AuthoritySet, SharedAuthoritySet}; use crate::communication::Network as NetworkT; use crate::consensus_changes::SharedConsensusChanges; -use crate::finality_proof::{SharedFinalityNotifiers, JustificationNotification}; +use crate::finality_proof::{GrandpaJustificationSender, JustificationNotification}; use crate::justification::GrandpaJustification; use crate::until_imported::UntilVoteTargetImported; use crate::voting_rule::VotingRule; @@ -401,7 +401,7 @@ pub(crate) struct Environment, SC, pub(crate) voter_set_state: SharedVoterSetState, pub(crate) voting_rule: VR, pub(crate) metrics: Option, - pub(crate) finality_notifiers: Option>, + pub(crate) finality_subscription: Option>, pub(crate) _phantom: PhantomData, } @@ -913,7 +913,7 @@ where number, (round, commit).into(), false, - self.finality_notifiers.clone(), + &self.finality_subscription, ) } @@ -974,7 +974,7 @@ pub(crate) fn finalize_block( number: NumberFor, justification_or_commit: JustificationOrCommit, initial_sync: bool, - finality_notifiers: Option>, + finality_subscription: &Option>, ) -> Result<(), CommandOrError>> where Block: BlockT, BE: Backend, @@ -1081,7 +1081,7 @@ pub(crate) fn finalize_block( }, }; - if let Some(notifiers) = finality_notifiers { + if let Some(subscription) = finality_subscription { // Q: We `finalized()` this at L37, so can I be sure // that it's fine to unwrap here? if let Some(justification) = justification.clone() { @@ -1092,10 +1092,7 @@ pub(crate) fn finalize_block( justification, }; - for s in notifiers.read().iter() { - // TODO: Deal with Result - s.unbounded_send(notification.clone()); - } + subscription.notify(notification); } } diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 066c879ebc7bd..9a88943fd626c 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -147,7 +147,6 @@ impl AuthoritySetForFinalityChecker for Arc { @@ -158,46 +157,76 @@ pub struct JustificationNotification { } type JustificationStream = TracingUnboundedReceiver>; -pub type SharedFinalitySubscribers = Arc>>>; - type FinalityNotifier = TracingUnboundedSender>; -pub type SharedFinalityNotifiers = Arc>>>; +type SharedFinalityNotifiers = Arc>>>; -pub struct FinalityProofSubscription { - notifiers: SharedFinalityNotifiers, +/// The sending half of the Grandpa justification channel. +/// +/// Used to send notifictions about justifications generated +/// at the end of a Grandpa round. +#[derive(Clone)] +pub struct GrandpaJustificationSender { + notifiers: SharedFinalityNotifiers } -impl FinalityProofSubscription { - pub fn new() -> Self { +impl GrandpaJustificationSender { + /// The `notifiers` should be shared with a corresponding + /// `GrandpaJustificationReceiver`. + pub fn new(notifiers: SharedFinalityNotifiers) -> Self { Self { - notifiers: Arc::new(RwLock::new(vec![])), + notifiers, } } - pub fn notifiers(&self) -> SharedFinalityNotifiers { - self.notifiers.clone() - } + /// Send out a notification to all subscribers that a new justification + /// is available for a block. + pub fn notify(&self, notification: JustificationNotification) -> Result<(), ()> { + { + // Clean up any closed sinks + self.notifiers.write().retain(|n| !n.is_closed()); + } - // Will notify subsribers (receivers) - pub fn notify(&self, notification: JustificationNotification) { - // TODO: Look at `notify_justified()` in `client` for s in self.notifiers.read().iter() { s.unbounded_send(notification.clone()); } - todo!() + Ok(()) + } +} + +/// The receiving half of the Grandpa justification channel. +/// +/// Used to recieve notifictions about justifications generated +/// at the end of a Grandpa round. +#[derive(Clone)] +pub struct GrandpaJustificationReceiver { + notifiers: SharedFinalityNotifiers +} + +impl GrandpaJustificationReceiver { + /// Crate a new receiver of justification notifications. + /// + /// The `notifiers` should be shared with a corresponding + /// `GrandpaJustificationSender`. + pub fn new(notifiers: SharedFinalityNotifiers) -> Self { + Self { + notifiers, + } } - pub fn stream(&self) -> JustificationStream { - // TODO: I only want one channel to be created per instance - let (sink, stream) = tracing_unbounded("mpsc_justification_notification_stream"); - self.notifiers.write().push(sink); - stream + /// Subscribe to a channel through which justifications are sent + /// at the end of each Grandpa voting round. + pub fn subscribe(&self) -> JustificationStream { + let (sender, receiver) = tracing_unbounded("mpsc_justification_notification_stream"); + self.notifiers.write().push(sender); + receiver } } + + /// Finality proof provider for serving network requests. -pub struct FinalityProofProvider { +pub struct FinalityProofProvider { backend: Arc, authority_provider: Arc>, } diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index 0cfdb01326adf..05315db6409ec 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -42,7 +42,7 @@ use crate::authorities::{AuthoritySet, SharedAuthoritySet, DelayKind, PendingCha use crate::consensus_changes::SharedConsensusChanges; use crate::environment::finalize_block; use crate::justification::GrandpaJustification; -use crate::finality_proof::SharedFinalityNotifiers; +use crate::finality_proof::GrandpaJustificationSender; use std::marker::PhantomData; /// A block-import handler for GRANDPA. @@ -61,7 +61,7 @@ pub struct GrandpaBlockImport { send_voter_commands: TracingUnboundedSender>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: HashMap>>, - finality_notifiers: Option>, + finality_subscription: Option>, _phantom: PhantomData, } @@ -76,7 +76,7 @@ impl Clone for send_voter_commands: self.send_voter_commands.clone(), consensus_changes: self.consensus_changes.clone(), authority_set_hard_forks: self.authority_set_hard_forks.clone(), - finality_notifiers: self.finality_notifiers.clone(), + finality_subscription: self.finality_subscription.clone(), _phantom: PhantomData, } } @@ -561,7 +561,7 @@ impl GrandpaBlockImport>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: Vec<(SetId, PendingChange>)>, - finality_notifiers: Option>, + finality_subscription: Option>, ) -> GrandpaBlockImport { // check for and apply any forced authority set hard fork that applies // to the *current* authority set. @@ -605,7 +605,7 @@ impl GrandpaBlockImport { /// The prometheus metrics registry. pub prometheus_registry: Option, /// A subscription to new block justifications - pub finality_subscription: Option>, + pub finality_subscription: Option>, } /// Run a GRANDPA voter as a task. Provide configuration and a link to a @@ -700,12 +700,6 @@ pub fn run_grandpa_voter( future::Either::Right(future::pending()) }; - let finality_notifiers = if let Some(s) = finality_subscription { - Some(s.notifiers()) - } else { - None - }; - let voter_work = VoterWork::new( client, config, @@ -715,7 +709,7 @@ pub fn run_grandpa_voter( persistent_data, voter_commands_rx, prometheus_registry, - finality_notifiers, + finality_subscription, ); let voter_work = voter_work @@ -773,7 +767,7 @@ where persistent_data: PersistentData, voter_commands_rx: TracingUnboundedReceiver>>, prometheus_registry: Option, - finality_notifiers: Option>, + finality_subscription: Option>, ) -> Self { let metrics = match prometheus_registry.as_ref().map(Metrics::register) { Some(Ok(metrics)) => Some(metrics), @@ -797,7 +791,7 @@ where consensus_changes: persistent_data.consensus_changes.clone(), voter_set_state: persistent_data.set_state.clone(), metrics: metrics.as_ref().map(|m| m.environment.clone()), - finality_notifiers, + finality_subscription, _phantom: PhantomData, }); @@ -921,7 +915,7 @@ where network: self.env.network.clone(), voting_rule: self.env.voting_rule.clone(), metrics: self.env.metrics.clone(), - finality_notifiers: self.env.finality_notifiers.clone(), + finality_subscription: self.env.finality_subscription.clone(), _phantom: PhantomData, }); diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index dcf76d29c43d8..9c8edb7e205d3 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -125,7 +125,7 @@ fn grandpa_observer( finalized_number, (round, commit).into(), false, - None, // TODO: Should I include the finality_subscribers here? + &None, // TODO: Should I include the finality_subscribers here? ) { Ok(_) => {}, Err(e) => return future::err(e), From 254bebe88607dc60130d20892bf53cc6796af285 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Sun, 26 Apr 2020 14:34:37 -0400 Subject: [PATCH 12/59] Replace RwLock with a Mutex --- client/finality-grandpa/src/finality_proof.rs | 18 +++++++----------- 1 file changed, 7 insertions(+), 11 deletions(-) diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 9a88943fd626c..dea86ee603193 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -36,7 +36,7 @@ use std::sync::Arc; use log::{trace, warn}; -use parking_lot::RwLock; +use parking_lot::Mutex; use sp_blockchain::{Backend as BlockchainBackend, Error as ClientError, Result as ClientResult}; use sc_client_api::{ @@ -158,7 +158,7 @@ pub struct JustificationNotification { type JustificationStream = TracingUnboundedReceiver>; type FinalityNotifier = TracingUnboundedSender>; -type SharedFinalityNotifiers = Arc>>>; +type SharedFinalityNotifiers = Arc>>>; /// The sending half of the Grandpa justification channel. /// @@ -181,14 +181,10 @@ impl GrandpaJustificationSender { /// Send out a notification to all subscribers that a new justification /// is available for a block. pub fn notify(&self, notification: JustificationNotification) -> Result<(), ()> { - { - // Clean up any closed sinks - self.notifiers.write().retain(|n| !n.is_closed()); - } - - for s in self.notifiers.read().iter() { - s.unbounded_send(notification.clone()); - } + self.notifiers.lock() + .retain(|n| { + !n.is_closed() || n.unbounded_send(notification.clone()).is_ok() + }); Ok(()) } @@ -218,7 +214,7 @@ impl GrandpaJustificationReceiver { /// at the end of each Grandpa voting round. pub fn subscribe(&self) -> JustificationStream { let (sender, receiver) = tracing_unbounded("mpsc_justification_notification_stream"); - self.notifiers.write().push(sender); + self.notifiers.lock().push(sender); receiver } } From 9c996ea6494c1d8aa9090f5ab9a051bc8d235009 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Sun, 26 Apr 2020 14:37:32 -0400 Subject: [PATCH 13/59] Add sample usage from the Service's point of view --- Cargo.lock | 1 + bin/node/cli/src/service.rs | 27 ++++++++++++++++++--------- bin/node/rpc/Cargo.toml | 1 + bin/node/rpc/src/lib.rs | 13 +++++++++++++ 4 files changed, 33 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5aaf6dfa6d27d..efe4639305c70 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3535,6 +3535,7 @@ dependencies = [ "sc-consensus-babe", "sc-consensus-babe-rpc", "sc-consensus-epochs", + "sc-finality-grandpa", "sc-keystore", "sp-api", "sp-blockchain", diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 5777876e006b1..7525a0719d64d 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -26,7 +26,8 @@ use grandpa::{ self, FinalityProofProvider as GrandpaFinalityProofProvider, StorageAndProofProvider, - FinalityProofSubscription, + GrandpaJustificationSender, + GrandpaJustificationReceiver, }; use node_executor; use node_primitives::Block; @@ -44,6 +45,8 @@ use node_executor::NativeExecutor; use sc_network::NetworkService; use sc_offchain::OffchainWorkers; +use parking_lot::Mutex; + /// Starts a `ServiceBuilder` for a full service. /// /// Use this macro if you don't actually need the full service, but just the builder in order to @@ -54,7 +57,13 @@ macro_rules! new_full_start { type RpcExtension = jsonrpc_core::IoHandler; let mut import_setup = None; let inherent_data_providers = sp_inherents::InherentDataProviders::new(); - let finality_proof_subscription = grandpa::FinalityProofSubscription::new(); + + // Q: Is there any way to enforce that they point to the same set of notifiers? + let finality_notifiers = Arc::new(Mutex::new(vec![])); + let justification_receiver = + grandpa::GrandpaJustificationReceiver::new(finality_notifiers.clone()); + let justification_sender = + grandpa::GrandpaJustificationReceiver::new(finality_notifiers.clone()); let builder = sc_service::ServiceBuilder::new_full::< node_primitives::Block, node_runtime::RuntimeApi, node_executor::Executor @@ -95,10 +104,6 @@ macro_rules! new_full_start { Ok(import_queue) })? .with_rpc_extensions(|builder| -> std::result::Result { - - // TODO: Do something with this stream - let justification_stream = finality_proof_subscription.stream(); - let babe_link = import_setup.as_ref().map(|s| &s.2) .expect("BabeLink is present for full services or set up failed; qed."); let deps = node_rpc::FullDeps { @@ -110,12 +115,15 @@ macro_rules! new_full_start { keystore: builder.keystore(), babe_config: sc_consensus_babe::BabeLink::config(babe_link).clone(), shared_epoch_changes: sc_consensus_babe::BabeLink::epoch_changes(babe_link).clone() + }, + grandpa: node_rpc::GrandpaDeps { + justification_receiver } }; Ok(node_rpc::create_full(deps)) })?; - (builder, import_setup, inherent_data_providers, finality_tx) + (builder, import_setup, inherent_data_providers, justification_sender) }} } @@ -141,7 +149,8 @@ macro_rules! new_full { $config.disable_grandpa, ); - let (builder, mut import_setup, inherent_data_providers, finality_tx) = new_full_start!($config); + let (builder, mut import_setup, inherent_data_providers, justification_sender) = + new_full_start!($config); let service = builder .with_finality_proof_provider(|client, backend| { @@ -253,7 +262,7 @@ macro_rules! new_full { telemetry_on_connect: Some(service.telemetry_on_connect_stream()), voting_rule: grandpa::VotingRulesBuilder::default().build(), prometheus_registry: service.prometheus_registry(), - finality_subscribers: Some(finality_proof_subscription), + finality_subscription: Some(justification_sender), }; // the GRANDPA voter task is considered infallible, i.e. diff --git a/bin/node/rpc/Cargo.toml b/bin/node/rpc/Cargo.toml index d6db49ebf01a6..7d4bee70e503b 100644 --- a/bin/node/rpc/Cargo.toml +++ b/bin/node/rpc/Cargo.toml @@ -28,3 +28,4 @@ sc-keystore = { version = "2.0.0-dev", path = "../../../client/keystore" } sc-consensus-epochs = { version = "0.8.0-dev", path = "../../../client/consensus/epochs" } sp-consensus = { version = "0.8.0-dev", path = "../../../primitives/consensus/common" } sp-blockchain = { version = "2.0.0-dev", path = "../../../primitives/blockchain" } +sc-finality-grandpa = { version = "0.8.0-dev", path = "../../../client/finality-grandpa" } diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index 4e1cfa56733a7..024b49cafc32e 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -42,6 +42,7 @@ use sp_consensus_babe::BabeApi; use sc_consensus_epochs::SharedEpochChanges; use sc_consensus_babe::{Config, Epoch}; use sc_consensus_babe_rpc::BabeRPCHandler; +use sc_finality_grandpa::GrandpaJustificationReceiver; /// Light client extra dependencies. pub struct LightDeps { @@ -65,6 +66,12 @@ pub struct BabeDeps { pub keystore: KeyStorePtr, } +/// Extra dependencies for GRANDPA. +pub struct GrandpaDeps { + /// Receives notifications about justification events from Grandpa. + pub justification_receiver: GrandpaJustificationReceiver, +} + /// Full client dependencies. pub struct FullDeps { /// The client instance to use. @@ -75,6 +82,8 @@ pub struct FullDeps { pub select_chain: SC, /// BABE specific dependencies. pub babe: BabeDeps, + /// GRANDPA specific dependencies. + pub grandpa: GrandpaDeps, } /// Instantiate all Full RPC extensions. @@ -104,12 +113,15 @@ pub fn create_full( select_chain, babe } = deps; + let BabeDeps { keystore, babe_config, shared_epoch_changes, } = babe; + let GrandpaDeps { justification_receiver } = grandpa; + io.extend_with( SystemApi::to_delegate(FullSystem::new(client.clone(), pool)) ); @@ -127,6 +139,7 @@ pub fn create_full( BabeRPCHandler::new(client, shared_epoch_changes, keystore, babe_config, select_chain) ) ); + io.extend_with(todo!()); io } From f42b6c389498b1033b18f44e9ca707cbc0493cce Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Fri, 15 May 2020 13:21:30 -0400 Subject: [PATCH 14/59] Remove code that referred to "chain_" RPC --- client/rpc-api/src/chain/mod.rs | 20 ----------------- client/rpc/src/chain/mod.rs | 39 --------------------------------- client/rpc/src/chain/tests.rs | 33 ---------------------------- 3 files changed, 92 deletions(-) diff --git a/client/rpc-api/src/chain/mod.rs b/client/rpc-api/src/chain/mod.rs index 0cad109173af8..13afa3ffc6a87 100644 --- a/client/rpc-api/src/chain/mod.rs +++ b/client/rpc-api/src/chain/mod.rs @@ -110,24 +110,4 @@ pub trait ChainApi { metadata: Option, id: SubscriptionId, ) -> RpcResult; - - /// Subscribe to finality events. - #[pubsub( - subscription = "grandpa_finality", - subscribe, - name = "grandpa_subscribeJustifications", - )] - fn subscribe_finality(&self, metadata: Self::Metadata, subscriber: Subscriber
); - - /// Unsubscribe to finality events. - #[pubsub( - subscription = "grandpa_finality", - unsubscribe, - name = "grandpa_unsubscribeJustifications", - )] - fn unsubscribe_finality( - &self, - metadata: Option, - id: SubscriptionId, - ) -> RpcResult; } diff --git a/client/rpc/src/chain/mod.rs b/client/rpc/src/chain/mod.rs index 2fc2a84279702..23e43e57fb53b 100644 --- a/client/rpc/src/chain/mod.rs +++ b/client/rpc/src/chain/mod.rs @@ -171,36 +171,6 @@ trait ChainBackend: Send + Sync + 'static ) -> RpcResult { Ok(self.subscriptions().cancel(id)) } - - /// Subscribe to finality events. - /// - /// Informs subscribers about blocks with have beeen explicitly finalized - /// by Grandpa and thus contain a justification. - fn subscribe_finality( - &self, - metadata: crate::metadata::Metadata, - subscriber: Subscriber> - ) { - // TODO: Figure out how to get access to channel here - let justification_stream = self.grandpa_subscription().justification_notification_stream(); - - self.subscriptions().add(subscriber, |sink| { - sink - .sink_map_err(|e| warn!("Error sending notifications: {:?}", e)) - .send_all(stream) - // we ignore the resulting Stream (if the first stream is over we are unsubscribed) - .map(|_| ()) - }); - } - - /// Unsubscribe to finality events. - fn unsubscribe_finality( - &self, - metadata: Option, - id: SubscriptionId, - ) -> RpcResult { - Ok(self.Subscriptions.cancel(id)) - } } /// Create new state API that works on full node. @@ -303,15 +273,6 @@ impl ChainApi, Block::Hash, Block::Header, Signe fn unsubscribe_finalized_heads(&self, metadata: Option, id: SubscriptionId) -> RpcResult { self.backend.unsubscribe_finalized_heads(metadata, id) } - - // TODO: Figure out what `T` should be for `Subscriber` - fn subscribe_finality(&self, metadata: Self::Metadata, subscriber: Subscriber) { - self.backend.subscribe_finality(metadata, subscriber) - } - - fn unsubscribe_finality(&self, metadata: Option, id: SubscriptionId) -> RpcResult { - self.backend.unsubscribe_finality(metadata, id) - } } /// Subscribe to new headers. diff --git a/client/rpc/src/chain/tests.rs b/client/rpc/src/chain/tests.rs index c729063bb129e..3c8ee5c817851 100644 --- a/client/rpc/src/chain/tests.rs +++ b/client/rpc/src/chain/tests.rs @@ -262,36 +262,3 @@ fn should_notify_about_finalized_block() { // no more notifications on this channel assert_eq!(executor::block_on(next.into_future().compat()).unwrap().0, None); } - -#[test] -fn should_notify_about_finality_event() { - let mut core = ::tokio::runtime::Runtime::new().unwrap(); - let remote = core.executor(); - let (subscriber, id, transport) = Subscriber::new_test("test"); - - { - let mut client = Arc::new(substrate_test_runtime_client::new()); - let api = new_full(client.clone(), Subscriptions::new(Arc::new(remote))); - - api.subscribe_finality(Default::default(), subscriber); - - // assert id assigned - assert_eq!(core.block_on(id), Ok(Ok(SubscriptionId::Number(1)))); - - let block = client.new_block(Default::default()).unwrap().build().unwrap().block; - client.import(BlockOrigin::Own, block).unwrap(); - // Do I need some way to trigger a "justification" event aside from - // simply finalizing a block? - // Maybe something to do with a voting round ending? - client.finalize_block(BlockId::number(1), None).unwrap(); - } - - // assert initial head sent. - let (notification, next) = core.block_on(transport.into_future()).unwrap(); - assert!(notification.is_some()); - // assert notification sent to transport - let (notification, next) = core.block_on(next.into_future()).unwrap(); - assert!(notification.is_some()); - // no more notifications on this channel - assert_eq!(core.block_on(next.into_future()).unwrap().0, None); -} From d61d7ecacefca2bccbe30c5bf973962b7af551df Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Sun, 17 May 2020 17:39:38 -0400 Subject: [PATCH 15/59] Use the Justification sender/receivers from Grandpa LinkHalf --- bin/node/cli/src/service.rs | 15 +++----- client/finality-grandpa/src/environment.rs | 30 ++++++++-------- client/finality-grandpa/src/import.rs | 12 +++---- client/finality-grandpa/src/lib.rs | 42 ++++++++++++++++------ client/finality-grandpa/src/observer.rs | 15 ++++++-- 5 files changed, 68 insertions(+), 46 deletions(-) diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 7cad0e39cf7c7..7fb1ac69dca5e 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -5,7 +5,7 @@ // 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 +// 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, @@ -54,13 +54,6 @@ macro_rules! new_full_start { let mut rpc_setup = None; let inherent_data_providers = sp_inherents::InherentDataProviders::new(); - // Q: Is there any way to enforce that they point to the same set of notifiers? - let finality_notifiers = Arc::new(Mutex::new(vec![])); - let justification_receiver = - grandpa::GrandpaJustificationReceiver::new(finality_notifiers.clone()); - let justification_sender = - grandpa::GrandpaJustificationSender::new(finality_notifiers.clone()); - let builder = sc_service::ServiceBuilder::new_full::< node_primitives::Block, node_runtime::RuntimeApi, node_executor::Executor >($config)? @@ -115,9 +108,12 @@ macro_rules! new_full_start { .with_rpc_extensions(|builder| -> std::result::Result { let babe_link = import_setup.as_ref().map(|s| &s.2) .expect("BabeLink is present for full services or set up failed; qed."); + let grandpa_link = import_setup.as_ref().map(|s| &s.1) .expect("GRANDPA LinkHalf is present for full services or set up failed; qed."); let shared_authority_set = grandpa_link.shared_authority_set(); + let justification_receiver = grandpa_link.justification_receiver(); + let shared_voter_state = grandpa::SharedVoterState::empty(); let deps = node_rpc::FullDeps { client: builder.client().clone(), @@ -132,7 +128,7 @@ macro_rules! new_full_start { grandpa: node_rpc::GrandpaDeps { shared_voter_state: shared_voter_state.clone(), shared_authority_set: shared_authority_set.clone(), - justification_receiver + justification_receiver, }, }; rpc_setup = Some((shared_voter_state)); @@ -282,7 +278,6 @@ macro_rules! new_full { voting_rule: grandpa::VotingRulesBuilder::default().build(), prometheus_registry: service.prometheus_registry(), shared_voter_state, - finality_subscription: Some(justification_sender), }; // the GRANDPA voter task is considered infallible, i.e. diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index e82d77dc3896a..ecf8a4d5229e2 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -5,7 +5,7 @@ // 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 +// 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, @@ -404,7 +404,7 @@ pub(crate) struct Environment, SC, pub(crate) voter_set_state: SharedVoterSetState, pub(crate) voting_rule: VR, pub(crate) metrics: Option, - pub(crate) finality_subscription: Option>, + pub(crate) justification_sender: GrandpaJustificationSender, pub(crate) _phantom: PhantomData, } @@ -1009,7 +1009,7 @@ where number, (round, commit).into(), false, - &self.finality_subscription, + &self.justification_sender, ) } @@ -1074,7 +1074,7 @@ pub(crate) fn finalize_block( number: NumberFor, justification_or_commit: JustificationOrCommit, initial_sync: bool, - finality_subscription: &Option>, + justification_sender: &GrandpaJustificationSender, ) -> Result<(), CommandOrError>> where Block: BlockT, BE: Backend, @@ -1181,19 +1181,17 @@ pub(crate) fn finalize_block( }, }; - if let Some(subscription) = finality_subscription { - // Q: We `finalized()` this at L37, so can I be sure - // that it's fine to unwrap here? - if let Some(justification) = justification.clone() { - let header = client.header(BlockId::Hash(hash))? - .expect(""); - let notification = JustificationNotification { - header, - justification, - }; + // Q: We `finalized()` this at L37, so can I be sure + // that it's fine to unwrap here? + if let Some(justification) = justification.clone() { + let header = client.header(BlockId::Hash(hash))? + .expect(""); + let notification = JustificationNotification { + header, + justification, + }; - subscription.notify(notification); - } + justification_sender.notify(notification); } debug!(target: "afg", "Finalizing blocks up to ({:?}, {})", number, hash); diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index 2cd781668cf89..6ff41140f82ba 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -5,7 +5,7 @@ // 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 +// 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, @@ -62,7 +62,7 @@ pub struct GrandpaBlockImport { send_voter_commands: TracingUnboundedSender>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: HashMap>>, - finality_subscription: Option>, + justification_sender: GrandpaJustificationSender, _phantom: PhantomData, } @@ -77,7 +77,7 @@ impl Clone for send_voter_commands: self.send_voter_commands.clone(), consensus_changes: self.consensus_changes.clone(), authority_set_hard_forks: self.authority_set_hard_forks.clone(), - finality_subscription: self.finality_subscription.clone(), + justification_sender: self.justification_sender.clone(), _phantom: PhantomData, } } @@ -562,7 +562,7 @@ impl GrandpaBlockImport>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: Vec<(SetId, PendingChange>)>, - finality_subscription: Option>, + justification_sender: GrandpaJustificationSender, ) -> GrandpaBlockImport { // check for and apply any forced authority set hard fork that applies // to the *current* authority set. @@ -606,7 +606,7 @@ impl GrandpaBlockImport { select_chain: SC, persistent_data: PersistentData, voter_commands_rx: TracingUnboundedReceiver>>, + justification_sender: GrandpaJustificationSender, + justification_receiver: GrandpaJustificationReceiver, } impl LinkHalf { @@ -447,6 +449,16 @@ impl LinkHalf { pub fn shared_authority_set(&self) -> &SharedAuthoritySet> { &self.persistent_data.authority_set } + + /// Get the sender end of justification notifications. + pub fn justification_sender(&self) -> GrandpaJustificationSender { + self.justification_sender.clone() + } + + /// Get the receiving end of justification notifications. + pub fn justification_receiver(&self) -> GrandpaJustificationReceiver { + self.justification_receiver.clone() + } } /// Provider for the Grandpa authority set configured on the genesis block. @@ -545,6 +557,13 @@ where let (voter_commands_tx, voter_commands_rx) = tracing_unbounded("mpsc_grandpa_voter_command"); + // Q: Is there any way to enforce that they point to the same set of notifiers? + let finality_notifiers = Arc::new(Mutex::new(vec![])); + let justification_receiver = + GrandpaJustificationReceiver::new(finality_notifiers.clone()); + let justification_sender = + GrandpaJustificationSender::new(finality_notifiers.clone()); + // create pending change objects with 0 delay and enacted on finality // (i.e. standard changes) for each authority set hard fork. let authority_set_hard_forks = authority_set_hard_forks @@ -571,13 +590,15 @@ where voter_commands_tx, persistent_data.consensus_changes.clone(), authority_set_hard_forks, - None, //finality_notifiers.clone(), // TODO: Figure out how to get subs here + justification_sender.clone(), ), LinkHalf { client, select_chain, persistent_data, voter_commands_rx, + justification_sender, + justification_receiver, }, )) } @@ -673,8 +694,6 @@ pub struct GrandpaParams { pub prometheus_registry: Option, /// The voter state is exposed at an RPC endpoint. pub shared_voter_state: SharedVoterState, - /// A subscription to new block justifications - pub finality_subscription: Option>, } /// Run a GRANDPA voter as a task. Provide configuration and a link to a @@ -701,7 +720,6 @@ pub fn run_grandpa_voter( voting_rule, prometheus_registry, shared_voter_state, - finality_subscription, } = grandpa_params; // NOTE: we have recently removed `run_grandpa_observer` from the public @@ -715,6 +733,8 @@ pub fn run_grandpa_voter( select_chain, persistent_data, voter_commands_rx, + justification_sender, + justification_receiver: _, } = link; let network = NetworkBridge::new( @@ -763,7 +783,7 @@ pub fn run_grandpa_voter( voter_commands_rx, prometheus_registry, shared_voter_state, - finality_subscription, + justification_sender, ); let voter_work = voter_work @@ -824,7 +844,7 @@ where voter_commands_rx: TracingUnboundedReceiver>>, prometheus_registry: Option, shared_voter_state: SharedVoterState, - finality_subscription: Option>, + justification_sender: GrandpaJustificationSender, ) -> Self { let metrics = match prometheus_registry.as_ref().map(Metrics::register) { Some(Ok(metrics)) => Some(metrics), @@ -848,7 +868,7 @@ where consensus_changes: persistent_data.consensus_changes.clone(), voter_set_state: persistent_data.set_state, metrics: metrics.as_ref().map(|m| m.environment.clone()), - finality_subscription, + justification_sender, _phantom: PhantomData, }); @@ -988,7 +1008,7 @@ where network: self.env.network.clone(), voting_rule: self.env.voting_rule.clone(), metrics: self.env.metrics.clone(), - finality_subscription: self.env.finality_subscription.clone(), + justification_sender: self.env.justification_sender.clone(), _phantom: PhantomData, }); diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index 7376aa61db069..fa4e1f726ecc3 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -5,7 +5,7 @@ // 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 +// 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, @@ -39,6 +39,7 @@ use crate::{ use crate::authorities::SharedAuthoritySet; use crate::communication::{Network as NetworkT, NetworkBridge}; use crate::consensus_changes::SharedConsensusChanges; +use crate::finality_proof::GrandpaJustificationSender; use sp_finality_grandpa::AuthorityId; use std::marker::{PhantomData, Unpin}; @@ -68,6 +69,7 @@ fn grandpa_observer( authority_set: &SharedAuthoritySet>, consensus_changes: &SharedConsensusChanges>, voters: &Arc>, + justification_sender: &GrandpaJustificationSender, last_finalized_number: NumberFor, commits: S, note_round: F, @@ -84,6 +86,7 @@ fn grandpa_observer( let consensus_changes = consensus_changes.clone(); let client = client.clone(); let voters = voters.clone(); + let justification_sender = justification_sender.clone(); let observer = commits.try_fold(last_finalized_number, move |last_finalized_number, global| { let (round, commit, callback) = match global { @@ -126,7 +129,7 @@ fn grandpa_observer( finalized_number, (round, commit).into(), false, - &None, // TODO: Should I include the finality_subscribers here? + &justification_sender, ) { Ok(_) => {}, Err(e) => return future::err(e), @@ -177,6 +180,7 @@ where select_chain: _, persistent_data, voter_commands_rx, + justification_sender, .. } = link; @@ -192,7 +196,8 @@ where network, persistent_data, config.keystore, - voter_commands_rx + voter_commands_rx, + justification_sender, ); let observer_work = observer_work @@ -213,6 +218,7 @@ struct ObserverWork> { persistent_data: PersistentData, keystore: Option, voter_commands_rx: TracingUnboundedReceiver>>, + justification_sender: GrandpaJustificationSender, _phantom: PhantomData, } @@ -230,6 +236,7 @@ where persistent_data: PersistentData, keystore: Option, voter_commands_rx: TracingUnboundedReceiver>>, + justification_sender: GrandpaJustificationSender, ) -> Self { let mut work = ObserverWork { @@ -241,6 +248,7 @@ where persistent_data, keystore, voter_commands_rx, + justification_sender, _phantom: PhantomData, }; work.rebuild_observer(); @@ -287,6 +295,7 @@ where &self.persistent_data.authority_set, &self.persistent_data.consensus_changes, &voters, + &self.justification_sender, last_finalized_number, global_in, note_round, From 56e716b97e4b698333397b97b0c27d07fe5d815e Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Mon, 18 May 2020 18:41:49 -0400 Subject: [PATCH 16/59] Add some PubSub boilerplate --- Cargo.lock | 3 ++ bin/node/rpc/src/lib.rs | 5 ++- client/finality-grandpa/rpc/Cargo.toml | 3 ++ client/finality-grandpa/rpc/src/lib.rs | 45 +++++++++++++++++++++++--- client/finality-grandpa/src/lib.rs | 6 ++-- 5 files changed, 52 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 5c5093383949a..3b1b0b2bc0d1a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6414,11 +6414,14 @@ dependencies = [ "jsonrpc-core", "jsonrpc-core-client", "jsonrpc-derive", + "jsonrpc-pubsub", "log", "sc-finality-grandpa", + "sc-rpc", "serde", "serde_json", "sp-core", + "sp-runtime", ] [[package]] diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index bac8b8367802f..a903235596e27 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -129,10 +129,9 @@ pub fn create_full( let GrandpaDeps { shared_voter_state, shared_authority_set, + justification_receiver, } = grandpa; - let GrandpaDeps { justification_receiver } = grandpa; - io.extend_with( SystemApi::to_delegate(FullSystem::new(client.clone(), pool)) ); @@ -152,7 +151,7 @@ pub fn create_full( ); io.extend_with( sc_finality_grandpa_rpc::GrandpaApi::to_delegate( - GrandpaRpcHandler::new(shared_authority_set, shared_voter_state) + GrandpaRpcHandler::new(shared_authority_set, shared_voter_state, justification_receiver) ) ); diff --git a/client/finality-grandpa/rpc/Cargo.toml b/client/finality-grandpa/rpc/Cargo.toml index 0eecec19f70c3..bc750e5a2bdc1 100644 --- a/client/finality-grandpa/rpc/Cargo.toml +++ b/client/finality-grandpa/rpc/Cargo.toml @@ -9,10 +9,13 @@ license = "GPL-3.0-or-later WITH Classpath-exception-2.0" [dependencies] sc-finality-grandpa = { version = "0.8.0-dev", path = "../" } +sc-rpc = { version = "2.0.0-dev", path = "../../rpc" } +sp-runtime = { version = "2.0.0-dev", path = "../../../primitives/runtime" } finality-grandpa = { version = "0.12.3", features = ["derive-codec"] } jsonrpc-core = "14.0.3" jsonrpc-core-client = "14.0.3" jsonrpc-derive = "14.0.3" +jsonrpc-pubsub = "14.0.3" futures = { version = "0.3.4", features = ["compat"] } serde = { version = "1.0.105", features = ["derive"] } serde_json = "1.0.50" diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index c146aaf10c13e..2063147b28291 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -5,7 +5,7 @@ // 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 +// 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, @@ -15,17 +15,23 @@ // You should have received a copy of the GNU General Public License // along with this program. If not, see . + //! RPC API for GRANDPA. #![warn(missing_docs)] use futures::{FutureExt, TryFutureExt}; use jsonrpc_derive::rpc; +use jsonrpc_pubsub::{typed::Subscriber, SubscriptionId}; mod error; mod report; use report::{ReportAuthoritySet, ReportVoterState, ReportedRoundStates}; +use sc_finality_grandpa::GrandpaJustificationReceiver; +use sc_rpc::Metadata; +use sp_runtime::traits::Block as BlockT; + /// Returned when Grandpa RPC endpoint is not ready. pub const NOT_READY_ERROR_CODE: i64 = 1; @@ -35,38 +41,67 @@ type FutureResult = /// Provides RPC methods for interacting with GRANDPA. #[rpc] pub trait GrandpaApi { + /// RPC Metadata + type Metadata; + /// Returns the state of the current best round state as well as the /// ongoing background rounds. #[rpc(name = "grandpa_roundState")] fn round_state(&self) -> FutureResult; + + /// Returns the block most recently finalized by Grandpa, alongside + /// side its justification. + #[pubsub(subscription = "grandpa_justifications", subscribe, name = "grandpa_subscribeJustifications")] + fn justification_subscription(&self, metadata: Self::Metadata, subscriber: Subscriber); + + /// Unsubscribe from receiving notifications about recently finalized blocks. + #[pubsub(subscription = "grandpa_justifications", unsubscribe, name = "grandpa_unsubscribeJustifications")] + fn unsubscribe_justifications(&self, metadata: Option, id: SubscriptionId) -> FutureResult; } /// Implements the GrandpaApi RPC trait for interacting with GRANDPA. -pub struct GrandpaRpcHandler { +pub struct GrandpaRpcHandler { authority_set: AuthoritySet, voter_state: VoterState, + justification_receiver: GrandpaJustificationReceiver, } -impl GrandpaRpcHandler { +impl GrandpaRpcHandler { /// Creates a new GrandpaRpcHander instance. - pub fn new(authority_set: AuthoritySet, voter_state: VoterState) -> Self { + pub fn new( + authority_set: AuthoritySet, + voter_state: VoterState, + justification_receiver: GrandpaJustificationReceiver, + ) -> Self { Self { authority_set, voter_state, + justification_receiver, } } } -impl GrandpaApi for GrandpaRpcHandler +impl GrandpaApi for GrandpaRpcHandler where VoterState: ReportVoterState + Send + Sync + 'static, AuthoritySet: ReportAuthoritySet + Send + Sync + 'static, + Block: BlockT, { + type Metadata = Metadata; + fn round_state(&self) -> FutureResult { let round_states = ReportedRoundStates::from(&self.authority_set, &self.voter_state); let future = async move { round_states }.boxed(); Box::new(future.map_err(jsonrpc_core::Error::from).compat()) } + + fn justification_subscription(&self, _metadata: Self::Metadata, _subscriber: Subscriber) { + todo!() + } + + fn unsubscribe_justifications(&self, _metadata: Option, _id: SubscriptionId) -> FutureResult { + todo!() + } } #[cfg(test)] diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index c6b7ff1ae7fca..cd9d26059e698 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -117,7 +117,10 @@ mod until_imported; mod voting_rule; pub use authorities::SharedAuthoritySet; -pub use finality_proof::{FinalityProofProvider, StorageAndProofProvider}; +pub use finality_proof::{ + FinalityProofProvider, StorageAndProofProvider, + GrandpaJustificationSender, GrandpaJustificationReceiver +}; pub use justification::GrandpaJustification; pub use light_import::light_block_import; pub use voting_rule::{ @@ -131,7 +134,6 @@ use import::GrandpaBlockImport; use until_imported::UntilGlobalMessageBlocksImported; use communication::{NetworkBridge, Network as NetworkT}; use sp_finality_grandpa::{AuthorityList, AuthorityPair, AuthoritySignature, SetId}; -use crate::finality_proof::{GrandpaJustificationSender, GrandpaJustificationReceiver}; // Re-export these two because it's just so damn convenient. pub use sp_finality_grandpa::{AuthorityId, GrandpaApi, ScheduledChange}; From 8b0850addbb3e47e1abf6757376c09fc503e08f1 Mon Sep 17 00:00:00 2001 From: Hernando Castano Date: Wed, 3 Jun 2020 17:22:29 -0400 Subject: [PATCH 17/59] Add guiding comments --- bin/node/cli/src/service.rs | 4 ++-- bin/node/rpc/src/lib.rs | 2 +- client/finality-grandpa/rpc/src/lib.rs | 19 ++++++++++++++++++- client/finality-grandpa/src/finality_proof.rs | 2 +- client/finality-grandpa/src/import.rs | 2 +- 5 files changed, 23 insertions(+), 6 deletions(-) diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 23757e7ede0b3..6935d5faad67f 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -148,7 +148,7 @@ macro_rules! new_full_start { }) })?; - (builder, import_setup, inherent_data_providers, rpc_setup, justification_sender) + (builder, import_setup, inherent_data_providers, rpc_setup) }} } @@ -174,7 +174,7 @@ macro_rules! new_full { $config.disable_grandpa, ); - let (builder, mut import_setup, inherent_data_providers, mut rpc_setup, justification_sender) = + let (builder, mut import_setup, inherent_data_providers, mut rpc_setup) = new_full_start!($config); let service = builder diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index 0409db59accd5..47d92373a598a 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -42,7 +42,7 @@ use sc_keystore::KeyStorePtr; use sp_consensus_babe::BabeApi; use sc_consensus_epochs::SharedEpochChanges; use sc_consensus_babe::{Config, Epoch}; -use sc_consensus_babe_rpc::BabeRPCHandler; +use sc_consensus_babe_rpc::BabeRpcHandler; use sc_finality_grandpa::{SharedVoterState, SharedAuthoritySet, GrandpaJustificationReceiver}; use sc_finality_grandpa_rpc::GrandpaRpcHandler; use sc_rpc_api::DenyUnsafe; diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 2063147b28291..e37766bd6e07e 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -21,7 +21,7 @@ use futures::{FutureExt, TryFutureExt}; use jsonrpc_derive::rpc; -use jsonrpc_pubsub::{typed::Subscriber, SubscriptionId}; +use jsonrpc_pubsub::{typed::Subscriber, SubscriptionId /*, manager::SubscriptionManager*/}; mod error; mod report; @@ -64,6 +64,7 @@ pub struct GrandpaRpcHandler { authority_set: AuthoritySet, voter_state: VoterState, justification_receiver: GrandpaJustificationReceiver, + // manager: SubscriptionManager, } impl GrandpaRpcHandler { @@ -79,6 +80,10 @@ impl GrandpaRpcHandler &SubscriptionManager { + // &self.manager + // } } impl GrandpaApi for GrandpaRpcHandler @@ -95,11 +100,23 @@ where Box::new(future.map_err(jsonrpc_core::Error::from).compat()) } + // NOTE: Should be changed to Subscriber fn justification_subscription(&self, _metadata: Self::Metadata, _subscriber: Subscriber) { + // let stream = self.receiver.subscribe().compat(); + // + // This will need to use the SubscriptionManager from `jsonrpc_pubsub` + // + // manager.add(subscriber, |sink| { + // // Write to the sink using the receiver stream + // // Check client/rpc/src/chain/mod.rs or the jsonrpc + // // repo for examples + // }); + todo!() } fn unsubscribe_justifications(&self, _metadata: Option, _id: SubscriptionId) -> FutureResult { + // Ok(self.manager().cancel(id)) todo!() } } diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 53dbcfadb61c1..f266897e09009 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -203,7 +203,7 @@ pub struct GrandpaJustificationReceiver { } impl GrandpaJustificationReceiver { - /// Crate a new receiver of justification notifications. + /// Create a new receiver of justification notifications. /// /// The `notifiers` should be shared with a corresponding /// `GrandpaJustificationSender`. diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index e1f31cca1a9db..b039e602908ee 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -39,7 +39,7 @@ use sp_runtime::traits::{ Block as BlockT, DigestFor, Header as HeaderT, NumberFor, Zero, }; -use crate::{Error, CommandOrError, NewAuthoritySet, VoterCommand, }; +use crate::{Error, CommandOrError, NewAuthoritySet, VoterCommand}; use crate::authorities::{AuthoritySet, SharedAuthoritySet, DelayKind, PendingChange}; use crate::consensus_changes::SharedConsensusChanges; use crate::environment::finalize_block; From cf07f2fdc0a2c7ad163ccd7e2abcb0efce5645a2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Mon, 8 Jun 2020 15:56:38 +0200 Subject: [PATCH 18/59] TMP: comment out to fix compilation --- bin/node/cli/src/service.rs | 2 +- client/finality-grandpa/rpc/src/lib.rs | 56 +++++++++++++------------- 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 6935d5faad67f..c108f2ff2147d 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -140,7 +140,7 @@ macro_rules! new_full_start { grandpa: node_rpc::GrandpaDeps { shared_voter_state: shared_voter_state.clone(), shared_authority_set: shared_authority_set.clone(), - justification_receiver, + justification_receiver: justification_receiver.clone(), }, }; diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index e37766bd6e07e..600f50a6fa7c8 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -42,21 +42,21 @@ type FutureResult = #[rpc] pub trait GrandpaApi { /// RPC Metadata - type Metadata; + // type Metadata; /// Returns the state of the current best round state as well as the /// ongoing background rounds. #[rpc(name = "grandpa_roundState")] fn round_state(&self) -> FutureResult; - /// Returns the block most recently finalized by Grandpa, alongside - /// side its justification. - #[pubsub(subscription = "grandpa_justifications", subscribe, name = "grandpa_subscribeJustifications")] - fn justification_subscription(&self, metadata: Self::Metadata, subscriber: Subscriber); - - /// Unsubscribe from receiving notifications about recently finalized blocks. - #[pubsub(subscription = "grandpa_justifications", unsubscribe, name = "grandpa_unsubscribeJustifications")] - fn unsubscribe_justifications(&self, metadata: Option, id: SubscriptionId) -> FutureResult; +// /// Returns the block most recently finalized by Grandpa, alongside +// /// side its justification. +// #[pubsub(subscription = "grandpa_justifications", subscribe, name = "grandpa_subscribeJustifications")] +// fn justification_subscription(&self, metadata: Self::Metadata, subscriber: Subscriber); +// +// /// Unsubscribe from receiving notifications about recently finalized blocks. +// #[pubsub(subscription = "grandpa_justifications", unsubscribe, name = "grandpa_unsubscribeJustifications")] +// fn unsubscribe_justifications(&self, metadata: Option, id: SubscriptionId) -> FutureResult; } /// Implements the GrandpaApi RPC trait for interacting with GRANDPA. @@ -92,7 +92,7 @@ where AuthoritySet: ReportAuthoritySet + Send + Sync + 'static, Block: BlockT, { - type Metadata = Metadata; + // type Metadata = Metadata; fn round_state(&self) -> FutureResult { let round_states = ReportedRoundStates::from(&self.authority_set, &self.voter_state); @@ -101,24 +101,24 @@ where } // NOTE: Should be changed to Subscriber - fn justification_subscription(&self, _metadata: Self::Metadata, _subscriber: Subscriber) { - // let stream = self.receiver.subscribe().compat(); - // - // This will need to use the SubscriptionManager from `jsonrpc_pubsub` - // - // manager.add(subscriber, |sink| { - // // Write to the sink using the receiver stream - // // Check client/rpc/src/chain/mod.rs or the jsonrpc - // // repo for examples - // }); - - todo!() - } - - fn unsubscribe_justifications(&self, _metadata: Option, _id: SubscriptionId) -> FutureResult { - // Ok(self.manager().cancel(id)) - todo!() - } + //fn justification_subscription(&self, _metadata: Self::Metadata, _subscriber: Subscriber) { + // // let stream = self.receiver.subscribe().compat(); + // // + // // This will need to use the SubscriptionManager from `jsonrpc_pubsub` + // // + // // manager.add(subscriber, |sink| { + // // // Write to the sink using the receiver stream + // // // Check client/rpc/src/chain/mod.rs or the jsonrpc + // // // repo for examples + // // }); + + // todo!() + //} + + //fn unsubscribe_justifications(&self, _metadata: Option, _id: SubscriptionId) -> FutureResult { + // // Ok(self.manager().cancel(id)) + // todo!() + //} } #[cfg(test)] From b1c04ba2067f4863e9b8bc039972346b9a698fba Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Wed, 10 Jun 2020 14:24:33 +0200 Subject: [PATCH 19/59] Return MetaIoHandler from PubSubHandler in create_full --- Cargo.lock | 1 + bin/node/rpc/Cargo.toml | 1 + bin/node/rpc/src/lib.rs | 10 ++++++---- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 8aa5663c4d89f..b4879be0b8c5a 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3427,6 +3427,7 @@ name = "node-rpc" version = "2.0.0-rc3" dependencies = [ "jsonrpc-core", + "jsonrpc-pubsub", "node-primitives", "node-runtime", "pallet-contracts-rpc", diff --git a/bin/node/rpc/Cargo.toml b/bin/node/rpc/Cargo.toml index 0c6c913b137ad..5e5bbcfed5893 100644 --- a/bin/node/rpc/Cargo.toml +++ b/bin/node/rpc/Cargo.toml @@ -13,6 +13,7 @@ targets = ["x86_64-unknown-linux-gnu"] [dependencies] sc-client-api = { version = "2.0.0-rc3", path = "../../../client/api" } jsonrpc-core = "14.2.0" +jsonrpc-pubsub = "14.2.0" node-primitives = { version = "2.0.0-rc3", path = "../primitives" } node-runtime = { version = "2.0.0-rc3", path = "../runtime" } sp-runtime = { version = "2.0.0-rc3", path = "../../../primitives/runtime" } diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index 47d92373a598a..8fce739d90622 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -47,6 +47,8 @@ use sc_finality_grandpa::{SharedVoterState, SharedAuthoritySet, GrandpaJustifica use sc_finality_grandpa_rpc::GrandpaRpcHandler; use sc_rpc_api::DenyUnsafe; +use jsonrpc_core::IoHandlerExtension; + /// Light client extra dependencies. pub struct LightDeps { /// The client instance to use. @@ -98,7 +100,7 @@ pub struct FullDeps { /// Instantiate all Full RPC extensions. pub fn create_full( deps: FullDeps, -) -> jsonrpc_core::IoHandler where +) -> jsonrpc_core::MetaIoHandler where C: ProvideRuntimeApi, C: HeaderBackend + HeaderMetadata + 'static, C: Send + Sync + 'static, @@ -108,14 +110,14 @@ pub fn create_full( C::Api: BabeApi, ::Error: fmt::Debug, P: TransactionPool + 'static, - M: jsonrpc_core::Metadata + Default, + M: jsonrpc_pubsub::PubSubMetadata + Default, SC: SelectChain +'static, { use substrate_frame_rpc_system::{FullSystem, SystemApi}; use pallet_contracts_rpc::{Contracts, ContractsApi}; use pallet_transaction_payment_rpc::{TransactionPayment, TransactionPaymentApi}; - let mut io = jsonrpc_core::IoHandler::default(); + let mut io = jsonrpc_pubsub::PubSubHandler::new(jsonrpc_core::MetaIoHandler::default()); let FullDeps { client, pool, @@ -166,7 +168,7 @@ pub fn create_full( ) ); - io + io.into() } /// Instantiate all Light RPC extensions. From ebad4d760c07fa5d798d75cd31d8625f91ff2eab Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Wed, 10 Jun 2020 14:53:24 +0200 Subject: [PATCH 20/59] Uncomment pubsub methods in rpc handler (fails to build) --- client/finality-grandpa/rpc/src/lib.rs | 56 +++++++++++++------------- 1 file changed, 28 insertions(+), 28 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 600f50a6fa7c8..e37766bd6e07e 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -42,21 +42,21 @@ type FutureResult = #[rpc] pub trait GrandpaApi { /// RPC Metadata - // type Metadata; + type Metadata; /// Returns the state of the current best round state as well as the /// ongoing background rounds. #[rpc(name = "grandpa_roundState")] fn round_state(&self) -> FutureResult; -// /// Returns the block most recently finalized by Grandpa, alongside -// /// side its justification. -// #[pubsub(subscription = "grandpa_justifications", subscribe, name = "grandpa_subscribeJustifications")] -// fn justification_subscription(&self, metadata: Self::Metadata, subscriber: Subscriber); -// -// /// Unsubscribe from receiving notifications about recently finalized blocks. -// #[pubsub(subscription = "grandpa_justifications", unsubscribe, name = "grandpa_unsubscribeJustifications")] -// fn unsubscribe_justifications(&self, metadata: Option, id: SubscriptionId) -> FutureResult; + /// Returns the block most recently finalized by Grandpa, alongside + /// side its justification. + #[pubsub(subscription = "grandpa_justifications", subscribe, name = "grandpa_subscribeJustifications")] + fn justification_subscription(&self, metadata: Self::Metadata, subscriber: Subscriber); + + /// Unsubscribe from receiving notifications about recently finalized blocks. + #[pubsub(subscription = "grandpa_justifications", unsubscribe, name = "grandpa_unsubscribeJustifications")] + fn unsubscribe_justifications(&self, metadata: Option, id: SubscriptionId) -> FutureResult; } /// Implements the GrandpaApi RPC trait for interacting with GRANDPA. @@ -92,7 +92,7 @@ where AuthoritySet: ReportAuthoritySet + Send + Sync + 'static, Block: BlockT, { - // type Metadata = Metadata; + type Metadata = Metadata; fn round_state(&self) -> FutureResult { let round_states = ReportedRoundStates::from(&self.authority_set, &self.voter_state); @@ -101,24 +101,24 @@ where } // NOTE: Should be changed to Subscriber - //fn justification_subscription(&self, _metadata: Self::Metadata, _subscriber: Subscriber) { - // // let stream = self.receiver.subscribe().compat(); - // // - // // This will need to use the SubscriptionManager from `jsonrpc_pubsub` - // // - // // manager.add(subscriber, |sink| { - // // // Write to the sink using the receiver stream - // // // Check client/rpc/src/chain/mod.rs or the jsonrpc - // // // repo for examples - // // }); - - // todo!() - //} - - //fn unsubscribe_justifications(&self, _metadata: Option, _id: SubscriptionId) -> FutureResult { - // // Ok(self.manager().cancel(id)) - // todo!() - //} + fn justification_subscription(&self, _metadata: Self::Metadata, _subscriber: Subscriber) { + // let stream = self.receiver.subscribe().compat(); + // + // This will need to use the SubscriptionManager from `jsonrpc_pubsub` + // + // manager.add(subscriber, |sink| { + // // Write to the sink using the receiver stream + // // Check client/rpc/src/chain/mod.rs or the jsonrpc + // // repo for examples + // }); + + todo!() + } + + fn unsubscribe_justifications(&self, _metadata: Option, _id: SubscriptionId) -> FutureResult { + // Ok(self.manager().cancel(id)) + todo!() + } } #[cfg(test)] From caaaa6181111b00f2f723044be0a3c30b33c0743 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 11 Jun 2020 21:20:20 +0200 Subject: [PATCH 21/59] node/rpc: make Metadata concrete in create_full to fix compilation --- Cargo.lock | 1 + bin/node/rpc/Cargo.toml | 1 + bin/node/rpc/src/lib.rs | 5 ++--- client/finality-grandpa/rpc/src/lib.rs | 3 +-- 4 files changed, 5 insertions(+), 5 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index b4879be0b8c5a..ecc498706d5a5 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3439,6 +3439,7 @@ dependencies = [ "sc-finality-grandpa", "sc-finality-grandpa-rpc", "sc-keystore", + "sc-rpc", "sc-rpc-api", "sp-api", "sp-blockchain", diff --git a/bin/node/rpc/Cargo.toml b/bin/node/rpc/Cargo.toml index 5e5bbcfed5893..a918d104b91b3 100644 --- a/bin/node/rpc/Cargo.toml +++ b/bin/node/rpc/Cargo.toml @@ -32,3 +32,4 @@ sp-blockchain = { version = "2.0.0-rc3", path = "../../../primitives/blockchain" sc-finality-grandpa = { version = "0.8.0-rc3", path = "../../../client/finality-grandpa" } sc-finality-grandpa-rpc = { version = "0.8.0-rc3", path = "../../../client/finality-grandpa/rpc" } sc-rpc-api = { version = "0.8.0-rc3", path = "../../../client/rpc-api" } +sc-rpc = { version = "2.0.0-rc3", path = "../../../client/rpc" } diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index 8fce739d90622..7061878af4481 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -98,9 +98,9 @@ pub struct FullDeps { } /// Instantiate all Full RPC extensions. -pub fn create_full( +pub fn create_full( deps: FullDeps, -) -> jsonrpc_core::MetaIoHandler where +) -> jsonrpc_core::MetaIoHandler where C: ProvideRuntimeApi, C: HeaderBackend + HeaderMetadata + 'static, C: Send + Sync + 'static, @@ -110,7 +110,6 @@ pub fn create_full( C::Api: BabeApi, ::Error: fmt::Debug, P: TransactionPool + 'static, - M: jsonrpc_pubsub::PubSubMetadata + Default, SC: SelectChain +'static, { use substrate_frame_rpc_system::{FullSystem, SystemApi}; diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index e37766bd6e07e..feb7f6a6067a6 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -29,7 +29,6 @@ mod report; use report::{ReportAuthoritySet, ReportVoterState, ReportedRoundStates}; use sc_finality_grandpa::GrandpaJustificationReceiver; -use sc_rpc::Metadata; use sp_runtime::traits::Block as BlockT; /// Returned when Grandpa RPC endpoint is not ready. @@ -92,7 +91,7 @@ where AuthoritySet: ReportAuthoritySet + Send + Sync + 'static, Block: BlockT, { - type Metadata = Metadata; + type Metadata = sc_rpc::Metadata; fn round_state(&self) -> FutureResult { let round_states = ReportedRoundStates::from(&self.authority_set, &self.voter_state); From f3cc272216902844e9fa7e7c98a70fe82ae311bb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 12 Jun 2020 00:16:23 +0200 Subject: [PATCH 22/59] node: pass in SubscriptionManger to grandpa rpc handler --- bin/node/cli/src/service.rs | 3 ++- bin/node/rpc/src/lib.rs | 6 +++++- client/finality-grandpa/rpc/src/lib.rs | 12 +++++++----- client/service/src/builder.rs | 18 +++++++++--------- 4 files changed, 23 insertions(+), 16 deletions(-) diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index db0977fdb0b40..bc54e8739dd9e 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -129,7 +129,7 @@ macro_rules! new_full_start { .expect("SelectChain is present for full services or set up failed; qed."); let keystore = builder.keystore().clone(); - Ok(move |deny_unsafe| { + Ok(move |deny_unsafe, subscriptions| { let deps = node_rpc::FullDeps { client: client.clone(), pool: pool.clone(), @@ -144,6 +144,7 @@ macro_rules! new_full_start { shared_voter_state: shared_voter_state.clone(), shared_authority_set: shared_authority_set.clone(), justification_receiver: justification_receiver.clone(), + subscriptions, }, }; diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index 7061878af4481..e20bd881f813f 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -46,6 +46,7 @@ use sc_consensus_babe_rpc::BabeRpcHandler; use sc_finality_grandpa::{SharedVoterState, SharedAuthoritySet, GrandpaJustificationReceiver}; use sc_finality_grandpa_rpc::GrandpaRpcHandler; use sc_rpc_api::DenyUnsafe; +use jsonrpc_pubsub::manager::SubscriptionManager; use jsonrpc_core::IoHandlerExtension; @@ -79,6 +80,8 @@ pub struct GrandpaDeps { pub shared_authority_set: SharedAuthoritySet, /// Receives notifications about justification events from Grandpa. pub justification_receiver: GrandpaJustificationReceiver, + /// WIP + pub subscriptions: SubscriptionManager, } /// Full client dependencies. @@ -135,6 +138,7 @@ pub fn create_full( shared_voter_state, shared_authority_set, justification_receiver, + subscriptions, } = grandpa; io.extend_with( @@ -163,7 +167,7 @@ pub fn create_full( ); io.extend_with( sc_finality_grandpa_rpc::GrandpaApi::to_delegate( - GrandpaRpcHandler::new(shared_authority_set, shared_voter_state, justification_receiver) + GrandpaRpcHandler::new(shared_authority_set, shared_voter_state, justification_receiver, subscriptions) ) ); diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index feb7f6a6067a6..873df9f214506 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -21,7 +21,7 @@ use futures::{FutureExt, TryFutureExt}; use jsonrpc_derive::rpc; -use jsonrpc_pubsub::{typed::Subscriber, SubscriptionId /*, manager::SubscriptionManager*/}; +use jsonrpc_pubsub::{typed::Subscriber, SubscriptionId, manager::SubscriptionManager}; mod error; mod report; @@ -63,7 +63,7 @@ pub struct GrandpaRpcHandler { authority_set: AuthoritySet, voter_state: VoterState, justification_receiver: GrandpaJustificationReceiver, - // manager: SubscriptionManager, + manager: SubscriptionManager, } impl GrandpaRpcHandler { @@ -72,17 +72,19 @@ impl GrandpaRpcHandler, + manager: SubscriptionManager, ) -> Self { Self { authority_set, voter_state, justification_receiver, + manager, } } - // fn manager(&self) -> &SubscriptionManager { - // &self.manager - // } + fn manager(&self) -> &SubscriptionManager { + &self.manager + } } impl GrandpaApi for GrandpaRpcHandler diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index 16500baae1e7b..e8c9db09e509b 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -114,17 +114,17 @@ pub trait RpcExtensionBuilder { /// Returns an instance of the RPC extension for a particular `DenyUnsafe` /// value, e.g. the RPC extension might not expose some unsafe methods. - fn build(&self, deny: sc_rpc::DenyUnsafe) -> Self::Output; + fn build(&self, deny: sc_rpc::DenyUnsafe, subscriptions: SubscriptionManager) -> Self::Output; } impl RpcExtensionBuilder for F where - F: Fn(sc_rpc::DenyUnsafe) -> R, + F: Fn(sc_rpc::DenyUnsafe, SubscriptionManager) -> R, R: sc_rpc::RpcExtension, { type Output = R; - fn build(&self, deny: sc_rpc::DenyUnsafe) -> Self::Output { - (*self)(deny) + fn build(&self, deny: sc_rpc::DenyUnsafe, subscriptions: SubscriptionManager) -> Self::Output { + (*self)(deny, subscriptions) } } @@ -138,7 +138,7 @@ impl RpcExtensionBuilder for NoopRpcExtensionBuilder where { type Output = R; - fn build(&self, _deny: sc_rpc::DenyUnsafe) -> Self::Output { + fn build(&self, _deny: sc_rpc::DenyUnsafe, subscriptions: SubscriptionManager) -> Self::Output { self.0.clone() } } @@ -361,7 +361,7 @@ impl ServiceBuilder<(), (), (), (), (), (), (), (), (), (), ()> { finality_proof_request_builder: None, finality_proof_provider: None, transaction_pool: Arc::new(()), - rpc_extensions_builder: Box::new(|_| ()), + rpc_extensions_builder: Box::new(|_,_| ()), remote_backend: None, block_announce_validator_builder: None, marker: PhantomData, @@ -444,7 +444,7 @@ impl ServiceBuilder<(), (), (), (), (), (), (), (), (), (), ()> { finality_proof_request_builder: None, finality_proof_provider: None, transaction_pool: Arc::new(()), - rpc_extensions_builder: Box::new(|_| ()), + rpc_extensions_builder: Box::new(|_,_| ()), remote_backend: Some(remote_blockchain), block_announce_validator_builder: None, marker: PhantomData, @@ -1234,7 +1234,7 @@ ServiceBuilder< let author = sc_rpc::author::Author::new( client.clone(), transaction_pool.clone(), - subscriptions, + subscriptions.clone(), keystore.clone(), deny_unsafe, ); @@ -1256,7 +1256,7 @@ ServiceBuilder< maybe_offchain_rpc, author::AuthorApi::to_delegate(author), system::SystemApi::to_delegate(system), - rpc_extensions_builder.build(deny_unsafe), + rpc_extensions_builder.build(deny_unsafe, subscriptions), )) }; let rpc = start_rpc_servers(&config, gen_handler)?; From 3c4505a68d95d5c400b06b3288a44775f4a9203f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 12 Jun 2020 16:00:59 +0200 Subject: [PATCH 23/59] grandpa-rpc: use SubscriptionManger to add subscriber --- client/finality-grandpa/rpc/src/lib.rs | 21 +++++++++++++++++---- 1 file changed, 17 insertions(+), 4 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 873df9f214506..2c8b4103376a5 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -19,10 +19,14 @@ //! RPC API for GRANDPA. #![warn(missing_docs)] -use futures::{FutureExt, TryFutureExt}; +use futures::{FutureExt, TryFutureExt, stream, TryStreamExt}; use jsonrpc_derive::rpc; use jsonrpc_pubsub::{typed::Subscriber, SubscriptionId, manager::SubscriptionManager}; +use jsonrpc_core::futures::sink::Sink as Sink01; +use jsonrpc_core::futures::stream::Stream as Stream01; +use jsonrpc_core::futures::future::Future as Future01; + mod error; mod report; @@ -101,8 +105,8 @@ where Box::new(future.map_err(jsonrpc_core::Error::from).compat()) } - // NOTE: Should be changed to Subscriber - fn justification_subscription(&self, _metadata: Self::Metadata, _subscriber: Subscriber) { + // WIP: Should be changed to Subscriber + fn justification_subscription(&self, _metadata: Self::Metadata, subscriber: Subscriber) { // let stream = self.receiver.subscribe().compat(); // // This will need to use the SubscriptionManager from `jsonrpc_pubsub` @@ -113,10 +117,19 @@ where // // repo for examples // }); + let stream = stream::iter(vec![Ok::<_,()>(true)]).compat(); + + let id = self.manager.add(subscriber, |sink| { + let stream = stream.map(|res| Ok(res)); + sink.sink_map_err(|e| ()) + .send_all(stream) + .map(|_| ()) + }); + todo!() } - fn unsubscribe_justifications(&self, _metadata: Option, _id: SubscriptionId) -> FutureResult { + fn unsubscribe_justifications(&self, _metadata: Option, id: SubscriptionId) -> FutureResult { // Ok(self.manager().cancel(id)) todo!() } From 9a225accf01e2fa1f7349a65e88380474b8505f7 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 16 Jun 2020 15:12:23 +0200 Subject: [PATCH 24/59] grandpa-rpc: attempt at setting up the justification stream (fails to build) --- Cargo.lock | 1 + client/finality-grandpa/Cargo.toml | 1 + client/finality-grandpa/rpc/src/lib.rs | 32 +++++++------------ client/finality-grandpa/src/finality_proof.rs | 3 +- client/finality-grandpa/src/lib.rs | 3 +- 5 files changed, 17 insertions(+), 23 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ecc498706d5a5..3a36b6892ac0f 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6269,6 +6269,7 @@ dependencies = [ "sc-network-gossip", "sc-network-test", "sc-telemetry", + "serde", "serde_json", "sp-api", "sp-application-crypto", diff --git a/client/finality-grandpa/Cargo.toml b/client/finality-grandpa/Cargo.toml index d101fe66d083d..7ff487e783caa 100644 --- a/client/finality-grandpa/Cargo.toml +++ b/client/finality-grandpa/Cargo.toml @@ -33,6 +33,7 @@ sp-core = { version = "2.0.0-rc3", path = "../../primitives/core" } sp-api = { version = "2.0.0-rc3", path = "../../primitives/api" } sc-telemetry = { version = "2.0.0-rc3", path = "../telemetry" } sc-keystore = { version = "2.0.0-rc3", path = "../keystore" } +serde = { version = "1.0", features=["derive"] } serde_json = "1.0.41" sc-client-api = { version = "2.0.0-rc3", path = "../api" } sp-inherents = { version = "2.0.0-rc3", path = "../../primitives/inherents" } diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 2c8b4103376a5..e86769dabd296 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -32,7 +32,7 @@ mod report; use report::{ReportAuthoritySet, ReportVoterState, ReportedRoundStates}; -use sc_finality_grandpa::GrandpaJustificationReceiver; +use sc_finality_grandpa::{JustificationNotification , GrandpaJustificationReceiver}; use sp_runtime::traits::Block as BlockT; /// Returned when Grandpa RPC endpoint is not ready. @@ -43,7 +43,10 @@ type FutureResult = /// Provides RPC methods for interacting with GRANDPA. #[rpc] -pub trait GrandpaApi { +pub trait GrandpaApi +where + Block: BlockT +{ /// RPC Metadata type Metadata; @@ -55,7 +58,7 @@ pub trait GrandpaApi { /// Returns the block most recently finalized by Grandpa, alongside /// side its justification. #[pubsub(subscription = "grandpa_justifications", subscribe, name = "grandpa_subscribeJustifications")] - fn justification_subscription(&self, metadata: Self::Metadata, subscriber: Subscriber); + fn subscribe_justifications(&self, metadata: Self::Metadata, subscriber: Subscriber>); /// Unsubscribe from receiving notifications about recently finalized blocks. #[pubsub(subscription = "grandpa_justifications", unsubscribe, name = "grandpa_unsubscribeJustifications")] @@ -91,7 +94,7 @@ impl GrandpaRpcHandler GrandpaApi for GrandpaRpcHandler +impl GrandpaApi for GrandpaRpcHandler where VoterState: ReportVoterState + Send + Sync + 'static, AuthoritySet: ReportAuthoritySet + Send + Sync + 'static, @@ -105,28 +108,15 @@ where Box::new(future.map_err(jsonrpc_core::Error::from).compat()) } - // WIP: Should be changed to Subscriber - fn justification_subscription(&self, _metadata: Self::Metadata, subscriber: Subscriber) { - // let stream = self.receiver.subscribe().compat(); - // - // This will need to use the SubscriptionManager from `jsonrpc_pubsub` - // - // manager.add(subscriber, |sink| { - // // Write to the sink using the receiver stream - // // Check client/rpc/src/chain/mod.rs or the jsonrpc - // // repo for examples - // }); - - let stream = stream::iter(vec![Ok::<_,()>(true)]).compat(); - - let id = self.manager.add(subscriber, |sink| { + fn subscribe_justifications(&self, _metadata: Self::Metadata, subscriber: Subscriber>) { + let stream = self.justification_receiver.subscribe().compat(); + + let _id = self.manager.add(subscriber, |sink| { let stream = stream.map(|res| Ok(res)); sink.sink_map_err(|e| ()) .send_all(stream) .map(|_| ()) }); - - todo!() } fn unsubscribe_justifications(&self, _metadata: Option, id: SubscriptionId) -> FutureResult { diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index f266897e09009..76e00b8761b67 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -39,6 +39,7 @@ use std::sync::Arc; use log::{trace, warn}; use parking_lot::Mutex; +use serde::{Deserialize, Serialize}; use sp_blockchain::{Backend as BlockchainBackend, Error as ClientError, Result as ClientResult}; use sc_client_api::{ @@ -151,7 +152,7 @@ impl AuthoritySetForFinalityChecker for Arc { /// Highest finalized block header pub header: Block::Header, diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 6eba5006d1b15..2a82ffb46087f 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -125,7 +125,8 @@ mod voting_rule; pub use authorities::SharedAuthoritySet; pub use finality_proof::{ FinalityProofProvider, StorageAndProofProvider, - GrandpaJustificationSender, GrandpaJustificationReceiver + GrandpaJustificationSender, GrandpaJustificationReceiver, + JustificationNotification, }; pub use import::GrandpaBlockImport; pub use justification::GrandpaJustification; From a85dd53e6010c5de76666698c49a9eecb61e8deb Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 16 Jun 2020 23:44:32 +0200 Subject: [PATCH 25/59] grandpa-rpc: fix compilation of connecting stream to sink --- client/finality-grandpa/rpc/src/lib.rs | 15 +++++++-------- 1 file changed, 7 insertions(+), 8 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index e86769dabd296..7122779b05243 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -19,7 +19,7 @@ //! RPC API for GRANDPA. #![warn(missing_docs)] -use futures::{FutureExt, TryFutureExt, stream, TryStreamExt}; +use futures::{FutureExt, TryFutureExt, stream, TryStreamExt, future, StreamExt}; use jsonrpc_derive::rpc; use jsonrpc_pubsub::{typed::Subscriber, SubscriptionId, manager::SubscriptionManager}; @@ -43,10 +43,7 @@ type FutureResult = /// Provides RPC methods for interacting with GRANDPA. #[rpc] -pub trait GrandpaApi -where - Block: BlockT -{ +pub trait GrandpaApi { /// RPC Metadata type Metadata; @@ -58,7 +55,7 @@ where /// Returns the block most recently finalized by Grandpa, alongside /// side its justification. #[pubsub(subscription = "grandpa_justifications", subscribe, name = "grandpa_subscribeJustifications")] - fn subscribe_justifications(&self, metadata: Self::Metadata, subscriber: Subscriber>); + fn subscribe_justifications(&self, metadata: Self::Metadata, subscriber: Subscriber); /// Unsubscribe from receiving notifications about recently finalized blocks. #[pubsub(subscription = "grandpa_justifications", unsubscribe, name = "grandpa_unsubscribeJustifications")] @@ -94,7 +91,7 @@ impl GrandpaRpcHandler GrandpaApi for GrandpaRpcHandler +impl GrandpaApi> for GrandpaRpcHandler where VoterState: ReportVoterState + Send + Sync + 'static, AuthoritySet: ReportAuthoritySet + Send + Sync + 'static, @@ -109,7 +106,9 @@ where } fn subscribe_justifications(&self, _metadata: Self::Metadata, subscriber: Subscriber>) { - let stream = self.justification_receiver.subscribe().compat(); + let stream = self.justification_receiver.subscribe() + .map(|x| Ok::<_,()>(x)) + .compat(); let _id = self.manager.add(subscriber, |sink| { let stream = stream.map(|res| Ok(res)); From 1b0344f662713a6f2677f03edbb17bb232d216f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Wed, 17 Jun 2020 00:23:32 +0200 Subject: [PATCH 26/59] grandpa-rpc: implement unsubscribe --- client/finality-grandpa/rpc/src/lib.rs | 14 ++++++++++++-- 1 file changed, 12 insertions(+), 2 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 7122779b05243..ef07fe9bd5700 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -119,8 +119,18 @@ where } fn unsubscribe_justifications(&self, _metadata: Option, id: SubscriptionId) -> FutureResult { - // Ok(self.manager().cancel(id)) - todo!() + let cancel = self.manager.cancel(id); + let future = async move { cancel } + .boxed() + .unit_error() + .map_err(|_| { + jsonrpc_core::Error { + message: "WIP: JON".to_string(), + code: jsonrpc_core::ErrorCode::ServerError(0), // WIP: change me + data: None, + } + }); + Box::new(future.compat()) } } From 12dd5df5a1ac9ed258746f5424a191182e9ec3bf Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 18 Jun 2020 15:45:36 +0200 Subject: [PATCH 27/59] grandpa-rpc: update older tests --- Cargo.lock | 3 ++ client/finality-grandpa/rpc/Cargo.toml | 3 ++ client/finality-grandpa/rpc/src/lib.rs | 63 ++++++++++++++++++++++--- client/finality-grandpa/src/observer.rs | 5 ++ client/finality-grandpa/src/tests.rs | 4 ++ 5 files changed, 71 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 3a36b6892ac0f..ca0176a3c0291 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6302,8 +6302,11 @@ dependencies = [ "jsonrpc-core-client", "jsonrpc-derive", "jsonrpc-pubsub", + "lazy_static", "log", + "parking_lot 0.10.2", "sc-finality-grandpa", + "sc-network-test", "sc-rpc", "serde", "serde_json", diff --git a/client/finality-grandpa/rpc/Cargo.toml b/client/finality-grandpa/rpc/Cargo.toml index 62d5983abf8d8..dd8278a17d175 100644 --- a/client/finality-grandpa/rpc/Cargo.toml +++ b/client/finality-grandpa/rpc/Cargo.toml @@ -21,6 +21,9 @@ serde = { version = "1.0.105", features = ["derive"] } serde_json = "1.0.50" log = "0.4.8" derive_more = "0.99.2" +parking_lot = "0.10.0" [dev-dependencies] sp-core = { version = "2.0.0-rc3", path = "../../../primitives/core" } +lazy_static = "1.4" +sc-network-test = { version = "0.8.0-rc3", path = "../../network/test" } \ No newline at end of file diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index ef07fe9bd5700..8f042faa364f5 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -140,7 +140,16 @@ mod tests { use jsonrpc_core::IoHandler; use sc_finality_grandpa::{report, AuthorityId}; use sp_core::crypto::Public; - use std::{collections::HashSet, convert::TryInto}; + use std::{collections::HashSet, convert::TryInto, sync::Arc}; + use parking_lot::Mutex; + + use sc_network_test::Block; + + use jsonrpc_core::futures::sink::Sink as Sink01; + use jsonrpc_core::futures::stream::Stream as Stream01; + use jsonrpc_core::futures::{future as future01, Future as Future01}; + + use futures::{compat::Future01CompatExt, executor, FutureExt}; struct TestAuthoritySet; struct TestVoterState; @@ -197,22 +206,61 @@ mod tests { } } + lazy_static::lazy_static! { + static ref EXECUTOR: executor::ThreadPool = executor::ThreadPool::new() + .expect("Failed to create thread pool executor for tests"); + } + + pub struct TestTaskExecutor; + type Boxed01Future01 = Box + Send + 'static>; + + impl future01::Executor for TestTaskExecutor { + fn execute(&self, future: Boxed01Future01) -> std::result::Result<(), future01::ExecuteError> { + EXECUTOR.spawn_ok(future.compat().map(drop)); + Ok(()) + } + } + #[test] fn uninitialized_rpc_handler() { - let handler = GrandpaRpcHandler::new(TestAuthoritySet, EmptyVoterState); - let mut io = IoHandler::new(); + let finality_notifiers = Arc::new(Mutex::new(vec![])); + let justification_receiver = + GrandpaJustificationReceiver::::new(finality_notifiers.clone()); + let manager = SubscriptionManager::new(Arc::new(TestTaskExecutor)); + + let handler = GrandpaRpcHandler::new( + TestAuthoritySet, + EmptyVoterState, + justification_receiver, + manager, + ); + // let mut io = IoHandler::new(); + let mut io = jsonrpc_core::MetaIoHandler::default(); + // let mut io = jsonrpc_pubsub::PubSubHandler::new(jsonrpc_core::MetaIoHandler::default()); io.extend_with(GrandpaApi::to_delegate(handler)); let request = r#"{"jsonrpc":"2.0","method":"grandpa_roundState","params":[],"id":1}"#; let response = r#"{"jsonrpc":"2.0","error":{"code":1,"message":"GRANDPA RPC endpoint not ready"},"id":1}"#; - assert_eq!(Some(response.into()), io.handle_request_sync(request)); + let meta = sc_rpc::Metadata::default(); + + assert_eq!(Some(response.into()), io.handle_request_sync(request, meta)); } #[test] fn working_rpc_handler() { - let handler = GrandpaRpcHandler::new(TestAuthoritySet, TestVoterState); - let mut io = IoHandler::new(); + let finality_notifiers = Arc::new(Mutex::new(vec![])); + let justification_receiver = GrandpaJustificationReceiver::::new(finality_notifiers.clone()); + let manager = SubscriptionManager::new(Arc::new(TestTaskExecutor)); + + let handler = GrandpaRpcHandler::new( + TestAuthoritySet, + TestVoterState, + justification_receiver, + manager, + ); + // let mut io = IoHandler::new(); + let mut io = jsonrpc_core::MetaIoHandler::default(); io.extend_with(GrandpaApi::to_delegate(handler)); let request = r#"{"jsonrpc":"2.0","method":"grandpa_roundState","params":[],"id":1}"#; @@ -230,6 +278,7 @@ mod tests { \"setId\":1\ },\"id\":1}"; - assert_eq!(io.handle_request_sync(request), Some(response.into())); + let meta = sc_rpc::Metadata::default(); + assert_eq!(io.handle_request_sync(request, meta), Some(response.into())); } } diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index e112654d1f907..4c52da707692a 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -432,12 +432,17 @@ mod tests { ).unwrap(); let (_tx, voter_command_rx) = tracing_unbounded(""); + + let finality_notifiers = Arc::new(parking_lot::Mutex::new(vec![])); + let justification_sender = GrandpaJustificationSender::new(finality_notifiers.clone()); + let observer = ObserverWork::new( client, tester.net_handle.clone(), persistent_data, None, voter_command_rx, + justification_sender, ); // Trigger a reputation change through the gossip validator. diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index ffd8f1c8c642d..bfcc2844ef63b 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -1558,6 +1558,9 @@ fn grandpa_environment_respects_voting_rules() { None, ); + let finality_notifiers = Arc::new(parking_lot::Mutex::new(vec![])); + let justification_sender = GrandpaJustificationSender::new(finality_notifiers.clone()); + Environment { authority_set: authority_set.clone(), config: config.clone(), @@ -1570,6 +1573,7 @@ fn grandpa_environment_respects_voting_rules() { network, voting_rule, metrics: None, + justification_sender, _phantom: PhantomData, } }; From bca6a13b55afd5a49c02781ed32a1edb98673650 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Mon, 22 Jun 2020 11:32:32 +0200 Subject: [PATCH 28/59] grandpa-rpc: add full prefix to avoid confusing rust-analyzer --- client/finality-grandpa/rpc/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 8f042faa364f5..497faefa64bb2 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -179,7 +179,7 @@ mod tests { let voter_id_1 = AuthorityId::from_slice(&[1; 32]); let voters_best: HashSet<_> = vec![voter_id_1].into_iter().collect(); - let best_round_state = report::RoundState { + let best_round_state = sc_finality_grandpa::report::RoundState { total_weight: 100_u64.try_into().unwrap(), threshold_weight: 67_u64.try_into().unwrap(), prevote_current_weight: 50.into(), @@ -188,7 +188,7 @@ mod tests { precommit_ids: HashSet::new(), }; - let past_round_state = report::RoundState { + let past_round_state = sc_finality_grandpa::report::RoundState { total_weight: 100_u64.try_into().unwrap(), threshold_weight: 67_u64.try_into().unwrap(), prevote_current_weight: 100.into(), From efaa0d1f452acb0d9ea5b1615d30b415325f8a8a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Mon, 22 Jun 2020 13:14:39 +0200 Subject: [PATCH 29/59] grandpa-rpc: add test for pubsub not available --- client/finality-grandpa/rpc/src/lib.rs | 25 +++++++++++++++++++++++++ 1 file changed, 25 insertions(+) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 497faefa64bb2..8d0f9709d0af5 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -281,4 +281,29 @@ mod tests { let meta = sc_rpc::Metadata::default(); assert_eq!(io.handle_request_sync(request, meta), Some(response.into())); } + + #[test] + fn subscribe_to_justifications_not_available() { + let finality_notifiers = Arc::new(Mutex::new(vec![])); + let justification_receiver = + GrandpaJustificationReceiver::::new(finality_notifiers.clone()); + let manager = SubscriptionManager::new(Arc::new(TestTaskExecutor)); + + let handler = GrandpaRpcHandler::new( + TestAuthoritySet, + EmptyVoterState, + justification_receiver, + manager, + ); + + let mut io = jsonrpc_pubsub::PubSubHandler::new(jsonrpc_core::MetaIoHandler::default()); + io.extend_with(GrandpaApi::to_delegate(handler)); + + let request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; + let response = r#"{"jsonrpc":"2.0","error":{"code":-32090,"message":"Subscriptions are not available on this transport."},"id":1}"#; + + let meta = sc_rpc::Metadata::default(); + + assert_eq!(Some(response.into()), io.handle_request_sync(request, meta)); + } } From 96329885566304c1d65293c1c0587c933fa7cf4a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Mon, 22 Jun 2020 14:20:17 +0200 Subject: [PATCH 30/59] grandpa-rpc: tidy up leftover code --- bin/node/cli/src/service.rs | 4 - bin/node/rpc/src/lib.rs | 4 +- client/finality-grandpa/rpc/src/lib.rs | 102 +++++++++++++-------- client/finality-grandpa/src/environment.rs | 2 +- client/service/src/builder.rs | 2 +- 5 files changed, 66 insertions(+), 48 deletions(-) diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index bc54e8739dd9e..c35311d7fe1fe 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -26,8 +26,6 @@ use grandpa::{ self, FinalityProofProvider as GrandpaFinalityProofProvider, StorageAndProofProvider, - GrandpaJustificationSender, - GrandpaJustificationReceiver, }; use node_executor; use node_primitives::Block; @@ -38,8 +36,6 @@ use sc_service::{ use sp_inherents::InherentDataProviders; use sc_consensus::LongestChain; -use parking_lot::Mutex; - /// Starts a `ServiceBuilder` for a full service. /// /// Use this macro if you don't actually need the full service, but just the builder in order to diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index e20bd881f813f..df9b3fbc1b9d6 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -48,8 +48,6 @@ use sc_finality_grandpa_rpc::GrandpaRpcHandler; use sc_rpc_api::DenyUnsafe; use jsonrpc_pubsub::manager::SubscriptionManager; -use jsonrpc_core::IoHandlerExtension; - /// Light client extra dependencies. pub struct LightDeps { /// The client instance to use. @@ -80,7 +78,7 @@ pub struct GrandpaDeps { pub shared_authority_set: SharedAuthoritySet, /// Receives notifications about justification events from Grandpa. pub justification_receiver: GrandpaJustificationReceiver, - /// WIP + /// Subscription manager to keep track of pubsub subscribers. pub subscriptions: SubscriptionManager, } diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 8d0f9709d0af5..800aa146d7173 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -19,24 +19,28 @@ //! RPC API for GRANDPA. #![warn(missing_docs)] -use futures::{FutureExt, TryFutureExt, stream, TryStreamExt, future, StreamExt}; +use futures::{FutureExt, TryFutureExt, TryStreamExt, StreamExt}; +use log::warn; use jsonrpc_derive::rpc; use jsonrpc_pubsub::{typed::Subscriber, SubscriptionId, manager::SubscriptionManager}; - -use jsonrpc_core::futures::sink::Sink as Sink01; -use jsonrpc_core::futures::stream::Stream as Stream01; -use jsonrpc_core::futures::future::Future as Future01; +use jsonrpc_core::futures::{ + sink::Sink as Sink01, + stream::Stream as Stream01, + future::Future as Future01, +}; mod error; mod report; -use report::{ReportAuthoritySet, ReportVoterState, ReportedRoundStates}; - use sc_finality_grandpa::{JustificationNotification , GrandpaJustificationReceiver}; use sp_runtime::traits::Block as BlockT; +use report::{ReportAuthoritySet, ReportVoterState, ReportedRoundStates}; + /// Returned when Grandpa RPC endpoint is not ready. pub const NOT_READY_ERROR_CODE: i64 = 1; +/// Returned when unsubscribing to finality notifications fail. +pub const UNSUBSCRIBE_ERROR_CODE: i64 = 2; type FutureResult = Box + Send>; @@ -54,12 +58,28 @@ pub trait GrandpaApi { /// Returns the block most recently finalized by Grandpa, alongside /// side its justification. - #[pubsub(subscription = "grandpa_justifications", subscribe, name = "grandpa_subscribeJustifications")] - fn subscribe_justifications(&self, metadata: Self::Metadata, subscriber: Subscriber); + #[pubsub( + subscription = "grandpa_justifications", + subscribe, + name = "grandpa_subscribeJustifications" + )] + fn subscribe_justifications( + &self, + metadata: Self::Metadata, + subscriber: Subscriber + ); /// Unsubscribe from receiving notifications about recently finalized blocks. - #[pubsub(subscription = "grandpa_justifications", unsubscribe, name = "grandpa_unsubscribeJustifications")] - fn unsubscribe_justifications(&self, metadata: Option, id: SubscriptionId) -> FutureResult; + #[pubsub( + subscription = "grandpa_justifications", + unsubscribe, + name = "grandpa_unsubscribeJustifications" + )] + fn unsubscribe_justifications( + &self, + metadata: Option, + id: SubscriptionId + ) -> FutureResult; } /// Implements the GrandpaApi RPC trait for interacting with GRANDPA. @@ -85,13 +105,10 @@ impl GrandpaRpcHandler &SubscriptionManager { - &self.manager - } } -impl GrandpaApi> for GrandpaRpcHandler +impl GrandpaApi> + for GrandpaRpcHandler where VoterState: ReportVoterState + Send + Sync + 'static, AuthoritySet: ReportAuthoritySet + Send + Sync + 'static, @@ -105,28 +122,38 @@ where Box::new(future.map_err(jsonrpc_core::Error::from).compat()) } - fn subscribe_justifications(&self, _metadata: Self::Metadata, subscriber: Subscriber>) { + fn subscribe_justifications( + &self, + _metadata: Self::Metadata, + subscriber: Subscriber> + ) { let stream = self.justification_receiver.subscribe() .map(|x| Ok::<_,()>(x)) + .map_err(|e| warn!("Notification stream error: {:?}", e)) .compat(); - let _id = self.manager.add(subscriber, |sink| { + self.manager.add(subscriber, |sink| { let stream = stream.map(|res| Ok(res)); - sink.sink_map_err(|e| ()) + sink.sink_map_err(|e| warn!("Error sending notifications: {:?}", e)) .send_all(stream) .map(|_| ()) }); } - fn unsubscribe_justifications(&self, _metadata: Option, id: SubscriptionId) -> FutureResult { + fn unsubscribe_justifications( + &self, + _metadata: Option, + id: SubscriptionId + ) -> FutureResult { let cancel = self.manager.cancel(id); let future = async move { cancel } .boxed() .unit_error() - .map_err(|_| { + .map_err(|e| { + warn!("Error unsubscribing: {:?}", &e); jsonrpc_core::Error { - message: "WIP: JON".to_string(), - code: jsonrpc_core::ErrorCode::ServerError(0), // WIP: change me + message: format!("Error unsubscribing: {:?}", e), + code: jsonrpc_core::ErrorCode::ServerError(UNSUBSCRIBE_ERROR_CODE), data: None, } }); @@ -137,20 +164,15 @@ where #[cfg(test)] mod tests { use super::*; - use jsonrpc_core::IoHandler; - use sc_finality_grandpa::{report, AuthorityId}; - use sp_core::crypto::Public; use std::{collections::HashSet, convert::TryInto, sync::Arc}; + use futures::{compat::Future01CompatExt, executor}; + use jsonrpc_core::futures::future as future01; use parking_lot::Mutex; + use sc_finality_grandpa::{report, AuthorityId}; + use sp_core::crypto::Public; use sc_network_test::Block; - use jsonrpc_core::futures::sink::Sink as Sink01; - use jsonrpc_core::futures::stream::Stream as Stream01; - use jsonrpc_core::futures::{future as future01, Future as Future01}; - - use futures::{compat::Future01CompatExt, executor, FutureExt}; - struct TestAuthoritySet; struct TestVoterState; struct EmptyVoterState; @@ -211,11 +233,15 @@ mod tests { .expect("Failed to create thread pool executor for tests"); } + // These are lifted from thye jsonrpc_pubsub crate tests pub struct TestTaskExecutor; type Boxed01Future01 = Box + Send + 'static>; impl future01::Executor for TestTaskExecutor { - fn execute(&self, future: Boxed01Future01) -> std::result::Result<(), future01::ExecuteError> { + fn execute( + &self, + future: Boxed01Future01 + ) -> std::result::Result<(), future01::ExecuteError> { EXECUTOR.spawn_ok(future.compat().map(drop)); Ok(()) } @@ -234,23 +260,22 @@ mod tests { justification_receiver, manager, ); - // let mut io = IoHandler::new(); + let mut io = jsonrpc_core::MetaIoHandler::default(); - // let mut io = jsonrpc_pubsub::PubSubHandler::new(jsonrpc_core::MetaIoHandler::default()); io.extend_with(GrandpaApi::to_delegate(handler)); let request = r#"{"jsonrpc":"2.0","method":"grandpa_roundState","params":[],"id":1}"#; let response = r#"{"jsonrpc":"2.0","error":{"code":1,"message":"GRANDPA RPC endpoint not ready"},"id":1}"#; let meta = sc_rpc::Metadata::default(); - assert_eq!(Some(response.into()), io.handle_request_sync(request, meta)); } #[test] fn working_rpc_handler() { let finality_notifiers = Arc::new(Mutex::new(vec![])); - let justification_receiver = GrandpaJustificationReceiver::::new(finality_notifiers.clone()); + let justification_receiver = + GrandpaJustificationReceiver::::new(finality_notifiers.clone()); let manager = SubscriptionManager::new(Arc::new(TestTaskExecutor)); let handler = GrandpaRpcHandler::new( @@ -259,7 +284,7 @@ mod tests { justification_receiver, manager, ); - // let mut io = IoHandler::new(); + let mut io = jsonrpc_core::MetaIoHandler::default(); io.extend_with(GrandpaApi::to_delegate(handler)); @@ -303,7 +328,6 @@ mod tests { let response = r#"{"jsonrpc":"2.0","error":{"code":-32090,"message":"Subscriptions are not available on this transport."},"id":1}"#; let meta = sc_rpc::Metadata::default(); - assert_eq!(Some(response.into()), io.handle_request_sync(request, meta)); } } diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index d62751dd84298..b103eb8d1c459 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -1192,7 +1192,7 @@ pub(crate) fn finalize_block( justification, }; - justification_sender.notify(notification); + let _ = justification_sender.notify(notification); } debug!(target: "afg", "Finalizing blocks up to ({:?}, {})", number, hash); diff --git a/client/service/src/builder.rs b/client/service/src/builder.rs index e8c9db09e509b..a71f99946690b 100644 --- a/client/service/src/builder.rs +++ b/client/service/src/builder.rs @@ -138,7 +138,7 @@ impl RpcExtensionBuilder for NoopRpcExtensionBuilder where { type Output = R; - fn build(&self, _deny: sc_rpc::DenyUnsafe, subscriptions: SubscriptionManager) -> Self::Output { + fn build(&self, _deny: sc_rpc::DenyUnsafe, _subscriptions: SubscriptionManager) -> Self::Output { self.0.clone() } } From 964f1e3b42d07bc037abffc70322cf752b47b308 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 23 Jun 2020 00:47:49 +0200 Subject: [PATCH 31/59] grandpa-rpc: add test for sub and unsub of justifications --- client/finality-grandpa/rpc/src/lib.rs | 39 +++++++++++++++++++++----- 1 file changed, 32 insertions(+), 7 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 800aa146d7173..4f251b29c1f53 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -166,7 +166,7 @@ mod tests { use super::*; use std::{collections::HashSet, convert::TryInto, sync::Arc}; use futures::{compat::Future01CompatExt, executor}; - use jsonrpc_core::futures::future as future01; + use jsonrpc_core::{Output, futures::future as future01}; use parking_lot::Mutex; use sc_finality_grandpa::{report, AuthorityId}; @@ -233,7 +233,7 @@ mod tests { .expect("Failed to create thread pool executor for tests"); } - // These are lifted from thye jsonrpc_pubsub crate tests + // These are lifted from the jsonrpc_pubsub crate tests pub struct TestTaskExecutor; type Boxed01Future01 = Box + Send + 'static>; @@ -308,7 +308,7 @@ mod tests { } #[test] - fn subscribe_to_justifications_not_available() { + fn subscribe_and_unsubscribe_to_justifications() { let finality_notifiers = Arc::new(Mutex::new(vec![])); let justification_receiver = GrandpaJustificationReceiver::::new(finality_notifiers.clone()); @@ -324,10 +324,35 @@ mod tests { let mut io = jsonrpc_pubsub::PubSubHandler::new(jsonrpc_core::MetaIoHandler::default()); io.extend_with(GrandpaApi::to_delegate(handler)); - let request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; - let response = r#"{"jsonrpc":"2.0","error":{"code":-32090,"message":"Subscriptions are not available on this transport."},"id":1}"#; + let (tx, _rx) = jsonrpc_core::futures::sync::mpsc::channel(1); + let meta = sc_rpc::Metadata::new(tx); - let meta = sc_rpc::Metadata::default(); - assert_eq!(Some(response.into()), io.handle_request_sync(request, meta)); + // Subscribe + let sub_request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; + let resp = io.handle_request_sync(sub_request, meta.clone()); + let resp: Output = serde_json::from_str(&resp.unwrap()).unwrap(); + + let sub_id = match resp { + Output::Success(success) => success.result, + _ => panic!(), + }; + + // Unsubscribe with wrong ID + let invalid_unsub_req= r#"{"jsonrpc":"2.0","method":"grandpa_unsubscribeJustifications","params":["FOO"],"id":1}"#; + let resp = io.handle_request_sync(&invalid_unsub_req, meta.clone()); + assert_eq!(resp, Some(r#"{"jsonrpc":"2.0","result":false,"id":1}"#.into())); + + let unsub_req = format!( + "{{\"jsonrpc\":\"2.0\",\"method\":\"grandpa_unsubscribeJustifications\",\"params\":[{}],\"id\":1}}", + sub_id + ); + + // Unsubscribe + let resp = io.handle_request_sync(&unsub_req, meta.clone()); + assert_eq!(resp, Some(r#"{"jsonrpc":"2.0","result":true,"id":1}"#.into())); + + // Unsubscribe again and fail + let resp = io.handle_request_sync(&unsub_req, meta); + assert_eq!(resp, Some(r#"{"jsonrpc":"2.0","result":false,"id":1}"#.into())); } } From 82004cb8f266888e1e362c6b2211860d1410778e Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 23 Jun 2020 16:48:37 +0200 Subject: [PATCH 32/59] grandpa-rpc: minor stylistic changes --- client/finality-grandpa/rpc/src/lib.rs | 25 ++++++++++++++++--------- 1 file changed, 16 insertions(+), 9 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 4f251b29c1f53..1f8f48ff8c2f2 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -338,21 +338,28 @@ mod tests { }; // Unsubscribe with wrong ID - let invalid_unsub_req= r#"{"jsonrpc":"2.0","method":"grandpa_unsubscribeJustifications","params":["FOO"],"id":1}"#; - let resp = io.handle_request_sync(&invalid_unsub_req, meta.clone()); - assert_eq!(resp, Some(r#"{"jsonrpc":"2.0","result":false,"id":1}"#.into())); + assert_eq!( + io.handle_request_sync( + r#"{"jsonrpc":"2.0","method":"grandpa_unsubscribeJustifications","params":["FOO"],"id":1}"#, + meta.clone() + ), + Some(r#"{"jsonrpc":"2.0","result":false,"id":1}"#.into()) + ); + // Unsubscribe let unsub_req = format!( "{{\"jsonrpc\":\"2.0\",\"method\":\"grandpa_unsubscribeJustifications\",\"params\":[{}],\"id\":1}}", sub_id ); - - // Unsubscribe - let resp = io.handle_request_sync(&unsub_req, meta.clone()); - assert_eq!(resp, Some(r#"{"jsonrpc":"2.0","result":true,"id":1}"#.into())); + assert_eq!( + io.handle_request_sync(&unsub_req, meta.clone()), + Some(r#"{"jsonrpc":"2.0","result":true,"id":1}"#.into()), + ); // Unsubscribe again and fail - let resp = io.handle_request_sync(&unsub_req, meta); - assert_eq!(resp, Some(r#"{"jsonrpc":"2.0","result":false,"id":1}"#.into())); + assert_eq!( + io.handle_request_sync(&unsub_req, meta), + Some(r#"{"jsonrpc":"2.0","result":false,"id":1}"#.into()), + ); } } From e526f95eee5835796ce0c4e0f8e6722237c10ce4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 23 Jun 2020 21:37:21 +0200 Subject: [PATCH 33/59] grandpa-rpc: split unit test --- client/finality-grandpa/rpc/src/lib.rs | 69 ++++++++++++-------------- 1 file changed, 33 insertions(+), 36 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 1f8f48ff8c2f2..3b92d9e3f6296 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -247,8 +247,7 @@ mod tests { } } - #[test] - fn uninitialized_rpc_handler() { + fn setup_rpc_handler(voter_state: VoterState) -> GrandpaRpcHandler { let finality_notifiers = Arc::new(Mutex::new(vec![])); let justification_receiver = GrandpaJustificationReceiver::::new(finality_notifiers.clone()); @@ -256,11 +255,16 @@ mod tests { let handler = GrandpaRpcHandler::new( TestAuthoritySet, - EmptyVoterState, + voter_state, justification_receiver, manager, ); + handler + } + #[test] + fn uninitialized_rpc_handler() { + let handler = setup_rpc_handler(EmptyVoterState); let mut io = jsonrpc_core::MetaIoHandler::default(); io.extend_with(GrandpaApi::to_delegate(handler)); @@ -273,18 +277,7 @@ mod tests { #[test] fn working_rpc_handler() { - let finality_notifiers = Arc::new(Mutex::new(vec![])); - let justification_receiver = - GrandpaJustificationReceiver::::new(finality_notifiers.clone()); - let manager = SubscriptionManager::new(Arc::new(TestTaskExecutor)); - - let handler = GrandpaRpcHandler::new( - TestAuthoritySet, - TestVoterState, - justification_receiver, - manager, - ); - + let handler = setup_rpc_handler(TestVoterState); let mut io = jsonrpc_core::MetaIoHandler::default(); io.extend_with(GrandpaApi::to_delegate(handler)); @@ -309,18 +302,7 @@ mod tests { #[test] fn subscribe_and_unsubscribe_to_justifications() { - let finality_notifiers = Arc::new(Mutex::new(vec![])); - let justification_receiver = - GrandpaJustificationReceiver::::new(finality_notifiers.clone()); - let manager = SubscriptionManager::new(Arc::new(TestTaskExecutor)); - - let handler = GrandpaRpcHandler::new( - TestAuthoritySet, - EmptyVoterState, - justification_receiver, - manager, - ); - + let handler = setup_rpc_handler(TestVoterState); let mut io = jsonrpc_pubsub::PubSubHandler::new(jsonrpc_core::MetaIoHandler::default()); io.extend_with(GrandpaApi::to_delegate(handler)); @@ -337,15 +319,6 @@ mod tests { _ => panic!(), }; - // Unsubscribe with wrong ID - assert_eq!( - io.handle_request_sync( - r#"{"jsonrpc":"2.0","method":"grandpa_unsubscribeJustifications","params":["FOO"],"id":1}"#, - meta.clone() - ), - Some(r#"{"jsonrpc":"2.0","result":false,"id":1}"#.into()) - ); - // Unsubscribe let unsub_req = format!( "{{\"jsonrpc\":\"2.0\",\"method\":\"grandpa_unsubscribeJustifications\",\"params\":[{}],\"id\":1}}", @@ -362,4 +335,28 @@ mod tests { Some(r#"{"jsonrpc":"2.0","result":false,"id":1}"#.into()), ); } + + #[test] + fn subscribe_and_unsubscribe_with_wrong_id() { + let handler = setup_rpc_handler(TestVoterState); + let mut io = jsonrpc_pubsub::PubSubHandler::new(jsonrpc_core::MetaIoHandler::default()); + io.extend_with(GrandpaApi::to_delegate(handler)); + + let (tx, _rx) = jsonrpc_core::futures::sync::mpsc::channel(1); + let meta = sc_rpc::Metadata::new(tx); + + // Subscribe + let sub_request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; + let resp = io.handle_request_sync(sub_request, meta.clone()); + let resp: Output = serde_json::from_str(&resp.unwrap()).unwrap(); + + // Unsubscribe with wrong ID + assert_eq!( + io.handle_request_sync( + r#"{"jsonrpc":"2.0","method":"grandpa_unsubscribeJustifications","params":["FOO"],"id":1}"#, + meta.clone() + ), + Some(r#"{"jsonrpc":"2.0","result":false,"id":1}"#.into()) + ); + } } From 6c71b8fb87b523ba99c81ee3f5e659ae539bd031 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 23 Jun 2020 21:50:16 +0200 Subject: [PATCH 34/59] grandpa-rpc: minor stylistic changes in test --- client/finality-grandpa/rpc/src/lib.rs | 17 +++++++++-------- 1 file changed, 9 insertions(+), 8 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 3b92d9e3f6296..4abeb51d31f01 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -300,14 +300,19 @@ mod tests { assert_eq!(io.handle_request_sync(request, meta), Some(response.into())); } - #[test] - fn subscribe_and_unsubscribe_to_justifications() { + fn setup_pubsub_handler() -> (sc_rpc::Metadata, jsonrpc_pubsub::PubSubHandler) { let handler = setup_rpc_handler(TestVoterState); let mut io = jsonrpc_pubsub::PubSubHandler::new(jsonrpc_core::MetaIoHandler::default()); io.extend_with(GrandpaApi::to_delegate(handler)); let (tx, _rx) = jsonrpc_core::futures::sync::mpsc::channel(1); let meta = sc_rpc::Metadata::new(tx); + (meta, io) + } + + #[test] + fn subscribe_and_unsubscribe_to_justifications() { + let (meta, io) = setup_pubsub_handler(); // Subscribe let sub_request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; @@ -338,17 +343,13 @@ mod tests { #[test] fn subscribe_and_unsubscribe_with_wrong_id() { - let handler = setup_rpc_handler(TestVoterState); - let mut io = jsonrpc_pubsub::PubSubHandler::new(jsonrpc_core::MetaIoHandler::default()); - io.extend_with(GrandpaApi::to_delegate(handler)); - - let (tx, _rx) = jsonrpc_core::futures::sync::mpsc::channel(1); - let meta = sc_rpc::Metadata::new(tx); + let (meta, io) = setup_pubsub_handler(); // Subscribe let sub_request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; let resp = io.handle_request_sync(sub_request, meta.clone()); let resp: Output = serde_json::from_str(&resp.unwrap()).unwrap(); + assert!(matches!(resp, Output::Success(_))); // Unsubscribe with wrong ID assert_eq!( From 8bfb560d405522f1b9730e6a29b3c5607492440a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 25 Jun 2020 13:06:14 +0200 Subject: [PATCH 35/59] grandpa-rpc: skip returning future when cancelling --- client/finality-grandpa/rpc/src/lib.rs | 18 +++--------------- 1 file changed, 3 insertions(+), 15 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 4abeb51d31f01..18042871208c1 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -79,7 +79,7 @@ pub trait GrandpaApi { &self, metadata: Option, id: SubscriptionId - ) -> FutureResult; + ) -> jsonrpc_core::Result; } /// Implements the GrandpaApi RPC trait for interacting with GRANDPA. @@ -144,20 +144,8 @@ where &self, _metadata: Option, id: SubscriptionId - ) -> FutureResult { - let cancel = self.manager.cancel(id); - let future = async move { cancel } - .boxed() - .unit_error() - .map_err(|e| { - warn!("Error unsubscribing: {:?}", &e); - jsonrpc_core::Error { - message: format!("Error unsubscribing: {:?}", e), - code: jsonrpc_core::ErrorCode::ServerError(UNSUBSCRIBE_ERROR_CODE), - data: None, - } - }); - Box::new(future.compat()) + ) -> jsonrpc_core::Result { + Ok(self.manager.cancel(id)) } } From be7cb789cb951643888aad5c255a5f5bc034a6a8 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 25 Jun 2020 14:17:44 +0200 Subject: [PATCH 36/59] grandpa-rpc: reuse testing executor from sc-rpc --- client/finality-grandpa/rpc/Cargo.toml | 3 ++- client/finality-grandpa/rpc/src/lib.rs | 24 ++---------------------- client/rpc/Cargo.toml | 5 ++++- client/rpc/src/lib.rs | 5 +++-- client/rpc/src/testing.rs | 1 + 5 files changed, 12 insertions(+), 26 deletions(-) diff --git a/client/finality-grandpa/rpc/Cargo.toml b/client/finality-grandpa/rpc/Cargo.toml index dd8278a17d175..ea8c93a7c4a7f 100644 --- a/client/finality-grandpa/rpc/Cargo.toml +++ b/client/finality-grandpa/rpc/Cargo.toml @@ -26,4 +26,5 @@ parking_lot = "0.10.0" [dev-dependencies] sp-core = { version = "2.0.0-rc3", path = "../../../primitives/core" } lazy_static = "1.4" -sc-network-test = { version = "0.8.0-rc3", path = "../../network/test" } \ No newline at end of file +sc-network-test = { version = "0.8.0-rc3", path = "../../network/test" } +sc-rpc = { version = "2.0.0-rc3", path = "../../rpc", features = ["test-helpers"] } \ No newline at end of file diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 18042871208c1..d850ea39d539a 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -153,8 +153,7 @@ where mod tests { use super::*; use std::{collections::HashSet, convert::TryInto, sync::Arc}; - use futures::{compat::Future01CompatExt, executor}; - use jsonrpc_core::{Output, futures::future as future01}; + use jsonrpc_core::Output; use parking_lot::Mutex; use sc_finality_grandpa::{report, AuthorityId}; @@ -216,30 +215,11 @@ mod tests { } } - lazy_static::lazy_static! { - static ref EXECUTOR: executor::ThreadPool = executor::ThreadPool::new() - .expect("Failed to create thread pool executor for tests"); - } - - // These are lifted from the jsonrpc_pubsub crate tests - pub struct TestTaskExecutor; - type Boxed01Future01 = Box + Send + 'static>; - - impl future01::Executor for TestTaskExecutor { - fn execute( - &self, - future: Boxed01Future01 - ) -> std::result::Result<(), future01::ExecuteError> { - EXECUTOR.spawn_ok(future.compat().map(drop)); - Ok(()) - } - } - fn setup_rpc_handler(voter_state: VoterState) -> GrandpaRpcHandler { let finality_notifiers = Arc::new(Mutex::new(vec![])); let justification_receiver = GrandpaJustificationReceiver::::new(finality_notifiers.clone()); - let manager = SubscriptionManager::new(Arc::new(TestTaskExecutor)); + let manager = SubscriptionManager::new(Arc::new(sc_rpc::testing::TaskExecutor)); let handler = GrandpaRpcHandler::new( TestAuthoritySet, diff --git a/client/rpc/Cargo.toml b/client/rpc/Cargo.toml index f3557ca6b2b5f..6461380fc0d4d 100644 --- a/client/rpc/Cargo.toml +++ b/client/rpc/Cargo.toml @@ -37,6 +37,7 @@ sp-transaction-pool = { version = "2.0.0-rc3", path = "../../primitives/transact sp-blockchain = { version = "2.0.0-rc3", path = "../../primitives/blockchain" } hash-db = { version = "0.15.2", default-features = false } parking_lot = "0.10.0" +lazy_static = "1.4.0" [dev-dependencies] assert_matches = "1.3.0" @@ -46,4 +47,6 @@ sp-io = { version = "2.0.0-rc3", path = "../../primitives/io" } substrate-test-runtime-client = { version = "2.0.0-rc3", path = "../../test-utils/runtime/client" } tokio = "0.1.22" sc-transaction-pool = { version = "2.0.0-rc3", path = "../transaction-pool" } -lazy_static = "1.4.0" + +[features] +test-helpers = [] diff --git a/client/rpc/src/lib.rs b/client/rpc/src/lib.rs index 53a63b449c87e..cc35b56c063b6 100644 --- a/client/rpc/src/lib.rs +++ b/client/rpc/src/lib.rs @@ -33,5 +33,6 @@ pub mod chain; pub mod offchain; pub mod state; pub mod system; -#[cfg(test)] -mod testing; + +#[cfg(any(test, feature = "test-helpers"))] +pub mod testing; diff --git a/client/rpc/src/testing.rs b/client/rpc/src/testing.rs index afca07a7fbe6a..9530ff0020644 100644 --- a/client/rpc/src/testing.rs +++ b/client/rpc/src/testing.rs @@ -32,6 +32,7 @@ lazy_static::lazy_static! { type Boxed01Future01 = Box + Send + 'static>; +/// Executor for use in testing pub struct TaskExecutor; impl future01::Executor for TaskExecutor { fn execute( From ac81cdfdced20dc4a54686365fe8ce48baa27226 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 25 Jun 2020 14:54:39 +0200 Subject: [PATCH 37/59] grandpa-rpc: don't need to use PubSubHandler in tests --- client/finality-grandpa/rpc/src/lib.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index d850ea39d539a..02b024a5000ad 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -268,9 +268,9 @@ mod tests { assert_eq!(io.handle_request_sync(request, meta), Some(response.into())); } - fn setup_pubsub_handler() -> (sc_rpc::Metadata, jsonrpc_pubsub::PubSubHandler) { + fn setup_io_handler() -> (sc_rpc::Metadata, jsonrpc_core::MetaIoHandler) { let handler = setup_rpc_handler(TestVoterState); - let mut io = jsonrpc_pubsub::PubSubHandler::new(jsonrpc_core::MetaIoHandler::default()); + let mut io = jsonrpc_core::MetaIoHandler::default(); io.extend_with(GrandpaApi::to_delegate(handler)); let (tx, _rx) = jsonrpc_core::futures::sync::mpsc::channel(1); @@ -280,7 +280,7 @@ mod tests { #[test] fn subscribe_and_unsubscribe_to_justifications() { - let (meta, io) = setup_pubsub_handler(); + let (meta, io) = setup_io_handler(); // Subscribe let sub_request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; @@ -311,7 +311,7 @@ mod tests { #[test] fn subscribe_and_unsubscribe_with_wrong_id() { - let (meta, io) = setup_pubsub_handler(); + let (meta, io) = setup_io_handler(); // Subscribe let sub_request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; From 3f8630db9649d5094262bde499ed809bc8ed78a4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 26 Jun 2020 11:54:24 +0200 Subject: [PATCH 38/59] node-rpc: use MetaIoHandler rather than PubSubHandler --- bin/node/rpc/src/lib.rs | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index df9b3fbc1b9d6..478b7c7492bfa 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -117,7 +117,7 @@ pub fn create_full( use pallet_contracts_rpc::{Contracts, ContractsApi}; use pallet_transaction_payment_rpc::{TransactionPayment, TransactionPaymentApi}; - let mut io = jsonrpc_pubsub::PubSubHandler::new(jsonrpc_core::MetaIoHandler::default()); + let mut io = jsonrpc_core::MetaIoHandler::default(); let FullDeps { client, pool, @@ -169,7 +169,7 @@ pub fn create_full( ) ); - io.into() + io } /// Instantiate all Light RPC extensions. From 3b387e6f23c3945add8e89db2309978572faf055 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 26 Jun 2020 12:59:47 +0200 Subject: [PATCH 39/59] grandpa: log if getting header failed --- client/finality-grandpa/src/environment.rs | 19 ++++++++++--------- 1 file changed, 10 insertions(+), 9 deletions(-) diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index b103eb8d1c459..f11780775839c 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -1182,17 +1182,18 @@ pub(crate) fn finalize_block( }, }; - // Q: We `finalized()` this at L37, so can I be sure - // that it's fine to unwrap here? if let Some(justification) = justification.clone() { - let header = client.header(BlockId::Hash(hash))? - .expect(""); - let notification = JustificationNotification { - header, - justification, + match client.header(BlockId::Hash(hash)) { + Ok(Some(header)) => { + let notification = JustificationNotification { + header, + justification, + }; + let _ = justification_sender.notify(notification); + }, + Ok(None) => debug!(target: "afg", "Expected a header for sending a justification notification."), + Err(err) => debug!(target: "afg", "Getting header failed: {}", err), }; - - let _ = justification_sender.notify(notification); } debug!(target: "afg", "Finalizing blocks up to ({:?}, {})", number, hash); From bf4b19ab7d4b50fa4685665c776efb584ffece0c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 26 Jun 2020 22:27:13 +0200 Subject: [PATCH 40/59] grandpa: move justification channel creation into factory function --- client/finality-grandpa/rpc/src/lib.rs | 4 +--- client/finality-grandpa/src/finality_proof.rs | 20 +++++++++++++++---- client/finality-grandpa/src/lib.rs | 8 ++------ 3 files changed, 19 insertions(+), 13 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 02b024a5000ad..782c50812fa83 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -216,9 +216,7 @@ mod tests { } fn setup_rpc_handler(voter_state: VoterState) -> GrandpaRpcHandler { - let finality_notifiers = Arc::new(Mutex::new(vec![])); - let justification_receiver = - GrandpaJustificationReceiver::::new(finality_notifiers.clone()); + let (_, justification_receiver) = GrandpaJustificationReceiver::channel(); let manager = SubscriptionManager::new(Arc::new(sc_rpc::testing::TaskExecutor)); let handler = GrandpaRpcHandler::new( diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 76e00b8761b67..69bc80bb39053 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -160,8 +160,14 @@ pub struct JustificationNotification { pub justification: Vec, } +// Stream of justifications returned when subscribing. type JustificationStream = TracingUnboundedReceiver>; + +// Sending endpoint for notifying about justifications. type FinalityNotifier = TracingUnboundedSender>; + +// Collection of channel sending endpoints shared with the receiver side so they can register +// themselves. type SharedFinalityNotifiers = Arc>>>; /// The sending half of the Grandpa justification channel. @@ -176,7 +182,7 @@ pub struct GrandpaJustificationSender { impl GrandpaJustificationSender { /// The `notifiers` should be shared with a corresponding /// `GrandpaJustificationReceiver`. - pub fn new(notifiers: SharedFinalityNotifiers) -> Self { + fn new(notifiers: SharedFinalityNotifiers) -> Self { Self { notifiers, } @@ -204,11 +210,19 @@ pub struct GrandpaJustificationReceiver { } impl GrandpaJustificationReceiver { + /// Creates a new pair of receiver and sender of justification notifications. + pub fn channel() -> (GrandpaJustificationSender, Self) { + let finality_notifiers = Arc::new(Mutex::new(vec![])); + let receiver = GrandpaJustificationReceiver::new(finality_notifiers.clone()); + let sender = GrandpaJustificationSender::new(finality_notifiers.clone()); + (sender, receiver) + } + /// Create a new receiver of justification notifications. /// /// The `notifiers` should be shared with a corresponding /// `GrandpaJustificationSender`. - pub fn new(notifiers: SharedFinalityNotifiers) -> Self { + fn new(notifiers: SharedFinalityNotifiers) -> Self { Self { notifiers, } @@ -223,8 +237,6 @@ impl GrandpaJustificationReceiver { } } - - /// Finality proof provider for serving network requests. pub struct FinalityProofProvider { backend: Arc, diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 2a82ffb46087f..0c3abca1fde34 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -568,12 +568,8 @@ where let (voter_commands_tx, voter_commands_rx) = tracing_unbounded("mpsc_grandpa_voter_command"); - // Q: Is there any way to enforce that they point to the same set of notifiers? - let finality_notifiers = Arc::new(Mutex::new(vec![])); - let justification_receiver = - GrandpaJustificationReceiver::new(finality_notifiers.clone()); - let justification_sender = - GrandpaJustificationSender::new(finality_notifiers.clone()); + let (justification_sender, justification_receiver) = + GrandpaJustificationReceiver::channel(); // create pending change objects with 0 delay and enacted on finality // (i.e. standard changes) for each authority set hard fork. From 4c96b73612cf7139acda2c5e2482a246bce4be3c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 26 Jun 2020 22:57:45 +0200 Subject: [PATCH 41/59] grandpa: make the justification sender optional --- client/finality-grandpa/src/environment.rs | 8 +++++--- client/finality-grandpa/src/import.rs | 2 +- client/finality-grandpa/src/lib.rs | 2 +- client/finality-grandpa/src/observer.rs | 13 +++++-------- client/finality-grandpa/src/tests.rs | 5 +---- 5 files changed, 13 insertions(+), 17 deletions(-) diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index f11780775839c..153e44a4993cc 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -404,7 +404,7 @@ pub(crate) struct Environment, SC, pub(crate) voter_set_state: SharedVoterSetState, pub(crate) voting_rule: VR, pub(crate) metrics: Option, - pub(crate) justification_sender: GrandpaJustificationSender, + pub(crate) justification_sender: Option>, pub(crate) _phantom: PhantomData, } @@ -1075,7 +1075,7 @@ pub(crate) fn finalize_block( number: NumberFor, justification_or_commit: JustificationOrCommit, initial_sync: bool, - justification_sender: &GrandpaJustificationSender, + justification_sender: &Option>, ) -> Result<(), CommandOrError>> where Block: BlockT, BE: Backend, @@ -1189,7 +1189,9 @@ pub(crate) fn finalize_block( header, justification, }; - let _ = justification_sender.notify(notification); + if let Some(sender) = justification_sender { + sender.notify(notification); + } }, Ok(None) => debug!(target: "afg", "Expected a header for sending a justification notification."), Err(err) => debug!(target: "afg", "Getting header failed: {}", err), diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index d840767544872..aa08931b6b5c1 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -653,7 +653,7 @@ where number, justification.into(), initial_sync, - &self.justification_sender, + &Some(self.justification_sender.clone()), ); match result { diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 0c3abca1fde34..333dcf6efba0c 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -875,7 +875,7 @@ where consensus_changes: persistent_data.consensus_changes.clone(), voter_set_state: persistent_data.set_state, metrics: metrics.as_ref().map(|m| m.environment.clone()), - justification_sender, + justification_sender: Some(justification_sender), _phantom: PhantomData, }); diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index 4c52da707692a..760c3fd5fa5aa 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -70,7 +70,7 @@ fn grandpa_observer( authority_set: &SharedAuthoritySet>, consensus_changes: &SharedConsensusChanges>, voters: &Arc>, - justification_sender: &GrandpaJustificationSender, + justification_sender: &Option>, last_finalized_number: NumberFor, commits: S, note_round: F, @@ -198,7 +198,7 @@ where persistent_data, config.keystore, voter_commands_rx, - justification_sender, + Some(justification_sender), ); let observer_work = observer_work @@ -219,7 +219,7 @@ struct ObserverWork> { persistent_data: PersistentData, keystore: Option, voter_commands_rx: TracingUnboundedReceiver>>, - justification_sender: GrandpaJustificationSender, + justification_sender: Option>, _phantom: PhantomData, } @@ -237,7 +237,7 @@ where persistent_data: PersistentData, keystore: Option, voter_commands_rx: TracingUnboundedReceiver>>, - justification_sender: GrandpaJustificationSender, + justification_sender: Option>, ) -> Self { let mut work = ObserverWork { @@ -433,16 +433,13 @@ mod tests { let (_tx, voter_command_rx) = tracing_unbounded(""); - let finality_notifiers = Arc::new(parking_lot::Mutex::new(vec![])); - let justification_sender = GrandpaJustificationSender::new(finality_notifiers.clone()); - let observer = ObserverWork::new( client, tester.net_handle.clone(), persistent_data, None, voter_command_rx, - justification_sender, + None, ); // Trigger a reputation change through the gossip validator. diff --git a/client/finality-grandpa/src/tests.rs b/client/finality-grandpa/src/tests.rs index bfcc2844ef63b..a309591944c98 100644 --- a/client/finality-grandpa/src/tests.rs +++ b/client/finality-grandpa/src/tests.rs @@ -1558,9 +1558,6 @@ fn grandpa_environment_respects_voting_rules() { None, ); - let finality_notifiers = Arc::new(parking_lot::Mutex::new(vec![])); - let justification_sender = GrandpaJustificationSender::new(finality_notifiers.clone()); - Environment { authority_set: authority_set.clone(), config: config.clone(), @@ -1573,7 +1570,7 @@ fn grandpa_environment_respects_voting_rules() { network, voting_rule, metrics: None, - justification_sender, + justification_sender: None, _phantom: PhantomData, } }; From 12158a2b942d68cee7b4dbc1d8c549dc6e784e7b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Sun, 28 Jun 2020 00:27:03 +0200 Subject: [PATCH 42/59] grandpa: fix compilation warnings --- client/finality-grandpa/rpc/src/lib.rs | 1 - client/finality-grandpa/src/environment.rs | 2 +- client/finality-grandpa/src/lib.rs | 2 +- 3 files changed, 2 insertions(+), 3 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 782c50812fa83..dbc277ae389bd 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -154,7 +154,6 @@ mod tests { use super::*; use std::{collections::HashSet, convert::TryInto, sync::Arc}; use jsonrpc_core::Output; - use parking_lot::Mutex; use sc_finality_grandpa::{report, AuthorityId}; use sp_core::crypto::Public; diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 153e44a4993cc..3df0de60e403c 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -1190,7 +1190,7 @@ pub(crate) fn finalize_block( justification, }; if let Some(sender) = justification_sender { - sender.notify(notification); + let _ = sender.notify(notification); } }, Ok(None) => debug!(target: "afg", "Expected a header for sending a justification notification."), diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 333dcf6efba0c..0dbe9e78a6470 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -81,7 +81,7 @@ use sp_core::{ use sp_application_crypto::AppKey; use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver}; use sc_telemetry::{telemetry, CONSENSUS_INFO, CONSENSUS_DEBUG}; -use parking_lot::{RwLock, Mutex}; +use parking_lot::RwLock; use finality_grandpa::Error as GrandpaError; use finality_grandpa::{voter, BlockNumberOps, voter_set::VoterSet}; From e2584e5800b1577c8ccff6cc6d5802ff7535f558 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Sun, 28 Jun 2020 00:48:31 +0200 Subject: [PATCH 43/59] grandpa: move justification notification types to new file --- client/finality-grandpa/src/environment.rs | 2 +- client/finality-grandpa/src/finality_proof.rs | 89 -------------- client/finality-grandpa/src/import.rs | 2 +- client/finality-grandpa/src/lib.rs | 8 +- client/finality-grandpa/src/notification.rs | 110 ++++++++++++++++++ client/finality-grandpa/src/observer.rs | 2 +- 6 files changed, 117 insertions(+), 96 deletions(-) create mode 100644 client/finality-grandpa/src/notification.rs diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 3df0de60e403c..59f1340b072f2 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -51,7 +51,7 @@ use sp_consensus::SelectChain; use crate::authorities::{AuthoritySet, SharedAuthoritySet}; use crate::communication::Network as NetworkT; use crate::consensus_changes::SharedConsensusChanges; -use crate::finality_proof::{GrandpaJustificationSender, JustificationNotification}; +use crate::notification::{GrandpaJustificationSender, JustificationNotification}; use crate::justification::GrandpaJustification; use crate::until_imported::UntilVoteTargetImported; use crate::voting_rule::VotingRule; diff --git a/client/finality-grandpa/src/finality_proof.rs b/client/finality-grandpa/src/finality_proof.rs index 69bc80bb39053..fc6ca86db8ccd 100644 --- a/client/finality-grandpa/src/finality_proof.rs +++ b/client/finality-grandpa/src/finality_proof.rs @@ -38,8 +38,6 @@ use std::sync::Arc; use log::{trace, warn}; -use parking_lot::Mutex; -use serde::{Deserialize, Serialize}; use sp_blockchain::{Backend as BlockchainBackend, Error as ClientError, Result as ClientResult}; use sc_client_api::{ @@ -56,7 +54,6 @@ use sp_runtime::{ use sp_core::storage::StorageKey; use sc_telemetry::{telemetry, CONSENSUS_INFO}; use sp_finality_grandpa::{AuthorityId, AuthorityList, VersionedAuthorityList, GRANDPA_AUTHORITIES_KEY}; -use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedSender, TracingUnboundedReceiver}; use crate::justification::GrandpaJustification; use crate::VoterSet; @@ -151,92 +148,6 @@ impl AuthoritySetForFinalityChecker for Arc { - /// Highest finalized block header - pub header: Block::Header, - /// An encoded justification proving that the given header has been finalized - pub justification: Vec, -} - -// Stream of justifications returned when subscribing. -type JustificationStream = TracingUnboundedReceiver>; - -// Sending endpoint for notifying about justifications. -type FinalityNotifier = TracingUnboundedSender>; - -// Collection of channel sending endpoints shared with the receiver side so they can register -// themselves. -type SharedFinalityNotifiers = Arc>>>; - -/// The sending half of the Grandpa justification channel. -/// -/// Used to send notifictions about justifications generated -/// at the end of a Grandpa round. -#[derive(Clone)] -pub struct GrandpaJustificationSender { - notifiers: SharedFinalityNotifiers -} - -impl GrandpaJustificationSender { - /// The `notifiers` should be shared with a corresponding - /// `GrandpaJustificationReceiver`. - fn new(notifiers: SharedFinalityNotifiers) -> Self { - Self { - notifiers, - } - } - - /// Send out a notification to all subscribers that a new justification - /// is available for a block. - pub fn notify(&self, notification: JustificationNotification) -> Result<(), ()> { - self.notifiers.lock() - .retain(|n| { - !n.is_closed() || n.unbounded_send(notification.clone()).is_ok() - }); - - Ok(()) - } -} - -/// The receiving half of the Grandpa justification channel. -/// -/// Used to recieve notifictions about justifications generated -/// at the end of a Grandpa round. -#[derive(Clone)] -pub struct GrandpaJustificationReceiver { - notifiers: SharedFinalityNotifiers -} - -impl GrandpaJustificationReceiver { - /// Creates a new pair of receiver and sender of justification notifications. - pub fn channel() -> (GrandpaJustificationSender, Self) { - let finality_notifiers = Arc::new(Mutex::new(vec![])); - let receiver = GrandpaJustificationReceiver::new(finality_notifiers.clone()); - let sender = GrandpaJustificationSender::new(finality_notifiers.clone()); - (sender, receiver) - } - - /// Create a new receiver of justification notifications. - /// - /// The `notifiers` should be shared with a corresponding - /// `GrandpaJustificationSender`. - fn new(notifiers: SharedFinalityNotifiers) -> Self { - Self { - notifiers, - } - } - - /// Subscribe to a channel through which justifications are sent - /// at the end of each Grandpa voting round. - pub fn subscribe(&self) -> JustificationStream { - let (sender, receiver) = tracing_unbounded("mpsc_justification_notification_stream"); - self.notifiers.lock().push(sender); - receiver - } -} - /// Finality proof provider for serving network requests. pub struct FinalityProofProvider { backend: Arc, diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index aa08931b6b5c1..c9f2d8d2f7bcc 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -44,7 +44,7 @@ use crate::authorities::{AuthoritySet, SharedAuthoritySet, DelayKind, PendingCha use crate::consensus_changes::SharedConsensusChanges; use crate::environment::finalize_block; use crate::justification::GrandpaJustification; -use crate::finality_proof::GrandpaJustificationSender; +use crate::notification::GrandpaJustificationSender; use std::marker::PhantomData; /// A block-import handler for GRANDPA. diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 0dbe9e78a6470..3e88873ccc901 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -118,15 +118,15 @@ mod finality_proof; mod import; mod justification; mod light_import; +mod notification; mod observer; mod until_imported; mod voting_rule; pub use authorities::SharedAuthoritySet; -pub use finality_proof::{ - FinalityProofProvider, StorageAndProofProvider, - GrandpaJustificationSender, GrandpaJustificationReceiver, - JustificationNotification, +pub use finality_proof::{FinalityProofProvider, StorageAndProofProvider}; +pub use notification::{ + GrandpaJustificationSender, GrandpaJustificationReceiver, JustificationNotification, }; pub use import::GrandpaBlockImport; pub use justification::GrandpaJustification; diff --git a/client/finality-grandpa/src/notification.rs b/client/finality-grandpa/src/notification.rs new file mode 100644 index 0000000000000..e9a13e68b2f0a --- /dev/null +++ b/client/finality-grandpa/src/notification.rs @@ -0,0 +1,110 @@ +// This file is part of Substrate. + +// Copyright (C) 2020 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 . + +use std::sync::Arc; +use parking_lot::Mutex; +use serde::{Deserialize, Serialize}; + +use sp_runtime::traits::Block as BlockT; +use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedSender, TracingUnboundedReceiver}; + +/// Justification for a finalized block. +#[derive(Clone, Serialize, Deserialize)] +pub struct JustificationNotification { + /// Highest finalized block header + pub header: Block::Header, + /// An encoded justification proving that the given header has been finalized + pub justification: Vec, +} + +// Stream of justifications returned when subscribing. +type JustificationStream = TracingUnboundedReceiver>; + +// Sending endpoint for notifying about justifications. +type FinalityNotifier = TracingUnboundedSender>; + +// Collection of channel sending endpoints shared with the receiver side so they can register +// themselves. +type SharedFinalityNotifiers = Arc>>>; + +/// The sending half of the Grandpa justification channel. +/// +/// Used to send notifictions about justifications generated +/// at the end of a Grandpa round. +#[derive(Clone)] +pub struct GrandpaJustificationSender { + notifiers: SharedFinalityNotifiers +} + +impl GrandpaJustificationSender { + /// The `notifiers` should be shared with a corresponding + /// `GrandpaJustificationReceiver`. + fn new(notifiers: SharedFinalityNotifiers) -> Self { + Self { + notifiers, + } + } + + /// Send out a notification to all subscribers that a new justification + /// is available for a block. + pub fn notify(&self, notification: JustificationNotification) -> Result<(), ()> { + self.notifiers.lock() + .retain(|n| { + !n.is_closed() || n.unbounded_send(notification.clone()).is_ok() + }); + + Ok(()) + } +} + +/// The receiving half of the Grandpa justification channel. +/// +/// Used to recieve notifictions about justifications generated +/// at the end of a Grandpa round. +#[derive(Clone)] +pub struct GrandpaJustificationReceiver { + notifiers: SharedFinalityNotifiers +} + +impl GrandpaJustificationReceiver { + /// Creates a new pair of receiver and sender of justification notifications. + pub fn channel() -> (GrandpaJustificationSender, Self) { + let finality_notifiers = Arc::new(Mutex::new(vec![])); + let receiver = GrandpaJustificationReceiver::new(finality_notifiers.clone()); + let sender = GrandpaJustificationSender::new(finality_notifiers.clone()); + (sender, receiver) + } + + /// Create a new receiver of justification notifications. + /// + /// The `notifiers` should be shared with a corresponding + /// `GrandpaJustificationSender`. + fn new(notifiers: SharedFinalityNotifiers) -> Self { + Self { + notifiers, + } + } + + /// Subscribe to a channel through which justifications are sent + /// at the end of each Grandpa voting round. + pub fn subscribe(&self) -> JustificationStream { + let (sender, receiver) = tracing_unbounded("mpsc_justification_notification_stream"); + self.notifiers.lock().push(sender); + receiver + } +} diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index 760c3fd5fa5aa..94387cbb047f1 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -40,7 +40,7 @@ use crate::{ use crate::authorities::SharedAuthoritySet; use crate::communication::{Network as NetworkT, NetworkBridge}; use crate::consensus_changes::SharedConsensusChanges; -use crate::finality_proof::GrandpaJustificationSender; +use crate::notification::GrandpaJustificationSender; use sp_finality_grandpa::AuthorityId; use std::marker::{PhantomData, Unpin}; From dc3c8881ed974aac7bdc318d7c463a3035cb9687 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Sun, 28 Jun 2020 01:14:08 +0200 Subject: [PATCH 44/59] grandpa-rpc: move JustificationNotification to grandpa-rpc --- client/finality-grandpa/rpc/src/lib.rs | 23 +++++++++++++++++++-- client/finality-grandpa/src/environment.rs | 7 ++----- client/finality-grandpa/src/lib.rs | 4 +--- client/finality-grandpa/src/notification.rs | 9 +------- 4 files changed, 25 insertions(+), 18 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index dbc277ae389bd..e3ac4c35cf244 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -28,11 +28,12 @@ use jsonrpc_core::futures::{ stream::Stream as Stream01, future::Future as Future01, }; +use serde::{Serialize, Deserialize}; mod error; mod report; -use sc_finality_grandpa::{JustificationNotification , GrandpaJustificationReceiver}; +use sc_finality_grandpa::GrandpaJustificationReceiver; use sp_runtime::traits::Block as BlockT; use report::{ReportAuthoritySet, ReportVoterState, ReportedRoundStates}; @@ -128,7 +129,7 @@ where subscriber: Subscriber> ) { let stream = self.justification_receiver.subscribe() - .map(|x| Ok::<_,()>(x)) + .map(|x| Ok::<_,()>(JustificationNotification::from(x))) .map_err(|e| warn!("Notification stream error: {:?}", e)) .compat(); @@ -149,6 +150,24 @@ where } } +/// Justification for a finalized block. +#[derive(Clone, Serialize, Deserialize)] +pub struct JustificationNotification { + /// Highest finalized block header + pub header: Block::Header, + /// An encoded justification proving that the given header has been finalized + pub justification: Vec, +} + +impl From<(Block::Header, Vec)> for JustificationNotification { + fn from(notification: (Block::Header, Vec)) -> Self { + JustificationNotification { + header: notification.0, + justification: notification.1, + } + } +} + #[cfg(test)] mod tests { use super::*; diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 59f1340b072f2..5bc1277d0e548 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -51,7 +51,7 @@ use sp_consensus::SelectChain; use crate::authorities::{AuthoritySet, SharedAuthoritySet}; use crate::communication::Network as NetworkT; use crate::consensus_changes::SharedConsensusChanges; -use crate::notification::{GrandpaJustificationSender, JustificationNotification}; +use crate::notification::GrandpaJustificationSender; use crate::justification::GrandpaJustification; use crate::until_imported::UntilVoteTargetImported; use crate::voting_rule::VotingRule; @@ -1185,10 +1185,7 @@ pub(crate) fn finalize_block( if let Some(justification) = justification.clone() { match client.header(BlockId::Hash(hash)) { Ok(Some(header)) => { - let notification = JustificationNotification { - header, - justification, - }; + let notification = (header, justification); if let Some(sender) = justification_sender { let _ = sender.notify(notification); } diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 3e88873ccc901..3544491de9970 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -125,9 +125,7 @@ mod voting_rule; pub use authorities::SharedAuthoritySet; pub use finality_proof::{FinalityProofProvider, StorageAndProofProvider}; -pub use notification::{ - GrandpaJustificationSender, GrandpaJustificationReceiver, JustificationNotification, -}; +pub use notification::{GrandpaJustificationSender, GrandpaJustificationReceiver}; pub use import::GrandpaBlockImport; pub use justification::GrandpaJustification; pub use light_import::light_block_import; diff --git a/client/finality-grandpa/src/notification.rs b/client/finality-grandpa/src/notification.rs index e9a13e68b2f0a..f95bb58c2c7de 100644 --- a/client/finality-grandpa/src/notification.rs +++ b/client/finality-grandpa/src/notification.rs @@ -18,19 +18,12 @@ use std::sync::Arc; use parking_lot::Mutex; -use serde::{Deserialize, Serialize}; use sp_runtime::traits::Block as BlockT; use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedSender, TracingUnboundedReceiver}; /// Justification for a finalized block. -#[derive(Clone, Serialize, Deserialize)] -pub struct JustificationNotification { - /// Highest finalized block header - pub header: Block::Header, - /// An encoded justification proving that the given header has been finalized - pub justification: Vec, -} +type JustificationNotification = (::Header, Vec); // Stream of justifications returned when subscribing. type JustificationStream = TracingUnboundedReceiver>; From 410af1fb8722108372df620e67ba5d204f3b3c71 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Sun, 28 Jun 2020 01:18:18 +0200 Subject: [PATCH 45/59] grandpa-rpc: move JustificationNotification to its own file --- client/finality-grandpa/rpc/src/lib.rs | 21 +--------- .../finality-grandpa/rpc/src/notification.rs | 38 +++++++++++++++++++ 2 files changed, 40 insertions(+), 19 deletions(-) create mode 100644 client/finality-grandpa/rpc/src/notification.rs diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index e3ac4c35cf244..97cf94d084101 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -28,15 +28,16 @@ use jsonrpc_core::futures::{ stream::Stream as Stream01, future::Future as Future01, }; -use serde::{Serialize, Deserialize}; mod error; +mod notification; mod report; use sc_finality_grandpa::GrandpaJustificationReceiver; use sp_runtime::traits::Block as BlockT; use report::{ReportAuthoritySet, ReportVoterState, ReportedRoundStates}; +use notification::JustificationNotification; /// Returned when Grandpa RPC endpoint is not ready. pub const NOT_READY_ERROR_CODE: i64 = 1; @@ -150,24 +151,6 @@ where } } -/// Justification for a finalized block. -#[derive(Clone, Serialize, Deserialize)] -pub struct JustificationNotification { - /// Highest finalized block header - pub header: Block::Header, - /// An encoded justification proving that the given header has been finalized - pub justification: Vec, -} - -impl From<(Block::Header, Vec)> for JustificationNotification { - fn from(notification: (Block::Header, Vec)) -> Self { - JustificationNotification { - header: notification.0, - justification: notification.1, - } - } -} - #[cfg(test)] mod tests { use super::*; diff --git a/client/finality-grandpa/rpc/src/notification.rs b/client/finality-grandpa/rpc/src/notification.rs new file mode 100644 index 0000000000000..f2f943d07041b --- /dev/null +++ b/client/finality-grandpa/rpc/src/notification.rs @@ -0,0 +1,38 @@ +// This file is part of Substrate. + +// Copyright (C) 2020 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 . + +use serde::{Serialize, Deserialize}; +use sp_runtime::traits::Block as BlockT; + +/// Justification for a finalized block. +#[derive(Clone, Serialize, Deserialize)] +pub struct JustificationNotification { + /// Highest finalized block header + pub header: Block::Header, + /// An encoded justification proving that the given header has been finalized + pub justification: Vec, +} + +impl From<(Block::Header, Vec)> for JustificationNotification { + fn from(notification: (Block::Header, Vec)) -> Self { + JustificationNotification { + header: notification.0, + justification: notification.1, + } + } +} \ No newline at end of file From fb3ad631efbcf47bf9aef58f7591a3ae069ee2f9 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Sun, 28 Jun 2020 01:25:07 +0200 Subject: [PATCH 46/59] grandpa: rename justification channel pairs --- bin/node/rpc/src/lib.rs | 4 ++-- client/finality-grandpa/rpc/src/lib.rs | 8 ++++---- client/finality-grandpa/src/environment.rs | 6 +++--- client/finality-grandpa/src/import.rs | 6 +++--- client/finality-grandpa/src/lib.rs | 14 +++++++------- client/finality-grandpa/src/notification.rs | 16 ++++++++-------- client/finality-grandpa/src/observer.rs | 8 ++++---- 7 files changed, 31 insertions(+), 31 deletions(-) diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index 478b7c7492bfa..bac840ed578bc 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -43,7 +43,7 @@ use sp_consensus_babe::BabeApi; use sc_consensus_epochs::SharedEpochChanges; use sc_consensus_babe::{Config, Epoch}; use sc_consensus_babe_rpc::BabeRpcHandler; -use sc_finality_grandpa::{SharedVoterState, SharedAuthoritySet, GrandpaJustificationReceiver}; +use sc_finality_grandpa::{SharedVoterState, SharedAuthoritySet, GrandpaJustifications}; use sc_finality_grandpa_rpc::GrandpaRpcHandler; use sc_rpc_api::DenyUnsafe; use jsonrpc_pubsub::manager::SubscriptionManager; @@ -77,7 +77,7 @@ pub struct GrandpaDeps { /// Authority set info. pub shared_authority_set: SharedAuthoritySet, /// Receives notifications about justification events from Grandpa. - pub justification_receiver: GrandpaJustificationReceiver, + pub justification_receiver: GrandpaJustifications, /// Subscription manager to keep track of pubsub subscribers. pub subscriptions: SubscriptionManager, } diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 97cf94d084101..442e3724d3a1d 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -33,7 +33,7 @@ mod error; mod notification; mod report; -use sc_finality_grandpa::GrandpaJustificationReceiver; +use sc_finality_grandpa::GrandpaJustifications; use sp_runtime::traits::Block as BlockT; use report::{ReportAuthoritySet, ReportVoterState, ReportedRoundStates}; @@ -88,7 +88,7 @@ pub trait GrandpaApi { pub struct GrandpaRpcHandler { authority_set: AuthoritySet, voter_state: VoterState, - justification_receiver: GrandpaJustificationReceiver, + justification_receiver: GrandpaJustifications, manager: SubscriptionManager, } @@ -97,7 +97,7 @@ impl GrandpaRpcHandler, + justification_receiver: GrandpaJustifications, manager: SubscriptionManager, ) -> Self { Self { @@ -217,7 +217,7 @@ mod tests { } fn setup_rpc_handler(voter_state: VoterState) -> GrandpaRpcHandler { - let (_, justification_receiver) = GrandpaJustificationReceiver::channel(); + let (_, justification_receiver) = GrandpaJustifications::channel(); let manager = SubscriptionManager::new(Arc::new(sc_rpc::testing::TaskExecutor)); let handler = GrandpaRpcHandler::new( diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 5bc1277d0e548..99f04739ee2e8 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -51,7 +51,7 @@ use sp_consensus::SelectChain; use crate::authorities::{AuthoritySet, SharedAuthoritySet}; use crate::communication::Network as NetworkT; use crate::consensus_changes::SharedConsensusChanges; -use crate::notification::GrandpaJustificationSender; +use crate::notification::GrandpaJustificationSubscribers; use crate::justification::GrandpaJustification; use crate::until_imported::UntilVoteTargetImported; use crate::voting_rule::VotingRule; @@ -404,7 +404,7 @@ pub(crate) struct Environment, SC, pub(crate) voter_set_state: SharedVoterSetState, pub(crate) voting_rule: VR, pub(crate) metrics: Option, - pub(crate) justification_sender: Option>, + pub(crate) justification_sender: Option>, pub(crate) _phantom: PhantomData, } @@ -1075,7 +1075,7 @@ pub(crate) fn finalize_block( number: NumberFor, justification_or_commit: JustificationOrCommit, initial_sync: bool, - justification_sender: &Option>, + justification_sender: &Option>, ) -> Result<(), CommandOrError>> where Block: BlockT, BE: Backend, diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index c9f2d8d2f7bcc..e1bc0d0aa1002 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -44,7 +44,7 @@ use crate::authorities::{AuthoritySet, SharedAuthoritySet, DelayKind, PendingCha use crate::consensus_changes::SharedConsensusChanges; use crate::environment::finalize_block; use crate::justification::GrandpaJustification; -use crate::notification::GrandpaJustificationSender; +use crate::notification::GrandpaJustificationSubscribers; use std::marker::PhantomData; /// A block-import handler for GRANDPA. @@ -63,7 +63,7 @@ pub struct GrandpaBlockImport { send_voter_commands: TracingUnboundedSender>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: HashMap>>, - justification_sender: GrandpaJustificationSender, + justification_sender: GrandpaJustificationSubscribers, _phantom: PhantomData, } @@ -563,7 +563,7 @@ impl GrandpaBlockImport>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: Vec<(SetId, PendingChange>)>, - justification_sender: GrandpaJustificationSender, + justification_sender: GrandpaJustificationSubscribers, ) -> GrandpaBlockImport { // check for and apply any forced authority set hard fork that applies // to the *current* authority set. diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 3544491de9970..d67b6e2a02f09 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -125,7 +125,7 @@ mod voting_rule; pub use authorities::SharedAuthoritySet; pub use finality_proof::{FinalityProofProvider, StorageAndProofProvider}; -pub use notification::{GrandpaJustificationSender, GrandpaJustificationReceiver}; +pub use notification::{GrandpaJustificationSubscribers, GrandpaJustifications}; pub use import::GrandpaBlockImport; pub use justification::GrandpaJustification; pub use light_import::light_block_import; @@ -449,8 +449,8 @@ pub struct LinkHalf { select_chain: SC, persistent_data: PersistentData, voter_commands_rx: TracingUnboundedReceiver>>, - justification_sender: GrandpaJustificationSender, - justification_receiver: GrandpaJustificationReceiver, + justification_sender: GrandpaJustificationSubscribers, + justification_receiver: GrandpaJustifications, } impl LinkHalf { @@ -460,12 +460,12 @@ impl LinkHalf { } /// Get the sender end of justification notifications. - pub fn justification_sender(&self) -> GrandpaJustificationSender { + pub fn justification_sender(&self) -> GrandpaJustificationSubscribers { self.justification_sender.clone() } /// Get the receiving end of justification notifications. - pub fn justification_receiver(&self) -> GrandpaJustificationReceiver { + pub fn justification_receiver(&self) -> GrandpaJustifications { self.justification_receiver.clone() } } @@ -567,7 +567,7 @@ where let (voter_commands_tx, voter_commands_rx) = tracing_unbounded("mpsc_grandpa_voter_command"); let (justification_sender, justification_receiver) = - GrandpaJustificationReceiver::channel(); + GrandpaJustifications::channel(); // create pending change objects with 0 delay and enacted on finality // (i.e. standard changes) for each authority set hard fork. @@ -849,7 +849,7 @@ where voter_commands_rx: TracingUnboundedReceiver>>, prometheus_registry: Option, shared_voter_state: SharedVoterState, - justification_sender: GrandpaJustificationSender, + justification_sender: GrandpaJustificationSubscribers, ) -> Self { let metrics = match prometheus_registry.as_ref().map(Metrics::register) { Some(Ok(metrics)) => Some(metrics), diff --git a/client/finality-grandpa/src/notification.rs b/client/finality-grandpa/src/notification.rs index f95bb58c2c7de..510511e645ca0 100644 --- a/client/finality-grandpa/src/notification.rs +++ b/client/finality-grandpa/src/notification.rs @@ -35,16 +35,16 @@ type FinalityNotifier = TracingUnboundedSender>; // themselves. type SharedFinalityNotifiers = Arc>>>; -/// The sending half of the Grandpa justification channel. +/// The sending half of the Grandpa justification channel(s). /// /// Used to send notifictions about justifications generated /// at the end of a Grandpa round. #[derive(Clone)] -pub struct GrandpaJustificationSender { +pub struct GrandpaJustificationSubscribers { notifiers: SharedFinalityNotifiers } -impl GrandpaJustificationSender { +impl GrandpaJustificationSubscribers { /// The `notifiers` should be shared with a corresponding /// `GrandpaJustificationReceiver`. fn new(notifiers: SharedFinalityNotifiers) -> Self { @@ -70,16 +70,16 @@ impl GrandpaJustificationSender { /// Used to recieve notifictions about justifications generated /// at the end of a Grandpa round. #[derive(Clone)] -pub struct GrandpaJustificationReceiver { +pub struct GrandpaJustifications { notifiers: SharedFinalityNotifiers } -impl GrandpaJustificationReceiver { +impl GrandpaJustifications { /// Creates a new pair of receiver and sender of justification notifications. - pub fn channel() -> (GrandpaJustificationSender, Self) { + pub fn channel() -> (GrandpaJustificationSubscribers, Self) { let finality_notifiers = Arc::new(Mutex::new(vec![])); - let receiver = GrandpaJustificationReceiver::new(finality_notifiers.clone()); - let sender = GrandpaJustificationSender::new(finality_notifiers.clone()); + let receiver = GrandpaJustifications::new(finality_notifiers.clone()); + let sender = GrandpaJustificationSubscribers::new(finality_notifiers.clone()); (sender, receiver) } diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index 94387cbb047f1..f4694c8b5f41d 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -40,7 +40,7 @@ use crate::{ use crate::authorities::SharedAuthoritySet; use crate::communication::{Network as NetworkT, NetworkBridge}; use crate::consensus_changes::SharedConsensusChanges; -use crate::notification::GrandpaJustificationSender; +use crate::notification::GrandpaJustificationSubscribers; use sp_finality_grandpa::AuthorityId; use std::marker::{PhantomData, Unpin}; @@ -70,7 +70,7 @@ fn grandpa_observer( authority_set: &SharedAuthoritySet>, consensus_changes: &SharedConsensusChanges>, voters: &Arc>, - justification_sender: &Option>, + justification_sender: &Option>, last_finalized_number: NumberFor, commits: S, note_round: F, @@ -219,7 +219,7 @@ struct ObserverWork> { persistent_data: PersistentData, keystore: Option, voter_commands_rx: TracingUnboundedReceiver>>, - justification_sender: Option>, + justification_sender: Option>, _phantom: PhantomData, } @@ -237,7 +237,7 @@ where persistent_data: PersistentData, keystore: Option, voter_commands_rx: TracingUnboundedReceiver>>, - justification_sender: Option>, + justification_sender: Option>, ) -> Self { let mut work = ObserverWork { From 5d8039977a5b53d195643d292be1c2f36527da8c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Sun, 28 Jun 2020 01:30:38 +0200 Subject: [PATCH 47/59] grandpa: rename notifier types --- client/finality-grandpa/src/notification.rs | 20 ++++++++++---------- 1 file changed, 10 insertions(+), 10 deletions(-) diff --git a/client/finality-grandpa/src/notification.rs b/client/finality-grandpa/src/notification.rs index 510511e645ca0..248cc6c13297b 100644 --- a/client/finality-grandpa/src/notification.rs +++ b/client/finality-grandpa/src/notification.rs @@ -29,11 +29,11 @@ type JustificationNotification = (::Header, Vec); type JustificationStream = TracingUnboundedReceiver>; // Sending endpoint for notifying about justifications. -type FinalityNotifier = TracingUnboundedSender>; +type JustificationSubscriber = TracingUnboundedSender>; // Collection of channel sending endpoints shared with the receiver side so they can register // themselves. -type SharedFinalityNotifiers = Arc>>>; +type SharedJustificationSubscribers = Arc>>>; /// The sending half of the Grandpa justification channel(s). /// @@ -41,22 +41,22 @@ type SharedFinalityNotifiers = Arc>>>; /// at the end of a Grandpa round. #[derive(Clone)] pub struct GrandpaJustificationSubscribers { - notifiers: SharedFinalityNotifiers + subscribers: SharedJustificationSubscribers } impl GrandpaJustificationSubscribers { /// The `notifiers` should be shared with a corresponding /// `GrandpaJustificationReceiver`. - fn new(notifiers: SharedFinalityNotifiers) -> Self { + fn new(subscribers: SharedJustificationSubscribers) -> Self { Self { - notifiers, + subscribers, } } /// Send out a notification to all subscribers that a new justification /// is available for a block. pub fn notify(&self, notification: JustificationNotification) -> Result<(), ()> { - self.notifiers.lock() + self.subscribers.lock() .retain(|n| { !n.is_closed() || n.unbounded_send(notification.clone()).is_ok() }); @@ -71,7 +71,7 @@ impl GrandpaJustificationSubscribers { /// at the end of a Grandpa round. #[derive(Clone)] pub struct GrandpaJustifications { - notifiers: SharedFinalityNotifiers + subscribers: SharedJustificationSubscribers } impl GrandpaJustifications { @@ -87,9 +87,9 @@ impl GrandpaJustifications { /// /// The `notifiers` should be shared with a corresponding /// `GrandpaJustificationSender`. - fn new(notifiers: SharedFinalityNotifiers) -> Self { + fn new(subscribers: SharedJustificationSubscribers) -> Self { Self { - notifiers, + subscribers, } } @@ -97,7 +97,7 @@ impl GrandpaJustifications { /// at the end of each Grandpa voting round. pub fn subscribe(&self) -> JustificationStream { let (sender, receiver) = tracing_unbounded("mpsc_justification_notification_stream"); - self.notifiers.lock().push(sender); + self.subscribers.lock().push(sender); receiver } } From 3f33fd2c83f2cec57e8c82a54e3334d033a47a1f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Mon, 29 Jun 2020 11:56:30 +0200 Subject: [PATCH 48/59] grandpa: pass justification as GrandpaJustification to the rpc module --- Cargo.lock | 1 + client/finality-grandpa/rpc/Cargo.toml | 1 + client/finality-grandpa/rpc/src/notification.rs | 8 +++++--- client/finality-grandpa/src/environment.rs | 11 +++++++---- client/finality-grandpa/src/justification.rs | 2 +- client/finality-grandpa/src/notification.rs | 4 +++- 6 files changed, 18 insertions(+), 9 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index ca0176a3c0291..5cef5a1089d63 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6304,6 +6304,7 @@ dependencies = [ "jsonrpc-pubsub", "lazy_static", "log", + "parity-scale-codec", "parking_lot 0.10.2", "sc-finality-grandpa", "sc-network-test", diff --git a/client/finality-grandpa/rpc/Cargo.toml b/client/finality-grandpa/rpc/Cargo.toml index ea8c93a7c4a7f..bde4cebdaa868 100644 --- a/client/finality-grandpa/rpc/Cargo.toml +++ b/client/finality-grandpa/rpc/Cargo.toml @@ -22,6 +22,7 @@ serde_json = "1.0.50" log = "0.4.8" derive_more = "0.99.2" parking_lot = "0.10.0" +parity-scale-codec = { version = "1.3.0", features = ["derive"] } [dev-dependencies] sp-core = { version = "2.0.0-rc3", path = "../../../primitives/core" } diff --git a/client/finality-grandpa/rpc/src/notification.rs b/client/finality-grandpa/rpc/src/notification.rs index f2f943d07041b..678f91a6a233f 100644 --- a/client/finality-grandpa/rpc/src/notification.rs +++ b/client/finality-grandpa/rpc/src/notification.rs @@ -17,7 +17,9 @@ // along with this program. If not, see . use serde::{Serialize, Deserialize}; +use parity_scale_codec::Encode; use sp_runtime::traits::Block as BlockT; +use sc_finality_grandpa::GrandpaJustification; /// Justification for a finalized block. #[derive(Clone, Serialize, Deserialize)] @@ -28,11 +30,11 @@ pub struct JustificationNotification { pub justification: Vec, } -impl From<(Block::Header, Vec)> for JustificationNotification { - fn from(notification: (Block::Header, Vec)) -> Self { +impl From<(Block::Header, GrandpaJustification)> for JustificationNotification { + fn from(notification: (Block::Header, GrandpaJustification)) -> Self { JustificationNotification { header: notification.0, - justification: notification.1, + justification: notification.1.encode(), } } } \ No newline at end of file diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index 99f04739ee2e8..9ef5078ed1918 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -1148,7 +1148,7 @@ pub(crate) fn finalize_block( // justifications for transition blocks which will be requested by // syncing clients. let justification = match justification_or_commit { - JustificationOrCommit::Justification(justification) => Some(justification.encode()), + JustificationOrCommit::Justification(justification) => Some(justification), JustificationOrCommit::Commit((round_number, commit)) => { let mut justification_required = // justification is always required when block that enacts new authorities @@ -1175,17 +1175,18 @@ pub(crate) fn finalize_block( commit, )?; - Some(justification.encode()) + Some(justification) } else { None } }, }; - if let Some(justification) = justification.clone() { + // Notify any registered listeners in case we have a justification + if let Some(ref justification) = justification { match client.header(BlockId::Hash(hash)) { Ok(Some(header)) => { - let notification = (header, justification); + let notification = (header, justification.clone()); if let Some(sender) = justification_sender { let _ = sender.notify(notification); } @@ -1195,6 +1196,8 @@ pub(crate) fn finalize_block( }; } + let justification = justification.map(|j| j.encode()); + debug!(target: "afg", "Finalizing blocks up to ({:?}, {})", number, hash); // ideally some handle to a synchronization oracle would be used diff --git a/client/finality-grandpa/src/justification.rs b/client/finality-grandpa/src/justification.rs index b4db81f8a42bf..7035571dac217 100644 --- a/client/finality-grandpa/src/justification.rs +++ b/client/finality-grandpa/src/justification.rs @@ -37,7 +37,7 @@ use crate::{Commit, Error}; /// /// This is meant to be stored in the db and passed around the network to other /// nodes, and are used by syncing nodes to prove authority set handoffs. -#[derive(Encode, Decode)] +#[derive(Clone, Encode, Decode)] pub struct GrandpaJustification { round: u64, pub(crate) commit: Commit, diff --git a/client/finality-grandpa/src/notification.rs b/client/finality-grandpa/src/notification.rs index 248cc6c13297b..0fd6600ebe88a 100644 --- a/client/finality-grandpa/src/notification.rs +++ b/client/finality-grandpa/src/notification.rs @@ -22,8 +22,10 @@ use parking_lot::Mutex; use sp_runtime::traits::Block as BlockT; use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedSender, TracingUnboundedReceiver}; +use crate::justification::GrandpaJustification; + /// Justification for a finalized block. -type JustificationNotification = (::Header, Vec); +type JustificationNotification = (::Header, GrandpaJustification); // Stream of justifications returned when subscribing. type JustificationStream = TracingUnboundedReceiver>; From 90ae56ed94f7714db7a5feed2b3e634cb68802ae Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Tomasz=20Drwi=C4=99ga?= Date: Tue, 30 Jun 2020 12:29:11 +0200 Subject: [PATCH 49/59] Move Metadata to sc-rpc-api --- Cargo.lock | 2 -- bin/node/rpc/Cargo.toml | 18 ++++++++---------- bin/node/rpc/src/lib.rs | 18 +++++++++--------- client/rpc-api/src/lib.rs | 4 +++- client/{rpc => rpc-api}/src/metadata.rs | 4 ++-- client/rpc/src/author/mod.rs | 2 +- client/rpc/src/chain/mod.rs | 14 +++++++------- client/rpc/src/lib.rs | 5 +---- client/rpc/src/state/mod.rs | 12 ++++++------ client/rpc/src/state/state_full.rs | 8 ++++---- client/rpc/src/state/state_light.rs | 8 ++++---- 11 files changed, 45 insertions(+), 50 deletions(-) rename client/{rpc => rpc-api}/src/metadata.rs (95%) diff --git a/Cargo.lock b/Cargo.lock index 47355a8156757..6a9f29d5b2eed 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -3485,14 +3485,12 @@ dependencies = [ "sc-finality-grandpa", "sc-finality-grandpa-rpc", "sc-keystore", - "sc-rpc", "sc-rpc-api", "sp-api", "sp-block-builder", "sp-blockchain", "sp-consensus", "sp-consensus-babe", - "sp-runtime", "sp-transaction-pool", "substrate-frame-rpc-system", ] diff --git a/bin/node/rpc/Cargo.toml b/bin/node/rpc/Cargo.toml index 9536b85266bad..2095e688c2c2e 100644 --- a/bin/node/rpc/Cargo.toml +++ b/bin/node/rpc/Cargo.toml @@ -11,26 +11,24 @@ repository = "https://github.com/paritytech/substrate/" targets = ["x86_64-unknown-linux-gnu"] [dependencies] -sc-client-api = { version = "2.0.0-rc4", path = "../../../client/api" } jsonrpc-core = "14.2.0" jsonrpc-pubsub = "14.2.0" node-primitives = { version = "2.0.0-rc4", path = "../primitives" } node-runtime = { version = "2.0.0-rc4", path = "../runtime" } -sp-runtime = { version = "2.0.0-rc4", path = "../../../primitives/runtime" } -sp-api = { version = "2.0.0-rc4", path = "../../../primitives/api" } pallet-contracts-rpc = { version = "0.8.0-rc4", path = "../../../frame/contracts/rpc/" } pallet-transaction-payment-rpc = { version = "2.0.0-rc4", path = "../../../frame/transaction-payment/rpc/" } -substrate-frame-rpc-system = { version = "2.0.0-rc4", path = "../../../utils/frame/rpc/system" } -sp-transaction-pool = { version = "2.0.0-rc4", path = "../../../primitives/transaction-pool" } +sc-client-api = { version = "2.0.0-rc4", path = "../../../client/api" } sc-consensus-babe = { version = "0.8.0-rc4", path = "../../../client/consensus/babe" } sc-consensus-babe-rpc = { version = "0.8.0-rc4", path = "../../../client/consensus/babe/rpc" } -sp-consensus-babe = { version = "0.8.0-rc4", path = "../../../primitives/consensus/babe" } -sc-keystore = { version = "2.0.0-rc4", path = "../../../client/keystore" } sc-consensus-epochs = { version = "0.8.0-rc4", path = "../../../client/consensus/epochs" } -sp-consensus = { version = "0.8.0-rc4", path = "../../../primitives/consensus/common" } -sp-blockchain = { version = "2.0.0-rc4", path = "../../../primitives/blockchain" } sc-finality-grandpa = { version = "0.8.0-rc4", path = "../../../client/finality-grandpa" } sc-finality-grandpa-rpc = { version = "0.8.0-rc4", path = "../../../client/finality-grandpa/rpc" } +sc-keystore = { version = "2.0.0-rc4", path = "../../../client/keystore" } sc-rpc-api = { version = "0.8.0-rc4", path = "../../../client/rpc-api" } -sc-rpc = { version = "2.0.0-rc4", path = "../../../client/rpc" } +sp-api = { version = "2.0.0-rc4", path = "../../../primitives/api" } sp-block-builder = { version = "2.0.0-rc4", path = "../../../primitives/block-builder" } +sp-blockchain = { version = "2.0.0-rc4", path = "../../../primitives/blockchain" } +sp-consensus = { version = "0.8.0-rc4", path = "../../../primitives/consensus/common" } +sp-consensus-babe = { version = "0.8.0-rc4", path = "../../../primitives/consensus/babe" } +sp-transaction-pool = { version = "2.0.0-rc4", path = "../../../primitives/transaction-pool" } +substrate-frame-rpc-system = { version = "2.0.0-rc4", path = "../../../utils/frame/rpc/system" } diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index c150526323619..5ac7aee80c27b 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -32,22 +32,22 @@ use std::sync::Arc; +use jsonrpc_pubsub::manager::SubscriptionManager; use node_primitives::{Block, BlockNumber, AccountId, Index, Balance, Hash}; use node_runtime::UncheckedExtrinsic; -use sp_api::ProvideRuntimeApi; -use sp_transaction_pool::TransactionPool; -use sp_blockchain::{Error as BlockChainError, HeaderMetadata, HeaderBackend}; -use sp_consensus::SelectChain; -use sc_keystore::KeyStorePtr; -use sp_consensus_babe::BabeApi; -use sc_consensus_epochs::SharedEpochChanges; use sc_consensus_babe::{Config, Epoch}; use sc_consensus_babe_rpc::BabeRpcHandler; +use sc_consensus_epochs::SharedEpochChanges; use sc_finality_grandpa::{SharedVoterState, SharedAuthoritySet, GrandpaJustifications}; use sc_finality_grandpa_rpc::GrandpaRpcHandler; +use sc_keystore::KeyStorePtr; use sc_rpc_api::DenyUnsafe; +use sp_api::ProvideRuntimeApi; use sp_block_builder::BlockBuilder; -use jsonrpc_pubsub::manager::SubscriptionManager; +use sp_blockchain::{Error as BlockChainError, HeaderMetadata, HeaderBackend}; +use sp_consensus::SelectChain; +use sp_consensus_babe::BabeApi; +use sp_transaction_pool::TransactionPool; /// Light client extra dependencies. pub struct LightDeps { @@ -102,7 +102,7 @@ pub struct FullDeps { /// Instantiate all Full RPC extensions. pub fn create_full( deps: FullDeps, -) -> jsonrpc_core::MetaIoHandler where +) -> jsonrpc_core::MetaIoHandler where C: ProvideRuntimeApi, C: HeaderBackend + HeaderMetadata + 'static, C: Send + Sync + 'static, diff --git a/client/rpc-api/src/lib.rs b/client/rpc-api/src/lib.rs index cd2608dda92be..7bae75181056f 100644 --- a/client/rpc-api/src/lib.rs +++ b/client/rpc-api/src/lib.rs @@ -22,10 +22,12 @@ mod errors; mod helpers; +mod metadata; mod policy; -pub use jsonrpc_core::IoHandlerExtension as RpcExtension; pub use helpers::Receiver; +pub use jsonrpc_core::IoHandlerExtension as RpcExtension; +pub use metadata::Metadata; pub use policy::DenyUnsafe; pub mod author; diff --git a/client/rpc/src/metadata.rs b/client/rpc-api/src/metadata.rs similarity index 95% rename from client/rpc/src/metadata.rs rename to client/rpc-api/src/metadata.rs index 0416b07a6797b..cffcbf61f5440 100644 --- a/client/rpc/src/metadata.rs +++ b/client/rpc-api/src/metadata.rs @@ -19,8 +19,8 @@ //! RPC Metadata use std::sync::Arc; +use jsonrpc_core::futures::sync::mpsc; use jsonrpc_pubsub::{Session, PubSubMetadata}; -use rpc::futures::sync::mpsc; /// RPC Metadata. /// @@ -32,7 +32,7 @@ pub struct Metadata { session: Option>, } -impl rpc::Metadata for Metadata {} +impl jsonrpc_core::Metadata for Metadata {} impl PubSubMetadata for Metadata { fn session(&self) -> Option> { self.session.clone() diff --git a/client/rpc/src/author/mod.rs b/client/rpc/src/author/mod.rs index 974c1b85e1b39..e6384c995dce8 100644 --- a/client/rpc/src/author/mod.rs +++ b/client/rpc/src/author/mod.rs @@ -94,7 +94,7 @@ impl AuthorApi, BlockHash

> for Author Client: HeaderBackend + ProvideRuntimeApi + Send + Sync + 'static, Client::Api: SessionKeys, { - type Metadata = crate::metadata::Metadata; + type Metadata = crate::Metadata; fn insert_key( &self, diff --git a/client/rpc/src/chain/mod.rs b/client/rpc/src/chain/mod.rs index 8b6bf19d23565..cb67d9ba23166 100644 --- a/client/rpc/src/chain/mod.rs +++ b/client/rpc/src/chain/mod.rs @@ -106,7 +106,7 @@ trait ChainBackend: Send + Sync + 'static /// All new head subscription fn subscribe_all_heads( &self, - _metadata: crate::metadata::Metadata, + _metadata: crate::Metadata, subscriber: Subscriber, ) { subscribe_headers( @@ -123,7 +123,7 @@ trait ChainBackend: Send + Sync + 'static /// Unsubscribe from all head subscription. fn unsubscribe_all_heads( &self, - _metadata: Option, + _metadata: Option, id: SubscriptionId, ) -> RpcResult { Ok(self.subscriptions().cancel(id)) @@ -132,7 +132,7 @@ trait ChainBackend: Send + Sync + 'static /// New best head subscription fn subscribe_new_heads( &self, - _metadata: crate::metadata::Metadata, + _metadata: crate::Metadata, subscriber: Subscriber, ) { subscribe_headers( @@ -150,7 +150,7 @@ trait ChainBackend: Send + Sync + 'static /// Unsubscribe from new best head subscription. fn unsubscribe_new_heads( &self, - _metadata: Option, + _metadata: Option, id: SubscriptionId, ) -> RpcResult { Ok(self.subscriptions().cancel(id)) @@ -159,7 +159,7 @@ trait ChainBackend: Send + Sync + 'static /// Finalized head subscription fn subscribe_finalized_heads( &self, - _metadata: crate::metadata::Metadata, + _metadata: crate::Metadata, subscriber: Subscriber, ) { subscribe_headers( @@ -176,7 +176,7 @@ trait ChainBackend: Send + Sync + 'static /// Unsubscribe from finalized head subscription. fn unsubscribe_finalized_heads( &self, - _metadata: Option, + _metadata: Option, id: SubscriptionId, ) -> RpcResult { Ok(self.subscriptions().cancel(id)) @@ -230,7 +230,7 @@ impl ChainApi, Block::Hash, Block::Header, Signe Block: BlockT + 'static, Client: HeaderBackend + BlockchainEvents + 'static, { - type Metadata = crate::metadata::Metadata; + type Metadata = crate::Metadata; fn header(&self, hash: Option) -> FutureResult> { self.backend.header(hash) diff --git a/client/rpc/src/lib.rs b/client/rpc/src/lib.rs index cc35b56c063b6..9ff84d14f584e 100644 --- a/client/rpc/src/lib.rs +++ b/client/rpc/src/lib.rs @@ -22,10 +22,7 @@ #![warn(missing_docs)] -mod metadata; - -pub use sc_rpc_api::DenyUnsafe; -pub use self::metadata::Metadata; +pub use sc_rpc_api::{DenyUnsafe, Metadata}; pub use rpc::IoHandlerExtension as RpcExtension; pub mod author; diff --git a/client/rpc/src/state/mod.rs b/client/rpc/src/state/mod.rs index 921cc7efc699d..ded87c73dc8f7 100644 --- a/client/rpc/src/state/mod.rs +++ b/client/rpc/src/state/mod.rs @@ -140,21 +140,21 @@ pub trait StateBackend: Send + Sync + 'static /// New runtime version subscription fn subscribe_runtime_version( &self, - _meta: crate::metadata::Metadata, + _meta: crate::Metadata, subscriber: Subscriber, ); /// Unsubscribe from runtime version subscription fn unsubscribe_runtime_version( &self, - _meta: Option, + _meta: Option, id: SubscriptionId, ) -> RpcResult; /// New storage subscription fn subscribe_storage( &self, - _meta: crate::metadata::Metadata, + _meta: crate::Metadata, subscriber: Subscriber>, keys: Option>, ); @@ -162,7 +162,7 @@ pub trait StateBackend: Send + Sync + 'static /// Unsubscribe from storage subscription fn unsubscribe_storage( &self, - _meta: Option, + _meta: Option, id: SubscriptionId, ) -> RpcResult; } @@ -230,7 +230,7 @@ impl StateApi for State Block: BlockT + 'static, Client: Send + Sync + 'static, { - type Metadata = crate::metadata::Metadata; + type Metadata = crate::Metadata; fn call(&self, method: String, data: Bytes, block: Option) -> FutureResult { self.backend.call(block, method, data) @@ -390,7 +390,7 @@ impl ChildStateApi for ChildState Block: BlockT + 'static, Client: Send + Sync + 'static, { - type Metadata = crate::metadata::Metadata; + type Metadata = crate::Metadata; fn storage( &self, diff --git a/client/rpc/src/state/state_full.rs b/client/rpc/src/state/state_full.rs index f0ae79a033b58..3a5580c539a4c 100644 --- a/client/rpc/src/state/state_full.rs +++ b/client/rpc/src/state/state_full.rs @@ -373,7 +373,7 @@ impl StateBackend for FullState, ) { let stream = match self.client.storage_changes_notification_stream( @@ -424,7 +424,7 @@ impl StateBackend for FullState, + _meta: Option, id: SubscriptionId, ) -> RpcResult { Ok(self.subscriptions.cancel(id)) @@ -432,7 +432,7 @@ impl StateBackend for FullState>, keys: Option>, ) { @@ -484,7 +484,7 @@ impl StateBackend for FullState, + _meta: Option, id: SubscriptionId, ) -> RpcResult { Ok(self.subscriptions.cancel(id)) diff --git a/client/rpc/src/state/state_light.rs b/client/rpc/src/state/state_light.rs index ec275a2d78b79..1d9c4308800d1 100644 --- a/client/rpc/src/state/state_light.rs +++ b/client/rpc/src/state/state_light.rs @@ -289,7 +289,7 @@ impl StateBackend for LightState>, keys: Option> ) { @@ -384,7 +384,7 @@ impl StateBackend for LightState, + _meta: Option, id: SubscriptionId, ) -> RpcResult { if !self.subscriptions.cancel(id.clone()) { @@ -412,7 +412,7 @@ impl StateBackend for LightState, ) { self.subscriptions.add(subscriber, move |sink| { @@ -459,7 +459,7 @@ impl StateBackend for LightState, + _meta: Option, id: SubscriptionId, ) -> RpcResult { Ok(self.subscriptions.cancel(id)) From be1df4f0c0ddb4d70fab7ddba9efb0a9e8789211 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Mon, 29 Jun 2020 13:55:59 +0200 Subject: [PATCH 50/59] grandpa-rpc: remove unsed error code --- client/finality-grandpa/rpc/src/lib.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 442e3724d3a1d..0f6a7b14c6df5 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -41,8 +41,6 @@ use notification::JustificationNotification; /// Returned when Grandpa RPC endpoint is not ready. pub const NOT_READY_ERROR_CODE: i64 = 1; -/// Returned when unsubscribing to finality notifications fail. -pub const UNSUBSCRIBE_ERROR_CODE: i64 = 2; type FutureResult = Box + Send>; From d4cddf3d5068025b0090464f8df50e607deae2bc Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Wed, 1 Jul 2020 12:42:56 +0200 Subject: [PATCH 51/59] grandpa: fix bug for checking if channel is closed before sendind --- client/finality-grandpa/src/notification.rs | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/finality-grandpa/src/notification.rs b/client/finality-grandpa/src/notification.rs index 0fd6600ebe88a..7fde9b99e67bb 100644 --- a/client/finality-grandpa/src/notification.rs +++ b/client/finality-grandpa/src/notification.rs @@ -60,7 +60,7 @@ impl GrandpaJustificationSubscribers { pub fn notify(&self, notification: JustificationNotification) -> Result<(), ()> { self.subscribers.lock() .retain(|n| { - !n.is_closed() || n.unbounded_send(notification.clone()).is_ok() + !n.is_closed() && n.unbounded_send(notification.clone()).is_ok() }); Ok(()) From 8bd9e5c2d1c9358d6663f17cad0d5075f0e034c2 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 30 Jun 2020 23:47:10 +0200 Subject: [PATCH 52/59] grandpa-rpc: unit test for sending justifications --- Cargo.lock | 6 + client/finality-grandpa/rpc/Cargo.toml | 12 +- client/finality-grandpa/rpc/src/lib.rs | 135 ++++++++++++++++--- client/finality-grandpa/src/justification.rs | 2 +- client/finality-grandpa/src/notification.rs | 5 + 5 files changed, 135 insertions(+), 25 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 6a9f29d5b2eed..08c7307c288ed 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6411,13 +6411,19 @@ dependencies = [ "log", "parity-scale-codec", "parking_lot 0.10.2", + "sc-block-builder", "sc-finality-grandpa", "sc-network-test", "sc-rpc", "serde", "serde_json", + "sp-blockchain", + "sp-consensus", "sp-core", + "sp-finality-grandpa", + "sp-keyring", "sp-runtime", + "substrate-test-runtime-client", ] [[package]] diff --git a/client/finality-grandpa/rpc/Cargo.toml b/client/finality-grandpa/rpc/Cargo.toml index 1480e3e935e20..5268afba7d900 100644 --- a/client/finality-grandpa/rpc/Cargo.toml +++ b/client/finality-grandpa/rpc/Cargo.toml @@ -25,7 +25,13 @@ parking_lot = "0.10.0" parity-scale-codec = { version = "1.3.0", features = ["derive"] } [dev-dependencies] -sp-core = { version = "2.0.0-rc4", path = "../../../primitives/core" } -lazy_static = "1.4" +sc-block-builder = { version = "0.8.0-rc4", path = "../../block-builder" } sc-network-test = { version = "0.8.0-rc4", path = "../../network/test" } -sc-rpc = { version = "2.0.0-rc4", path = "../../rpc", features = ["test-helpers"] } \ No newline at end of file +sc-rpc = { version = "2.0.0-rc4", path = "../../rpc", features = ["test-helpers"] } +sp-blockchain = { version = "2.0.0-rc4", path = "../../../primitives/blockchain" } +sp-consensus = { version = "0.8.0-rc4", path = "../../../primitives/consensus/common" } +sp-core = { version = "2.0.0-rc4", path = "../../../primitives/core" } +sp-finality-grandpa = { version = "2.0.0-rc4", path = "../../../primitives/finality-grandpa" } +sp-keyring = { version = "2.0.0-rc4", path = "../../../primitives/keyring" } +substrate-test-runtime-client = { version = "2.0.0-rc4", path = "../../../test-utils/runtime/client" } +lazy_static = "1.4" \ No newline at end of file diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 0f6a7b14c6df5..4f9d282c88242 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -155,9 +155,20 @@ mod tests { use std::{collections::HashSet, convert::TryInto, sync::Arc}; use jsonrpc_core::Output; - use sc_finality_grandpa::{report, AuthorityId}; + use sc_block_builder::BlockBuilder; + use sc_finality_grandpa::{report, AuthorityId, GrandpaJustificationSubscribers, GrandpaJustification}; + use sp_blockchain::HeaderBackend; + use sp_consensus::RecordProof; use sp_core::crypto::Public; - use sc_network_test::Block; + use sp_keyring::Ed25519Keyring; + use sp_runtime::generic::Header; + use sp_runtime::traits::{Header as HeaderT, BlakeTwo256}; + use substrate_test_runtime_client::{ + runtime::Block, + DefaultTestClientBuilderExt, + TestClientBuilderExt, + TestClientBuilder, + }; struct TestAuthoritySet; struct TestVoterState; @@ -214,24 +225,31 @@ mod tests { } } - fn setup_rpc_handler(voter_state: VoterState) -> GrandpaRpcHandler { - let (_, justification_receiver) = GrandpaJustifications::channel(); + fn setup_io_handler(voter_state: VoterState) -> ( + jsonrpc_core::MetaIoHandler, + GrandpaJustificationSubscribers, + ) where + VoterState: ReportVoterState + Send + Sync + 'static, + { + let (subscribers, justifications) = GrandpaJustifications::channel(); let manager = SubscriptionManager::new(Arc::new(sc_rpc::testing::TaskExecutor)); let handler = GrandpaRpcHandler::new( TestAuthoritySet, voter_state, - justification_receiver, + justifications, manager, ); - handler + + let mut io = jsonrpc_core::MetaIoHandler::default(); + io.extend_with(GrandpaApi::to_delegate(handler)); + + (io, subscribers) } #[test] fn uninitialized_rpc_handler() { - let handler = setup_rpc_handler(EmptyVoterState); - let mut io = jsonrpc_core::MetaIoHandler::default(); - io.extend_with(GrandpaApi::to_delegate(handler)); + let (io, _) = setup_io_handler(EmptyVoterState); let request = r#"{"jsonrpc":"2.0","method":"grandpa_roundState","params":[],"id":1}"#; let response = r#"{"jsonrpc":"2.0","error":{"code":1,"message":"GRANDPA RPC endpoint not ready"},"id":1}"#; @@ -242,9 +260,7 @@ mod tests { #[test] fn working_rpc_handler() { - let handler = setup_rpc_handler(TestVoterState); - let mut io = jsonrpc_core::MetaIoHandler::default(); - io.extend_with(GrandpaApi::to_delegate(handler)); + let (io, _) = setup_io_handler(TestVoterState); let request = r#"{"jsonrpc":"2.0","method":"grandpa_roundState","params":[],"id":1}"#; let response = "{\"jsonrpc\":\"2.0\",\"result\":{\ @@ -265,19 +281,16 @@ mod tests { assert_eq!(io.handle_request_sync(request, meta), Some(response.into())); } - fn setup_io_handler() -> (sc_rpc::Metadata, jsonrpc_core::MetaIoHandler) { - let handler = setup_rpc_handler(TestVoterState); - let mut io = jsonrpc_core::MetaIoHandler::default(); - io.extend_with(GrandpaApi::to_delegate(handler)); - - let (tx, _rx) = jsonrpc_core::futures::sync::mpsc::channel(1); + fn setup_session() -> (sc_rpc::Metadata, jsonrpc_core::futures::sync::mpsc::Receiver) { + let (tx, rx) = jsonrpc_core::futures::sync::mpsc::channel(1); let meta = sc_rpc::Metadata::new(tx); - (meta, io) + (meta, rx) } #[test] fn subscribe_and_unsubscribe_to_justifications() { - let (meta, io) = setup_io_handler(); + let (io, _) = setup_io_handler(TestVoterState); + let (meta, _) = setup_session(); // Subscribe let sub_request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; @@ -308,7 +321,8 @@ mod tests { #[test] fn subscribe_and_unsubscribe_with_wrong_id() { - let (meta, io) = setup_io_handler(); + let (io, _) = setup_io_handler(TestVoterState); + let (meta, _) = setup_session(); // Subscribe let sub_request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; @@ -325,4 +339,83 @@ mod tests { Some(r#"{"jsonrpc":"2.0","result":false,"id":1}"#.into()) ); } + + fn create_justification() -> (Header, GrandpaJustification) { + let peers = &[Ed25519Keyring::Alice]; + + let builder = TestClientBuilder::new(); + let backend = builder.backend(); + let client = builder.build(); + let client = Arc::new(client); + + let built_block = BlockBuilder::new( + &*client, + client.info().best_hash, + client.info().best_number, + RecordProof::Yes, + Default::default(), + &*backend, + ).unwrap().build().unwrap(); + + let block = built_block.block; + let block_hash = block.hash(); + let block_header = block.header.clone(); + + let justification = { + let round = 1; + let set_id = 0; + + let precommit = finality_grandpa::Precommit { + target_hash: block_hash, + target_number: *block.header.number(), + }; + + let msg = finality_grandpa::Message::Precommit(precommit.clone()); + let encoded = sp_finality_grandpa::localized_payload(round, set_id, &msg); + let signature = peers[0].sign(&encoded[..]).into(); + + let precommit = finality_grandpa::SignedPrecommit { + precommit, + signature, + id: peers[0].public().into(), + }; + + let commit = finality_grandpa::Commit { + target_hash: block_hash, + target_number: *block.header.number(), + precommits: vec![precommit], + }; + + GrandpaJustification::from_commit( + &client, + round, + commit, + ).unwrap() + }; + + (block_header, justification) + } + + #[test] + #[ignore] + fn subscribe_and_listen_to_one_justification() { + let (io, subscribers) = setup_io_handler(TestVoterState); + let (meta, receiver) = setup_session(); + + let sub_request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; + + assert!(subscribers.len() == 0); + let resp = io.handle_request_sync(sub_request, meta.clone()); + assert!(subscribers.len() == 1); + dbg!(&resp); + + let (block_header, justification) = create_justification(); + let _ = subscribers.notify((block_header, justification)).unwrap(); + + let _ = receiver.for_each(|item| { + dbg!(item); + Ok(()) + }).wait().ok(); + } + } diff --git a/client/finality-grandpa/src/justification.rs b/client/finality-grandpa/src/justification.rs index 7035571dac217..0d47468726fd8 100644 --- a/client/finality-grandpa/src/justification.rs +++ b/client/finality-grandpa/src/justification.rs @@ -47,7 +47,7 @@ pub struct GrandpaJustification { impl GrandpaJustification { /// Create a GRANDPA justification from the given commit. This method /// assumes the commit is valid and well-formed. - pub(crate) fn from_commit( + pub fn from_commit( client: &Arc, round: u64, commit: Commit, diff --git a/client/finality-grandpa/src/notification.rs b/client/finality-grandpa/src/notification.rs index 7fde9b99e67bb..43723e25bac7c 100644 --- a/client/finality-grandpa/src/notification.rs +++ b/client/finality-grandpa/src/notification.rs @@ -65,6 +65,11 @@ impl GrandpaJustificationSubscribers { Ok(()) } + + /// Return the number of subscribers. + pub fn len(&self) -> usize { + self.subscribers.lock().len() + } } /// The receiving half of the Grandpa justification channel. From f43ee9b3472fb60a7d91149d01e5020b6ab8a717 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 2 Jul 2020 10:46:35 +0200 Subject: [PATCH 53/59] grandpa-rpc: update comments for the pubsub test --- client/finality-grandpa/rpc/src/lib.rs | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 4f9d282c88242..806743ddafcf9 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -412,10 +412,12 @@ mod tests { let (block_header, justification) = create_justification(); let _ = subscribers.notify((block_header, justification)).unwrap(); + // WIP: we want to just poll a single time, ensuring we get the justification pushed. let _ = receiver.for_each(|item| { dbg!(item); Ok(()) - }).wait().ok(); - } + }).wait().ok(); // FIXME: hangs! + // WIP: unsubscribe and notify again to make sure we don't send anything. + } } From 39640c1912e93862f6fe6a41f9a96a192b343879 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 2 Jul 2020 10:54:48 +0200 Subject: [PATCH 54/59] grandpa-rpc: update pubsub tests with more steps --- client/finality-grandpa/rpc/src/lib.rs | 34 +++++++++++++++++++++++--- 1 file changed, 31 insertions(+), 3 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 806743ddafcf9..e5142b3ac2db7 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -402,22 +402,50 @@ mod tests { let (io, subscribers) = setup_io_handler(TestVoterState); let (meta, receiver) = setup_session(); + // Subscribe let sub_request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; assert!(subscribers.len() == 0); let resp = io.handle_request_sync(sub_request, meta.clone()); + let resp: Output = serde_json::from_str(&resp.unwrap()).unwrap(); assert!(subscribers.len() == 1); - dbg!(&resp); + let sub_id = match resp { + Output::Success(success) => success.result, + _ => panic!(), + }; + + // Notify with a header and justification let (block_header, justification) = create_justification(); let _ = subscribers.notify((block_header, justification)).unwrap(); - // WIP: we want to just poll a single time, ensuring we get the justification pushed. + // FIXME: we want to just poll a single time, ensuring we get the justification pushed. let _ = receiver.for_each(|item| { dbg!(item); Ok(()) }).wait().ok(); // FIXME: hangs! - // WIP: unsubscribe and notify again to make sure we don't send anything. + // Unsubscribe + let unsub_req = format!( + "{{\"jsonrpc\":\"2.0\",\"method\":\"grandpa_unsubscribeJustifications\",\"params\":[{}],\"id\":1}}", + sub_id + ); + assert!(subscribers.len() == 1); + assert_eq!( + io.handle_request_sync(&unsub_req, meta.clone()), + Some(r#"{"jsonrpc":"2.0","result":true,"id":1}"#.into()), + ); + assert!(subscribers.len() == 0); + + // Notify with another header and justification + // TODO: make the second justification different. + let (block_header, justification) = create_justification(); + let _ = subscribers.notify((block_header, justification)).unwrap(); + + // FIXME: nothing should be sent since we unsubscribed. + //let _ = receiver.for_each(|item| { + // dbg!(item); + // Ok(()) + //}).wait().ok(); // FIXME: hangs! } } From f3c6c543af4acf873917747ea00fc26175f20613 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 2 Jul 2020 19:02:18 +0200 Subject: [PATCH 55/59] grandpa-rpc: fix pubsub test --- client/finality-grandpa/rpc/src/lib.rs | 57 ++++++++------------- client/finality-grandpa/src/notification.rs | 5 -- 2 files changed, 20 insertions(+), 42 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index e5142b3ac2db7..3acd60f242784 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -153,8 +153,9 @@ where mod tests { use super::*; use std::{collections::HashSet, convert::TryInto, sync::Arc}; - use jsonrpc_core::Output; + use jsonrpc_core::{Notification, Output, types::Params}; + use parity_scale_codec::Encode; use sc_block_builder::BlockBuilder; use sc_finality_grandpa::{report, AuthorityId, GrandpaJustificationSubscribers, GrandpaJustification}; use sp_blockchain::HeaderBackend; @@ -397,7 +398,6 @@ mod tests { } #[test] - #[ignore] fn subscribe_and_listen_to_one_justification() { let (io, subscribers) = setup_io_handler(TestVoterState); let (meta, receiver) = setup_session(); @@ -405,47 +405,30 @@ mod tests { // Subscribe let sub_request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; - assert!(subscribers.len() == 0); let resp = io.handle_request_sync(sub_request, meta.clone()); - let resp: Output = serde_json::from_str(&resp.unwrap()).unwrap(); - assert!(subscribers.len() == 1); - - let sub_id = match resp { - Output::Success(success) => success.result, - _ => panic!(), - }; + let mut resp: serde_json::Value = serde_json::from_str(&resp.unwrap()).unwrap(); + let sub_id: String = serde_json::from_value(resp["result"].take()).unwrap(); // Notify with a header and justification let (block_header, justification) = create_justification(); - let _ = subscribers.notify((block_header, justification)).unwrap(); - - // FIXME: we want to just poll a single time, ensuring we get the justification pushed. - let _ = receiver.for_each(|item| { - dbg!(item); - Ok(()) - }).wait().ok(); // FIXME: hangs! + let _ = subscribers.notify((block_header.clone(), justification.clone())).unwrap(); - // Unsubscribe - let unsub_req = format!( - "{{\"jsonrpc\":\"2.0\",\"method\":\"grandpa_unsubscribeJustifications\",\"params\":[{}],\"id\":1}}", - sub_id - ); - assert!(subscribers.len() == 1); - assert_eq!( - io.handle_request_sync(&unsub_req, meta.clone()), - Some(r#"{"jsonrpc":"2.0","result":true,"id":1}"#.into()), - ); - assert!(subscribers.len() == 0); + // Inspect what we received + let recv = receiver.take(1).wait().flatten().collect::>(); + let recv: Notification = serde_json::from_str(&recv[0]).unwrap(); + let mut json_map = match recv.params { + Params::Map(json_map) => json_map, + _ => panic!(), + }; - // Notify with another header and justification - // TODO: make the second justification different. - let (block_header, justification) = create_justification(); - let _ = subscribers.notify((block_header, justification)).unwrap(); + let recv_sub_id: String = serde_json::from_value(json_map["subscription"].take()).unwrap(); + let mut result = json_map["result"].take(); + let recv_block_header: Header = serde_json::from_value(result["header"].take()).unwrap(); + let recv_justification: Vec = serde_json::from_value(result["justification"].take()).unwrap(); - // FIXME: nothing should be sent since we unsubscribed. - //let _ = receiver.for_each(|item| { - // dbg!(item); - // Ok(()) - //}).wait().ok(); // FIXME: hangs! + assert_eq!(recv.method, "grandpa_justifications"); + assert_eq!(recv_sub_id, sub_id); + assert_eq!(recv_block_header, block_header); + assert_eq!(recv_justification, justification.encode()); } } diff --git a/client/finality-grandpa/src/notification.rs b/client/finality-grandpa/src/notification.rs index 43723e25bac7c..7fde9b99e67bb 100644 --- a/client/finality-grandpa/src/notification.rs +++ b/client/finality-grandpa/src/notification.rs @@ -65,11 +65,6 @@ impl GrandpaJustificationSubscribers { Ok(()) } - - /// Return the number of subscribers. - pub fn len(&self) -> usize { - self.subscribers.lock().len() - } } /// The receiving half of the Grandpa justification channel. From 44ba73ed94dfefb1dd3d894391c0ed9483ee3c17 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 3 Jul 2020 14:11:25 +0200 Subject: [PATCH 56/59] grandpa-rpc: minor indendation --- bin/node/rpc/src/lib.rs | 7 ++++++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index 5ac7aee80c27b..9e264bd0a05bf 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -166,7 +166,12 @@ pub fn create_full( ); io.extend_with( sc_finality_grandpa_rpc::GrandpaApi::to_delegate( - GrandpaRpcHandler::new(shared_authority_set, shared_voter_state, justification_receiver, subscriptions) + GrandpaRpcHandler::new( + shared_authority_set, + shared_voter_state, + justification_receiver, + subscriptions, + ) ) ); From 3f3b136546977eaacd896d6978ac154ee41ac67c Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Thu, 23 Jul 2020 12:42:43 +0200 Subject: [PATCH 57/59] grandpa-rpc: decode instead of encode in test --- client/finality-grandpa/rpc/src/lib.rs | 21 ++++++++++---------- client/finality-grandpa/src/justification.rs | 2 +- 2 files changed, 12 insertions(+), 11 deletions(-) diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 3acd60f242784..90d05c0164fc2 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -155,7 +155,7 @@ mod tests { use std::{collections::HashSet, convert::TryInto, sync::Arc}; use jsonrpc_core::{Notification, Output, types::Params}; - use parity_scale_codec::Encode; + use parity_scale_codec::Decode; use sc_block_builder::BlockBuilder; use sc_finality_grandpa::{report, AuthorityId, GrandpaJustificationSubscribers, GrandpaJustification}; use sp_blockchain::HeaderBackend; @@ -387,11 +387,7 @@ mod tests { precommits: vec![precommit], }; - GrandpaJustification::from_commit( - &client, - round, - commit, - ).unwrap() + GrandpaJustification::from_commit(&client, round, commit).unwrap() }; (block_header, justification) @@ -403,7 +399,8 @@ mod tests { let (meta, receiver) = setup_session(); // Subscribe - let sub_request = r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; + let sub_request = + r#"{"jsonrpc":"2.0","method":"grandpa_subscribeJustifications","params":[],"id":1}"#; let resp = io.handle_request_sync(sub_request, meta.clone()); let mut resp: serde_json::Value = serde_json::from_str(&resp.unwrap()).unwrap(); @@ -423,12 +420,16 @@ mod tests { let recv_sub_id: String = serde_json::from_value(json_map["subscription"].take()).unwrap(); let mut result = json_map["result"].take(); - let recv_block_header: Header = serde_json::from_value(result["header"].take()).unwrap(); - let recv_justification: Vec = serde_json::from_value(result["justification"].take()).unwrap(); + let recv_block_header: Header = + serde_json::from_value(result["header"].take()).unwrap(); + let recv_justification: Vec = + serde_json::from_value(result["justification"].take()).unwrap(); + let recv_justification: GrandpaJustification = + Decode::decode(&mut &recv_justification[..]).unwrap(); assert_eq!(recv.method, "grandpa_justifications"); assert_eq!(recv_sub_id, sub_id); assert_eq!(recv_block_header, block_header); - assert_eq!(recv_justification, justification.encode()); + assert_eq!(recv_justification, justification); } } diff --git a/client/finality-grandpa/src/justification.rs b/client/finality-grandpa/src/justification.rs index 7d90349538537..d5ca92d50e937 100644 --- a/client/finality-grandpa/src/justification.rs +++ b/client/finality-grandpa/src/justification.rs @@ -37,7 +37,7 @@ use crate::{Commit, Error}; /// /// This is meant to be stored in the db and passed around the network to other /// nodes, and are used by syncing nodes to prove authority set handoffs. -#[derive(Clone, Encode, Decode)] +#[derive(Clone, Encode, Decode, PartialEq, Eq, Debug)] pub struct GrandpaJustification { round: u64, pub(crate) commit: Commit, From 410ddea1c38cf1264922b4e37318b49fe4add361 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Fri, 31 Jul 2020 15:23:56 +0200 Subject: [PATCH 58/59] grandpa: fix review comments --- Cargo.lock | 1 - bin/node/cli/src/service.rs | 4 +- bin/node/rpc/src/lib.rs | 14 ++--- client/finality-grandpa/rpc/Cargo.toml | 1 - client/finality-grandpa/rpc/src/lib.rs | 49 ++++++++--------- .../finality-grandpa/rpc/src/notification.rs | 20 ++----- client/finality-grandpa/src/environment.rs | 21 +++---- client/finality-grandpa/src/import.rs | 6 +- client/finality-grandpa/src/lib.rs | 25 ++++----- client/finality-grandpa/src/notification.rs | 55 +++++++++---------- client/finality-grandpa/src/observer.rs | 8 +-- client/rpc/Cargo.toml | 4 +- 12 files changed, 89 insertions(+), 119 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 13b2cf8d74ebd..1dd3730a5d8ae 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6694,7 +6694,6 @@ dependencies = [ "lazy_static", "log", "parity-scale-codec", - "parking_lot 0.10.2", "sc-block-builder", "sc-finality-grandpa", "sc-network-test", diff --git a/bin/node/cli/src/service.rs b/bin/node/cli/src/service.rs index 51c01169bea34..9cea3801ed4de 100644 --- a/bin/node/cli/src/service.rs +++ b/bin/node/cli/src/service.rs @@ -111,7 +111,7 @@ pub fn new_full_params(config: Configuration) -> Result<( let (rpc_extensions_builder, rpc_setup) = { let (_, grandpa_link, babe_link) = &import_setup; - let justification_receiver = grandpa_link.justification_receiver(); + let justification_stream = grandpa_link.justification_stream(); let shared_authority_set = grandpa_link.shared_authority_set().clone(); let shared_voter_state = grandpa::SharedVoterState::empty(); @@ -139,7 +139,7 @@ pub fn new_full_params(config: Configuration) -> Result<( grandpa: node_rpc::GrandpaDeps { shared_voter_state: shared_voter_state.clone(), shared_authority_set: shared_authority_set.clone(), - justification_receiver: justification_receiver.clone(), + justification_stream: justification_stream.clone(), subscriptions, }, }; diff --git a/bin/node/rpc/src/lib.rs b/bin/node/rpc/src/lib.rs index c2ebc800e18a9..2c786f5ca72eb 100644 --- a/bin/node/rpc/src/lib.rs +++ b/bin/node/rpc/src/lib.rs @@ -38,7 +38,7 @@ use node_runtime::UncheckedExtrinsic; use sc_consensus_babe::{Config, Epoch}; use sc_consensus_babe_rpc::BabeRpcHandler; use sc_consensus_epochs::SharedEpochChanges; -use sc_finality_grandpa::{SharedVoterState, SharedAuthoritySet, GrandpaJustifications}; +use sc_finality_grandpa::{SharedVoterState, SharedAuthoritySet, GrandpaJustificationStream}; use sc_finality_grandpa_rpc::GrandpaRpcHandler; use sc_keystore::KeyStorePtr; use sc_rpc_api::DenyUnsafe; @@ -78,7 +78,7 @@ pub struct GrandpaDeps { /// Authority set info. pub shared_authority_set: SharedAuthoritySet, /// Receives notifications about justification events from Grandpa. - pub justification_receiver: GrandpaJustifications, + pub justification_stream: GrandpaJustificationStream, /// Subscription manager to keep track of pubsub subscribers. pub subscriptions: SubscriptionManager, } @@ -100,12 +100,12 @@ pub struct FullDeps { } /// A IO handler that uses all Full RPC extensions. -pub type IoHandler = jsonrpc_core::MetaIoHandler; +pub type IoHandler = jsonrpc_core::IoHandler; /// Instantiate all Full RPC extensions. pub fn create_full( deps: FullDeps, -) -> jsonrpc_core::MetaIoHandler where +) -> jsonrpc_core::IoHandler where C: ProvideRuntimeApi, C: HeaderBackend + HeaderMetadata + 'static, C: Send + Sync + 'static, @@ -121,7 +121,7 @@ pub fn create_full( use pallet_contracts_rpc::{Contracts, ContractsApi}; use pallet_transaction_payment_rpc::{TransactionPayment, TransactionPaymentApi}; - let mut io = jsonrpc_core::MetaIoHandler::default(); + let mut io = jsonrpc_core::IoHandler::default(); let FullDeps { client, pool, @@ -139,7 +139,7 @@ pub fn create_full( let GrandpaDeps { shared_voter_state, shared_authority_set, - justification_receiver, + justification_stream, subscriptions, } = grandpa; @@ -172,7 +172,7 @@ pub fn create_full( GrandpaRpcHandler::new( shared_authority_set, shared_voter_state, - justification_receiver, + justification_stream, subscriptions, ) ) diff --git a/client/finality-grandpa/rpc/Cargo.toml b/client/finality-grandpa/rpc/Cargo.toml index 3bd99ed3dc362..ca405eaec9dcb 100644 --- a/client/finality-grandpa/rpc/Cargo.toml +++ b/client/finality-grandpa/rpc/Cargo.toml @@ -21,7 +21,6 @@ serde = { version = "1.0.105", features = ["derive"] } serde_json = "1.0.50" log = "0.4.8" derive_more = "0.99.2" -parking_lot = "0.10.0" parity-scale-codec = { version = "1.3.0", features = ["derive"] } [dev-dependencies] diff --git a/client/finality-grandpa/rpc/src/lib.rs b/client/finality-grandpa/rpc/src/lib.rs index 90d05c0164fc2..c00c95c5f776f 100644 --- a/client/finality-grandpa/rpc/src/lib.rs +++ b/client/finality-grandpa/rpc/src/lib.rs @@ -33,7 +33,7 @@ mod error; mod notification; mod report; -use sc_finality_grandpa::GrandpaJustifications; +use sc_finality_grandpa::GrandpaJustificationStream; use sp_runtime::traits::Block as BlockT; use report::{ReportAuthoritySet, ReportVoterState, ReportedRoundStates}; @@ -86,28 +86,28 @@ pub trait GrandpaApi { pub struct GrandpaRpcHandler { authority_set: AuthoritySet, voter_state: VoterState, - justification_receiver: GrandpaJustifications, + justification_stream: GrandpaJustificationStream, manager: SubscriptionManager, } impl GrandpaRpcHandler { - /// Creates a new GrandpaRpcHander instance. + /// Creates a new GrandpaRpcHandler instance. pub fn new( authority_set: AuthoritySet, voter_state: VoterState, - justification_receiver: GrandpaJustifications, + justification_stream: GrandpaJustificationStream, manager: SubscriptionManager, ) -> Self { Self { authority_set, voter_state, - justification_receiver, + justification_stream, manager, } } } -impl GrandpaApi> +impl GrandpaApi for GrandpaRpcHandler where VoterState: ReportVoterState + Send + Sync + 'static, @@ -125,9 +125,9 @@ where fn subscribe_justifications( &self, _metadata: Self::Metadata, - subscriber: Subscriber> + subscriber: Subscriber ) { - let stream = self.justification_receiver.subscribe() + let stream = self.justification_stream.subscribe() .map(|x| Ok::<_,()>(JustificationNotification::from(x))) .map_err(|e| warn!("Notification stream error: {:?}", e)) .compat(); @@ -157,13 +157,12 @@ mod tests { use parity_scale_codec::Decode; use sc_block_builder::BlockBuilder; - use sc_finality_grandpa::{report, AuthorityId, GrandpaJustificationSubscribers, GrandpaJustification}; + use sc_finality_grandpa::{report, AuthorityId, GrandpaJustificationSender, GrandpaJustification}; use sp_blockchain::HeaderBackend; use sp_consensus::RecordProof; use sp_core::crypto::Public; use sp_keyring::Ed25519Keyring; - use sp_runtime::generic::Header; - use sp_runtime::traits::{Header as HeaderT, BlakeTwo256}; + use sp_runtime::traits::Header as HeaderT; use substrate_test_runtime_client::{ runtime::Block, DefaultTestClientBuilderExt, @@ -228,24 +227,24 @@ mod tests { fn setup_io_handler(voter_state: VoterState) -> ( jsonrpc_core::MetaIoHandler, - GrandpaJustificationSubscribers, + GrandpaJustificationSender, ) where VoterState: ReportVoterState + Send + Sync + 'static, { - let (subscribers, justifications) = GrandpaJustifications::channel(); + let (justification_sender, justification_stream) = GrandpaJustificationStream::channel(); let manager = SubscriptionManager::new(Arc::new(sc_rpc::testing::TaskExecutor)); let handler = GrandpaRpcHandler::new( TestAuthoritySet, voter_state, - justifications, + justification_stream, manager, ); let mut io = jsonrpc_core::MetaIoHandler::default(); io.extend_with(GrandpaApi::to_delegate(handler)); - (io, subscribers) + (io, justification_sender) } #[test] @@ -341,7 +340,7 @@ mod tests { ); } - fn create_justification() -> (Header, GrandpaJustification) { + fn create_justification() -> GrandpaJustification { let peers = &[Ed25519Keyring::Alice]; let builder = TestClientBuilder::new(); @@ -360,7 +359,6 @@ mod tests { let block = built_block.block; let block_hash = block.hash(); - let block_header = block.header.clone(); let justification = { let round = 1; @@ -390,12 +388,12 @@ mod tests { GrandpaJustification::from_commit(&client, round, commit).unwrap() }; - (block_header, justification) + justification } #[test] fn subscribe_and_listen_to_one_justification() { - let (io, subscribers) = setup_io_handler(TestVoterState); + let (io, justification_sender) = setup_io_handler(TestVoterState); let (meta, receiver) = setup_session(); // Subscribe @@ -407,8 +405,8 @@ mod tests { let sub_id: String = serde_json::from_value(resp["result"].take()).unwrap(); // Notify with a header and justification - let (block_header, justification) = create_justification(); - let _ = subscribers.notify((block_header.clone(), justification.clone())).unwrap(); + let justification = create_justification(); + let _ = justification_sender.notify(justification.clone()).unwrap(); // Inspect what we received let recv = receiver.take(1).wait().flatten().collect::>(); @@ -418,18 +416,15 @@ mod tests { _ => panic!(), }; - let recv_sub_id: String = serde_json::from_value(json_map["subscription"].take()).unwrap(); - let mut result = json_map["result"].take(); - let recv_block_header: Header = - serde_json::from_value(result["header"].take()).unwrap(); + let recv_sub_id: String = + serde_json::from_value(json_map["subscription"].take()).unwrap(); let recv_justification: Vec = - serde_json::from_value(result["justification"].take()).unwrap(); + serde_json::from_value(json_map["result"].take()).unwrap(); let recv_justification: GrandpaJustification = Decode::decode(&mut &recv_justification[..]).unwrap(); assert_eq!(recv.method, "grandpa_justifications"); assert_eq!(recv_sub_id, sub_id); - assert_eq!(recv_block_header, block_header); assert_eq!(recv_justification, justification); } } diff --git a/client/finality-grandpa/rpc/src/notification.rs b/client/finality-grandpa/rpc/src/notification.rs index 678f91a6a233f..831f4681549a7 100644 --- a/client/finality-grandpa/rpc/src/notification.rs +++ b/client/finality-grandpa/rpc/src/notification.rs @@ -21,20 +21,12 @@ use parity_scale_codec::Encode; use sp_runtime::traits::Block as BlockT; use sc_finality_grandpa::GrandpaJustification; -/// Justification for a finalized block. +/// An encoded justification proving that the given header has been finalized #[derive(Clone, Serialize, Deserialize)] -pub struct JustificationNotification { - /// Highest finalized block header - pub header: Block::Header, - /// An encoded justification proving that the given header has been finalized - pub justification: Vec, -} +pub struct JustificationNotification(Vec); -impl From<(Block::Header, GrandpaJustification)> for JustificationNotification { - fn from(notification: (Block::Header, GrandpaJustification)) -> Self { - JustificationNotification { - header: notification.0, - justification: notification.1.encode(), - } +impl From> for JustificationNotification { + fn from(notification: GrandpaJustification) -> Self { + JustificationNotification(notification.encode()) } -} \ No newline at end of file +} diff --git a/client/finality-grandpa/src/environment.rs b/client/finality-grandpa/src/environment.rs index ca0dc679e3a23..97aaedba8167f 100644 --- a/client/finality-grandpa/src/environment.rs +++ b/client/finality-grandpa/src/environment.rs @@ -51,7 +51,7 @@ use sp_consensus::SelectChain; use crate::authorities::{AuthoritySet, SharedAuthoritySet}; use crate::communication::Network as NetworkT; use crate::consensus_changes::SharedConsensusChanges; -use crate::notification::GrandpaJustificationSubscribers; +use crate::notification::GrandpaJustificationSender; use crate::justification::GrandpaJustification; use crate::until_imported::UntilVoteTargetImported; use crate::voting_rule::VotingRule; @@ -404,7 +404,7 @@ pub(crate) struct Environment, SC, pub(crate) voter_set_state: SharedVoterSetState, pub(crate) voting_rule: VR, pub(crate) metrics: Option, - pub(crate) justification_sender: Option>, + pub(crate) justification_sender: Option>, pub(crate) _phantom: PhantomData, } @@ -1083,7 +1083,7 @@ pub(crate) fn finalize_block( number: NumberFor, justification_or_commit: JustificationOrCommit, initial_sync: bool, - justification_sender: &Option>, + justification_sender: &Option>, ) -> Result<(), CommandOrError>> where Block: BlockT, BE: Backend, @@ -1191,17 +1191,10 @@ pub(crate) fn finalize_block( }; // Notify any registered listeners in case we have a justification - if let Some(ref justification) = justification { - match client.header(BlockId::Hash(hash)) { - Ok(Some(header)) => { - let notification = (header, justification.clone()); - if let Some(sender) = justification_sender { - let _ = sender.notify(notification); - } - }, - Ok(None) => debug!(target: "afg", "Expected a header for sending a justification notification."), - Err(err) => debug!(target: "afg", "Getting header failed: {}", err), - }; + if let Some(sender) = justification_sender { + if let Some(ref justification) = justification { + let _ = sender.notify(justification.clone()); + } } let justification = justification.map(|j| j.encode()); diff --git a/client/finality-grandpa/src/import.rs b/client/finality-grandpa/src/import.rs index e1bc0d0aa1002..c9f2d8d2f7bcc 100644 --- a/client/finality-grandpa/src/import.rs +++ b/client/finality-grandpa/src/import.rs @@ -44,7 +44,7 @@ use crate::authorities::{AuthoritySet, SharedAuthoritySet, DelayKind, PendingCha use crate::consensus_changes::SharedConsensusChanges; use crate::environment::finalize_block; use crate::justification::GrandpaJustification; -use crate::notification::GrandpaJustificationSubscribers; +use crate::notification::GrandpaJustificationSender; use std::marker::PhantomData; /// A block-import handler for GRANDPA. @@ -63,7 +63,7 @@ pub struct GrandpaBlockImport { send_voter_commands: TracingUnboundedSender>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: HashMap>>, - justification_sender: GrandpaJustificationSubscribers, + justification_sender: GrandpaJustificationSender, _phantom: PhantomData, } @@ -563,7 +563,7 @@ impl GrandpaBlockImport>>, consensus_changes: SharedConsensusChanges>, authority_set_hard_forks: Vec<(SetId, PendingChange>)>, - justification_sender: GrandpaJustificationSubscribers, + justification_sender: GrandpaJustificationSender, ) -> GrandpaBlockImport { // check for and apply any forced authority set hard fork that applies // to the *current* authority set. diff --git a/client/finality-grandpa/src/lib.rs b/client/finality-grandpa/src/lib.rs index 65786588559f0..a586dc8e31f6a 100644 --- a/client/finality-grandpa/src/lib.rs +++ b/client/finality-grandpa/src/lib.rs @@ -126,7 +126,7 @@ mod voting_rule; pub use authorities::SharedAuthoritySet; pub use finality_proof::{FinalityProofProvider, StorageAndProofProvider}; -pub use notification::{GrandpaJustificationSubscribers, GrandpaJustifications}; +pub use notification::{GrandpaJustificationSender, GrandpaJustificationStream}; pub use import::GrandpaBlockImport; pub use justification::GrandpaJustification; pub use light_import::{light_block_import, GrandpaLightBlockImport}; @@ -450,8 +450,8 @@ pub struct LinkHalf { select_chain: SC, persistent_data: PersistentData, voter_commands_rx: TracingUnboundedReceiver>>, - justification_sender: GrandpaJustificationSubscribers, - justification_receiver: GrandpaJustifications, + justification_sender: GrandpaJustificationSender, + justification_stream: GrandpaJustificationStream, } impl LinkHalf { @@ -460,14 +460,9 @@ impl LinkHalf { &self.persistent_data.authority_set } - /// Get the sender end of justification notifications. - pub fn justification_sender(&self) -> GrandpaJustificationSubscribers { - self.justification_sender.clone() - } - /// Get the receiving end of justification notifications. - pub fn justification_receiver(&self) -> GrandpaJustifications { - self.justification_receiver.clone() + pub fn justification_stream(&self) -> GrandpaJustificationStream { + self.justification_stream.clone() } } @@ -567,8 +562,8 @@ where let (voter_commands_tx, voter_commands_rx) = tracing_unbounded("mpsc_grandpa_voter_command"); - let (justification_sender, justification_receiver) = - GrandpaJustifications::channel(); + let (justification_sender, justification_stream) = + GrandpaJustificationStream::channel(); // create pending change objects with 0 delay and enacted on finality // (i.e. standard changes) for each authority set hard fork. @@ -604,7 +599,7 @@ where persistent_data, voter_commands_rx, justification_sender, - justification_receiver, + justification_stream, }, )) } @@ -740,7 +735,7 @@ pub fn run_grandpa_voter( persistent_data, voter_commands_rx, justification_sender, - justification_receiver: _, + justification_stream: _, } = link; let network = NetworkBridge::new( @@ -850,7 +845,7 @@ where voter_commands_rx: TracingUnboundedReceiver>>, prometheus_registry: Option, shared_voter_state: SharedVoterState, - justification_sender: GrandpaJustificationSubscribers, + justification_sender: GrandpaJustificationSender, ) -> Self { let metrics = match prometheus_registry.as_ref().map(Metrics::register) { Some(Ok(metrics)) => Some(metrics), diff --git a/client/finality-grandpa/src/notification.rs b/client/finality-grandpa/src/notification.rs index 7fde9b99e67bb..16f705f0eeb1f 100644 --- a/client/finality-grandpa/src/notification.rs +++ b/client/finality-grandpa/src/notification.rs @@ -24,32 +24,29 @@ use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedSender, TracingUnbounded use crate::justification::GrandpaJustification; -/// Justification for a finalized block. -type JustificationNotification = (::Header, GrandpaJustification); - // Stream of justifications returned when subscribing. -type JustificationStream = TracingUnboundedReceiver>; +type JustificationStream = TracingUnboundedReceiver>; // Sending endpoint for notifying about justifications. -type JustificationSubscriber = TracingUnboundedSender>; +type JustificationSender = TracingUnboundedSender>; // Collection of channel sending endpoints shared with the receiver side so they can register // themselves. -type SharedJustificationSubscribers = Arc>>>; +type SharedJustificationSenders = Arc>>>; /// The sending half of the Grandpa justification channel(s). /// -/// Used to send notifictions about justifications generated +/// Used to send notifications about justifications generated /// at the end of a Grandpa round. #[derive(Clone)] -pub struct GrandpaJustificationSubscribers { - subscribers: SharedJustificationSubscribers +pub struct GrandpaJustificationSender { + subscribers: SharedJustificationSenders } -impl GrandpaJustificationSubscribers { - /// The `notifiers` should be shared with a corresponding - /// `GrandpaJustificationReceiver`. - fn new(subscribers: SharedJustificationSubscribers) -> Self { +impl GrandpaJustificationSender { + /// The `subscribers` should be shared with a corresponding + /// `GrandpaJustificationStream`. + fn new(subscribers: SharedJustificationSenders) -> Self { Self { subscribers, } @@ -57,39 +54,39 @@ impl GrandpaJustificationSubscribers { /// Send out a notification to all subscribers that a new justification /// is available for a block. - pub fn notify(&self, notification: JustificationNotification) -> Result<(), ()> { - self.subscribers.lock() - .retain(|n| { - !n.is_closed() && n.unbounded_send(notification.clone()).is_ok() - }); - + pub fn notify(&self, notification: GrandpaJustification) -> Result<(), ()> { + self.subscribers.lock().retain(|n| { + !n.is_closed() && n.unbounded_send(notification.clone()).is_ok() + }); Ok(()) } } /// The receiving half of the Grandpa justification channel. /// -/// Used to recieve notifictions about justifications generated +/// Used to receive notifications about justifications generated /// at the end of a Grandpa round. +/// The `GrandpaJustificationStream` entity stores the `SharedJustificationSenders` +/// so it can be used to add more subscriptions. #[derive(Clone)] -pub struct GrandpaJustifications { - subscribers: SharedJustificationSubscribers +pub struct GrandpaJustificationStream { + subscribers: SharedJustificationSenders } -impl GrandpaJustifications { +impl GrandpaJustificationStream { /// Creates a new pair of receiver and sender of justification notifications. - pub fn channel() -> (GrandpaJustificationSubscribers, Self) { - let finality_notifiers = Arc::new(Mutex::new(vec![])); - let receiver = GrandpaJustifications::new(finality_notifiers.clone()); - let sender = GrandpaJustificationSubscribers::new(finality_notifiers.clone()); + pub fn channel() -> (GrandpaJustificationSender, Self) { + let subscribers = Arc::new(Mutex::new(vec![])); + let receiver = GrandpaJustificationStream::new(subscribers.clone()); + let sender = GrandpaJustificationSender::new(subscribers.clone()); (sender, receiver) } /// Create a new receiver of justification notifications. /// - /// The `notifiers` should be shared with a corresponding + /// The `subscribers` should be shared with a corresponding /// `GrandpaJustificationSender`. - fn new(subscribers: SharedJustificationSubscribers) -> Self { + fn new(subscribers: SharedJustificationSenders) -> Self { Self { subscribers, } diff --git a/client/finality-grandpa/src/observer.rs b/client/finality-grandpa/src/observer.rs index 670c3367ee94c..8fb536a369751 100644 --- a/client/finality-grandpa/src/observer.rs +++ b/client/finality-grandpa/src/observer.rs @@ -40,7 +40,7 @@ use crate::{ use crate::authorities::SharedAuthoritySet; use crate::communication::{Network as NetworkT, NetworkBridge}; use crate::consensus_changes::SharedConsensusChanges; -use crate::notification::GrandpaJustificationSubscribers; +use crate::notification::GrandpaJustificationSender; use sp_finality_grandpa::AuthorityId; use std::marker::{PhantomData, Unpin}; @@ -70,7 +70,7 @@ fn grandpa_observer( authority_set: &SharedAuthoritySet>, consensus_changes: &SharedConsensusChanges>, voters: &Arc>, - justification_sender: &Option>, + justification_sender: &Option>, last_finalized_number: NumberFor, commits: S, note_round: F, @@ -219,7 +219,7 @@ struct ObserverWork> { persistent_data: PersistentData, keystore: Option, voter_commands_rx: TracingUnboundedReceiver>>, - justification_sender: Option>, + justification_sender: Option>, _phantom: PhantomData, } @@ -237,7 +237,7 @@ where persistent_data: PersistentData, keystore: Option, voter_commands_rx: TracingUnboundedReceiver>>, - justification_sender: Option>, + justification_sender: Option>, ) -> Self { let mut work = ObserverWork { diff --git a/client/rpc/Cargo.toml b/client/rpc/Cargo.toml index 8bcfc2da05899..0adc2c6c07e4a 100644 --- a/client/rpc/Cargo.toml +++ b/client/rpc/Cargo.toml @@ -37,7 +37,7 @@ sp-transaction-pool = { version = "2.0.0-rc5", path = "../../primitives/transact sp-blockchain = { version = "2.0.0-rc5", path = "../../primitives/blockchain" } hash-db = { version = "0.15.2", default-features = false } parking_lot = "0.10.0" -lazy_static = "1.4.0" +lazy_static = { version = "1.4.0", optional = true } [dev-dependencies] assert_matches = "1.3.0" @@ -49,4 +49,4 @@ tokio = "0.1.22" sc-transaction-pool = { version = "2.0.0-rc5", path = "../transaction-pool" } [features] -test-helpers = [] +test-helpers = ["lazy_static"] From 452024f02802f1bfef24956f6cee8c805f24a65f Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Jon=20H=C3=A4ggblad?= Date: Tue, 4 Aug 2020 12:52:13 +0200 Subject: [PATCH 59/59] grandpa: remove unused serde dependency --- Cargo.lock | 1 - client/finality-grandpa/Cargo.toml | 1 - 2 files changed, 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 405a6c3829160..6963e426eff4d 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -6664,7 +6664,6 @@ dependencies = [ "sc-network-gossip", "sc-network-test", "sc-telemetry", - "serde", "serde_json", "sp-api", "sp-application-crypto", diff --git a/client/finality-grandpa/Cargo.toml b/client/finality-grandpa/Cargo.toml index 9c3008401ecd1..7b2e58b8be9a6 100644 --- a/client/finality-grandpa/Cargo.toml +++ b/client/finality-grandpa/Cargo.toml @@ -33,7 +33,6 @@ sp-core = { version = "2.0.0-rc5", path = "../../primitives/core" } sp-api = { version = "2.0.0-rc5", path = "../../primitives/api" } sc-telemetry = { version = "2.0.0-rc5", path = "../telemetry" } sc-keystore = { version = "2.0.0-rc5", path = "../keystore" } -serde = { version = "1.0", features = ["derive"] } serde_json = "1.0.41" sc-client-api = { version = "2.0.0-rc5", path = "../api" } sp-inherents = { version = "2.0.0-rc5", path = "../../primitives/inherents" }