no need to rehash with index to compare output with input spending it (#3260)

* no need to rehash with index to compare output with input spending it

* compare output identifier when checking is_unspent()

* output identifier from cleanup

api/src/types.rs

@@ -288,7 +288,7 @@ impl OutputPrintable {
 			OutputType::Transaction
 		};
 
-		let out_id = core::OutputIdentifier::from_output(&output);
+		let out_id = core::OutputIdentifier::from(output);
 		let res = chain.is_unspent(&out_id);
 		let (spent, block_height) = if let Ok(output_pos) = res {
 			(false, Some(output_pos.height))

chain/src/txhashset/txhashset.rs

@@ -20,7 +20,7 @@ use crate::core::core::hash::{Hash, Hashed};
 use crate::core::core::merkle_proof::MerkleProof;
 use crate::core::core::pmmr::{self, Backend, ReadonlyPMMR, RewindablePMMR, PMMR};
 use crate::core::core::{Block, BlockHeader, Input, Output, OutputIdentifier, TxKernel};
-use crate::core::ser::{PMMRIndexHashable, PMMRable, ProtocolVersion};
+use crate::core::ser::{PMMRable, ProtocolVersion};
 use crate::error::{Error, ErrorKind};
 use crate::store::{Batch, ChainStore};
 use crate::txhashset::bitmap_accumulator::BitmapAccumulator;
@@ -229,11 +229,11 @@ impl TxHashSet {
 			Ok((pos, height)) => {
 				let output_pmmr: ReadonlyPMMR<'_, Output, _> =
 					ReadonlyPMMR::at(&self.output_pmmr_h.backend, self.output_pmmr_h.last_pos);
-				if let Some(hash) = output_pmmr.get_hash(pos) {
+				if let Some(out) = output_pmmr.get_data(pos) {
-					if hash == output_id.hash_with_index(pos - 1) {
+					if OutputIdentifier::from(out) == *output_id {
 						Ok(CommitPos { pos, height })
 					} else {
-						Err(ErrorKind::TxHashSetErr("txhashset hash mismatch".to_string()).into())
+						Err(ErrorKind::TxHashSetErr("txhashset mismatch".to_string()).into())
 					}
 				} else {
 					Err(ErrorKind::OutputNotFound.into())
@@ -984,11 +984,9 @@ impl<'a> Extension<'a> {
 		let commit = input.commitment();
 		if let Ok((pos, height)) = batch.get_output_pos_height(&commit) {
 			// First check this input corresponds to an existing entry in the output MMR.
-			if let Some(hash) = self.output_pmmr.get_hash(pos) {
+			if let Some(out) = self.output_pmmr.get_data(pos) {
-				if hash != input.hash_with_index(pos - 1) {
+				if OutputIdentifier::from(input) != out {
-					return Err(
+					return Err(ErrorKind::TxHashSetErr("output pmmr mismatch".to_string()).into());
-						ErrorKind::TxHashSetErr("output pmmr hash mismatch".to_string()).into(),
-					);
 				}
 			}
 

chain/src/txhashset/utxo_view.rs

@@ -16,9 +16,8 @@
 
 use crate::core::core::hash::{Hash, Hashed};
 use crate::core::core::pmmr::{self, ReadonlyPMMR};
-use crate::core::core::{Block, BlockHeader, Input, Output, Transaction};
+use crate::core::core::{Block, BlockHeader, Input, Output, OutputIdentifier, Transaction};
 use crate::core::global;
-use crate::core::ser::PMMRIndexHashable;
 use crate::error::{Error, ErrorKind};
 use crate::store::Batch;
 use crate::util::secp::pedersen::RangeProof;
@@ -75,11 +74,11 @@ impl<'a> UTXOView<'a> {
 
 	// Input is valid if it is spending an (unspent) output
 	// that currently exists in the output MMR.
-	// Compare the hash in the output MMR at the expected pos.
+	// Compare against the entry in output MMR at the expected pos.
 	fn validate_input(&self, input: &Input, batch: &Batch<'_>) -> Result<(), Error> {
 		if let Ok(pos) = batch.get_output_pos(&input.commitment()) {
-			if let Some(hash) = self.output_pmmr.get_hash(pos) {
+			if let Some(out) = self.output_pmmr.get_data(pos) {
-				if hash == input.hash_with_index(pos - 1) {
+				if OutputIdentifier::from(input) == out {
 					return Ok(());
 				}
 			}

chain/tests/mine_simple_chain.rs

@@ -546,7 +546,7 @@ fn spend_rewind_spend() {
 		// mine the first block and keep track of the block_hash
 		// so we can spend the coinbase later
 		let b = prepare_block_key_idx(&kc, &head, &chain, 2, 1);
-		let out_id = OutputIdentifier::from_output(&b.outputs()[0]);
+		let out_id = OutputIdentifier::from(&b.outputs()[0]);
 		assert!(out_id.features.is_coinbase());
 		head = b.header.clone();
 		chain
@@ -620,7 +620,7 @@ fn spend_in_fork_and_compact() {
 		// mine the first block and keep track of the block_hash
 		// so we can spend the coinbase later
 		let b = prepare_block(&kc, &fork_head, &chain, 2);
-		let out_id = OutputIdentifier::from_output(&b.outputs()[0]);
+		let out_id = OutputIdentifier::from(&b.outputs()[0]);
 		assert!(out_id.features.is_coinbase());
 		fork_head = b.header.clone();
 		chain
@@ -694,10 +694,10 @@ fn spend_in_fork_and_compact() {
 		assert_eq!(head.height, 6);
 		assert_eq!(head.hash(), prev_main.hash());
 		assert!(chain
-			.is_unspent(&OutputIdentifier::from_output(&tx2.outputs()[0]))
+			.is_unspent(&OutputIdentifier::from(&tx2.outputs()[0]))
 			.is_ok());
 		assert!(chain
-			.is_unspent(&OutputIdentifier::from_output(&tx1.outputs()[0]))
+			.is_unspent(&OutputIdentifier::from(&tx1.outputs()[0]))
 			.is_err());
 
 		// make the fork win
@@ -713,10 +713,10 @@ fn spend_in_fork_and_compact() {
 		assert_eq!(head.height, 7);
 		assert_eq!(head.hash(), prev_fork.hash());
 		assert!(chain
-			.is_unspent(&OutputIdentifier::from_output(&tx2.outputs()[0]))
+			.is_unspent(&OutputIdentifier::from(&tx2.outputs()[0]))
 			.is_ok());
 		assert!(chain
-			.is_unspent(&OutputIdentifier::from_output(&tx1.outputs()[0]))
+			.is_unspent(&OutputIdentifier::from(&tx1.outputs()[0]))
 			.is_err());
 
 		// add 20 blocks to go past the test horizon
@@ -790,7 +790,7 @@ fn output_header_mappings() {
 			chain.process_block(b, chain::Options::MINE).unwrap();
 
 			let header_for_output = chain
-				.get_header_for_output(&OutputIdentifier::from_output(&reward_outputs[n - 1]))
+				.get_header_for_output(&OutputIdentifier::from(&reward_outputs[n - 1]))
 				.unwrap();
 			assert_eq!(header_for_output.height, n as u64);
 
@@ -800,7 +800,7 @@ fn output_header_mappings() {
 		// Check all output positions are as expected
 		for n in 1..15 {
 			let header_for_output = chain
-				.get_header_for_output(&OutputIdentifier::from_output(&reward_outputs[n - 1]))
+				.get_header_for_output(&OutputIdentifier::from(&reward_outputs[n - 1]))
 				.unwrap();
 			assert_eq!(header_for_output.height, n as u64);
 		}

core/src/core/transaction.rs

@@ -1424,7 +1424,7 @@ impl PMMRable for Output {
 	type E = OutputIdentifier;
 
 	fn as_elmt(&self) -> OutputIdentifier {
-		OutputIdentifier::from_output(self)
+		OutputIdentifier::from(self)
 	}
 
 	fn elmt_size() -> Option<u16> {
@@ -1515,14 +1515,6 @@ impl OutputIdentifier {
 		self.commit
 	}
 
-	/// Build an output_identifier from an existing output.
-	pub fn from_output(output: &Output) -> OutputIdentifier {
-		OutputIdentifier {
-			features: output.features,
-			commit: output.commit,
-		}
-	}
-
 	/// Converts this identifier to a full output, provided a RangeProof
 	pub fn into_output(self, proof: RangeProof) -> Output {
 		Output {
@@ -1532,14 +1524,6 @@ impl OutputIdentifier {
 		}
 	}
 
-	/// Build an output_identifier from an existing input.
-	pub fn from_input(input: &Input) -> OutputIdentifier {
-		OutputIdentifier {
-			features: input.features,
-			commit: input.commit,
-		}
-	}
-
 	/// convert an output_identifier to hex string format.
 	pub fn to_hex(&self) -> String {
 		format!(
@@ -1567,8 +1551,8 @@ impl Readable for OutputIdentifier {
 	}
 }
 
-impl From<Output> for OutputIdentifier {
+impl From<&Output> for OutputIdentifier {
-	fn from(out: Output) -> Self {
+	fn from(out: &Output) -> Self {
 		OutputIdentifier {
 			features: out.features,
 			commit: out.commit,
@@ -1576,6 +1560,15 @@ impl From<Output> for OutputIdentifier {
 	}
 }
 
+impl From<&Input> for OutputIdentifier {
+	fn from(input: &Input) -> Self {
+		OutputIdentifier {
+			features: input.features,
+			commit: input.commit,
+		}
+	}
+}
+
 #[cfg(test)]
 mod test {
 	use super::*;