Ignore multiaddresses that can't be parsed in discovery (#705)

* Ignore multiaddresses that can't be parsed in discovery * PR link * Rustfmt
smol-dot · Jun 9, 2023 · daba320 · daba320
1 parent 75a4c72
commit daba320
Show file tree

Hide file tree

Showing 7 changed files with 44 additions and 12 deletions.
diff --git a/full-node/src/run/network_service.rs b/full-node/src/run/network_service.rs
@@ -916,11 +916,25 @@ async fn background_task(mut inner: Inner) {
                             Ok(nodes) => {
                                 log::debug!("discovered; nodes={:?}", nodes);
                                 for (peer_id, addrs) in nodes {
+                                    let mut valid_addrs = Vec::with_capacity(addrs.len());
+                                    for addr in addrs {
+                                        match Multiaddr::try_from(addr) {
+                                            Ok(a) => valid_addrs.push(a),
+                                            Err(err) => {
+                                                log::debug!(
+                                                    "discovery-invalid-address; addr={}",
+                                                    hex::encode(&err.addr)
+                                                );
+                                                continue;
+                                            }
+                                        }
+                                    }
+
                                     inner.network.discover(
                                         &Instant::now(),
                                         chain_index,
                                         peer_id,
-                                        addrs,
+                                        valid_addrs,
                                     );
                                 }
                             }

diff --git a/lib/src/libp2p/multiaddr.rs b/lib/src/libp2p/multiaddr.rs
@@ -153,7 +153,7 @@ impl TryFrom<Vec<u8>> for Multiaddr {
         ))(&bytes)
         .is_err()
         {
-            return Err(FromVecError {});
+            return Err(FromVecError { addr: bytes });
         }
 
         Ok(Multiaddr { bytes })
@@ -178,7 +178,10 @@ impl fmt::Display for Multiaddr {
 
 // TODO: more doc and properly derive Display
 #[derive(Debug, derive_more::Display, Clone, PartialEq, Eq)]
-pub struct FromVecError {}
+#[display(fmt = "Unable to parse multiaddress")]
+pub struct FromVecError {
+    pub addr: Vec<u8>,
+}
 
 // TODO: more doc and properly derive Display
 #[derive(Debug, derive_more::Display, Clone)]

diff --git a/lib/src/network/protocol/kademlia.rs b/lib/src/network/protocol/kademlia.rs
@@ -43,7 +43,7 @@ pub fn build_find_node_request(peer_id: &[u8]) -> Vec<u8> {
 // TODO: return a borrow of the response bytes ; we're limited by protobuf library
 pub fn decode_find_node_response(
     response_bytes: &[u8],
-) -> Result<Vec<(peer_id::PeerId, Vec<multiaddr::Multiaddr>)>, DecodeFindNodeResponseError> {
+) -> Result<Vec<(peer_id::PeerId, Vec<Vec<u8>>)>, DecodeFindNodeResponseError> {
     let mut parser = nom::combinator::all_consuming::<_, _, nom::error::Error<&[u8]>, _>(
         nom::combinator::complete(protobuf::message_decode! {
             #[optional] response_ty = 1 => protobuf::enum_tag_decode,
@@ -71,9 +71,7 @@ pub fn decode_find_node_response(
 
         let mut multiaddrs = Vec::with_capacity(peer.addrs.len());
         for addr in peer.addrs {
-            let addr = multiaddr::Multiaddr::try_from(addr.to_vec())
-                .map_err(DecodeFindNodeResponseError::BadMultiaddr)?;
-            multiaddrs.push(addr);
+            multiaddrs.push(addr.to_vec());
         }
 
         result.push((peer_id, multiaddrs));

diff --git a/lib/src/network/service.rs b/lib/src/network/service.rs
@@ -1274,7 +1274,7 @@ pub enum Event {
 
     KademliaDiscoveryResult {
         operation_id: KademliaOperationId,
-        result: Result<Vec<(PeerId, Vec<multiaddr::Multiaddr>)>, DiscoveryError>,
+        result: Result<Vec<(PeerId, Vec<Vec<u8>>)>, DiscoveryError>,
     },
     /*Transactions {
         peer_id: PeerId,

diff --git a/lib/src/network/service/requests_responses.rs b/lib/src/network/service/requests_responses.rs
@@ -934,9 +934,7 @@ pub enum RequestResult {
     State(Result<EncodedStateResponse, StateRequestError>),
     StorageProof(Result<EncodedMerkleProof, StorageProofRequestError>),
     CallProof(Result<EncodedMerkleProof, CallProofRequestError>),
-    KademliaFindNode(
-        Result<Vec<(peer_id::PeerId, Vec<multiaddr::Multiaddr>)>, KademliaFindNodeError>,
-    ),
+    KademliaFindNode(Result<Vec<(peer_id::PeerId, Vec<Vec<u8>>)>, KademliaFindNodeError>),
 }
 
 /// Undecoded but valid block announce.

diff --git a/light-base/src/network_service.rs b/light-base/src/network_service.rs
@@ -1177,11 +1177,26 @@ async fn update_round<TPlat: PlatformRef>(
                             );
 
                             for (peer_id, addrs) in nodes {
+                                let mut valid_addrs = Vec::with_capacity(addrs.len());
+                                for addr in addrs {
+                                    match Multiaddr::try_from(addr) {
+                                        Ok(a) => valid_addrs.push(a),
+                                        Err(err) => {
+                                            log::debug!(
+                                                target: "connections",
+                                                "Discovery => InvalidAddress({})",
+                                                hex::encode(&err.addr)
+                                            );
+                                            continue;
+                                        }
+                                    }
+                                }
+
                                 guarded.network.discover(
                                     &shared.platform.now(),
                                     chain_index,
                                     peer_id,
-                                    addrs,
+                                    valid_addrs,
                                 );
                             }
                         }

diff --git a/wasm-node/CHANGELOG.md b/wasm-node/CHANGELOG.md
@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### Changed
+
+- Multiaddresses that can't be parsed during the discovery process are now silently ignored instead of causing the entire list of discovered nodes to be discarded. ([#705](https://github.com/smol-dot/smoldot/pull/705))
+
 ## 1.0.9 - 2023-06-08
 
 ### Added