Merge pull request #633 from wpaulino/insert-unconfirmed-after-confimed

wtxmgr: prevent adding existing confirmed transactions as unconfirmed

wallet/wallet.go

@@ -3369,7 +3369,7 @@ func (w *Wallet) publishTransaction(tx *wire.MsgTx) (*chainhash.Hash, error) {
 		return nil, err
 	}
 
-	txid, err := chainClient.SendRawTransaction(tx, false)
+	_, err = chainClient.SendRawTransaction(tx, false)
 
 	// Determine if this was an RPC error thrown due to the transaction
 	// already confirming.
@@ -3378,9 +3378,10 @@ func (w *Wallet) publishTransaction(tx *wire.MsgTx) (*chainhash.Hash, error) {
 		rpcTxConfirmed = rpcErr.Code == btcjson.ErrRPCTxAlreadyInChain
 	}
 
+	txid := tx.TxHash()
 	switch {
 	case err == nil:
-		return txid, nil
+		return &txid, nil
 
 	// Since we have different backends that can be used with the wallet,
 	// we'll need to check specific errors for each one.
@@ -3399,7 +3400,7 @@ func (w *Wallet) publishTransaction(tx *wire.MsgTx) (*chainhash.Hash, error) {
 	case strings.Contains(
 		strings.ToLower(err.Error()), "txn-already-in-mempool",
 	):
-		return txid, nil
+		return &txid, nil
 
 	// If the transaction has already confirmed, we can safely remove it
 	// from the unconfirmed store as it should already exist within the
@@ -3434,7 +3435,7 @@ func (w *Wallet) publishTransaction(tx *wire.MsgTx) (*chainhash.Hash, error) {
 				"from unconfirmed store: %v", tx.TxHash(), dbErr)
 		}
 
-		return txid, nil
+		return &txid, nil
 
 	// If the transaction was rejected for whatever other reason, then we'll
 	// remove it from the transaction store, as otherwise, we'll attempt to

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.

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
+	// Check whether the transaction has already been added to the store,
-	// unconfirmed bucket.
+	// regardless of whether is has confirmed or not. This ensures that we
-	if existsRawUnmined(ns, rec.Hash[:]) != nil {
+	// 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
 	}