From 35ca74f5d0ca538573d049e8f68d84c59505fd0a Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Mon, 3 Oct 2022 15:50:05 -0400 Subject: [PATCH 01/15] feat(p2p): add anchor file for discovery state --- Cargo.lock | 34 +++++++++ crates/net/p2p/Cargo.toml | 5 ++ crates/net/p2p/src/anchor.rs | 134 +++++++++++++++++++++++++++++++++++ crates/net/p2p/src/lib.rs | 2 + 4 files changed, 175 insertions(+) create mode 100644 crates/net/p2p/src/anchor.rs diff --git a/Cargo.lock b/Cargo.lock index 2f8c89b2d5b..51255cba946 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -151,6 +151,12 @@ dependencies = [ "generic-array", ] +[[package]] +name = "bs58" +version = "0.4.0" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "771fe0050b883fcc3ea2359b1a96bcfbc090b7116eae7c3c512c7a083fdf23d3" + [[package]] name = "bstr" version = "0.2.17" @@ -340,12 +346,33 @@ dependencies = [ "ff", "generic-array", "group", + "pkcs8", "rand_core", "sec1", "subtle", "zeroize", ] +[[package]] +name = "enr" +version = "0.6.2" +source = "registry+https://github.com/rust-lang/crates.io-index" +checksum = "26fa0a0be8915790626d5759eb51fe47435a8eac92c2f212bd2da9aa7f30ea56" +dependencies = [ + "base64", + "bs58", + "bytes", + "hex", + "k256", + "log", + "rand", + "rlp", + "secp256k1", + "serde", + "sha3", + "zeroize", +] + [[package]] name = "ethabi" version = "17.2.0" @@ -1427,6 +1454,13 @@ dependencies = [ [[package]] name = "reth-p2p" version = "0.1.0" +dependencies = [ + "enr", + "serde", + "serde_derive", + "thiserror", + "toml", +] [[package]] name = "reth-primitives" diff --git a/crates/net/p2p/Cargo.toml b/crates/net/p2p/Cargo.toml index ac3fef40742..0eee5f62aba 100644 --- a/crates/net/p2p/Cargo.toml +++ b/crates/net/p2p/Cargo.toml @@ -8,3 +8,8 @@ readme = "README.md" description = "Utilities for interacting with ethereum's peer to peer network." [dependencies] +enr = { version = "0.6.2", features = ["serde", "rust-secp256k1"] } +serde = "1.0.145" +serde_derive = "1.0.145" +thiserror = "1.0.37" +toml = "0.5.9" diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs new file mode 100644 index 00000000000..adf5e9416e4 --- /dev/null +++ b/crates/net/p2p/src/anchor.rs @@ -0,0 +1,134 @@ +//! Peer persistence utilities + +use std::{ + fs::{File, OpenOptions}, + io::{BufReader, Read, Write}, + path::Path, +}; + +use enr::{secp256k1::SecretKey, Enr}; +use serde_derive::{Deserialize, Serialize}; +use thiserror::Error; + +// TODO: impl Hash for Enr upstream and change to HashSet to prevent duplicates +// TODO: enforce one-to-one mapping between IP and key + +/// Contains a list of peers to persist across node restarts. +/// +/// Addresses in this list can come from: +/// * Discovery methods (discv4, discv5, dnsdisc) +/// * Known static peers +/// +/// Updates to this list can come from: +/// * Discovery methods (discv4, discv5, dnsdisc) +/// * All peers we connect to from discovery methods are outbound peers. +/// * Inbound connections +/// * Updates for inbound connections must be based on the address advertised in the node record +/// for that peer, if a node record exists. +/// * Updates for inbound connections must NOT be based on the peer's remote address, since its +/// port may be ephemeral. +#[derive(Debug, Clone, Default, Serialize, Deserialize)] +pub struct Anchor { + /// Peers that have been obtained discovery sources when the node is running + pub discovered_peers: Vec>, + + /// Pre-determined peers to reach out to + pub static_peers: Vec>, +} + +impl Anchor { + /// Creates an empty [`Anchor`] + pub fn new() -> Self { + Self::default() + } + + /// Adds peers to the discovered peer list + pub fn add_discovered(&mut self, new_peers: Vec>) { + self.discovered_peers.extend(new_peers); + self.discovered_peers.dedup(); + } + + /// Adds peers to the static peer list. + pub fn add_static(&mut self, new_peers: Vec>) { + self.static_peers.extend(new_peers); + self.static_peers.dedup(); + } +} + +#[derive(Debug, Error)] +/// An error that can occur while parsing a peer list from a file +pub enum AnchorError { + /// Error opening the anchor file + #[error("error opening or writing the anchor file")] + IoError(#[from] std::io::Error), + + /// Error occurred when loading the anchor file from TOML + #[error("error deserializing the peer list from TOML")] + LoadError(#[from] toml::de::Error), + + /// Error occurred when saving the anchor file to TOML + #[error("error serializing the peer list as TOML")] + SaveError(#[from] toml::ser::Error), +} + +/// A version of [`Anchor`] that is loaded from a TOML file and saves its contents when it is +/// dropped. +#[derive(Debug)] +pub struct PersistentAnchor { + /// The list of addresses to persist + anchor: Anchor, + + /// The File handle for the anchor + file: File, +} + +impl PersistentAnchor { + /// This will attempt to load the [`Anchor`] from a file, and if the file doesn't exist it will + /// attempt to initialize it with an empty peer list. + pub fn new_from_file>(path: P) -> Result { + let file = OpenOptions::new().write(true).create(true).open(path)?; + Self::from_toml(file) + } + + /// Load the [`Anchor`] from the given TOML file. + pub fn from_toml(file: File) -> Result { + let mut reader = BufReader::new(&file); + let mut contents = String::new(); + reader.read_to_string(&mut contents)?; + + let anchor: Anchor = toml::from_str(&contents)?; + Ok(Self { anchor, file }) + } + + /// Save the contents of the [`Anchor`] into the associated file as TOML. + pub fn save_toml(&mut self) -> Result<(), AnchorError> { + let anchor_contents = toml::to_vec(&self.anchor)?; + self.file.write_all(anchor_contents.as_ref())?; + Ok(()) + } +} + +impl Drop for PersistentAnchor { + fn drop(&mut self) { + self.save_toml().unwrap() + } +} + +#[cfg(test)] +mod tests { + use super::Anchor; + + #[test] + fn serde_read_toml() { + let _: Anchor = toml::from_str(r#" + discovered_peers = [ + "enr:-Iu4QGuiaVXBEoi4kcLbsoPYX7GTK9ExOODTuqYBp9CyHN_PSDtnLMCIL91ydxUDRPZ-jem-o0WotK6JoZjPQWhTfEsTgmlkgnY0gmlwhDbOLfeJc2VjcDI1NmsxoQLVqNEoCVTC74VmUx25USyFe7lL0TgpXHaCX9CDy9H6boN0Y3CCIyiDdWRwgiMo", + "enr:-Iu4QLNTiVhgyDyvCBnewNcn9Wb7fjPoKYD2NPe-jDZ3_TqaGFK8CcWr7ai7w9X8Im_ZjQYyeoBP_luLLBB4wy39gQ4JgmlkgnY0gmlwhCOhiGqJc2VjcDI1NmsxoQMrmBYg_yR_ZKZKoLiChvlpNqdwXwodXmgw_TRow7RVwYN0Y3CCIyiDdWRwgiMo", + ] + static_peers = [ + "enr:-Iu4QLpJhdfRFsuMrAsFQOSZTIW1PAf7Ndg0GB0tMByt2-n1bwVgLsnHOuujMg-YLns9g1Rw8rfcw1KCZjQrnUcUdekNgmlkgnY0gmlwhA01ZgSJc2VjcDI1NmsxoQPk2OMW7stSjbdcMgrKEdFOLsRkIuxgBFryA3tIJM0YxYN0Y3CCIyiDdWRwgiMo", + "enr:-Iu4QBHuAmMN5ogZP_Mwh_bADnIOS2xqj8yyJI3EbxW66WKtO_JorshNQJ1NY8zo-u3G7HQvGW3zkV6_kRx5d0R19bETgmlkgnY0gmlwhDRCMUyJc2VjcDI1NmsxoQJZ8jY1HYauxirnJkVI32FoN7_7KrE05asCkZb7nj_b-YN0Y3CCIyiDdWRwgiMo", + ] + "#).expect("Parsing valid TOML into an Anchor should not fail"); + } +} diff --git a/crates/net/p2p/src/lib.rs b/crates/net/p2p/src/lib.rs index bec2e0709f2..f4cfb70309d 100644 --- a/crates/net/p2p/src/lib.rs +++ b/crates/net/p2p/src/lib.rs @@ -6,3 +6,5 @@ ))] //! Utilities for interacting with ethereum's peer to peer network. + +pub mod anchor; From e6a62b9c1d52757d9a27da10977763ecba7368e4 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Tue, 4 Oct 2022 13:51:22 -0400 Subject: [PATCH 02/15] move rustdoc and improve error messages Co-authored-by: Bjerg --- crates/net/p2p/src/anchor.rs | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index adf5e9416e4..6b70d10ee83 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -55,19 +55,19 @@ impl Anchor { } } -#[derive(Debug, Error)] /// An error that can occur while parsing a peer list from a file +#[derive(Debug, Error)] pub enum AnchorError { /// Error opening the anchor file - #[error("error opening or writing the anchor file")] + #[error("Could not open or write to the anchor file.")] IoError(#[from] std::io::Error), /// Error occurred when loading the anchor file from TOML - #[error("error deserializing the peer list from TOML")] + #[error("Could not deserialize the peer list from TOML.")] LoadError(#[from] toml::de::Error), /// Error occurred when saving the anchor file to TOML - #[error("error serializing the peer list as TOML")] + #[error("Could not serialize the peer list as TOML.")] SaveError(#[from] toml::ser::Error), } From 34c9c6feafda10fce16bbbcb7c606ec1aafbedb8 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Tue, 4 Oct 2022 14:50:07 -0400 Subject: [PATCH 03/15] add temp file tests and log drop error * fix error due to lack of read option * fix empty and nonexistent file error --- Cargo.lock | 2 + crates/net/p2p/Cargo.toml | 4 ++ crates/net/p2p/src/anchor.rs | 100 +++++++++++++++++++++++++++++++---- 3 files changed, 96 insertions(+), 10 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 51255cba946..e8d94cdf70c 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1456,6 +1456,7 @@ name = "reth-p2p" version = "0.1.0" dependencies = [ "enr", + "secp256k1", "serde", "serde_derive", "thiserror", @@ -1662,6 +1663,7 @@ version = "0.24.0" source = "registry+https://github.com/rust-lang/crates.io-index" checksum = "b7649a0b3ffb32636e60c7ce0d70511eda9c52c658cd0634e194d5a19943aeff" dependencies = [ + "rand", "secp256k1-sys", ] diff --git a/crates/net/p2p/Cargo.toml b/crates/net/p2p/Cargo.toml index 0eee5f62aba..ac6ebed5d48 100644 --- a/crates/net/p2p/Cargo.toml +++ b/crates/net/p2p/Cargo.toml @@ -13,3 +13,7 @@ serde = "1.0.145" serde_derive = "1.0.145" thiserror = "1.0.37" toml = "0.5.9" + +[dev-dependencies.secp256k1] +version = "0.24" +features = ["rand-std"] diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index 6b70d10ee83..54a4544e189 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -2,7 +2,7 @@ use std::{ fs::{File, OpenOptions}, - io::{BufReader, Read, Write}, + io::{Read, Write}, path::Path, }; @@ -27,7 +27,7 @@ use thiserror::Error; /// for that peer, if a node record exists. /// * Updates for inbound connections must NOT be based on the peer's remote address, since its /// port may be ephemeral. -#[derive(Debug, Clone, Default, Serialize, Deserialize)] +#[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct Anchor { /// Peers that have been obtained discovery sources when the node is running pub discovered_peers: Vec>, @@ -53,6 +53,11 @@ impl Anchor { self.static_peers.extend(new_peers); self.static_peers.dedup(); } + + /// Returns true if both peer lists are empty + pub fn is_empty(&self) -> bool { + self.static_peers.is_empty() && self.discovered_peers.is_empty() + } } /// An error that can occur while parsing a peer list from a file @@ -86,15 +91,36 @@ impl PersistentAnchor { /// This will attempt to load the [`Anchor`] from a file, and if the file doesn't exist it will /// attempt to initialize it with an empty peer list. pub fn new_from_file>(path: P) -> Result { - let file = OpenOptions::new().write(true).create(true).open(path)?; + let file = OpenOptions::new().read(true).write(true).create(true).open(path)?; + + // if the file does not exist then we should create it + if file.metadata()?.len() == 0 { + let mut anchor = Self { + anchor: Anchor::default(), + file, + }; + anchor.save_toml()?; + return Ok(anchor) + } + Self::from_toml(file) } /// Load the [`Anchor`] from the given TOML file. - pub fn from_toml(file: File) -> Result { - let mut reader = BufReader::new(&file); + pub fn from_toml(mut file: File) -> Result { let mut contents = String::new(); - reader.read_to_string(&mut contents)?; + file.read_to_string(&mut contents)?; + + // if the file exists but is empty then we should initialize the file format and return an + // empty [`Anchor`] + if contents.is_empty() { + let mut anchor = Self { + anchor: Anchor::default(), + file, + }; + anchor.save_toml()?; + return Ok(anchor) + } let anchor: Anchor = toml::from_str(&contents)?; Ok(Self { anchor, file }) @@ -102,21 +128,28 @@ impl PersistentAnchor { /// Save the contents of the [`Anchor`] into the associated file as TOML. pub fn save_toml(&mut self) -> Result<(), AnchorError> { - let anchor_contents = toml::to_vec(&self.anchor)?; - self.file.write_all(anchor_contents.as_ref())?; + if !self.anchor.is_empty() { + let anchor_contents = toml::to_string_pretty(&self.anchor)?; + self.file.write_all(anchor_contents.as_bytes())?; + } Ok(()) } } impl Drop for PersistentAnchor { fn drop(&mut self) { - self.save_toml().unwrap() + if let Err(save_error) = self.save_toml() { + println!("Could not save anchor to file: {}", save_error) + } } } #[cfg(test)] mod tests { - use super::Anchor; + use std::{fs::{remove_file, OpenOptions}, net::Ipv4Addr}; + use enr::{secp256k1::{SecretKey, rand::thread_rng}, EnrBuilder}; + + use super::{Anchor, PersistentAnchor}; #[test] fn serde_read_toml() { @@ -131,4 +164,51 @@ mod tests { ] "#).expect("Parsing valid TOML into an Anchor should not fail"); } + + #[test] + fn create_empty_anchor() { + let file_name = "temp_anchor.toml"; + let temp_file_path = std::env::temp_dir().with_file_name(file_name); + + // this test's purpose is to make sure new_from_file works if the file doesn't exist + assert!(!temp_file_path.exists()); + let persistent_anchor = PersistentAnchor::new_from_file(&temp_file_path); + + // make sure to clean up + remove_file(temp_file_path).unwrap(); + persistent_anchor.unwrap(); + } + + #[test] + fn save_temp_anchor() { + let file_name = "temp_anchor_two.toml"; + let temp_file_path = std::env::temp_dir().with_file_name(file_name); + let temp_file = OpenOptions::new().read(true).write(true).create(true).open(&temp_file_path).unwrap(); + + let mut persistent_anchor = PersistentAnchor::from_toml(temp_file).unwrap(); + + // add some ENRs to both lists + let mut rng = thread_rng(); + + let key = SecretKey::new(&mut rng); + let ip = Ipv4Addr::new(192,168,1,1); + let enr = EnrBuilder::new("v4").ip4(ip).tcp4(8000).build(&key).unwrap(); + persistent_anchor.anchor.add_discovered(vec![enr]); + + let key = SecretKey::new(&mut rng); + let ip = Ipv4Addr::new(192,168,1,2); + let enr = EnrBuilder::new("v4").ip4(ip).tcp4(8000).build(&key).unwrap(); + persistent_anchor.anchor.add_static(vec![enr]); + + // save the old struct before dropping + let prev_anchor = persistent_anchor.anchor.clone(); + drop(persistent_anchor); + + // finally check file contents + let prev_file = OpenOptions::new().read(true).write(true).create(true).open(&temp_file_path).unwrap(); + let new_anchor = PersistentAnchor::from_toml(prev_file).unwrap(); + + remove_file(temp_file_path).unwrap(); + assert_eq!(new_anchor.anchor, prev_anchor); + } } From 5eee2796cdc3341e4998f0c8bd74dab905a70d13 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Tue, 4 Oct 2022 14:55:50 -0400 Subject: [PATCH 04/15] remove redundant new --- crates/net/p2p/src/anchor.rs | 5 ----- 1 file changed, 5 deletions(-) diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index 54a4544e189..99cdd33d838 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -37,11 +37,6 @@ pub struct Anchor { } impl Anchor { - /// Creates an empty [`Anchor`] - pub fn new() -> Self { - Self::default() - } - /// Adds peers to the discovered peer list pub fn add_discovered(&mut self, new_peers: Vec>) { self.discovered_peers.extend(new_peers); From c1260e05f0c14f63601ebef22fe8ad2d6e7346b4 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Wed, 5 Oct 2022 16:35:25 -0400 Subject: [PATCH 05/15] replace println with tracing --- Cargo.lock | 1 + crates/net/p2p/Cargo.toml | 1 + crates/net/p2p/src/anchor.rs | 3 ++- 3 files changed, 4 insertions(+), 1 deletion(-) diff --git a/Cargo.lock b/Cargo.lock index e8d94cdf70c..dd95d4b43bb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1461,6 +1461,7 @@ dependencies = [ "serde_derive", "thiserror", "toml", + "tracing", ] [[package]] diff --git a/crates/net/p2p/Cargo.toml b/crates/net/p2p/Cargo.toml index ac6ebed5d48..0de4d7fdb4d 100644 --- a/crates/net/p2p/Cargo.toml +++ b/crates/net/p2p/Cargo.toml @@ -13,6 +13,7 @@ serde = "1.0.145" serde_derive = "1.0.145" thiserror = "1.0.37" toml = "0.5.9" +tracing = "0.1.36" [dev-dependencies.secp256k1] version = "0.24" diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index 99cdd33d838..24b04f58c17 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -6,6 +6,7 @@ use std::{ path::Path, }; +use tracing::error; use enr::{secp256k1::SecretKey, Enr}; use serde_derive::{Deserialize, Serialize}; use thiserror::Error; @@ -134,7 +135,7 @@ impl PersistentAnchor { impl Drop for PersistentAnchor { fn drop(&mut self) { if let Err(save_error) = self.save_toml() { - println!("Could not save anchor to file: {}", save_error) + error!("Could not save anchor to file: {}", save_error) } } } From 319524cdc751b858baae486886ba2c60e82d67d7 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Wed, 5 Oct 2022 16:36:28 -0400 Subject: [PATCH 06/15] show underlying error in custom error message --- crates/net/p2p/src/anchor.rs | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index 24b04f58c17..da491e00ebe 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -60,15 +60,15 @@ impl Anchor { #[derive(Debug, Error)] pub enum AnchorError { /// Error opening the anchor file - #[error("Could not open or write to the anchor file.")] + #[error("Could not open or write to the anchor file: {0}")] IoError(#[from] std::io::Error), /// Error occurred when loading the anchor file from TOML - #[error("Could not deserialize the peer list from TOML.")] + #[error("Could not deserialize the peer list from TOML: {0}")] LoadError(#[from] toml::de::Error), /// Error occurred when saving the anchor file to TOML - #[error("Could not serialize the peer list as TOML.")] + #[error("Could not serialize the peer list as TOML: {0}")] SaveError(#[from] toml::ser::Error), } From fd4f0c93c8671048c754e0a9911df95e20d27ca8 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Wed, 5 Oct 2022 23:42:54 -0400 Subject: [PATCH 07/15] chore: cargo fmt --- crates/net/p2p/src/anchor.rs | 32 +++++++++++++++++--------------- 1 file changed, 17 insertions(+), 15 deletions(-) diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index da491e00ebe..0f355ae8e90 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -6,10 +6,10 @@ use std::{ path::Path, }; -use tracing::error; use enr::{secp256k1::SecretKey, Enr}; use serde_derive::{Deserialize, Serialize}; use thiserror::Error; +use tracing::error; // TODO: impl Hash for Enr upstream and change to HashSet to prevent duplicates // TODO: enforce one-to-one mapping between IP and key @@ -91,10 +91,7 @@ impl PersistentAnchor { // if the file does not exist then we should create it if file.metadata()?.len() == 0 { - let mut anchor = Self { - anchor: Anchor::default(), - file, - }; + let mut anchor = Self { anchor: Anchor::default(), file }; anchor.save_toml()?; return Ok(anchor) } @@ -110,10 +107,7 @@ impl PersistentAnchor { // if the file exists but is empty then we should initialize the file format and return an // empty [`Anchor`] if contents.is_empty() { - let mut anchor = Self { - anchor: Anchor::default(), - file, - }; + let mut anchor = Self { anchor: Anchor::default(), file }; anchor.save_toml()?; return Ok(anchor) } @@ -142,8 +136,14 @@ impl Drop for PersistentAnchor { #[cfg(test)] mod tests { - use std::{fs::{remove_file, OpenOptions}, net::Ipv4Addr}; - use enr::{secp256k1::{SecretKey, rand::thread_rng}, EnrBuilder}; + use enr::{ + secp256k1::{rand::thread_rng, SecretKey}, + EnrBuilder, + }; + use std::{ + fs::{remove_file, OpenOptions}, + net::Ipv4Addr, + }; use super::{Anchor, PersistentAnchor}; @@ -179,7 +179,8 @@ mod tests { fn save_temp_anchor() { let file_name = "temp_anchor_two.toml"; let temp_file_path = std::env::temp_dir().with_file_name(file_name); - let temp_file = OpenOptions::new().read(true).write(true).create(true).open(&temp_file_path).unwrap(); + let temp_file = + OpenOptions::new().read(true).write(true).create(true).open(&temp_file_path).unwrap(); let mut persistent_anchor = PersistentAnchor::from_toml(temp_file).unwrap(); @@ -187,12 +188,12 @@ mod tests { let mut rng = thread_rng(); let key = SecretKey::new(&mut rng); - let ip = Ipv4Addr::new(192,168,1,1); + let ip = Ipv4Addr::new(192, 168, 1, 1); let enr = EnrBuilder::new("v4").ip4(ip).tcp4(8000).build(&key).unwrap(); persistent_anchor.anchor.add_discovered(vec![enr]); let key = SecretKey::new(&mut rng); - let ip = Ipv4Addr::new(192,168,1,2); + let ip = Ipv4Addr::new(192, 168, 1, 2); let enr = EnrBuilder::new("v4").ip4(ip).tcp4(8000).build(&key).unwrap(); persistent_anchor.anchor.add_static(vec![enr]); @@ -201,7 +202,8 @@ mod tests { drop(persistent_anchor); // finally check file contents - let prev_file = OpenOptions::new().read(true).write(true).create(true).open(&temp_file_path).unwrap(); + let prev_file = + OpenOptions::new().read(true).write(true).create(true).open(&temp_file_path).unwrap(); let new_anchor = PersistentAnchor::from_toml(prev_file).unwrap(); remove_file(temp_file_path).unwrap(); From 02841755f1919d683b29574063d8fd748dcf0aff Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Thu, 6 Oct 2022 12:27:52 -0400 Subject: [PATCH 08/15] change AsRef to &Path --- crates/net/p2p/src/anchor.rs | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index 0f355ae8e90..677a44dc870 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -2,8 +2,7 @@ use std::{ fs::{File, OpenOptions}, - io::{Read, Write}, - path::Path, + io::{Read, Write}, path::Path, }; use enr::{secp256k1::SecretKey, Enr}; @@ -86,7 +85,7 @@ pub struct PersistentAnchor { impl PersistentAnchor { /// This will attempt to load the [`Anchor`] from a file, and if the file doesn't exist it will /// attempt to initialize it with an empty peer list. - pub fn new_from_file>(path: P) -> Result { + pub fn new_from_file(path: &Path) -> Result { let file = OpenOptions::new().read(true).write(true).create(true).open(path)?; // if the file does not exist then we should create it From d9347a61d3ee7e7da79f311329d1d583e683fe74 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Thu, 6 Oct 2022 12:39:32 -0400 Subject: [PATCH 09/15] remove ineffective dedups --- crates/net/p2p/src/anchor.rs | 2 -- 1 file changed, 2 deletions(-) diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index 677a44dc870..d6bb2dab68b 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -40,13 +40,11 @@ impl Anchor { /// Adds peers to the discovered peer list pub fn add_discovered(&mut self, new_peers: Vec>) { self.discovered_peers.extend(new_peers); - self.discovered_peers.dedup(); } /// Adds peers to the static peer list. pub fn add_static(&mut self, new_peers: Vec>) { self.static_peers.extend(new_peers); - self.static_peers.dedup(); } /// Returns true if both peer lists are empty From 64a752aec6cff8e47b94bc74da377a6bbbe33cec Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Thu, 6 Oct 2022 14:45:53 -0400 Subject: [PATCH 10/15] chore: cargo fmt --- crates/net/p2p/src/anchor.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index d6bb2dab68b..0914516f898 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -2,7 +2,8 @@ use std::{ fs::{File, OpenOptions}, - io::{Read, Write}, path::Path, + io::{Read, Write}, + path::Path, }; use enr::{secp256k1::SecretKey, Enr}; From f4d5e24634037ac1f68d41160dbdd64d35ad273d Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Fri, 7 Oct 2022 17:26:23 -0400 Subject: [PATCH 11/15] switch out Vec> for HashSet> --- Cargo.lock | 3 +-- crates/net/p2p/Cargo.toml | 2 +- crates/net/p2p/src/anchor.rs | 7 +++---- 3 files changed, 5 insertions(+), 7 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index dd95d4b43bb..cbc61d6ebb0 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -356,8 +356,7 @@ dependencies = [ [[package]] name = "enr" version = "0.6.2" -source = "registry+https://github.com/rust-lang/crates.io-index" -checksum = "26fa0a0be8915790626d5759eb51fe47435a8eac92c2f212bd2da9aa7f30ea56" +source = "git+https://github.com/sigp/enr#125f8a5f2deede3e47e852ea70fc9b6e1e9c6e50" dependencies = [ "base64", "bs58", diff --git a/crates/net/p2p/Cargo.toml b/crates/net/p2p/Cargo.toml index 0de4d7fdb4d..151eef43759 100644 --- a/crates/net/p2p/Cargo.toml +++ b/crates/net/p2p/Cargo.toml @@ -8,7 +8,7 @@ readme = "README.md" description = "Utilities for interacting with ethereum's peer to peer network." [dependencies] -enr = { version = "0.6.2", features = ["serde", "rust-secp256k1"] } +enr = { git = "https://github.com/sigp/enr", features = ["serde", "rust-secp256k1"] } serde = "1.0.145" serde_derive = "1.0.145" thiserror = "1.0.37" diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index 0914516f898..6d2a1556cb4 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -3,7 +3,7 @@ use std::{ fs::{File, OpenOptions}, io::{Read, Write}, - path::Path, + path::Path, collections::HashSet, }; use enr::{secp256k1::SecretKey, Enr}; @@ -11,7 +11,6 @@ use serde_derive::{Deserialize, Serialize}; use thiserror::Error; use tracing::error; -// TODO: impl Hash for Enr upstream and change to HashSet to prevent duplicates // TODO: enforce one-to-one mapping between IP and key /// Contains a list of peers to persist across node restarts. @@ -31,10 +30,10 @@ use tracing::error; #[derive(Debug, Clone, Default, PartialEq, Eq, Serialize, Deserialize)] pub struct Anchor { /// Peers that have been obtained discovery sources when the node is running - pub discovered_peers: Vec>, + pub discovered_peers: HashSet>, /// Pre-determined peers to reach out to - pub static_peers: Vec>, + pub static_peers: HashSet>, } impl Anchor { From 95f8ef100b57561cfc5822fcc847dbb11c8acb7e Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Fri, 7 Oct 2022 17:33:40 -0400 Subject: [PATCH 12/15] cargo fmt --- crates/net/p2p/src/anchor.rs | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index 6d2a1556cb4..7d7010dcf1f 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -1,9 +1,10 @@ //! Peer persistence utilities use std::{ + collections::HashSet, fs::{File, OpenOptions}, io::{Read, Write}, - path::Path, collections::HashSet, + path::Path, }; use enr::{secp256k1::SecretKey, Enr}; From 925be648283a82f0ceea28bda9eccdbc76df76fd Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Fri, 7 Oct 2022 19:56:43 -0400 Subject: [PATCH 13/15] use tempdir instead of of std::env::temp_dir --- Cargo.lock | 1 + crates/net/p2p/Cargo.toml | 3 +++ crates/net/p2p/src/anchor.rs | 5 +++-- 3 files changed, 7 insertions(+), 2 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index cbc61d6ebb0..73317d3f632 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -1458,6 +1458,7 @@ dependencies = [ "secp256k1", "serde", "serde_derive", + "tempfile", "thiserror", "toml", "tracing", diff --git a/crates/net/p2p/Cargo.toml b/crates/net/p2p/Cargo.toml index 151eef43759..c0c8395781a 100644 --- a/crates/net/p2p/Cargo.toml +++ b/crates/net/p2p/Cargo.toml @@ -15,6 +15,9 @@ thiserror = "1.0.37" toml = "0.5.9" tracing = "0.1.36" +[dev-dependencies] +tempfile = "3.3.0" + [dev-dependencies.secp256k1] version = "0.24" features = ["rand-std"] diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index 7d7010dcf1f..f95c9145d3e 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -142,6 +142,7 @@ mod tests { fs::{remove_file, OpenOptions}, net::Ipv4Addr, }; + use tempfile::tempdir; use super::{Anchor, PersistentAnchor}; @@ -162,7 +163,7 @@ mod tests { #[test] fn create_empty_anchor() { let file_name = "temp_anchor.toml"; - let temp_file_path = std::env::temp_dir().with_file_name(file_name); + let temp_file_path = tempdir().unwrap().path().with_file_name(file_name); // this test's purpose is to make sure new_from_file works if the file doesn't exist assert!(!temp_file_path.exists()); @@ -176,7 +177,7 @@ mod tests { #[test] fn save_temp_anchor() { let file_name = "temp_anchor_two.toml"; - let temp_file_path = std::env::temp_dir().with_file_name(file_name); + let temp_file_path = tempdir().unwrap().path().with_file_name(file_name); let temp_file = OpenOptions::new().read(true).write(true).create(true).open(&temp_file_path).unwrap(); From af66121f374aa821bca33aee8b02b644b2612788 Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Mon, 10 Oct 2022 11:31:58 -0400 Subject: [PATCH 14/15] refactor anchor to contain &Path instead of File * change new_from_file to explicitly include logic for opening existing files, rather than calling out to from_toml * remove from_toml because new_from_file handles existing files properly with only a path. It is not possible to obtain a Path from a File anyways, its only purpose was to accept a File type --- crates/net/p2p/src/anchor.rs | 72 +++++++++++++++++------------------- 1 file changed, 34 insertions(+), 38 deletions(-) diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index f95c9145d3e..32a47462f94 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -2,7 +2,7 @@ use std::{ collections::HashSet, - fs::{File, OpenOptions}, + fs::OpenOptions, io::{Read, Write}, path::Path, }; @@ -73,58 +73,55 @@ pub enum AnchorError { /// A version of [`Anchor`] that is loaded from a TOML file and saves its contents when it is /// dropped. #[derive(Debug)] -pub struct PersistentAnchor { +pub struct PersistentAnchor<'a> { /// The list of addresses to persist anchor: Anchor, - /// The File handle for the anchor - file: File, + /// The Path to store the anchor file + path: &'a Path, } -impl PersistentAnchor { +impl<'a> PersistentAnchor<'a> { /// This will attempt to load the [`Anchor`] from a file, and if the file doesn't exist it will /// attempt to initialize it with an empty peer list. - pub fn new_from_file(path: &Path) -> Result { - let file = OpenOptions::new().read(true).write(true).create(true).open(path)?; + pub fn new_from_file(path: &'a Path) -> Result { + let mut binding = OpenOptions::new(); + let rw_opts = binding.read(true).write(true); - // if the file does not exist then we should create it - if file.metadata()?.len() == 0 { - let mut anchor = Self { anchor: Anchor::default(), file }; - anchor.save_toml()?; - return Ok(anchor) - } - - Self::from_toml(file) - } + let mut file = if path.try_exists()? { + rw_opts.open(path)? + } else { + rw_opts.create(true).open(path)? + }; - /// Load the [`Anchor`] from the given TOML file. - pub fn from_toml(mut file: File) -> Result { let mut contents = String::new(); file.read_to_string(&mut contents)?; - // if the file exists but is empty then we should initialize the file format and return an - // empty [`Anchor`] + // if the file exists but is empty then we should initialize the file format and return + // an empty [`Anchor`] if contents.is_empty() { - let mut anchor = Self { anchor: Anchor::default(), file }; + let mut anchor = Self { anchor: Anchor::default(), path }; anchor.save_toml()?; return Ok(anchor) } let anchor: Anchor = toml::from_str(&contents)?; - Ok(Self { anchor, file }) + Ok(Self { anchor, path }) } /// Save the contents of the [`Anchor`] into the associated file as TOML. pub fn save_toml(&mut self) -> Result<(), AnchorError> { + let mut file = OpenOptions::new().read(true).write(true).create(true).open(self.path)?; + if !self.anchor.is_empty() { let anchor_contents = toml::to_string_pretty(&self.anchor)?; - self.file.write_all(anchor_contents.as_bytes())?; + file.write_all(anchor_contents.as_bytes())?; } Ok(()) } } -impl Drop for PersistentAnchor { +impl Drop for PersistentAnchor<'_> { fn drop(&mut self) { if let Err(save_error) = self.save_toml() { error!("Could not save anchor to file: {}", save_error) @@ -138,10 +135,7 @@ mod tests { secp256k1::{rand::thread_rng, SecretKey}, EnrBuilder, }; - use std::{ - fs::{remove_file, OpenOptions}, - net::Ipv4Addr, - }; + use std::{fs::remove_file, net::Ipv4Addr}; use tempfile::tempdir; use super::{Anchor, PersistentAnchor}; @@ -170,18 +164,19 @@ mod tests { let persistent_anchor = PersistentAnchor::new_from_file(&temp_file_path); // make sure to clean up - remove_file(temp_file_path).unwrap(); - persistent_anchor.unwrap(); + let anchor = persistent_anchor.unwrap(); + + // need to drop the PersistentAnchor explicitly before cleanup or it will be saved + drop(anchor); + remove_file(&temp_file_path).unwrap(); } #[test] fn save_temp_anchor() { let file_name = "temp_anchor_two.toml"; let temp_file_path = tempdir().unwrap().path().with_file_name(file_name); - let temp_file = - OpenOptions::new().read(true).write(true).create(true).open(&temp_file_path).unwrap(); - let mut persistent_anchor = PersistentAnchor::from_toml(temp_file).unwrap(); + let mut persistent_anchor = PersistentAnchor::new_from_file(&temp_file_path).unwrap(); // add some ENRs to both lists let mut rng = thread_rng(); @@ -201,11 +196,12 @@ mod tests { drop(persistent_anchor); // finally check file contents - let prev_file = - OpenOptions::new().read(true).write(true).create(true).open(&temp_file_path).unwrap(); - let new_anchor = PersistentAnchor::from_toml(prev_file).unwrap(); + let new_persistent = PersistentAnchor::new_from_file(&temp_file_path).unwrap(); + let new_anchor = new_persistent.anchor.clone(); - remove_file(temp_file_path).unwrap(); - assert_eq!(new_anchor.anchor, prev_anchor); + // need to drop the PersistentAnchor explicitly before cleanup or it will be saved + drop(new_persistent); + remove_file(&temp_file_path).unwrap(); + assert_eq!(new_anchor, prev_anchor); } } From a67730d0dd6c2fb5e352d710b62a08da197cee5e Mon Sep 17 00:00:00 2001 From: Dan Cline <6798349+Rjected@users.noreply.github.com> Date: Mon, 10 Oct 2022 12:45:52 -0400 Subject: [PATCH 15/15] use PathBuf instead of Path --- crates/net/p2p/src/anchor.rs | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/crates/net/p2p/src/anchor.rs b/crates/net/p2p/src/anchor.rs index 32a47462f94..eb6881743d9 100644 --- a/crates/net/p2p/src/anchor.rs +++ b/crates/net/p2p/src/anchor.rs @@ -4,7 +4,7 @@ use std::{ collections::HashSet, fs::OpenOptions, io::{Read, Write}, - path::Path, + path::{Path, PathBuf}, }; use enr::{secp256k1::SecretKey, Enr}; @@ -73,18 +73,18 @@ pub enum AnchorError { /// A version of [`Anchor`] that is loaded from a TOML file and saves its contents when it is /// dropped. #[derive(Debug)] -pub struct PersistentAnchor<'a> { +pub struct PersistentAnchor { /// The list of addresses to persist anchor: Anchor, /// The Path to store the anchor file - path: &'a Path, + path: PathBuf, } -impl<'a> PersistentAnchor<'a> { +impl PersistentAnchor { /// This will attempt to load the [`Anchor`] from a file, and if the file doesn't exist it will /// attempt to initialize it with an empty peer list. - pub fn new_from_file(path: &'a Path) -> Result { + pub fn new_from_file(path: &Path) -> Result { let mut binding = OpenOptions::new(); let rw_opts = binding.read(true).write(true); @@ -100,18 +100,18 @@ impl<'a> PersistentAnchor<'a> { // if the file exists but is empty then we should initialize the file format and return // an empty [`Anchor`] if contents.is_empty() { - let mut anchor = Self { anchor: Anchor::default(), path }; + let mut anchor = Self { anchor: Anchor::default(), path: path.to_path_buf() }; anchor.save_toml()?; return Ok(anchor) } let anchor: Anchor = toml::from_str(&contents)?; - Ok(Self { anchor, path }) + Ok(Self { anchor, path: path.to_path_buf() }) } /// Save the contents of the [`Anchor`] into the associated file as TOML. pub fn save_toml(&mut self) -> Result<(), AnchorError> { - let mut file = OpenOptions::new().read(true).write(true).create(true).open(self.path)?; + let mut file = OpenOptions::new().read(true).write(true).create(true).open(&self.path)?; if !self.anchor.is_empty() { let anchor_contents = toml::to_string_pretty(&self.anchor)?; @@ -121,7 +121,7 @@ impl<'a> PersistentAnchor<'a> { } } -impl Drop for PersistentAnchor<'_> { +impl Drop for PersistentAnchor { fn drop(&mut self) { if let Err(save_error) = self.save_toml() { error!("Could not save anchor to file: {}", save_error)