hash (features|commitment) in output mmr (#615)

* experiment with lock_heights on outputs

* playing around with lock_height as part of the switch commitment hash

* cleanup

* include features in the switch commit hash key

* commit

* rebase off master

* commit

* cleanup

* missing docs

* rework coinbase maturity test to build valid tx

* pool and chain tests passing (inputs have switch commitments)

* commit

* cleanup

* check inputs spending coinbase outputs have valid lock_heights

* wip - got it building (tests still failing)

* use zero key for non coinbase switch commit hash

* fees and height wrong order...

* send output lock_height over to wallet via api

* no more header by height index
workaround this for wallet refresh and wallet restore

* refresh heights for unspent wallet outputs where missing

* TODO - might be slow?

* simplify - do not pass around lock_height for non coinbase outputs

* commit

* fix tests after merge

* build input vs coinbase_input
switch commit hash key encodes lock_height
cleanup output by commit index (currently broken...)

* is_unspent and get_unspent cleanup - we have no outputs, only switch_commit_hashes

* separate concept of utxo vs output in the api
utxos come from the sumtrees (and only the sumtrees, limited info)
outputs come from blocks (and we need to look them up via block height)

* cleanup

* better api support for block outputs with range proofs

* basic wallet operations appear to work
restore is not working fully
refresh refreshes heights correctly (at least appears to)

* wallet refresh and wallet restore appear to be working now

* fix core tests

* fix some mine_simple_chain tests

* fixup chain tests

* rework so pool tests pass

* wallet restore now safely habndles duplicate commitments (reused wallet keys)
for coinbase outputs where lock_height is _very_ important

* wip

* validate_coinbase_maturity
got things building
tests are failing

* lite vs full versions of is_unspent

* builds and working locally
zero-conf - what to do here?

* handle zero-conf edge case (use latest block)

* introduce OutputIdentifier, avoid leaking SumCommit everywhere

* fix the bad merge

* pool verifies coinbase maturity via is_matured
this uses sumtree in a consistent way

* cleanup

* add docs, cleanup build warnings

* fix core tests

* fix chain tests

* fix pool tests

* cleanup debug logging that we no longer need

* make out_block optional on an input (only care about it for spending coinbase outputs)

* cleanup

* bump the build

chain/src/chain.rs

@@ -19,12 +19,12 @@ use std::collections::HashMap;
 use std::sync::{Arc, Mutex, RwLock};
 use std::time::{Duration, Instant};
 
-use util::secp::pedersen::{Commitment, RangeProof};
+use util::secp::pedersen::RangeProof;
 
-use core::core::SumCommit;
+use core::core::{Input, OutputIdentifier, SumCommit};
 use core::core::pmmr::{HashSum, NoSum};
 
-use core::core::{Block, BlockHeader, Output, TxKernel};
+use core::core::{Block, BlockHeader, TxKernel};
 use core::core::target::Difficulty;
 use core::core::hash::Hash;
 use grin_store::Error::NotFoundErr;
@@ -34,6 +34,7 @@ use sumtree;
 use types::*;
 use util::LOGGER;
 
+
 const MAX_ORPHAN_AGE_SECS: u64 = 30;
 
 #[derive(Debug, Clone)]
@@ -331,31 +332,23 @@ impl Chain {
 		}
 	}
 
-	/// Gets an unspent output from its commitment. With return None if the
-	/// output doesn't exist or has been spent. This querying is done in a
-	/// way that's consistent with the current chain state and more
-	/// specifically the current winning fork.
-	pub fn get_unspent(&self, output_ref: &Commitment) -> Result<Output, Error> {
-		match self.store.get_output_by_commit(output_ref) {
-			Ok(out) => {
-				let mut sumtrees = self.sumtrees.write().unwrap();
-				if sumtrees.is_unspent(output_ref)? {
-					Ok(out)
-				} else {
-					Err(Error::OutputNotFound)
-				}
-			}
-			Err(NotFoundErr) => Err(Error::OutputNotFound),
-			Err(e) => Err(Error::StoreErr(e, "chain get unspent".to_owned())),
-		}
-	}
-
-	/// Checks whether an output is unspent
-	pub fn is_unspent(&self, output_ref: &Commitment) -> Result<bool, Error> {
+	/// For the given commitment find the unspent output and return the associated
+	/// Return an error if the output does not exist or has been 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<(), Error> {
 		let mut sumtrees = self.sumtrees.write().unwrap();
 		sumtrees.is_unspent(output_ref)
 	}
 
+	/// Check if the input has matured sufficiently for the given block height.
+	/// This only applies to inputs spending coinbase outputs.
+	/// An input spending a non-coinbase output will always pass this check.
+	pub fn is_matured(&self, input: &Input, height: u64) -> Result<(), Error> {
+		let mut sumtrees = self.sumtrees.write().unwrap();
+		sumtrees.is_matured(input, height)
+	}
+
 	/// Sets the sumtree roots on a brand new block by applying the block on the
 	/// current sumtree state.
 	pub fn set_sumtree_roots(&self, b: &mut Block, is_fork: bool) -> Result<(), Error> {
@@ -391,17 +384,9 @@ impl Chain {
 	}
 
 	/// returns the last n nodes inserted into the utxo sum tree
-	/// returns sum tree hash plus output itself (as the sum is contained
-	/// in the output anyhow)
-	pub fn get_last_n_utxo(&self, distance: u64) -> Vec<(Hash, Output)> {
+	pub fn get_last_n_utxo(&self, distance: u64) -> Vec<HashSum<SumCommit>> {
 		let mut sumtrees = self.sumtrees.write().unwrap();
-		let mut return_vec = Vec::new();
-		let sum_nodes = sumtrees.last_n_utxo(distance);
-		for sum_commit in sum_nodes {
-			let output = self.store.get_output_by_commit(&sum_commit.sum.commit);
-			return_vec.push((sum_commit.hash, output.unwrap()));
-		}
-		return_vec
+		sumtrees.last_n_utxo(distance)
 	}
 
 	/// as above, for rangeproofs
@@ -469,16 +454,6 @@ impl Chain {
 		})
 	}
 
-	/// Gets the block header by the provided output commitment
-	pub fn get_block_header_by_output_commit(
-		&self,
-		commit: &Commitment,
-	) -> Result<BlockHeader, Error> {
-		self.store
-			.get_block_header_by_output_commit(commit)
-			.map_err(|e| Error::StoreErr(e, "chain get commitment".to_owned()))
-	}
-
 	/// Get the tip of the current "sync" header chain.
 	/// This may be significantly different to current header chain.
 	pub fn get_sync_head(&self) -> Result<Tip, Error> {

chain/src/pipe.rs

@@ -22,7 +22,6 @@ use core::consensus;
 use core::core::hash::{Hash, Hashed};
 use core::core::{Block, BlockHeader};
 use core::core::target::Difficulty;
-use core::core::transaction;
 use grin_store;
 use types::*;
 use store;
@@ -291,21 +290,6 @@ fn validate_block(
 		return Err(Error::InvalidRoot);
 	}
 
-	// check for any outputs with lock_heights greater than current block height
-	for input in &b.inputs {
-		if let Ok(output) = ctx.store.get_output_by_commit(&input.commitment()) {
-			if output.features.contains(transaction::COINBASE_OUTPUT) {
-				if let Ok(output_header) = ctx.store
-					.get_block_header_by_output_commit(&input.commitment())
-				{
-					if b.header.height <= output_header.height + global::coinbase_maturity() {
-						return Err(Error::ImmatureCoinbase);
-					}
-				};
-			};
-		};
-	}
-
 	Ok(())
 }
 

chain/src/store.rs

@@ -20,7 +20,7 @@ use util::secp::pedersen::Commitment;
 
 use types::*;
 use core::core::hash::{Hash, Hashed};
-use core::core::{Block, BlockHeader, Output};
+use core::core::{Block, BlockHeader};
 use core::consensus::TargetError;
 use core::core::target::Difficulty;
 use grin_store::{self, option_to_not_found, to_key, Error, u64_to_key};
@@ -33,8 +33,6 @@ const HEAD_PREFIX: u8 = 'H' as u8;
 const HEADER_HEAD_PREFIX: u8 = 'I' as u8;
 const SYNC_HEAD_PREFIX: u8 = 's' as u8;
 const HEADER_HEIGHT_PREFIX: u8 = '8' as u8;
-const OUTPUT_COMMIT_PREFIX: u8 = 'o' as u8;
-const HEADER_BY_OUTPUT_PREFIX: u8 = 'p' as u8;
 const COMMIT_POS_PREFIX: u8 = 'c' as u8;
 const KERNEL_POS_PREFIX: u8 = 'k' as u8;
 
@@ -106,70 +104,21 @@ impl ChainStore for ChainKVStore {
 		)
 	}
 
-	fn check_block_exists(&self, h: &Hash) -> Result<bool, Error> {
-		self.db.exists(&to_key(BLOCK_PREFIX, &mut h.to_vec()))
-	}
-
+	/// Save the block and its header
 	fn save_block(&self, b: &Block) -> Result<(), Error> {
-		// saving the block and its header
-		let mut batch = self.db
+		let batch = self.db
 			.batch()
-			.put_ser(&to_key(BLOCK_PREFIX, &mut b.hash().to_vec())[..], b)?
+			.put_ser(
+				&to_key(BLOCK_PREFIX, &mut b.hash().to_vec())[..],
+				b,
+			)?
 			.put_ser(
 				&to_key(BLOCK_HEADER_PREFIX, &mut b.hash().to_vec())[..],
 				&b.header,
 			)?;
-
-		// saving the full output under its hash, as well as a commitment to hash index
-		for out in &b.outputs {
-			batch = batch
-				.put_ser(
-					&to_key(
-						OUTPUT_COMMIT_PREFIX,
-						&mut out.commitment().as_ref().to_vec(),
-					)[..],
-					out,
-				)?
-				.put_ser(
-					&to_key(
-						HEADER_BY_OUTPUT_PREFIX,
-						&mut out.commitment().as_ref().to_vec(),
-					)[..],
-					&b.hash(),
-				)?;
-		}
 		batch.write()
 	}
 
-	// lookup the block header hash by output commitment
-	// lookup the block header based on this hash
-	// to check the chain is correct compare this block header to
-	// the block header currently indexed at the relevant block height (tbd if
-	// actually necessary)
-	//
-	// NOTE: This index is not exhaustive.
-	// This node may not have seen this full block, so may not have populated the
-	// index.
-	// Block headers older than some threshold (2 months?) will not necessarily be
-	// included
-	// in this index.
-	//
-	fn get_block_header_by_output_commit(&self, commit: &Commitment) -> Result<BlockHeader, Error> {
-		let block_hash = self.db.get_ser(&to_key(
-			HEADER_BY_OUTPUT_PREFIX,
-			&mut commit.as_ref().to_vec(),
-		))?;
-
-		match block_hash {
-			Some(hash) => {
-				let block_header = self.get_block_header(&hash)?;
-				self.is_on_current_chain(&block_header)?;
-				Ok(block_header)
-			}
-			None => Err(Error::NotFoundErr),
-		}
-	}
-
 	fn is_on_current_chain(&self, header: &BlockHeader) -> Result<(), Error> {
 		let header_at_height = self.get_header_by_height(header.height)?;
 		if header.hash() == header_at_height.hash() {
@@ -194,13 +143,6 @@ impl ChainStore for ChainKVStore {
 		self.db.delete(&u64_to_key(HEADER_HEIGHT_PREFIX, height))
 	}
 
-	fn get_output_by_commit(&self, commit: &Commitment) -> Result<Output, Error> {
-		option_to_not_found(
-			self.db
-				.get_ser(&to_key(OUTPUT_COMMIT_PREFIX, &mut commit.as_ref().to_vec())),
-		)
-	}
-
 	fn save_output_pos(&self, commit: &Commitment, pos: u64) -> Result<(), Error> {
 		self.db.put_ser(
 			&to_key(COMMIT_POS_PREFIX, &mut commit.as_ref().to_vec())[..],

chain/src/sumtree.rs

@@ -20,9 +20,7 @@ use std::collections::HashMap;
 use std::path::Path;
 use std::sync::Arc;
 
-use util::secp::pedersen::{RangeProof, Commitment};
-
-use core::core::{Block, SumCommit, Input, Output, TxKernel, COINBASE_OUTPUT};
+use core::core::{Block, SumCommit, Input, Output, OutputIdentifier, TxKernel, COINBASE_OUTPUT};
 use core::core::pmmr::{HashSum, NoSum, Summable, PMMR};
 use core::core::hash::Hashed;
 use grin_store;
@@ -30,6 +28,7 @@ use grin_store::sumtree::PMMRBackend;
 use types::ChainStore;
 use types::Error;
 use util::LOGGER;
+use util::secp::pedersen::{RangeProof, Commitment};
 
 const SUMTREES_SUBDIR: &'static str = "sumtrees";
 const UTXO_SUBDIR: &'static str = "utxo";
@@ -89,30 +88,62 @@ impl SumTrees {
 		})
 	}
 
-	/// Whether a given commitment exists in the Output MMR and it's unspent
-	pub fn is_unspent(&mut self, commit: &Commitment) -> Result<bool, Error> {
-		let rpos = self.commit_index.get_output_pos(commit);
-		match rpos {
+	/// Check is 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(&mut self, output: &OutputIdentifier) -> Result<(), Error> {
+		match self.commit_index.get_output_pos(&output.commit) {
 			Ok(pos) => {
 				let output_pmmr = PMMR::at(
 					&mut self.output_pmmr_h.backend,
-					self.output_pmmr_h.last_pos
+					self.output_pmmr_h.last_pos,
 				);
-				if let Some(hs) = output_pmmr.get(pos) {
-					let hashsum = HashSum::from_summable(
-						pos,
-						&SumCommit::from_commit(&commit),
-					);
-					Ok(hs.hash == hashsum.hash)
+				if let Some(HashSum { hash, sum: _ }) = output_pmmr.get(pos) {
+					let sum_commit = output.as_sum_commit();
+					let hash_sum = HashSum::from_summable(pos, &sum_commit);
+					if hash == hash_sum.hash {
+						Ok(())
+					} else {
+						Err(Error::SumTreeErr(format!("sumtree hash mismatch")))
+					}
 				} else {
-					Ok(false)
+					Err(Error::OutputNotFound)
 				}
 			}
-			Err(grin_store::Error::NotFoundErr) => Ok(false),
-			Err(e) => Err(Error::StoreErr(e, "sumtree unspent check".to_owned())),
+			Err(grin_store::Error::NotFoundErr) => Err(Error::OutputNotFound),
+			Err(e) => Err(Error::StoreErr(e, format!("sumtree unspent check"))),
 		}
 	}
 
+	/// Check the output being spent by the input has sufficiently matured.
+	/// This only applies for coinbase outputs being spent (1,000 blocks).
+	/// Non-coinbase outputs will always pass this check.
+	/// For a coinbase output we find the block by the block hash provided in the input
+	/// and check coinbase maturty based on the height of this block.
+	pub fn is_matured(
+		&mut self,
+		input: &Input,
+		height: u64,
+	) -> Result<(), Error> {
+		// We should never be in a situation where we are checking maturity rules
+		// if the output is already spent (this should have already been checked).
+		let output = OutputIdentifier::from_input(&input);
+		assert!(self.is_unspent(&output).is_ok());
+
+		// At this point we can be sure the input is spending the output
+		// it claims to be spending, and that it is coinbase or non-coinbase.
+		// If we are spending a coinbase output then go find the block
+		// and check the coinbase maturity rule is being met.
+		if input.features.contains(COINBASE_OUTPUT) {
+			let block_hash = &input.out_block
+				.expect("input spending coinbase output must have a block hash");
+			let block = self.commit_index.get_block(&block_hash)?;
+			block.verify_coinbase_maturity(&input, height)
+				.map_err(|_| Error::ImmatureCoinbase)?;
+		}
+		Ok(())
+	}
+
 	/// returns the last N nodes inserted into the tree (i.e. the 'bottom'
 	/// nodes at level 0
 	pub fn last_n_utxo(&mut self, distance: u64) -> Vec<HashSum<SumCommit>> {
@@ -253,8 +284,8 @@ impl<'a> Extension<'a> {
 				self.apply_output(out)?;
 			}
 		}
-	
-		// then doing inputsm guarantees an input can't spend an output in the
+
+		// then doing inputs guarantees an input can't spend an output in the
 		// same block, enforcing block cut-through
 		for input in &b.inputs {
 			self.apply_input(input, b.header.height)?;
@@ -266,7 +297,7 @@ impl<'a> Extension<'a> {
 				self.apply_output(out)?;
 			}
 		}
-	
+
 		// finally, applying all kernels
 		for kernel in &b.kernels {
 			self.apply_kernel(kernel)?;
@@ -275,24 +306,10 @@ impl<'a> Extension<'a> {
 	}
 
 	fn save_pos_index(&self) -> Result<(), Error> {
-		debug!(
-			LOGGER,
-			"sumtree: save_pos_index: outputs: {}, {:?}",
-			self.new_output_commits.len(),
-			self.new_output_commits.values().collect::<Vec<_>>(),
-		);
-
 		for (commit, pos) in &self.new_output_commits {
 			self.commit_index.save_output_pos(commit, *pos)?;
 		}
 
-		debug!(
-			LOGGER,
-			"sumtree: save_pos_index: kernels: {}, {:?}",
-			self.new_kernel_excesses.len(),
-			self.new_kernel_excesses.values().collect::<Vec<_>>(),
-		);
-
 		for (excess, pos) in &self.new_kernel_excesses {
 			self.commit_index.save_kernel_pos(excess, *pos)?;
 		}
@@ -304,6 +321,32 @@ impl<'a> Extension<'a> {
 		let commit = input.commitment();
 		let pos_res = self.get_output_pos(&commit);
 		if let Ok(pos) = pos_res {
+			if let Some(HashSum { hash, sum: _ }) = self.output_pmmr.get(pos) {
+				let sum_commit = SumCommit::from_input(&input);
+
+				// check hash from pmmr matches hash from input
+				// if not then the input is not being honest about
+				// what it is attempting to spend...
+				let hash_sum = HashSum::from_summable(pos, &sum_commit);
+				if hash != hash_sum.hash {
+					return Err(Error::SumTreeErr(format!("output pmmr hash mismatch")));
+				}
+
+				// At this point we can be sure the input is spending the output
+				// it claims to be spending, and it is coinbase or non-coinbase.
+				// If we are spending a coinbase output then go find the block
+				// and check the coinbase maturity rule is being met.
+				if input.features.contains(COINBASE_OUTPUT) {
+					let block_hash = &input.out_block
+						.expect("input spending coinbase output must have a block hash");
+					let block = self.commit_index.get_block(&block_hash)?;
+					block.verify_coinbase_maturity(&input, height)
+						.map_err(|_| Error::ImmatureCoinbase)?;
+				}
+			}
+
+			// Now prune the output_pmmr and rproof_pmmr.
+			// Input is not valid if we cannot prune successfully (to spend an unspent output).
 			match self.output_pmmr.prune(pos, height as u32) {
 				Ok(true) => {
 					self.rproof_pmmr
@@ -321,11 +364,7 @@ impl<'a> Extension<'a> {
 
 	fn apply_output(&mut self, out: &Output) -> Result<(), Error> {
 		let commit = out.commitment();
-		let switch_commit_hash = out.switch_commit_hash();
-		let sum_commit = SumCommit {
-			commit,
-			switch_commit_hash,
-		};
+		let sum_commit = SumCommit::from_output(out);
 
 		if let Ok(pos) = self.get_output_pos(&commit) {
 			// we need to check whether the commitment is in the current MMR view
@@ -334,12 +373,12 @@ impl<'a> Extension<'a> {
 			// note that this doesn't show the commitment *never* existed, just
 			// that this is not an existing unspent commitment right now
 			if let Some(c) = self.output_pmmr.get(pos) {
-				let hashsum = HashSum::from_summable(pos, &sum_commit);
+				let hash_sum = HashSum::from_summable(pos, &sum_commit);
 
 				// processing a new fork so we may get a position on the old
 				// fork that exists but matches a different node
 				// filtering that case out
-				if c.hash == hashsum.hash {
+				if c.hash == hash_sum.hash {
 					return Err(Error::DuplicateCommitment(commit));
 				}
 			}

chain/src/types.rs

@@ -19,7 +19,7 @@ use std::io;
 use util::secp::pedersen::Commitment;
 
 use grin_store as store;
-use core::core::{block, Block, BlockHeader, Output};
+use core::core::{Block, BlockHeader, block, transaction};
 use core::core::hash::{Hash, Hashed};
 use core::core::target::Difficulty;
 use core::ser;
@@ -58,6 +58,8 @@ pub enum Error {
 	InvalidBlockHeight,
 	/// One of the root hashes in the block is invalid
 	InvalidRoot,
+	/// Something does not look right with the switch commitment
+	InvalidSwitchCommit,
 	/// One of the inputs in the block has already been spent
 	AlreadySpent(Commitment),
 	/// An output with that commitment already exists (should be unique)
@@ -76,10 +78,12 @@ pub enum Error {
 	StoreErr(grin_store::Error, String),
 	/// Error serializing or deserializing a type
 	SerErr(ser::Error),
-	/// Error while updating the sum trees
+	/// Error with the sumtrees
 	SumTreeErr(String),
 	/// No chain exists and genesis block is required
 	GenesisBlockRequired,
+	/// Error from underlying tx handling
+	Transaction(transaction::Error),
 	/// Anything else
 	Other(String),
 }
@@ -117,6 +121,12 @@ impl Error {
 	}
 }
 
+impl From<transaction::Error> for Error {
+	fn from(e: transaction::Error) -> Error {
+		Error::Transaction(e)
+	}
+}
+
 /// The tip of a fork. A handle to the fork ancestry from its leaf in the
 /// blockchain tree. References the max height and the latest and previous
 /// blocks
@@ -202,9 +212,6 @@ pub trait ChainStore: Send + Sync {
 	/// Gets a block header by hash
 	fn get_block_header(&self, h: &Hash) -> Result<BlockHeader, store::Error>;
 
-	/// Checks whether a block has been been processed and saved
-	fn check_block_exists(&self, h: &Hash) -> Result<bool, store::Error>;
-
 	/// Save the provided block in store
 	fn save_block(&self, b: &Block) -> Result<(), store::Error>;
 
@@ -236,15 +243,6 @@ pub trait ChainStore: Send + Sync {
 	/// Use the header_by_height index to verify the block header is where we think it is.
 	fn is_on_current_chain(&self, header: &BlockHeader) -> Result<(), store::Error>;
 
-	/// Gets an output by its commitment
-	fn get_output_by_commit(&self, commit: &Commitment) -> Result<Output, store::Error>;
-
-	/// Gets a block_header for the given input commit
-	fn get_block_header_by_output_commit(
-		&self,
-		commit: &Commitment,
-	) -> Result<BlockHeader, store::Error>;
-
 	/// Saves the position of an output, represented by its commitment, in the
 	/// UTXO MMR. Used as an index for spending and pruning.
 	fn save_output_pos(&self, commit: &Commitment, pos: u64) -> Result<(), store::Error>;