fix(dcutr): Skip unparsable multiaddr (#3280) #3300
Conversation
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 multiaddr. See libp2p#3244 for details.
🤦♂️ Thanks! Mind taking another look @elenaf9? |
| .filter_map(|a| match Multiaddr::try_from(a) { | ||
| Ok(a) => Some(a), | ||
| Err(e) => { | ||
| log::debug!("Unable to parse multiaddr: {e}"); | ||
| None | ||
| } | ||
| }) | ||
| // Filter out relayed addresses. | ||
| .filter(|a| match a { | ||
| Ok(a) => !a.iter().any(|p| p == Protocol::P2pCircuit), | ||
| Err(_) => true, | ||
| .filter(|a| { | ||
| if a.iter().any(|p| p == Protocol::P2pCircuit) { | ||
| log::debug!("Dropping relayed address {a}"); | ||
| false | ||
| } else { | ||
| true | ||
| } | ||
| }) |
There was a problem hiding this comment.
We could merged these two filter blocks:
.filter_map(|a| match Multiaddr::try_from(a) {
Ok(a) if !a.iter().any(|p| p == Protocol::P2pCircuit) => Some(a),
Ok(a) => {
log::debug!("Dropping relayed address {a}");
None
}
Err(e) => {
log::debug!("Unable to parse multiaddr: {e}");
None
}
})(here and below)
No strong opinion.
There was a problem hiding this comment.
What would be the benefit of merging the two?
Structure wise, I find the separation to ease understanding.
There was a problem hiding this comment.
The benefit would be to have a single place where addresses are filtered out. I actually first oversaw that there was a second filter call.
But that's subjective and nit-picky; feel free to ignore 🙂.
There was a problem hiding this comment.
Alternative idea: Keep using .map(Multiaddr::try_from) and use .filter twice for consistency.
There was a problem hiding this comment.
Alternative idea: Keep using
.map(Multiaddr::try_from)and use.filtertwice for consistency.
Don't mind me, that doesn't work because we need to unpack the result somewhere.
Neat trick. That said, is it worth the abstraction given that it is only used twice? I will move forward here, releasing this patch right away. |
Probably not :) |
Description
With this commit
libp2p-dcutrno longer discards the whole remote payload in case an addr is unparsable, but instead logs the failure and skips the unparsable multiaddr.Notes
Links to any relevant issues
See #3244 for details.
Open Questions
Change checklist