Cap mux simple #5577

emhane · 2023-11-26T12:04:46Z

an alternative approach to the RlpxSatelliteStream. a simplification of #2981.

contrary to RlpxSatelliteStream, MuxDemuxStream is wrapped by EthStream. this means existing implementation of a session is unchanged.

the equivalent to ProtocolProxy is StreamClone, although StreamClone is wrapped by a capability stream e.g. SnapStream<StreamClone>.

functionality that is removed from #2981

ability to clone streams from other clones. stream can only be cloned from MuxDemuxStream (the main stream).

mattsse

I like this a lot.

This would allow us to detach a subprotocol connection via StreamClone.
compared to the "SatelliteStream" approach, this requires that the StreamClone is spawned onto another task, right?

crates/net/eth-wire/src/muxdemux.rs

emhane · 2023-11-27T13:14:34Z

I like this a lot.

This would allow us to detach a subprotocol connection via StreamClone. compared to the "SatelliteStream" approach, this requires that the StreamClone is spawned onto another task, right?

stream clone doesn't necessarily have to be spawned in another task than the one that the eth session is running in, but it can be. running another capability stream in the same session thread as eth works fine too.

each extra protocol is an extra stream, and should be modelled after the EthStream type.

one stream, usually EthStream, will be the owner of the de-/muxed p2p connection EthStream<MuxDemuxStream<P2PStream<S>> and other streams will be clones of the de-/muxed p2p connection e.g. SnapStream<StreamClone>.

crates/net/eth-wire/src/capability.rs

crates/net/eth-wire/src/muxdemux.rs

emhane · 2023-11-28T16:04:08Z

commit d73be94 plugs RlpxSubprotocol into the de-/muxed p2p connection. messages can be sent and received via the connections in extra_conns.

what I see that remains to fix to close #791 is re-building the HelloMessageWithProtocols when add_rlpx_sub_protocol is called. adressed in #5640 .

emhane · 2023-11-28T17:52:30Z

commit 703634b stores the extra conns in SessionManager but we can also pass them higher up in the app via SessionEvent instead. possibly which option is better will be apparent when another caps protocol has been implemented using ProtocolHandler and ConnectionHandler IF.

we may want to introduce a bound on how many extra protocol connections can be open in total.

mattsse

I like the mux/demux sink+stream a lot.

I'm unsure about the integration in reth-network yet, imo the satellite stream would be easier to handle for this use case.

But still undecided. So to move forward I'd like to include the stream impl alone and think about reth-network integration separately.

mattsse · 2023-11-29T18:16:58Z

crates/net/eth-wire/src/muxdemux.rs

+        for _ in 0..self.demux.len() {
+            if self.inner.poll_ready_unpin(cx).is_pending() {
+                break
+            }


we should flip this:

as long as the underlying sink can accept messages, start sending buffered messages from demux.

so the main stream should not be prioritised over the secondary streams? e.g. eth protocol should not be prioritised over snap protocol?

commit c6c86d1 addresses this by sending at least as many messages as there are stream clones, then sending as long as their is no message received for the main stream

emhane · 2023-11-29T22:59:54Z

I like the mux/demux sink+stream a lot.

I'm unsure about the integration in reth-network yet, imo the satellite stream would be easier to handle for this use case.

But still undecided. So to move forward I'd like to include the stream impl alone and think about reth-network integration separately.

sgtm

emhane · 2023-11-30T15:16:01Z

commit d73be94 plugs RlpxSubprotocol into the de-/muxed p2p connection. messages can be sent and received via the connections in extra_conns.

what I see that remains to fix to close #791 is re-building the HelloMessageWithProtocols when add_rlpx_sub_protocol is called. adressed in #5640 .

opened a pr to address this #5640

mattsse

this looks great, I like the muxer a lot.

one nit re poll loop.

also perhaps we should prioritize sending outgoing messages and put the sink logic in its own loop? so we process as many outgoing messages as possible before reading from the wire. not sure though

crates/net/eth-wire/src/muxdemux.rs

mattsse · 2023-12-04T17:13:54Z

crates/net/eth-wire/src/muxdemux.rs

+/// More or less a weak clone of the stream wrapped in [`MuxDemuxer`] but the bytes belonging to
+/// other capabilities have been filtered out.
+#[derive(Debug)]
+pub struct StreamClone {


unsure about the naming yet,

I believe you had WeakStream or something.
perhaps WeakCapabilityStream?

WeakStreamClone initially, 'weak' since it is some kind of reference to the stream but doesn't own it. if not stream clone then the naming I like the most is ProtocolProxy from multiplex.rs. MuxDemuxer or MuxDemuxStream is then the proxy server.

Then I added ManuallyDrop wrapper, then I removed it for simplicity since anyway method of trait CanDisconnet should be used before drop. Now it's kind of a weak clone (of owning ref MuxDemuxStream) because MuxDemuxStream can be dropped when there are still StreamClones using the pointed to MuxDemuxer but it will prevent disconnecting if method from CanDisconnect trait is used.

Signed-off-by: Emilia Hane <[email protected]>

mattsse

great!
I'm very happy with how this turned out.

injected user defined sub protocol handles span up a new attack surface. to what extent do we want to account for that? for example, if a sub protocol has a DoS vulnerability so that there are always subprotocol messages to send then the main protocol won't receive any messages if we don't implement poll_next like on MuxDemuxStream in this pr.

I think it should be the protocol's impl to handle spam, punish peer for sending bad/spammy messages, like eth also does

emhane requested review from mattsse, Rjected, Evalir, onbjerg, rkrasiuk and gakonst as code owners November 26, 2023 12:04

emhane mentioned this pull request Nov 26, 2023

WIP: caps mux #2981

Closed

mattsse requested changes Nov 27, 2023

View reviewed changes

crates/net/eth-wire/src/muxdemux.rs Outdated Show resolved Hide resolved

crates/net/eth-wire/src/muxdemux.rs Outdated Show resolved Hide resolved

crates/net/eth-wire/src/muxdemux.rs Outdated Show resolved Hide resolved

crates/net/eth-wire/src/muxdemux.rs Outdated Show resolved Hide resolved

mattsse requested changes Nov 27, 2023

View reviewed changes

crates/net/eth-wire/src/capability.rs Outdated Show resolved Hide resolved

crates/net/eth-wire/src/muxdemux.rs Outdated Show resolved Hide resolved

crates/net/eth-wire/src/muxdemux.rs Outdated Show resolved Hide resolved

emhane force-pushed the cap-mux-simple branch from 62571da to 013a448 Compare November 27, 2023 17:09

emhane requested a review from mattsse November 27, 2023 17:22

emhane force-pushed the cap-mux-simple branch from 013a448 to 7615d3d Compare November 27, 2023 18:54

emhane force-pushed the cap-mux-simple branch from 7615d3d to d73be94 Compare November 28, 2023 16:08

mattsse requested changes Nov 29, 2023

View reviewed changes

emhane force-pushed the cap-mux-simple branch 5 times, most recently from ad8bfd2 to 4bdb173 Compare November 30, 2023 01:53

emhane requested a review from shekhirin as a code owner November 30, 2023 01:53

emhane force-pushed the cap-mux-simple branch from 4bdb173 to 8bda850 Compare November 30, 2023 01:59

emhane force-pushed the cap-mux-simple branch from cc6b446 to 48303b3 Compare December 2, 2023 21:06

emhane mentioned this pull request Dec 2, 2023

Fix eth message ID length bug #5666

Merged

mattsse requested changes Dec 4, 2023

View reviewed changes

emhane force-pushed the cap-mux-simple branch from ea6657b to 65aa041 Compare December 5, 2023 18:38

emhane and others added 23 commits December 7, 2023 10:28

Fix unsafe code

0bdbe60

Signed-off-by: Emilia Hane <[email protected]>

Fix stream termination bug

c7ce0ad

Signed-off-by: Emilia Hane <[email protected]>

fixup! Fix unsafe code

764b447

Signed-off-by: Emilia Hane <[email protected]>

Remove indirection of cap stream type

1f2df0f

Signed-off-by: Emilia Hane <[email protected]>

Plug-in p2p connection to rlpx sub protocols via stream clones

7a8e9a6

Signed-off-by: Emilia Hane <[email protected]>

Tweak logic in protocol handlers

6ebe6f5

Signed-off-by: Emilia Hane <[email protected]>

Placeholder POC store of extra conns in session manager

dddb17d

Signed-off-by: Emilia Hane <[email protected]>

fixup! Placeholder POC store of extra conns in session manager

93793e3

Signed-off-by: Emilia Hane <[email protected]>

Remove crumbs from git context switch

e4b0016

Signed-off-by: Emilia Hane <[email protected]>

Unplug MxDmxStream from network

e4fb50a

Signed-off-by: Emilia Hane <[email protected]>

Send all messages from stream clones but prioritise main stream

9cbb6bb

fixup! Send all messages from stream clones but prioritise main stream

4c862d9

fixup! Send all messages from stream clones but prioritise main stream

d7bb018

fixup! Send all messages from stream clones but prioritise main stream

39fb682

Fix poll close for mxdmx stream

8bc2a78

Add test for mxdmx stream and bug fixes

f4d8028

fixup! Add test for mxdmx stream and bug fixes

59f020f

Permanent fix to const eth number of values in message id

4bf76ff

Small optimisation

65c545f

fixup! Small optimisation

1628288

Remove duplicate logic

da5b3ae

Pass down context

d21b66f

Update docs

884bfe0

emhane force-pushed the cap-mux-simple branch from 751d0f9 to 884bfe0 Compare December 7, 2023 09:32

mattsse approved these changes Dec 8, 2023

View reviewed changes

mattsse added this pull request to the merge queue Dec 8, 2023

Merged via the queue into paradigmxyz:main with commit cd4d6c5 Dec 8, 2023

emhane deleted the cap-mux-simple branch December 9, 2023 23:19

emhane mentioned this pull request Jan 17, 2024

Prioritise primary stream #6086

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cap mux simple #5577

Cap mux simple #5577

emhane commented Nov 26, 2023

mattsse left a comment

emhane commented Nov 27, 2023

emhane commented Nov 28, 2023 •

edited

Loading

emhane commented Nov 28, 2023

mattsse left a comment

mattsse Nov 29, 2023

emhane Nov 29, 2023

emhane Dec 2, 2023

emhane commented Nov 29, 2023

emhane commented Nov 30, 2023

mattsse left a comment

mattsse Dec 4, 2023

emhane Dec 5, 2023

emhane Dec 5, 2023

mattsse left a comment •

edited

Loading

Cap mux simple #5577

Cap mux simple #5577

Conversation

emhane commented Nov 26, 2023

mattsse left a comment

Choose a reason for hiding this comment

emhane commented Nov 27, 2023

emhane commented Nov 28, 2023 • edited Loading

emhane commented Nov 28, 2023

mattsse left a comment

Choose a reason for hiding this comment

mattsse Nov 29, 2023

Choose a reason for hiding this comment

emhane Nov 29, 2023

Choose a reason for hiding this comment

emhane Dec 2, 2023

Choose a reason for hiding this comment

emhane commented Nov 29, 2023

emhane commented Nov 30, 2023

mattsse left a comment

Choose a reason for hiding this comment

mattsse Dec 4, 2023

Choose a reason for hiding this comment

emhane Dec 5, 2023

Choose a reason for hiding this comment

emhane Dec 5, 2023

Choose a reason for hiding this comment

mattsse left a comment • edited Loading

Choose a reason for hiding this comment

emhane commented Nov 28, 2023 •

edited

Loading

mattsse left a comment •

edited

Loading