Prysm: Nodes Should Send Goodbye Messages When Shutting Down

[nisdas]

Currently when we shut down our node, we just disconnect with all our peers at once. The proper way would be to send a goodbye message to them and then disconnect with them

[rauljordan]

Is this solved @nisdas ? I recall we just merged in a PR regarding a panic in the goodbye message

[nisdas]

hey no @rauljordan not yet, that PR was just to fix a panic when receiving goodbye messages

[prestonvanloon]

I'd recommend this is part of the p2p service Stop() method.

https://github.com/prysmaticlabs/prysm/blob/acb15a1f047ced45ac72eec0709dd02a04d0fd1c/beacon-chain/p2p/service.go#L232-L239

[lsankar4033]

If this is still open, I'd be interested in taking it!

[prestonvanloon]

It's still open. Thanks!

>

[lsankar4033]

I noticed that the goodbye code for client shutdown is currently unexported (https://github.com/prysmaticlabs/prysm/blob/master/beacon-chain/sync/rpc_goodbye.go#L12). Is there another way to refer to this that I should be using?

I was planning on using Service.Send for these goodbye messages like so: https://github.com/lsankar4033/prysm/blob/send-goodbye-message-on-shutdown/beacon-chain/p2p/service.go#L238

[rauljordan]

Feel free to export what you sensibly need to complete the feature @lsankar4033. Also, yes we should be handling the error message result from service.Send

[rauljordan]

Hi @lsankar4033 are you still interested in this issue?

[lsankar4033]

yep! sorry for slow-ness; have been a bit overloaded on other things past couple weeks.

I should have this in by this week

[lsankar4033]

is there a nice way in tests check if a host (i.e. created with this method) has received a given libp2p or rpc message?

trying to figure out how to write the unit test

[nisdas]

hey @lsankar4033 for testing rpc methods we use a pattern like this. https://github.com/prysmaticlabs/prysm/blob/master/beacon-chain/sync/rpc_goodbye_test.go#L16

Basically we initialize 2 test p2p services and use that to perform any testing between peers to ensure that messages are received/responded to.

[lsankar4033]

oh lol, I just created a PR for this, but it looks like you guys were already on it :)

although for my understanding @nisdas - it doesn't seem that you send goodbyes on every case of Service.Stop in your PR; isn't that the desired underlying functionality?

[lsankar4033]

I've been a little underwater with other things and haven't had time to resolve this. If it's pressing, feel free to re-assign!