rust-libp2p: identify/: Don't fail on unknown multiaddr protocol
Summary
libp2p-identify currently discards the entire identify payload of a remote peer, if that payload contains a multiaddr it can not parse.
Expected behaviour
When parsing an identify payload of a remote peer with an unknown multiaddr protocol (e.g. quic-v1/ or webrtc/) log the parsing error, but don’t discard the entire payload.
Actual behaviour
In libp2p-identify’s parsing logic, it returns an error for the entire payload, in case it can not parse a multiaddr.
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.
//CC @dignifiedquire for the role out of quic-v1/ and webtransport/ on IPFS
//CC @melekes for the role out of webrtc/ and quic-v1/ on Polkadot
//CC @divagant-martian and @AgeManning in case you experiment with quic-v1/ on lighthouse
Possible Solution
Release patch versions of libp2p-identify which log the parsing failure, but don’t discard the entire identify payload.
Version
libp2p <=v0.50.0
Misc
- This was already discovered by @elenaf9 on https://github.com/libp2p/punchr/pull/64 before. I missed that this hard a larger impact beyond punchr.
- Ideally our Testground tests would catch issues like this in the future. Given that our current Testground tests run the
libp2p-pingprotocol only, they have not caught it.
Would you like to work on fixing this bug?
Yes, unless someone else has capacity to do so in the next couple of days.
About this issue
- Original URL
- State: closed
- Created 2 years ago
- Reactions: 3
- Comments: 21 (14 by maintainers)
Commits related to this issue
- fix(identify): Skip invalid multiaddr in listen_addrs 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... — committed to mxinden/rust-libp2p by mxinden 2 years ago
- fix(identify): Skip invalid multiaddr in listen_addrs (#3246) With this commit `libp2p-identify` no longer discards the whole identify payload in case a listen addr of the remote node is invalid, bu... — committed to libp2p/rust-libp2p by mxinden 2 years ago
- fix(kad): Skip invalid multiaddr With this commit `libp2p-kad` no longer discards the whole peer payload in case an addr is invalid, but instead logs the failure, skips the invalid multiaddr and pars... — committed to mxinden/rust-libp2p by mxinden 2 years ago
- fix(identify): Don't fail on unknown multiaddr protocol (#3279) With this commit `libp2p-identify` no longer discards the whole identify payload in case a listen addr of the remote node is invalid, b... — committed to libp2p/rust-libp2p by mxinden 2 years ago
- fix(kad): Skip invalid multiaddr (#3280) With this commit `libp2p-kad` no longer discards the whole peer payload in case an addr is invalid, but instead logs the failure, skips the invalid multiaddr... — committed to libp2p/rust-libp2p by mxinden 2 years ago
- fix(kad): Skip invalid multiaddr (#3280) With this commit `libp2p-kad` no longer discards the whole peer payload in case an addr is invalid, but instead logs the failure, skips the invalid multiaddr ... — committed to mxinden/rust-libp2p by mxinden 2 years ago
- fix(kad): Skip invalid multiaddr (#3284) With this commit `libp2p-kad` no longer discards the whole peer payload in case an addr is invalid, but instead logs the failure, skips the invalid multiaddr ... — committed to libp2p/rust-libp2p by mxinden 2 years ago
- fix(dcutr): Skip unparsable multiaddr (#3280) With this commit `libp2p-dcutr` no longer discards the whole remote payload in case an addr is unparsable, but instead logs the failure and skips the unp... — committed to mxinden/rust-libp2p by mxinden a year ago
- fix(dcutr): Skip unparsable multiaddr (#3300) With this commit `libp2p-dcutr` no longer discards the whole remote payload in case an addr is unparsable, but instead logs the failure and skips the un... — committed to libp2p/rust-libp2p by mxinden a year ago
- fix(dcutr): Skip unparsable multiaddr With this commit `libp2p-dcutr` no longer discards the whole remote payload in case an addr is unparsable, but instead logs the failure and skips the unparsable ... — committed to mxinden/rust-libp2p by mxinden a year ago
- fix(dcutr): Skip unparsable multiaddr (#3320) With this commit `libp2p-dcutr` no longer discards the whole remote payload in case an addr is unparsable, but instead logs the failure and skips the unp... — committed to libp2p/rust-libp2p by mxinden a year ago
- fix(autonat): Skip unparsable multiaddr With this commit `libp2p-autonat` no longer discards the whole remote payload in case an addr is unparsable, but instead logs the failure and skips the unparsa... — committed to mxinden/rust-libp2p by mxinden a year ago
- fix(autonat): Skip unparsable multiaddr With this commit `libp2p-autonat` no longer discards the whole remote payload in case an addr is unparsable, but instead logs the failure and skips the unparsa... — committed to mxinden/rust-libp2p by mxinden a year ago
- fix(autonat): Skip unparsable multiaddr With this commit `libp2p-autonat` no longer discards the whole remote payload in case an addr is unparsable, but instead logs the failure and skips the unparsa... — committed to mxinden/rust-libp2p by mxinden a year ago
- fix(autonat): Skip unparsable multiaddr (#3351) With this commit `libp2p-autonat` no longer discards the whole remote payload in case an addr is unparsable, but instead logs the failure and skips th... — committed to libp2p/rust-libp2p by mxinden a year ago
- fix(autonat): Skip unparsable multiaddr (#3351) With this commit `libp2p-autonat` no longer discards the whole remote payload in case an addr is unparsable, but instead logs the failure and skips the... — committed to jxs/rust-libp2p by mxinden a year ago
- fix(autonat): Skip unparsable multiaddr (#3363) With this commit `libp2p-autonat` no longer discards the whole remote payload in case an addr is unparsable, but instead logs the failure and skips the... — committed to libp2p/rust-libp2p by jxs a year ago
- fix(autonat): Skip unparsable multiaddr (#3363) With this commit `libp2p-autonat` no longer discards the whole remote payload in case an addr is unparsable, but instead logs the failure and skips the... — committed to umgefahren/rust-libp2p by jxs a year ago
In addition to this fix — which I fully agree with — I usually add a FutureCompat variant to all enums representing protocol elements that may be extended in the future (which is basically every enum that gets (de)serialised, unless Moses literally brought the protocol with him from Mt Sinaï). In this particular case, that variant could even contain the string value naming the locally unknown protocol.
The current approach in
multiaddrdoes not foresee such a feature, do you think it makes sense to add a parsing facility that accepts syntactically valid but semantically unknown strings?EDIT: adding a use-case: forwarding multiaddrs should be fine as long as they’re syntactically valid, even when we don’t (yet) understand them.
Hi @mxinden sorry for late repy
We had a discussion internally and decided that it is easiest for us to update libp2p, release a node, wait for a few releases and only then release QUIC/WebRTC. In other words, we don’t need backports, sorry for the trouble
done: https://github.com/multiformats/rust-multiaddr/issues/74
Note that these are 4 crates (identify, kad, dcutr, autonat) across 6 versions (0.45 - 0.50), thus 24 releases in total. Do you really need all those backports? In other words, are these outdated parachains going to upgrade version by version? Note that in case they do upgrade version by version, and do so across all nodes in a parachain, only the version before rolling out a new transport needs this set of patches.
In case you still need all those backports, would you mind creating appropriate backport pull requests like https://github.com/libp2p/rust-libp2p/pull/3246 targeting the release branches?
should we open an issue for this?