Do not try to synchronize from light client (#2039)

* do not try to sync from light node

* use Roles' utility methods

substrate/core/network/src/config.rs

@@ -73,6 +73,18 @@ bitflags! {
 	}
 }
 
+impl Roles {
+	/// Does this role represents a client that holds full chain data locally?
+	pub fn is_full(&self) -> bool {
+		self.intersects(Roles::FULL | Roles::AUTHORITY)
+	}
+
+	/// Does this role represents a client that does not hold full chain data locally?
+	pub fn is_light(&self) -> bool {
+		!self.is_full()
+	}
+}
+
 impl parity_codec::Encode for Roles {
 	fn encode_to<T: parity_codec::Output>(&self, dest: &mut T) {
 		dest.push_byte(self.bits())

substrate/core/network/src/consensus_gossip.rs

@@ -266,7 +266,7 @@ impl<B: BlockT> ConsensusGossip<B> {
 	/// Handle new connected peer.
 	pub fn new_peer(&mut self, protocol: &mut Context<B>, who: PeerId, roles: Roles) {
 		// light nodes are not valid targets for consensus gossip messages
-		if !roles.intersects(Roles::FULL | Roles::AUTHORITY) {
+		if !roles.is_full() {
 			return;
 		}
 

substrate/core/network/src/on_demand.rs

@@ -284,7 +284,7 @@ impl<B> OnDemandService<B> for OnDemand<B> where
 	B::Header: HeaderT,
 {
 	fn on_connect(&self, peer: PeerId, role: Roles, best_number: NumberFor<B>) {
-		if !role.intersects(Roles::FULL | Roles::AUTHORITY) {
+		if !role.is_full() {
 			return;
 		}
 

substrate/core/network/src/protocol.rs

@@ -341,7 +341,7 @@ impl<B: BlockT, S: NetworkSpecialization<B>, H: ExHashT> Protocol<B, S, H> {
 	}
 
 	/// Returns an object representing the status of the protocol.
-	pub fn status(&mut self) -> ProtocolStatus<B> {
+	pub fn status(&self) -> ProtocolStatus<B> {
 		ProtocolStatus {
 			sync: self.sync.status(),
 			num_peers: self.context_data.peers.values().count(),
@@ -611,6 +611,15 @@ impl<B: BlockT, S: NetworkSpecialization<B>, H: ExHashT> Protocol<B, S, H> {
 			request.from,
 			request.to,
 			request.max);
+
+		// sending block requests to the node that is unable to serve it is considered a bad behavior
+		if !self.config.roles.is_full() {
+			trace!(target: "sync", "Peer {} is trying to sync from the light node", peer);
+			self.network_chan.send(NetworkMsg::DisconnectPeer(peer.clone()));
+			self.network_chan.send(NetworkMsg::ReportPeer(peer, i32::min_value()));
+			return;
+		}
+
 		let mut blocks = Vec::new();
 		let mut id = match request.from {
 			message::FromBlock::Hash(h) => BlockId::Hash(h),
@@ -772,7 +781,7 @@ impl<B: BlockT, S: NetworkSpecialization<B>, H: ExHashT> Protocol<B, S, H> {
 				self.network_chan.send(NetworkMsg::DisconnectPeer(who));
 				return;
 			}
-			if self.config.roles & Roles::LIGHT == Roles::LIGHT {
+			if self.config.roles.is_light() {
 				let self_best_block = self
 					.context_data
 					.chain
@@ -961,7 +970,7 @@ impl<B: BlockT, S: NetworkSpecialization<B>, H: ExHashT> Protocol<B, S, H> {
 		);
 
 		// blocks are not announced by light clients
-		if self.config.roles & Roles::LIGHT == Roles::LIGHT {
+		if self.config.roles.is_light() {
 			return;
 		}
 

substrate/core/network/src/sync.rs

@@ -181,7 +181,7 @@ impl<B: BlockT> ChainSync<B> {
 		info: &ClientInfo<B>,
 	) -> Self {
 		let mut required_block_attributes = message::BlockAttributes::HEADER | message::BlockAttributes::JUSTIFICATION;
-		if role.intersects(Roles::FULL | Roles::AUTHORITY) {
+		if role.is_full() {
 			required_block_attributes |= message::BlockAttributes::BODY;
 		}
 
@@ -233,6 +233,12 @@ impl<B: BlockT> ChainSync<B> {
 	/// Handle new connected peer. Call this method whenever we connect to a new peer.
 	pub(crate) fn new_peer(&mut self, protocol: &mut Context<B>, who: PeerId) {
 		if let Some(info) = protocol.peer_info(&who) {
+			// there's nothing sync can get from the node that has no blockchain data
+			// (the opposite is not true, but all requests are served at protocol level)
+			if !info.roles.is_full() {
+				return;
+			}
+
 			let status = block_status(&*protocol.client(), &self.queue_blocks, info.best_hash);
 			match (status, info.best_number) {
 				(Err(e), _) => {

substrate/core/network/src/test/mod.rs

@@ -23,7 +23,6 @@ mod sync;
 
 use std::collections::{HashMap, HashSet, VecDeque};
 use std::sync::Arc;
-use std::sync::atomic::{AtomicBool, Ordering};
 
 use log::trace;
 use crate::chain::FinalityProofProvider;
@@ -45,7 +44,9 @@ use crate::message::Message;
 use network_libp2p::PeerId;
 use parking_lot::{Mutex, RwLock};
 use primitives::{H256, sr25519::Public as AuthorityId, Blake2Hasher};
-use crate::protocol::{ConnectedPeer, Context, Protocol, ProtocolMsg, CustomMessageOutcome};
+use crate::protocol::{
+	ConnectedPeer, Context, Protocol, ProtocolStatus, ProtocolMsg, CustomMessageOutcome,
+};
 use runtime_primitives::generic::BlockId;
 use runtime_primitives::traits::{AuthorityIdFor, Block as BlockT, Digest, DigestItem, Header, NumberFor};
 use runtime_primitives::{Justification, ConsensusEngineId};
@@ -268,12 +269,11 @@ impl<S: NetworkSpecialization<Block>> Link<Block> for TestLink<S> {
 }
 
 pub struct Peer<D, S: NetworkSpecialization<Block>> {
-	pub is_offline: Arc<AtomicBool>,
-	pub is_major_syncing: Arc<AtomicBool>,
 	pub peers: Arc<RwLock<HashMap<PeerId, ConnectedPeer<Block>>>>,
 	pub peer_id: PeerId,
 	client: PeersClient,
 	net_proto_channel: ProtocolChannel<S>,
+	protocol_status: Arc<RwLock<ProtocolStatus<Block>>>,
 	pub import_queue: Box<BasicQueue<Block>>,
 	pub data: D,
 	best_hash: Mutex<Option<H256>>,
@@ -385,8 +385,7 @@ impl<S: NetworkSpecialization<Block>> ProtocolChannel<S> {
 
 impl<D, S: NetworkSpecialization<Block>> Peer<D, S> {
 	fn new(
-		is_offline: Arc<AtomicBool>,
+		protocol_status: Arc<RwLock<ProtocolStatus<Block>>>,
-		is_major_syncing: Arc<AtomicBool>,
 		peers: Arc<RwLock<HashMap<PeerId, ConnectedPeer<Block>>>>,
 		client: PeersClient,
 		import_queue: Box<BasicQueue<Block>>,
@@ -408,8 +407,7 @@ impl<D, S: NetworkSpecialization<Block>> Peer<D, S> {
 		);
 		import_queue.start(Box::new(network_link)).expect("Test ImportQueue always starts");
 		Peer {
-			is_offline,
+			protocol_status,
-			is_major_syncing,
 			peers,
 			peer_id: PeerId::random(),
 			client,
@@ -443,13 +441,18 @@ impl<D, S: NetworkSpecialization<Block>> Peer<D, S> {
 	/// SyncOracle: are we connected to any peer?
 	#[cfg(test)]
 	pub fn is_offline(&self) -> bool {
-		self.is_offline.load(std::sync::atomic::Ordering::Relaxed)
+		self.protocol_status.read().sync.is_offline()
 	}
 
 	/// SyncOracle: are we in the process of catching-up with the chain?
 	#[cfg(test)]
 	pub fn is_major_syncing(&self) -> bool {
-		self.is_major_syncing.load(std::sync::atomic::Ordering::Relaxed)
+		self.protocol_status.read().sync.is_major_syncing()
+	}
+
+	/// Get protocol status.
+	pub fn protocol_status(&self) -> ProtocolStatus<Block> {
+		self.protocol_status.read().clone()
 	}
 
 	/// Called on connection to other indicated peer.
@@ -741,8 +744,7 @@ pub trait TestNetFactory: Sized {
 	/// Add created peer.
 	fn add_peer(
 		&mut self,
-		is_offline: Arc<AtomicBool>,
+		protocol_status: Arc<RwLock<ProtocolStatus<Block>>>,
-		is_major_syncing: Arc<AtomicBool>,
 		import_queue: Box<BasicQueue<Block>>,
 		mut protocol: Protocol<Block, Self::Specialization, Hash>,
 		mut network_to_protocol_rx: mpsc::UnboundedReceiver<FromNetworkMsg<Block>>,
@@ -784,9 +786,7 @@ pub trait TestNetFactory: Sized {
 					return Ok(Async::Ready(()))
 				}
 
-
+				*protocol_status.write() = protocol.status();
-				is_offline.store(protocol.is_offline(), Ordering::Relaxed);
-				is_major_syncing.store(protocol.is_major_syncing(), Ordering::Relaxed);
 
 				Ok(Async::NotReady)
 			}));
@@ -821,8 +821,6 @@ pub trait TestNetFactory: Sized {
 			finality_proof_import,
 			finality_proof_request_builder,
 		));
-		let is_offline = Arc::new(AtomicBool::new(true));
-		let is_major_syncing = Arc::new(AtomicBool::new(false));
 		let specialization = self::SpecializationFactory::create();
 		let peers: Arc<RwLock<HashMap<PeerId, ConnectedPeer<Block>>>> = Arc::new(Default::default());
 
@@ -839,15 +837,14 @@ pub trait TestNetFactory: Sized {
 			specialization,
 		).unwrap();
 
+		let protocol_status = Arc::new(RwLock::new(protocol.status()));
 		self.add_peer(
-			is_offline.clone(),
+			protocol_status.clone(),
-			is_major_syncing.clone(),
 			import_queue.clone(),
 			protocol,
 			network_to_protocol_rx,
 			Arc::new(Peer::new(
-				is_offline,
+				protocol_status,
-				is_major_syncing,
 				peers,
 				PeersClient::Full(client),
 				import_queue,
@@ -879,8 +876,6 @@ pub trait TestNetFactory: Sized {
 			finality_proof_import,
 			finality_proof_request_builder,
 		));
-		let is_offline = Arc::new(AtomicBool::new(true));
-		let is_major_syncing = Arc::new(AtomicBool::new(false));
 		let specialization = self::SpecializationFactory::create();
 		let peers: Arc<RwLock<HashMap<PeerId, ConnectedPeer<Block>>>> = Arc::new(Default::default());
 
@@ -897,15 +892,14 @@ pub trait TestNetFactory: Sized {
 			specialization,
 		).unwrap();
 
+		let protocol_status = Arc::new(RwLock::new(protocol.status()));
 		self.add_peer(
-			is_offline.clone(),
+			protocol_status.clone(),
-			is_major_syncing.clone(),
 			import_queue.clone(),
 			protocol,
 			network_to_protocol_rx,
 			Arc::new(Peer::new(
-				is_offline,
+				protocol_status,
-				is_major_syncing,
 				peers,
 				PeersClient::Light(client),
 				import_queue,

substrate/core/network/src/test/sync.rs

@@ -16,6 +16,7 @@
 
 use client::{backend::Backend, blockchain::HeaderBackend};
 use crate::config::Roles;
+use crate::message;
 use consensus::BlockOrigin;
 use std::collections::HashSet;
 use super::*;
@@ -398,3 +399,56 @@ fn can_sync_small_non_best_forks() {
 	assert!(net.peer(0).client().header(&BlockId::Hash(small_hash)).unwrap().is_some());
 	assert!(net.peer(1).client().header(&BlockId::Hash(small_hash)).unwrap().is_some());
 }
+
+#[test]
+fn can_not_sync_from_light_peer() {
+	let _ = ::env_logger::try_init();
+
+	// given the network with 1 full nodes (#0) and 1 light node (#1)
+	let mut net = TestNet::new(1);
+	net.add_light_peer(&Default::default());
+
+	// generate some blocks on #0
+	net.peer(0).push_blocks(1, false);
+
+	// and let the light client sync from this node
+	// (mind the #1 is disconnected && not syncing)
+	net.sync();
+
+	// ensure #0 && #1 have the same best block
+	let full0_info = net.peer(0).client.info().unwrap().chain;
+	let light_info = net.peer(1).client.info().unwrap().chain;
+	assert_eq!(full0_info.best_number, 1);
+	assert_eq!(light_info.best_number, 1);
+	assert_eq!(light_info.best_hash, full0_info.best_hash);
+
+	// add new full client (#2) && sync without #0
+	net.add_full_peer(&Default::default());
+	net.peer(1).on_connect(net.peer(2));
+	net.peer(2).on_connect(net.peer(1));
+	net.peer(1).announce_block(light_info.best_hash);
+	net.sync_with(true, Some(vec![0].into_iter().collect()));
+
+	// ensure that the #2 has failed to sync block #1
+	assert_eq!(net.peer(2).client.info().unwrap().chain.best_number, 0);
+	// and that the #1 is still connected to #2
+	// (because #2 has not tried to fetch block data from the #1 light node)
+	assert_eq!(net.peer(1).protocol_status().num_peers, 2);
+
+	// and now try to fetch block data from light peer #1
+	// (this should result in disconnect)
+	net.peer(1).receive_message(
+		&net.peer(2).peer_id,
+		message::generic::Message::BlockRequest(message::generic::BlockRequest {
+			id: 0,
+			fields: message::BlockAttributes::HEADER,
+			from: message::FromBlock::Hash(light_info.best_hash),
+			to: None,
+			direction: message::Direction::Ascending,
+			max: Some(1),
+		}),
+	);
+	net.sync();
+	// check that light #1 has disconnected from #2
+	assert_eq!(net.peer(1).protocol_status().num_peers, 1);
+}

substrate/core/service/src/lib.rs

@@ -169,7 +169,7 @@ impl<Components: components::Components> Service<Components> {
 			Components::build_transaction_pool(config.transaction_pool.clone(), client.clone())?
 		);
 		let transaction_pool_adapter = Arc::new(TransactionPoolAdapter::<Components> {
-			imports_external_transactions: !(config.roles == Roles::LIGHT),
+			imports_external_transactions: !config.roles.is_light(),
 			pool: transaction_pool.clone(),
 			client: client.clone(),
 		 });