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

Fix Issues With Peer Handshakes #5799

Merged
merged 16 commits into from
May 11, 2020
Merged

Fix Issues With Peer Handshakes #5799

merged 16 commits into from
May 11, 2020

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented May 10, 2020

What type of PR is this?

Bug Fix

What does this PR do? Why is it needed?

  • Does not perform status requests on inbound dials, instead waiting for the peer
    to send their status request and then appropriately responding.
  • Prevents multiple handshakes happening for the same peer at once to prevent multiple redundant goodbyes.
  • We add a small delay for each goodbye, so that it gives enough time for the remote peer to be able to read the message.

Which issues(s) does this PR fix?

Fixes #5741 #5796

Other notes for review

@nisdas nisdas requested a review from a team as a code owner May 10, 2020 13:04
@nisdas nisdas added the Ready For Review A pull request ready for code review label May 10, 2020
farazdagi
farazdagi previously approved these changes May 10, 2020
Copy link
Contributor

@farazdagi farazdagi left a comment

Choose a reason for hiding this comment

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

LGTM.
Couple of minor things (feel free to ignore):

  • magic constants (like 10sec to wait for peer to handshake, 50 seconds to flush the stream)
  • not sure if repeatPeerConnections is used anywhere. It is declared in monigoring.go and then incremented, but not sure whether its data is used anywhere.

Copy link
Member

@terencechain terencechain left a comment

Choose a reason for hiding this comment

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

I realize the usual lack of tests for p2p related handles, is this another instance that's due to libp2p?

beacon-chain/p2p/handshake.go Outdated Show resolved Hide resolved
beacon-chain/p2p/handshake.go Outdated Show resolved Hide resolved
@nisdas
Copy link
Member Author

nisdas commented May 10, 2020

@farazdagi alright changed it so as to declare them as constants first. Although I am not sure on this point.

not sure if repeatPeerConnections is used anywhere. It is declared in monigoring.go and then incremented, but not sure whether its data is used anywhere.

This is for our prometheus metrics collection, it usually isn't used anywhere else.

@terencechain Yeap this is a bit complicated to do, as there as many moving parts to this. The handshake process involves code from sync too and has a lot of pre-conditions which will be difficult to write a test for. On top of that testing libp2p related paths are non-trivial.

@codecov
Copy link

codecov bot commented May 10, 2020

Codecov Report

Merging #5799 into master will increase coverage by 0.07%.
The diff coverage is 11.42%.

@@            Coverage Diff             @@
##           master    #5799      +/-   ##
==========================================
+ Coverage   59.75%   59.83%   +0.07%     
==========================================
  Files         311      311              
  Lines       26148    26108      -40     
==========================================
- Hits        15626    15621       -5     
+ Misses       8392     8358      -34     
+ Partials     2130     2129       -1     

@nisdas nisdas merged commit 6840afb into master May 11, 2020
@delete-merged-branch delete-merged-branch bot deleted the improveHandshake branch May 11, 2020 04:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nodes Send Status Requests for Inbound Dials
4 participants