Make wallet rebroadcast transactions until they are confirmed by ChrisSchinnerl · Pull Request #2611 · NebulousLabs/Sia

ChrisSchinnerl · 2018-01-04T23:14:20Z

This is a work in progress but I would appreciate some code review before I continue this.

DavidVorick · 2018-01-05T07:27:47Z

+
+	// rebroadcastMaxTries is the maximum number of times a transaction set
+	// will be rebroadcasted before the wallet stops tracking it
+	rebroadcastMaxTries = 10


There's a const called RespendTimeout in wallet.go (that should be moved to consts.go) which says the wallet will try to spend an output again after 40 blocks of it not being spent. Instead of having rebroadcastMaxTries, you should just stop trying once the respend timeout has expired.

Now that we are trying to rebroadcast transactions, we can probably jump the RespendTimeout to 72 blocks, which is 12 hours.

DavidVorick · 2018-01-05T07:42:29Z

Part of the purpose of this PR is to make sure transactions get re-broadcast. Right now, I'm pretty sure that if you call tpool.AcceptTransactionSet with a transaction set that you've already added to the tpool, you will get an error, and it won't broadcast.

So we should modify the AcceptTransactionSet to go ahead and broadcast the transaction and reset the timer on the transactions (they get booted after 12 blocks) if the broadcast function is called again.

I guess it's a little complicated though, because the wallet is going to start respending the transactions at some point, and we need to make sure that by that point the previous transaction in the transaction pool has been properly booted. I guess we'll need to leverage some consts in the modules package.

DavidVorick · 2018-01-05T07:44:24Z

+	for _, sco := range txn.SiacoinOutputs {
+		relevant = relevant || w.isWalletAddress(sco.UnlockHash)
+	}
+	return


Just throwing it out there to put it on the radar, it's not important for this PR, but the wallet is going to need to start recognizing file contracts as well as siacoin + siafund transactions.

DavidVorick · 2018-01-05T07:52:21Z

+	tries        int
+	confirmedTxn map[types.TransactionID]bool
+	transactions []types.Transaction
+}


should probably move this code into broadcast.go or something. I think there's enough logic here that it deserves its own file.

DavidVorick · 2018-01-05T07:54:13Z

I think generally speaking this code is on the right track. The one thing that seems to be missing is persistence - if you create a transaction, you should persist it in the database, so that if you start the wallet again later, it can be re-broadcast again. Have to be careful about that though, because after RespendTimeout the wallet will start using those outputs to make other transactions again, and we want to make sure we aren't broadcasting transactions which the wallet will try to spend again.

4Dolio · 2018-01-05T16:12:46Z

Great issue! Just throwing it out there, wr could use a traditional 'progressive' retry? Begin to retry in a short block height and get slower. Not sure is a very short retry would induce problems or cause a feedback overload to the blockchain/network itself? So pick a reasonably fast yet safe start duration. Say 5, then 10, 20, 40, 80, 160, untill acceptable max like 1 month? Week?

DavidVorick · 2018-01-07T03:59:37Z

+	// 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)


excellent comments

DavidVorick · 2018-01-07T18:08:49Z

Well, it's bad for the network to broadcast frequently. And, if you haven't broadcasted in 24 blocks, your transactions will have been dropped from any nodes running standard code, and will be unlikely to reach miners. So at the very least, you'd want to try once every 24 blocks I'd think.

But, once every 6 blocks seems pretty sound / okay as well, at least for now. If transaction pools are full or the fees are too high, it'll likely change after 6 blocks.

jarcko · 2018-01-11T17:58:32Z

Tranascations not found here
https://explore.sia.tech/hashes/50a860e7e4ce6976710d1d52b580bb83ca5e125dbbfd5e34722811a365676a05

I see it every time so can't send coins at all, I reinstalled all from scratch and recovered the wallet, but still the same. Can you reproduce the issue?

DavidVorick · 2018-01-16T14:56:20Z

Are you running a host? Did you just make that transaction? How long did you wait for the coins to send?

DavidVorick · 2018-01-16T15:13:36Z

+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)


I believe that there are other consts throughout the modules package, we may want to collect them all here

Should that be done in the same PR?

DavidVorick · 2018-01-16T15:14:27Z

+					tp.transactionHeights[txn.ID()] = tp.blockHeight
+				}
+			}
+			go tp.gateway.Broadcast("RelayTransactionSet", ts, tp.gateway.Peers())


calling tp.gateway.Peers() while the tpool is under lock is a deadlock risk, need to restructure to avoid this (looks like the existing code had this deadlock risk in it as well)

The general convention is that a module can't call exported methods of other modules while under lock

DavidVorick · 2018-01-16T15:18:43Z

+
+// confirmed is a helper function that sets a certain transactions to confirmed
+// or unconfirmed. It also updates the state on disk.
+func (bts *broadcastedTSet) confirmed(txid types.TransactionID, confirmed bool) error {


this function name suggests a query, as though you are asking for the confirmed status of a transaction. Should change the name to 'markConfirmation' or something similar.

DavidVorick · 2018-01-16T15:24:05Z

+
+// rebroadcastOldTransaction rebroadcasts transactions that haven't been
+// confirmed within rebroadcastInterval blocks
+func (w *Wallet) rebroadcastOldTransactions(tx *bolt.Tx, cc modules.ConsensusChange) error {


This function should probably be moved to broadcast.go

DavidVorick · 2018-01-16T15:29:10Z

+
+	// Mark reverted transactions as not confirmed
+	for _, block := range cc.RevertedBlocks {
+		for _, bts := range w.broadcastedTSets {


This is an n^2 algorithm. The thing that worries me is that for each broadcastedTSet, you are iterating through every transaction of every block. If a wallet has worked up something like 100 broadcastedTSets over the past few blocks (not unreasonable for an exchange with lots of inbound attention), and there are 2000 transactions per block, we could be seeing 200,000 operations happening to get through each block. Combine that with a large (say 5 blocks) reorg and you could be seeing 4 reversions + 5 additions = 1.8 million total operations.

Need to find a faster way to do this.

DavidVorick · 2018-01-16T15:29:32Z

+				if _, exists := bts.confirmedTxn[txn.ID()]; exists {
+					err = bts.confirmed(txn.ID(), false)
+				}
+				if err != nil {


error handling needs to happen in the same scope as error creation

DavidVorick · 2018-01-16T15:30:28Z

 		tp.mu.Lock()
 		defer tp.mu.Unlock()
 		err := tp.acceptTransactionSet(ts, txnFn)
 		if err != nil {


if err == nil, right now you are only updating subscribers if there is an error :P

DavidVorick · 2018-01-16T15:34:28Z

+			for _, txn := range ts {
+				tp.transactionHeights[txn.ID()] = tp.blockHeight
+			}
+			tp.mu.Unlock()


ah I missed this. This should happen under the same lock that you update the subscribers with.

DavidVorick · 2018-01-16T15:38:58Z

+			// blocks
+			if consensusHeight >= bts.firstTry+RebroadcastTimeout {
+				if err := w.deleteBroadcastedTSet(tSetID); err != nil {
+					return err


probably better to log the error than to return in this case.

…ries counter

… error

ChrisSchinnerl requested review from DavidVorick and lukechampine January 4, 2018 23:14

DavidVorick reviewed Jan 5, 2018

View reviewed changes

ChrisSchinnerl force-pushed the wallet-rebroadcast branch 4 times, most recently from 87422ae to 29c5100 Compare January 5, 2018 14:42

DavidVorick reviewed Jan 7, 2018

View reviewed changes

ChrisSchinnerl force-pushed the wallet-rebroadcast branch from 767ff66 to 17b5d9e Compare January 8, 2018 17:16

DavidVorick reviewed Jan 16, 2018

View reviewed changes

ChrisSchinnerl force-pushed the wallet-rebroadcast branch from f26934a to 0d3edb8 Compare January 16, 2018 15:35

DavidVorick reviewed Jan 16, 2018

View reviewed changes

ChrisSchinnerl added 3 commits January 17, 2018 16:42

Make wallet rebroadcast transactions until they are confirmed

41eeb45

move broadcast code to new file and use RespendTimeout instead of a t…

f707467

…ries counter

fix failing TestValidRevertedTransaction

6d54370

ChrisSchinnerl added 9 commits January 17, 2018 16:42

fix race condition

cdc217b

persist tracked transaction sets

dd3e2f7

make tpool broadcast transactions if there is a duplicate transaction…

4fc4302

… error

added api integration test

8a023f0

export rebroadcast timeout for other modules and skip pruning testcase

538b838

fix segfault

8c3091a

reset txn height after rebroadcasting txn

3164e5b

move broadcast out of lockedTryTransactionSet

4635265

changes requested in review

00e9604

ChrisSchinnerl force-pushed the wallet-rebroadcast branch from 5260b38 to 00e9604 Compare January 17, 2018 21:50

Uh oh!

Conversation

ChrisSchinnerl commented Jan 4, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidVorick commented Jan 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidVorick commented Jan 5, 2018

Uh oh!

4Dolio commented Jan 5, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DavidVorick commented Jan 7, 2018

Uh oh!

jarcko commented Jan 11, 2018

Uh oh!

DavidVorick commented Jan 16, 2018

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants