Answer queries by marcio-diaz · Pull Request #65 · paritytech/finality-grandpa

marcio-diaz · 2019-05-08T12:31:37Z

Starting #10

This PR adds the HistoricalVotes struct, for saving votes in received order and indicating the moment of prevoting and precommiting.

…y-grandpa into marcio/answer-queries

rphmeier · 2019-05-08T17:17:29Z

src/round.rs

+	/// Set the length of prevotes received at the moment of prevoting.
+	pub fn set_prevote_idx(&mut self) -> bool {
+		self.prevote.voted_at = Some(self.prevote.votes().len());
+		println!("set prevote idx to {:?}", self.prevote.voted_at);


stray println

rphmeier · 2019-05-08T17:17:35Z

src/round.rs

+	/// Set the length of precommits received at the moment of precommiting.
+	pub fn set_precommit_idx(&mut self) -> bool {
+		self.precommit.voted_at = Some(self.precommit.votes().len());
+		println!("set precommit idx to {:?}", self.precommit.voted_at);


rphmeier · 2019-05-08T17:20:57Z

src/voter/voting_round.rs

+		HistoricalVotes {
+			seen: prevotes.chain(precommits).collect(),
+			prevote_idx: self.votes.prevote_idx(),
+			precommit_idx: self.votes.precommit_idx().map(|idx| idx + prevotes_len),


I don't think this logic is right. There is no guarantee that we see all prevotes before seeing any precommits. Since the estimate logic depends on both prevotes and precommits, we need an interleaved list where the indices are those we've voted at.

Ok, I'll fix it. I thought it shouldn't invalidate the votes. But it's ugly and not what was asked anyway.

rphmeier · 2019-05-08T17:21:29Z

The main difficulty with this feature isn't necessarily answering the queries, but persisting them to the disk so that we can answer queries after restarts and even for old rounds.

marcio-diaz · 2019-05-09T16:11:03Z

The main difficulty with this feature isn't necessarily answering the queries, but persisting them to the disk so that we can answer queries after restarts and even for old rounds.

The persistence should be done in substrate, right? I'll make a PR there, receiving the HistoricalVotes and saving it in a appropriate format.

Then I'm not sure if the answering methods should be done here or in substrate. My guess is to add them in Environment.

rphmeier · 2019-05-09T17:33:59Z

Ideally we can have the functionality in this crate which is generic over some trait that provides the persistence, with the implementation living in Substrate.

…sorted votes

andresilva · 2019-05-13T11:18:58Z

src/round.rs

+
 					None
 				}
 				VoteMultiplicity::Equivocated(ref first, ref second) => {


Don't we need to store equivocated votes as well?

At the very least, the first equivocation of a type from a given voter

andresilva · 2019-05-13T11:24:58Z

src/round.rs

+
 					None
 				}
 				VoteMultiplicity::Equivocated(ref first, ref second) => {


Same as above.

andresilva · 2019-05-13T11:29:45Z

src/voter/voting_round.rs

 					if let Some(prevote) = self.construct_prevote(last_round_state)? {
 						debug!(target: "afg", "Casting prevote for round {}", self.votes.number());
 						self.env.prevoted(self.round_number(), prevote.clone())?;
+						self.votes.set_prevoted_index();


Isn't this going to race? Is it guaranteed that the next vote we process will be our own?

Nevermind, I understand the semantics of this are to mark what we had seen so far when we prevoted/precommited (regardless of whether we'll eval our own vote before/after other new votes).

andresilva · 2019-05-13T11:30:43Z

src/voter/voting_round.rs

 	}

-	/// Return all imported votes for the round (prevotes and precommits).
+	/// Return all imported votes for the round (prevotes and precommits) unordered.


This is probably unused now.

andresilva

Minor grumbles. Overall lgtm.

andresilva · 2019-05-14T11:15:00Z

src/round.rs

 					)?;

-					self.historical_votes.seen.push(Message::Precommit(vote.clone()));
+					let message = Message::Precommit(vote.clone());


Is this clone necessary?

andresilva · 2019-05-14T11:15:12Z

src/round.rs


-					self.historical_votes.seen.push(Message::Precommit(vote.clone()));
+					let message = Message::Precommit(vote.clone());
+					let signed_message = SignedMessage { id: signer.clone(), signature, message };


Unnecessary clone?

andresilva · 2019-05-14T11:22:20Z

src/round.rs

+					// Push the vote into HistoricalVotes.
+					let message = Message::Prevote(vote);
+					let signed_message = SignedMessage { id: signer.clone(), signature, message };
+					self.historical_votes.push_vote(signed_message);


Maybe only add if vote == second? Or maybe if vote == first || vote == second just to be safe wrt ordering.

This is not needed since we early-exit after we've already seen an equivocation.

andresilva · 2019-05-14T11:22:37Z

src/round.rs

+					// Push the vote into HistoricalVotes.
+					let message = Message::Precommit(vote);
+					let signed_message = SignedMessage { id: signer.clone(), signature, message };
+					self.historical_votes.push_vote(signed_message);


Same as above.

feat: add length of hash when voted

feaaac7

marcio-diaz added the A3-inprogress label May 8, 2019

seerscode added 3 commits May 8, 2019 14:48

feat: add length of hash when voted

75ec15b

feat: add HistoricalVotes struct

2e9ade6

Merge branch 'marcio/answer-queries' of github.com:paritytech/finalit…

d055dad

…y-grandpa into marcio/answer-queries

marcio-diaz requested review from andresilva and rphmeier May 8, 2019 14:46

marcio-diaz added the A0-pleasereview label May 8, 2019

rphmeier reviewed May 8, 2019

View reviewed changes

seerscode added 12 commits May 9, 2019 20:09

fix: seen is more interleaved now, not sure if enough

c1ac719

fix: compiles

b174a92

fix: test compiles

8674ae1

fix: add index to votes in VoteTracker, since votes() doesn't return …

00c48c8

…sorted votes

fix: indices

70197e3

chore: add note about performance

72bb3d1

fix: ok, I'll just duplicate votes for now

0d6d721

fix: add new

3d0f81b

fix: test compiles

0060027

fix: add some methods, clean

c2c8b1c

fix: pass HistoricalVotes to completed()

4411de9

fix: test

5d5a86a

andresilva suggested changes May 13, 2019

View reviewed changes

seerscode added 3 commits May 13, 2019 16:10

fix: add signatures, remove votes(), add equivocations

296fd18

fix: add new_with_votes

404186c

chore: add some doc comments

0e104ac

marcio-diaz force-pushed the marcio/answer-queries branch from 5c2cfe1 to 0e104ac Compare May 14, 2019 07:25

marcio-diaz mentioned this pull request May 14, 2019

grandpa: save historical votes paritytech/substrate#2551

Closed

add test

f234092

marcio-diaz removed the A3-inprogress label May 14, 2019

fix test

448c697

andresilva reviewed May 14, 2019

View reviewed changes

fix: remove unnecesary clones

29a62dc

andresilva approved these changes May 14, 2019

View reviewed changes

marcio-diaz merged commit 715c3cb into master May 14, 2019

This was referenced Jun 24, 2019

grandpa: round catchup messages paritytech/substrate#2801

Merged

Update codec version to the 4.1 version paritytech/substrate#2948

Merged

andresilva deleted the marcio/answer-queries branch June 13, 2022 14:21

Conversation

marcio-diaz commented May 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

marcio-diaz May 9, 2019 • 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!

rphmeier commented May 8, 2019

Uh oh!

marcio-diaz commented May 9, 2019

Uh oh!

rphmeier commented May 9, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rphmeier May 13, 2019 • 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

marcio-diaz commented May 8, 2019 •

edited

Loading

marcio-diaz May 9, 2019 •

edited

Loading

rphmeier May 13, 2019 •

edited

Loading