network: handle empty wsPeer supplied to transaction handler #6195
Conversation
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## master #6195 +/- ##
==========================================
+ Coverage 51.88% 51.91% +0.02%
==========================================
Files 639 639
Lines 85489 85495 +6
==========================================
+ Hits 44359 44382 +23
+ Misses 38320 38301 -19
- Partials 2810 2812 +2 ☔ View full report in Codecov by Sentry. |
| @@ -282,6 +282,8 @@ type wsPeer struct { | |||
|
|
|||
| // closers is a slice of functions to run when the peer is closed | |||
| closers []func() | |||
| // closersMu synchronizes access to closers | |||
| closersMu deadlock.RWMutex | |||
There was a problem hiding this comment.
Oh interesting, so you spotted a race in wsNetwork when txHandler is processing multiple messages from the same peer
| @@ -979,6 +976,8 @@ L: | |||
| } | |||
|
|
|||
| } | |||
| wp.closersMu.RLock() | |||
| defer wp.closersMu.RUnlock() | |||
| @@ -1115,6 +1114,9 @@ func (wp *wsPeer) sendMessagesOfInterest(messagesOfInterestGeneration uint32, me | |||
| } | |||
|
|
|||
| func (wp *wsPeer) OnClose(f func()) { | |||
| wp.closersMu.Lock() | |||
There was a problem hiding this comment.
Not directly related to this PR, but its' odd to me that the same type has both a Close() and OnClose() with minimal overlap.
There was a problem hiding this comment.
wsPeer.OnClose(f) actually means "register a function f to be called when wsPeer closes."
gossip or gossipSub |
| } | ||
|
|
||
| func (p *gsPeer) RoutingAddr() []byte { | ||
| return []byte(p.peerID) |
There was a problem hiding this comment.
Some day, we could consider calling net.Network().ConnsToPeer(p.peerID), and then getting an IP address here, but it seems like an expensive thing to do per-message, and we are likely going to have a wsPeer conn appear soon anyway that we can get the IP address from.
| @@ -1401,7 +1401,7 @@ func (wn *msgBroadcaster) innerBroadcast(request broadcastRequest, prio bool, pe | |||
| if wn.config.BroadcastConnectionsLimit >= 0 && sentMessageCount >= wn.config.BroadcastConnectionsLimit { | |||
| break | |||
| } | |||
| if peer == request.except { | |||
| if Peer(peer) == request.except { | |||
There was a problem hiding this comment.
This comparison still works right? I forgot we are just doing pointer comparison for except.
There was a problem hiding this comment.
I did a standalone test but happy to write a unit test for this.
Summary
There is a race between pubsub new peer discovery and wsPeer registration:
{"time":"2024-12-04T16:42:43.237595Z","log":"[signal SIGSEGV: segmentation violation code=0x1 addr=0xa0 pc=0x1a46d72]"} {"time":"2024-12-04T16:42:43.237610Z","log":"goroutine 1012678170 [running]:"} {"time":"2024-12-04T16:42:43.237617Z","log":"github.com/algorand/go-algorand/network.(*wsPeer).RoutingAddr(0xc02a57b588?)"} {"time":"2024-12-04T16:42:43.237623Z","log":"\tgithub.com/algorand/go-algorand/network/wsPeer.go:387 +0x12"} {"time":"2024-12-04T16:42:43.237628Z","log":"github.com/algorand/go-algorand/data.(*TxHandler).incomingTxGroupAppRateLimit(0xc0000fec60, {0xc0b02a6008, 0x1, 0x2}, {0x2c51360, 0x0})"} {"time":"2024-12-04T16:42:43.237634Z","log":"\tgithub.com/algorand/go-algorand/data/txHandler.go:722 +0xcd"}Suggested fix is to use
gsPeertemporary type good enough for tx handler.Additional fixes:
Test Plan
Added a test confirming
txTopicValidatordoes not call tx handler with an empty wsPeer.