From 44d818d81334f55059c909cb77f733f99cf031e6 Mon Sep 17 00:00:00 2001
From: Wilmer Paulino <wilmer.paulino@gmail.com>
Date: Wed, 10 Jul 2019 15:54:28 -0700
Subject: [PATCH] wtxmgr: prevent adding existing confirmed transactions as
 unconfirmed

We do this as a sanity check for users of the wallet to ensure the
wallet maintains a correct representation of the chain.
---
 wtxmgr/tx_test.go     | 62 +++++++++++++++++++++++++++++++++++++++++++
 wtxmgr/unconfirmed.go | 13 +++++----
 2 files changed, 70 insertions(+), 5 deletions(-)

diff --git a/wtxmgr/tx_test.go b/wtxmgr/tx_test.go
index 484999f..8914e1a 100644
--- a/wtxmgr/tx_test.go
+++ b/wtxmgr/tx_test.go
@@ -1460,6 +1460,68 @@ func TestRemoveUnminedTx(t *testing.T) {
 	checkBalance(btcutil.Amount(initialBalance), true)
 }
 
+// TestInsertMempoolTxAlreadyConfirmed ensures that transactions that already
+// exist within the store as confirmed cannot be added as unconfirmed.
+func TestInsertMempoolTxAlreadyConfirmed(t *testing.T) {
+	t.Parallel()
+
+	store, db, teardown, err := testStore()
+	if err != nil {
+		t.Fatal(err)
+	}
+	defer teardown()
+
+	// In order to reproduce real-world scenarios, we'll use a new database
+	// transaction for each interaction with the wallet.
+	//
+	// We'll start off the test by creating a new coinbase output at height
+	// 100 and inserting it into the store.
+	b100 := &BlockMeta{
+		Block: Block{Height: 100},
+		Time:  time.Now(),
+	}
+	tx := newCoinBase(1e8)
+	txRec, err := NewTxRecordFromMsgTx(tx, b100.Time)
+	if err != nil {
+		t.Fatal(err)
+	}
+	commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) {
+		if err := store.InsertTx(ns, txRec, b100); err != nil {
+			t.Fatal(err)
+		}
+	})
+
+	// checkStore is a helper we'll use to ensure the transaction only
+	// exists within the store's confirmed bucket.
+	checkStore := func() {
+		t.Helper()
+		commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) {
+			if existsRawUnmined(ns, txRec.Hash[:]) != nil {
+				t.Fatalf("expected transaction to not exist " +
+					"in unconfirmed bucket")
+			}
+			_, v := existsTxRecord(ns, &txRec.Hash, &b100.Block)
+			if v == nil {
+				t.Fatalf("expected transaction to exist in " +
+					"confirmed bucket")
+			}
+		})
+	}
+
+	checkStore()
+
+	// Inserting the transaction again as unconfirmed should result in a
+	// NOP, i.e., no error should be returned and no disk modifications are
+	// needed.
+	commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) {
+		if err := store.InsertTx(ns, txRec, nil); err != nil {
+			t.Fatal(err)
+		}
+	})
+
+	checkStore()
+}
+
 // TestOutputsAfterRemoveDoubleSpend ensures that when we remove a transaction
 // that double spends an existing output within the wallet, it doesn't remove
 // any other spending transactions of the same output.
diff --git a/wtxmgr/unconfirmed.go b/wtxmgr/unconfirmed.go
index a27925d..fc0d13b 100644
--- a/wtxmgr/unconfirmed.go
+++ b/wtxmgr/unconfirmed.go
@@ -14,11 +14,14 @@ import (
 // insertMemPoolTx inserts the unmined transaction record.  It also marks
 // previous outputs referenced by the inputs as spent.
 func (s *Store) insertMemPoolTx(ns walletdb.ReadWriteBucket, rec *TxRecord) error {
-	// Check whether the transaction has already been added to the
-	// unconfirmed bucket.
-	if existsRawUnmined(ns, rec.Hash[:]) != nil {
-		// TODO: compare serialized txs to ensure this isn't a hash
-		// collision?
+	// Check whether the transaction has already been added to the store,
+	// regardless of whether is has confirmed or not. This ensures that we
+	// don't add it to the unconfirmed bucket again if it has already
+	// confirmed.
+	//
+	// TODO: compare serialized txs to ensure this isn't a hash
+	// collision?
+	if txDetails, _ := s.TxDetails(ns, &rec.Hash); txDetails != nil {
 		return nil
 	}