Cleanup output pos idx (#3227)

* cleanup legacy output pos index
cleanup unused migration code
consolidate use of output_pos index

* refactor to cleanup duplicate code

chain/src/chain.rs

@@ -217,9 +217,6 @@ impl Chain {
 		{
 			// Migrate full blocks to protocol version v2.
 			chain.migrate_db_v1_v2()?;
-
-			// Rebuild height_for_pos index.
-			chain.rebuild_height_for_pos()?;
 		}
 
 		chain.log_heads()?;
@@ -975,8 +972,8 @@ impl Chain {
 			batch.save_body_tail(&tip)?;
 		}
 
-		// Rebuild our output_pos index in the db based on current UTXO set.
+		// Rebuild our output_pos index in the db based on fresh UTXO set.
-		txhashset.rebuild_height_pos_index(&header_pmmr, &mut batch)?;
+		txhashset.init_output_pos_index(&header_pmmr, &mut batch)?;
 
 		// Commit all the changes to the db.
 		batch.commit()?;
@@ -1079,15 +1076,12 @@ impl Chain {
 		if let (Ok(tail), Ok(head)) = (self.tail(), self.head()) {
 			let horizon = global::cut_through_horizon() as u64;
 			let threshold = horizon.saturating_add(60);
-			debug!(
+			let next_compact = tail.height.saturating_add(threshold);
-				"compact: head: {}, tail: {}, diff: {}, horizon: {}",
+			if next_compact > head.height {
-				head.height,
+				debug!(
-				tail.height,
+					"compact: skipping startup compaction (next at {})",
-				head.height.saturating_sub(tail.height),
+					next_compact
-				horizon
+				);
-			);
-			if tail.height.saturating_add(threshold) > head.height {
-				debug!("compact: skipping compaction - threshold is 60 blocks beyond horizon.");
 				return Ok(());
 			}
 		}
@@ -1293,57 +1287,6 @@ impl Chain {
 		Ok(())
 	}
 
-	/// Migrate the index 'commitment -> output_pos' to index 'commitment -> (output_pos, block_height)'
-	/// Note: should only be called when Node start-up, for database migration from the old version.
-	fn rebuild_height_for_pos(&self) -> Result<(), Error> {
-		let header_pmmr = self.header_pmmr.read();
-		let txhashset = self.txhashset.read();
-		let mut outputs_pos = txhashset.get_all_output_pos()?;
-		let total_outputs = outputs_pos.len();
-		if total_outputs == 0 {
-			debug!("rebuild_height_for_pos: nothing to be rebuilt");
-			return Ok(());
-		} else {
-			debug!(
-				"rebuild_height_for_pos: rebuilding {} output_pos's height...",
-				total_outputs
-			);
-		}
-		outputs_pos.sort_by(|a, b| a.1.cmp(&b.1));
-
-		let max_height = {
-			let head = self.head()?;
-			head.height
-		};
-
-		let batch = self.store.batch()?;
-		// clear it before rebuilding
-		batch.clear_output_pos_height()?;
-
-		let mut i = 0;
-		for search_height in 0..max_height {
-			let hash = header_pmmr.get_header_hash_by_height(search_height + 1)?;
-			let h = batch.get_block_header(&hash)?;
-			while i < total_outputs {
-				let (commit, pos) = outputs_pos[i];
-				if pos > h.output_mmr_size {
-					// Note: MMR position is 1-based and not 0-based, so here must be '>' instead of '>='
-					break;
-				}
-				batch.save_output_pos_height(&commit, pos, h.height)?;
-				trace!("rebuild_height_for_pos: {:?}", (commit, pos, h.height));
-				i += 1;
-			}
-		}
-
-		// clear the output_pos since now it has been replaced by the new index
-		batch.clear_output_pos()?;
-
-		batch.commit()?;
-		debug!("rebuild_height_for_pos: done");
-		Ok(())
-	}
-
 	/// Gets the block header in which a given output appears in the txhashset.
 	pub fn get_header_for_output(
 		&self,

chain/src/store.rs

@@ -32,8 +32,7 @@ const BLOCK_HEADER_PREFIX: u8 = b'h';
 const BLOCK_PREFIX: u8 = b'b';
 const HEAD_PREFIX: u8 = b'H';
 const TAIL_PREFIX: u8 = b'T';
-const COMMIT_POS_PREFIX: u8 = b'c';
+const OUTPUT_POS_PREFIX: u8 = b'p';
-const COMMIT_POS_HGT_PREFIX: u8 = b'p';
 const BLOCK_INPUT_BITMAP_PREFIX: u8 = b'B';
 const BLOCK_SUMS_PREFIX: u8 = b'M';
 
@@ -112,42 +111,16 @@ impl ChainStore {
 		)
 	}
 
-	/// Get all outputs PMMR pos. (only for migration purpose)
-	pub fn get_all_output_pos(&self) -> Result<Vec<(Commitment, u64)>, Error> {
-		let mut outputs_pos = Vec::new();
-		let key = to_key(COMMIT_POS_PREFIX, &mut "".to_string().into_bytes());
-		for (k, pos) in self.db.iter::<u64>(&key)? {
-			outputs_pos.push((Commitment::from_vec(k[2..].to_vec()), pos));
-		}
-		Ok(outputs_pos)
-	}
-
 	/// Get PMMR pos for the given output commitment.
-	/// Note:
-	///     - Original prefix 'COMMIT_POS_PREFIX' is not used anymore for normal case, refer to #2889 for detail.
-	///     - To be compatible with the old callers, let's keep this function name but replace with new prefix 'COMMIT_POS_HGT_PREFIX'
 	pub fn get_output_pos(&self, commit: &Commitment) -> Result<u64, Error> {
-		let res: Result<Option<(u64, u64)>, Error> = self.db.get_ser(&to_key(
+		self.get_output_pos_height(commit).map(|(pos, _)| pos)
-			COMMIT_POS_HGT_PREFIX,
-			&mut commit.as_ref().to_vec(),
-		));
-		match res {
-			Ok(None) => Err(Error::NotFoundErr(format!(
-				"Output position for: {:?}",
-				commit
-			))),
-			Ok(Some((pos, _height))) => Ok(pos),
-			Err(e) => Err(e),
-		}
 	}
 
 	/// Get PMMR pos and block height for the given output commitment.
 	pub fn get_output_pos_height(&self, commit: &Commitment) -> Result<(u64, u64), Error> {
 		option_to_not_found(
-			self.db.get_ser(&to_key(
+			self.db
-				COMMIT_POS_HGT_PREFIX,
+				.get_ser(&to_key(OUTPUT_POS_PREFIX, &mut commit.as_ref().to_vec())),
-				&mut commit.as_ref().to_vec(),
-			)),
 			|| format!("Output position for: {:?}", commit),
 		)
 	}
@@ -269,65 +242,31 @@ impl<'a> Batch<'a> {
 		height: u64,
 	) -> Result<(), Error> {
 		self.db.put_ser(
-			&to_key(COMMIT_POS_HGT_PREFIX, &mut commit.as_ref().to_vec())[..],
+			&to_key(OUTPUT_POS_PREFIX, &mut commit.as_ref().to_vec())[..],
 			&(pos, height),
 		)
 	}
 
 	/// Iterator over the output_pos index.
 	pub fn output_pos_iter(&self) -> Result<SerIterator<(u64, u64)>, Error> {
-		let key = to_key(COMMIT_POS_HGT_PREFIX, &mut "".to_string().into_bytes());
+		let key = to_key(OUTPUT_POS_PREFIX, &mut "".to_string().into_bytes());
 		self.db.iter(&key)
 	}
 
 	/// Get output_pos from index.
-	/// Note:
-	///     - Original prefix 'COMMIT_POS_PREFIX' is not used for normal case anymore, refer to #2889 for detail.
-	///     - To be compatible with the old callers, let's keep this function name but replace with new prefix 'COMMIT_POS_HGT_PREFIX'
 	pub fn get_output_pos(&self, commit: &Commitment) -> Result<u64, Error> {
-		let res: Result<Option<(u64, u64)>, Error> = self.db.get_ser(&to_key(
+		self.get_output_pos_height(commit).map(|(pos, _)| pos)
-			COMMIT_POS_HGT_PREFIX,
-			&mut commit.as_ref().to_vec(),
-		));
-		match res {
-			Ok(None) => Err(Error::NotFoundErr(format!(
-				"Output position for: {:?}",
-				commit
-			))),
-			Ok(Some((pos, _height))) => Ok(pos),
-			Err(e) => Err(e),
-		}
 	}
 
 	/// Get output_pos and block height from index.
 	pub fn get_output_pos_height(&self, commit: &Commitment) -> Result<(u64, u64), Error> {
 		option_to_not_found(
-			self.db.get_ser(&to_key(
+			self.db
-				COMMIT_POS_HGT_PREFIX,
+				.get_ser(&to_key(OUTPUT_POS_PREFIX, &mut commit.as_ref().to_vec())),
-				&mut commit.as_ref().to_vec(),
-			)),
 			|| format!("Output position for commit: {:?}", commit),
 		)
 	}
 
-	/// Clear all entries from the output_pos index. (only for migration purpose)
-	pub fn clear_output_pos(&self) -> Result<(), Error> {
-		let key = to_key(COMMIT_POS_PREFIX, &mut "".to_string().into_bytes());
-		for (k, _) in self.db.iter::<u64>(&key)? {
-			self.db.delete(&k)?;
-		}
-		Ok(())
-	}
-
-	/// Clear all entries from the (output_pos,height) index (must be rebuilt after).
-	pub fn clear_output_pos_height(&self) -> Result<(), Error> {
-		let key = to_key(COMMIT_POS_HGT_PREFIX, &mut "".to_string().into_bytes());
-		for (k, _) in self.db.iter::<(u64, u64)>(&key)? {
-			self.db.delete(&k)?;
-		}
-		Ok(())
-	}
-
 	/// Get the previous header.
 	pub fn get_previous_header(&self, header: &BlockHeader) -> Result<BlockHeader, Error> {
 		self.get_block_header(&header.prev_hash)

chain/src/txhashset/txhashset.rs

@@ -273,11 +273,6 @@ impl TxHashSet {
 		Ok(self.commit_index.get_block_header(&hash)?)
 	}
 
-	/// Get all outputs MMR pos
-	pub fn get_all_output_pos(&self) -> Result<Vec<(Commitment, u64)>, Error> {
-		Ok(self.commit_index.get_all_output_pos()?)
-	}
-
 	/// returns outputs from the given pmmr index up to the
 	/// specified limit. Also returns the last index actually populated
 	/// max index is the last PMMR index to consider, not leaf index
@@ -391,9 +386,9 @@ impl TxHashSet {
 		Ok(())
 	}
 
-	/// Rebuild the index of block height & MMR positions to the corresponding UTXOs.
+	/// Initialize the output pos index based on current UTXO set.
 	/// This is a costly operation performed only when we receive a full new chain state.
-	pub fn rebuild_height_pos_index(
+	pub fn init_output_pos_index(
 		&self,
 		header_pmmr: &PMMRHandle<BlockHeader>,
 		batch: &mut Batch<'_>,
@@ -403,26 +398,16 @@ impl TxHashSet {
 		let output_pmmr =
 			ReadonlyPMMR::at(&self.output_pmmr_h.backend, self.output_pmmr_h.last_pos);
 
-		// clear it before rebuilding
-		batch.clear_output_pos_height()?;
-
 		let mut outputs_pos: Vec<(Commitment, u64)> = vec![];
 		for pos in output_pmmr.leaf_pos_iter() {
 			if let Some(out) = output_pmmr.get_data(pos) {
 				outputs_pos.push((out.commit, pos));
 			}
 		}
-		let total_outputs = outputs_pos.len();
+		if outputs_pos.is_empty() {
-		if total_outputs == 0 {
-			debug!("rebuild_height_pos_index: nothing to be rebuilt");
 			return Ok(());
-		} else {
-			debug!(
-				"rebuild_height_pos_index: rebuilding {} outputs position & height...",
-				total_outputs
-			);
 		}
-
+		let total_outputs = outputs_pos.len();
 		let max_height = batch.head()?.height;
 
 		let mut i = 0;
@@ -436,13 +421,11 @@ impl TxHashSet {
 					break;
 				}
 				batch.save_output_pos_height(&commit, pos, h.height)?;
-				trace!("rebuild_height_pos_index: {:?}", (commit, pos, h.height));
 				i += 1;
 			}
 		}
-
 		debug!(
-			"rebuild_height_pos_index: {} UTXOs, took {}s",
+			"init_height_pos_index: {} UTXOs, took {}s",
 			total_outputs,
 			now.elapsed().as_secs(),
 		);