les: hash or number in headers req, not both

ethcore/light/src/net/mod.rs

@@ -34,7 +34,7 @@ use std::sync::Arc;
 use std::sync::atomic::{AtomicUsize, Ordering};
 
 use provider::Provider;
-use request::{self, Request};
+use request::{self, HashOrNumber, Request};
 
 use self::buffer_flow::{Buffer, FlowParams};
 use self::context::Ctx;
@@ -453,19 +453,8 @@ impl LightProtocol {
 			}
 		};
 
-		// if something went wrong, figure out how much to punish the peer.
 		if let Err(e) = res {
-			match e.punishment() {
-				Punishment::None => {}
-				Punishment::Disconnect => {
-					debug!(target: "les", "Disconnecting peer {}: {}", peer, e);
-					io.disconnect_peer(*peer)
-				}
-				Punishment::Disable => {
-					debug!(target: "les", "Disabling peer {}: {}", peer, e);
-					io.disable_peer(*peer)
-				}
-			}
+			punish(*peer, io, e);
 		}
 	}
 
@@ -521,19 +510,37 @@ impl LightProtocol {
 impl LightProtocol {
 	// called when a peer connects.
 	fn on_connect(&self, peer: &PeerId, io: &IoContext) {
-		let peer = *peer;
-
-		trace!(target: "les", "Peer {} connecting", peer);
+		let proto_version = match io.protocol_version(*peer).ok_or(Error::WrongNetwork) {
+			Ok(pv) => pv,
+			Err(e) => { punish(*peer, io, e); return }
+		};
 
-		match self.send_status(peer, io) {
-			Ok(pending_peer) => {
-				self.pending_peers.write().insert(peer, pending_peer);
-			}
-			Err(e) => {
-				trace!(target: "les", "Error while sending status: {}", e);
-				io.disconnect_peer(peer);
-			}
+		if PROTOCOL_VERSIONS.iter().find(|x| **x == proto_version).is_none() {
+			punish(*peer, io, Error::UnsupportedProtocolVersion(proto_version));
+			return;
 		}
+
+		let chain_info = self.provider.chain_info();
+
+		let status = Status {
+			head_td: chain_info.total_difficulty,
+			head_hash: chain_info.best_block_hash,
+			head_num: chain_info.best_block_number,
+			genesis_hash: chain_info.genesis_hash,
+			protocol_version: proto_version as u32, // match peer proto version
+			network_id: self.network_id,
+			last_head: None,
+		};
+
+		let capabilities = self.capabilities.read().clone();
+		let status_packet = status::write_handshake(&status, &capabilities, Some(&self.flow_params));
+
+		self.pending_peers.write().insert(*peer, PendingPeer {
+			sent_head: chain_info.best_block_hash,
+			last_update: SteadyTime::now(),
+		});
+
+		io.send(*peer, packet::STATUS, status_packet);		
 	}
 
 	// called when a peer disconnects.
@@ -566,37 +573,6 @@ impl LightProtocol {
 		}
 	}
 
-	// send status to a peer.
-	fn send_status(&self, peer: PeerId, io: &IoContext) -> Result<PendingPeer, Error> {
-		let proto_version = try!(io.protocol_version(peer).ok_or(Error::WrongNetwork));
-
-		if PROTOCOL_VERSIONS.iter().find(|x| **x == proto_version).is_none() {
-			return Err(Error::UnsupportedProtocolVersion(proto_version));
-		}
-
-		let chain_info = self.provider.chain_info();
-
-		let status = Status {
-			head_td: chain_info.total_difficulty,
-			head_hash: chain_info.best_block_hash,
-			head_num: chain_info.best_block_number,
-			genesis_hash: chain_info.genesis_hash,
-			protocol_version: proto_version as u32, // match peer proto version
-			network_id: self.network_id,
-			last_head: None,
-		};
-
-		let capabilities = self.capabilities.read().clone();
-		let status_packet = status::write_handshake(&status, &capabilities, Some(&self.flow_params));
-
-		io.send(peer, packet::STATUS, status_packet);
-
-		Ok(PendingPeer {
-			sent_head: chain_info.best_block_hash,
-			last_update: SteadyTime::now(),
-		})
-	}
-
 	// Handle status message from peer.
 	fn status(&self, peer: &PeerId, io: &IoContext, data: UntrustedRlp) -> Result<(), Error> {
 		let pending = match self.pending_peers.write().remove(peer) {
@@ -686,7 +662,7 @@ impl LightProtocol {
 	}
 
 	// Handle a request for block headers.
-	fn get_block_headers(&self, peer: &PeerId, io: &IoContext, data: UntrustedRlp) -> Result<(), Error> {
+	fn get_block_headers(&self, peer: &PeerId, io: &IoContext, data: UntrustedRlp) -> Result<(), Error> {		
 		const MAX_HEADERS: usize = 512;
 
 		let peers = self.peers.read();
@@ -701,18 +677,21 @@ impl LightProtocol {
 		let mut peer = peer.lock();
 
 		let req_id: u64 = try!(data.val_at(0));
+		let data = try!(data.at(1));
 
-		let block = {
-			let rlp = try!(data.at(1));
-			(try!(rlp.val_at(0)), try!(rlp.val_at(1)))
+		let start_block = {
+			if try!(data.at(0)).size() == 32 {
+				HashOrNumber::Hash(try!(data.val_at(0)))
+			} else {
+				HashOrNumber::Number(try!(data.val_at(0)))
+			}
 		};
 
 		let req = request::Headers {
-			block_num: block.0,
-			block_hash: block.1,
-			max: ::std::cmp::min(MAX_HEADERS, try!(data.val_at(2))),
-			skip: try!(data.val_at(3)),
-			reverse: try!(data.val_at(4)),
+			start: start_block,
+			max: ::std::cmp::min(MAX_HEADERS, try!(data.val_at(1))),
+			skip: try!(data.val_at(2)),
+			reverse: try!(data.val_at(3)),
 		};
 
 		let max_cost = try!(peer.deduct_max(&self.flow_params, request::Kind::Headers, req.max));
@@ -1111,6 +1090,21 @@ impl LightProtocol {
 	}
 }
 
+// if something went wrong, figure out how much to punish the peer.
+fn punish(peer: PeerId, io: &IoContext, e: Error) {
+	match e.punishment() {
+		Punishment::None => {}
+		Punishment::Disconnect => {
+			debug!(target: "les", "Disconnecting peer {}: {}", peer, e);
+			io.disconnect_peer(peer)
+		}
+		Punishment::Disable => {
+			debug!(target: "les", "Disabling peer {}: {}", peer, e);
+			io.disable_peer(peer)
+		}
+	}
+}
+
 impl NetworkProtocolHandler for LightProtocol {
 	fn initialize(&self, io: &NetworkContext) {
 		io.register_timer(TIMEOUT, TIMEOUT_INTERVAL_MS).expect("Error registering sync timer.");
@@ -1140,15 +1134,19 @@ impl NetworkProtocolHandler for LightProtocol {
 fn encode_request(req: &Request, req_id: usize) -> Vec<u8> {
 	match *req {
 		Request::Headers(ref headers) => {
-			let mut stream = RlpStream::new_list(5);
+			let mut stream = RlpStream::new_list(2);
+			stream.append(&req_id).begin_list(4);
+
+			match headers.start {
+				HashOrNumber::Hash(ref hash) => stream.append(hash),
+				HashOrNumber::Number(ref num) => stream.append(num),
+			};
+
 			stream
-				.append(&req_id)
-				.begin_list(2)
-					.append(&headers.block_num)
-					.append(&headers.block_hash)
 				.append(&headers.max)
 				.append(&headers.skip)
 				.append(&headers.reverse);
+
 			stream.out()
 		}
 		Request::Bodies(ref request) => {

ethcore/light/src/net/tests/mod.rs

@@ -91,16 +91,27 @@ impl Provider for TestProvider {
 	}
 
 	fn block_headers(&self, req: request::Headers) -> Vec<Bytes> {
-		let best_num = self.0.client.chain_info().best_block_number;
-		let start_num = req.block_num;
-
-		match self.0.client.block_hash(BlockId::Number(req.block_num)) {
-			Some(hash) if hash == req.block_hash => {}
-			_=> {
-				trace!(target: "les_provider", "unknown/non-canonical start block in header request: {:?}", (req.block_num, req.block_hash));
-				return vec![]
+		use request::HashOrNumber;
+		use ethcore::views::HeaderView;
+
+		let best_num = self.chain_info().best_block_number;
+		let start_num = match req.start {
+			HashOrNumber::Number(start_num) => start_num,
+			HashOrNumber::Hash(hash) => match self.0.client.block_header(BlockId::Hash(hash)) {
+				None => {
+					return Vec::new();
+				}
+				Some(header) => {
+					let num = HeaderView::new(&header).number();
+					if req.max == 1 || self.0.client.block_hash(BlockId::Number(num)) != Some(hash) {
+						// Non-canonical header or single header requested.
+						return vec![header];
+					}
+
+					num
+				}
 			}
-		}
+		};
 
 		(0u64..req.max as u64)
 			.map(|x: u64| x.saturating_mul(req.skip + 1))
@@ -250,8 +261,7 @@ fn buffer_overflow() {
 
 	// 1000 requests is far too many for the default flow params.
 	let request = encode_request(&Request::Headers(Headers {
-		block_num: 1,
-		block_hash: provider.client.chain_info().genesis_hash,
+		start: 1.into(),
 		max: 1000,
 		skip: 0,
 		reverse: false,
@@ -284,8 +294,7 @@ fn get_block_headers() {
 	}
 
 	let request = Headers {
-		block_num: 1,
-		block_hash: provider.client.block_hash(BlockId::Number(1)).unwrap(),
+		start: 1.into(),
 		max: 10,
 		skip: 0,
 		reverse: false,

ethcore/light/src/provider.rs

@@ -97,17 +97,29 @@ impl<T: ProvingBlockChainClient + ?Sized> Provider for T {
 	}
 
 	fn block_headers(&self, req: request::Headers) -> Vec<Bytes> {
-		let best_num = self.chain_info().best_block_number;
-		let start_num = req.block_num;
+		use request::HashOrNumber;
+		use ethcore::views::HeaderView;
 
-		match self.block_hash(BlockId::Number(req.block_num)) {
-			Some(hash) if hash == req.block_hash => {}
-			_=> {
-				trace!(target: "les_provider", "unknown/non-canonical start block in header request: {:?}", (req.block_num, req.block_hash));
-				return vec![]
+		let best_num = self.chain_info().best_block_number;
+		let start_num = match req.start {
+			HashOrNumber::Number(start_num) => start_num,
+			HashOrNumber::Hash(hash) => match self.block_header(BlockId::Hash(hash)) {
+				None => {
+					trace!(target: "les_provider", "Unknown block hash {} requested", hash);
+					return Vec::new();
+				}
+				Some(header) => {
+					let num = HeaderView::new(&header).number();
+					if req.max == 1 || self.block_hash(BlockId::Number(num)) != Some(hash) {
+						// Non-canonical header or single header requested.
+						return vec![header];
+					}
+
+					num
+				}
 			}
-		}
-
+		};
+		
 		(0u64..req.max as u64)
 			.map(|x: u64| x.saturating_mul(req.skip + 1))
 			.take_while(|x| if req.reverse { x < &start_num } else { best_num - start_num >= *x })

ethcore/light/src/types/les_request.rs

@@ -18,15 +18,34 @@
 
 use util::H256;
 
+/// Either a hash or a number.
+#[derive(Debug, Clone, PartialEq, Eq)]
+#[cfg_attr(feature = "ipc", derive(Binary))]
+pub enum HashOrNumber {
+	/// Block hash variant.
+	Hash(H256),
+	/// Block number variant.
+	Number(u64),
+}
+
+impl From<H256> for HashOrNumber {
+	fn from(hash: H256) -> Self {
+		HashOrNumber::Hash(hash)
+	}
+}
+
+impl From<u64> for HashOrNumber {
+	fn from(num: u64) -> Self {
+		HashOrNumber::Number(num)
+	}
+}
+
 /// A request for block headers.
 #[derive(Debug, Clone, PartialEq, Eq)]
 #[cfg_attr(feature = "ipc", derive(Binary))]
 pub struct Headers {
-	/// Starting block number
-	pub block_num: u64,
-	/// Starting block hash. This and number could be combined but IPC codegen is
-	/// not robust enough to support it.
-	pub block_hash: H256,
+	/// Starting block number or hash.
+	pub start: HashOrNumber,
 	/// The maximum amount of headers which can be returned.
 	pub max: usize,
 	/// The amount of headers to skip between each response entry.