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

Refactor p2p.PeerTracker #2701

Merged
merged 16 commits into from
Feb 10, 2024
Merged

Refactor p2p.PeerTracker #2701

merged 16 commits into from
Feb 10, 2024

Conversation

StephenButtolph
Copy link
Contributor

Why this should be merged

  • Refactors the peer tracker to perform all operations in O(1) time.
  • Removes the peerInfo struct
  • Removes unintuitive handling of TrackBandwidth(..., 0) by explicitly requiring the caller to choose to either call RegisterResponse or RegisterFailure
    • This now also registers the 0 value into the global metrics averager.
  • Improves logging inside of GetAnyPeer (now called SelectPeer).
  • Moves removal of the peer from the bandwidth into RegisterRequest rather than SelectPeer so that SelectPeer can be read-only.
    • This required a slight restructuring of the network client to hold the lock between the call to SelectPeer and RegisterRequest. This was done just to maintain the prior behavior... It could be reverted if we wanted to allow potentially sending multiple requests to the highest bandwidth providing peer.
  • Fixes a minor bug in how bandwidth is calculated. epsilon is documented to avoid division by 0... But it was actually being used to prevent marking a node as unresponsive if it responded with a 0 byte message. Previously we should have been using epsilon in both these cases... Now we only need to use it to prevent the division by 0.

How this works

For the most part - this is a pure refactor.

How this was tested

  • Existing CI

@StephenButtolph StephenButtolph added the networking This involves networking label Feb 2, 2024
@StephenButtolph StephenButtolph added this to the v1.11.0 milestone Feb 2, 2024
@StephenButtolph StephenButtolph self-assigned this Feb 2, 2024
x/sync/network_client.go Outdated Show resolved Hide resolved
network/p2p/peer_tracker.go Outdated Show resolved Hide resolved
network/p2p/peer_tracker.go Show resolved Hide resolved
network/p2p/peer_tracker.go Show resolved Hide resolved
network/p2p/peer_tracker.go Show resolved Hide resolved
network/p2p/peer_tracker.go Outdated Show resolved Hide resolved
network/p2p/peer_tracker.go Show resolved Hide resolved
network/p2p/peer_tracker.go Outdated Show resolved Hide resolved
network/p2p/peer_tracker_test.go Show resolved Hide resolved
network/p2p/peer_tracker.go Show resolved Hide resolved
@StephenButtolph StephenButtolph merged commit 4b24261 into master Feb 10, 2024
16 of 17 checks passed
@StephenButtolph StephenButtolph deleted the cleanup-peer-tracker branch February 10, 2024 19:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
networking This involves networking
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants