From f67ae6ce4f44f25b8c2d50720809752e889551d4 Mon Sep 17 00:00:00 2001 From: qima Date: Thu, 3 Dec 2020 21:06:09 +0800 Subject: [PATCH] chore: address review comments --- src/messages/mod.rs | 2 +- src/messages/variant.rs | 39 ++++++++++++++------ src/routing/approved.rs | 67 +++++++++++++++++++++------------- src/routing/bootstrap.rs | 41 +++++++++++---------- src/routing/mod.rs | 2 +- src/routing/tests/mod.rs | 79 +++++++++++++++++----------------------- tests/drop.rs | 5 +-- tests/messages.rs | 4 +- 8 files changed, 128 insertions(+), 111 deletions(-) diff --git a/src/messages/mod.rs b/src/messages/mod.rs index 451fd0900b..f5bb78a5ad 100644 --- a/src/messages/mod.rs +++ b/src/messages/mod.rs @@ -14,7 +14,7 @@ mod variant; pub use self::{hash::MessageHash, src_authority::SrcAuthority}; pub(crate) use self::{ plain_message::PlainMessage, - variant::{BootstrapResponse, JoinRequest, Variant}, + variant::{BootstrapResponse, JoinRequest, Proof, Variant}, }; use crate::{ crypto::{self, name, Verifier}, diff --git a/src/messages/variant.rs b/src/messages/variant.rs index 85bb0c38f0..2cdb9a8882 100644 --- a/src/messages/variant.rs +++ b/src/messages/variant.rs @@ -9,6 +9,7 @@ use super::{Message, MessageHash, VerifyStatus}; use crate::{ consensus::{DkgKey, ProofShare, Proven, Vote}, + crypto::Signature, error::{Error, Result}, network::Network, relocation::{RelocateDetails, RelocatePayload, RelocatePromise}, @@ -102,6 +103,13 @@ pub(crate) enum Variant { content: Vote, proof_share: ProofShare, }, + /// Challenge sent from existing elder nodes to the joining peer for resource proofing. + Challenge { + data_size: usize, + difficulty: u8, + nonce: [u8; 32], + signature: Signature, + }, } impl Variant { @@ -200,6 +208,15 @@ impl Debug for Variant { .field("content", content) .field("proof_share", proof_share) .finish(), + Self::Challenge { + data_size, + difficulty, + .. + } => f + .debug_struct("Challenge") + .field("data_size", data_size) + .field("difficulty", difficulty) + .finish(), } } } @@ -216,12 +233,15 @@ pub enum BootstrapResponse { /// The new peer should retry bootstrapping with another section. The set of connection infos /// of the members of that section is provided. Rebootstrap(Vec), - /// Expecting the new peer to carry out a resouce proofing. - ResourceProof { - data_size: usize, - difficulty: u8, - nonce: [u8; 32], - }, +} + +/// Joining peer's proof of resolvement of given resource proofing challenge. +#[derive(Clone, Eq, PartialEq, Serialize, Deserialize)] +pub(crate) struct Proof { + pub(crate) solution: u64, + pub(crate) data: VecDeque, + pub(crate) nonce: [u8; 32], + pub(crate) signature: Signature, } /// Request to join a section @@ -232,7 +252,7 @@ pub(crate) struct JoinRequest { /// If the peer is being relocated, contains `RelocatePayload`. Otherwise contains `None`. pub relocate_payload: Option, /// Proof of the resouce proofing - pub resource_proof: Option<(u64, VecDeque)>, + pub proof: Option, } impl Debug for JoinRequest { @@ -247,10 +267,7 @@ impl Debug for JoinRequest { .as_ref() .map(|payload| payload.relocate_details()), ) - .field( - "resouce_proof", - &self.resource_proof.as_ref().map(|(solution, _)| solution), - ) + .field("proof", &self.proof.as_ref().map(|proof| proof.solution)) .finish() } } diff --git a/src/routing/approved.rs b/src/routing/approved.rs index 9c45043bc2..85486a91d4 100644 --- a/src/routing/approved.rs +++ b/src/routing/approved.rs @@ -12,14 +12,14 @@ use crate::{ AccumulationError, DkgCommands, DkgKey, DkgVoter, Proof, ProofShare, Proven, Vote, VoteAccumulator, }, - delivery_group, + crypto, delivery_group, error::{Error, Result}, event::Event, location::{DstLocation, SrcLocation}, message_filter::MessageFilter, messages::{ - BootstrapResponse, JoinRequest, Message, MessageHash, MessageStatus, PlainMessage, Variant, - VerifyStatus, PING, + BootstrapResponse, JoinRequest, Message, MessageHash, MessageStatus, PlainMessage, + Proof as ResourceProofingProof, Variant, VerifyStatus, PING, }, network::Network, node::Node, @@ -36,16 +36,15 @@ use crate::{ }; use bls_dkg::key_gen::{message::Message as DkgMessage, outcome::Outcome as DkgOutcome}; use bytes::Bytes; +use ed25519_dalek::Verifier; use itertools::Itertools; -use lru_time_cache::LruCache; use resource_proof::ResourceProof; use std::{cmp, net::SocketAddr, slice}; use tokio::sync::mpsc; use xor_name::{Prefix, XorName}; -pub(crate) const RESOURCE_PROOF_DATA_SIZE: usize = 256; -pub(crate) const RESOURCE_PROOF_DIFF: u8 = 8; -const RESOURCE_PROOF_ENTRIES: usize = 100; +pub(crate) const RESOURCE_PROOF_DATA_SIZE: usize = 128; +pub(crate) const RESOURCE_PROOF_DIFFICULTY: u8 = 4; // The approved stage - node is a full member of a section and is performing its duties according // to its persona (adult or elder). @@ -63,7 +62,6 @@ pub(crate) struct Approved { pub(super) event_tx: mpsc::UnboundedSender, joins_allowed: bool, resource_proof: ResourceProof, - nonces: LruCache, } impl Approved { @@ -94,8 +92,7 @@ impl Approved { msg_filter: MessageFilter::new(), event_tx, joins_allowed: true, - resource_proof: ResourceProof::new(RESOURCE_PROOF_DATA_SIZE, RESOURCE_PROOF_DIFF), - nonces: LruCache::with_capacity(RESOURCE_PROOF_ENTRIES), + resource_proof: ResourceProof::new(RESOURCE_PROOF_DATA_SIZE, RESOURCE_PROOF_DIFFICULTY), } } @@ -481,6 +478,10 @@ impl Approved { } } } + Variant::Challenge { .. } => { + // Already approved, no need to resolve challenge. + return Ok(MessageStatus::Useless); + } Variant::Sync { .. } | Variant::Relocate(_) | Variant::BootstrapRequest(_) @@ -590,6 +591,10 @@ impl Approved { Ok(vec![]) } + Variant::Challenge { .. } => { + trace!("Already got approved to join, no need to resolve challenge"); + Ok(vec![]) + } } } @@ -1023,29 +1028,39 @@ impl Approved { (MIN_AGE + 1, None, None) }; - if let Some((solution, data)) = join_request.resource_proof { - if let Some(nonce) = self.nonces.get(peer.name()) { - if self.resource_proof.validate_all(nonce, &data, solution) { - return self.vote(Vote::Online { - member_info: MemberInfo::joined(peer.with_age(age)), - previous_name, - their_knowledge, - }); - } + if let Some(ResourceProofingProof { + solution, + data, + nonce, + signature, + }) = join_request.proof + { + let serialized = bincode::serialize(&(*peer.name(), nonce))?; + if self + .node + .keypair + .public + .verify(&serialized, &signature) + .is_ok() + && self.resource_proof.validate_all(&nonce, &data, solution) + { + return self.vote(Vote::Online { + member_info: MemberInfo::joined(peer.with_age(age)), + previous_name, + their_knowledge, + }); } } let nonce: [u8; 32] = rand::random(); - let _ = self.nonces.insert(*peer.name(), nonce); - let response = BootstrapResponse::ResourceProof { + let serialized = bincode::serialize(&(*peer.name(), nonce))?; + let response = Variant::Challenge { data_size: RESOURCE_PROOF_DATA_SIZE, - difficulty: RESOURCE_PROOF_DIFF, + difficulty: RESOURCE_PROOF_DIFFICULTY, nonce, + signature: crypto::sign(&serialized, &self.node.keypair), }; - Ok(vec![self.send_direct_message( - peer.addr(), - Variant::BootstrapResponse(response), - )?]) + Ok(vec![self.send_direct_message(peer.addr(), response)?]) } fn handle_dkg_start( diff --git a/src/routing/bootstrap.rs b/src/routing/bootstrap.rs index c95d9b1498..f0d7bf1f7f 100644 --- a/src/routing/bootstrap.rs +++ b/src/routing/bootstrap.rs @@ -9,10 +9,10 @@ use super::{comm::ConnectionEvent, Comm}; use crate::{ consensus::Proven, - crypto, + crypto::{self, Signature}, error::{Error, Result}, location::DstLocation, - messages::{BootstrapResponse, JoinRequest, Message, Variant, VerifyStatus}, + messages::{BootstrapResponse, JoinRequest, Message, Proof, Variant, VerifyStatus}, node::Node, peer::Peer, relocation::{RelocatePayload, SignedRelocateDetails}, @@ -33,7 +33,7 @@ const BACKLOG_CAPACITY: usize = 100; /// NOTE: It's not guaranteed this function ever returns. This can happen due to messages being /// lost in transit or other reasons. It's the responsibility of the caller to handle this case, /// for example by using a timeout. -pub(crate) async fn adult( +pub(crate) async fn initial( node: Node, comm: &Comm, incoming_conns: &mut mpsc::Receiver, @@ -155,12 +155,6 @@ impl<'a> State<'a> { ); bootstrap_addrs = new_bootstrap_addrs.to_vec(); } - BootstrapResponse::ResourceProof { .. } => { - error!( - "{} Received an unexpected ResourceProof request from {:?}", - self.node, sender, - ); - } } } } @@ -236,7 +230,7 @@ impl<'a> State<'a> { // Send `JoinRequest` and wait for the response. If the response is `Rejoin`, repeat with the // new info. If it is `Approval`, returns the initial `Section` value to use by this node, - // completing the bootstrap. If it is `ResourceProff`, carries out a resource proof calculation. + // completing the bootstrap. If it is `Challenge`, carries out a resource proof calculation. async fn join( mut self, elders_info: EldersInfo, @@ -246,7 +240,7 @@ impl<'a> State<'a> { let mut join_request = JoinRequest { section_key, relocate_payload: relocate_payload.clone(), - resource_proof: None, + proof: None, }; let mut recipients: Vec<_> = elders_info .elders @@ -291,7 +285,7 @@ impl<'a> State<'a> { join_request = JoinRequest { section_key, relocate_payload: relocate_payload.clone(), - resource_proof: None, + proof: None, }; recipients = elders_info .elders @@ -306,19 +300,25 @@ impl<'a> State<'a> { ); } } - JoinResponse::ResourceProof { + JoinResponse::Challenge { data_size, difficulty, nonce, + signature, } => { let rp = ResourceProof::new(data_size, difficulty); let data = rp.create_proof_data(&nonce); let mut prover = rp.create_prover(data.clone()); - let proof = prover.solve(); + let solution = prover.solve(); join_request = JoinRequest { section_key, relocate_payload: relocate_payload.clone(), - resource_proof: Some((proof, data)), + proof: Some(Proof { + solution, + data, + nonce, + signature, + }), }; recipients.push(sender); } @@ -369,20 +369,22 @@ impl<'a> State<'a> { sender, )); } - Variant::BootstrapResponse(BootstrapResponse::ResourceProof { + Variant::Challenge { data_size, difficulty, nonce, - }) => { + signature, + } => { if !self.verify_message(&message, None) { continue; } return Ok(( - JoinResponse::ResourceProof { + JoinResponse::Challenge { data_size: *data_size, difficulty: *difficulty, nonce: *nonce, + signature: *signature, }, sender, )); @@ -465,10 +467,11 @@ enum JoinResponse { elders_info: EldersInfo, section_key: bls::PublicKey, }, - ResourceProof { + Challenge { data_size: usize, difficulty: u8, nonce: [u8; 32], + signature: Signature, }, } diff --git a/src/routing/mod.rs b/src/routing/mod.rs index fda30a6d3f..b0ee0cd572 100644 --- a/src/routing/mod.rs +++ b/src/routing/mod.rs @@ -107,7 +107,7 @@ impl Routing { Comm::bootstrap(config.transport_config, connection_event_tx).await?; let node = Node::new(keypair, comm.our_connection_info().await?); let (node, section, backlog) = - bootstrap::adult(node, &comm, &mut connection_event_rx, bootstrap_addr).await?; + bootstrap::initial(node, &comm, &mut connection_event_rx, bootstrap_addr).await?; let state = Approved::new(node, section, None, event_tx); (state, comm, backlog) diff --git a/src/routing/tests/mod.rs b/src/routing/tests/mod.rs index 74b884c633..540b7ba293 100644 --- a/src/routing/tests/mod.rs +++ b/src/routing/tests/mod.rs @@ -7,7 +7,7 @@ // permissions and limitations relating to use of the SAFE Network Software. use super::{ - approved::{RESOURCE_PROOF_DATA_SIZE, RESOURCE_PROOF_DIFF}, + approved::{RESOURCE_PROOF_DATA_SIZE, RESOURCE_PROOF_DIFFICULTY}, Approved, Comm, Command, Stage, }; use crate::{ @@ -16,7 +16,10 @@ use crate::{ event::Event, location::DstLocation, majority, - messages::{BootstrapResponse, JoinRequest, Message, PlainMessage, Variant}, + messages::{ + BootstrapResponse, JoinRequest, Message, PlainMessage, Proof as ResourceProofingProof, + Variant, + }, network::Network, node::Node, peer::Peer, @@ -94,7 +97,7 @@ async fn receive_join_request() -> Result<()> { Variant::JoinRequest(Box::new(JoinRequest { section_key, relocate_payload: None, - resource_proof: None, + proof: None, })), None, None, @@ -113,13 +116,13 @@ async fn receive_join_request() -> Result<()> { ); let response_message = Message::from_bytes(&response_message)?; - let nonce = assert_matches!( + let (nonce, signature) = assert_matches!( response_message.variant(), - Variant::BootstrapResponse(BootstrapResponse::ResourceProof { nonce, .. }) => nonce + Variant::Challenge { nonce, signature, .. } => (*nonce, *signature) ); - let rp = ResourceProof::new(RESOURCE_PROOF_DATA_SIZE, RESOURCE_PROOF_DIFF); - let data = rp.create_proof_data(nonce); + let rp = ResourceProof::new(RESOURCE_PROOF_DATA_SIZE, RESOURCE_PROOF_DIFFICULTY); + let data = rp.create_proof_data(&nonce); let mut prover = rp.create_prover(data.clone()); let solution = prover.solve(); @@ -129,7 +132,12 @@ async fn receive_join_request() -> Result<()> { Variant::JoinRequest(Box::new(JoinRequest { section_key, relocate_payload: None, - resource_proof: Some((solution, data)), + proof: Some(ResourceProofingProof { + solution, + data, + nonce, + signature, + }), })), None, None, @@ -220,27 +228,10 @@ async fn accumulate_votes() -> Result<()> { } #[tokio::test] -async fn handle_consensus_on_online_during_startup_phase() -> Result<()> { - handle_consensus_on_online(NetworkPhase::Startup).await -} - -#[tokio::test] -async fn handle_consensus_on_online_after_startup_phase() -> Result<()> { - handle_consensus_on_online(NetworkPhase::Regular).await -} - -enum NetworkPhase { - Startup, - Regular, -} - -async fn handle_consensus_on_online(phase: NetworkPhase) -> Result<()> { +async fn handle_consensus_on_online() -> Result<()> { let (event_tx, mut event_rx) = mpsc::unbounded_channel(); - let prefix = match phase { - NetworkPhase::Startup => Prefix::default(), - NetworkPhase::Regular => "0".parse().unwrap(), - }; + let prefix = Prefix::default(); let (elders_info, mut nodes) = gen_elders_info(prefix, ELDER_SIZE); let sk_set = SecretKeySet::random(); @@ -254,19 +245,12 @@ async fn handle_consensus_on_online(phase: NetworkPhase) -> Result<()> { let (node_approval_sent, relocate_age) = handle_online_command(&new_peer, &sk_set, &stage, elders_info).await?; assert!(node_approval_sent); + assert!(relocate_age.is_none()); - match phase { - NetworkPhase::Startup => { - assert!(relocate_age.unwrap() > MIN_AGE); - assert!(event_rx.try_recv().is_err()); - } - NetworkPhase::Regular => { - assert_matches!(event_rx.try_recv(), Ok(Event::MemberJoined { name, age, .. }) => { - assert_eq!(name, *new_peer.name()); - assert_eq!(age, MIN_AGE); - }); - } - } + assert_matches!(event_rx.try_recv(), Ok(Event::MemberJoined { name, age, .. }) => { + assert_eq!(name, *new_peer.name()); + assert_eq!(age, MIN_AGE); + }); Ok(()) } @@ -444,6 +428,11 @@ async fn handle_online_command( Ok((node_approval_sent, relocate_age)) } +enum NetworkPhase { + Startup, + Regular, +} + async fn handle_consensus_on_online_of_rejoined_node(phase: NetworkPhase, age: u8) -> Result<()> { let prefix = match phase { NetworkPhase::Startup => Prefix::default(), @@ -472,13 +461,11 @@ async fn handle_consensus_on_online_of_rejoined_node(phase: NetworkPhase, age: u let (node_approval_sent, relocate_age) = handle_online_command(&peer, &sk_set, &stage, elders_info).await?; assert!(event_rx.try_recv().is_err()); - // After startup, a rejoin node with low age will be rejected. - if let NetworkPhase::Regular = phase { - if age / 2 <= MIN_AGE { - assert!(!node_approval_sent); - assert!(relocate_age.is_none()); - return Ok(()); - } + // A rejoin node with low age will be rejected. + if age / 2 <= MIN_AGE { + assert!(!node_approval_sent); + assert!(relocate_age.is_none()); + return Ok(()); } assert!(node_approval_sent); let expected_age = cmp::max(MIN_AGE, age / 2); diff --git a/tests/drop.rs b/tests/drop.rs index 87b40da1c1..644e0d461d 100644 --- a/tests/drop.rs +++ b/tests/drop.rs @@ -18,11 +18,8 @@ async fn test_node_drop() -> Result<()> { // majority and the `Offline` votes accumulate. let mut nodes = create_connected_nodes(3).await?; - // We are in the startup phase, so the second and third node are instantly relocated. - // Let's wait until they re-join. for (_, events) in &mut nodes[1..] { - assert_next_event!(events, Event::RelocationStarted { .. }); - assert_next_event!(events, Event::Relocated { .. }); + assert_event!(events, Event::PromotedToElder); } // Wait for the DKG(s) to complete, to make sure there are no more messages being exchanged diff --git a/tests/messages.rs b/tests/messages.rs index a6614de51e..56714b73ff 100644 --- a/tests/messages.rs +++ b/tests/messages.rs @@ -92,9 +92,7 @@ async fn test_messages_between_nodes() -> Result<()> { // start a second node which sends a message to the first node let (node2, mut event_stream) = create_node(config_with_contact(node1_contact)).await?; - // We are in the startup phase, so node2 is instantly relocated. Let's wait until it re-joins. - assert_next_event!(event_stream, Event::RelocationStarted { .. }); - assert_next_event!(event_stream, Event::Relocated { .. }); + assert_event!(event_stream, Event::PromotedToElder); let node2_name = node2.name().await;