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: add pending connection limits + bump libp2p to 0.54.1 #2150

Open
wants to merge 11 commits into
base: master
Choose a base branch
from

Conversation

maschad
Copy link
Member

@maschad maschad commented Sep 1, 2024

Linked Issues/PRs

Description

This is apart of the broader initiative outlined in #1968 to improve the DoS resilience

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog

Before requesting review

  • I have reviewed the code myself


/// Max number of concurrent pending outgoing connections
#[clap(long = "max-pending-outgoing-connections", default_value = "100", env)]
pub max_pending_outgoing_connections: u32,
Copy link
Member Author

@maschad maschad Sep 1, 2024

Choose a reason for hiding this comment

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

Even though I've added this parameter, I'm not fully sold that it should be present. The rationale is we only dial a peer if:

a) We are below the desired amount of peers in a gossipsub mesh (which would imply we would be below any potential max_pending_outgoing_connections anyway) and we determined that the peer in question would have a reasonable peer score. So essentially our gossipsub config should handle this scenario.

AND

b) We are relatively certain that peer is reachable i.e. the multiaddr for that peer can be successfully dialled - if that is so, we want to prioritize dialling it.

But I may be missing something about the current p2p setup.

Copy link
Member

Choose a reason for hiding this comment

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

You are unsure if we should put a limit on outgoing connections?

Copy link
Member Author

Choose a reason for hiding this comment

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

Correct, more specifically on outgoing pending connections (as opposed to established outgoing connections)

@maschad maschad changed the title feat!: add pending connection limits + bump libp2p to 0.54.1 feat: add pending connection limits + bump libp2p to 0.54.1 Sep 1, 2024
@maschad maschad marked this pull request as ready for review September 2, 2024 03:27
@maschad maschad self-assigned this Sep 2, 2024
Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

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

For me that's an interesting add but it's doesn't close : #1560. IMO, in order to close this issue we also need to add parameters for the number of established connections (in/out)

AurelienFT
AurelienFT previously approved these changes Sep 6, 2024
Copy link
Contributor

@AurelienFT AurelienFT left a comment

Choose a reason for hiding this comment

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

Looks good to me. Little nit on the CHANGELOG. Can you update it now that you have a new parameter added ?

@maschad
Copy link
Member Author

maschad commented Sep 7, 2024

Looks good to me. Little nit on the CHANGELOG. Can you update it now that you have a new parameter added ?

Good catch @AurelienFT , I've updated it in 1127c72

AurelienFT
AurelienFT previously approved these changes Sep 9, 2024
@xgreenx
Copy link
Collaborator

xgreenx commented Sep 9, 2024

We shouldn't merge this PR before #2131

Plus, let's wait for my review as well, since maybe we need to clean up some other logic

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
5 participants