Expose voter state #114

octol · 2020-03-24T09:52:01Z

Implement #55 and support paritytech/substrate#4921 / paritytech/substrate#5375

Expose voter state to the outside world. Useful for querying diagnostics

Left to do:

Populate background_rounds https://github.com/paritytech/finality-grandpa/pull/114/files#diff-7dbd927baa51274d73fc2a7cb663e166R484

parity-cla-bot · 2020-03-24T09:52:04Z

It looks like @octol hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

octol · 2020-03-24T09:54:03Z

It looks like @octol hasn't signed our Contributor License Agreement, yet.

The purpose of a CLA is to ensure that the guardian of a project's outputs has the necessary ownership or grants of rights over all contributions to allow them to distribute under the chosen licence.
Wikipedia

You can read and sign our full Contributor License Agreement at the following URL: https://cla.parity.io

Once you've signed, please reply to this thread with [clabot:check] to prove it.

Many thanks,

Parity Technologies CLA Bot

[clabot:check]

parity-cla-bot · 2020-03-24T09:54:06Z

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

Many thanks,

Parity Technologies CLA Bot

andresilva

overall lgtm. could you try adding a test where we spin up a set of voters and check the voter state afterwards? have a look at this test: https://github.com/paritytech/finality-grandpa/blob/master/src/voter/mod.rs#L948

andresilva · 2020-04-17T19:07:26Z

src/voter/past_rounds.rs

@@ -286,6 +290,13 @@ impl<H, N, E: Environment<H, N>> PastRounds<H, N, E> where
 		}
 	}

+	pub(super) fn voting_rounds(&self) -> impl Iterator<Item = &VotingRound<H, N, E>> {


andresilva · 2020-04-17T19:08:05Z

src/voter/mod.rs

@@ -420,6 +423,76 @@ fn instantiate_last_round<H, N, E: Environment<H, N>>(
 	}
 }

+pub trait VoterState<Id> {


These and the types below need docs :)

Good catch!

andresilva · 2020-04-17T19:08:35Z

src/testing.rs

@@ -198,11 +199,19 @@ pub mod environment {
 	}

 	impl crate::voter::Environment<&'static str, u32> for Environment {
-		type Timer = BoxFuture<'static, Result<(),Error>>;
+		type Timer = Pin<Box<dyn Future<Output = Result<(), Error>> + Unpin + Send + Sync>>;


I need to double check why we need to pin this and the stream/sink below.

Removed in latest

octol · 2020-04-22T13:41:21Z

I updated the tests so that now the check for background_rounds should be reliable

andresilva

lgtm. @rphmeier do you want to give this a look?

andresilva · 2020-04-24T11:56:00Z

src/voter/mod.rs

+				   inner.best_round.round_number(),
+				   inner.best_round.round_number() + 1,


Suggested change

inner.best_round.round_number(),

inner.best_round.round_number() + 1,

inner.best_round.round_number(),

inner.best_round.round_number() + 1,

src/voter/mod.rs

rphmeier · 2020-04-24T18:20:10Z

src/voter/mod.rs

+	#[cfg_attr(test, derive(Debug))]
+	pub struct RoundState<Id: Eq + std::hash::Hash> {
+		pub total_weight: VoterWeight,
+		pub threshold_weight: VoterWeight,


missing docs on these fields, VoterState fields, and the mod report, and fn voter_state. There are even more missing docs - remember the guideline that all public items should be documented.

(yes, I know it's a PITA but it makes the code much more maintainable and readable)

rphmeier · 2020-04-24T18:23:43Z

src/voter/mod.rs

+	}
+}
+
+pub struct Inner<H, N, E> where


Exposing a struct Inner from voter.rs seems a little ambiguous. Likewise, implementation on Arc<RwLock<...>> directly seems difficult from a backwards-compatibility perspective. Could you elaborate on the goal of the trait VoterState rather than having a simple struct?

It's used in struct Voter as well, but it's called inner although there are other fields. This would not have to be exposed as public with a wrapper struct, meaning it could gain a more descriptive name than Inner.

The first approach Jon tried used a struct, the problem was that this struct had to be typed on H, N, E, and we'd have to propagate these types throughout the substrate codebase, it was especially hairy because types like Environment are currently not explicitly typed anywhere but would have to be due to the RPC initialization process. In substrate this trait is used as a trait object to avoid the type proliferation.

OK, I assumed something like that. Still, the API could be improved substantially with a wrapping struct.

Thanks for the feedback @rphmeier :)
I think we can actually remove the pub from Inner if I'm not mistaken? I'll add the missing docs

IMO I think it's better to keep the struct anyway (although not exported) to be able to lock inner as a whole for correctness (rather than best and background distinctly).

Updated the docs and removed the pub from Inner.

OK, I assumed something like that. Still, the API could be improved substantially with a wrapping struct.

Sorry, do you mean that the API could have been improved if using a simple struct instead of the trait would have been practical? Or that the API could be improved by creating a struct wrapping Arc<RwLock<...>> just so that we don't have to impl directly on Arc<RwLock<...>>?

I started sketching on the latter but I got stuck on finding a good name ... :/
Probably VoterState would be appropriate for such a struct, but then the trait would need to be renamed ... VoterStateQueryable? Doesn't roll of the tongue very well ...

struct SharedVoterState<...>(Arc<RwLock<Inner<...>>>); and then you implement VoterState for SharedVoterState.

Added the SharedVoterState to wrap Arc<RwLock<Inner>>

rphmeier

Looks fine except for grumbles

andresilva

thanks for adding all the missing docs to this crate, just some last minor nits.

src/lib.rs

src/voter/voting_round.rs

src/voter/mod.rs

Co-Authored-By: André Silva <[email protected]>

src/lib.rs

Co-Authored-By: Joshy Orndorff <[email protected]>

octol added the A3-inprogress label Mar 24, 2020

octol mentioned this pull request Mar 24, 2020

Expose GRANDPA round state through RPC paritytech/substrate#5375

Merged

6 tasks

andresilva and others added 10 commits April 1, 2020 00:37

use trait object for exposed voterstate

95c6cdf

implement voter state trait for inner

a3d8568

voter state reporting struct

51360b8

fix test compilation

5ab3a5d

make VoterState and Voter::voter_state() public

9b181b3

tag voter_state with Send and Sync

0634e90

populate data sent back by Voter::voter_state

11aeb74

add background_rounds field without yet populating it

3034d3b

fix deadlock when reading voter state

c5577e5

fix compilation when rebase on master

e8b45e5

octol force-pushed the jon/issue-55-expose-voter-state branch from 2fe5d65 to e8b45e5 Compare April 15, 2020 09:53

octol added 3 commits April 15, 2020 15:22

use weight types in report::RoundState

c64afa6

populate background_rounds from past_rounds

1100f1b

fix compilation of RoundState constructor

68850f1

octol marked this pull request as ready for review April 17, 2020 08:27

tidy up reporting function

d2805dd

octol added A0-pleasereview F8-enhancement and removed A3-inprogress F8-enhancement labels Apr 17, 2020

andresilva reviewed Apr 17, 2020

View reviewed changes

octol added 4 commits April 20, 2020 11:34

add missing doc strings for pub traits and structs

1c5e2ff

remove unnecessary Pin in testing impl of Environment

f82db74

create voter_state test

9b23006

don't check background_rounds

7d1c784

octol added 2 commits April 22, 2020 15:20

check background_rounds in skips_to_latest_round_after_catch_up

114b870

deduplicate expected_round_state

080a910

andresilva approved these changes Apr 24, 2020

View reviewed changes

minor formatting from review

851b01c

rphmeier reviewed Apr 24, 2020

View reviewed changes

rphmeier approved these changes Apr 24, 2020

View reviewed changes

octol added 4 commits April 24, 2020 23:20

don't export Inner

eaa3fe0

add more docs to exposed types and fields

9221016

wrap arc rwlock inner in SharedVoterState

4c06f15

add warning for missing docs and fill in missing docs

84d0899

andresilva reviewed Apr 28, 2020

View reviewed changes

octol and others added 3 commits April 29, 2020 10:21

apply doc suggestions from code review

e031aea

Co-Authored-By: André Silva <[email protected]>

fix whitespace that github commit created + typos

8fea9d5

rename VoterState::voter_state to get

dd47aec

JoshOrndorff reviewed Apr 29, 2020

View reviewed changes

src/lib.rs Outdated Show resolved Hide resolved

andresilva approved these changes Apr 29, 2020

View reviewed changes

andresilva and others added 3 commits April 29, 2020 20:39

Update src/lib.rs

b017c8f

Co-Authored-By: Joshy Orndorff <[email protected]>

move voter state reporting position in file

ed67070

rename Inner to InnerVoterState

e24b046

andresilva approved these changes Apr 30, 2020

View reviewed changes

andresilva merged commit d4351d1 into paritytech:master Apr 30, 2020

andresilva mentioned this pull request Apr 30, 2020

Expose information about the Voter to outside #55

Closed

andresilva added a commit that referenced this pull request Apr 30, 2020

Expose voter state (#114)

630ab8c

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Expose voter state #114

Expose voter state #114

octol commented Mar 24, 2020 •

edited

Loading

parity-cla-bot commented Mar 24, 2020

octol commented Mar 24, 2020

parity-cla-bot commented Mar 24, 2020

andresilva left a comment

andresilva Apr 17, 2020

octol Apr 22, 2020

andresilva Apr 17, 2020

octol Apr 22, 2020

andresilva Apr 17, 2020

octol Apr 22, 2020

octol commented Apr 22, 2020 •

edited

Loading

andresilva left a comment

andresilva Apr 24, 2020

rphmeier Apr 24, 2020 •

edited

Loading

rphmeier Apr 24, 2020 •

edited

Loading

rphmeier Apr 24, 2020

andresilva Apr 24, 2020

rphmeier Apr 24, 2020

octol Apr 24, 2020

andresilva Apr 24, 2020

octol Apr 24, 2020

andresilva Apr 24, 2020

octol Apr 27, 2020

rphmeier left a comment

andresilva left a comment

		inner.best_round.round_number(),
		inner.best_round.round_number() + 1,

Expose voter state #114

Expose voter state #114

Conversation

octol commented Mar 24, 2020 • edited Loading

parity-cla-bot commented Mar 24, 2020

octol commented Mar 24, 2020

parity-cla-bot commented Mar 24, 2020

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

octol commented Apr 22, 2020 • edited Loading

andresilva left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier Apr 24, 2020 • edited Loading

Choose a reason for hiding this comment

rphmeier 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rphmeier left a comment

Choose a reason for hiding this comment

andresilva left a comment

Choose a reason for hiding this comment

octol commented Mar 24, 2020 •

edited

Loading

octol commented Apr 22, 2020 •

edited

Loading

rphmeier Apr 24, 2020 •

edited

Loading

rphmeier Apr 24, 2020 •

edited

Loading