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

[Networking] Fixing enforced dial conflict between unicast manager and swarm #4951

Merged
merged 26 commits into from
Nov 16, 2023

Conversation

yhassanzadeh13
Copy link
Contributor

@yhassanzadeh13 yhassanzadeh13 commented Nov 7, 2023

The latest update simplifies the unicast manager's operations as follows:

  1. The unicast manager will create streams without checking for an existing connection, trusting the libp2p swarm to either dial a new connection or use the best available one.
  2. It removes the single connection limit between pairs, allowing the libp2p resource manager to regulate connections, which is crucial for network stability based on our mainnet debug findings.
  3. The manager no longer initiates connections but focuses on stream creation, with the swarm handling any necessary connections.
  4. Concurrent stream creation is not delayed by the unicast manager; libp2p's internal mechanisms already address this efficiently.

Reference: https://github.com/dapperlabs/flow-go/issues/6895

The following diagrams summarize the updates in the resource manger.

Unicast Manager Architecture master
Screenshot 2023-11-08 at 10 40 03 AM

Unicast Manager Architecture this PR
Screenshot 2023-11-08 at 10 40 40 AM

@yhassanzadeh13 yhassanzadeh13 changed the base branch from yahya/6895-fix-peer-blocking-issue to master November 7, 2023 22:50
@codecov-commenter
Copy link

codecov-commenter commented Nov 8, 2023

Codecov Report

Attention: 12 lines in your changes are missing coverage. Please review.

Comparison is base (ab5b93f) 56.30% compared to head (3263ab6) 56.21%.

Files Patch % Lines
network/p2p/unicast/cache/unicastConfigCache.go 81.25% 6 Missing ⚠️
network/p2p/unicast/manager.go 88.46% 2 Missing and 1 partial ⚠️
network/p2p/p2pbuilder/libp2pNodeBuilder.go 0.00% 2 Missing ⚠️
network/netconf/flags.go 0.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #4951      +/-   ##
==========================================
- Coverage   56.30%   56.21%   -0.10%     
==========================================
  Files         963      971       +8     
  Lines       90184    90434     +250     
==========================================
+ Hits        50776    50833      +57     
- Misses      35610    35803     +193     
  Partials     3798     3798              
Flag Coverage Δ
unittests 56.21% <80.95%> (-0.10%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@yhassanzadeh13 yhassanzadeh13 marked this pull request as ready for review November 9, 2023 18:49
Co-authored-by: Khalil Claybon <khalil.claybon@dapperlabs.com>
@yhassanzadeh13 yhassanzadeh13 added this pull request to the merge queue Nov 16, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Nov 16, 2023
@yhassanzadeh13 yhassanzadeh13 added this pull request to the merge queue Nov 16, 2023
Merged via the queue into master with commit d6e0d92 Nov 16, 2023
54 checks passed
@yhassanzadeh13 yhassanzadeh13 deleted the yahya/6895-removing-unicast-manager branch November 16, 2023 21:57
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants