Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

feat: integrate multiplexing #5559

Merged
merged 12 commits into from
Dec 12, 2023
Merged

feat: integrate multiplexing #5559

merged 12 commits into from
Dec 12, 2023

Conversation

mattsse
Copy link
Collaborator

@mattsse mattsse commented Nov 24, 2023

refactor some multiplex internals and add test for test protocol.

@mattsse mattsse added C-test A change that impacts how or what we test A-networking Related to networking in general labels Nov 24, 2023
@mattsse mattsse force-pushed the matt/implement-multiplex-test branch from 9f8084b to 158a08d Compare November 24, 2023 15:13
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like the tests and updated inner type, just left some notes on extracting some parts of the poll loop to methods

crates/net/eth-wire/src/multiplex.rs Show resolved Hide resolved
crates/net/eth-wire/src/multiplex.rs Outdated Show resolved Hide resolved
@mattsse mattsse force-pushed the matt/implement-multiplex-test branch from 91ad39f to 51855ab Compare December 2, 2023 11:52
crates/net/network/src/protocol.rs Outdated Show resolved Hide resolved
Comment on lines +31 to +36
pub enum EthRlpxConnection {
/// A That only supports the ETH protocol.
EthOnly(Box<EthPeerConnection>),
/// A connection that supports the ETH protocol and __at least one other__ RLPx protocol.
Satellite(Box<EthSatelliteConnection>),
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is starting a SatelliteStream that much overhead when no extra protocols are shared that this enum is necessary?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

possibly, because this would involve routing through a channel,

@mattsse mattsse changed the title feat: add multiplexing test feat: integrate multiplexing Dec 8, 2023
@mattsse mattsse requested a review from Rjected December 8, 2023 09:20
Copy link
Member

@Rjected Rjected left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, this looks good, super excited to see multiple caps in practice

@mattsse
Copy link
Collaborator Author

mattsse commented Dec 12, 2023

After thinking a bit longer about this,
I'm going with this satellite approach over the MuxDemuxer because

  • this still allows us to run a regular wrapped EthStream<TcpStream> most of the time (if no custom handlers installed and no matching additional caps) and we don't have any overhead routing through channels
  • the SatelitteStream is convenient because it hides polling the other cap streams internally so the interface can be unified over the enum EthRlpxConnection

@mattsse mattsse added this pull request to the merge queue Dec 12, 2023
Merged via the queue into main with commit 5062b7e Dec 12, 2023
28 checks passed
@mattsse mattsse deleted the matt/implement-multiplex-test branch December 12, 2023 16:43
@emhane
Copy link
Member

emhane commented Dec 12, 2023

After thinking a bit longer about this, I'm going with this satellite approach over the MuxDemuxer because

  • this still allows us to run a regular wrapped EthStream<TcpStream> most of the time (if no custom handlers installed and no matching additional caps) and we don't have any overhead routing through channels

MuxDemuxer doesn't route the main stream through channels

  • the SatelitteStream is convenient because it hides polling the other cap streams internally so the interface can be unified over the enum EthRlpxConnection

polling the main stream that owns the MuxDemuxer polls the other cap streams

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-networking Related to networking in general C-test A change that impacts how or what we test
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants