From 29c5100b493a5e6fa0ae622dced39d11a5e1c599 Mon Sep 17 00:00:00 2001 From: Christopher Schinnerl Date: Fri, 5 Jan 2018 08:47:56 -0500 Subject: [PATCH] move broadcast code to new file and use RespendTimeout instead of a tries counter --- modules/consts.go | 10 +++ modules/transactionpool/consts.go | 4 -- modules/transactionpool/update.go | 2 +- modules/transactionpool/update_test.go | 6 +- modules/wallet/broadcast.go | 31 +++++++++ modules/wallet/broadcast_test.go | 89 ++++++++++++++++++++++++++ modules/wallet/consts.go | 20 ++++-- modules/wallet/transactionbuilder.go | 6 +- modules/wallet/update.go | 13 ++-- modules/wallet/wallet.go | 34 ---------- modules/wallet/wallet_test.go | 83 ------------------------ 11 files changed, 159 insertions(+), 139 deletions(-) create mode 100644 modules/consts.go create mode 100644 modules/wallet/broadcast.go create mode 100644 modules/wallet/broadcast_test.go diff --git a/modules/consts.go b/modules/consts.go new file mode 100644 index 0000000000..9f5a66a892 --- /dev/null +++ b/modules/consts.go @@ -0,0 +1,10 @@ +package modules + +import "github.com/NebulousLabs/Sia/types" + +// Consts that are required by multiple modules +const ( + // maxTxnAge determines the maximum age of a transaction (in block height) + // allowed before the transaction is pruned from the transaction pool. + MaxTxnAge = types.BlockHeight(24) +) diff --git a/modules/transactionpool/consts.go b/modules/transactionpool/consts.go index 4c36f0143d..a0441d1ae7 100644 --- a/modules/transactionpool/consts.go +++ b/modules/transactionpool/consts.go @@ -16,10 +16,6 @@ const ( // Constants related to the size and ease-of-entry of the transaction pool. const ( - // maxTxnAge determines the maximum age of a transaction (in block height) - // allowed before the transaction is pruned from the transaction pool. - maxTxnAge = types.BlockHeight(24) - // TransactionPoolFeeExponentiation defines the polynomial rate of growth // required to keep putting transactions into the transaction pool. If the // exponentiation is 2, then doubling the size of the transaction pool diff --git a/modules/transactionpool/update.go b/modules/transactionpool/update.go index 7792c769d8..e3e5b84de1 100644 --- a/modules/transactionpool/update.go +++ b/modules/transactionpool/update.go @@ -324,7 +324,7 @@ func (tp *TransactionPool) ProcessConsensusChange(cc modules.ConsensusChange) { var validTxns []types.Transaction for _, txn := range tSet { seenHeight, seen := tp.transactionHeights[txn.ID()] - if tp.blockHeight-seenHeight <= maxTxnAge || !seen { + if tp.blockHeight-seenHeight <= modules.MaxTxnAge || !seen { validTxns = append(validTxns, txn) } else { delete(tp.transactionHeights, txn.ID()) diff --git a/modules/transactionpool/update_test.go b/modules/transactionpool/update_test.go index e09b2575e0..3f483c7d45 100644 --- a/modules/transactionpool/update_test.go +++ b/modules/transactionpool/update_test.go @@ -265,7 +265,7 @@ func TestValidRevertedTransaction(t *testing.T) { } // TestTransactionPoolPruning verifies that the transaction pool correctly -// prunes transactions older than maxTxnAge. +// prunes transactions older than MaxTxnAge. func TestTransactionPoolPruning(t *testing.T) { if testing.Short() { t.SkipNow() @@ -298,7 +298,7 @@ func TestTransactionPoolPruning(t *testing.T) { t.Fatal("testers did not have the same block height after one minute") } - // disconnect tpt, create an unconfirmed transaction on tpt, mine maxTxnAge + // disconnect tpt, create an unconfirmed transaction on tpt, mine MaxTxnAge // blocks on tpt2 and reconnect. The unconfirmed transactions should be // removed from tpt's pool. err = tpt.gateway.Disconnect(tpt2.gateway.Address()) @@ -310,7 +310,7 @@ func TestTransactionPoolPruning(t *testing.T) { if err != nil { t.Fatal(err) } - for i := types.BlockHeight(0); i < maxTxnAge+1; i++ { + for i := types.BlockHeight(0); i < modules.MaxTxnAge+1; i++ { _, err = tpt2.miner.AddBlock() if err != nil { t.Fatal(err) diff --git a/modules/wallet/broadcast.go b/modules/wallet/broadcast.go new file mode 100644 index 0000000000..26a7189fe9 --- /dev/null +++ b/modules/wallet/broadcast.go @@ -0,0 +1,31 @@ +package wallet + +import "github.com/NebulousLabs/Sia/types" + +// broadcastedTSet is a helper struct to keep track of transaction sets and to +// help rebroadcast them. +type broadcastedTSet struct { + firstTry types.BlockHeight // first time the tSet was broadcasted + lastTry types.BlockHeight // last time the tSet was broadcasted + confirmedTxn map[types.TransactionID]bool // tracks confirmed txns of set + transactions []types.Transaction // the tSet +} + +// newBroadcastedTSet creates a broadcastedTSet from a normal tSet +func (w *Wallet) newBroadcastedTSet(tSet []types.Transaction) (bts *broadcastedTSet, err error) { + bts = &broadcastedTSet{} + // Set the height of the first and last try + bts.firstTry, err = dbGetConsensusHeight(w.dbTx) + if err != nil { + return + } + bts.lastTry = bts.firstTry + + // Initialize confirmedTxn and transactions + bts.confirmedTxn = make(map[types.TransactionID]bool) + for _, txn := range tSet { + bts.confirmedTxn[txn.ID()] = false + bts.transactions = append(bts.transactions, txn) + } + return +} diff --git a/modules/wallet/broadcast_test.go b/modules/wallet/broadcast_test.go new file mode 100644 index 0000000000..c8f2d8532c --- /dev/null +++ b/modules/wallet/broadcast_test.go @@ -0,0 +1,89 @@ +package wallet + +import ( + "testing" + + "github.com/NebulousLabs/Sia/crypto" + "github.com/NebulousLabs/Sia/modules" + "github.com/NebulousLabs/Sia/types" +) + +// TestRebroadcastTransactions checks if transactions are correctly +// rebroadcasted after some time if they haven't been confirmed +func TestRebroadcastTransactions(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 to send money to + uc, err := wt.wallet.NextAddress() + if err != nil { + t.Fatal(err) + } + // Send money to the address + _, err = wt.wallet.SendSiacoins(types.SiacoinPrecision, uc.UnlockHash()) + if err != nil { + t.Fatal(err) + } + // The wallet should track the new tSet + if len(wt.wallet.broadcastedTSets) != 1 { + t.Fatalf("len(broadcastedTSets) should be %v but was %v", + 1, len(wt.wallet.broadcastedTSets)) + } + // Mine enough blocks for the wallet to stop tracking the tSet + for i := 0; i < rebroadcastInterval+1; i++ { + if _, err := wt.miner.AddBlock(); err != nil { + t.Fatal(err) + } + } + if len(wt.wallet.broadcastedTSets) > 0 { + t.Fatalf("len(broadcastedTSets) should be 0 but was %v", + len(wt.wallet.broadcastedTSets)) + } + + // Send some more money to the address + tSet, err := wt.wallet.SendSiacoins(types.SiacoinPrecision, uc.UnlockHash()) + if err != nil { + t.Fatal(err) + } + // The wallet should track the new tSet + if len(wt.wallet.broadcastedTSets) != 1 { + t.Fatalf("len(broadcastedTSets) should be %v but was %v", + 1, len(wt.wallet.broadcastedTSets)) + } + // Mine a block to get the tSet confirmed + if _, err := wt.miner.AddBlock(); err != nil { + t.Fatal(err) + } + // Corrupt the new tSet to make sure the wallet believes it is not confirmed + tSetID := modules.TransactionSetID(crypto.HashAll(tSet)) + bts := wt.wallet.broadcastedTSets[tSetID] + for tid := range bts.confirmedTxn { + bts.confirmedTxn[tid] = false + } + // Mine the same number of blocks. This time the wallet should still track + // the tSet afterwards. + for i := 0; i < rebroadcastInterval+1; i++ { + if _, err := wt.miner.AddBlock(); err != nil { + t.Fatal(err) + } + } + if len(wt.wallet.broadcastedTSets) != 1 { + t.Fatalf("The wallet should still track the tSet") + } + // Continue mining to make sure that the wallet stops tracking the tSet + // once the max number of retries is reached + for i := types.BlockHeight(0); i < rebroadcastTimeout; i++ { + if _, err := wt.miner.AddBlock(); err != nil { + t.Fatal(err) + } + } + if _, exists := wt.wallet.broadcastedTSets[tSetID]; exists { + t.Fatalf("Wallet should drop txnSet after %v blocks", rebroadcastTimeout) + } +} diff --git a/modules/wallet/consts.go b/modules/wallet/consts.go index 6e92a73d0c..8cc17b7435 100644 --- a/modules/wallet/consts.go +++ b/modules/wallet/consts.go @@ -2,6 +2,8 @@ package wallet import ( "github.com/NebulousLabs/Sia/build" + "github.com/NebulousLabs/Sia/modules" + "github.com/NebulousLabs/Sia/types" ) const ( @@ -19,14 +21,24 @@ const ( // rebroadcastInterval is the number of blocks the wallet will wait until // it rebroadcasts an unconfirmed transaction by adding it to the // transaction pool again. - rebroadcastInterval = 10 + rebroadcastInterval = 6 - // rebroadcastMaxTries is the maximum number of times a transaction set - // will be rebroadcasted before the wallet stops tracking it - rebroadcastMaxTries = 10 + // 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 = 72 ) var ( + // rebroadcastTimeout is the amount of blocks after which we stop trying to + // rebroadcast transactions. The reason why we can't just use + // respendTimeout as the rebroadcastTimeout is, that the transaction pool + // will boot transactions after MaxTxnAge. We need to make sure that we + // leave at least MaxTxnAge blocks after the last broadcast to allow for + // the transasction to be pruned before the wallet tries to respend it. + rebroadcastTimeout = types.BlockHeight(respendTimeout - modules.MaxTxnAge) + // lookaheadBuffer together with lookaheadRescanThreshold defines the constant part // of the maxLookahead lookaheadBuffer = build.Select(build.Var{ 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 fa03a9f250..a046a5a68f 100644 --- a/modules/wallet/update.go +++ b/modules/wallet/update.go @@ -510,7 +510,7 @@ func (w *Wallet) rebroadcastOldTransactions(tx *bolt.Tx, cc modules.ConsensusCha } // If the transaction set has been confirmed for one broadcast cycle it // should be safe to remove it - if confirmed && consensusHeight > bts.height+rebroadcastInterval { + if confirmed && consensusHeight > bts.lastTry+rebroadcastInterval { delete(w.broadcastedTSets, tSetID) continue } @@ -521,17 +521,16 @@ func (w *Wallet) rebroadcastOldTransactions(tx *bolt.Tx, cc modules.ConsensusCha } // If the transaction set is not confirmed and hasn't been broadcasted // for rebroadcastInterval blocks we try to broadcast it again - if consensusHeight > bts.height+rebroadcastInterval { - bts.height = consensusHeight - bts.tries++ + if consensusHeight > bts.lastTry+rebroadcastInterval { + bts.lastTry = consensusHeight go func() { - w.mu.Lock() - defer w.mu.Unlock() if err := w.tpool.AcceptTransactionSet(bts.transactions); err != nil { w.log.Println("WARNING: Rebroadcast failed: ", err) } }() - if bts.tries >= rebroadcastMaxTries { + // Delete the transaction set once we have tried for RespendTimeout + // blocks + if consensusHeight >= bts.firstTry+rebroadcastTimeout { delete(w.broadcastedTSets, tSetID) } } diff --git a/modules/wallet/wallet.go b/modules/wallet/wallet.go index c4cc471ce3..c2a4034c66 100644 --- a/modules/wallet/wallet.go +++ b/modules/wallet/wallet.go @@ -20,14 +20,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") @@ -43,32 +35,6 @@ type spendableKey struct { SecretKeys []crypto.SecretKey } -// broadcastedTSet is a helper struct to keep track of transaction sets and to -// help rebroadcast them. -type broadcastedTSet struct { - height types.BlockHeight - tries int - confirmedTxn map[types.TransactionID]bool - transactions []types.Transaction -} - -// newBroadcastedTSet -func (w *Wallet) newBroadcastedTSet(tSet []types.Transaction) (bts *broadcastedTSet, err error) { - bts = &broadcastedTSet{} - // Set the height - bts.height, err = dbGetConsensusHeight(w.dbTx) - if err != nil { - return - } - // Initialize confirmedTxn and transactions - bts.confirmedTxn = make(map[types.TransactionID]bool) - for _, txn := range tSet { - bts.confirmedTxn[txn.ID()] = false - bts.transactions = append(bts.transactions, txn) - } - return -} - // Wallet is an object that tracks balances, creates keys and addresses, // manages building and sending transactions. type Wallet struct { diff --git a/modules/wallet/wallet_test.go b/modules/wallet/wallet_test.go index 4736d99286..08672ab641 100644 --- a/modules/wallet/wallet_test.go +++ b/modules/wallet/wallet_test.go @@ -591,86 +591,3 @@ func TestDistantWallets(t *testing.T) { t.Fatal("wallet should not recognize coins sent to very high seed index") } } - -// TestRebroadcastTransactions checks if transactions are correctly -// rebroadcasted after some time if they haven't been confirmed -func TestRebroadcastTransactions(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 to send money to - uc, err := wt.wallet.NextAddress() - if err != nil { - t.Fatal(err) - } - // Send money to the address - _, err = wt.wallet.SendSiacoins(types.SiacoinPrecision, uc.UnlockHash()) - if err != nil { - t.Fatal(err) - } - // The wallet should track the new tSet - if len(wt.wallet.broadcastedTSets) != 1 { - t.Fatalf("len(broadcastedTSets) should be %v but was %v", - 1, len(wt.wallet.broadcastedTSets)) - } - // Mine enough blocks for the wallet to stop tracking the tSet - for i := 0; i < rebroadcastInterval+1; i++ { - if _, err := wt.miner.AddBlock(); err != nil { - t.Fatal(err) - } - } - if len(wt.wallet.broadcastedTSets) > 0 { - t.Fatalf("len(broadcastedTSets) should be 0 but was %v", - len(wt.wallet.broadcastedTSets)) - } - - // Send some more money to the address - tSet, err := wt.wallet.SendSiacoins(types.SiacoinPrecision, uc.UnlockHash()) - if err != nil { - t.Fatal(err) - } - // The wallet should track the new tSet - if len(wt.wallet.broadcastedTSets) != 1 { - t.Fatalf("len(broadcastedTSets) should be %v but was %v", - 1, len(wt.wallet.broadcastedTSets)) - } - // Mine a block to get the tSet confirmed - if _, err := wt.miner.AddBlock(); err != nil { - t.Fatal(err) - } - // Corrupt the new tSet to make sure the wallet believes it is not confirmed - tSetID := modules.TransactionSetID(crypto.HashAll(tSet)) - bts := wt.wallet.broadcastedTSets[tSetID] - for tid := range bts.confirmedTxn { - bts.confirmedTxn[tid] = false - } - // Mine the same number of blocks. This time the wallet should still track - // the tSet afterwards. - for i := 0; i < rebroadcastInterval+1; i++ { - if _, err := wt.miner.AddBlock(); err != nil { - t.Fatal(err) - } - } - if len(wt.wallet.broadcastedTSets) != 1 { - t.Fatalf("The wallet should still track the tSet") - } - if bts.tries != 1 { - t.Fatalf("The transaction set's tries counter should be 1") - } - // Continue mining to make sure that the wallet stops tracking the tSet - // once the max number of retries is reached - for i := 0; i < rebroadcastInterval*rebroadcastMaxTries; i++ { - if _, err := wt.miner.AddBlock(); err != nil { - t.Fatal(err) - } - } - if _, exists := wt.wallet.broadcastedTSets[tSetID]; exists { - t.Fatalf("Wallet should drop txnSet after %v tries", rebroadcastMaxTries) - } -}