Merge pull request #609 from wpaulino/utxos-after-remove

wtxmgr: only remove entry for 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
 		}
 	}