Skip to content
This repository was archived by the owner on Nov 15, 2023. It is now read-only.
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
Show all changes
75 commits
Select commit Hold shift + click to select a range
2f5367f
Rough skeleton for what I think the RPC should look like
HCastano Apr 8, 2020
9d369ac
Create channel for sending justifications
HCastano Apr 10, 2020
0095b78
WIP: Add subscribers for justifications to Grandpa
HCastano Apr 18, 2020
7719292
WIP: Add a struct for managing subscriptions
HCastano Apr 21, 2020
8c35b1e
Make naming more clear and lock data in Arc
HCastano Apr 21, 2020
07ef5fa
Rough idea of what RPC would look like
HCastano Apr 21, 2020
c9aa9e2
Remove code from previous approach
HCastano Apr 22, 2020
78250b7
Missed some things
HCastano Apr 22, 2020
fe31500
Update client/rpc-api/src/chain/mod.rs
HCastano Apr 22, 2020
ba13ee6
Update client/rpc-api/src/chain/mod.rs
HCastano Apr 22, 2020
fe360b9
Split justification subscription into sender and receiver halves
HCastano Apr 26, 2020
254bebe
Replace RwLock with a Mutex
HCastano Apr 26, 2020
9c996ea
Add sample usage from the Service's point of view
HCastano Apr 26, 2020
8090161
Merge branch 'master' into hc-add-subscription-rpc-for-finality
HCastano Apr 29, 2020
1150e5d
Merge branch 'master' into hc-add-subscription-rpc-for-finality
HCastano May 7, 2020
130a871
Merge branch 'master' into hc-add-subscription-rpc-for-finality
HCastano May 15, 2020
f42b6c3
Remove code that referred to "chain_" RPC
HCastano May 15, 2020
d61d7ec
Use the Justification sender/receivers from Grandpa LinkHalf
HCastano May 17, 2020
56e716b
Add some PubSub boilerplate
HCastano May 18, 2020
c0c6508
Merge branch 'master' into hc-add-subscription-rpc-for-finality
HCastano May 18, 2020
1bc9103
Merge branch 'master' into hc-add-subscription-rpc-for-finality
HCastano Jun 1, 2020
30e3831
Merge branch 'master' into hc-add-subscription-rpc-for-finality
HCastano Jun 3, 2020
8b0850a
Add guiding comments
HCastano Jun 3, 2020
cf07f2f
TMP: comment out to fix compilation
octol Jun 8, 2020
ab64ecb
Merge branch 'master' into hc-add-subscription-rpc-for-finality
octol Jun 9, 2020
b1c04ba
Return MetaIoHandler from PubSubHandler in create_full
octol Jun 10, 2020
ebad4d7
Uncomment pubsub methods in rpc handler (fails to build)
octol Jun 10, 2020
caaaa61
node/rpc: make Metadata concrete in create_full to fix compilation
octol Jun 11, 2020
f3cc272
node: pass in SubscriptionManger to grandpa rpc handler
octol Jun 11, 2020
3c4505a
grandpa-rpc: use SubscriptionManger to add subscriber
octol Jun 12, 2020
9a225ac
grandpa-rpc: attempt at setting up the justification stream (fails to…
octol Jun 16, 2020
a85dd53
grandpa-rpc: fix compilation of connecting stream to sink
octol Jun 16, 2020
1b0344f
grandpa-rpc: implement unsubscribe
octol Jun 16, 2020
12dd5df
grandpa-rpc: update older tests
octol Jun 18, 2020
bca6a13
grandpa-rpc: add full prefix to avoid confusing rust-analyzer
octol Jun 22, 2020
efaa0d1
grandpa-rpc: add test for pubsub not available
octol Jun 22, 2020
9632988
grandpa-rpc: tidy up leftover code
octol Jun 22, 2020
964f1e3
grandpa-rpc: add test for sub and unsub of justifications
octol Jun 22, 2020
82004cb
grandpa-rpc: minor stylistic changes
octol Jun 23, 2020
e526f95
grandpa-rpc: split unit test
octol Jun 23, 2020
6c71b8f
grandpa-rpc: minor stylistic changes in test
octol Jun 23, 2020
8bfb560
grandpa-rpc: skip returning future when cancelling
octol Jun 25, 2020
be7cb78
grandpa-rpc: reuse testing executor from sc-rpc
octol Jun 25, 2020
ac81cdf
grandpa-rpc: don't need to use PubSubHandler in tests
octol Jun 25, 2020
3f8630d
node-rpc: use MetaIoHandler rather than PubSubHandler
octol Jun 26, 2020
3b387e6
grandpa: log if getting header failed
octol Jun 26, 2020
bf4b19a
grandpa: move justification channel creation into factory function
octol Jun 26, 2020
4c96b73
grandpa: make the justification sender optional
octol Jun 26, 2020
12158a2
grandpa: fix compilation warnings
octol Jun 27, 2020
e2584e5
grandpa: move justification notification types to new file
octol Jun 27, 2020
dc3c888
grandpa-rpc: move JustificationNotification to grandpa-rpc
octol Jun 27, 2020
410af1f
grandpa-rpc: move JustificationNotification to its own file
octol Jun 27, 2020
fb3ad63
grandpa: rename justification channel pairs
octol Jun 27, 2020
5d80399
grandpa: rename notifier types
octol Jun 27, 2020
3f33fd2
grandpa: pass justification as GrandpaJustification to the rpc module
octol Jun 29, 2020
57f1963
Merge branch 'master' into hc-add-subscription-rpc-for-finality
octol Jun 29, 2020
90ae56e
Move Metadata to sc-rpc-api
tomusdrw Jun 30, 2020
be1df4f
grandpa-rpc: remove unsed error code
octol Jun 29, 2020
d4cddf3
grandpa: fix bug for checking if channel is closed before sendind
octol Jul 1, 2020
8bd9e5c
grandpa-rpc: unit test for sending justifications
octol Jun 30, 2020
127c19e
Merge branch 'master' into hc-add-subscription-rpc-for-finality
octol Jul 2, 2020
f43ee9b
grandpa-rpc: update comments for the pubsub test
octol Jul 2, 2020
39640c1
grandpa-rpc: update pubsub tests with more steps
octol Jul 2, 2020
f3c6c54
grandpa-rpc: fix pubsub test
octol Jul 2, 2020
44ba73e
grandpa-rpc: minor indendation
octol Jul 3, 2020
01f7d73
Merge branch 'master' into hc-add-subscription-rpc-for-finality
octol Jul 22, 2020
3f3b136
grandpa-rpc: decode instead of encode in test
octol Jul 23, 2020
908cf15
Merge branch 'master' into hc-add-subscription-rpc-for-finality
octol Jul 27, 2020
bb9ad4b
Merge branch 'master' into hc-add-subscription-rpc-for-finality
octol Jul 27, 2020
410ddea
grandpa: fix review comments
octol Jul 31, 2020
ca126fd
Merge branch 'master' into hc-add-subscription-rpc-for-finality
octol Aug 4, 2020
452024f
grandpa: remove unused serde dependency
octol Aug 4, 2020
8b0cf76
Merge branch 'master' into hc-add-subscription-rpc-for-finality
octol Aug 5, 2020
7ddffef
Merge branch 'master' into hc-add-subscription-rpc-for-finality
octol Aug 6, 2020
817b425
Merge remote-tracking branch 'upstream/master' into hc-add-subscripti…
octol Aug 7, 2020
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
8 changes: 4 additions & 4 deletions Cargo.lock

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

17 changes: 14 additions & 3 deletions bin/node/cli/src/service.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -49,6 +54,7 @@ macro_rules! new_full_start {
type RpcExtension = jsonrpc_core::IoHandler<sc_rpc::Metadata>;
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
Expand Down Expand Up @@ -89,6 +95,10 @@ macro_rules! new_full_start {
Ok(import_queue)
})?
.with_rpc_extensions(|builder| -> std::result::Result<RpcExtension, _> {

// 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 {
Expand All @@ -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)
}}
}

Expand All @@ -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| {
Expand Down Expand Up @@ -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: Some(finality_proof_subscription),
};

// the GRANDPA voter task is considered infallible, i.e.
Expand Down
24 changes: 23 additions & 1 deletion client/finality-grandpa/src/environment.rs
Original file line number Diff line number Diff line change
Expand Up @@ -51,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;
Expand Down Expand Up @@ -387,7 +388,6 @@ impl Metrics {
}
}


/// The environment we run GRANDPA in.
pub(crate) struct Environment<Backend, Block: BlockT, C, N: NetworkT<Block>, SC, VR> {
pub(crate) client: Arc<C>,
Expand All @@ -401,6 +401,7 @@ pub(crate) struct Environment<Backend, Block: BlockT, C, N: NetworkT<Block>, SC,
pub(crate) voter_set_state: SharedVoterSetState<Block>,
pub(crate) voting_rule: VR,
pub(crate) metrics: Option<Metrics>,
pub(crate) finality_notifiers: Option<SharedFinalityNotifiers<Block>>,
pub(crate) _phantom: PhantomData<Backend>,
}

Expand Down Expand Up @@ -912,6 +913,7 @@ where
number,
(round, commit).into(),
false,
self.finality_notifiers.clone(),
)
}

Expand Down Expand Up @@ -972,6 +974,7 @@ pub(crate) fn finalize_block<BE, Block, Client>(
number: NumberFor<Block>,
justification_or_commit: JustificationOrCommit<Block>,
initial_sync: bool,
finality_notifiers: Option<SharedFinalityNotifiers<Block>>,
) -> Result<(), CommandOrError<Block::Hash, NumberFor<Block>>> where
Block: BlockT,
BE: Backend<Block>,
Expand All @@ -983,6 +986,7 @@ pub(crate) fn finalize_block<BE, Block, Client>(
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
Expand Down Expand Up @@ -1077,6 +1081,24 @@ pub(crate) fn finalize_block<BE, Block, Client>(
},
};

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.clone() {
let header = client.header(BlockId::Hash(hash))?
.expect("");
let notification = JustificationNotification {
header,
justification,
};

for s in notifiers.read().iter() {
// TODO: Deal with Result
s.unbounded_send(notification.clone());
}
}
}

debug!(target: "afg", "Finalizing blocks up to ({:?}, {})", number, hash);

// ideally some handle to a synchronization oracle would be used
Expand Down
51 changes: 51 additions & 0 deletions client/finality-grandpa/src/finality_proof.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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::{
Expand All @@ -52,6 +53,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;

Expand Down Expand Up @@ -145,6 +147,55 @@ impl<Block: BlockT> AuthoritySetForFinalityChecker<Block> for Arc<dyn FetchCheck
}
}


/// Justification for a finalized block.
#[derive(Clone)]
pub struct JustificationNotification<Block: BlockT> {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO, this type should be defined in the rpc module. We can just use (Block::Header, Justification) in this crate. Also not sure if we need the header here or if just the block hash is enough. The fianlized block number and block hash is already defined in the justification (although for convenience I guess it might be useful to have it on a top-level like this).

/// Highest finalized block header
pub header: Block::Header,
/// An encoded justification proving that the given header has been finalized
pub justification: Vec<u8>,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't there a typedef for that?
Since we implement Serialize/Deserialize we should at least encode this as bytes, not a vector of numbers (default). Please add Serialization/deserialization test so that we can see how it's represented as string.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be using the proper justification type https://github.com/paritytech/substrate/blob/master/client/finality-grandpa/src/justification.rs#L41 and not an encoded blob.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've changed this to use GrandpaJustification and not encoding it in finalize_block so that we don't use the encoded blob.
What's the best way to deal with the serialization that happens in the rpc handler? Should we encode + serialize there?

}

type JustificationStream<Block> = TracingUnboundedReceiver<JustificationNotification<Block>>;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think all the changes in this file should be moved to a new file (they don't relate to finality proofs at all). Maybe notification.rs?

pub type SharedFinalitySubscribers<T> = Arc<RwLock<Vec<JustificationStream<T>>>>;

type FinalityNotifier<T> = TracingUnboundedSender<JustificationNotification<T>>;
pub type SharedFinalityNotifiers<T> = Arc<RwLock<Vec<FinalityNotifier<T>>>>;

pub struct FinalityProofSubscription<Block: BlockT> {
notifiers: SharedFinalityNotifiers<Block>,
}

impl<Block: BlockT> FinalityProofSubscription<Block> {
pub fn new() -> Self {
Self {
notifiers: Arc::new(RwLock::new(vec![])),
}
}

pub fn notifiers(&self) -> SharedFinalityNotifiers<Block> {
self.notifiers.clone()
}

// Will notify subsribers (receivers)
pub fn notify(&self, notification: JustificationNotification<Block>) {
// TODO: Look at `notify_justified()` in `client`
for s in self.notifiers.read().iter() {
s.unbounded_send(notification.clone());
}

todo!()
}

pub fn stream(&self) -> JustificationStream<Block> {
// 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
}
}

/// Finality proof provider for serving network requests.
pub struct FinalityProofProvider<B, Block: BlockT> {
backend: Arc<B>,
Expand Down
8 changes: 7 additions & 1 deletion client/finality-grandpa/src/import.rs
Original file line number Diff line number Diff line change
Expand Up @@ -37,11 +37,12 @@ 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;
use crate::justification::GrandpaJustification;
use crate::finality_proof::SharedFinalityNotifiers;
use std::marker::PhantomData;

/// A block-import handler for GRANDPA.
Expand All @@ -60,6 +61,7 @@ pub struct GrandpaBlockImport<Backend, Block: BlockT, Client, SC> {
send_voter_commands: TracingUnboundedSender<VoterCommand<Block::Hash, NumberFor<Block>>>,
consensus_changes: SharedConsensusChanges<Block::Hash, NumberFor<Block>>,
authority_set_hard_forks: HashMap<Block::Hash, PendingChange<Block::Hash, NumberFor<Block>>>,
finality_notifiers: Option<SharedFinalityNotifiers<Block>>,
_phantom: PhantomData<Backend>,
}

Expand All @@ -74,6 +76,7 @@ impl<Backend, Block: BlockT, Client, SC: Clone> 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(),
_phantom: PhantomData,
}
}
Expand Down Expand Up @@ -558,6 +561,7 @@ impl<Backend, Block: BlockT, Client, SC> GrandpaBlockImport<Backend, Block, Clie
send_voter_commands: TracingUnboundedSender<VoterCommand<Block::Hash, NumberFor<Block>>>,
consensus_changes: SharedConsensusChanges<Block::Hash, NumberFor<Block>>,
authority_set_hard_forks: Vec<(SetId, PendingChange<Block::Hash, NumberFor<Block>>)>,
finality_notifiers: Option<SharedFinalityNotifiers<Block>>,
) -> GrandpaBlockImport<Backend, Block, Client, SC> {
// check for and apply any forced authority set hard fork that applies
// to the *current* authority set.
Expand Down Expand Up @@ -601,6 +605,7 @@ impl<Backend, Block: BlockT, Client, SC> GrandpaBlockImport<Backend, Block, Clie
send_voter_commands,
consensus_changes,
authority_set_hard_forks,
finality_notifiers,
_phantom: PhantomData,
}
}
Expand Down Expand Up @@ -646,6 +651,7 @@ where
number,
justification.into(),
initial_sync,
self.finality_notifiers.clone(),
);

match result {
Expand Down
17 changes: 16 additions & 1 deletion client/finality-grandpa/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -69,7 +69,7 @@ use sc_keystore::KeyStorePtr;
use sp_inherents::InherentDataProviders;
use sp_consensus::{SelectChain, BlockImport};
use sp_core::Pair;
use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver};
use sp_utils::mpsc::{tracing_unbounded, TracingUnboundedReceiver, TracingUnboundedSender};
use sc_telemetry::{telemetry, CONSENSUS_INFO, CONSENSUS_DEBUG};
use serde_json;

Expand Down Expand Up @@ -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, SharedFinalityNotifiers};

// Re-export these two because it's just so damn convenient.
pub use sp_finality_grandpa::{AuthorityId, ScheduledChange};
Expand Down Expand Up @@ -521,6 +522,7 @@ 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
),
LinkHalf {
client,
Expand Down Expand Up @@ -620,6 +622,8 @@ pub struct GrandpaParams<Block: BlockT, C, N, SC, VR> {
pub voting_rule: VR,
/// The prometheus metrics registry.
pub prometheus_registry: Option<prometheus_endpoint::Registry>,
/// A subscription to new block justifications
pub finality_subscription: Option<FinalityProofSubscription<Block>>,
}

/// Run a GRANDPA voter as a task. Provide configuration and a link to a
Expand All @@ -644,6 +648,7 @@ pub fn run_grandpa_voter<Block: BlockT, BE: 'static, C, N, SC, VR>(
telemetry_on_connect,
voting_rule,
prometheus_registry,
finality_subscription,
} = grandpa_params;

// NOTE: we have recently removed `run_grandpa_observer` from the public
Expand Down Expand Up @@ -695,6 +700,12 @@ pub fn run_grandpa_voter<Block: BlockT, BE: 'static, C, N, SC, VR>(
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,
Expand All @@ -704,6 +715,7 @@ pub fn run_grandpa_voter<Block: BlockT, BE: 'static, C, N, SC, VR>(
persistent_data,
voter_commands_rx,
prometheus_registry,
finality_notifiers,
);

let voter_work = voter_work
Expand Down Expand Up @@ -761,6 +773,7 @@ where
persistent_data: PersistentData<Block>,
voter_commands_rx: TracingUnboundedReceiver<VoterCommand<Block::Hash, NumberFor<Block>>>,
prometheus_registry: Option<prometheus_endpoint::Registry>,
finality_notifiers: Option<SharedFinalityNotifiers<Block>>,
) -> Self {
let metrics = match prometheus_registry.as_ref().map(Metrics::register) {
Some(Ok(metrics)) => Some(metrics),
Expand All @@ -784,6 +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_notifiers,
_phantom: PhantomData,
});

Expand Down Expand Up @@ -907,6 +921,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(),
_phantom: PhantomData,
});

Expand Down
1 change: 1 addition & 0 deletions client/finality-grandpa/src/observer.rs
Original file line number Diff line number Diff line change
Expand Up @@ -125,6 +125,7 @@ fn grandpa_observer<BE, Block: BlockT, Client, S, F>(
finalized_number,
(round, commit).into(),
false,
None, // TODO: Should I include the finality_subscribers here?
) {
Ok(_) => {},
Err(e) => return future::err(e),
Expand Down
Loading