Expose GRANDPA round state through RPC by octol · Pull Request #5375 · paritytech/substrate

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

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

-	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

 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

+ 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

+ 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

- 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

 	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

 >;

+// 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.

andresilva

just some minor nits but overall lgtm! :)

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

 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

 })?;

- (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.

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

 >;

+// 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

+ 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

 );

+ // 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

+
+ 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 <andre.beat@gmail.com> Co-authored-by: Benjamin Kampmann <ben@gnunicorn.org>

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

Comment threadbin/node/rpc/src/lib.rs Outdated

octol force-pushed the jon/issue-4921-expose-grandpa-round-state-try-shared-arc branch from 2426a3d to 58f52daCompare 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 cdd55feCompare April 7, 2020 11:03

octol force-pushed the jon/issue-4921-expose-grandpa-round-state-try-shared-arc branch from db2a02d to 83c82c9Compare 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 c6486d6Compare April 23, 2020 21:07

octol commented Apr 23, 2020
View reviewed changes

Comment threadclient/finality-grandpa/rpc/src/lib.rs Outdated

andresilva reviewed Apr 24, 2020
View reviewed changes

octoland 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 7 commits May 1, 2020 00:20

grandpa: remove VoterState error variant
12004bc

grandpa: enable missing futures compat feature
1ef01df

grandpa: fix typo in error variant
a97bb87

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:masterMay 4, 2020

andresilva mentioned this pull request May 4, 2020
Expose GRANDPA round state through RPC #4921
Closed

		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();

Uh oh!

Conversation

octol commented Mar 24, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

parity-cla-bot commented Mar 24, 2020

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octolMar 25, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

andresilva left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

octolApr 24, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

andresilva commented May 1, 2020

Uh oh!

octolMay 1, 2020• edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gavofyork commented May 1, 2020

Uh oh!

octol commented May 1, 2020

Uh oh!

andresilva commented May 1, 2020

Uh oh!

tomusdrw commented May 4, 2020

Uh oh!

octol commented Mar 24, 2020•
edited
Loading

octolMar 25, 2020•
edited
Loading

octolApr 24, 2020•
edited
Loading

octolMay 1, 2020•
edited
Loading

gnunicorn commented May 4, 2020•
edited
Loading