From 8fee3b6922b0b8565d7a085ae2550377206be046 Mon Sep 17 00:00:00 2001
From: Antioch Peverell <30642645+antiochp@users.noreply.github.com>
Date: Wed, 20 Jun 2018 16:11:46 -0400
Subject: [PATCH] [consensus breaking] use hash_with_index(pos - 1) when
 checking the hash in the MMR (#1114)

* use hash_with_index(pos - 1) when checking the hash in the MMR

* fix tests, get rid of todo related to duplicate commitments
---
 chain/src/txhashset.rs             | 15 ++++-----------
 pool/tests/block_reconciliation.rs | 17 ++---------------
 2 files changed, 6 insertions(+), 26 deletions(-)

diff --git a/chain/src/txhashset.rs b/chain/src/txhashset.rs
index 4969c9cf9..6deaea9b5 100644
--- a/chain/src/txhashset.rs
+++ b/chain/src/txhashset.rs
@@ -656,17 +656,10 @@ 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(hash) = self.output_pmmr.get_hash(pos) {
-				// 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
-				//
-				// TODO - the following call to hash() is *INCORRECT*
-				// TODO - we should be calling hash_with_index(pos - 1)
-				// TODO - but we cannot safely make this change in testnet2
-				// TODO - with this incorrect behavior we never get a match, so duplicates are
-				// allowed
-				//
-				if hash == OutputIdentifier::from_output(out).hash() {
+				// Check the hash matches what we expect.
+				// We may be on a fork which may result in the entry at that pos being
+				// different to the one we expect.
+				if hash == OutputIdentifier::from_output(out).hash_with_index(pos - 1) {
 					return Err(Error::DuplicateCommitment(commit));
 				}
 			}
diff --git a/pool/tests/block_reconciliation.rs b/pool/tests/block_reconciliation.rs
index ec3c0785b..20c207014 100644
--- a/pool/tests/block_reconciliation.rs
+++ b/pool/tests/block_reconciliation.rs
@@ -199,23 +199,10 @@ fn test_transaction_pool_block_reconciliation() {
 		let mut write_pool = pool.write().unwrap();
 		write_pool.reconcile_block(&block).unwrap();
 
-		// TODO - this is the "correct" behavior (see below)
-		// assert_eq!(write_pool.total_size(), 4);
-		// assert_eq!(write_pool.txpool.entries[0].tx, valid_transaction);
-		// assert_eq!(write_pool.txpool.entries[1].tx, pool_child);
-		// assert_eq!(write_pool.txpool.entries[2].tx, conflict_valid_child);
-		// assert_eq!(write_pool.txpool.entries[3].tx, valid_child_valid);
-
-		//
-		// TODO - once the hash() vs hash_with_index(pos - 1) change is made in
-		// txhashset.apply_output() TODO - and we no longer incorrectly allow
-		// duplicate outputs in the MMR TODO - then this test will fail
-		//
-		assert_eq!(write_pool.total_size(), 5);
+		assert_eq!(write_pool.total_size(), 4);
 		assert_eq!(write_pool.txpool.entries[0].tx, valid_transaction);
 		assert_eq!(write_pool.txpool.entries[1].tx, pool_child);
 		assert_eq!(write_pool.txpool.entries[2].tx, conflict_valid_child);
-		assert_eq!(write_pool.txpool.entries[3].tx, valid_child_conflict);
-		assert_eq!(write_pool.txpool.entries[4].tx, valid_child_valid);
+		assert_eq!(write_pool.txpool.entries[3].tx, valid_child_valid);
 	}
 }