Merge pull request #751 from LN-Zap/fix-2398

Prevent duplicate notifications for (un)confirmed transactions

wallet/chainntfns.go

@@ -280,11 +280,19 @@ func (w *Wallet) addRelevantTx(dbtx walletdb.ReadWriteTx, rec *wtxmgr.TxRecord,
 	// relevant.  This assumption will not hold true when SPV support is
 	// added, but until then, simply insert the transaction because there
 	// should either be one or more relevant inputs or outputs.
-	err := w.TxStore.InsertTx(txmgrNs, rec, block)
+	exists, err := w.TxStore.InsertTxCheckIfExists(txmgrNs, rec, block)
 	if err != nil {
 		return err
 	}
 
+	// If the transaction has already been recorded, we can return early.
+	// Note: Returning here is safe as we're within the context of an atomic
+	// database transaction, so we don't need to worry about the MarkUsed
+	// calls below.
+	if exists {
+		return nil
+	}
+
 	// Check every output to determine whether it is controlled by a wallet
 	// key.  If so, mark the output as a credit.
 	for i, output := range rec.MsgTx.TxOut {

wtxmgr/tx.go

@@ -55,6 +55,10 @@ var (
 	// ErrOutputUnlockNotAllowed is an error returned when an output unlock
 	// is attempted with a different ID than the one which locked it.
 	ErrOutputUnlockNotAllowed = errors.New("output unlock not alowed")
+
+	// ErrDuplicateTx is returned when attempting to record a mined or
+	// unmined transaction that is already recorded.
+	ErrDuplicateTx = errors.New("transaction already exists")
 )
 
 // Block contains the minimum amount of data to uniquely identify any block on
@@ -355,11 +359,30 @@ func (s *Store) deleteUnminedTx(ns walletdb.ReadWriteBucket, rec *TxRecord) erro
 // InsertTx records a transaction as belonging to a wallet's transaction
 // history.  If block is nil, the transaction is considered unspent, and the
 // transaction's index must be unset.
-func (s *Store) InsertTx(ns walletdb.ReadWriteBucket, rec *TxRecord, block *BlockMeta) error {
+func (s *Store) InsertTx(ns walletdb.ReadWriteBucket, rec *TxRecord,
-	if block == nil {
+	block *BlockMeta) error {
-		return s.insertMemPoolTx(ns, rec)
+	_, err := s.InsertTxCheckIfExists(ns, rec, block)
+	return err
 }
-	return s.insertMinedTx(ns, rec, block)
+
+// InsertTxCheckIfExists records a transaction as belonging to a wallet's
+// transaction history.  If block is nil, the transaction is considered unspent,
+// and the transaction's index must be unset. It will return true if the
+// transaction was already recorded prior to the call.
+func (s *Store) InsertTxCheckIfExists(ns walletdb.ReadWriteBucket,
+	rec *TxRecord, block *BlockMeta) (bool, error) {
+
+	var err error
+	if block == nil {
+		if err = s.insertMemPoolTx(ns, rec); err == ErrDuplicateTx {
+			return true, nil
+		}
+		return false, err
+	}
+	if err = s.insertMinedTx(ns, rec, block); err == ErrDuplicateTx {
+		return true, nil
+	}
+	return false, err
 }
 
 // RemoveUnminedTx attempts to remove an unmined transaction from the
@@ -386,7 +409,7 @@ func (s *Store) insertMinedTx(ns walletdb.ReadWriteBucket, rec *TxRecord,
 	// If a transaction record for this hash and block already exists, we
 	// can exit early.
 	if _, v := existsTxRecord(ns, &rec.Hash, &block.Block); v != nil {
-		return nil
+		return ErrDuplicateTx
 	}
 
 	// If a block record does not yet exist for any transactions from this

wtxmgr/tx_test.go

@@ -7,6 +7,7 @@ package wtxmgr
 import (
 	"bytes"
 	"encoding/hex"
+	"fmt"
 	"io/ioutil"
 	"os"
 	"path/filepath"
@@ -192,6 +193,13 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) {
 					return nil, err
 				}
 
+				// Check that the duplicate transaction is found.
+				if exists, _ := s.InsertTxCheckIfExists(ns, rec, nil); !exists {
+					return nil, fmt.Errorf(
+						"duplicate transaction was not found as already recorded",
+					)
+				}
+
 				err = s.AddCredit(ns, rec, nil, 0, false)
 				return s, err
 			},
@@ -244,6 +252,13 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) {
 					return nil, err
 				}
 
+				// Make sure the duplicate transaction is found.
+				if exists, _ := s.InsertTxCheckIfExists(ns, rec, TstRecvTxBlockDetails); !exists {
+					return nil, fmt.Errorf(
+						"duplicate transaction was not found as already recorded",
+					)
+				}
+
 				err = s.AddCredit(ns, rec, TstRecvTxBlockDetails, 0, false)
 				return s, err
 			},

wtxmgr/unconfirmed.go

@@ -22,7 +22,7 @@ func (s *Store) insertMemPoolTx(ns walletdb.ReadWriteBucket, rec *TxRecord) erro
 	// TODO: compare serialized txs to ensure this isn't a hash
 	// collision?
 	if txDetails, _ := s.TxDetails(ns, &rec.Hash); txDetails != nil {
-		return nil
+		return ErrDuplicateTx
 	}
 
 	// Since transaction records within the store are keyed by their