Do not panic when passed an invalid bootnode address

bin/full-node/src/cli.rs

@@ -30,7 +30,13 @@
 //!
 // TODO: I believe this example isn't tested ^ which kills the point of having it
 
-use smoldot::{identity::seed_phrase, libp2p::multiaddr::Multiaddr};
+use smoldot::{
+    identity::seed_phrase,
+    libp2p::{
+        multiaddr::{Multiaddr, ProtocolRef},
+        PeerId,
+    },
+};
 use std::{net::SocketAddr, path::PathBuf};
 
 // Note: the doc-comments applied to this struct and its field are visible when the binary is
@@ -79,8 +85,8 @@ pub struct CliOptionsRun {
     #[structopt(long, parse(try_from_str = decode_multiaddr))]
     pub listen_addr: Vec<Multiaddr>,
     /// Multiaddr of an additional node to try to connect to on startup.
-    #[structopt(long)]
-    pub additional_bootnode: Vec<String>, // TODO: parse the value here
+    #[structopt(long, parse(try_from_str = parse_bootnode))]
+    pub additional_bootnode: Vec<Bootnode>,
     /// Bind point of the JSON-RPC server ("none" or <ip>:<port>).
     #[structopt(long, default_value = "127.0.0.1:9944", parse(try_from_str = parse_json_rpc_address))]
     pub json_rpc_address: JsonRpcAddress,
@@ -204,6 +210,24 @@ fn parse_json_rpc_address(string: &str) -> Result<JsonRpcAddress, String> {
     Err("Failed to parse JSON-RPC server address".into())
 }
 
+#[derive(Debug)]
+pub struct Bootnode {
+    pub address: Multiaddr,
+    pub peer_id: PeerId,
+}
+
+fn parse_bootnode(string: &str) -> Result<Bootnode, String> {
+    let mut address = string.parse::<Multiaddr>().map_err(|err| err.to_string())?;
+    if let Some(ProtocolRef::P2p(peer_id)) = address.iter().last() {
+        let peer_id = PeerId::from_bytes(peer_id.to_vec())
+            .map_err(|(err, _)| format!("Failed to parse PeerId in bootnode: {}", err))?;
+        address.pop();
+        Ok(Bootnode { address, peer_id })
+    } else {
+        Err("Bootnode address must end with /p2p/...".into())
+    }
+}
+
 // `clap` requires error types to implement the `std::error::Error` trait.
 // For this reason, we locally define some wrappers.
 fn decode_ed25519_private_key(phrase: &str) -> Result<[u8; 32], String> {

bin/full-node/src/run.rs

@@ -238,21 +238,33 @@ pub async fn run(cli_options: cli::CliOptionsRun) {
                     })
                     .await,
                 bootstrap_nodes: {
-                    let mut list = Vec::with_capacity(chain_spec.boot_nodes().len());
-                    for node in chain_spec
-                        .boot_nodes()
-                        .iter()
-                        .chain(cli_options.additional_bootnode.iter())
-                    {
-                        let mut address: multiaddr::Multiaddr = node.parse().unwrap(); // TODO: don't unwrap?
-                        if let Some(multiaddr::ProtocolRef::P2p(peer_id)) = address.iter().last() {
-                            let peer_id = PeerId::from_bytes(peer_id.to_vec()).unwrap(); // TODO: don't unwrap
-                            address.pop();
-                            list.push((peer_id, address));
-                        } else {
-                            panic!() // TODO:
+                    let mut list = Vec::with_capacity(
+                        chain_spec.boot_nodes().len() + cli_options.additional_bootnode.len(),
+                    );
+
+                    for node in chain_spec.boot_nodes() {
+                        match node {
+                            chain_spec::Bootnode::UnrecognizedFormat(raw) => {
+                                panic!("Failed to parse bootnode in chain specification: {}", raw)
+                            }
+                            chain_spec::Bootnode::Parsed { multiaddr, peer_id } => {
+                                let multiaddr: multiaddr::Multiaddr = match multiaddr.parse() {
+                                    Ok(a) => a,
+                                    Err(_) => panic!(
+                                        "Failed to parse bootnode in chain specification: {}",
+                                        multiaddr
+                                    ),
+                                };
+                                let peer_id = PeerId::from_bytes(peer_id.to_vec()).unwrap();
+                                list.push((peer_id, multiaddr));
+                            }
                         }
                     }
+
+                    for bootnode in &cli_options.additional_bootnode {
+                        list.push((bootnode.peer_id.clone(), bootnode.address.clone()));
+                    }
+
                     list
                 },
             })
@@ -283,16 +295,22 @@ pub async fn run(cli_options: cli::CliOptionsRun) {
                         bootstrap_nodes: {
                             let mut list =
                                 Vec::with_capacity(relay_chains_specs.boot_nodes().len());
-                            for node in relay_chains_specs.boot_nodes().iter() {
-                                let mut address: multiaddr::Multiaddr = node.parse().unwrap(); // TODO: don't unwrap?
-                                if let Some(multiaddr::ProtocolRef::P2p(peer_id)) =
-                                    address.iter().last()
-                                {
-                                    let peer_id = PeerId::from_bytes(peer_id.to_vec()).unwrap(); // TODO: don't unwrap
-                                    address.pop();
-                                    list.push((peer_id, address));
-                                } else {
-                                    panic!() // TODO:
+                            for node in relay_chains_specs.boot_nodes() {
+                                match node {
+                                    chain_spec::Bootnode::UnrecognizedFormat(raw) => {
+                                        panic!("Failed to parse bootnode in chain specification: {}", raw)
+                                    }
+                                    chain_spec::Bootnode::Parsed { multiaddr, peer_id } => {
+                                        let multiaddr: multiaddr::Multiaddr = match multiaddr.parse() {
+                                            Ok(a) => a,
+                                            Err(_) => panic!(
+                                                "Failed to parse bootnode in chain specification: {}",
+                                                multiaddr
+                                            ),
+                                        };
+                                        let peer_id = PeerId::from_bytes(peer_id.to_vec()).unwrap();
+                                        list.push((peer_id, multiaddr));
+                                    }
                                 }
                             }
                             list

bin/light-base/src/lib.rs

@@ -452,20 +452,35 @@ impl<TChain, TPlat: Platform> Client<TChain, TPlat> {
         };
 
         // Build the list of bootstrap nodes ahead of time.
-        // TODO: either leave this here if it's fallible, or move below if it's not fallible
-        let bootstrap_nodes = {
-            let mut list = Vec::with_capacity(chain_spec.boot_nodes().len());
+        // Because the specification of the format of a multiaddress is a bit flexible, it is
+        // not possible to firmly affirm that a multiaddress is invalid. For this reason, we
+        // simply ignore unparsable bootnode addresses rather than returning an error.
+        // A list of invalid bootstrap node addresses is kept in order to print a warning later
+        // in case it is non-empty. This list is sanitized in order to be safely printable as part
+        // of the logs.
+        let (bootstrap_nodes, invalid_bootstrap_nodes_sanitized) = {
+            let mut valid_list = Vec::with_capacity(chain_spec.boot_nodes().len());
+            let mut invalid_list = Vec::with_capacity(0);
             for node in chain_spec.boot_nodes() {
-                let mut address: multiaddr::Multiaddr = node.parse().unwrap(); // TODO: don't unwrap?
-                if let Some(multiaddr::ProtocolRef::P2p(peer_id)) = address.iter().last() {
-                    let peer_id = peer_id::PeerId::from_bytes(peer_id.to_vec()).unwrap(); // TODO: don't unwrap
-                    address.pop();
-                    list.push((peer_id, vec![address]));
-                } else {
-                    panic!() // TODO:
+                match node {
+                    chain_spec::Bootnode::Parsed { multiaddr, peer_id } => {
+                        if let Ok(multiaddr) = multiaddr.parse::<multiaddr::Multiaddr>() {
+                            let peer_id = peer_id::PeerId::from_bytes(peer_id).unwrap();
+                            valid_list.push((peer_id, vec![multiaddr]));
+                            continue;
+                        } else {
+                            invalid_list.push(multiaddr)
+                        }
+                    }
+                    chain_spec::Bootnode::UnrecognizedFormat(unparsed) => invalid_list.push(
+                        unparsed
+                            .chars()
+                            .filter(|c| c.is_ascii())
+                            .collect::<String>(),
+                    ),
                 }
             }
-            list
+            (valid_list, invalid_list)
         };
 
         // All the checks are performed above. Adding the chain can't fail anymore at this point.
@@ -676,6 +691,15 @@ impl<TChain, TPlat: Platform> Client<TChain, TPlat> {
             }
         };
 
+        if !invalid_bootstrap_nodes_sanitized.is_empty() {
+            log::warn!(
+                target: "smoldot",
+                "Failed to parse some of the bootnodes of {}. \
+                These bootnodes have been ignored. List: {}",
+                log_name, invalid_bootstrap_nodes_sanitized.join(", ")
+            );
+        }
+
         // Print a warning if the list of bootnodes is empty, as this is a common mistake.
         if bootstrap_nodes.is_empty() {
             // Note the usage of the word "likely", because another chain with the same key might

bin/wasm-node/CHANGELOG.md

@@ -2,6 +2,10 @@
 
 ## Unreleased
 
+### Fixed
+
+- No longer panic if passed a chain specification containing an invalid bootnode address. Because the specification of the format of a multiaddress is flexible, invalid bootnode addresses do not trigger a hard error but instead are ignored and a warning is printed. ([#2207](https://github.com/paritytech/smoldot/pull/2207))
+
 ## 0.6.12 - 2022-04-04
 
 ### Fixed

src/chain_spec.rs

@@ -38,8 +38,13 @@ use crate::chain::chain_information::{
     aura_config, babe_genesis_config, grandpa_genesis_config, BabeEpochInformation,
     ChainInformation, ChainInformationConsensus, ChainInformationFinality,
 };
+use crate::libp2p;
 
-use alloc::{borrow::ToOwned as _, string::String, vec::Vec};
+use alloc::{
+    borrow::ToOwned as _,
+    string::{String, ToString as _},
+    vec::Vec,
+};
 use core::num::NonZeroU64;
 
 mod light_sync_state;
@@ -190,10 +195,28 @@ impl ChainSpec {
         }
     }
 
-    /// Returns the list of bootnode addresses in the chain specs.
-    // TODO: more strongly typed?
-    pub fn boot_nodes(&self) -> &[String] {
-        &self.client_spec.boot_nodes
+    /// Returns the list of bootnode addresses found in the chain spec.
+    ///
+    /// Bootnode addresses that have failed to be parsed are returned as well in the form of
+    /// a [`Bootnode::UnrecognizedFormat`].
+    pub fn boot_nodes(&'_ self) -> impl ExactSizeIterator<Item = Bootnode<'_>> + '_ {
+        // Note that we intentionally don't expose types found in the `libp2p` module in order to
+        // not tie the code that parses chain specifications to the libp2p code.
+        self.client_spec.boot_nodes.iter().map(|unparsed| {
+            if let Ok(mut addr) = unparsed.parse::<libp2p::Multiaddr>() {
+                if let Some(libp2p::multiaddr::ProtocolRef::P2p(peer_id)) = addr.iter().last() {
+                    if let Ok(peer_id) = libp2p::peer_id::PeerId::from_bytes(peer_id.to_vec()) {
+                        addr.pop();
+                        return Bootnode::Parsed {
+                            multiaddr: addr.to_string(),
+                            peer_id: peer_id.into_bytes(),
+                        };
+                    }
+                }
+            }
+
+            Bootnode::UnrecognizedFormat(unparsed)
+        })
     }
 
     /// Returns the list of libp2p multiaddresses of the default telemetry servers of the chain.
@@ -263,6 +286,30 @@ impl ChainSpec {
     }
 }
 
+/// See [`ChainSpec::boot_nodes`].
+#[derive(Debug, Clone, PartialEq, Eq)]
+pub enum Bootnode<'a> {
+    /// The address of the bootnode is valid.
+    Parsed {
+        /// String representation of the multiaddr that can be used to reach the bootnode.
+        ///
+        /// Does *not* contain the trailing `/p2p/...`.
+        multiaddr: String,
+
+        /// Bytes representation of the libp2p peer id of the bootnode.
+        ///
+        /// The format can be found in cthe libp2p specification:
+        /// <https://github.com/libp2p/specs/blob/master/peer-ids/peer-ids.md>
+        peer_id: Vec<u8>,
+    },
+
+    /// The address of the bootnode couldn't be parsed.
+    ///
+    /// This could be due to the format being invalid, or to smoldot not supporting one of the
+    /// multiaddress components that is being used.
+    UnrecognizedFormat(&'a str),
+}
+
 /// See [`ChainSpec::genesis_storage`].
 pub enum GenesisStorage<'a> {
     /// The items of the genesis storage are known.
@@ -404,7 +451,7 @@ pub enum FromGenesisStorageError {
 
 #[cfg(test)]
 mod tests {
-    use super::ChainSpec;
+    use super::{Bootnode, ChainSpec};
 
     #[test]
     fn can_decode_polkadot_genesis() {
@@ -415,5 +462,29 @@ mod tests {
         // code_substitutes field
         assert_eq!(specs.client_spec.code_substitutes.get(&1), None);
         assert!(specs.client_spec.code_substitutes.get(&5203203).is_some());
+
+        // bootnodes field
+        assert_eq!(
+            specs.boot_nodes().collect::<Vec<_>>(),
+            vec![
+                Bootnode::Parsed {
+                    multiaddr: "/dns4/p2p.cc1-0.polkadot.network/tcp/30100".into(),
+                    peer_id: vec![
+                        0, 36, 8, 1, 18, 32, 71, 154, 61, 188, 212, 39, 215, 192, 217, 22, 168, 87,
+                        162, 148, 234, 176, 0, 195, 4, 31, 109, 123, 175, 185, 26, 169, 218, 92,
+                        192, 0, 126, 111
+                    ]
+                },
+                Bootnode::Parsed {
+                    multiaddr: "/dns4/cc1-1.parity.tech/tcp/30333".into(),
+                    peer_id: vec![
+                        0, 36, 8, 1, 18, 32, 82, 103, 22, 131, 223, 29, 166, 147, 119, 199, 217,
+                        185, 69, 70, 87, 73, 165, 110, 224, 141, 138, 44, 217, 75, 191, 55, 156,
+                        212, 204, 41, 11, 59
+                    ]
+                },
+                Bootnode::UnrecognizedFormat("/some/wrong/multiaddress")
+            ]
+        );
     }
 }

src/chain_spec/example.json

@@ -4,13 +4,8 @@
   "chainType": "Live",
   "bootNodes": [
     "/dns4/p2p.cc1-0.polkadot.network/tcp/30100/p2p/12D3KooWEdsXX9657ppNqqrRuaCHFvuNemasgU5msLDwSJ6WqsKc",
-    "/dns4/p2p.cc1-1.polkadot.network/tcp/30100/p2p/12D3KooWAtx477KzC8LwqLjWWUG6WF4Gqp2eNXmeqAG98ehAMWYH",
-    "/dns4/p2p.cc1-2.polkadot.network/tcp/30100/p2p/12D3KooWAGCCPZbr9UWGXPtBosTZo91Hb5M3hU8v6xbKgnC5LVao",
-    "/dns4/p2p.cc1-3.polkadot.network/tcp/30100/p2p/12D3KooWJ4eyPowiVcPU46pXuE2cDsiAmuBKXnFcFPapm4xKFdMJ",
-    "/dns4/p2p.cc1-4.polkadot.network/tcp/30100/p2p/12D3KooWNMUcqwSj38oEq1zHeGnWKmMvrCFnpMftw7JzjAtRj2rU",
-    "/dns4/p2p.cc1-5.polkadot.network/tcp/30100/p2p/12D3KooWDs6LnpmWDWgZyGtcLVr3E75CoBxzg1YZUPL5Bb1zz6fM",
-    "/dns4/cc1-0.parity.tech/tcp/30333/p2p/12D3KooWSz8r2WyCdsfWHgPyvD8GKQdJ1UAiRmrcrs8sQB3fe2KU",
-    "/dns4/cc1-1.parity.tech/tcp/30333/p2p/12D3KooWFN2mhgpkJsDBuNuE5427AcDrsib8EoqGMZmkxWwx3Md4"
+    "/dns4/cc1-1.parity.tech/tcp/30333/p2p/12D3KooWFN2mhgpkJsDBuNuE5427AcDrsib8EoqGMZmkxWwx3Md4",
+    "/some/wrong/multiaddress"
   ],
   "telemetryEndpoints": [
     [