core/finality-grandpa: Gossip synced blocks via neighbor packets #3755

mxinden · 2019-10-03T20:30:54Z

Goal

In order to participate in the Grandpa gossip process a Grandpa voter needs
to know:

Which blocks are relevant for the current round.
Where to retrieve these relevant blocks from.

Grandpa voters already exchange neighbor packets with each other in an
epidemic fashion. This pull request extends the neighbor packet by a set of
block hashes which:

The node has imported.
Have been mentioned by Grandpa prevote or precommit messages.
Within the current Grandpa round.

Listening to these extended neighbor packets a node can pass the information
down to the network sync service enabled through
#3633.

This is part two of #3629.

Possible (current) solution

In order for a component within core/finality-grandpa/src to be able to
achieve the above goal it would need to:

(1) Receive prevote and precommit messages to keep track of the mentioned
block hashes.

(2) Somehow know whether a given block is available locally (is synced).

(3) Have access to a sink to send out the neighbor packets.

This pull request uses
core/finality-grandpa/src/communication/gossip::GossipValidator as it has
access to (1) and (3) and is already the component sending out
NeighborPackets. We pass down a Client (stripped down version via
BlockStatus trait) through NetworkBridge into the GossipValidator thus
also achieving (2).

Pull request status

For now this pull request is only a draft. I am opening it up at this point
to ease architectural discussions and to ask questions marked with TODO.

What do people think of the approach of extending the GossipValidator? Does
this stretch the responsibilities of a network validator a bit too much given
that the new logic is proactive?

andresilva

Tried to answer all the questions I found :)

andresilva · 2019-10-04T16:41:51Z

core/finality-grandpa/primitives/src/lib.rs

 pub type AuthorityIndex = u64;

 /// The identifier of a GRANDPA set.
+// TODO: Explain what a set is. A set of notes participating in the grandpa process?


A set is a given stable set of authorities, in this case it is identified by a monotonic id.

andresilva · 2019-10-04T16:42:49Z

core/finality-grandpa/primitives/src/lib.rs

 pub type SetId = u64;

 /// The round indicator.
+// TODO: Explain what a round is and how it relates to a set. A round in which each participant within the set votes?


A round is a voting round of the GRANDPA protocol, rounds exist in the context of a given set, i.e. they validate with the authorities assigned to the given set id.

andresilva · 2019-10-04T16:44:37Z

core/finality-grandpa/src/communication/gossip.rs

 const KEEP_RECENT_ROUNDS: usize = 3;

 /// Tracks topics we keep messages for.
+// TODO: What is a topic? What is a KeepTopic? Maybe more of a TopicsToKeep?


A topic is an identifier for gossip messages, you subscribe to a given topic and you get messages for it. At the beginning of each round a new topic is created named something like "{SET_ID}-{ROUND_ID}". And there's also a global topic (per set) for commit and catch up messages.

IIRC this data structure is used to keep track of what topics we are currently interested in. It is used in this context to validate messages we might get from other peers and also to inform them about our current status (neighbor packet).

andresilva · 2019-10-04T16:46:44Z

core/finality-grandpa/src/communication/gossip.rs

 /// Network level message with topic information.
 #[derive(Debug, Encode, Decode)]
+// TODO: Are both *prevote* and *precommit* *vote* messages? If so, this would need to be called
+// PrevoteOrPrecommitMessage, right?


Yes, this can be renamed to PrevoteOrPrecommitMessage to be more explicit.

andresilva · 2019-10-04T16:48:13Z

core/finality-grandpa/src/communication/gossip.rs

 #[derive(Debug, Encode, Decode)]
-pub(super) enum VersionedNeighborPacket<N> {
+pub(super) enum VersionedNeighborPacket<Block: BlockT> {
+	// TODO: Should we be updating this version given that this effort is a breaking change?


Ideally we should add a new variant here V2 wrapping the new neighbor packet type. In principle I think it should be possible to make this backwards-compatible (but I didn't think it through tbqh.)

andresilva · 2019-10-04T16:53:42Z

core/finality-grandpa/src/communication/gossip.rs

-	inner: parking_lot::RwLock<Inner<Block>>,
+// TODO: The name `GossipValidator` seems confusing, as it does more than validating (e.g. constructing new messages).
+pub(super) struct GossipValidator<Block: BlockT, Client> {
+	// TODO: Why does all of core/grandpa need to be Sync, hence the RwLock?


There's multiple components of substrate that will need to interact with GRANDPA. The network gets a GossipValidator, the import pipeline gets a GrandpaBlockImport, and there's also the GrandpaVoter which is running in some future.

andresilva · 2019-10-04T16:54:51Z

core/finality-grandpa/src/communication/mod.rs

 	/// A stream of input messages for a topic.
 	type In: Stream<Item=network_gossip::TopicNotification,Error=()>;

+	// TODO: Why are the topics block hashes?


The topics in our gossip system are hashes, they didn't need to be restricted to the same hashing as we used for blocks but that's what we have for now.

andresilva · 2019-10-04T16:56:01Z

core/finality-grandpa/src/communication/mod.rs

 	}

 	/// Get the round messages for a round in the current set ID. These are signature-checked.
+	// TODO: Why is the doc comment 'Get' when this function returns a Sink?


The doc is incomplete, this gives you both a sink and stream that should provide you with IO infrastructure for the round (to get and send messages).

andresilva · 2019-10-04T16:59:04Z

core/finality-grandpa/src/communication/gossip.rs


+		// TODO: Adding a block in a function called 'validate' seems missleading.
+		//
+		// TODO: Is it save to give these vote and commit messages any attention at this point? Do they need to be


At this point we have already done checked that the message is well-formed and the signature is valid. Any further validation will only happen once we have the full ancestry of the block imported now.

andresilva · 2019-10-04T17:00:44Z

core/finality-grandpa/src/communication/mod.rs


+	// TODO: This is very complex. If I understood correctly we are passing a closure to the network in which we pass a
+	// stream back via a oneshot. The other end of the oneshot is embedded in the `NetworkStream`, which retrieves the
+	// actual stream from the oneshot and polls it from now on. Couldn't `gossip.messages_for` return a stream?


TBH I don't remember of the top of my head why we do this.

mxinden · 2019-10-11T12:44:39Z

Closing in favor of #3800. I will follow up on the comments in future pull requests. Thanks for the help @andresilva.

mxinden added 5 commits October 2, 2019 12:16

*: Add documentation comments and questions

9ed26a2

core/finality-grandpa: Add known_vote_blocks to NeighborPacket

b356765

core/finality-grandpa: Pass client down to NetworkBridge

87ba689

core/finality-grandpa: Pass client down to GossipValidator

b6fb3f2

core/finality-grandpa: Make NeighborPacket aware of a Block for its hash

dd2a75e

mxinden added A3-in_progress Pull request is in progress. No review needed at this stage. J0-enhancement An additional feature request. labels Oct 3, 2019

andresilva reviewed Oct 4, 2019

View reviewed changes

mxinden mentioned this pull request Oct 11, 2019

core/finality-grandpa: Request block sync from network after import timeout #3800

Merged

mxinden closed this Oct 11, 2019

mxinden mentioned this pull request Oct 15, 2019

core/finality-grandpa: Minor refactorings #3825

Merged

core/finality-grandpa: Gossip synced blocks via neighbor packets #3755

core/finality-grandpa: Gossip synced blocks via neighbor packets #3755

Uh oh!

Conversation

mxinden commented Oct 3, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Goal

Possible (current) solution

Pull request status

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!

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!

mxinden commented Oct 11, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

mxinden commented Oct 3, 2019 •

edited

Loading