feat: add pending connection limits + bump libp2p to 0.54.1
#2150
Conversation
|
|
||
| /// Max number of concurrent pending outgoing connections | ||
| #[clap(long = "max-pending-outgoing-connections", default_value = "100", env)] | ||
| pub max_pending_outgoing_connections: u32, |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
You are unsure if we should put a limit on outgoing connections?
There was a problem hiding this comment.
Correct, more specifically on outgoing pending connections (as opposed to established outgoing connections)
maschad
changed the title
0.54.10.54.1
There was a problem hiding this comment.
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)
Good catch @AurelienFT , I've updated it in 1127c72 |
|
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 |
|
How's this coming? |
Thanks for resurfacing this @MitchTurner it got lost in the ether with the mainnet whirlwind of changes. I've resolved the conflicts so it's ready for review again. I know @xgreenx wanted to look into it again but the changes imo are relatively low surface - I've upgraded our libp2p version + introduced connection limit options. |
|
Hello @maschad, As you mentionned that @xgreenx wanted to look again we will wait him before merging this but I can already re-approve as a first approver. However before approving the conflicts should be resolved, are you ok to resolve them ? Or we can do it for you, if you prefer :) Thanks again for this PR. |
|
Thanks @AurelienFT I've gone ahead and resolved the conflicts. |
Linked Issues/PRs
connection_limitsbehavior toFuelBehavior#1560Description
This is apart of the broader initiative outlined in #1968 to improve the DoS resilience
Checklist
Before requesting review