Open one substream for each notifications protocol #4909

tomaka · 2020-02-12T15:46:37Z

In #4284 we introduced a new API for the networking crate, where one can register a "notifications protocol". The implementation, however, was simply translating notifications into old messages and sending them the same way as before.

This PR is the next step.
In addition to the current unique substream with the remote, we now also try to open a separate substream for each notifications protocol that has been registered.
If the remote doesn't support the new protocol, we don't care. If the remote does support the new protocol, then we use it to send our notifications.

(Note: connections that are already open when a new protocol is registered will not use the new protocol. This really should be fixed eventually by forbidding registering new protocols after the initialization process (cc #4587 ).)

Right now the only protocol concerned is GrandPa. I went with /paritytech/grandpa/1 for the protocol name.
cc'ing @rphmeier @andresilva

New protocol description

The new substreams have the following properties:

They are unidirectional w.r.t notifications. If A wants to send notifications to B, it needs to open a substream. If then B wants to send notifications to A, then it needs to open its own substream for that. It is encouraged, but not mandatory, for B to open a substream with A if A opens one with B. The reason is to avoid collisions if both sides want to open a substream at the same time.
After A opens a substream with B, it starts by sending an initial message. B then needs to send back a handshake message. A can then start sending notifications. B can refuse the substream (if it is full) by closing the substream without replying back. This handshake isn't accessible from the public API yet.
It is a protocol error to open another substream for a given protocol while one already exists.

Backwards compatibility

This PR is fully backwards compatible with the current network.
Also note that if we receive a notification the "old way", we still accept it even if the new protocol is in use.

Right now the legacy protocol is still the only way to perform syncing and light client requests, and therefore we make the presence of the legacy substream act as "the authority".
In other words, we are considered "connected" to a node if and only if we have a legacy substream open with that node. The new substreams exist in parallel but are optional. Also note that it is not possible to open a new substream without opening the legacy substream as well, but this will change in the future.

However nodes that have this PR will immediately talk to each other using the new protocol. It should therefore be strongly tested before it is deployed "for real" on Kusama.

tomaka · 2020-02-12T16:01:24Z

I just remembered that at the moment GRANDPA uses the legacy substream handshake to determine whether the remote is a full node or an authority.

This "role" should instead be passed as part of the handshake. Before we merge, I will expose this handshake API and make it pass the role for GRANDPA.

EDIT: actually no, we can't do that for backwards compatibility reasons

twittner

I did not check every line but the overall structure looks good to me.

client/network/src/protocol/generic_proto/behaviour.rs

client/network/src/protocol/generic_proto/handler/group.rs

client/network/src/protocol/generic_proto/upgrade/notifications.rs

Co-Authored-By: Toralf Wittner <[email protected]>

mxinden

Took a first look. Thanks for putting so much effort into making this backward compatible!

Will take another (more in-depth) look. I am sorry for the delay.

mxinden · 2020-02-18T08:14:02Z

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


 		let validator = Arc::new(validator);
-		let gossip_engine = GossipEngine::new(service.clone(), GRANDPA_ENGINE_ID, validator.clone());
+		let gossip_engine = GossipEngine::new(service.clone(), GRANDPA_ENGINE_ID, GRANDPA_PROTOCOL_NAME, validator.clone());


Suggested change

let gossip_engine = GossipEngine::new(service.clone(), GRANDPA_ENGINE_ID, GRANDPA_PROTOCOL_NAME, validator.clone());

let gossip_engine = GossipEngine::new(

service.clone(),

GRANDPA_ENGINE_ID,

GRANDPA_PROTOCOL_NAME,

validator.clone(),

);

mxinden · 2020-02-18T08:21:22Z

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


 		let validator = Arc::new(validator);
-		let gossip_engine = GossipEngine::new(service.clone(), GRANDPA_ENGINE_ID, validator.clone());
+		let gossip_engine = GossipEngine::new(service.clone(), GRANDPA_ENGINE_ID, GRANDPA_PROTOCOL_NAME, validator.clone());


Do I understand correctly, that GRANDPA_ENGINE_ID is a legacy concept once this merges? If so, could we rename it to LEGACY_GRANDPA_ENGINE_ID or mention it in the doc comment of its definition?

It is also used in non-networking parts of the code.

mxinden · 2020-02-18T08:27:38Z

client/network/src/protocol/generic_proto/behaviour.rs


+	/// Registers a new notifications protocol.
+	///
+	/// You are very strongly encouraged to call this method very early on. Any connection open


Suggested change

/// You are very strongly encouraged to call this method very early on. Any connection open

/// You are very strongly encouraged to call this method very early on. Any open connection

mxinden · 2020-02-18T08:28:53Z

client/network/src/protocol/generic_proto/behaviour.rs

+	/// will retain the protocols that were registered then, and not any new one.
+	pub fn register_notif_protocol(
+		&mut self,
+		proto_name: impl Into<Cow<'static, [u8]>>,


This patch uses both proto_name and protocol_name to refer to the same thing. Can we settle on one?

mxinden · 2020-02-18T08:58:31Z

client/network/src/protocol/generic_proto/behaviour.rs

+/// - For each registered protocol, we also open an additional substream for this protocol. If the
+/// remote refuses this substream, then it's fine.
+///
+/// - Whenever we want to send a message, we pass either `None` to force the legacy substream, or


Does this refer to write_notification? As far as I can tell, none of its arguments take an Option.

mxinden · 2020-02-18T09:04:15Z

client/network/src/protocol/generic_proto/handler/group.rs

+///
+/// See the documentation at the module level for more information.
+pub struct NotifsHandlerProto {
+	/// Prototypes for handlers for ingoing substreams.


Suggested change

/// Prototypes for handlers for ingoing substreams.

/// Prototypes for handlers for incoming substreams.

mxinden · 2020-02-18T09:05:03Z

client/network/src/protocol/generic_proto/handler/group.rs

+///
+/// See the documentation at the module level for more information.
+pub struct NotifsHandler {
+	/// Handlers for ingoing substreams.


Suggested change

/// Handlers for ingoing substreams.

/// Handlers for incoming substreams.

* Open one substream for each notifications protocol * Fix WASM build * Apply suggestions from code review Co-Authored-By: Toralf Wittner <[email protected]> * Address concerns * Use unsigned-varint to read the varint * Use unsigned-varint * Forgot Cargo.lock Co-authored-by: Toralf Wittner <[email protected]>

Open one substream for each notifications protocol

e855c33

tomaka requested review from Demi-Marie and andresilva as code owners February 12, 2020 15:46

tomaka requested review from mxinden and twittner February 12, 2020 15:46

tomaka added A0-please_review Pull request needs code review. B1-clientnoteworthy labels Feb 12, 2020

Fix WASM build

6884db2

mxinden mentioned this pull request Feb 13, 2020

[WIP] Refactor gossiping system #4125

Closed

twittner reviewed Feb 13, 2020

View reviewed changes

client/network/src/protocol/generic_proto/behaviour.rs Outdated Show resolved Hide resolved

client/network/src/protocol/generic_proto/handler/group.rs Outdated Show resolved Hide resolved

client/network/src/protocol/generic_proto/upgrade/notifications.rs Outdated Show resolved Hide resolved

tomaka and others added 2 commits February 14, 2020 13:55

Apply suggestions from code review

0be46d3

Co-Authored-By: Toralf Wittner <[email protected]>

Merge remote-tracking branch 'upstream/master' into split-substreams2

1672e3a

mxinden reviewed Feb 18, 2020

View reviewed changes

tomaka added 5 commits February 18, 2020 15:39

Merge remote-tracking branch 'upstream/master' into split-substreams2

e160419

Address concerns

4ac628e

Use unsigned-varint to read the varint

d50c835

Use unsigned-varint

8292185

Merge remote-tracking branch 'upstream/master' into split-substreams2

b2ef3e8

tomaka requested review from mxinden and twittner and removed request for Demi-Marie and andresilva February 20, 2020 12:41

twittner approved these changes Feb 20, 2020

View reviewed changes

Forgot Cargo.lock

d25e820

tomaka merged commit 397f53a into paritytech:master Feb 21, 2020

tomaka deleted the split-substreams2 branch February 21, 2020 10:06

tomaka mentioned this pull request Mar 23, 2020

Make transactions and block announces use notifications substreams #5360

Merged

	/// You are very strongly encouraged to call this method very early on. Any connection open
	/// You are very strongly encouraged to call this method very early on. Any open connection

	/// Prototypes for handlers for ingoing substreams.
	/// Prototypes for handlers for incoming substreams.

	/// Handlers for ingoing substreams.
	/// Handlers for incoming substreams.

Open one substream for each notifications protocol #4909

Open one substream for each notifications protocol #4909

Uh oh!

Conversation

tomaka commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

New protocol description

Backwards compatibility

Uh oh!

tomaka commented Feb 12, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

twittner left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

mxinden 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!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

tomaka commented Feb 12, 2020 •

edited

Loading

tomaka commented Feb 12, 2020 •

edited

Loading