diff --git a/wtxmgr/db.go b/wtxmgr/db.go index 17185d3..28839a6 100644 --- a/wtxmgr/db.go +++ b/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 } diff --git a/wtxmgr/tx_test.go b/wtxmgr/tx_test.go index ad271c0..484999f 100644 --- a/wtxmgr/tx_test.go +++ b/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, diff --git a/wtxmgr/unconfirmed.go b/wtxmgr/unconfirmed.go index 0f171e7..a27925d 100644 --- a/wtxmgr/unconfirmed.go +++ b/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 } }