Expose GRANDPA round state through RPC #5375

octol · 2020-03-24T10:40:23Z

Implements #4921

Depends on paritytech/finality-grandpa#114

Left to do

JSON RPC error code?
Convert SharedVoterState to struct
Reintroduce the test stub
~~Add to light client~~
Move serialized structs to separate file
Can we use u32 for serialized fields?

polkadot companion: paritytech/polkadot#1040

parity-cla-bot · 2020-03-24T10:40:27Z

It looks like @octol signed our Contributor License Agreement. 👍

Many thanks,

Parity Technologies CLA Bot

bin/node/rpc/src/lib.rs

andresilva · 2020-03-25T12:38:18Z

client/finality-grandpa/src/environment.rs

-	pub(crate) voter_set_state: SharedVoterSetState<Block>,
-	pub(crate) voting_rule: VR,
+pub struct Environment<B, E, Block: BlockT, N: NetworkT<Block>, RA, SC, VR> {
+	pub client: Arc<Client<B, E, Block, RA>>,


Do we need to make the fields public? We only care about exporting the Environment type. If the fields are not public I assume that a lot less types would have to be exported.

Yeah I have some hope to be able to roll back some of these pub changes once it's all up and working

octol · 2020-03-25T21:21:50Z

bin/node/cli/src/service.rs

@@ -54,6 +55,8 @@ 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();
+		// WIP: sort out construction
+		let shared_voter_state = Arc::new(RwLock::new(None));


This part here, how to construct when we don't yet have Environment available. Any ideas on the best approach @seunlanlege ?

I have no idea what VoterState is.

tried checking out this PR, but it looks like you're using a custom finality-grandpa crate. Try using a git dependency.

Yeah that's right, it depends on paritytech/finality-grandpa#114

Updated to use git dependency pointing to my fork on github

This object will always be constructed by the Voter in finality-grandpa. You just keep it uninitialized here.

andresilva

I didn't expect the VoterSet changes to cause so much disruption here and I think they should be handled properly before we migrate to grandpa 0.12. So in order to not block this PR on that let's target grandpa 0.11 here. I can help with rebasing this if you need.
Most of your WIP comments that aren't related to 0.12 changes seem sensible to me and should be done 👍

andresilva · 2020-04-20T16:31:46Z

bin/node/cli/src/service.rs

+		let shared_voter_state: grandpa::SharedVoterState<grandpa::AuthorityId>
+			= Arc::new(parking_lot::RwLock::new(None));


Suggested change

let shared_voter_state: grandpa::SharedVoterState<grandpa::AuthorityId>

= Arc::new(parking_lot::RwLock::new(None));

let shared_voter_state: grandpa::SharedVoterState<grandpa::AuthorityId> =

Arc::new(parking_lot::RwLock::new(None));

Is the type still needed?

Nope not anymore!

andresilva · 2020-04-20T16:38:33Z

client/finality-grandpa/rpc/src/lib.rs

+        jsonrpc_core::Error {
+            message: format!("{}", error).into(),
+            // WIP: what error code should we use?
+            code: jsonrpc_core::ErrorCode::ServerError(1234),


1? Put it in a const named NOT_READY?

andresilva · 2020-04-20T16:45:43Z

client/finality-grandpa/src/authorities.rs

-		self.inner.read().current_authorities.iter().cloned().collect()
+	pub fn current_authorities(&self) -> VoterSet<AuthorityId> {
+		// WIP: unwrap
+		VoterSet::new(self.inner.read().current_authorities.iter().cloned()).unwrap()


For now let's add an expect here with a message saying that authorities should not be 0. Let's deal with this in a follow-up PR.

andresilva · 2020-04-20T16:48:43Z

client/finality-grandpa/src/lib.rs

@@ -620,6 +632,9 @@ pub struct GrandpaParams<Block: BlockT, C, N, SC, VR> {
 	pub voting_rule: VR,
 	/// The prometheus metrics registry.
 	pub prometheus_registry: Option<prometheus_endpoint::Registry>,
+	/// The voter state is exposed at an RPC endpoint.
+	// WIP: should we use Environment::Id istead of AuthorityId?
+	pub shared_voter_state: SharedVoterState<AuthorityId>,


This isn't typed by Environment, it's okay to keep the id fixed.

andresilva · 2020-04-20T16:50:09Z

client/finality-grandpa/src/lib.rs

@@ -203,6 +206,9 @@ type CommunicationOutH<Block, H> = finality_grandpa::voter::CommunicationOut<
 	AuthorityId,
 >;

+// WIP: convert to struct
+pub type SharedVoterState<Id> = Arc<RwLock<Option<Box<dyn voter::VoterState<Id> + Sync + Send>>>>;


This shouldn't be typed by Id and instead should fix the VoterState to AuthorityId.

client/finality-grandpa/rpc/src/lib.rs

andresilva

just some minor nits but overall lgtm! :)

andresilva · 2020-04-24T11:33:04Z

bin/node/cli/src/service.rs

@@ -100,12 +104,16 @@ 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 {
+						shared_voter_state: Arc::clone(&shared_voter_state),


Suggested change

shared_voter_state: Arc::clone(&shared_voter_state),

shared_voter_state: shared_voter_state.clone(),

Interesting, hadn't spotted that it wasn't the recommended style anymore

andresilva · 2020-04-24T11:37:22Z

bin/node/cli/src/service.rs

 					}
 				};
 				Ok(node_rpc::create_full(deps))
 			})?;

-		(builder, import_setup, inherent_data_providers)
+		(builder, import_setup, inherent_data_providers, shared_voter_state)


Do you mind instead creating an let rpc_setup = Some((shared_voter_state)); and returning that instead? Similar to what we do for the import_setup. It's futile right now as we only return one thing from the RPC setup but will make it easier if we need to add anything else in the future.

client/finality-grandpa/rpc/src/lib.rs

andresilva · 2020-04-24T11:43:27Z

client/finality-grandpa/src/lib.rs

@@ -203,6 +206,9 @@ type CommunicationOutH<Block, H> = finality_grandpa::voter::CommunicationOut<
 	AuthorityId,
 >;

+// WIP: convert to struct
+pub type SharedVoterState = Arc<RwLock<Option<Box<dyn voter::VoterState<AuthorityId> + Sync + Send>>>>;


Yeah would be nice mainly to avoid exposing the lock, i.e. it would only expose a getter method to get the data we need. Not a blocker though.

andresilva · 2020-04-24T11:45:04Z

client/finality-grandpa/rpc/src/lib.rs

+        let future = async move { round_states }.boxed();
+        Box::new(future.map_err(jsonrpc_core::Error::from).compat())
+    }
+}


Initially you had a test stub here and I think it was nice test to have.

I second that.

andresilva · 2020-04-24T11:50:16Z

client/finality-grandpa/src/lib.rs

@@ -858,6 +877,10 @@ where
 					last_finalized,
 				);

+				// Repoint shared_voter_state so that the RPC endpoint can query the state
+				let mut shared_voter_state = self.shared_voter_state.write();


Just to be safe, could you replace this .write() with .try_write_for(Duration::from_secs(1))? Just to be safe, in case there's a deadlock we won't block the voter.

Yes that makes sense!

andresilva · 2020-05-01T01:44:59Z

Fixed a bunch of nits, made the RPC handler testable and wrote tests. This currently depends on paritytech/finality-grandpa#118 being merged and v0.12.1 published. @tomusdrw do you mind giving this another look?

octol · 2020-05-01T09:51:33Z

client/finality-grandpa/rpc/src/report.rs

+
+		let voter_state = voter_state.get().ok_or(Error::EndpointNotReady)?;
+
+		let (set_id, current_voters) = authority_set.get();


~~The first set_id is shadowed and unused?~~

EDIT: my bad, it's not!

gavofyork · 2020-05-01T12:18:15Z

doesn't build?

octol · 2020-05-01T12:38:02Z

doesn't build?

Waiting for a new release of finality-grandpa containing this: paritytech/finality-grandpa#118

andresilva · 2020-05-01T14:03:54Z

Should build and pass tests now (if it doesn't then I did something wrong :)

tomusdrw · 2020-05-04T08:30:24Z

This one looks good, but polkadot companion PR seems to have build failures.

andresilva · 2020-05-04T10:34:24Z

@tomusdrw Not sure if there were any previous errors and/or you restarted the build, polkadot companion is green here atm. The PR on polkadot itself will be green once this is merged and substrate is updated.

gnunicorn · 2020-05-04T12:08:55Z

Updated the companion PR, @andresilva could you take another look here and there? (companion fails because I already updated the companion PR, but the reference is missing. However, it had passed prior.)

* Companion PR for Substrate paritytech#5375 * fix compilation * Update rpc/Cargo.toml * update substrate Co-authored-by: André Silva <[email protected]> Co-authored-by: Benjamin Kampmann <[email protected]>

octol added the A3-in_progress Pull request is in progress. No review needed at this stage. label Mar 24, 2020

paritytech-ci added the B2-breaksapi label Mar 24, 2020

seunlanlege reviewed Mar 25, 2020

View reviewed changes

bin/node/rpc/src/lib.rs Outdated Show resolved Hide resolved

octol force-pushed the jon/issue-4921-expose-grandpa-round-state-try-shared-arc branch from 2426a3d to 58f52da Compare March 25, 2020 11:08

andresilva reviewed Mar 25, 2020

View reviewed changes

octol commented Mar 25, 2020

View reviewed changes

octol force-pushed the jon/issue-4921-expose-grandpa-round-state-try-shared-arc branch 2 times, most recently from 572ce7c to cdd55fe Compare April 7, 2020 11:03

octol force-pushed the jon/issue-4921-expose-grandpa-round-state-try-shared-arc branch from db2a02d to 83c82c9 Compare April 15, 2020 20:06

octol mentioned this pull request Apr 17, 2020

Expose voter state paritytech/finality-grandpa#114

Merged

1 task

andresilva reviewed Apr 20, 2020

View reviewed changes

HCastano mentioned this pull request Apr 22, 2020

Add Subscription RPC for Grandpa Finality #5732

Merged

octol force-pushed the jon/issue-4921-expose-grandpa-round-state-try-shared-arc branch from 6464139 to c6486d6 Compare April 23, 2020 21:07

octol commented Apr 23, 2020

View reviewed changes

client/finality-grandpa/rpc/src/lib.rs Outdated Show resolved Hide resolved

andresilva reviewed Apr 24, 2020

View reviewed changes

octol and others added 13 commits April 27, 2020 11:34

grandpa: wire up basic RPC call

5f6bb70

grandpa: make it compile against GRANDPA with expose round state

4ef39fa

grandpa: use shared voter state to expose RPC endpoint

eea294d

grandpa: restructure into nested structs

a8109a2

grandpa: return background rounds too

04fca7e

grandpa: return error when endpoint not ready

627c7a8

grandpa: collect grandpa rpc deps

123e249

grandpa: decide to use concrete AuthorityId in finality-grandpa-rpc

10b3266

grandpa: remove unncessary type annotation

eb5d87d

grandpa: move error code to const

6edfa13

grandpa: remove unnecessary WIP comment

bcb206d

grandpa: remove Id type parameter for SharedVoterState

4ca212d

grandpa: update tests to add shared_voter_state in parameters

b2af4f7

andresilva added 4 commits May 1, 2020 01:03

grandpa: remove test_utils

7aed9bb

grandpa: allow mocking rpc handler components

65f8674

grandpa: rename serialized to report in rpc module

a3e477d

grandpa: add proper test for RPC

23536a0

octol commented May 1, 2020

View reviewed changes

grandpa: update to finality-grandpa v0.12.1

aabacb8

tomusdrw approved these changes May 4, 2020

View reviewed changes

tomusdrw added A8-mergeoncegreen and removed A6-mustntgrumble labels May 4, 2020

gnunicorn added A8-mergeoncegreen and removed A7-needspolkadotpr labels May 4, 2020

gnunicorn requested a review from andresilva May 4, 2020 12:08

gnunicorn added A0-please_review Pull request needs code review. and removed A8-mergeoncegreen labels May 4, 2020

andresilva approved these changes May 4, 2020

View reviewed changes

andresilva merged commit c0ccc24 into paritytech:master May 4, 2020

andresilva mentioned this pull request May 4, 2020

Expose GRANDPA round state through RPC #4921

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose GRANDPA round state through RPC #5375

Expose GRANDPA round state through RPC #5375

octol commented Mar 24, 2020 •

edited

Loading

parity-cla-bot commented Mar 24, 2020

andresilva Mar 25, 2020

octol Mar 25, 2020

octol Mar 25, 2020 •

edited

Loading

seunlanlege Mar 26, 2020

seunlanlege Mar 26, 2020

octol Mar 26, 2020

octol Mar 26, 2020

andresilva Mar 26, 2020

andresilva left a comment

andresilva Apr 20, 2020

andresilva Apr 20, 2020

octol Apr 23, 2020

andresilva Apr 20, 2020

andresilva Apr 20, 2020

andresilva Apr 20, 2020

andresilva Apr 20, 2020

andresilva left a comment

andresilva Apr 24, 2020

octol Apr 24, 2020 •

edited

Loading

andresilva Apr 24, 2020

andresilva Apr 24, 2020

andresilva Apr 24, 2020

tomusdrw Apr 28, 2020

andresilva Apr 24, 2020

octol Apr 24, 2020

andresilva commented May 1, 2020

octol May 1, 2020 •

edited

Loading

gavofyork commented May 1, 2020

octol commented May 1, 2020

andresilva commented May 1, 2020

tomusdrw commented May 4, 2020

andresilva commented May 4, 2020

gnunicorn commented May 4, 2020 •

edited

Loading

		let shared_voter_state: grandpa::SharedVoterState<grandpa::AuthorityId>
		= Arc::new(parking_lot::RwLock::new(None));

	shared_voter_state: Arc::clone(&shared_voter_state),
	shared_voter_state: shared_voter_state.clone(),


		let voter_state = voter_state.get().ok_or(Error::EndpointNotReady)?;

		let (set_id, current_voters) = authority_set.get();

Expose GRANDPA round state through RPC #5375

Expose GRANDPA round state through RPC #5375

Conversation

octol commented Mar 24, 2020 • edited Loading

parity-cla-bot commented Mar 24, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octol Mar 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

octol Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

andresilva commented May 1, 2020

octol May 1, 2020 • edited Loading

Choose a reason for hiding this comment

gavofyork commented May 1, 2020

octol commented May 1, 2020

andresilva commented May 1, 2020

tomusdrw commented May 4, 2020

andresilva commented May 4, 2020

gnunicorn commented May 4, 2020 • edited Loading

octol commented Mar 24, 2020 •

edited

Loading

octol Mar 25, 2020 •

edited

Loading

octol Apr 24, 2020 •

edited

Loading

octol May 1, 2020 •

edited

Loading

gnunicorn commented May 4, 2020 •

edited

Loading