search

radicle-dev / radicle-link

The second iteration of the Radicle code collaboration protocol.
Other
421 stars 39 forks source link

Peers sending unsolicited messages are not properly disconnected #715

Open geigerzaehler opened 3 years ago

geigerzaehler commented 3 years ago

When a peer sends unsolicited messages the connection to the peer is not properly terminated under some circumstances.

2021-06-25T17:23:48.797 INFO  librad::net::protocol::broadcast                      > apply; remote_id=hyyao9irufccjs8pjjkijasgdt1ts4toiynkceg3scg58th95mmo5h message=Want { origin: GenericPeerInfo { peer_id: hynkyndc6w3p8urucakobzna7sxwgc
qny7xxtw88dtx3pkf7m3nrzc, advertised_info: PeerAdvertisement { listen_addrs: Within { inner: [10.164.0.8:12345, 127.0.0.1:12345], _min: PhantomData, _max: PhantomData }, capabilities: {} }, seen_addrs: Within { inner: [], _min: PhantomData
, _max: PhantomData } }, val: Payload { urn: Urn { id: Oid(6473a027a10e24148872cda22d67bf831ed32adf), path: Some(RefLike("rad/id")) }, rev: Some(Git(e0ad2de9be92f2f5ea12c71c465307d6f5bed906)), origin: None } }
 2021-06-25T17:23:48.797 ERROR librad::net::protocol::broadcast                      > error=unsolicited message from hyyao9irufccjs8pjjkijasgdt1ts4toiynkceg3scg58th95mmo5h 
 2021-06-25T17:23:48.797 WARN  librad::net::protocol::io::recv::gossip               > unsolicited broadcast message, sending disconnect remote_id=hyyao9irufccjs8pjjkijasgdt1ts4toiynkceg3scg58th95mmo5h 
 2021-06-25T17:23:48.797 WARN  librad::net::protocol::tick                           > reliable send error err=NotConnected { to: hyyao9irufccjs8pjjkijasgdt1ts4toiynkceg3scg58th95mmo5h }

This was witnessed in https://github.com/radicle-dev/radicle-upstream/issues/2010. There, you can also find the complete logs.

alexjg commented 3 years ago

This isn't necessarily an incorrect termination as I understand things. The local peer is trying to send a Disconnect to the remote peer but the remote peer has already gone away. There are a bunch of ways that could happen I think. For example, the remote peer could be spamming unsolicited messages, then being rate limited and therefore dropping the connection on it's end before we're able to send the disconnect.

kim commented 3 years ago

Yes the peer is obviously already disconnected (NotConnected). It's a bit funky that there seems to be a backlog of broadcast messages still being applied. Not sure there's much we can do about this, but perhaps rate-limiting the egress disconnects would be a good idea, so we just drop the message on the floor.

I'm not sure which codebase this is in these days, but I get the impression something is pretty wrong with those wants (it's always the same object they're requesting).