Skip to content

network: handle stream creation on existing conns#6572

Merged
algorandskiy merged 8 commits intoalgorand:masterfrom
algorandskiy:pavel/p2p-extra-conns
Mar 5, 2026
Merged

network: handle stream creation on existing conns#6572
algorandskiy merged 8 commits intoalgorand:masterfrom
algorandskiy:pavel/p2p-extra-conns

Conversation

@algorandskiy
Copy link
Copy Markdown
Contributor

@algorandskiy algorandskiy commented Feb 27, 2026

Summary

  1. TestP2PNetworkDHTCapabilities failure (and TestCapabilities_Varying failure) showed the following race:

    1. DHT creates connection faster than our dialer
    2. That conn is not being marked so Connected does not create a stream

    Fixed by calling handleConnected directly and splitting peerID and direction checks - for direct call these might prevent from stream establishing.

  2. Changed Read to ReadFull in p2p metadata exchange code in order to workaround possible p2p transport level fragmentation for remote nodes.

  3. Updated streamManager.streams maps clean up - now it should not have any stale (reset or abandoned streams)

  4. While testing with DHT enabled, found it can consume all conns fast and lot letting regular conns to get through. It gets fixes eventually when the dialer will be trying active conns but takes longer. Bumped limits to GossipFanuot *3 when DHT is on.

Test Plan

Added new unit tests
Run both tests repeatedly to make sure no failures anymore

@codecov
Copy link
Copy Markdown

codecov bot commented Feb 27, 2026

Codecov Report

❌ Patch coverage is 47.22222% with 19 lines in your changes missing coverage. Please review.
✅ Project coverage is 47.80%. Comparing base (136b97a) to head (53b6108).
⚠️ Report is 2 commits behind head on master.
✅ All tests successful. No failed tests found.

Files with missing lines Patch % Lines
network/p2p/streams.go 43.47% 13 Missing ⚠️
network/p2p/p2p.go 25.00% 6 Missing ⚠️

❗ There is a different number of reports uploaded between BASE (136b97a) and HEAD (53b6108). Click for more details.

HEAD has 4 uploads less than BASE
Flag BASE (136b97a) HEAD (53b6108)
full_coverage 4 0
Additional details and impacted files
@@             Coverage Diff             @@
##           master    #6572       +/-   ##
===========================================
- Coverage   63.53%   47.80%   -15.74%     
===========================================
  Files         486      646      +160     
  Lines       67821    88108    +20287     
===========================================
- Hits        43089    42116      -973     
- Misses      21143    43224    +22081     
+ Partials     3589     2768      -821     
Flag Coverage Δ
full_coverage ?

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.

Comment thread network/p2p/p2p.go
Copy link
Copy Markdown
Contributor

@gmalouf gmalouf left a comment

Choose a reason for hiding this comment

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

We should talk through the gossip fanout multiplier

gmalouf
gmalouf previously approved these changes Feb 27, 2026
Comment thread network/p2p/p2p.go
@algorandskiy algorandskiy force-pushed the pavel/p2p-extra-conns branch from 412114a to 17bd044 Compare March 3, 2026 19:01
@algorandskiy algorandskiy requested a review from gmalouf March 3, 2026 21:29
Comment thread network/p2p/streams.go Outdated
@algorandskiy algorandskiy requested a review from cce March 4, 2026 19:11
Comment thread network/p2p/streams.go Outdated
Co-authored-by: cce <51567+cce@users.noreply.github.com>
@algorandskiy algorandskiy requested a review from cce March 5, 2026 15:37
@algorandskiy algorandskiy merged commit 267cc35 into algorand:master Mar 5, 2026
62 of 64 checks passed
@algorandskiy algorandskiy deleted the pavel/p2p-extra-conns branch March 5, 2026 20:19
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants