From 1c43ed92940905ff4f4a2be323d03e6f86e0b528 Mon Sep 17 00:00:00 2001 From: Roei Erez Date: Mon, 28 Oct 2019 12:24:19 +0200 Subject: [PATCH] wtxmgr: fix broadcast existing transaction This commit ensures the wallet won't enter an inconsitent state by checking tx confirmation before adding credit. Without this fix, In the case the existing transaction is already confirmed on-chain the flow updates the bucketUnminedCredits but without adding en entry also to the bucketUnmined resulting in inconsistent state. --- wtxmgr/tx.go | 4 +- wtxmgr/tx_test.go | 106 +++++++++++++++++++++++++++++++++++++++------- 2 files changed, 94 insertions(+), 16 deletions(-) diff --git a/wtxmgr/tx.go b/wtxmgr/tx.go index b69142f..526d399 100644 --- a/wtxmgr/tx.go +++ b/wtxmgr/tx.go @@ -404,7 +404,9 @@ func (s *Store) addCredit(ns walletdb.ReadWriteBucket, rec *TxRecord, block *Blo if existsRawUnminedCredit(ns, k) != nil { return false, nil } - if existsRawUnspent(ns, k) != nil { + if _, tv := latestTxRecord(ns, &rec.Hash); tv != nil { + log.Tracef("Ignoring credit for existing confirmed transaction %v", + rec.Hash.String()) return false, nil } v := valueUnminedCredit(btcutil.Amount(rec.MsgTx.TxOut[index].Value), change) diff --git a/wtxmgr/tx_test.go b/wtxmgr/tx_test.go index 8914e1a..de46e9f 100644 --- a/wtxmgr/tx_test.go +++ b/wtxmgr/tx_test.go @@ -162,7 +162,7 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) { bal: 0, unc: btcutil.Amount(TstRecvTx.MsgTx().TxOut[0].Value), unspents: map[wire.OutPoint]struct{}{ - wire.OutPoint{ + { Hash: *TstRecvTx.Hash(), Index: 0, }: {}, @@ -189,7 +189,7 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) { bal: 0, unc: btcutil.Amount(TstRecvTx.MsgTx().TxOut[0].Value), unspents: map[wire.OutPoint]struct{}{ - wire.OutPoint{ + { Hash: *TstRecvTx.Hash(), Index: 0, }: {}, @@ -216,7 +216,7 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) { bal: btcutil.Amount(TstRecvTx.MsgTx().TxOut[0].Value), unc: 0, unspents: map[wire.OutPoint]struct{}{ - wire.OutPoint{ + { Hash: *TstRecvTx.Hash(), Index: 0, }: {}, @@ -241,7 +241,7 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) { bal: btcutil.Amount(TstRecvTx.MsgTx().TxOut[0].Value), unc: 0, unspents: map[wire.OutPoint]struct{}{ - wire.OutPoint{ + { Hash: *TstRecvTx.Hash(), Index: 0, }: {}, @@ -257,7 +257,7 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) { bal: 0, unc: btcutil.Amount(TstRecvTx.MsgTx().TxOut[0].Value), unspents: map[wire.OutPoint]struct{}{ - wire.OutPoint{ + { Hash: *TstRecvTx.Hash(), Index: 0, }: {}, @@ -284,7 +284,7 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) { bal: btcutil.Amount(TstDoubleSpendTx.MsgTx().TxOut[0].Value), unc: 0, unspents: map[wire.OutPoint]struct{}{ - wire.OutPoint{ + { Hash: *TstDoubleSpendTx.Hash(), Index: 0, }: {}, @@ -343,7 +343,7 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) { bal: 0, unc: btcutil.Amount(TstSpendingTx.MsgTx().TxOut[0].Value), unspents: map[wire.OutPoint]struct{}{ - wire.OutPoint{ + { Hash: *TstSpendingTx.Hash(), Index: 0, }: {}, @@ -369,11 +369,11 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) { bal: 0, unc: btcutil.Amount(TstSpendingTx.MsgTx().TxOut[0].Value + TstSpendingTx.MsgTx().TxOut[1].Value), unspents: map[wire.OutPoint]struct{}{ - wire.OutPoint{ + { Hash: *TstSpendingTx.Hash(), Index: 0, }: {}, - wire.OutPoint{ + { Hash: *TstSpendingTx.Hash(), Index: 1, }: {}, @@ -395,11 +395,11 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) { bal: btcutil.Amount(TstSpendingTx.MsgTx().TxOut[0].Value + TstSpendingTx.MsgTx().TxOut[1].Value), unc: 0, unspents: map[wire.OutPoint]struct{}{ - wire.OutPoint{ + { Hash: *TstSpendingTx.Hash(), Index: 0, }: {}, - wire.OutPoint{ + { Hash: *TstSpendingTx.Hash(), Index: 1, }: {}, @@ -415,11 +415,11 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) { bal: btcutil.Amount(TstSpendingTx.MsgTx().TxOut[0].Value + TstSpendingTx.MsgTx().TxOut[1].Value), unc: 0, unspents: map[wire.OutPoint]struct{}{ - wire.OutPoint{ + { Hash: *TstSpendingTx.Hash(), Index: 0, }: {}, - wire.OutPoint{ + { Hash: *TstSpendingTx.Hash(), Index: 1, }: {}, @@ -435,11 +435,11 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) { bal: 0, unc: btcutil.Amount(TstSpendingTx.MsgTx().TxOut[0].Value + TstSpendingTx.MsgTx().TxOut[1].Value), unspents: map[wire.OutPoint]struct{}{ - wire.OutPoint{ + { Hash: *TstSpendingTx.Hash(), Index: 0, }: {}, - wire.OutPoint{ + { Hash: *TstSpendingTx.Hash(), Index: 1, }: {}, @@ -1522,6 +1522,82 @@ func TestInsertMempoolTxAlreadyConfirmed(t *testing.T) { checkStore() } +// TestInsertMempoolTxAfterSpentOutput ensures that transactions that were +// both confirmed and spent cannot be added as unconfirmed. +func TestInsertMempoolTxAfterSpentOutput(t *testing.T) { + t.Parallel() + + store, db, teardown, err := testStore() + if err != nil { + t.Fatal(err) + } + defer teardown() + + // First we add a confirmed transaction to the wallet. + 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, &b100, 0, false) + if err != nil { + t.Fatal(err) + } + }) + + // Then create a transaction that spends the previous tx output. + b101 := BlockMeta{ + Block: Block{Height: 101}, + Time: time.Now(), + } + amt := int64(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) { + // We add the spending tx to the wallet as confirmed. + err := store.InsertTx(ns, spendRec, &b101) + if err != nil { + t.Fatal(err) + } + err = store.AddCredit(ns, spendRec, &b101, 0, false) + if err != nil { + t.Fatal(err) + } + + // We now adding the original transaction as mempool to simulate + // a real case where trying to broadcast a tx after it has been + // confirmed and spent. + if err := store.InsertTx(ns, cbRec, nil); err != nil { + t.Fatal(err) + } + err = store.AddCredit(ns, cbRec, nil, 0, false) + if err != nil { + t.Fatal(err) + } + }) + + // now we check that there no unminedCredit exists for the original tx. + commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) { + k := canonicalOutPoint(&cbRec.Hash, 0) + if existsRawUnminedCredit(ns, k) != nil { + t.Fatalf("expected output to not exist " + + "in unmined credit bucket") + } + }) +} + // 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.