fix(identify): Skip invalid multiaddr in listen_addrs #3246
Conversation
With this commit `libp2p-identify` no longer discards the whole identify payload in case a listen addr of the remote node is invalid, but instead logs the failure, skips the invalid multiaddr and parses the remaining identify payload. > This is especially relevant when rolling out a new protocol to a live network. Say that most nodes > of a network run on an implementation version v1. Say that the `multiaddr` implementation is not > aware of the `webrtc/` protocol. Say that a new version (v2) is rolled out to the network with > support for the `webrtc/` protocol, listening via `webrtc/` by default. In such case all v1 nodes > would discard all identify payloads of v2 nodes, given that the v2 identify payloads would contain > the `webrtc/` protocol in their `listen_addr` addresses. See libp2p#3244 for details.
mxinden
added
the
priority:important
| Ok(a) => a, | ||
| Err(e) => { | ||
| debug!("Unable to parse multiaddr: {e:?}"); | ||
| Multiaddr::empty() |
There was a problem hiding this comment.
This is not ideal but in my eyes not worse than the status quo. Downstream code has to handle a Multiaddr::empty due to the unwrap_or_default anyways.
One might first think that a remote could never return an observed address, i.e. an address that it observes the local node as, that the local node doesn't know. One would expect that the local node supports the multiaddr to all all its supported transport protocols. One exception is a relayed address though. The relay between A and B might use a protocol to B that A does not support nor be able to parse.
There was a problem hiding this comment.
One might first think that a remote could never return an observed address, i.e. an address that it observes the local node as, that the local node doesn't know.
A remote can return all sorts of garbage :)
How big is the impact if we make this field an Option? That would be cleaner IMO.
There was a problem hiding this comment.
Agreed that Option would be the way to go here. That said, I think we should keep the patch release minimal, i.e. only do this on latest master.
Description
With this commit
libp2p-identifyno longer discards the whole identify payload in case a listen addr of the remote node is invalid, but instead logs the failure, skips the invalid multiaddr and parses the remaining identify payload.This is especially relevant when rolling out a new protocol to a live network. Say that most nodes of a network run on an implementation version v1. Say that the
multiaddrimplementation is not aware of thewebrtc/protocol. Say that a new version (v2) is rolled out to the network with support for thewebrtc/protocol, listening viawebrtc/by default. In such case all v1 nodes would discard all identify payloads of v2 nodes, given that the v2 identify payloads would contain thewebrtc/protocol in theirlisten_addraddresses.See #3244 for details.
Notes
v0.50branch such that we can release a patch version oflibp2p-identify.v0.49release family.masterto be included inv0.51.Links to any relevant issues
#3244
Open Questions
Change checklist