network: Use "one shot" protocol handler. #3520

twittner · 2019-08-30T15:42:09Z

Add two new NetworkBehaviours, one handling remote block requests and another one to handle light client requests (both local and from remote). The change is motivated by the desire to use multiple substreams of a single connection for different protocols. To achieve this, libp2p's OneShotHandler is used as a protocol handler in each behaviour. It will open a fresh substream for the duration of the request and close it afterwards. For block requests, we currently only handle incoming requests from remote and tests are missing. For light client handling we support incoming requests from remote and also ported a substantial amount of functionality over from light_dispatch.rs (including several tests). However the result lacks in at least two aspects:

We require external updates w.r.t. the best block per peer and currently nothing updates this information.
We carry a lot of peer-related state around.

Both aspects could be simplified by externalising peer selection and just requiring a specific peer ID where the request should be sent to. We still have to maintain some peer related state due to the way libp2p's swarm and network behaviour work (e.g. we must make sure to always issue NetworkBehaviourAction::SendEvents to peers we are connected to, otherwise the actions die a silent death).

Another change implemented here is the use of protocol buffers as the encoding for network messages. Certain individual fields of messages are still SCALE encoded. There has been some discussion about this in another PR (#3452), so far without resolution.

arkpar · 2019-09-03T11:44:41Z

Is there any round-trip involved for opening a substream?
Does it increase network traffic?

twittner · 2019-09-03T13:06:15Z

Is there any round-trip involved for opening a substream?

Opening a new substream involves the use of multistream-select to negotiate the protocol to use with the stream. I think this involves at least 1 roundtrip currently. Version 2 of multistream-select will eventually improve on this.

Does it increase network traffic?

Depending on the multiplexer there might be an increase in control data. For example with Yamux there are WindowUpdate frames to implement flow control and those are sent per stream.

arkpar · 2019-09-03T13:32:13Z

The change is motivated by the desire to use multiple substreams of a single connection for different protocols.

So what motivates this desire then? Extra roundtrips for each request does not sound like a good idea to me.

tomaka · 2019-09-03T14:00:17Z

So what motivates this desire then? Extra roundtrips for each request does not sound like a good idea to me.

The main motivation is de-coupling the gossiping from block requests, so that:

Light clients and nodes in general can make requests without occupying a gossiping slot (which is the case right now). In other words, you wouldn't occupy a gossiping slot anymore while major syncing for example.
Nodes can close their gossiping channel without interrupting all on-going requests.
We can open multiple gossiping channels (for example one only within validators, one for the relay chain, and one for each parachain) without having the problem of which channel to send the request to.

tomaka · 2019-09-03T14:01:52Z

Note that, theoretically, if/when multistream-select 2 happens, then that new system will be very slightly more efficient than what we currently have, as we would no longer transmit the type of message nor the request ID.

arkpar · 2019-09-03T14:18:01Z

I don't see how adding extra network latency can be more efficient.

What exactly is "gossiping slot"?
The current protocol was designed in such a way that parallel requests are possible.

In general I'm OK with using a separate substream for gossiping, but not with opening a new substream for each request/response. One of our major goals is to minimize network latency and data propagation time.

tomaka · 2019-09-03T14:25:35Z

What exactly is "gossiping slot"?

The slots whose number is controlled with the --in-peers and --out-peers options. What the peerset manager decides.
Unfortunately, right now, if our bootnodes are full, nobody can connect to the network anymore. This is a protocol design issue that is in the process of being solved by de-coupling gossiping from other requests.

I don't see how adding extra network latency can be more efficient.

Theoretically, there shouldn't be any additional round-trip compared to what we have right now.
Opening a substream is a very cheap operation. A substream is basically equivalent to a request ID.

The problem is that, due to the way the multistream-select protocol is designed, we have to wait for the remote to acknowledge that it supports the protocol we want (eg. "requesting a block").

There is an optimization in place in the code of multistream-select that allows skipping this round-trip, but it unfortunately can't apply automatically because if the first bytes that the Substrate protocol transmitted were "/multistream-select/1.0\n" then things could break.
Since this isn't the case, we could add a few hacks to the code of multistream-select to instruct it to always remove the round-trip.

tomaka

Looks mostly good to me.

tomaka · 2019-09-04T13:31:02Z

core/network/src/behaviour.rs

@@ -109,6 +117,11 @@ impl<B: BlockT, S: NetworkSpecialization<B>, H: ExHashT> Behaviour<B, S, H> {
 	pub fn put_value(&mut self, key: record::Key, value: Vec<u8>) {
 		self.discovery.put_value(key, value);
 	}
+
+	/// Get unique access to the light client handler.
+	pub fn light_client_handler(&mut self) -> &mut protocol::LightClientHandler<Substream<StreamMuxerBox>, B> {


I don't think this is used anywhere, so I'd prefer to remove it, otherwise someone is going to use this method. The LightClientHandler is supposed to be an implementation detail, so not accessible directly.

How do you want to expose the request functionality of the light client handler? Do you want to reproduce its request method here and forward the call?

Do you want to reproduce its request method here and forward the call?

Yes!

tomaka · 2019-09-04T13:33:45Z

core/network/src/protocol/block_requests.rs

+			max_block_data_response: 128,
+			max_request_len: 1024 * 1024,
+			inactivity_timeout: Duration::from_secs(15),
+			protocol: b"/polkadot/sync/1",


I don't think there should be a default value for that.
The value should be format!("/{}/sync/1", protocol_id) (where protocol_id is the protocol_id of type ProtocolId that is passed around during the network's initialization), and that must be passed by parameter.

tomaka · 2019-09-04T13:35:00Z

core/network/src/protocol/light_client_handler.rs

+			max_pending_requests: 128,
+			inactivity_timeout: Duration::from_secs(15),
+			request_timeout: Duration::from_secs(15),
+			protocol: b"/polkadot/light/1",


Same remark.

tomaka · 2019-09-04T13:36:25Z

core/network/src/protocol/light_client_handler.rs

+							// random ID, we should not have an entry for it with this peer ID in
+							// our `outstanding` map, so a malicious peer should not be able to get
+							// us here. It is our own fault and must be fixed!
+							panic!("unexpected peer status {:?} for {}", info.status, peer);


I'd go for calling log::error! and returning instead.

I do not think it makes sense to continue past that point as the internal state is inconsistent. As mentioned in the comment, this should never be reached. If it is however, the programme as written is wrong and needs to be fixed.

It would be nice if the NetworkBehaviour trait provides a way to signal an error condition to the swarm which would then stop polling it.

@twittner our codebase policy with panickers is to prove that they won't happen or remove them - is it not possible to recover again to a valid protocol state? mild programmer error should not take down a decentralized network

If you prefer to keep the behaviour going no matter what, we could maybe attempt to reset all internal state.

Edit 1: However, since the swarm that manages this behaviour is unaware of it, we would not know about our peers anymore. So, we would have to keep parts of the state.

Edit 2: Also when removing request information we may punish peers if we receive responses for requests we have issued in the past but now forgot about.

Edit 3: I think that continuing under all circumstances is not advisable as it further complicates the set of possible states. As I mentioned in a comment above it would be nice if we could signal an error back to the swarm which could then decide what to do with this NetworkBehaviour, e.g. to reinitialise it.

Is there a way to kill the node gracefully then?

Not that I am aware of. The NetworkBehaviour trait offers no such mechanism at the moment AFAIK. This would need to be addressed on libp2p-side first. CC @tomaka.

I have opened libp2p/rust-libp2p#1256 to discuss this further.

It doesn't make sense to have in libp2p a generic mechanism to say "hey there's a bug in the code, please do something".
The NetworkBehaviour can however emit an event that can correspond to anything we want.

tomaka · 2019-09-04T13:36:39Z

core/network/src/protocol/light_client_handler.rs

+						// expected. So, if we can not find an entry for it in our peer information table,
+						// then these two collections are out of sync which must not happen and is a clear
+						// programmer error that must be fixed!
+						panic!("missing peer information for {}; response {}", peer, id);


Same: error!

core/network/src/behaviour.rs

tomaka

Looks good to me

arkpar · 2019-09-12T09:36:13Z

I'm still not convinced that opening a substream for each request is a good idea. Until libp2p is able to support this without extra round trips and additional traffic this is a regression and there's no good reason to introduce it.

As for slot managements, there should be still a configurable limit on the number of simultaneous connections to prevent DOS/OOM attacks.

Unfortunately, right now, if our bootnodes are full, nobody can connect to the network anymore. This is a protocol design issue that is in the process of being solved by de-coupling gossiping from other requests.

This should be fixed by allowing additional slot for short-lived discovery connections and not by allowing unlimited connections.

tomaka · 2019-09-12T13:35:36Z

Note that this PR adds support for this new clean protocol in addition to the dumb one we have right now. The default is still the dumb one.

This should be fixed by allowing additional slot for short-lived discovery connections and not by allowing unlimited connections.

The only way to do that is to open a second TCP connection, which is against the design of libp2p.

What the new protocol that this PR introduces does is make Substrate actually fit the design of libp2p, and what you're suggesting goes in the direction of going further away from how libp2p is designed.

gavofyork · 2019-09-12T13:48:11Z

What the new protocol that this PR introduces does is make Substrate actually fit the design of libp2p

Until libp2p is able to support this without extra round trips and additional traffic

What are the chances of this ever actually happening? While it might be nice to tick the box of "designed to the ideals of the Gods of libp2p", performance should not go out of the window in pursuit of that.

tomaka · 2019-09-12T13:58:03Z

What are the chances of this ever actually happening?

There are two ways to solve that:

Wait for the multistream-select v2 protocol to be specc'ed and implement it. We've been waiting for around 2 years now.
Say "fuck it" and implement our equivalent. We would no longer be compatible with js-libp2p and go-libp2p.

As time passes, I'm leaning more and more towards option 2.

romanb · 2019-09-12T17:34:37Z

What are the chances of this ever actually happening?
* Say "fuck it" and implement our equivalent. We would no longer be compatible with js-libp2p and go-libp2p.

As time passes, I'm leaning more and more towards option 2.

I don't think it's as bad as it sounds though. rust-libp2p could support both, a multistream-select v1 that supports 0-RTT negotiation (which I think would be identical on the wire, just differing in if and when negotiation frames are flushed) and the standard multistream-select v1 with the extra roundtrips. The choice would have to be static, of course, since 0-RTT is only really possible when the "dialer" only supports a single protocol at each level of the protocol stack (i.e. for each upgrade, including the initial multistream protocol). So an application using rust-libp2p would have to choose either the "improved" protocol and only be compatible with similarly configured rust-libp2p peers, or choose the standard protocol for maximum compatibility at the cost of extra roundtrips. This choice doesn't even need to be at the scope of the entirety of libp2p, however. It can be scoped to connections and substreams, so that standard libp2p protocols like libp2p-kad can remain compatible. I'm going to submit a small proposal for rust-libp2p shortly to demonstrate what I mean and which may settle the issue with the extra roundtrips here, by configuring the SubstreamProtocol in the two new network behaviours in this PR with a "0-RTT" version.

Also, since 0-RTT negotiation is by its very nature incompatible with supporting multiple, alternative protocols at any level, even if multistream-select 2.0 were already done and ready, if you would really want 0-RTT negotiation on substreams you would have to not support 1.0 at the same time, i.e. commit to 2.0 in the same manner as you would commit to a custom protocol, thereby dropping interoperability with all libp2p peers not speaking 2.0 (yet). You would only get both 0-RTT and full interoperability once 2.0 is supported and rolled out across all libp2p clients (and only until 3.0 ;-)), which is surely even further away than having a final 2.0 spec and implementations.

Is it fair to assume that it is not important for substrate to be compatible with peers potentially based on other libp2p implementations, at least for the foreseeable future (i.e. all peers of a substrate node are assumed to be, well, substrate nodes)?

gavofyork · 2019-09-13T13:06:50Z

sadly, i think we will need to retain a compatibility mode since there are at least three other languages (JS, Go, C++) creating a polkadot implementation and thus who we will need to talk to.

i'm good with the idea of having two alternatives for handling multistreams, where we use the efficient one if both ends support it and the "standard" one otherwise.

romanb · 2019-09-13T14:21:06Z

sadly, i think we will need to retain a compatibility mode since there are at least three other languages (JS, Go, C++) creating a polkadot implementation and thus who we will need to talk to.

I see. It may still be an option though to convince all these parties to adopt some additional assumptions around the use of multistream-select v1 that are necessary to make the use of it safe with "lazy" negotiation and thus 0-RTT negotiation on substreams, because I don't think it strictly requires any changes to the protocol itself (on the wire), just some additional assumptions on how it is implemented and knowledge of the (fixed set of) protocols being used. See libp2p/rust-libp2p#1212 and libp2p/rust-libp2p#1245 for further detail.

rphmeier · 2019-09-24T14:31:45Z

The choice would have to be static, of course, since 0-RTT is only really possible when the "dialer" only supports a single protocol at each level of the protocol stack (i.e. for each upgrade, including the initial multistream protocol)

This I don't quite understand. Is there no way to do a few round-trips in the beginning of a connection to negotiate on what kinds of substreams you could do 0-RTT negotiation for?

tomaka · 2019-09-24T16:27:23Z

This I don't quite understand. Is there no way to do a few round-trips in the beginning of a connection to negotiate on what kinds of substreams you could do 0-RTT negotiation for?

Not "automatically" (as in, through a built-in mechanism in the libp2p protocol).

We could do it specifically for Substrate/Polkadot though.

tomaka · 2019-10-25T13:11:46Z

@twittner Could you please update to the master branch? We might merge this soon.

bkchr · 2020-01-28T22:35:28Z

Any update on this?

twittner · 2020-01-29T09:21:32Z

I do not know if there is agreement in principle that the approach taken here would be merged. Other than that there are quite a few conflicts which need to be resolved once more.

tomaka · 2020-01-29T10:45:43Z

I'm still in favour of merging this. Again, this code path isn't used in production yet and we will upgrade over time to see if it works first.
If our network protocol is to eventually make sense, we need either to merge this PR or to rewrite a similar PR later.

Add two new `NetworkBehaviour`s, one handling remote block requests and another one to handle light client requests (both local and from remote). The change is motivated by the desire to use multiple substreams of a single connection for different protocols. To achieve this, libp2p's `OneShotHandler` is used as a protocol handler in each behaviour. It will open a fresh substream for the duration of the request and close it afterwards. For block requests, we currently only handle incoming requests from remote and tests are missing. For light client handling we support incoming requests from remote and also ported a substantial amount of functionality over from `light_dispatch.rs` (including several tests). However the result lacks in at least two aspects: (1) We require external updates w.r.t. the best block per peer and currently nothing updates this information. (2) We carry a lot of peer-related state around. Both aspects could be simplified by externalising peer selection and just requiring a specific peer ID where the request should be sent to. We still have to maintain some peer related state due to the way libp2p's swarm and network behaviour work (e.g. we must make sure to always issue `NetworkBehaviourAction::SendEvent`s to peers we are connected to, otherwise the actions die a silent death. Another change implemented here is the use of protocol buffers as the encoding for network messages. Certain individual fields of messages are still SCALE encoded. There has been some discussion about this in another PR (paritytech#3452), so far without resolution.

twittner · 2020-02-10T16:00:36Z

Updated and rebased. Ready for review again.

Too much time has passed since this approval.

tomaka

The design of the code is the same as last time, and it looks good to me.
I didn't review in small details to spot any possible typo or something, but the tests coverage gives the confidence that this works well.

client/network/src/protocol/block_requests.rs

client/network/src/protocol/light_client_handler.rs

tomaka · 2020-02-12T10:50:47Z

For the sake of not leaving this PR open for a couple more months, I'm going to merge it.
As discussed above, these code paths aren't used by default yet.

gavofyork added the A3-in_progress Pull request is in progress. No review needed at this stage. label Sep 1, 2019

twittner force-pushed the new-behaviour branch from c967b1d to 78d0721 Compare September 2, 2019 09:26

twittner marked this pull request as ready for review September 2, 2019 16:29

twittner added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Sep 2, 2019

twittner requested a review from tomaka September 2, 2019 16:30

tomaka suggested changes Sep 4, 2019

View reviewed changes

tomaka reviewed Sep 9, 2019

View reviewed changes

core/network/src/behaviour.rs Outdated Show resolved Hide resolved

tomaka previously approved these changes Sep 9, 2019

View reviewed changes

romanb mentioned this pull request Sep 13, 2019

Configurable multistream-select protocol. V1Lazy variant. libp2p/rust-libp2p#1245

Merged

gavofyork added A3-in_progress Pull request is in progress. No review needed at this stage. and removed A0-please_review Pull request needs code review. labels Sep 21, 2019

twittner force-pushed the new-behaviour branch from 08e1a68 to c5b5c0d Compare February 10, 2020 15:29

Uncomment Behaviour::light_client_request.

cb038dd

twittner requested a review from tomaka February 10, 2020 15:59

twittner added A0-please_review Pull request needs code review. and removed A3-in_progress Pull request is in progress. No review needed at this stage. labels Feb 10, 2020

tomaka added the B1-clientnoteworthy label Feb 11, 2020

tomaka approved these changes Feb 11, 2020

View reviewed changes

client/network/src/protocol/block_requests.rs Show resolved Hide resolved

client/network/src/protocol/light_client_handler.rs Show resolved Hide resolved

twittner added 2 commits February 11, 2020 17:13

Merge branch 'master' into new-behaviour

13b093f

Add license headers.

79a65a0

tomaka merged commit ea721a1 into paritytech:master Feb 12, 2020

twittner deleted the new-behaviour branch February 12, 2020 10:56

tomaka mentioned this pull request Mar 31, 2020

Switch to new light client protocol #5472

Merged

This was referenced Apr 21, 2020

Add a protocol that answers finality proofs #5718

Merged

Remove legacy single substream tracking issue #5670

Closed

network: Use "one shot" protocol handler. #3520

network: Use "one shot" protocol handler. #3520

Conversation

twittner commented Aug 30, 2019 • edited Loading

arkpar commented Sep 3, 2019

twittner commented Sep 3, 2019

arkpar commented Sep 3, 2019

tomaka commented Sep 3, 2019

tomaka commented Sep 3, 2019 • edited Loading

arkpar commented Sep 3, 2019

tomaka commented Sep 3, 2019 • edited Loading

tomaka 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

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twittner Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

twittner Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka Sep 24, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tomaka left a comment

Choose a reason for hiding this comment

arkpar commented Sep 12, 2019

tomaka commented Sep 12, 2019

gavofyork commented Sep 12, 2019

tomaka commented Sep 12, 2019 • edited Loading

romanb commented Sep 12, 2019

gavofyork commented Sep 13, 2019 • edited Loading

romanb commented Sep 13, 2019

rphmeier commented Sep 24, 2019

tomaka commented Sep 24, 2019 • edited Loading

tomaka commented Oct 25, 2019

bkchr commented Jan 28, 2020

twittner commented Jan 29, 2020

tomaka commented Jan 29, 2020

twittner commented Feb 10, 2020

tomaka left a comment • edited Loading

Choose a reason for hiding this comment

tomaka commented Feb 12, 2020

twittner commented Aug 30, 2019 •

edited

Loading

tomaka commented Sep 3, 2019 •

edited

Loading

tomaka commented Sep 3, 2019 •

edited

Loading

twittner Sep 24, 2019 •

edited

Loading

twittner Sep 24, 2019 •

edited

Loading

tomaka Sep 24, 2019 •

edited

Loading

tomaka commented Sep 12, 2019 •

edited

Loading

gavofyork commented Sep 13, 2019 •

edited

Loading

tomaka commented Sep 24, 2019 •

edited

Loading

tomaka left a comment •

edited

Loading