downgrade hello logs to debug #2690
Conversation
There was a problem hiding this comment.
This fixes an immediate problem, so ship it. However, I think when we do work to separate networks more strongly (new bootstrap IPs and/or separating libp2p DHTs) then the failure here will indicate something more important.
In order to make this not completely invisible, could we follow up with metrics counting these errors? We could then observe them when desired, and the impact of other fixes.
| @@ -90,11 +90,11 @@ func (h *Handler) handleNewStream(s net.Stream) { | |||
|
|
|||
| switch err := h.processHelloMessage(from, &hello); err { | |||
| case ErrBadGenesis: | |||
| log.Warningf("genesis cid: %s does not match: %s, disconnecting from peer: %s", &hello.GenesisHash, h.genesis, from) | |||
| log.Debugf("genesis cid: %s does not match: %s, disconnecting from peer: %s", &hello.GenesisHash, h.genesis, from) | |||
There was a problem hiding this comment.
Should the "bad hello message" case above also disconnect?
There was a problem hiding this comment.
I don't know. I believe an error for that case indicates a failure to decode the cbor message, my gut says disconnecting is the correct behaviour (it means the peer speaks the hello protocol but sent a bad message). I can add that here as well as the metics since it will be a small change.
There was a problem hiding this comment.
Yeah, I'd say it makes sense to disconnect if you send a bad hello. Not sending a hello at all should be okay though, but if youre gonna play the game, better follow the rules.
|
@anorth we may consider this PR as an alternative to downgrading the logs here: #2690 -- currently seeking review from the libp2p on correct usage of this. Filed issue here: libp2p/go-libp2p#620. Will move forward with this change as is. |
frrist
force-pushed
the
fix/hello-goodbye-errors
branch
from
c5b7322
to
fb402b5
Compare
* downgrade hello logs to debug * closes #2667 * disconnect on failure to read hello message * add error count metrics to hello protocol
closes #2667