wtxmgr: only remove entry for specified spending transaction

In this commit, we address an issue with the wallet store where it'd be
possible for us to keep lingering unconfirmed transaction entries for an
output that has been spent by a different, confirmed transaction. This
was caused due to us removing all spending transaction entries for a
given output when removing conflicts. Since all of the entries would be
removed, we weren't able to retrieve the hashes of the remaining
spending transactions to remove their records as well. Instead, we
propose to only remove the entry for the specified spending transaction.

wtxmgr/db.go

@@ -1246,12 +1246,43 @@ func fetchUnminedInputSpendTxHashes(ns walletdb.ReadBucket, k []byte) []chainhas
 	return spendTxHashes
 }
 
-func deleteRawUnminedInput(ns walletdb.ReadWriteBucket, k []byte) error {
-	err := ns.NestedReadWriteBucket(bucketUnminedInputs).Delete(k)
+// deleteRawUnminedInput removes a spending transaction entry from the list of
+// spending transactions for a given input.
+func deleteRawUnminedInput(ns walletdb.ReadWriteBucket, outPointKey []byte,
+	targetSpendHash chainhash.Hash) error {
+
+	// We'll start by fetching all of the possible spending transactions.
+	unminedInputs := ns.NestedReadWriteBucket(bucketUnminedInputs)
+	spendHashes := unminedInputs.Get(outPointKey)
+	if len(spendHashes) == 0 {
+		return nil
+	}
+
+	// We'll iterate through them and pick all the ones that don't match the
+	// specified spending transaction.
+	var newSpendHashes []byte
+	numHashes := len(spendHashes) / 32
+	for i, idx := 0, 0; i < numHashes; i, idx = i+1, idx+32 {
+		spendHash := spendHashes[idx : idx+32]
+		if !bytes.Equal(targetSpendHash[:], spendHash) {
+			newSpendHashes = append(newSpendHashes, spendHash...)
+		}
+	}
+
+	// If there aren't any entries left after filtering them, then we can
+	// remove the record completely. Otherwise, we'll store the filtered
+	// records.
+	var err error
+	if len(newSpendHashes) == 0 {
+		err = unminedInputs.Delete(outPointKey)
+	} else {
+		err = unminedInputs.Put(outPointKey, newSpendHashes)
+	}
 	if err != nil {
-		str := "failed to delete unmined input"
+		str := "failed to delete unmined input spend"
 		return storeError(ErrDatabase, str, err)
 	}
+
 	return nil
 }
 

wtxmgr/tx_test.go

@@ -1460,6 +1460,120 @@ func TestRemoveUnminedTx(t *testing.T) {
 	checkBalance(btcutil.Amount(initialBalance), true)
 }
 
+// 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.
+func TestOutputsAfterRemoveDoubleSpend(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(),
+	}
+	cb := newCoinBase(1e8)
+	cbRec, err := NewTxRecordFromMsgTx(cb, b100.Time)
+	if err != nil {
+		t.Fatal(err)
+	}
+	commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) {
+		if err := store.InsertTx(ns, cbRec, &b100); err != nil {
+			t.Fatal(err)
+		}
+		err := store.AddCredit(ns, cbRec, nil, 0, false)
+		if err != nil {
+			t.Fatal(err)
+		}
+	})
+
+	// We'll create three spending transactions for the same output and add
+	// them to the store.
+	const numSpendRecs = 3
+	spendRecs := make([]*TxRecord, 0, numSpendRecs)
+	for i := 0; i < numSpendRecs; i++ {
+		amt := int64((i + 1) * 1e7)
+		spend := spendOutput(&cbRec.Hash, 0, amt)
+		spendRec, err := NewTxRecordFromMsgTx(spend, time.Now())
+		if err != nil {
+			t.Fatal(err)
+		}
+
+		commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) {
+			err := store.InsertTx(ns, spendRec, nil)
+			if err != nil {
+				t.Fatal(err)
+			}
+			err = store.AddCredit(ns, spendRec, nil, 0, false)
+			if err != nil {
+				t.Fatal(err)
+			}
+		})
+
+		spendRecs = append(spendRecs, spendRec)
+	}
+
+	// checkOutputs is a helper closure we'll use to ensure none of the
+	// other outputs from spending transactions have been removed from the
+	// store just because we removed one of the spending transactions.
+	checkOutputs := func(txs ...*TxRecord) {
+		t.Helper()
+
+		ops := make(map[wire.OutPoint]struct{})
+		for _, tx := range txs {
+			for i := range tx.MsgTx.TxOut {
+				ops[wire.OutPoint{
+					Hash:  tx.Hash,
+					Index: uint32(i),
+				}] = struct{}{}
+			}
+		}
+
+		commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) {
+			t.Helper()
+
+			outputs, err := store.UnspentOutputs(ns)
+			if err != nil {
+				t.Fatalf("unable to get unspent outputs: %v", err)
+			}
+			if len(outputs) != len(ops) {
+				t.Fatalf("expected %d outputs, got %d", len(ops),
+					len(outputs))
+			}
+			for _, output := range outputs {
+				op := output.OutPoint
+				if _, ok := ops[op]; !ok {
+					t.Fatalf("found unexpected output %v", op)
+				}
+			}
+		})
+	}
+
+	// All of the outputs of our spending transactions should exist.
+	checkOutputs(spendRecs...)
+
+	// We'll then remove the last transaction we crafted from the store and
+	// check our outputs again to ensure they still exist.
+	spendToRemove := spendRecs[numSpendRecs-1]
+	spendRecs = spendRecs[:numSpendRecs-1]
+	commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) {
+		if err := store.RemoveUnminedTx(ns, spendToRemove); err != nil {
+			t.Fatalf("unable to remove unmined transaction: %v", err)
+		}
+	})
+
+	checkOutputs(spendRecs...)
+}
+
 // commitDBTx is a helper function that allows us to perform multiple operations
 // on a specific database's bucket as a single atomic operation.
 func commitDBTx(t *testing.T, store *Store, db walletdb.DB,

wtxmgr/unconfirmed.go

@@ -69,13 +69,19 @@ func (s *Store) removeDoubleSpends(ns walletdb.ReadWriteBucket, rec *TxRecord) e
 
 		doubleSpendHashes := fetchUnminedInputSpendTxHashes(ns, prevOutKey)
 		for _, doubleSpendHash := range doubleSpendHashes {
-			doubleSpendVal := existsRawUnmined(ns, doubleSpendHash[:])
+			// We'll make sure not to remove ourselves.
+			if rec.Hash == doubleSpendHash {
+				continue
+			}
 
 			// If the spending transaction spends multiple outputs
 			// from the same transaction, we'll find duplicate
 			// entries within the store, so it's possible we're
 			// unable to find it if the conflicts have already been
 			// removed in a previous iteration.
+			doubleSpendVal := existsRawUnmined(
+				ns, doubleSpendHash[:],
+			)
 			if doubleSpendVal == nil {
 				continue
 			}
@@ -91,6 +97,7 @@ func (s *Store) removeDoubleSpends(ns walletdb.ReadWriteBucket, rec *TxRecord) e
 
 			log.Debugf("Removing double spending transaction %v",
 				doubleSpend.Hash)
+
 			if err := s.removeConflict(ns, &doubleSpend); err != nil {
 				return err
 			}
@@ -112,13 +119,12 @@ func (s *Store) removeConflict(ns walletdb.ReadWriteBucket, rec *TxRecord) error
 		k := canonicalOutPoint(&rec.Hash, uint32(i))
 		spenderHashes := fetchUnminedInputSpendTxHashes(ns, k)
 		for _, spenderHash := range spenderHashes {
-			spenderVal := existsRawUnmined(ns, spenderHash[:])
-
 			// If the spending transaction spends multiple outputs
 			// from the same transaction, we'll find duplicate
 			// entries within the store, so it's possible we're
 			// unable to find it if the conflicts have already been
 			// removed in a previous iteration.
+			spenderVal := existsRawUnmined(ns, spenderHash[:])
 			if spenderVal == nil {
 				continue
 			}
@@ -147,7 +153,8 @@ func (s *Store) removeConflict(ns walletdb.ReadWriteBucket, rec *TxRecord) error
 	for _, input := range rec.MsgTx.TxIn {
 		prevOut := &input.PreviousOutPoint
 		k := canonicalOutPoint(&prevOut.Hash, prevOut.Index)
-		if err := deleteRawUnminedInput(ns, k); err != nil {
+		err := deleteRawUnminedInput(ns, k, rec.Hash)
+		if err != nil {
 			return err
 		}
 	}