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

Add Goodbye Message When Disconnecting With Peers #5589

Merged
merged 10 commits into from
Apr 23, 2020
Merged

Conversation

nisdas
Copy link
Member

@nisdas nisdas commented Apr 23, 2020

Resolves #4814

  • We now send goodbye messages to clients, when we disconnect to them in case we hit our peer limit.
  • Also we do not add timeout's to our primary context when passing it down to its sub-methods. This leads to the child methods adopting the parent context's deadlines and leads to early timeouts when performing handshakes with other peers.

cc @q9f , @AgeManning

@nisdas nisdas requested a review from a team as a code owner April 23, 2020 13:50
@nisdas nisdas added Ready For Review A pull request ready for code review Networking P2P related items labels Apr 23, 2020
}
go func() {
log.WithField("reason", "at peer limit").Trace("Ignoring connection request")
if err := goodbyeFunc(context.Background(), 2, conn.RemotePeer()); err != nil {
Copy link
Contributor

Choose a reason for hiding this comment

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

can we please name "2" what is this code?

@prylabs-bulldozer prylabs-bulldozer bot merged commit 2ebd684 into master Apr 23, 2020
@delete-merged-branch delete-merged-branch bot deleted the addGoodbye branch April 23, 2020 19:46
@protolambda
Copy link

My topaz prysm testnet node (on debug log level) has logged hundreds of thousands of lines over the past days, most of which are level=debug msg="Sending Goodbye message to peer" Reason="fault/error" peer=16Uiu2HAm4TsWtLtCmsS4szLRUacQiNFVAcJxZPHjPiDS9XnHLRC6 prefix=sync (with different peer IDs).

Also getting a lot "Failed to decode goodbye stream message" error="stream reset" peer=16Uiu2HAm7XYQgKi27yjvd1G433rJJzpCyrKvzbjeE54DA4n8y6FP prefix=sync topic="/eth2/beacon_chain/req/goodbye/1/ssz_snappy"

Also, the "failed to decode goodbye" sometimes logs up to 8 times, the exact same

Logs excerpt (literally hundreds of thousands of lines of logs, can't share it all, but you get the idea): https://gist.github.com/protolambda/a087de4f44a03deeff0c2a78f50c07ca

Can it be that this functionality is horribly broken in some unexpected way?

@protolambda
Copy link

4 days of running in debug mode:

$ sudo docker logs 8cb989bbd69b 2>&1 | cut -c34- | grep "Goodbye" | wc -l
780398
$ sudo docker logs 8cb989bbd69b 2>&1 | cut -c34- | grep "Sending Goodbye message to peer\" Reason=\"fault/error" | wc -l
780509
$ sudo docker logs 8cb989bbd69b 2>&1 | cut -c34- | grep "Goodbye" | wc -l
780599
$ sudo docker logs 8cb989bbd69b 2>&1 | cut -c34- | wc -l
1889091

That's a lot of log messages...

@prestonvanloon
Copy link
Member

Are you at max peers? You'll kick anyone that tries to connect to you if you have max peers and they are probably retrying anyway.

@prestonvanloon
Copy link
Member

Screenshot from 2020-05-09 14-36-00

Seeing this often in prod too. @nisdas It may be logging too much or sending multiple good bye to the same peer.

@protolambda
Copy link

At max peers yes, around 100 (changed upwards from default). docker image 9a7fbadca570, commit 2eac24c

@protolambda
Copy link

Still, if that's max-peers, and it logs goodbye because of failed connect, then that's almost 800.000 connection attempts to my node in 4 days. So maybe the goodbye code is correct, but the connection attempts are way too much.

@prestonvanloon
Copy link
Member

Confirmed we are logging more than once per peer. Possibly sending multiple goodbye messages. Some peers logging as much as 50+ on a single node.

Screenshot from 2020-05-09 14-42-36

Graph link for @nisdas https://kibana.prylabs.network/goto/873081efd6a6753438a9e8f253e6fd1a

@protolambda
Copy link

protolambda commented May 9, 2020

Even at 50 per peer, that's 16000 connection attempts, over 4 days

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Networking P2P related items Ready For Review A pull request ready for code review
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Nodes Should Send Goodbye Messages When Shutting Down
5 participants