go-header icon indicating copy to clipboard operation
go-header copied to clipboard

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

Open vgonkivs opened this issue 1 year ago • 2 comments

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 avatar Mar 15 '24 10:03 vgonkivs

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 :warning:
p2p/exchange.go 62.50% 2 Missing and 1 partial :warning:
p2p/session.go 70.00% 2 Missing and 1 partial :warning:
p2p/helpers.go 0.00% 0 Missing and 1 partial :warning:
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     

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Apr 02 '24 17:04 codecov-commenter

moving to draft until https://github.com/libp2p/go-libp2p/pull/2759 is integrated as it brings improvements to the IdentificationEvent that we rely on.

vgonkivs avatar Apr 05 '24 12:04 vgonkivs