node/approval-voting: Address second-pass feedback

node/core/approval-voting/src/backend.rs

@@ -26,7 +26,7 @@ use super::approval_db;
 use super::approval_db::v1::{
 	blocks_at_height_key, block_entry_key, candidate_entry_key, STORED_BLOCKS_KEY, Config,
 };
-use super::ops::StoredBlockRange;
+use super::ops::{self, StoredBlockRange};
 use super::persisted_entries::{BlockEntry, CandidateEntry};
 
 #[derive(Debug)]
@@ -61,31 +61,31 @@ impl Backend for DbBackend {
 		&self,
 		block_hash: &Hash,
 	) -> SubsystemResult<Option<BlockEntry>> {
-		super::ops::load_block_entry(&*self.inner, &self.config, block_hash)
+		ops::load_block_entry(&*self.inner, &self.config, block_hash)
 			.map(|e| e.map(Into::into))
 	}
 
 	fn load_candidate_entry(
 		&self,
 		candidate_hash: &CandidateHash,
 	) -> SubsystemResult<Option<CandidateEntry>> {
-		super::ops::load_candidate_entry(&*self.inner, &self.config, candidate_hash)
+		ops::load_candidate_entry(&*self.inner, &self.config, candidate_hash)
 			.map(|e| e.map(Into::into))
 	}
 
 	fn load_blocks_at_height(
 		&self,
 		block_height: &BlockNumber
 	) -> SubsystemResult<Vec<Hash>> {
-		super::ops::load_blocks_at_height(&*self.inner, &self.config, block_height)
+		ops::load_blocks_at_height(&*self.inner, &self.config, block_height)
 	}
 
 	fn load_all_blocks(&self) -> SubsystemResult<Vec<Hash>> {
-		super::ops::load_all_blocks(&*self.inner, &self.config)
+		ops::load_all_blocks(&*self.inner, &self.config)
 	}
 
 	fn load_stored_blocks(&self) -> SubsystemResult<Option<StoredBlockRange>> {
-		super::ops::load_stored_blocks(&*self.inner, &self.config)
+		ops::load_stored_blocks(&*self.inner, &self.config)
 	}
 
 	/// Atomically write the list of operations, with later operations taking precedence over prior.

node/core/approval-voting/src/ops.rs

@@ -50,22 +50,45 @@ pub(super) struct NewCandidateInfo {
 	pub our_assignment: Option<OurAssignment>,
 }
 
+fn visit_and_remove_block_entry(
+	block_hash: Hash,
+	overlayed_db: &mut OverlayedBackend<'_, impl Backend>,
+	visited_candidates: &mut HashMap<CandidateHash, CandidateEntry>,
+) -> SubsystemResult<Vec<Hash>> {
+	let block_entry = match overlayed_db.load_block_entry(&block_hash)? {
+		None => return Ok(Vec::new()),
+		Some(b) => b,
+	};
+
+	overlayed_db.delete_block_entry(&block_hash);
+	for &(_, ref candidate_hash) in block_entry.candidates() {
+		let candidate = match visited_candidates.entry(*candidate_hash) {
+			Entry::Occupied(e) => e.into_mut(),
+			Entry::Vacant(e) => {
+				e.insert(match overlayed_db.load_candidate_entry(candidate_hash)? {
+					None => continue, // Should not happen except for corrupt DB
+					Some(c) => c,
+				})
+			}
+		};
+
+		candidate.block_assignments.remove(&block_hash);
+	}
+
+	Ok(block_entry.children)
+}
+
 /// Canonicalize some particular block, pruning everything before it and
 /// pruning any competing branches at the same height.
-pub(super) fn canonicalize<T>(
-	overlay_db: &mut OverlayedBackend<'_, T>,
+pub(super) fn canonicalize(
+	overlay_db: &mut OverlayedBackend<'_, impl Backend>,
 	canon_number: BlockNumber,
 	canon_hash: Hash,
-) -> SubsystemResult<()>
-	where T: Backend
-{
+) -> SubsystemResult<()> {
 	let range = match overlay_db.load_stored_blocks()? {
 		None => return Ok(()),
-		Some(range) => if range.0 >= canon_number {
-			return Ok(())
-		} else {
-			range
-		},
+		Some(range) if range.0 >= canon_number => return Ok(()),
+		Some(range) => range,
 	};
 
 	// Storing all candidates in memory is potentially heavy, but should be fine
@@ -76,34 +99,6 @@ pub(super) fn canonicalize<T>(
 	// All the block heights we visited but didn't necessarily delete everything from.
 	let mut visited_heights = HashMap::new();
 
-	let visit_and_remove_block_entry = |
-		block_hash: Hash,
-		overlayed_db: &mut OverlayedBackend<'_, T>,
-		visited_candidates: &mut HashMap<CandidateHash, CandidateEntry>,
-	| -> SubsystemResult<Vec<Hash>> {
-		let block_entry = match overlayed_db.load_block_entry(&block_hash)? {
-			None => return Ok(Vec::new()),
-			Some(b) => b,
-		};
-
-		overlayed_db.delete_block_entry(&block_hash);
-		for &(_, ref candidate_hash) in block_entry.candidates() {
-			let candidate = match visited_candidates.entry(*candidate_hash) {
-				Entry::Occupied(e) => e.into_mut(),
-				Entry::Vacant(e) => {
-					e.insert(match overlayed_db.load_candidate_entry(candidate_hash)? {
-						None => continue, // Should not happen except for corrupt DB
-						Some(c) => c,
-					})
-				}
-			};
-
-			candidate.block_assignments.remove(&block_hash);
-		}
-
-		Ok(block_entry.children)
-	};
-
 	// First visit everything before the height.
 	for i in range.0..canon_number {
 		let at_height = overlay_db.load_blocks_at_height(&i)?;
@@ -220,11 +215,8 @@ pub(super) fn add_block_entry(
 	{
 		let new_range = match store.load_stored_blocks()? {
 			None => Some(StoredBlockRange(number, number + 1)),
-			Some(range) => if range.1 <= number {
-				Some(StoredBlockRange(range.0, number + 1))
-			} else {
-				None
-			}
+			Some(range) if range.1 <= number => Some(StoredBlockRange(range.0, number + 1)),
+			Some(_) => None,
 		};
 
 		new_range.map(|n| store.write_stored_block_range(n));