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.

Sign up for GitHub

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

Jump to bottom

Use bounded channels for peer events #1186

Merged
merged 2 commits into from
Sep 20, 2023
Merged

Use bounded channels for peer events #1186

merged 2 commits into from
Sep 20, 2023

Conversation

ImplOfAnImpl
Copy link
Contributor

@ImplOfAnImpl ImplOfAnImpl commented Sep 14, 2023 •
edited

This fixes #664 (note that as we've discussed earlier, the solution proposed in the issue itself is redundant; using a bounded channel for peer events should be enough).

P.S. I've based this PR on top of #1183 to simplify merging.

Base automatically changed from p2p_protocol_v2 to master September 15, 2023 08:55
@TheQuantumPhysicist
Copy link
Collaborator

Please fix the minor issues I mentioned then we'll go ahead and merge.

iljakuklic
iljakuklic reviewed Sep 15, 2023
View reviewed changes
p2p/src/net/default_backend/backend.rs Outdated Show resolved Hide resolved
p2p/src/net/default_backend/backend.rs Outdated Show resolved Hide resolved
@ImplOfAnImpl ImplOfAnImpl marked this pull request as draft September 15, 2023 11:15
@ImplOfAnImpl ImplOfAnImpl force-pushed the use_bounded_channels_for_peer_events branch from 9de5513 to b2e9122 Compare September 18, 2023 17:55
@ImplOfAnImpl ImplOfAnImpl marked this pull request as ready for review September 18, 2023 18:11
@iljakuklic
Copy link
Contributor

Just noticed that StreamMap stores the channels in a Vec and polls them in order defined by the order of elements in the vec. This may give some peers advantage over others. For example the first connection will be in the first position and, as far as I can see, will stay there until disconnected.

Do we want to address this? Is it possible to periodically shuffle the elements or something like that?

@ImplOfAnImpl
Copy link
Contributor Author

Just noticed that StreamMap stores the channels in a Vec and polls them in order defined by the order of elements in the vec. This may give some peers advantage over others. For example the first connection will be in the first position and, as far as I can see, will stay there until disconnected.

Do we want to address this? Is it possible to periodically shuffle the elements or something like that?

As far as I can see, StreamMap::poll_next_entry chooses starting index randomly, so this should not be a problem.

@iljakuklic
Copy link
Contributor

As far as I can see, StreamMap::poll_next_entry chooses starting index randomly, so this should not be a problem.

Ok, good. Thanks for finding out.

@ImplOfAnImpl ImplOfAnImpl merged commit b7f0b1d into master Sep 20, 2023
23 checks passed
@ImplOfAnImpl ImplOfAnImpl deleted the use_bounded_channels_for_peer_events branch September 20, 2023 12:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@iljakuklic iljakuklic iljakuklic approved these changes

@muursh muursh muursh approved these changes

@TheQuantumPhysicist TheQuantumPhysicist TheQuantumPhysicist approved these changes

@erubboli erubboli Awaiting requested review from erubboli erubboli is a code owner

@azarovh azarovh Awaiting requested review from azarovh azarovh is a code owner

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unbounded channels and DoS attacks
4 participants
@ImplOfAnImpl @TheQuantumPhysicist @iljakuklic @muursh