diff --git a/wallet/wallet.go b/wallet/wallet.go index fe52cb2..f2f9b07 100644 --- a/wallet/wallet.go +++ b/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 diff --git a/wtxmgr/tx_test.go b/wtxmgr/tx_test.go index 484999f..8914e1a 100644 --- a/wtxmgr/tx_test.go +++ b/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. diff --git a/wtxmgr/unconfirmed.go b/wtxmgr/unconfirmed.go index a27925d..fc0d13b 100644 --- a/wtxmgr/unconfirmed.go +++ b/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 - // unconfirmed bucket. - if existsRawUnmined(ns, rec.Hash[:]) != nil { - // TODO: compare serialized txs to ensure this isn't a hash - // collision? + // Check whether the transaction has already been added to the store, + // regardless of whether is has confirmed or not. This ensures that we + // 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 }