API: don't error on missing output (#3256)

* Node API: don't error on missing output

* Propagate errors from get_output*

* Rename is_unspent and small refactor

* Forgot to rename function in tests

* Change Batch get_output_pos_height type signature

api/src/handlers/blocks_api.rs

@@ -57,7 +57,10 @@ impl HeaderHandler {
 	}
 
 	fn get_header_for_output(&self, commit_id: String) -> Result<BlockHeaderPrintable, Error> {
-		let oid = get_output(&self.chain, &commit_id)?.1;
+		let oid = match get_output(&self.chain, &commit_id)? {
+			Some((_, o)) => o,
+			None => return Err(ErrorKind::NotFound.into()),
+		};
 		match w(&self.chain)?.get_header_for_output(&oid) {
 			Ok(header) => Ok(BlockHeaderPrintable::from_header(&header)),
 			Err(_) => Err(ErrorKind::NotFound.into()),
@@ -87,7 +90,10 @@ impl HeaderHandler {
 			return Ok(hash);
 		}
 		if let Some(commit) = commit {
-			let oid = get_output_v2(&self.chain, &commit, false, false)?.1;
+			let oid = match get_output_v2(&self.chain, &commit, false, false)? {
+				Some((_, o)) => o,
+				None => return Err(ErrorKind::NotFound.into()),
+			};
 			match w(&self.chain)?.get_header_for_output(&oid) {
 				Ok(header) => return Ok(header.hash()),
 				Err(_) => return Err(ErrorKind::NotFound.into()),
@@ -169,7 +175,10 @@ impl BlockHandler {
 			return Ok(hash);
 		}
 		if let Some(commit) = commit {
-			let oid = get_output_v2(&self.chain, &commit, false, false)?.1;
+			let oid = match get_output_v2(&self.chain, &commit, false, false)? {
+				Some((_, o)) => o,
+				None => return Err(ErrorKind::NotFound.into()),
+			};
 			match w(&self.chain)?.get_header_for_output(&oid) {
 				Ok(header) => return Ok(header.hash()),
 				Err(_) => return Err(ErrorKind::NotFound.into()),

api/src/handlers/chain_api.rs

@@ -108,21 +108,6 @@ pub struct OutputHandler {
 }
 
 impl OutputHandler {
-	fn get_output(&self, id: &str) -> Result<Output, Error> {
-		let res = get_output(&self.chain, id)?;
-		Ok(res.0)
-	}
-
-	fn get_output_v2(
-		&self,
-		id: &str,
-		include_proof: bool,
-		include_merkle_proof: bool,
-	) -> Result<OutputPrintable, Error> {
-		let res = get_output_v2(&self.chain, id, include_proof, include_merkle_proof)?;
-		Ok(res.0)
-	}
-
 	pub fn get_outputs_v2(
 		&self,
 		commits: Option<Vec<String>>,
@@ -144,17 +129,23 @@ impl OutputHandler {
 				}
 			}
 			for commit in commits {
-				match self.get_output_v2(
+				match get_output_v2(
+					&self.chain,
 					&commit,
 					include_proof.unwrap_or(false),
 					include_merkle_proof.unwrap_or(false),
 				) {
-					Ok(output) => outputs.push(output),
+					Ok(Some((output, _))) => outputs.push(output),
-					// do not crash here simply do not retrieve this output
+					Ok(None) => {
-					Err(e) => error!(
+						// Ignore outputs that are not found
+					}
+					Err(e) => {
+						error!(
 							"Failure to get output for commitment {} with error {}",
 							commit, e
-					),
+						);
+						return Err(e.into());
+					}
 				};
 			}
 		}
@@ -219,12 +210,18 @@ impl OutputHandler {
 
 		let mut outputs: Vec<Output> = vec![];
 		for x in commitments {
-			match self.get_output(&x) {
+			match get_output(&self.chain, &x) {
-				Ok(output) => outputs.push(output),
+				Ok(Some((output, _))) => outputs.push(output),
-				Err(e) => error!(
+				Ok(None) => {
+					// Ignore outputs that are not found
+				}
+				Err(e) => {
+					error!(
 						"Failure to get output for commitment {} with error {}",
 						x, e
-				),
+					);
+					return Err(e.into());
+				}
 			};
 		}
 		Ok(outputs)

api/src/handlers/utils.rs

@@ -13,6 +13,7 @@
 // limitations under the License.
 
 use crate::chain;
+use crate::chain::types::CommitPos;
 use crate::core::core::{OutputFeatures, OutputIdentifier};
 use crate::rest::*;
 use crate::types::*;
@@ -29,11 +30,11 @@ pub fn w<T>(weak: &Weak<T>) -> Result<Arc<T>, Error> {
 		.ok_or_else(|| ErrorKind::Internal("failed to upgrade weak refernce".to_owned()).into())
 }
 
-/// Retrieves an output from the chain given a commit id (a tiny bit iteratively)
+/// Internal function to retrieves an output by a given commitment
-pub fn get_output(
+fn get_unspent(
-	chain: &Weak<chain::Chain>,
+	chain: &Arc<chain::Chain>,
 	id: &str,
-) -> Result<(Output, OutputIdentifier), Error> {
+) -> Result<Option<(CommitPos, OutputIdentifier)>, Error> {
 	let c = util::from_hex(String::from(id)).context(ErrorKind::Argument(format!(
 		"Not a valid commitment: {}",
 		id
@@ -49,28 +50,29 @@ pub fn get_output(
 		OutputIdentifier::new(OutputFeatures::Coinbase, &commit),
 	];
 
-	let chain = w(chain)?;
-
 	for x in outputs.iter() {
-		let res = chain.is_unspent(x);
+		if let Some(output_pos) = chain.get_unspent(x)? {
-		match res {
+			return Ok(Some((output_pos, x.clone())));
-			Ok(output_pos) => {
-				return Ok((
-					Output::new(&commit, output_pos.height, output_pos.pos),
-					x.clone(),
-				));
-			}
-			Err(e) => {
-				trace!(
-					"get_output: err: {} for commit: {:?} with feature: {:?}",
-					e.to_string(),
-					x.commit,
-					x.features
-				);
 		}
 	}
-	}
+	Ok(None)
-	Err(ErrorKind::NotFound.into())
+}
+
+/// Retrieves an output from the chain given a commit id (a tiny bit iteratively)
+pub fn get_output(
+	chain: &Weak<chain::Chain>,
+	id: &str,
+) -> Result<Option<(Output, OutputIdentifier)>, Error> {
+	let chain = w(chain)?;
+	let (output_pos, identifier) = match get_unspent(&chain, id)? {
+		Some(x) => x,
+		None => return Ok(None),
+	};
+
+	Ok(Some((
+		Output::new(&identifier.commit, output_pos.height, output_pos.pos),
+		identifier,
+	)))
 }
 
 /// Retrieves an output from the chain given a commit id (a tiny bit iteratively)
@@ -79,63 +81,27 @@ pub fn get_output_v2(
 	id: &str,
 	include_proof: bool,
 	include_merkle_proof: bool,
-) -> Result<(OutputPrintable, OutputIdentifier), Error> {
+) -> Result<Option<(OutputPrintable, OutputIdentifier)>, Error> {
-	let c = util::from_hex(String::from(id)).context(ErrorKind::Argument(format!(
-		"Not a valid commitment: {}",
-		id
-	)))?;
-	let commit = Commitment::from_vec(c);
-
-	// We need the features here to be able to generate the necessary hash
-	// to compare against the hash in the output MMR.
-	// For now we can just try both (but this probably needs to be part of the api
-	// params)
-	let outputs = [
-		OutputIdentifier::new(OutputFeatures::Plain, &commit),
-		OutputIdentifier::new(OutputFeatures::Coinbase, &commit),
-	];
-
 	let chain = w(chain)?;
+	let (output_pos, identifier) = match get_unspent(&chain, id)? {
+		Some(x) => x,
+		None => return Ok(None),
+	};
 
-	for x in outputs.iter() {
+	let output = chain.get_unspent_output_at(output_pos.pos)?;
-		let res = chain.is_unspent(x);
-		match res {
-			Ok(output_pos) => match chain.get_unspent_output_at(output_pos.pos) {
-				Ok(output) => {
 	let header = if include_merkle_proof && output.is_coinbase() {
 		chain.get_header_by_height(output_pos.height).ok()
 	} else {
 		None
 	};
-					match OutputPrintable::from_output(
+
+	let output_printable = OutputPrintable::from_output(
 		&output,
 		chain.clone(),
 		header.as_ref(),
 		include_proof,
 		include_merkle_proof,
-					) {
+	)?;
-						Ok(output_printable) => return Ok((output_printable, x.clone())),
+
-						Err(e) => {
+	Ok(Some((output_printable, identifier)))
-							trace!(
-								"get_output: err: {} for commit: {:?} with feature: {:?}",
-								e.to_string(),
-								x.commit,
-								x.features
-							);
-						}
-					}
-				}
-				Err(_) => return Err(ErrorKind::NotFound.into()),
-			},
-			Err(e) => {
-				trace!(
-					"get_output: err: {} for commit: {:?} with feature: {:?}",
-					e.to_string(),
-					x.commit,
-					x.features
-				);
-			}
-		}
-	}
-	Err(ErrorKind::NotFound.into())
 }

api/src/rest.rs

@@ -104,6 +104,14 @@ impl From<RouterError> for Error {
 	}
 }
 
+impl From<crate::chain::Error> for Error {
+	fn from(error: crate::chain::Error) -> Error {
+		Error {
+			inner: Context::new(ErrorKind::Internal(error.to_string())),
+		}
+	}
+}
+
 /// TLS config
 #[derive(Clone)]
 pub struct TLSConfig {

api/src/types.rs

@@ -289,8 +289,8 @@ impl OutputPrintable {
 		};
 
 		let out_id = core::OutputIdentifier::from(output);
-		let res = chain.is_unspent(&out_id);
+		let res = chain.get_unspent(&out_id)?;
-		let (spent, block_height) = if let Ok(output_pos) = res {
+		let (spent, block_height) = if let Some(output_pos) = res {
 			(false, Some(output_pos.height))
 		} else {
 			(true, None)

chain/src/chain.rs

@@ -504,8 +504,8 @@ impl Chain {
 	/// spent. This querying is done in a way that is consistent with the
 	/// current chain state, specifically the current winning (valid, most
 	/// work) fork.
-	pub fn is_unspent(&self, output_ref: &OutputIdentifier) -> Result<CommitPos, Error> {
+	pub fn get_unspent(&self, output_ref: &OutputIdentifier) -> Result<Option<CommitPos>, Error> {
-		self.txhashset.read().is_unspent(output_ref)
+		self.txhashset.read().get_unspent(output_ref)
 	}
 
 	/// Retrieves an unspent output using its PMMR position
@@ -1305,7 +1305,10 @@ impl Chain {
 	) -> Result<BlockHeader, Error> {
 		let header_pmmr = self.header_pmmr.read();
 		let txhashset = self.txhashset.read();
-		let output_pos = txhashset.is_unspent(output_ref)?;
+		let output_pos = match txhashset.get_unspent(output_ref)? {
+			Some(o) => o,
+			None => return Err(ErrorKind::OutputNotFound.into()),
+		};
 		let hash = header_pmmr.get_header_hash_by_height(output_pos.height)?;
 		Ok(self.get_block_header(&hash)?)
 	}

chain/src/store.rs

@@ -115,16 +115,19 @@ impl ChainStore {
 
 	/// Get PMMR pos for the given output commitment.
 	pub fn get_output_pos(&self, commit: &Commitment) -> Result<u64, Error> {
-		self.get_output_pos_height(commit).map(|(pos, _)| pos)
+		match self.get_output_pos_height(commit)? {
+			Some((pos, _)) => Ok(pos),
+			None => Err(Error::NotFoundErr(format!(
+				"Output position for: {:?}",
+				commit
+			))),
+		}
 	}
 
 	/// Get PMMR pos and block height for the given output commitment.
-	pub fn get_output_pos_height(&self, commit: &Commitment) -> Result<(u64, u64), Error> {
+	pub fn get_output_pos_height(&self, commit: &Commitment) -> Result<Option<(u64, u64)>, Error> {
-		option_to_not_found(
 		self.db
-				.get_ser(&to_key(OUTPUT_POS_PREFIX, &mut commit.as_ref().to_vec())),
+			.get_ser(&to_key(OUTPUT_POS_PREFIX, &mut commit.as_ref().to_vec()))
-			|| format!("Output position for: {:?}", commit),
-		)
 	}
 
 	/// Builds a new batch to be used with this store.
@@ -274,16 +277,19 @@ impl<'a> Batch<'a> {
 
 	/// Get output_pos from index.
 	pub fn get_output_pos(&self, commit: &Commitment) -> Result<u64, Error> {
-		self.get_output_pos_height(commit).map(|(pos, _)| pos)
+		match self.get_output_pos_height(commit)? {
+			Some((pos, _)) => Ok(pos),
+			None => Err(Error::NotFoundErr(format!(
+				"Output position for: {:?}",
+				commit
+			))),
+		}
 	}
 
 	/// Get output_pos and block height from index.
-	pub fn get_output_pos_height(&self, commit: &Commitment) -> Result<(u64, u64), Error> {
+	pub fn get_output_pos_height(&self, commit: &Commitment) -> Result<Option<(u64, u64)>, Error> {
-		option_to_not_found(
 		self.db
-				.get_ser(&to_key(OUTPUT_POS_PREFIX, &mut commit.as_ref().to_vec())),
+			.get_ser(&to_key(OUTPUT_POS_PREFIX, &mut commit.as_ref().to_vec()))
-			|| format!("Output position for commit: {:?}", commit),
-		)
 	}
 
 	/// Get the previous header.

chain/src/txhashset/txhashset.rs

@@ -223,23 +223,23 @@ impl TxHashSet {
 	/// Check if an output is unspent.
 	/// We look in the index to find the output MMR pos.
 	/// Then we check the entry in the output MMR and confirm the hash matches.
-	pub fn is_unspent(&self, output_id: &OutputIdentifier) -> Result<CommitPos, Error> {
+	pub fn get_unspent(&self, output_id: &OutputIdentifier) -> Result<Option<CommitPos>, Error> {
 		let commit = output_id.commit;
 		match self.commit_index.get_output_pos_height(&commit) {
-			Ok((pos, height)) => {
+			Ok(Some((pos, height))) => {
 				let output_pmmr: ReadonlyPMMR<'_, Output, _> =
 					ReadonlyPMMR::at(&self.output_pmmr_h.backend, self.output_pmmr_h.last_pos);
 				if let Some(out) = output_pmmr.get_data(pos) {
 					if OutputIdentifier::from(out) == *output_id {
-						Ok(CommitPos { pos, height })
+						Ok(Some(CommitPos { pos, height }))
 					} else {
-						Err(ErrorKind::TxHashSetErr("txhashset mismatch".to_string()).into())
+						Ok(None)
 					}
 				} else {
-					Err(ErrorKind::OutputNotFound.into())
+					Ok(None)
 				}
 			}
-			Err(grin_store::Error::NotFoundErr(_)) => Err(ErrorKind::OutputNotFound.into()),
+			Ok(None) => Ok(None),
 			Err(e) => Err(ErrorKind::StoreErr(e, "txhashset unspent check".to_string()).into()),
 		}
 	}
@@ -425,7 +425,12 @@ impl TxHashSet {
 
 		debug!("init_output_pos_index: {} utxos", outputs_pos.len());
 
-		outputs_pos.retain(|x| batch.get_output_pos_height(&x.0).is_err());
+		outputs_pos.retain(|x| {
+			batch
+				.get_output_pos_height(&x.0)
+				.map(|p| p.is_none())
+				.unwrap_or(true)
+		});
 
 		debug!(
 			"init_output_pos_index: {} utxos with missing index entries",
@@ -982,7 +987,7 @@ impl<'a> Extension<'a> {
 
 	fn apply_input(&mut self, input: &Input, batch: &Batch<'_>) -> Result<CommitPos, Error> {
 		let commit = input.commitment();
-		if let Ok((pos, height)) = batch.get_output_pos_height(&commit) {
+		if let Some((pos, height)) = batch.get_output_pos_height(&commit)? {
 			// First check this input corresponds to an existing entry in the output MMR.
 			if let Some(out) = self.output_pmmr.get_data(pos) {
 				if OutputIdentifier::from(input) != out {

chain/tests/mine_simple_chain.rs

@@ -694,11 +694,13 @@ fn spend_in_fork_and_compact() {
 		assert_eq!(head.height, 6);
 		assert_eq!(head.hash(), prev_main.hash());
 		assert!(chain
-			.is_unspent(&OutputIdentifier::from(&tx2.outputs()[0]))
+			.get_unspent(&OutputIdentifier::from(&tx2.outputs()[0]))
-			.is_ok());
+			.unwrap()
+			.is_some());
 		assert!(chain
-			.is_unspent(&OutputIdentifier::from(&tx1.outputs()[0]))
+			.get_unspent(&OutputIdentifier::from(&tx1.outputs()[0]))
-			.is_err());
+			.unwrap()
+			.is_none());
 
 		// make the fork win
 		let fork_next = prepare_block(&kc, &prev_fork, &chain, 10);
@@ -713,11 +715,13 @@ fn spend_in_fork_and_compact() {
 		assert_eq!(head.height, 7);
 		assert_eq!(head.hash(), prev_fork.hash());
 		assert!(chain
-			.is_unspent(&OutputIdentifier::from(&tx2.outputs()[0]))
+			.get_unspent(&OutputIdentifier::from(&tx2.outputs()[0]))
-			.is_ok());
+			.unwrap()
+			.is_some());
 		assert!(chain
-			.is_unspent(&OutputIdentifier::from(&tx1.outputs()[0]))
+			.get_unspent(&OutputIdentifier::from(&tx1.outputs()[0]))
-			.is_err());
+			.unwrap()
+			.is_none());
 
 		// add 20 blocks to go past the test horizon
 		let mut prev = prev_fork;