From 80cf8ca9f31f35b37768c5d1f967b65c8516507e Mon Sep 17 00:00:00 2001 From: Kolby Moroz Liebl <31669092+KolbyML@users.noreply.github.com> Date: Wed, 4 Oct 2023 16:08:57 -0600 Subject: [PATCH] fix: enr signature changes on restart even if enr content is the same (#952) --- portalnet/src/discovery.rs | 61 +++++++++++++++----------------- portalnet/src/overlay_service.rs | 11 +++--- portalnet/src/types/messages.rs | 3 -- portalnet/tests/overlay.rs | 13 ++++--- rpc/src/rpc_server.rs | 4 ++- src/lib.rs | 3 +- utp-testing/src/lib.rs | 4 ++- 7 files changed, 51 insertions(+), 48 deletions(-) diff --git a/portalnet/src/discovery.rs b/portalnet/src/discovery.rs index d11acf72a..c28df5b25 100644 --- a/portalnet/src/discovery.rs +++ b/portalnet/src/discovery.rs @@ -3,7 +3,6 @@ use crate::socket; use anyhow::anyhow; use async_trait::async_trait; use bytes::BytesMut; -use discv5::enr::EnrPublicKey; use discv5::{ enr::{CombinedKey, EnrBuilder, NodeId}, ConfigBuilder, Discv5, Event, ListenConfig, RequestError, TalkRequest, @@ -17,6 +16,7 @@ use rlp::RlpStream; use serde_json::{json, Value}; use std::hash::{Hash, Hasher}; use std::net::Ipv4Addr; +use std::path::PathBuf; use std::str::FromStr; use std::{convert::TryFrom, fmt, fs, io, net::SocketAddr, sync::Arc}; use tokio::sync::mpsc; @@ -69,7 +69,7 @@ impl fmt::Debug for Discovery { } impl Discovery { - pub fn new(portal_config: PortalnetConfig) -> Result { + pub fn new(portal_config: PortalnetConfig, node_data_dir: PathBuf) -> Result { let listen_all_ips = SocketAddr::new( "0.0.0.0" .parse() @@ -112,27 +112,28 @@ impl Discovery { }; // Check if we have an old version of our Enr and if we do, increase our sequence number - if let Some(enr_file_location) = portal_config.enr_file_location { - let trin_enr_file_location = enr_file_location.join(ENR_FILE_NAME); - if trin_enr_file_location.is_file() { - let data = fs::read_to_string(trin_enr_file_location.clone()) - .expect("Unable to read Trin Enr from file"); - let old_enr = Enr::from_str(&data).expect("Expected read trin.enr to be valid"); - - enr.set_seq(old_enr.seq(), &enr_key) - .expect("Unable to set Enr sequence number"); - - // If the old Enr's signature is different then the new one - if !verify_by_signature(&enr, old_enr.signature()) - && !verify_by_signature(&old_enr, enr.signature()) - { - enr.set_seq(old_enr.seq() + 1, &enr_key) - .expect("Unable to increase Enr sequence number"); - } + let trin_enr_path = node_data_dir.join(ENR_FILE_NAME); + if trin_enr_path.is_file() { + let data = fs::read_to_string(trin_enr_path.clone()) + .expect("Unable to read Trin Enr from file"); + let old_enr = Enr::from_str(&data).expect("Expected to read valid Trin Enr from file"); + enr.set_seq(old_enr.seq(), &enr_key) + .expect("Unable to set Enr sequence number"); + + // If the content is different then increase the sequence number + if get_enr_rlp_content(&enr) != get_enr_rlp_content(&old_enr) { + enr.set_seq(old_enr.seq() + 1, &enr_key) + .expect("Unable to increase Enr sequence number"); + fs::write(trin_enr_path, enr.to_base64()) + .expect("Unable to update Trin Enr to file"); + } else { + // the content is the same, we don't want to change signatures on restart + // so set enr to old one to keep the same signature per sequence number + enr = old_enr; } + } else { // Write enr to disk - fs::write(trin_enr_file_location, enr.to_base64()) - .expect("Unable to write Trin Enr to file"); + fs::write(trin_enr_path, enr.to_base64()).expect("Unable to write Trin Enr to file"); } let listen_config = ListenConfig::Ipv4 { @@ -425,7 +426,7 @@ impl AsyncUdpSocket for Discv5UdpSocket { // todo: remove this once sigp/enr implements this for enr's // we need this because signatures can be different for the same data but still valid -fn verify_by_signature(enr: &Enr, signature: &[u8]) -> bool { +fn get_enr_rlp_content(enr: &Enr) -> BytesMut { match enr.id() { Some(ref id) if id == "v4" => { let mut stream = RlpStream::new_with_buffer(BytesMut::with_capacity(300)); @@ -439,10 +440,10 @@ fn verify_by_signature(enr: &Enr, signature: &[u8]) -> bool { stream.append_raw(v, 1); } - enr.public_key().verify_v4(&stream.out(), signature) + stream.out() } // unsupported identity schemes - _ => false, + _ => BytesMut::with_capacity(0), } } @@ -463,7 +464,6 @@ mod tests { let mut portalnet_config = PortalnetConfig { private_key, bootnodes: Bootnodes::None, - enr_file_location: Some(node_data_dir.clone()), ..Default::default() }; @@ -472,7 +472,7 @@ mod tests { assert!(!trin_enr_file_location.is_file()); // test trin.enr is made on first run - let discovery = Discovery::new(portalnet_config.clone()).unwrap(); + let discovery = Discovery::new(portalnet_config.clone(), node_data_dir.clone()).unwrap(); let data = fs::read_to_string(trin_enr_file_location.clone()).unwrap(); let old_enr = Enr::from_str(&data).unwrap(); assert_eq!(discovery.local_enr(), old_enr); @@ -480,7 +480,7 @@ mod tests { // test if Enr changes the Enr sequence is increased and if it is written to disk portalnet_config.listen_port = 2424; - let discovery = Discovery::new(portalnet_config.clone()).unwrap(); + let discovery = Discovery::new(portalnet_config.clone(), node_data_dir.clone()).unwrap(); assert_ne!(discovery.local_enr(), old_enr); let data = fs::read_to_string(trin_enr_file_location.clone()).unwrap(); let old_enr = Enr::from_str(&data).unwrap(); @@ -489,11 +489,8 @@ mod tests { assert_eq!(discovery.local_enr(), old_enr); // test if the enr isn't changed that it's sequence stays the same - let discovery = Discovery::new(portalnet_config).unwrap(); - assert!( - verify_by_signature(&discovery.local_enr(), old_enr.signature()) - && verify_by_signature(&old_enr, discovery.local_enr().signature()) - ); + let discovery = Discovery::new(portalnet_config, node_data_dir).unwrap(); + assert_eq!(discovery.local_enr(), old_enr); let data = fs::read_to_string(trin_enr_file_location).unwrap(); let old_enr = Enr::from_str(&data).unwrap(); assert_eq!(discovery.local_enr().seq(), 2); diff --git a/portalnet/src/overlay_service.rs b/portalnet/src/overlay_service.rs index 11a9fe8b7..84a66f4dd 100644 --- a/portalnet/src/overlay_service.rs +++ b/portalnet/src/overlay_service.rs @@ -2695,17 +2695,17 @@ mod tests { types::messages::PortalnetConfig, }; + use crate::utils::db::setup_temp_dir; + use discv5::kbucket::Entry; + use ethereum_types::U256; use ethportal_api::types::content_key::overlay::IdentityContentKey; use ethportal_api::types::distance::XorMetric; use ethportal_api::types::enr::generate_random_remote_enr; - use trin_validation::validator::MockValidator; - - use discv5::kbucket::Entry; - use ethereum_types::U256; use serial_test::serial; use std::net::SocketAddr; use tokio::sync::mpsc::unbounded_channel; use tokio_test::{assert_pending, assert_ready, task}; + use trin_validation::validator::MockValidator; macro_rules! poll_command_rx { ($service:ident) => { @@ -2719,7 +2719,8 @@ mod tests { no_stun: true, ..Default::default() }; - let discovery = Arc::new(Discovery::new(portal_config).unwrap()); + let temp_dir = setup_temp_dir().unwrap().into_path(); + let discovery = Arc::new(Discovery::new(portal_config, temp_dir).unwrap()); let (_utp_talk_req_tx, utp_talk_req_rx) = unbounded_channel(); let discv5_utp = diff --git a/portalnet/src/types/messages.rs b/portalnet/src/types/messages.rs index f09ab8ba3..216a2bcd3 100644 --- a/portalnet/src/types/messages.rs +++ b/portalnet/src/types/messages.rs @@ -1,4 +1,3 @@ -use std::path::PathBuf; use std::{ convert::{TryFrom, TryInto}, fmt, @@ -169,7 +168,6 @@ pub struct PortalnetConfig { pub internal_ip: bool, pub no_stun: bool, pub node_addr_cache_capacity: usize, - pub enr_file_location: Option, } impl Default for PortalnetConfig { @@ -182,7 +180,6 @@ impl Default for PortalnetConfig { data_radius: Distance::MAX, internal_ip: false, no_stun: false, - enr_file_location: None, node_addr_cache_capacity: NODE_ADDR_CACHE_CAPACITY, } } diff --git a/portalnet/tests/overlay.rs b/portalnet/tests/overlay.rs index 21c3f196e..400d0d557 100644 --- a/portalnet/tests/overlay.rs +++ b/portalnet/tests/overlay.rs @@ -21,6 +21,7 @@ use tokio::{ use utp_rs::socket::UtpSocket; use ethportal_api::utils::bytes::hex_encode_upper; +use portalnet::utils::db::setup_temp_dir; async fn init_overlay( discovery: Arc, @@ -102,7 +103,8 @@ async fn overlay() { external_addr: Some(SocketAddr::new(ip_addr, 8001)), ..PortalnetConfig::default() }; - let mut discovery_one = Discovery::new(portal_config_one).unwrap(); + let temp_dir_one = setup_temp_dir().unwrap().into_path(); + let mut discovery_one = Discovery::new(portal_config_one, temp_dir_one).unwrap(); let talk_req_rx_one = discovery_one.start().await.unwrap(); let discovery_one = Arc::new(discovery_one); let overlay_one = Arc::new(init_overlay(Arc::clone(&discovery_one), protocol.clone()).await); @@ -115,7 +117,8 @@ async fn overlay() { external_addr: Some(SocketAddr::new(ip_addr, 8002)), ..PortalnetConfig::default() }; - let mut discovery_two = Discovery::new(portal_config_two).unwrap(); + let temp_dir_two = setup_temp_dir().unwrap().into_path(); + let mut discovery_two = Discovery::new(portal_config_two, temp_dir_two).unwrap(); let talk_req_rx_two = discovery_two.start().await.unwrap(); let discovery_two = Arc::new(discovery_two); let overlay_two = Arc::new(init_overlay(Arc::clone(&discovery_two), protocol.clone()).await); @@ -128,7 +131,8 @@ async fn overlay() { external_addr: Some(SocketAddr::new(ip_addr, 8003)), ..PortalnetConfig::default() }; - let mut discovery_three = Discovery::new(portal_config_three).unwrap(); + let temp_dir_three = setup_temp_dir().unwrap().into_path(); + let mut discovery_three = Discovery::new(portal_config_three, temp_dir_three).unwrap(); let talk_req_rx_three = discovery_three.start().await.unwrap(); let discovery_three = Arc::new(discovery_three); let overlay_three = @@ -247,7 +251,8 @@ async fn overlay_event_stream() { no_stun: true, ..Default::default() }; - let discovery = Arc::new(Discovery::new(portal_config).unwrap()); + let temp_dir = setup_temp_dir().unwrap().into_path(); + let discovery = Arc::new(Discovery::new(portal_config, temp_dir).unwrap()); let overlay = init_overlay(discovery, ProtocolId::Beacon).await; overlay.event_stream().await.unwrap(); diff --git a/rpc/src/rpc_server.rs b/rpc/src/rpc_server.rs index 95339bfa0..59c2250d7 100644 --- a/rpc/src/rpc_server.rs +++ b/rpc/src/rpc_server.rs @@ -612,6 +612,7 @@ mod tests { use crate::builder::RpcModuleSelection; use crate::{PortalRpcModule, RpcModuleBuilder}; use portalnet::discovery::Discovery; + use portalnet::utils::db::setup_temp_dir; use std::io; use std::sync::Arc; @@ -633,7 +634,8 @@ mod tests { pub fn test_rpc_builder() -> RpcModuleBuilder { let (history_tx, _) = tokio::sync::mpsc::unbounded_channel(); let (beacon_tx, _) = tokio::sync::mpsc::unbounded_channel(); - let discv5 = Arc::new(Discovery::new(Default::default()).unwrap()); + let temp_dir = setup_temp_dir().unwrap().into_path(); + let discv5 = Arc::new(Discovery::new(Default::default(), temp_dir).unwrap()); RpcModuleBuilder::new(discv5) .with_history(history_tx) .with_beacon(beacon_tx) diff --git a/src/lib.rs b/src/lib.rs index ab633af8a..cf41a7b0f 100644 --- a/src/lib.rs +++ b/src/lib.rs @@ -51,12 +51,11 @@ pub async fn run_trin( listen_port: trin_config.discovery_port, no_stun: trin_config.no_stun, bootnodes: trin_config.bootnodes.clone(), - enr_file_location: Some(node_data_dir.clone()), ..Default::default() }; // Initialize base discovery protocol - let mut discovery = Discovery::new(portalnet_config.clone())?; + let mut discovery = Discovery::new(portalnet_config.clone(), node_data_dir.clone())?; let talk_req_rx = discovery.start().await?; let discovery = Arc::new(discovery); diff --git a/utp-testing/src/lib.rs b/utp-testing/src/lib.rs index 72fa488bf..e8da914aa 100644 --- a/utp-testing/src/lib.rs +++ b/utp-testing/src/lib.rs @@ -12,6 +12,7 @@ use jsonrpsee::proc_macros::rpc; use jsonrpsee::server::{Server, ServerHandle}; use portalnet::discovery::{Discovery, UtpEnr}; use portalnet::types::messages::{PortalnetConfig, ProtocolId}; +use portalnet::utils::db::setup_temp_dir; use std::net::SocketAddr; use std::str::FromStr; use std::sync::Arc; @@ -148,7 +149,8 @@ pub async fn run_test_app( ..Default::default() }; - let mut discovery = Discovery::new(config).unwrap(); + let temp_dir = setup_temp_dir().unwrap().into_path(); + let mut discovery = Discovery::new(config, temp_dir).unwrap(); let talk_req_rx = discovery.start().await.unwrap(); let enr = discovery.local_enr(); let discovery = Arc::new(discovery);