From 44d818d81334f55059c909cb77f733f99cf031e6 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 10 Jul 2019 15:54:28 -0700 Subject: [PATCH 1/2] wtxmgr: prevent adding existing confirmed transactions as unconfirmed We do this as a sanity check for users of the wallet to ensure the wallet maintains a correct representation of the chain. --- wtxmgr/tx_test.go | 62 +++++++++++++++++++++++++++++++++++++++++++ wtxmgr/unconfirmed.go | 13 +++++---- 2 files changed, 70 insertions(+), 5 deletions(-) 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 } From 02b0a7c18d80396a4263dad308077166809f04a4 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 10 Jul 2019 15:56:44 -0700 Subject: [PATCH 2/2] wallet: fix nil txid in publishTransaction A nil txid could've been returned from publishTransaction even if it was successful. This was due to the underlying SendRawTransaction call "failing", e.g., when the transaction being broadcast has already confirmed, but publishTranasction interpreting such failure as a success. --- wallet/wallet.go | 9 +++++---- 1 file changed, 5 insertions(+), 4 deletions(-) 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