Fix incorrect GoodPeers count
#17171
Conversation
There was a problem hiding this comment.
Pull Request Overview
Fixes incorrect GoodPeers count by delaying peer creation until after a successful handshake and applying block range updates atomically.
- Move getOrCreatePeer to after handshake completes to avoid leaking peers on failed handshakes.
- Introduce PeerInfo.SetBlockRange to update minBlock and height under a single lock.
- Downgrade block range announcement send failures from error to debug logs.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| p2p/sentry/sentry_multi_client/sentry_multi_client.go | Adjust log level when announcing block range fails. |
| p2p/sentry/sentry_grpc_server.go | Add SetBlockRange, capture handshake data pre-peer creation, apply block range post-handshake, and use SetBlockRange in RPC. |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
| }) | ||
| if err != nil { | ||
| cs.logger.Error("blockRangeUpdate", "err", err) | ||
| cs.logger.Debug("blockRangeUpdate", "err", err) |
There was a problem hiding this comment.
we run with debug logging fairly frequently (in testnets too - it's visible for me locally and in fusaka devnets) so lowering the err to debug is not a complete solution
instead of doing this we should try to check if the sentry has eth69 or higher as a protocol and then attempt to send this msg - then we can also keep the log.Error as it was
to do this you can call s.HandShake - it gives you the relevant info
After eth/69, we were calling `getOrCreatePeer` before the handshake, however in the case of a failed handshake these peers were not being cleaned up - leading to incorrect peer counts in the `GoodPeers` log. Since there is a constraint of setting up `defer`s in the correct order, we create the `peerInfo` after the handshake and store the handshake data in temporary variables.
After eth/69, we were calling
getOrCreatePeerbefore the handshake, however in the case of a failed handshake these peers were not being cleaned up - leading to incorrect peer counts in theGoodPeerslog.Since there is a constraint of setting up
defers in the correct order, we create thepeerInfoafter the handshake and store the handshake data in temporary variables.