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

refactoring(p2p/peerTracker): extend conditions for peers handling #165

Draft
wants to merge 5 commits into
base: main
Choose a base branch
from

Conversation

vgonkivs
Copy link
Member

@vgonkivs vgonkivs commented Mar 15, 2024

Overview

This PR brings a bunch of improvements to the peer tracker:

  1. removed any limiting. After some time I realized that we don't have to limit our peer tracker by some min score. The current improvement allows us to get rid of gc at all. Peers will be removed only after disconnection.
  2. Moved peer score to the ConnManager. Libp2p provides us with a good mechanism for tracking peers' scores, so I don't see any reason why we shouldn't use it. Now, instead of keeping all peerStats inside the peer tracker, we will store only peers, and constructing peerStats will be on fly.
  3. Added additional events and conditions for tracked peers:
    a. EvtPeerIdentificationCompleted helps to ensure that peer is correct and supports all basic libp2p protocols+ during EvtPeerConnectednessChanged peer store hasn't got information about supported protocols of the incoming peer, so we can't check if the peer supports header-ex protocolID.
    b. EvtPeerProtocolsUpdated allows to detect when peer stop/start supporting header-ex protocol.
    c. Check if the incoming peer supports a header-ex protocol before storing it.
  4. Handled case when the session can be started with an empty peerQueue.
  5. During tests, I randomly caught {err: close called for the closed stream}. This error was received when we were trying to close the stream after the request had been finished(for quic transport). Stream.Close() does stream.CloseRead() + stream.CloseWrite() internally, so I decided to change Close() -> CloseRead(). PS. The issue has gone.

Checklist

  • New and updated code has appropriate documentation
  • New and updated code has new and/or updated testing
  • Required CI checks are passing
  • Visual proof for any user facing features like CLI or documentation updates
  • Linked issues closed with keywords

@vgonkivs vgonkivs self-assigned this Mar 15, 2024
@vgonkivs vgonkivs force-pushed the peer_tracker_improvement branch 2 times, most recently from d1f04df to 6c3038a Compare April 1, 2024 15:05
@vgonkivs vgonkivs marked this pull request as ready for review April 1, 2024 15:08
@vgonkivs vgonkivs marked this pull request as draft April 2, 2024 09:15
@codecov-commenter
Copy link

Codecov Report

Attention: Patch coverage is 67.92453% with 34 lines in your changes are missing coverage. Please review.

Project coverage is 62.11%. Comparing base (6d0f9a4) to head (4f48548).
Report is 1 commits behind head on main.

Files Patch % Lines
p2p/peer_tracker.go 64.00% 19 Missing and 8 partials ⚠️
p2p/exchange.go 62.50% 2 Missing and 1 partial ⚠️
p2p/session.go 70.00% 2 Missing and 1 partial ⚠️
p2p/helpers.go 0.00% 0 Missing and 1 partial ⚠️
Additional details and impacted files
@@            Coverage Diff            @@
##           main     #165       +/-   ##
=========================================
+ Coverage      0   62.11%   +62.11%     
=========================================
  Files         0       39       +39     
  Lines         0     3590     +3590     
=========================================
+ Hits          0     2230     +2230     
- Misses        0     1182     +1182     
- Partials      0      178      +178     

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

@vgonkivs vgonkivs changed the title misc(p2p/peerTracker): extend conditions for peers handling refactoring(p2p/peerTracker): extend conditions for peers handling Apr 3, 2024
@vgonkivs vgonkivs marked this pull request as ready for review April 3, 2024 09:51
@vgonkivs
Copy link
Member Author

vgonkivs commented Apr 5, 2024

moving to draft until libp2p/go-libp2p#2759 is integrated as it brings improvements to the IdentificationEvent that we rely on.

@vgonkivs vgonkivs marked this pull request as draft April 5, 2024 12:50
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants