From 224b2b0b5c6d60b13b6ee4d1319cf390953f2b4a Mon Sep 17 00:00:00 2001 From: Christopher Schinnerl Date: Wed, 10 Jan 2018 16:33:39 -0500 Subject: [PATCH 1/7] add acceptTransactionSet wrapper for wallet --- modules/wallet/defrag.go | 2 +- modules/wallet/defrag_test.go | 4 ++-- modules/wallet/money.go | 6 +++--- modules/wallet/seed.go | 2 +- modules/wallet/transactionbuilder_test.go | 18 +++++++++--------- modules/wallet/wallet.go | 17 +++++++++++++++++ modules/wallet/wallet_test.go | 8 ++++---- 7 files changed, 37 insertions(+), 20 deletions(-) diff --git a/modules/wallet/defrag.go b/modules/wallet/defrag.go index 922e01054a..98cd998f47 100644 --- a/modules/wallet/defrag.go +++ b/modules/wallet/defrag.go @@ -175,7 +175,7 @@ func (w *Wallet) threadedDefragWallet() { return } // Submit the defrag to the transaction pool. - err = w.tpool.AcceptTransactionSet(txnSet) + err = w.managedCommitTransactionSet(txnSet) if err != nil { w.log.Println("WARN: defrag transaction was rejected:", err) return diff --git a/modules/wallet/defrag_test.go b/modules/wallet/defrag_test.go index 32e81c5bec..6f2456d9b8 100644 --- a/modules/wallet/defrag_test.go +++ b/modules/wallet/defrag_test.go @@ -96,7 +96,7 @@ func TestDefragWalletDust(t *testing.T) { t.Fatal(err) } - err = wt.tpool.AcceptTransactionSet(txns) + err = wt.wallet.managedCommitTransactionSet(txns) if err != nil { t.Fatal(err) } @@ -179,7 +179,7 @@ func TestDefragOutputExhaustion(t *testing.T) { if err != nil { t.Error("Error signing fragmenting transaction:", err) } - err = wt.tpool.AcceptTransactionSet(txns) + err = wt.wallet.managedCommitTransactionSet(txns) if err != nil { t.Error("Error accepting fragmenting transaction:", err) } diff --git a/modules/wallet/money.go b/modules/wallet/money.go index e2259c826f..cb4cb868ae 100644 --- a/modules/wallet/money.go +++ b/modules/wallet/money.go @@ -126,7 +126,7 @@ func (w *Wallet) SendSiacoins(amount types.Currency, dest types.UnlockHash) (txn if w.deps.Disrupt("SendSiacoinsInterrupted") { return nil, errors.New("failed to accept transaction set (SendSiacoinsInterrupted)") } - err = w.tpool.AcceptTransactionSet(txnSet) + err = w.managedCommitTransactionSet(txnSet) if err != nil { w.log.Println("Attempt to send coins has failed - transaction pool rejected transaction:", err) return nil, build.ExtendErr("unable to get transaction accepted", err) @@ -195,7 +195,7 @@ func (w *Wallet) SendSiacoinsMulti(outputs []types.SiacoinOutput) (txns []types. return nil, errors.New("failed to accept transaction set (SendSiacoinsInterrupted)") } w.log.Println("Attempting to broadcast a multi-send over the network") - err = w.tpool.AcceptTransactionSet(txnSet) + err = w.managedCommitTransactionSet(txnSet) if err != nil { w.log.Println("Attempt to send coins has failed - transaction pool rejected transaction:", err) return nil, build.ExtendErr("unable to get transaction accepted", err) @@ -247,7 +247,7 @@ func (w *Wallet) SendSiafunds(amount types.Currency, dest types.UnlockHash) ([]t if err != nil { return nil, err } - err = w.tpool.AcceptTransactionSet(txnSet) + err = w.managedCommitTransactionSet(txnSet) if err != nil { return nil, err } diff --git a/modules/wallet/seed.go b/modules/wallet/seed.go index d6b4d1a3ee..37954dc25a 100644 --- a/modules/wallet/seed.go +++ b/modules/wallet/seed.go @@ -510,7 +510,7 @@ func (w *Wallet) SweepSeed(seed modules.Seed) (coins, funds types.Currency, err txnSet := append(parents, txn) // submit the transactions - err = w.tpool.AcceptTransactionSet(txnSet) + err = w.managedCommitTransactionSet(txnSet) if err != nil { return } diff --git a/modules/wallet/transactionbuilder_test.go b/modules/wallet/transactionbuilder_test.go index 6b610e24b3..842f3363d5 100644 --- a/modules/wallet/transactionbuilder_test.go +++ b/modules/wallet/transactionbuilder_test.go @@ -93,7 +93,7 @@ func TestViewAdded(t *testing.T) { t.Error("seems like there's memory sharing happening between txn calls") } // Set1 should be missing some signatures. - err = wt.tpool.AcceptTransactionSet(set1) + err = wt.wallet.managedCommitTransactionSet(set1) if err == nil { t.Fatal(err) } @@ -108,7 +108,7 @@ func TestViewAdded(t *testing.T) { b2.AddTransactionSignature(unfinishedTxn3.TransactionSignatures[sigIndex]) } set2, err := b2.Sign(true) - err = wt.tpool.AcceptTransactionSet(set2) + err = wt.wallet.managedCommitTransactionSet(set2) if err != nil { t.Fatal(err) } @@ -122,7 +122,7 @@ func TestViewAdded(t *testing.T) { b.AddTransactionSignature(finishedTxn.TransactionSignatures[sigIndex]) } set3Txn, set3Parents := b.View() - err = wt.tpool.AcceptTransactionSet(append(set3Parents, set3Txn)) + err = wt.wallet.managedCommitTransactionSet(append(set3Parents, set3Txn)) if err != modules.ErrDuplicateTransactionSet { t.Fatal(err) } @@ -159,7 +159,7 @@ func TestDoubleSignError(t *testing.T) { if err != nil && txnSet2 != nil { t.Error("errored call to sign did not return a nil txn set") } - err = wt.tpool.AcceptTransactionSet(txnSet) + err = wt.wallet.managedCommitTransactionSet(txnSet) if err != nil { t.Fatal(err) } @@ -235,11 +235,11 @@ func TestConcurrentBuilders(t *testing.T) { if err != nil { t.Fatal(err) } - err = wt.tpool.AcceptTransactionSet(tset1) + err = wt.wallet.managedCommitTransactionSet(tset1) if err != nil { t.Fatal(err) } - err = wt.tpool.AcceptTransactionSet(tset2) + err = wt.wallet.managedCommitTransactionSet(tset2) if err != nil { t.Fatal(err) } @@ -294,7 +294,7 @@ func TestConcurrentBuildersSingleOutput(t *testing.T) { if err != nil { t.Fatal(err) } - err = wt.tpool.AcceptTransactionSet(tSet) + err = wt.wallet.managedCommitTransactionSet(tSet) if err != nil { t.Fatal(err) } @@ -347,7 +347,7 @@ func TestConcurrentBuildersSingleOutput(t *testing.T) { if err != nil { t.Fatal(err) } - err = wt.tpool.AcceptTransactionSet(tset1) + err = wt.wallet.managedCommitTransactionSet(tset1) if err != nil { t.Fatal(err) } @@ -422,7 +422,7 @@ func TestParallelBuilders(t *testing.T) { if err != nil { t.Fatal(err) } - err = wt.tpool.AcceptTransactionSet(tset) + err = wt.wallet.managedCommitTransactionSet(tset) if err != nil { t.Fatal(err) } diff --git a/modules/wallet/wallet.go b/modules/wallet/wallet.go index 5696c44504..0b037c11ad 100644 --- a/modules/wallet/wallet.go +++ b/modules/wallet/wallet.go @@ -123,6 +123,23 @@ func (w *Wallet) Height() types.BlockHeight { return types.BlockHeight(height) } +// acceptTransactionSet is a convenience wrapper for the transaction pools +// commitTransactionSet method. It only returns an error if the transaction was +// rejected and won't be rebroadcasted over time +func (w *Wallet) commitTransactionSet(txns []types.Transaction) error { + w.mu.Unlock() + err := w.tpool.AcceptTransactionSet(txns) + w.mu.Lock() + return err +} + +// managedCommitTransactionSet is a thread-safe version of acceptTransactionSet +func (w *Wallet) managedCommitTransactionSet(txns []types.Transaction) error { + w.mu.Lock() + defer w.mu.Unlock() + return w.commitTransactionSet(txns) +} + // New creates a new wallet, loading any known addresses from the input file // name and then using the file to save in the future. Keys and addresses are // not loaded into the wallet during the call to 'new', but rather during the diff --git a/modules/wallet/wallet_test.go b/modules/wallet/wallet_test.go index 08672ab641..23490124ae 100644 --- a/modules/wallet/wallet_test.go +++ b/modules/wallet/wallet_test.go @@ -373,7 +373,7 @@ func TestAdvanceLookaheadNoRescan(t *testing.T) { t.Fatal(err) } - err = wt.tpool.AcceptTransactionSet(tSet) + err = wt.wallet.managedCommitTransactionSet(tSet) if err != nil { t.Fatal(err) } @@ -447,7 +447,7 @@ func TestAdvanceLookaheadForceRescan(t *testing.T) { t.Fatal(err) } - err = wt.tpool.AcceptTransactionSet(txnSet) + err = wt.wallet.managedCommitTransactionSet(txnSet) if err != nil { t.Fatal(err) } @@ -490,7 +490,7 @@ func TestAdvanceLookaheadForceRescan(t *testing.T) { t.Fatal(err) } - err = wt.tpool.AcceptTransactionSet(txnSet) + err = wt.wallet.managedCommitTransactionSet(txnSet) if err != nil { t.Fatal(err) } @@ -581,7 +581,7 @@ func TestDistantWallets(t *testing.T) { if err != nil { t.Fatal(err) } - err = wt.tpool.AcceptTransactionSet(txnSet) + err = wt.wallet.managedCommitTransactionSet(txnSet) if err != nil { t.Fatal(err) } From 697cb575545551dd0f3e3721745dbdbd0eb8a49d Mon Sep 17 00:00:00 2001 From: Christopher Schinnerl Date: Thu, 11 Jan 2018 09:38:54 -0500 Subject: [PATCH 2/7] move adding sets to unconfirmed set to new wrapper --- build/errors.go | 10 +++++ modules/wallet/consts.go | 6 +++ modules/wallet/database.go | 27 ++++++++++++- modules/wallet/transactionbuilder.go | 6 +-- modules/wallet/update.go | 59 +++++++++++++++++++++++++++- modules/wallet/wallet.go | 40 +++++++++++++------ 6 files changed, 131 insertions(+), 17 deletions(-) diff --git a/build/errors.go b/build/errors.go index 68b50646e0..0ef9c55492 100644 --- a/build/errors.go +++ b/build/errors.go @@ -24,6 +24,16 @@ func ComposeErrors(errs ...error) error { return nil } + // If there is only a single error return it directly. This still allows + // callers of functions to check for specific errors + if len(errStrings) == 1 { + for _, err := range errs { + if err != nil { + return err + } + } + } + // Combine all of the non-nil errors into one larger return value. return errors.New(strings.Join(errStrings, "; ")) } diff --git a/modules/wallet/consts.go b/modules/wallet/consts.go index 4bdd51fb91..0442350456 100644 --- a/modules/wallet/consts.go +++ b/modules/wallet/consts.go @@ -15,6 +15,12 @@ const ( // defragThreshold is the number of outputs a wallet is allowed before it is // defragmented. defragThreshold = 50 + + // respendTimeout records the number of blocks that the wallet will wait + // before spending an output that has been spent in the past. If the + // transaction spending the output has not made it to the transaction pool + // after the limit, the assumption is that it never will. + respendTimeout = 40 ) var ( diff --git a/modules/wallet/database.go b/modules/wallet/database.go index fa884c7d7b..f915fc3502 100644 --- a/modules/wallet/database.go +++ b/modules/wallet/database.go @@ -3,9 +3,11 @@ package wallet import ( "encoding/binary" "errors" + "log" "reflect" "time" + "github.com/NebulousLabs/Sia/build" "github.com/NebulousLabs/Sia/encoding" "github.com/NebulousLabs/Sia/modules" "github.com/NebulousLabs/Sia/types" @@ -41,6 +43,8 @@ var ( // bucketWallet contains various fields needed by the wallet, such as its // UID, EncryptionVerification, and PrimarySeedFile. bucketWallet = []byte("bucketWallet") + // bucketUnconfirmedSets contains the unconfirmedSets field of the wallet + bucketUnconfirmedSets = []byte("bucketUnconfirmedSets") dbBuckets = [][]byte{ bucketProcessedTransactions, @@ -49,6 +53,7 @@ var ( bucketSiacoinOutputs, bucketSiafundOutputs, bucketSpentOutputs, + bucketUnconfirmedSets, bucketWallet, } @@ -211,7 +216,6 @@ func dbGetSpentOutput(tx *bolt.Tx, id types.OutputID) (height types.BlockHeight, func dbDeleteSpentOutput(tx *bolt.Tx, id types.OutputID) error { return dbDelete(tx.Bucket(bucketSpentOutputs), id) } - func dbPutAddrTransactions(tx *bolt.Tx, addr types.UnlockHash, txns []uint64) error { return dbPut(tx.Bucket(bucketAddrTransactions), addr, txns) } @@ -219,6 +223,27 @@ func dbGetAddrTransactions(tx *bolt.Tx, addr types.UnlockHash) (txns []uint64, e err = dbGet(tx.Bucket(bucketAddrTransactions), addr, &txns) return } +func dbPutUnconfirmedSet(tx *bolt.Tx, tSetID modules.TransactionSetID, ids []types.TransactionID) error { + return dbPut(tx.Bucket(bucketUnconfirmedSets), tSetID, ids) +} +func dbDeleteUnconfirmedSet(tx *bolt.Tx, tSetID modules.TransactionSetID) error { + return dbDelete(tx.Bucket(bucketUnconfirmedSets), tSetID) +} +func dbLoadUnconfirmedSets(tx *bolt.Tx) (map[modules.TransactionSetID][]types.TransactionID, error) { + sets := make(map[modules.TransactionSetID][]types.TransactionID) + err := tx.Bucket(bucketUnconfirmedSets).ForEach(func(k []byte, v []byte) error { + var tSetID modules.TransactionSetID + var ids []types.TransactionID + err := build.ComposeErrors(encoding.Unmarshal(k, &tSetID), encoding.Unmarshal(v, &ids)) + if err != nil { + log.Println(err) + return err + } + sets[tSetID] = ids + return nil + }) + return sets, err +} // dbAddAddrTransaction appends a single transaction index to the set of // transactions associated with addr. If the index is already in the set, it is diff --git a/modules/wallet/transactionbuilder.go b/modules/wallet/transactionbuilder.go index c4e0d78cb6..4e3e801661 100644 --- a/modules/wallet/transactionbuilder.go +++ b/modules/wallet/transactionbuilder.go @@ -100,7 +100,7 @@ func (w *Wallet) checkOutput(tx *bolt.Tx, currentHeight types.BlockHeight, id ty // Check that this output has not recently been spent by the wallet. spendHeight, err := dbGetSpentOutput(tx, types.OutputID(id)) if err == nil { - if spendHeight+RespendTimeout > currentHeight { + if spendHeight+respendTimeout > currentHeight { return errSpendHeightTooHigh } } @@ -287,8 +287,8 @@ func (tb *transactionBuilder) FundSiafunds(amount types.Currency) error { spendHeight = 0 } // Prevent an underflow error. - allowedHeight := consensusHeight - RespendTimeout - if consensusHeight < RespendTimeout { + allowedHeight := consensusHeight - respendTimeout + if consensusHeight < respendTimeout { allowedHeight = 0 } if spendHeight > allowedHeight { diff --git a/modules/wallet/update.go b/modules/wallet/update.go index bfa154412f..70b959fd3a 100644 --- a/modules/wallet/update.go +++ b/modules/wallet/update.go @@ -2,8 +2,10 @@ package wallet import ( "fmt" + "log" "math" + "github.com/NebulousLabs/Sia/build" "github.com/NebulousLabs/Sia/modules" "github.com/NebulousLabs/Sia/types" @@ -453,6 +455,26 @@ func (w *Wallet) ProcessConsensusChange(cc modules.ConsensusChange) { } } +// isSuperset is a helper function that checks if super is a superset of sub +func isSuperset(super []types.TransactionID, sub []types.TransactionID) bool { + if len(super) < len(sub) { + log.Println(false) + return false + } + // Create maps from the slices for faster verification + superset := make(map[types.TransactionID]struct{}) + for _, id := range super { + superset[id] = struct{}{} + } + // Check if all ids of sub are in the superset + for _, id := range sub { + if _, exists := superset[id]; !exists { + return false + } + } + return true +} + // ReceiveUpdatedUnconfirmedTransactions updates the wallet's unconfirmed // transaction set. func (w *Wallet) ReceiveUpdatedUnconfirmedTransactions(diff *modules.TransactionPoolDiff) { @@ -473,6 +495,9 @@ func (w *Wallet) ReceiveUpdatedUnconfirmedTransactions(diff *modules.Transaction droppedTransactions[txids[i]] = struct{}{} } delete(w.unconfirmedSets, diff.RevertedTransactions[i]) + if err := dbDeleteUnconfirmedSet(w.dbTx, diff.RevertedTransactions[i]); err != nil { + build.Critical(err) + } } // Skip the reallocation if we can, otherwise reallocate the @@ -503,7 +528,39 @@ func (w *Wallet) ReceiveUpdatedUnconfirmedTransactions(diff *modules.Transaction // // TODO: Technically only necessary to mark the ones that are relevant // to the wallet, but overhead should be low. - w.unconfirmedSets[unconfirmedTxnSet.ID] = unconfirmedTxnSet.IDs + // + // Check if commitTransactionSet already added the transaction + if _, exists := w.unconfirmedSets[unconfirmedTxnSet.ID]; !exists { + // Maybe acceptTransactionSet added a subset with a different + // id. If that's the case, replace it with the one from the + // tpool. + isSubset := false + for tSetID, ids := range w.unconfirmedSets { + if isSuperset(unconfirmedTxnSet.IDs, ids) { + // Add the new id + w.unconfirmedSets[unconfirmedTxnSet.ID] = w.unconfirmedSets[tSetID] + if err := dbPutUnconfirmedSet(w.dbTx, unconfirmedTxnSet.ID, unconfirmedTxnSet.IDs); err != nil { + build.Critical(err) + } + // Remove the old id + delete(w.unconfirmedSets, tSetID) + if err := dbDeleteUnconfirmedSet(w.dbTx, tSetID); err != nil { + build.Critical(err) + } + isSubset = true + break + } + } + // If we couldn't find a matching superset, the transaction + // probably wasn't created by this wallet. In that case we can add + // the transaction. + if !isSubset { + w.unconfirmedSets[unconfirmedTxnSet.ID] = unconfirmedTxnSet.IDs + if err := dbPutUnconfirmedSet(w.dbTx, unconfirmedTxnSet.ID, unconfirmedTxnSet.IDs); err != nil { + build.Critical(err) + } + } + } // Get the values for the spent outputs. spentSiacoinOutputs := make(map[types.SiacoinOutputID]types.SiacoinOutput) diff --git a/modules/wallet/wallet.go b/modules/wallet/wallet.go index 0b037c11ad..5f03a8c3ba 100644 --- a/modules/wallet/wallet.go +++ b/modules/wallet/wallet.go @@ -22,14 +22,6 @@ import ( "github.com/NebulousLabs/Sia/types" ) -const ( - // RespendTimeout records the number of blocks that the wallet will wait - // before spending an output that has been spent in the past. If the - // transaction spending the output has not made it to the transaction pool - // after the limit, the assumption is that it never will. - RespendTimeout = 40 -) - var ( errNilConsensusSet = errors.New("wallet cannot initialize with a nil consensus set") errNilTpool = errors.New("wallet cannot initialize with a nil transaction pool") @@ -123,17 +115,35 @@ func (w *Wallet) Height() types.BlockHeight { return types.BlockHeight(height) } -// acceptTransactionSet is a convenience wrapper for the transaction pools -// commitTransactionSet method. It only returns an error if the transaction was +// commitTransactionSet is a convenience wrapper for the transaction pools +// AcceptTransactionSet method. It only returns an error if the transaction was // rejected and won't be rebroadcasted over time func (w *Wallet) commitTransactionSet(txns []types.Transaction) error { + // Get the transaction ids and add them to the unconfirmedSets + tSetID := modules.TransactionSetID(crypto.HashObject(txns)) + ids := make([]types.TransactionID, 0, len(txns)) + for _, txn := range txns { + ids = append(ids, txn.ID()) + } + w.unconfirmedSets[tSetID] = ids + if err := dbPutUnconfirmedSet(w.dbTx, tSetID, ids); err != nil { + return err + } + + // Accept the set. If the error returned prevents the set from ever being + // accepted we should remove it from the wallet again. w.mu.Unlock() err := w.tpool.AcceptTransactionSet(txns) w.mu.Lock() - return err + if err != nil { // TODO: There are exceptions to this + delete(w.unconfirmedSets, tSetID) + err2 := dbDeleteUnconfirmedSet(w.dbTx, tSetID) + return build.ComposeErrors(err, err2) + } + return nil } -// managedCommitTransactionSet is a thread-safe version of acceptTransactionSet +// managedCommitTransactionSet is a thread-safe version of commitTransactionSet func (w *Wallet) managedCommitTransactionSet(txns []types.Transaction) error { w.mu.Lock() defer w.mu.Unlock() @@ -193,6 +203,12 @@ func newWallet(cs modules.ConsensusSet, tpool modules.TransactionPool, persistDi w.syncDB() } + // Load possible unconfirmed sets from disk + w.unconfirmedSets, err = dbLoadUnconfirmedSets(w.dbTx) + if err != nil { + return nil, err + } + // make sure we commit on shutdown w.tg.AfterStop(func() { err := w.dbTx.Commit() From 72cf19bf2e460d42d88905acd7de453c003a693b Mon Sep 17 00:00:00 2001 From: Christopher Schinnerl Date: Thu, 11 Jan 2018 12:40:50 -0500 Subject: [PATCH 3/7] only consider relevant txn sets for confirmedTransactionSets --- modules/wallet/update.go | 57 ++++++++++++++++++++++++++-------------- 1 file changed, 38 insertions(+), 19 deletions(-) diff --git a/modules/wallet/update.go b/modules/wallet/update.go index 70b959fd3a..cb5ea4153f 100644 --- a/modules/wallet/update.go +++ b/modules/wallet/update.go @@ -455,6 +455,29 @@ func (w *Wallet) ProcessConsensusChange(cc modules.ConsensusChange) { } } +// isRelevantTxn checks if a tranaction is relevant to the wallet +func (w *Wallet) isRelevantTxn(txn types.Transaction) (relevant bool) { + // determine whether transaction is relevant to the wallet + for _, sci := range txn.SiacoinInputs { + relevant = relevant || w.isWalletAddress(sci.UnlockConditions.UnlockHash()) + } + for _, sco := range txn.SiacoinOutputs { + relevant = relevant || w.isWalletAddress(sco.UnlockHash) + } + return +} + +// isRelevantTSet checks if a set of transactions is relevant to the wallet. It +// is relevant if at least one transaction is relevant. +func (w *Wallet) isRelevantTSet(txns []types.Transaction) bool { + for _, txn := range txns { + if w.isRelevantTxn(txn) { + return true + } + } + return false +} + // isSuperset is a helper function that checks if super is a superset of sub func isSuperset(super []types.TransactionID, sub []types.TransactionID) bool { if len(super) < len(sub) { @@ -504,11 +527,13 @@ func (w *Wallet) ReceiveUpdatedUnconfirmedTransactions(diff *modules.Transaction // unconfirmedProcessedTransactions to no longer have the dropped // transactions. if len(droppedTransactions) != 0 { - // Capacity can't be reduced, because we have no way of knowing if the - // dropped transactions are relevant to the wallet or not, and some will - // not be relevant to the wallet, meaning they don't have a counterpart - // in w.unconfirmedProcessedTransactions. - newUPT := make([]modules.ProcessedTransaction, 0, len(w.unconfirmedProcessedTransactions)) + // droppedTransactions should only contain transactions relevant to the + // wallet. Therefore we can safely reduce the allocated memory. + newLen := len(w.unconfirmedProcessedTransactions) - len(droppedTransactions) + if newLen < 0 { + newLen = 0 + } + newUPT := make([]modules.ProcessedTransaction, 0, newLen) for _, txn := range w.unconfirmedProcessedTransactions { _, exists := droppedTransactions[txn.TransactionID] if !exists { @@ -524,12 +549,15 @@ func (w *Wallet) ReceiveUpdatedUnconfirmedTransactions(diff *modules.Transaction // Scroll through all of the diffs and add any new transactions. for _, unconfirmedTxnSet := range diff.AppliedTransactions { + // We only need to do that for transactions relevant to the wallet + if !w.isRelevantTSet(unconfirmedTxnSet.Transactions) { + continue + } + // Mark all of the transactions that appeared in this set. // - // TODO: Technically only necessary to mark the ones that are relevant - // to the wallet, but overhead should be low. - // - // Check if commitTransactionSet already added the transaction + // Check if commitTransactionSet already added the transaction to the + // unconfirmedSets if _, exists := w.unconfirmedSets[unconfirmedTxnSet.ID]; !exists { // Maybe acceptTransactionSet added a subset with a different // id. If that's the case, replace it with the one from the @@ -574,17 +602,8 @@ func (w *Wallet) ReceiveUpdatedUnconfirmedTransactions(diff *modules.Transaction // Add each transaction to our set of unconfirmed transactions. for i, txn := range unconfirmedTxnSet.Transactions { - // determine whether transaction is relevant to the wallet - relevant := false - for _, sci := range txn.SiacoinInputs { - relevant = relevant || w.isWalletAddress(sci.UnlockConditions.UnlockHash()) - } - for _, sco := range txn.SiacoinOutputs { - relevant = relevant || w.isWalletAddress(sco.UnlockHash) - } - // only create a ProcessedTransaction if txn is relevant - if !relevant { + if !w.isRelevantTxn(txn) { continue } From ce33cf0ec131c43584e35cf2f9110f5e304b8356 Mon Sep 17 00:00:00 2001 From: Christopher Schinnerl Date: Fri, 12 Jan 2018 12:21:35 -0500 Subject: [PATCH 4/7] code restructuring --- modules/wallet/update.go | 67 +++++++++++++++++++++------------------- modules/wallet/wallet.go | 42 ++++++++++++++++++------- 2 files changed, 65 insertions(+), 44 deletions(-) diff --git a/modules/wallet/update.go b/modules/wallet/update.go index cb5ea4153f..8923ae3416 100644 --- a/modules/wallet/update.go +++ b/modules/wallet/update.go @@ -554,41 +554,27 @@ func (w *Wallet) ReceiveUpdatedUnconfirmedTransactions(diff *modules.Transaction continue } - // Mark all of the transactions that appeared in this set. - // - // Check if commitTransactionSet already added the transaction to the - // unconfirmedSets - if _, exists := w.unconfirmedSets[unconfirmedTxnSet.ID]; !exists { - // Maybe acceptTransactionSet added a subset with a different - // id. If that's the case, replace it with the one from the - // tpool. - isSubset := false - for tSetID, ids := range w.unconfirmedSets { - if isSuperset(unconfirmedTxnSet.IDs, ids) { - // Add the new id - w.unconfirmedSets[unconfirmedTxnSet.ID] = w.unconfirmedSets[tSetID] - if err := dbPutUnconfirmedSet(w.dbTx, unconfirmedTxnSet.ID, unconfirmedTxnSet.IDs); err != nil { - build.Critical(err) - } - // Remove the old id - delete(w.unconfirmedSets, tSetID) - if err := dbDeleteUnconfirmedSet(w.dbTx, tSetID); err != nil { - build.Critical(err) - } - isSubset = true - break - } - } - // If we couldn't find a matching superset, the transaction - // probably wasn't created by this wallet. In that case we can add - // the transaction. - if !isSubset { - w.unconfirmedSets[unconfirmedTxnSet.ID] = unconfirmedTxnSet.IDs - if err := dbPutUnconfirmedSet(w.dbTx, unconfirmedTxnSet.ID, unconfirmedTxnSet.IDs); err != nil { + // Check if unconfirmedSets already contains a subset of the + // unconfirmedTxnSet's ids. This might happen if the wallet manually + // added the set to unconfirmedSets after AcceptTransactionSet failed + // in commitTransactionSet. If it contains a subset it should be + // deleted and be replaced by the superset. + for tSetID, ids := range w.unconfirmedSets { + if isSuperset(unconfirmedTxnSet.IDs, ids) { + // Remove the old id + delete(w.unconfirmedSets, tSetID) + if err := dbDeleteUnconfirmedSet(w.dbTx, tSetID); err != nil { build.Critical(err) } + break } } + // Add the set to the unconfirmedSets + w.unconfirmedSets[unconfirmedTxnSet.ID] = unconfirmedTxnSet.IDs + err := dbPutUnconfirmedSet(w.dbTx, unconfirmedTxnSet.ID, unconfirmedTxnSet.IDs) + if err != nil { + build.Critical(err) + } // Get the values for the spent outputs. spentSiacoinOutputs := make(map[types.SiacoinOutputID]types.SiacoinOutput) @@ -600,6 +586,17 @@ func (w *Wallet) ReceiveUpdatedUnconfirmedTransactions(diff *modules.Transaction } } + // Build an index that maps a transaction id to it's index in + // unconfirmedProcessedTransactions. This allows us to find duplicates + // in unconfirmedProcessedTransactions. There is a chance that after a + // failed AcceptTransactionSet in commitTransactionSet the wallet + // already added a particular transaction. In that case we want to + // replace it instead of appending. + ptIndices := make(map[types.TransactionID]int) + for i, pt := range w.unconfirmedProcessedTransactions { + ptIndices[pt.TransactionID] = i + } + // Add each transaction to our set of unconfirmed transactions. for i, txn := range unconfirmedTxnSet.Transactions { // only create a ProcessedTransaction if txn is relevant @@ -638,7 +635,13 @@ func (w *Wallet) ReceiveUpdatedUnconfirmedTransactions(diff *modules.Transaction Value: fee, }) } - w.unconfirmedProcessedTransactions = append(w.unconfirmedProcessedTransactions, pt) + // Check if a transaction with that id already consists. If it does + // we replace it. Otherwise we append + if _, exists := ptIndices[pt.TransactionID]; exists { + w.unconfirmedProcessedTransactions[i] = pt + } else { + w.unconfirmedProcessedTransactions = append(w.unconfirmedProcessedTransactions, pt) + } } } } diff --git a/modules/wallet/wallet.go b/modules/wallet/wallet.go index 5f03a8c3ba..dcfdfd88cd 100644 --- a/modules/wallet/wallet.go +++ b/modules/wallet/wallet.go @@ -8,6 +8,7 @@ import ( "errors" "fmt" "log" + "math" "sort" "sync" @@ -119,27 +120,44 @@ func (w *Wallet) Height() types.BlockHeight { // AcceptTransactionSet method. It only returns an error if the transaction was // rejected and won't be rebroadcasted over time func (w *Wallet) commitTransactionSet(txns []types.Transaction) error { - // Get the transaction ids and add them to the unconfirmedSets + w.mu.Unlock() + err := w.tpool.AcceptTransactionSet(txns) + w.mu.Lock() + if err == nil { + // If we were able to add the transactions to the pool we are done. The + // wallet already updated the unconfirmedSets and + // unconfirmedProcessedTransactions fields in + // ReceiveUpdatedUnconfirmedTransactions + return nil + } + if err != nil { + // TODO: There might be some errors that make us abort here + } + + // If we couldn't add the transaction but still want the wallet to track it + // we need to add it manually to the unconfirmedSets and + // unconfirmedProcessedTransactions tSetID := modules.TransactionSetID(crypto.HashObject(txns)) ids := make([]types.TransactionID, 0, len(txns)) + pts := make([]modules.ProcessedTransaction, 0, len(txns)) for _, txn := range txns { ids = append(ids, txn.ID()) + pt := modules.ProcessedTransaction{ + Transaction: txn, + TransactionID: txn.ID(), + ConfirmationHeight: types.BlockHeight(math.MaxUint64), + ConfirmationTimestamp: types.Timestamp(math.MaxUint64), + } + // TODO Also add processed inputs and outputs + pts = append(pts, pt) } + // Add the unconfirmed set w.unconfirmedSets[tSetID] = ids if err := dbPutUnconfirmedSet(w.dbTx, tSetID, ids); err != nil { return err } - - // Accept the set. If the error returned prevents the set from ever being - // accepted we should remove it from the wallet again. - w.mu.Unlock() - err := w.tpool.AcceptTransactionSet(txns) - w.mu.Lock() - if err != nil { // TODO: There are exceptions to this - delete(w.unconfirmedSets, tSetID) - err2 := dbDeleteUnconfirmedSet(w.dbTx, tSetID) - return build.ComposeErrors(err, err2) - } + // Add the unconfirmed processed transactions + w.unconfirmedProcessedTransactions = append(w.unconfirmedProcessedTransactions, pts...) return nil } From 81860071c0e2b9dfb0074b55bc615615bb3c0651 Mon Sep 17 00:00:00 2001 From: Christopher Schinnerl Date: Fri, 12 Jan 2018 13:02:12 -0500 Subject: [PATCH 5/7] abort commitTransactionSet if we encounter consensus conflict or ErrDuplicatTransactionSet --- modules/wallet/wallet.go | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/modules/wallet/wallet.go b/modules/wallet/wallet.go index dcfdfd88cd..1d361dfb08 100644 --- a/modules/wallet/wallet.go +++ b/modules/wallet/wallet.go @@ -130,9 +130,15 @@ func (w *Wallet) commitTransactionSet(txns []types.Transaction) error { // ReceiveUpdatedUnconfirmedTransactions return nil } - if err != nil { - // TODO: There might be some errors that make us abort here + // If there was a consensus conflict we shouldn't add the set + if cconflict, ok := err.(modules.ConsensusConflict); ok { + return cconflict + } + // If the set was already added we don't need to add it again + if err == modules.ErrDuplicateTransactionSet { + return err } + // TODO: There might be more errors that make us abort here // If we couldn't add the transaction but still want the wallet to track it // we need to add it manually to the unconfirmedSets and From 28a5e45fb7c4e45dbfc39702bfe8a9867c135e96 Mon Sep 17 00:00:00 2001 From: Christopher Schinnerl Date: Mon, 15 Jan 2018 14:13:57 -0500 Subject: [PATCH 6/7] added test --- modules/transactionpool/consts.go | 18 +++++--- modules/wallet/update.go | 2 +- modules/wallet/wallet_test.go | 70 +++++++++++++++++++++++++++++++ 3 files changed, 83 insertions(+), 7 deletions(-) diff --git a/modules/transactionpool/consts.go b/modules/transactionpool/consts.go index 4c36f0143d..66b992f23f 100644 --- a/modules/transactionpool/consts.go +++ b/modules/transactionpool/consts.go @@ -28,12 +28,6 @@ const ( // default size during times of congestion. TransactionPoolExponentiation = 3 - // TransactionPoolSizeForFee defines how large the transaction pool needs to - // be before it starts expecting fees to be on the transaction. This initial - // limit is to help the network grow and provide some wiggle room for - // wallets that are not yet able to operate via a fee market. - TransactionPoolSizeForFee = 500e3 - // TransactionPoolSizeTarget defines the target size of the pool when the // transactions are paying 1 SC / kb in fees. TransactionPoolSizeTarget = 3e6 @@ -71,6 +65,18 @@ var ( minEstimation = types.SiacoinPrecision.Div64(100).Div64(1e3) ) +var ( + // TransactionPoolSizeForFee defines how large the transaction pool needs to + // be before it starts expecting fees to be on the transaction. This initial + // limit is to help the network grow and provide some wiggle room for + // wallets that are not yet able to operate via a fee market. + TransactionPoolSizeForFee = build.Select(build.Var{ + Standard: int(500e3), + Dev: int(500e3), + Testing: int(30e3), + }).(int) +) + // Variables related to propagating transactions through the network. var ( // relayTransactionSetTimeout establishes the timeout for a relay diff --git a/modules/wallet/update.go b/modules/wallet/update.go index 8923ae3416..748d8febb7 100644 --- a/modules/wallet/update.go +++ b/modules/wallet/update.go @@ -560,7 +560,7 @@ func (w *Wallet) ReceiveUpdatedUnconfirmedTransactions(diff *modules.Transaction // in commitTransactionSet. If it contains a subset it should be // deleted and be replaced by the superset. for tSetID, ids := range w.unconfirmedSets { - if isSuperset(unconfirmedTxnSet.IDs, ids) { + if isSuperset(unconfirmedTxnSet.IDs, ids) && len(unconfirmedTxnSet.IDs) != len(ids) { // Remove the old id delete(w.unconfirmedSets, tSetID) if err := dbDeleteUnconfirmedSet(w.dbTx, tSetID); err != nil { diff --git a/modules/wallet/wallet_test.go b/modules/wallet/wallet_test.go index 23490124ae..141ea8481b 100644 --- a/modules/wallet/wallet_test.go +++ b/modules/wallet/wallet_test.go @@ -591,3 +591,73 @@ func TestDistantWallets(t *testing.T) { t.Fatal("wallet should not recognize coins sent to very high seed index") } } + +// TestCommitTransactionSetInsufficientFees checks if a transaction still makes +// it into unconfirmedSets and unconfirmedProcessedTransactions even though the +// fees are not sufficient. +func TestCommitTransactionSetInsufficientFees(t *testing.T) { + if testing.Short() { + t.SkipNow() + } + wt, err := createWalletTester(t.Name(), &ProductionDependencies{}) + if err != nil { + t.Fatal(err) + } + defer wt.closeWt() + + // Get an address from the wallet + uc, err := wt.wallet.NextAddress() + if err != nil { + t.Fatal(err) + } + + // Send a bunch of transactions without fees. The transaction pool should + // expect an increasing number of fees once we hit the + // TransactionPoolSizeForFee limit but we will not add any to the + // transactions. + amount := types.SiacoinPrecision.Mul64(100) + poolSize := 0 + numTxnsSent := 0 + for poolSize < 2*transactionpool.TransactionPoolSizeForFee { + builder := wt.wallet.StartTransaction() + builder.AddSiacoinOutput(types.SiacoinOutput{ + UnlockHash: uc.UnlockHash(), + Value: amount, + }) + // Don't add any fees + err = builder.FundSiacoins(amount) + if err != nil { + t.Fatal(err) + } + txnSet, err := builder.Sign(true) + if err != nil { + t.Fatal(err) + } + + // Committing the transaction should work + err = wt.wallet.managedCommitTransactionSet(txnSet) + if err != nil { + t.Fatal(err) + } + + // Increase poolSize + for _, txn := range txnSet { + poolSize += txn.MarshalSiaSize() + } + numTxnsSent++ + } + // Each sent transaction creates a set of 2 transactions. There hsould be + // the same number of unconfirmedProcessedTransactions + if len(wt.wallet.unconfirmedProcessedTransactions) != 2*numTxnsSent { + t.Errorf("There should be 2 txns for each sent set.") + } + // For each unfirmed processed transaction there should be a transaction id + // in the unconfirmedSets + unconfirmedSetsTxns := 0 + for _, set := range wt.wallet.unconfirmedSets { + unconfirmedSetsTxns += len(set) + } + if len(wt.wallet.unconfirmedProcessedTransactions) != unconfirmedSetsTxns { + t.Errorf("There should be as many unconfirmed processed transactions as there are txns in the unconfirmed sets") + } +} From 0ec25bf62823b54e5dc5a8a26ad12fdb2d359aad Mon Sep 17 00:00:00 2001 From: Christopher Schinnerl Date: Mon, 15 Jan 2018 17:21:57 -0500 Subject: [PATCH 7/7] extend testcase. This will cause it to fail until we have rebroadcasting --- modules/wallet/wallet_test.go | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/modules/wallet/wallet_test.go b/modules/wallet/wallet_test.go index 141ea8481b..4cba11b50c 100644 --- a/modules/wallet/wallet_test.go +++ b/modules/wallet/wallet_test.go @@ -1,6 +1,7 @@ package wallet import ( + "errors" "path/filepath" "testing" "time" @@ -660,4 +661,19 @@ func TestCommitTransactionSetInsufficientFees(t *testing.T) { if len(wt.wallet.unconfirmedProcessedTransactions) != unconfirmedSetsTxns { t.Errorf("There should be as many unconfirmed processed transactions as there are txns in the unconfirmed sets") } + err = build.Retry(10, time.Millisecond*250, func() error { + if _, err := wt.miner.AddBlock(); err != nil { + return err + } + // TODO This won't work for now since it needs the Rebroadcast PR. Once + // we have that, calling AcceptTransactionSet again will cause the + // number of unconfirmed transactions to go down to 0. + if len(wt.wallet.unconfirmedSets) > 0 || len(wt.wallet.unconfirmedProcessedTransactions) > 0 { + return errors.New("there is still unconfirmed transactions and sets remaining after mining a couple of new blocks") + } + return nil + }) + if err != nil { + t.Error(err) + } }