Merge pull request #490 from jkczyz/2020-02-initial-routing-sync

Refactor logic for setting initial_routing_sync feature bit

Author: TheBlueMatt

lightning/src/ln/msgs.rs

@@ -604,6 +604,8 @@ pub trait RoutingMessageHandler : Send + Sync {
 	/// starting at the node *after* the provided publickey and including batch_amount entries.
 	/// If None is provided for starting_point, we start at the first node.
 	fn get_next_node_announcements(&self, starting_point: Option<&PublicKey>, batch_amount: u8) -> Vec<NodeAnnouncement>;
+	/// Returns whether a full sync should be requested from a peer.
+	fn should_request_full_sync(&self, node_id: &PublicKey) -> bool;
 }
 
 pub(crate) struct OnionRealm0HopData {

lightning/src/ln/peer_handler.rs

@@ -189,7 +189,6 @@ pub struct PeerManager<Descriptor: SocketDescriptor, CM: Deref> where CM::Target
 	peer_counter_low: AtomicUsize,
 	peer_counter_high: AtomicUsize,
 
-	initial_syncs_sent: AtomicUsize,
 	logger: Arc<Logger>,
 }
 
@@ -212,9 +211,6 @@ macro_rules! encode_msg {
 	}}
 }
 
-//TODO: Really should do something smarter for this
-const INITIAL_SYNCS_TO_SEND: usize = 5;
-
 /// Manages and reacts to connection events. You probably want to use file descriptors as PeerIds.
 /// PeerIds may repeat, but only after disconnect_event() has been called.
 impl<Descriptor: SocketDescriptor, CM: Deref> PeerManager<Descriptor, CM> where CM::Target: msgs::ChannelMessageHandler {
@@ -236,7 +232,6 @@ impl<Descriptor: SocketDescriptor, CM: Deref> PeerManager<Descriptor, CM> where
 			ephemeral_key_midstate,
 			peer_counter_low: AtomicUsize::new(0),
 			peer_counter_high: AtomicUsize::new(0),
-			initial_syncs_sent: AtomicUsize::new(0),
 			logger,
 		}
 	}
@@ -549,8 +544,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref> PeerManager<Descriptor, CM> where
 									peer.their_node_id = Some(their_node_id);
 									insert_node_id!();
 									let mut features = InitFeatures::supported();
-									if self.initial_syncs_sent.load(Ordering::Acquire) < INITIAL_SYNCS_TO_SEND {
-										self.initial_syncs_sent.fetch_add(1, Ordering::AcqRel);
+									if self.message_handler.route_handler.should_request_full_sync(&peer.their_node_id.unwrap()) {
 										features.set_initial_routing_sync();
 									}
 
@@ -648,8 +642,7 @@ impl<Descriptor: SocketDescriptor, CM: Deref> PeerManager<Descriptor, CM> where
 
 												if !peer.outbound {
 													let mut features = InitFeatures::supported();
-													if self.initial_syncs_sent.load(Ordering::Acquire) < INITIAL_SYNCS_TO_SEND {
-														self.initial_syncs_sent.fetch_add(1, Ordering::AcqRel);
+													if self.message_handler.route_handler.should_request_full_sync(&peer.their_node_id.unwrap()) {
 														features.set_initial_routing_sync();
 													}
 

lightning/src/ln/router.rs

@@ -22,6 +22,7 @@ use util::logger::Logger;
 
 use std::cmp;
 use std::sync::{RwLock,Arc};
+use std::sync::atomic::{AtomicUsize, Ordering};
 use std::collections::{HashMap,BinaryHeap,BTreeMap};
 use std::collections::btree_map::Entry as BtreeEntry;
 use std;
@@ -347,6 +348,7 @@ pub struct RouteHint {
 pub struct Router {
 	secp_ctx: Secp256k1<secp256k1::VerifyOnly>,
 	network_map: RwLock<NetworkMap>,
+	full_syncs_requested: AtomicUsize,
 	chain_monitor: Arc<ChainWatchInterface>,
 	logger: Arc<Logger>,
 }
@@ -390,6 +392,7 @@ impl<R: ::std::io::Read> ReadableArgs<R, RouterReadArgs> for Router {
 		Ok(Router {
 			secp_ctx: Secp256k1::verification_only(),
 			network_map: RwLock::new(network_map),
+			full_syncs_requested: AtomicUsize::new(0),
 			chain_monitor: args.chain_monitor,
 			logger: args.logger,
 		})
@@ -406,6 +409,7 @@ macro_rules! secp_verify_sig {
 }
 
 impl RoutingMessageHandler for Router {
+
 	fn handle_node_announcement(&self, msg: &msgs::NodeAnnouncement) -> Result<bool, LightningError> {
 		let msg_hash = hash_to_message!(&Sha256dHash::hash(&msg.contents.encode()[..])[..]);
 		secp_verify_sig!(self.secp_ctx, &msg_hash, &msg.signature, &msg.contents.node_id);
@@ -698,6 +702,17 @@ impl RoutingMessageHandler for Router {
 		}
 		result
 	}
+
+	fn should_request_full_sync(&self, _node_id: &PublicKey) -> bool {
+		//TODO: Determine whether to request a full sync based on the network map.
+		const FULL_SYNCS_TO_REQUEST: usize = 5;
+		if self.full_syncs_requested.load(Ordering::Acquire) < FULL_SYNCS_TO_REQUEST {
+			self.full_syncs_requested.fetch_add(1, Ordering::AcqRel);
+			true
+		} else {
+			false
+		}
+	}
 }
 
 #[derive(Eq, PartialEq)]
@@ -750,6 +765,7 @@ impl Router {
 				our_node_id: our_pubkey,
 				nodes: nodes,
 			}),
+			full_syncs_requested: AtomicUsize::new(0),
 			chain_monitor,
 			logger,
 		}
@@ -1035,7 +1051,7 @@ mod tests {
 	use ln::channelmanager;
 	use ln::router::{Router,NodeInfo,NetworkMap,ChannelInfo,DirectionalChannelInfo,RouteHint};
 	use ln::features::{ChannelFeatures, InitFeatures, NodeFeatures};
-	use ln::msgs::{LightningError, ErrorAction};
+	use ln::msgs::{ErrorAction, LightningError, RoutingMessageHandler};
 	use util::test_utils;
 	use util::test_utils::TestVecWriter;
 	use util::logger::Logger;
@@ -1048,17 +1064,23 @@ mod tests {
 	use hex;
 
 	use secp256k1::key::{PublicKey,SecretKey};
+	use secp256k1::All;
 	use secp256k1::Secp256k1;
 
 	use std::sync::Arc;
 
-	#[test]
-	fn route_test() {
+	fn create_router() -> (Secp256k1<All>, PublicKey, Router) {
 		let secp_ctx = Secp256k1::new();
 		let our_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&hex::decode("0101010101010101010101010101010101010101010101010101010101010101").unwrap()[..]).unwrap());
 		let logger: Arc<Logger> = Arc::new(test_utils::TestLogger::new());
 		let chain_monitor = Arc::new(chaininterface::ChainWatchInterfaceUtil::new(Network::Testnet, Arc::clone(&logger)));
 		let router = Router::new(our_id, chain_monitor, Arc::clone(&logger));
+		(secp_ctx, our_id, router)
+	}
+
+	#[test]
+	fn route_test() {
+		let (secp_ctx, our_id, router) = create_router();
 
 		// Build network from our_id to node8:
 		//
@@ -1823,4 +1845,17 @@ mod tests {
 			assert!(<NetworkMap>::read(&mut ::std::io::Cursor::new(&w.0)).unwrap() == *network);
 		}
 	}
+
+	#[test]
+	fn request_full_sync_finite_times() {
+		let (secp_ctx, _, router) = create_router();
+		let node_id = PublicKey::from_secret_key(&secp_ctx, &SecretKey::from_slice(&hex::decode("0202020202020202020202020202020202020202020202020202020202020202").unwrap()[..]).unwrap());
+
+		assert!(router.should_request_full_sync(&node_id));
+		assert!(router.should_request_full_sync(&node_id));
+		assert!(router.should_request_full_sync(&node_id));
+		assert!(router.should_request_full_sync(&node_id));
+		assert!(router.should_request_full_sync(&node_id));
+		assert!(!router.should_request_full_sync(&node_id));
+	}
 }

lightning/src/util/test_utils.rs

@@ -155,6 +155,9 @@ impl msgs::RoutingMessageHandler for TestRoutingMessageHandler {
 	fn get_next_node_announcements(&self, _starting_point: Option<&PublicKey>, _batch_amount: u8) -> Vec<msgs::NodeAnnouncement> {
 		Vec::new()
 	}
+	fn should_request_full_sync(&self, _node_id: &PublicKey) -> bool {
+		true
+	}
 }
 
 pub struct TestLogger {