From a71881aed81cc6862d1f21a1e61097d80b76afa3 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 29 Aug 2018 17:02:52 -0700 Subject: [PATCH 1/5] wtxmgr/tx_test: move store teardown after error check --- wtxmgr/tx_test.go | 28 ++++++++++++++++------------ 1 file changed, 16 insertions(+), 12 deletions(-) diff --git a/wtxmgr/tx_test.go b/wtxmgr/tx_test.go index 56873fc..33dc16e 100644 --- a/wtxmgr/tx_test.go +++ b/wtxmgr/tx_test.go @@ -64,17 +64,18 @@ func testStore() (*Store, walletdb.DB, func(), error) { if err != nil { return nil, nil, func() {}, err } + db, err := walletdb.Create("bdb", filepath.Join(tmpDir, "db")) if err != nil { - teardown := func() { - os.RemoveAll(tmpDir) - } - return nil, nil, teardown, err + os.RemoveAll(tmpDir) + return nil, nil, nil, err } + teardown := func() { db.Close() os.RemoveAll(tmpDir) } + var s *Store err = walletdb.Update(db, func(tx walletdb.ReadWriteTx) error { ns, err := tx.CreateTopLevelBucket(namespaceKey) @@ -88,6 +89,7 @@ func testStore() (*Store, walletdb.DB, func(), error) { s, err = Open(ns, &chaincfg.TestNet3Params) return err }) + return s, db, teardown, err } @@ -489,10 +491,10 @@ func TestInsertsCreditsDebitsRollbacks(t *testing.T) { } s, db, teardown, err := testStore() - defer teardown() if err != nil { t.Fatal(err) } + defer teardown() for _, test := range tests { err := walletdb.Update(db, func(tx walletdb.ReadWriteTx) error { @@ -560,10 +562,10 @@ func TestFindingSpentCredits(t *testing.T) { t.Parallel() s, db, teardown, err := testStore() - defer teardown() if err != nil { t.Fatal(err) } + defer teardown() dbtx, err := db.BeginReadWriteTx() if err != nil { @@ -667,10 +669,10 @@ func TestCoinbases(t *testing.T) { t.Parallel() s, db, teardown, err := testStore() - defer teardown() if err != nil { t.Fatal(err) } + defer teardown() dbtx, err := db.BeginReadWriteTx() if err != nil { @@ -1073,10 +1075,10 @@ func TestMoveMultipleToSameBlock(t *testing.T) { t.Parallel() s, db, teardown, err := testStore() - defer teardown() if err != nil { t.Fatal(err) } + defer teardown() dbtx, err := db.BeginReadWriteTx() if err != nil { @@ -1250,10 +1252,10 @@ func TestInsertUnserializedTx(t *testing.T) { t.Parallel() s, db, teardown, err := testStore() - defer teardown() if err != nil { t.Fatal(err) } + defer teardown() dbtx, err := db.BeginReadWriteTx() if err != nil { @@ -1317,10 +1319,10 @@ func TestRemoveUnminedTx(t *testing.T) { t.Parallel() store, db, teardown, err := testStore() - defer teardown() if err != nil { t.Fatal(err) } + defer teardown() dbtx, err := db.BeginReadWriteTx() if err != nil { @@ -1382,6 +1384,8 @@ func TestRemoveUnminedTx(t *testing.T) { func commitDBTx(t *testing.T, store *Store, db walletdb.DB, f func(walletdb.ReadWriteBucket)) { + t.Helper() + dbTx, err := db.BeginReadWriteTx() if err != nil { t.Fatal(err) @@ -1401,10 +1405,10 @@ func commitDBTx(t *testing.T, store *Store, db walletdb.DB, // removed from the wallet's store. func testInsertMempoolDoubleSpendTx(t *testing.T, first bool) { store, db, teardown, err := testStore() - defer teardown() 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. @@ -1559,10 +1563,10 @@ func TestInsertConfirmedDoubleSpendTx(t *testing.T) { t.Parallel() store, db, teardown, err := testStore() - defer teardown() 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. From 98f65ac943d5ae4641b30b5ad548ea08bffd3cb7 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 29 Aug 2018 15:09:19 -0700 Subject: [PATCH 2/5] wtxmgr/tx_test: add test case for duplicate outputs within the store In this commit, we add a new test case to the wtxmgr store to ensure that duplicate outputs don't exists within the store. It's possible for this to happen if an output is marked as unconfirmed credit, then marked as confirmed once it confirms, and once again marked as unconfirmed. It can be marked as unconfirmed again due to the backend notifying the client about this transaction. Ideally this should not happen, but the root cause is much more involved. As a stop gap, we'll ensure that outputs can be marked as unconfirmed credits more than once whatsoever. As is, the test case fails, which proves that this is an issue. A later commit will resolve this and the test case should pass. --- wtxmgr/tx_test.go | 134 ++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 134 insertions(+) diff --git a/wtxmgr/tx_test.go b/wtxmgr/tx_test.go index 33dc16e..4125849 100644 --- a/wtxmgr/tx_test.go +++ b/wtxmgr/tx_test.go @@ -1728,3 +1728,137 @@ func TestInsertConfirmedDoubleSpendTx(t *testing.T) { } }) } + +// TestAddDuplicateCreditAfterConfirm aims to test the case where a duplicate +// unconfirmed credit is added to the store after the intial credit has already +// confirmed. This can lead to outputs being duplicated in the store, which can +// lead to creating double spends when querying the wallet's UTXO set. +func TestAddDuplicateCreditAfterConfirm(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, b100, 0, false) + if err != nil { + t.Fatal(err) + } + }) + + // We'll confirm that there is one unspent output in the store, which + // should be the coinbase output created above. + commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) { + minedTxs, err := store.UnspentOutputs(ns) + if err != nil { + t.Fatal(err) + } + if len(minedTxs) != 1 { + t.Fatalf("expected 1 mined tx, got %v", len(minedTxs)) + } + if !minedTxs[0].Hash.IsEqual(&cbRec.Hash) { + t.Fatalf("expected tx hash %v, got %v", cbRec.Hash, + minedTxs[0].Hash) + } + }) + + // Then, we'll create an unconfirmed spend for the coinbase output. + b101 := &BlockMeta{ + Block: Block{Height: 101}, + Time: time.Now(), + } + spendTx := spendOutput(&cbRec.Hash, 0, 5e7, 4e7) + spendTxRec, err := NewTxRecordFromMsgTx(spendTx, b101.Time) + if err != nil { + t.Fatal(err) + } + commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) { + if err := store.InsertTx(ns, spendTxRec, nil); err != nil { + t.Fatal(err) + } + err := store.AddCredit(ns, spendTxRec, nil, 1, true) + if err != nil { + t.Fatal(err) + } + }) + + // Confirm the spending transaction at the next height. + commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) { + if err := store.InsertTx(ns, spendTxRec, b101); err != nil { + t.Fatal(err) + } + err := store.AddCredit(ns, spendTxRec, b101, 1, true) + if err != nil { + t.Fatal(err) + } + }) + + // We should see one unspent output within the store once again, this + // time being the change output of the spending transaction. + commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) { + minedTxs, err := store.UnspentOutputs(ns) + if err != nil { + t.Fatal(err) + } + if len(minedTxs) != 1 { + t.Fatalf("expected 1 mined txs, got %v", len(minedTxs)) + } + if !minedTxs[0].Hash.IsEqual(&spendTxRec.Hash) { + t.Fatalf("expected tx hash %v, got %v", spendTxRec.Hash, + minedTxs[0].Hash) + } + }) + + // Now, we'll insert the spending transaction once again, this time as + // unconfirmed. This can happen if the backend happens to forward an + // unconfirmed chain.RelevantTx notification to the client even after it + // has confirmed, which results in us adding it to the store once again. + // + // TODO(wilmer): ideally this shouldn't happen, so we should identify + // the real reason for this. + commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) { + if err := store.InsertTx(ns, spendTxRec, nil); err != nil { + t.Fatal(err) + } + err := store.AddCredit(ns, spendTxRec, nil, 1, true) + if err != nil { + t.Fatal(err) + } + }) + + // Finally, we'll ensure the change output is still the only unspent + // output within the store. + commitDBTx(t, store, db, func(ns walletdb.ReadWriteBucket) { + minedTxs, err := store.UnspentOutputs(ns) + if err != nil { + t.Fatal(err) + } + if len(minedTxs) != 1 { + t.Fatalf("expected 1 mined txs, got %v", len(minedTxs)) + } + if !minedTxs[0].Hash.IsEqual(&spendTxRec.Hash) { + t.Fatalf("expected tx hash %v, got %v", spendTxRec.Hash, + minedTxs[0].Hash) + } + }) +} From fbe82c3531344a0422bc5454f82ed44c83e93124 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 29 Aug 2018 15:28:40 -0700 Subject: [PATCH 3/5] wtxmgr: check existing unspent outputs before adding the credit In this commit, we resolve a lingering bug within the wallet where it's possible that an output is added as unconfirmed credit after the fact that it has already confirmed. This would lead to duplicate entries for the same output within the wallet causing double spend transactions to be crafted. --- wtxmgr/tx.go | 6 ++++++ wtxmgr/unconfirmed.go | 19 ++++++++++++++++--- 2 files changed, 22 insertions(+), 3 deletions(-) diff --git a/wtxmgr/tx.go b/wtxmgr/tx.go index a9b9651..0b232ea 100644 --- a/wtxmgr/tx.go +++ b/wtxmgr/tx.go @@ -405,10 +405,16 @@ func (s *Store) AddCredit(ns walletdb.ReadWriteBucket, rec *TxRecord, block *Blo // duplicate (false). func (s *Store) addCredit(ns walletdb.ReadWriteBucket, rec *TxRecord, block *BlockMeta, index uint32, change bool) (bool, error) { if block == nil { + // If the outpoint that we should mark as credit already exists + // within the store, either as unconfirmed or confirmed, then we + // have nothing left to do and can exit. k := canonicalOutPoint(&rec.Hash, index) if existsRawUnminedCredit(ns, k) != nil { return false, nil } + if existsRawUnspent(ns, k) != nil { + return false, nil + } v := valueUnminedCredit(btcutil.Amount(rec.MsgTx.TxOut[index].Value), change) return true, putRawUnminedCredit(ns, k, v) } diff --git a/wtxmgr/unconfirmed.go b/wtxmgr/unconfirmed.go index 812d2d2..37bf7e1 100644 --- a/wtxmgr/unconfirmed.go +++ b/wtxmgr/unconfirmed.go @@ -14,12 +14,25 @@ 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 { - v := existsRawUnmined(ns, rec.Hash[:]) - if v != nil { - // TODO: compare serialized txs to ensure this isn't a hash collision? + // 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? return nil } + // Since transaction records within the store are keyed by their + // transaction _and_ block confirmation, we'll iterate through the + // transaction's outputs to determine if we've already seen them to + // prevent from adding this transaction to the unconfirmed bucket. + for i := range rec.MsgTx.TxOut { + k := canonicalOutPoint(&rec.Hash, uint32(i)) + if existsRawUnspent(ns, k) != nil { + return nil + } + } + log.Infof("Inserting unconfirmed transaction %v", rec.Hash) v, err := valueTxRecord(rec) if err != nil { From 4a7f2c10780e7415d8c0abc55362f765cdb9fe44 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 29 Aug 2018 16:56:25 -0700 Subject: [PATCH 4/5] wtxmgr/query_test: remove t.Fatal calls within db transactions in TestStoreQueries --- wtxmgr/query_test.go | 287 ++++++++++++++++++++++--------------------- 1 file changed, 144 insertions(+), 143 deletions(-) diff --git a/wtxmgr/query_test.go b/wtxmgr/query_test.go index f5a2f50..6a9bb18 100644 --- a/wtxmgr/query_test.go +++ b/wtxmgr/query_test.go @@ -7,6 +7,7 @@ package wtxmgr_test import ( "bytes" "encoding/binary" + "errors" "fmt" "testing" "time" @@ -64,12 +65,8 @@ func deepCopyTxDetails(d *TxDetails) *TxDetails { return &cpy } -func (q *queryState) compare(t *testing.T, s *Store, ns walletdb.ReadBucket, changeDesc string) { - defer func() { - if t.Failed() { - t.Fatalf("Store state queries failed after '%s'", changeDesc) - } - }() +func (q *queryState) compare(s *Store, ns walletdb.ReadBucket, + changeDesc string) error { fwdBlocks := q.blocks revBlocks := make([][]TxDetails, len(q.blocks)) @@ -80,17 +77,22 @@ func (q *queryState) compare(t *testing.T, s *Store, ns walletdb.ReadBucket, cha checkBlock := func(blocks [][]TxDetails) func([]TxDetails) (bool, error) { return func(got []TxDetails) (bool, error) { if len(fwdBlocks) == 0 { - return false, fmt.Errorf("entered range when no more details expected") + return false, errors.New("entered range " + + "when no more details expected") } exp := blocks[0] if len(got) != len(exp) { - return false, fmt.Errorf("got len(details)=%d in transaction range, expected %d", len(got), len(exp)) + return false, fmt.Errorf("got len(details)=%d "+ + "in transaction range, expected %d", + len(got), len(exp)) } for i := range got { - equalTxDetails(t, &got[i], &exp[i]) - } - if t.Failed() { - return false, fmt.Errorf("Failed comparing range of transaction details") + err := equalTxDetails(&got[i], &exp[i]) + if err != nil { + return false, fmt.Errorf("failed "+ + "comparing range of "+ + "transaction details: %v", err) + } } blocks = blocks[1:] return false, nil @@ -98,11 +100,13 @@ func (q *queryState) compare(t *testing.T, s *Store, ns walletdb.ReadBucket, cha } err := s.RangeTransactions(ns, 0, -1, checkBlock(fwdBlocks)) if err != nil { - t.Fatalf("Failed in RangeTransactions (forwards iteration): %v", err) + return fmt.Errorf("%s: failed in RangeTransactions (forwards "+ + "iteration): %v", changeDesc, err) } err = s.RangeTransactions(ns, -1, 0, checkBlock(revBlocks)) if err != nil { - t.Fatalf("Failed in RangeTransactions (reverse iteration): %v", err) + return fmt.Errorf("%s: failed in RangeTransactions (reverse "+ + "iteration): %v", changeDesc, err) } for txHash, details := range q.txDetails { @@ -113,16 +117,18 @@ func (q *queryState) compare(t *testing.T, s *Store, ns walletdb.ReadBucket, cha } d, err := s.UniqueTxDetails(ns, &txHash, blk) if err != nil { - t.Fatal(err) + return err } if d == nil { - t.Errorf("Found no matching transaction at height %d", detail.Block.Height) - continue + return fmt.Errorf("found no matching "+ + "transaction at height %d", + detail.Block.Height) + } + if err := equalTxDetails(d, &detail); err != nil { + return fmt.Errorf("%s: failed querying latest "+ + "details regarding transaction %v", + changeDesc, txHash) } - equalTxDetails(t, d, &detail) - } - if t.Failed() { - t.Fatalf("Failed querying unique details regarding transaction %v", txHash) } // For the most recent tx with this hash, check that @@ -131,79 +137,80 @@ func (q *queryState) compare(t *testing.T, s *Store, ns walletdb.ReadBucket, cha detail := &details[len(details)-1] d, err := s.TxDetails(ns, &txHash) if err != nil { - t.Fatal(err) + return err } - equalTxDetails(t, d, detail) - if t.Failed() { - t.Fatalf("Failed querying latest details regarding transaction %v", txHash) + if err := equalTxDetails(d, detail); err != nil { + return fmt.Errorf("%s: failed querying latest details "+ + "regarding transaction %v", changeDesc, txHash) } } + + return nil } -func equalTxDetails(t *testing.T, got, exp *TxDetails) { +func equalTxDetails(got, exp *TxDetails) error { // Need to avoid using reflect.DeepEqual against slices, since it // returns false for nil vs non-nil zero length slices. + if err := equalTxs(&got.MsgTx, &exp.MsgTx); err != nil { + return err + } - equalTxs(t, &got.MsgTx, &exp.MsgTx) if got.Hash != exp.Hash { - t.Errorf("Found mismatched hashes") - t.Errorf("Got: %v", got.Hash) - t.Errorf("Expected: %v", exp.Hash) + return fmt.Errorf("found mismatched hashes: got %v, expected %v", + got.Hash, exp.Hash) } if got.Received != exp.Received { - t.Errorf("Found mismatched receive time") - t.Errorf("Got: %v", got.Received) - t.Errorf("Expected: %v", exp.Received) + return fmt.Errorf("found mismatched receive time: got %v, "+ + "expected %v", got.Received, exp.Received) } if !bytes.Equal(got.SerializedTx, exp.SerializedTx) { - t.Errorf("Found mismatched serialized txs") - t.Errorf("Got: %x", got.SerializedTx) - t.Errorf("Expected: %x", exp.SerializedTx) + return fmt.Errorf("found mismatched serialized txs: got %v, "+ + "expected %v", got.SerializedTx, exp.SerializedTx) } if got.Block != exp.Block { - t.Errorf("Found mismatched block meta") - t.Errorf("Got: %v", got.Block) - t.Errorf("Expected: %v", exp.Block) + return fmt.Errorf("found mismatched block meta: got %v, "+ + "expected %v", got.Block, exp.Block) } if len(got.Credits) != len(exp.Credits) { - t.Errorf("Credit slice lengths differ: Got %d Expected %d", len(got.Credits), len(exp.Credits)) - } else { - for i := range got.Credits { - if got.Credits[i] != exp.Credits[i] { - t.Errorf("Found mismatched Credit[%d]", i) - t.Errorf("Got: %v", got.Credits[i]) - t.Errorf("Expected: %v", exp.Credits[i]) - } + return fmt.Errorf("credit slice lengths differ: got %d, "+ + "expected %d", len(got.Credits), len(exp.Credits)) + } + for i := range got.Credits { + if got.Credits[i] != exp.Credits[i] { + return fmt.Errorf("found mismatched credit[%d]: got %v, "+ + "expected %v", i, got.Credits[i], exp.Credits[i]) } } if len(got.Debits) != len(exp.Debits) { - t.Errorf("Debit slice lengths differ: Got %d Expected %d", len(got.Debits), len(exp.Debits)) - } else { - for i := range got.Debits { - if got.Debits[i] != exp.Debits[i] { - t.Errorf("Found mismatched Debit[%d]", i) - t.Errorf("Got: %v", got.Debits[i]) - t.Errorf("Expected: %v", exp.Debits[i]) - } + return fmt.Errorf("debit slice lengths differ: got %d, "+ + "expected %d", len(got.Debits), len(exp.Debits)) + } + for i := range got.Debits { + if got.Debits[i] != exp.Debits[i] { + return fmt.Errorf("found mismatched debit[%d]: got %v, "+ + "expected %v", i, got.Debits[i], exp.Debits[i]) } } + + return nil } -func equalTxs(t *testing.T, got, exp *wire.MsgTx) { +func equalTxs(got, exp *wire.MsgTx) error { var bufGot, bufExp bytes.Buffer err := got.Serialize(&bufGot) if err != nil { - t.Fatal(err) + return err } err = exp.Serialize(&bufExp) if err != nil { - t.Fatal(err) + return err } if !bytes.Equal(bufGot.Bytes(), bufExp.Bytes()) { - t.Errorf("Found unexpected wire.MsgTx:") - t.Errorf("Got: %v", got) - t.Errorf("Expected: %v", exp) + return fmt.Errorf("found unexpected wire.MsgTx: got: %v, "+ + "expected %v", got, exp) } + + return nil } // Returns time.Now() with seconds resolution, this is what Store saves. @@ -238,7 +245,7 @@ func TestStoreQueries(t *testing.T) { type queryTest struct { desc string - updates func(ns walletdb.ReadWriteBucket) // Unwinds from t.Fatal if the update errors. + updates func(ns walletdb.ReadWriteBucket) error state *queryState } var tests []queryTest @@ -252,40 +259,16 @@ func TestStoreQueries(t *testing.T) { lastState := newQueryState() tests = append(tests, queryTest{ desc: "initial store", - updates: func(walletdb.ReadWriteBucket) {}, + updates: func(walletdb.ReadWriteBucket) error { return nil }, state: lastState, }) - // simplify error handling - insertTx := func(ns walletdb.ReadWriteBucket, rec *TxRecord, block *BlockMeta) { - err := s.InsertTx(ns, rec, block) - if err != nil { - t.Fatal(err) - } - } - addCredit := func(s *Store, ns walletdb.ReadWriteBucket, rec *TxRecord, block *BlockMeta, index uint32, change bool) { - err := s.AddCredit(ns, rec, block, index, change) - if err != nil { - t.Fatal(err) - } - } - newTxRecordFromMsgTx := func(tx *wire.MsgTx, received time.Time) *TxRecord { - rec, err := NewTxRecordFromMsgTx(tx, received) - if err != nil { - t.Fatal(err) - } - return rec - } - rollback := func(ns walletdb.ReadWriteBucket, height int32) { - err := s.Rollback(ns, height) - if err != nil { - t.Fatal(err) - } - } - // Insert an unmined transaction. Mark no credits yet. txA := spendOutput(&chainhash.Hash{}, 0, 100e8) - recA := newTxRecordFromMsgTx(txA, timeNow()) + recA, err := NewTxRecordFromMsgTx(txA, timeNow()) + if err != nil { + t.Fatal(err) + } newState := lastState.deepCopy() newState.blocks = [][]TxDetails{ { @@ -300,9 +283,11 @@ func TestStoreQueries(t *testing.T) { } lastState = newState tests = append(tests, queryTest{ - desc: "insert tx A unmined", - updates: func(ns walletdb.ReadWriteBucket) { insertTx(ns, recA, nil) }, - state: newState, + desc: "insert tx A unmined", + updates: func(ns walletdb.ReadWriteBucket) error { + return s.InsertTx(ns, recA, nil) + }, + state: newState, }) // Add txA:0 as a change credit. @@ -318,15 +303,20 @@ func TestStoreQueries(t *testing.T) { newState.txDetails[recA.Hash][0].Credits = newState.blocks[0][0].Credits lastState = newState tests = append(tests, queryTest{ - desc: "mark unconfirmed txA:0 as credit", - updates: func(ns walletdb.ReadWriteBucket) { addCredit(s, ns, recA, nil, 0, true) }, - state: newState, + desc: "mark unconfirmed txA:0 as credit", + updates: func(ns walletdb.ReadWriteBucket) error { + return s.AddCredit(ns, recA, nil, 0, true) + }, + state: newState, }) // Insert another unmined transaction which spends txA:0, splitting the // amount into outputs of 40 and 60 BTC. txB := spendOutput(&recA.Hash, 0, 40e8, 60e8) - recB := newTxRecordFromMsgTx(txB, timeNow()) + recB, err := NewTxRecordFromMsgTx(txB, timeNow()) + if err != nil { + t.Fatal(err) + } newState = lastState.deepCopy() newState.blocks[0][0].Credits[0].Spent = true newState.blocks[0] = append(newState.blocks[0], TxDetails{ @@ -343,9 +333,11 @@ func TestStoreQueries(t *testing.T) { newState.txDetails[recB.Hash] = []TxDetails{newState.blocks[0][1]} lastState = newState tests = append(tests, queryTest{ - desc: "insert tx B unmined", - updates: func(ns walletdb.ReadWriteBucket) { insertTx(ns, recB, nil) }, - state: newState, + desc: "insert tx B unmined", + updates: func(ns walletdb.ReadWriteBucket) error { + return s.InsertTx(ns, recB, nil) + }, + state: newState, }) newState = lastState.deepCopy() newState.blocks[0][1].Credits = []CreditRecord{ @@ -359,9 +351,11 @@ func TestStoreQueries(t *testing.T) { newState.txDetails[recB.Hash][0].Credits = newState.blocks[0][1].Credits lastState = newState tests = append(tests, queryTest{ - desc: "mark txB:0 as non-change credit", - updates: func(ns walletdb.ReadWriteBucket) { addCredit(s, ns, recB, nil, 0, false) }, - state: newState, + desc: "mark txB:0 as non-change credit", + updates: func(ns walletdb.ReadWriteBucket) error { + return s.AddCredit(ns, recB, nil, 0, false) + }, + state: newState, }) // Mine tx A at block 100. Leave tx B unmined. @@ -373,9 +367,11 @@ func TestStoreQueries(t *testing.T) { newState.txDetails[recA.Hash][0].Block = b100 lastState = newState tests = append(tests, queryTest{ - desc: "mine tx A", - updates: func(ns walletdb.ReadWriteBucket) { insertTx(ns, recA, &b100) }, - state: newState, + desc: "mine tx A", + updates: func(ns walletdb.ReadWriteBucket) error { + return s.InsertTx(ns, recA, &b100) + }, + state: newState, }) // Mine tx B at block 101. @@ -385,17 +381,20 @@ func TestStoreQueries(t *testing.T) { newState.txDetails[recB.Hash][0].Block = b101 lastState = newState tests = append(tests, queryTest{ - desc: "mine tx B", - updates: func(ns walletdb.ReadWriteBucket) { insertTx(ns, recB, &b101) }, - state: newState, + desc: "mine tx B", + updates: func(ns walletdb.ReadWriteBucket) error { + return s.InsertTx(ns, recB, &b101) + }, + state: newState, }) for _, tst := range tests { err := walletdb.Update(db, func(tx walletdb.ReadWriteTx) error { ns := tx.ReadWriteBucket(namespaceKey) - tst.updates(ns) - tst.state.compare(t, s, ns, tst.desc) - return nil + if err := tst.updates(ns); err != nil { + return err + } + return tst.state.compare(s, ns, tst.desc) }) if err != nil { t.Fatal(err) @@ -414,14 +413,18 @@ func TestStoreQueries(t *testing.T) { ns := tx.ReadWriteBucket(namespaceKey) missingTx := spendOutput(&recB.Hash, 0, 40e8) - missingRec := newTxRecordFromMsgTx(missingTx, timeNow()) + missingRec, err := NewTxRecordFromMsgTx(missingTx, timeNow()) + if err != nil { + return err + } missingBlock := makeBlockMeta(102) missingDetails, err := s.TxDetails(ns, &missingRec.Hash) if err != nil { - t.Fatal(err) + return err } if missingDetails != nil { - t.Errorf("Expected no details, found details for tx %v", missingDetails.Hash) + return fmt.Errorf("Expected no details, found details "+ + "for tx %v", missingDetails.Hash) } missingUniqueTests := []struct { hash *chainhash.Hash @@ -461,7 +464,9 @@ func TestStoreQueries(t *testing.T) { t.Errorf("RangeTransactions (reverse) ran func %d times", iterations) } // Make sure it also breaks early after one iteration through unmined transactions. - rollback(ns, b101.Height) + if err := s.Rollback(ns, b101.Height); err != nil { + return err + } iterations = 0 err = s.RangeTransactions(ns, -1, 0, func([]TxDetails) (bool, error) { iterations++ @@ -487,9 +492,11 @@ func TestStoreQueries(t *testing.T) { newState.txDetails[recB.Hash][0].Block = b100 lastState = newState tests = append(tests[:0:0], queryTest{ - desc: "move tx B to block 100", - updates: func(ns walletdb.ReadWriteBucket) { insertTx(ns, recB, &b100) }, - state: newState, + desc: "move tx B to block 100", + updates: func(ns walletdb.ReadWriteBucket) error { + return s.InsertTx(ns, recB, &b100) + }, + state: newState, }) newState = lastState.deepCopy() newState.blocks[0][0].Block = makeBlockMeta(-1) @@ -498,9 +505,11 @@ func TestStoreQueries(t *testing.T) { newState.txDetails[recB.Hash][0].Block = makeBlockMeta(-1) lastState = newState tests = append(tests, queryTest{ - desc: "rollback block 100", - updates: func(ns walletdb.ReadWriteBucket) { rollback(ns, b100.Height) }, - state: newState, + desc: "rollback block 100", + updates: func(ns walletdb.ReadWriteBucket) error { + return s.Rollback(ns, b100.Height) + }, + state: newState, }) // None of the above tests have tested transactions with colliding @@ -525,31 +534,23 @@ func TestStoreQueries(t *testing.T) { newState.txDetails[recA.Hash] = append(newState.txDetails[recA.Hash], newState.blocks[1][0]) lastState = newState tests = append(tests, queryTest{ - desc: "insert duplicate tx A", - updates: func(ns walletdb.ReadWriteBucket) { insertTx(ns, recA, &b100); insertTx(ns, recA, nil) }, - state: newState, - }) - newState = lastState.deepCopy() - newState.blocks = [][]TxDetails{ - newState.blocks[0], - {newState.blocks[1][0]}, - {newState.blocks[1][1]}, - } - newState.blocks[1][0].Block = b101 - newState.txDetails[recA.Hash][1].Block = b101 - lastState = newState - tests = append(tests, queryTest{ - desc: "mine duplicate tx A", - updates: func(ns walletdb.ReadWriteBucket) { insertTx(ns, recA, &b101) }, - state: newState, + desc: "insert duplicate tx A", + updates: func(ns walletdb.ReadWriteBucket) error { + if err := s.InsertTx(ns, recA, &b100); err != nil { + return err + } + return s.InsertTx(ns, recA, nil) + }, + state: newState, }) for _, tst := range tests { err := walletdb.Update(db, func(tx walletdb.ReadWriteTx) error { ns := tx.ReadWriteBucket(namespaceKey) - tst.updates(ns) - tst.state.compare(t, s, ns, tst.desc) - return nil + if err := tst.updates(ns); err != nil { + return err + } + return tst.state.compare(s, ns, tst.desc) }) if err != nil { t.Fatal(err) From 989a81eb24be422d6cb40b907fb80722fa89a638 Mon Sep 17 00:00:00 2001 From: Wilmer Paulino Date: Wed, 29 Aug 2018 16:56:50 -0700 Subject: [PATCH 5/5] wtxmgr/query_test: remove duplicate hash test case In this commit, we remove the duplicate test case from TestStoreQueries as we'll no longer allow storing a transaction as unconfirmed if it's already confirmed. --- wtxmgr/query_test.go | 32 -------------------------------- 1 file changed, 32 deletions(-) diff --git a/wtxmgr/query_test.go b/wtxmgr/query_test.go index 6a9bb18..bd2acf1 100644 --- a/wtxmgr/query_test.go +++ b/wtxmgr/query_test.go @@ -512,38 +512,6 @@ func TestStoreQueries(t *testing.T) { state: newState, }) - // None of the above tests have tested transactions with colliding - // hashes, so mine tx A in block 100, and then insert tx A again - // unmined. Also mine tx A in block 101 (this moves it from unmined). - // This is a valid test because the store does not perform signature - // validation or keep a full utxo set, and duplicated transaction hashes - // from different blocks are allowed so long as all previous outputs are - // spent. - newState = lastState.deepCopy() - newState.blocks = append(newState.blocks, newState.blocks[0][1:]) - newState.blocks[0] = newState.blocks[0][:1:1] - newState.blocks[0][0].Block = b100 - newState.blocks[1] = []TxDetails{ - { - TxRecord: *stripSerializedTx(recA), - Block: makeBlockMeta(-1), - }, - newState.blocks[1][0], - } - newState.txDetails[recA.Hash][0].Block = b100 - newState.txDetails[recA.Hash] = append(newState.txDetails[recA.Hash], newState.blocks[1][0]) - lastState = newState - tests = append(tests, queryTest{ - desc: "insert duplicate tx A", - updates: func(ns walletdb.ReadWriteBucket) error { - if err := s.InsertTx(ns, recA, &b100); err != nil { - return err - } - return s.InsertTx(ns, recA, nil) - }, - state: newState, - }) - for _, tst := range tests { err := walletdb.Update(db, func(tx walletdb.ReadWriteTx) error { ns := tx.ReadWriteBucket(namespaceKey)