fix race in light_peer_imports_header_from_announce (#4579)

substrate/client/src/cht.rs

@@ -62,6 +62,19 @@ pub fn is_build_required<N>(cht_size: N, block_num: N) -> Option<N>
 	Some(block_cht_num - two)
 }
 
+/// Returns Some(max_cht_number) if CHT has ever been built given maximal canonical block number.
+pub fn max_cht_number<N>(cht_size: N, max_canonical_block: N) -> Option<N>
+	where
+		N: Clone + SimpleArithmetic,
+{
+	let max_cht_number = block_to_cht_number(cht_size, max_canonical_block)?;
+	let two = N::one() + N::one();
+	if max_cht_number < two {
+		return None;
+	}
+	Some(max_cht_number - two)
+}
+
 /// Compute a CHT root from an iterator of block hashes. Fails if shorter than
 /// SIZE items. The items are assumed to proceed sequentially from `start_number(cht_num)`.
 /// Discards the trie's nodes.
@@ -329,8 +342,24 @@ mod tests {
 		assert_eq!(is_build_required(SIZE, SIZE + 1), None);
 		assert_eq!(is_build_required(SIZE, 2 * SIZE), None);
 		assert_eq!(is_build_required(SIZE, 2 * SIZE + 1), Some(0));
+		assert_eq!(is_build_required(SIZE, 2 * SIZE + 2), None);
 		assert_eq!(is_build_required(SIZE, 3 * SIZE), None);
 		assert_eq!(is_build_required(SIZE, 3 * SIZE + 1), Some(1));
+		assert_eq!(is_build_required(SIZE, 3 * SIZE + 2), None);
+	}
+
+	#[test]
+	fn max_cht_number_works() {
+		assert_eq!(max_cht_number(SIZE, 0u32.into()), None);
+		assert_eq!(max_cht_number(SIZE, 1u32.into()), None);
+		assert_eq!(max_cht_number(SIZE, SIZE), None);
+		assert_eq!(max_cht_number(SIZE, SIZE + 1), None);
+		assert_eq!(max_cht_number(SIZE, 2 * SIZE), None);
+		assert_eq!(max_cht_number(SIZE, 2 * SIZE + 1), Some(0));
+		assert_eq!(max_cht_number(SIZE, 2 * SIZE + 2), Some(0));
+		assert_eq!(max_cht_number(SIZE, 3 * SIZE), Some(0));
+		assert_eq!(max_cht_number(SIZE, 3 * SIZE + 1), Some(1));
+		assert_eq!(max_cht_number(SIZE, 3 * SIZE + 2), Some(1));
 	}
 
 	#[test]

substrate/client/src/in_mem.rs

@@ -433,18 +433,20 @@ impl<Block: BlockT> sc_client_api::light::Storage<Block> for Blockchain<Block>
 		&self,
 		_cht_size: NumberFor<Block>,
 		block: NumberFor<Block>,
-	) -> sp_blockchain::Result<Block::Hash> {
+	) -> sp_blockchain::Result<Option<Block::Hash>> {
 		self.storage.read().header_cht_roots.get(&block).cloned()
 			.ok_or_else(|| sp_blockchain::Error::Backend(format!("Header CHT for block {} not exists", block)))
+			.map(Some)
 	}
 
 	fn changes_trie_cht_root(
 		&self,
 		_cht_size: NumberFor<Block>,
 		block: NumberFor<Block>,
-	) -> sp_blockchain::Result<Block::Hash> {
+	) -> sp_blockchain::Result<Option<Block::Hash>> {
 		self.storage.read().changes_trie_cht_roots.get(&block).cloned()
 			.ok_or_else(|| sp_blockchain::Error::Backend(format!("Changes trie CHT for block {} not exists", block)))
+			.map(Some)
 	}
 
 	fn cache(&self) -> Option<Arc<dyn blockchain::Cache<Block>>> {

substrate/client/src/light/blockchain.rs

@@ -164,7 +164,10 @@ impl<S, Block: BlockT> RemoteBlockchain<Block> for Blockchain<S>
 		}
 
 		Ok(LocalOrRemote::Remote(RemoteHeaderRequest {
-			cht_root: self.storage.header_cht_root(cht::size(), number)?,
+			cht_root: match self.storage.header_cht_root(cht::size(), number)? {
+				Some(cht_root) => cht_root,
+				None => return Ok(LocalOrRemote::Unknown),
+			},
 			block: number,
 			retry_count: None,
 		}))
@@ -298,17 +301,18 @@ pub mod tests {
 			Err(ClientError::Backend("Test error".into()))
 		}
 
-		fn header_cht_root(&self, _cht_size: u64, _block: u64) -> ClientResult<Hash> {
+		fn header_cht_root(&self, _cht_size: u64, _block: u64) -> ClientResult<Option<Hash>> {
 			Err(ClientError::Backend("Test error".into()))
 		}
 
-		fn changes_trie_cht_root(&self, cht_size: u64, block: u64) -> ClientResult<Hash> {
+		fn changes_trie_cht_root(&self, cht_size: u64, block: u64) -> ClientResult<Option<Hash>> {
 			cht::block_to_cht_number(cht_size, block)
 				.and_then(|cht_num| self.changes_tries_cht_roots.get(&cht_num))
 				.cloned()
 				.ok_or_else(|| ClientError::Backend(
 					format!("Test error: CHT for block #{} not found", block)
 				).into())
+				.map(Some)
 		}
 
 		fn cache(&self) -> Option<Arc<dyn BlockchainCache<Block>>> {

substrate/client/src/light/fetcher.rs

@@ -153,7 +153,7 @@ impl<E, H, B: BlockT, S: BlockchainStorage<B>> LightDataChecker<E, H, B, S> {
 		// all the checks are sharing the same storage
 		let storage = create_proof_check_backend_storage(remote_roots_proof);
 
-		// we remote_roots.keys() are sorted => we can use this to group changes tries roots
+		// remote_roots.keys() are sorted => we can use this to group changes tries roots
 		// that are belongs to the same CHT
 		let blocks = remote_roots.keys().cloned();
 		cht::for_each_cht_group::<B::Header, _, _, _>(cht_size, blocks, |mut storage, _, cht_blocks| {
@@ -162,7 +162,8 @@ impl<E, H, B: BlockT, S: BlockchainStorage<B>> LightDataChecker<E, H, B, S> {
 			// when required header has been pruned (=> replaced with CHT)
 			let first_block = cht_blocks.first().cloned()
 				.expect("for_each_cht_group never calls callback with empty groups");
-			let local_cht_root = self.blockchain.storage().changes_trie_cht_root(cht_size, first_block)?;
+			let local_cht_root = self.blockchain.storage().changes_trie_cht_root(cht_size, first_block)?
+				.ok_or(ClientError::InvalidCHTProof)?;
 
 			// check changes trie root for every block within CHT range
 			for block in cht_blocks {