Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

client/asset/dcr: workaround TxDetails unconfirmed bug #2444

Closed
wants to merge 1 commit into from

Conversation

chappjc
Copy link
Member

@chappjc chappjc commented Jul 20, 2023

Pertains to #2442.

There is an issue given:

  • taker's swap tx is DCR
  • you shutdown dexc after the (taker) DCR swap transaction is broadcast, but before it is mined
  • start dexc, login
  • wait for DCR block(s), transaction always appears unconfirmed, swap never proceeds (except refund)

I believe you must be both maker and taker (self trade) for it to halt the swap because if you are not also the taker who authored the DCR swap transaction, then for maker, the SwapConfirmations method will fallback to cfilters scan when lookupTxOutput fails to find the transaction. Or so I would expect since the counterparty contract transaction would not be a wallet transaction.

The above is not always reproducible, but I managed to get it to happen twice with the native SPV wallet.

I tracked the culprit down to dcrwallet/wallet/udb.(*Store).TxDetails. It will find and return an unmined transaction that it finds with existsRawUnmined and unminedTxDetails (but it's really mined, several confs even).

https://github.com/decred/dcrwallet/blob/master/wallet/udb/txquery.go#L180-L194

I do not know if it would find a mined transaction also if it could get down to latestTxRecord and minedTxDetails, but it seems the transaction is left in bucketUnmined. i.e. deleteRawUnmined is not called as expected. There are only two call sites for deleteRawUnmined:

  • wallet/udb.(*Store).moveMinedTx
  • wallet/udb.(*Store).RemoveUnconfirmed

I don't know which isn't getting called.

Perhaps one detail worth noting is that I generally use a split tx in the order, meaning the resulting DCR swap contract transaction will have no change, so it's just the input/debit that belongs to the authoring wallet, and the one swap output technically belongs to neither wallet.

Comment on lines +3544 to +3546
if dcr.wallet.SpvMode() {
return txOut, confs, -1, nil // make it do a cfilters scan because GetTransaction lies
}
Copy link
Member Author

@chappjc chappjc Jul 20, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a possible workaround, which unfortunately forces the cfilters scan path even when GetTransaction did return a result (it's a wallet transaction i.e. checking your own swap confirms, or it's a self-trade)

Comment on lines +699 to +722
// Check with TxConfirms and TxBlock now, why not.
confs, err := w.dcrWallet.TxConfirms(ctx, txHash)
if err != nil {
w.log.Warnf("TxConfirms: %v", err)
} else if confs > 0 {
w.log.Warnf("TxConfirms says yes! %d confs", confs)
}
blockHash, blockHeight, err := w.dcrWallet.TxBlock(ctx, txHash)
if err != nil {
w.log.Warnf("TxBlock: %v", err)
} else if blockHeight > 0 {
w.log.Warnf("TxBlock says yes! block %v, height %d", blockHash, blockHeight)
ret.BlockHash = blockHash.String()
ret.Confirmations = int64(tipHeight - blockHeight + 1)
goto gotit
}
_, confs2, blockHashP, err := w.dcrWallet.TransactionSummary(ctx, txHash)
if err != nil {
w.log.Warnf("TransactionSummary: %v", err)
} else if confs2 > 0 {
w.log.Warnf("TransactionSummary says yes! block %v, confs %d", blockHashP, confs2)
ret.BlockHash = blockHashP.String()
ret.Confirmations = int64(confs2)
goto gotit
Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

All of this is because I wanted to see if any of the other wallet methods can give a conf or block inspite of TxDetails. It does not appear to return any different results. This seems to be because they all stop on existsRawUnmined, just like TxDetails.

@buck54321
Copy link
Member

buck54321 commented Jul 20, 2023

Appears that under normal operation without shutting down, dcrwallet always reaches deleteRawUnmined through moveMinedTx, ultimately from a headers announcement.

decred.org/dcrwallet/v3/wallet/udb.deleteRawUnmined({0x2811878, 0xc000b69ac0}, {0xc000b4d018, 0x20, 0x20})
	/path/to/dcrwallet/wallet/udb/txdb.go:1377 +0x36
decred.org/dcrwallet/v3/wallet/udb.(*Store).moveMinedTx(0x20?, {0x2811878?, 0xc000b69ac0}, {0x7fbf4c7a8c50, 0xc000b69bc0}, 0xc000b4cfd0, {0xc0013b42d0, 0x44, 0x44}, {0xc0000efd30, ...}, ...)
	/path/to/dcrwallet/wallet/udb/txmined.go:1125 +0xc09
decred.org/dcrwallet/v3/wallet/udb.(*Store).InsertMinedTx(0xbe5466cf452821e6?, {0x280acf8, 0xc0003cb648}, 0xc000b4cfd0, 0xc000b39770)
	/path/to/dcrwallet/wallet/udb/txmined.go:1269 +0xbf1
decred.org/dcrwallet/v3/wallet.(*Wallet).processTransactionRec thoughord(0xc0001a4200, {0x2804a98, 0xc0004c17c0}, {0x280acf8?, 0xc0003cb648}, 0xc000b4cfd0, 0xc000b44b60, 0xc000b39770)
	/path/to/dcrwallet/wallet/chainntfns.go:448 +0x17e
decred.org/dcrwallet/v3/wallet.(*Wallet).extendMainChain(0xc0001a4200, {0x2804a98, 0xc0004c17c0}, {0x1b93765, 0x12}, {0x280acf8, 0xc0003cb648}, 0xc000b44b60, 0x0?, {0xc0003cb638, ...})
	/path/to/dcrwallet/wallet/chainntfns.go:76 +0x80e
decred.org/dcrwallet/v3/wallet.(*Wallet).ChainSwitch.func1({0x280acf8, 0xc0003cb648})
	/path/to/dcrwallet/wallet/chainntfns.go:170 +0x890
decred.org/dcrwallet/v3/wallet/walletdb.Update({0x2804a98, 0xc0004c17c0}, {0x2804de0, 0xc00012a480}, 0xc0005f9bd0)
	/path/to/dcrwallet/wallet/walletdb/interface.go:252 +0x195
decred.org/dcrwallet/v3/wallet.(*Wallet).ChainSwitch(0xc0001a4200, {0x2804a98, 0xc0004c17c0}, 0xc00011e3f8, {0xc0003cb590, 0x1, 0x1}, 0xc00139e930)
	/path/to/dcrwallet/wallet/chainntfns.go:114 +0x2da
decred.org/dcrwallet/v3/spv.(*Syncer).handleBlockAnnouncements.func2(0xc00011e360, {0x2804a98, 0xc0004c17c0}, 0xc000b44b60, 0xc000b359e0?, {0xc0003cb558, 0x1, 0x4?}, 0xc0005f9ed8, 0xc0005f9ef0, ...)
	/path/to/dcrwallet/spv/sync.go:1223 +0xa8b
decred.org/dcrwallet/v3/spv.(*Syncer).handleBlockAnnouncements(0xc00011e360, {0x2804a98, 0xc0004c17c0}, 0xc000268960, {0xc0003cb558, 0x1, 0x1}, 0xc001381860?)
	/path/to/dcrwallet/spv/sync.go:1239 +0x2ca
decred.org/dcrwallet/v3/spv.(*Syncer).receiveHeadersAnnouncements.func1()
	/path/to/dcrwallet/spv/sync.go:940 +0x4b
created by decred.org/dcrwallet/v3/spv.(*Syncer).receiveHeadersAnnouncements
	/path/to/dcrwallet/spv/sync.go:939 +0x33

Way back at handleBlockAnnouncements, the new blocks are scanned for relevant transactions.

matchingTxs, err = s.scanChain(ctx, rp, bestChain, bmap)

which are passed in to ChainSwitch.

On startup though, they add headers via (*Syncer).Run -> connectToCandidates,connectToPersistent -> startupSync -> getHeaders where ChainSwitch is called with a nil relevantTxs argument, and I don't see a place where it attempts to catch wallet transactions in those blocks.

Still poking around.

@chappjc
Copy link
Member Author

chappjc commented Jul 20, 2023

On startup though, they add headers via (*Syncer).Run -> connectToCandidates,connectToPersistent -> startupSync -> getHeaders where ChainSwitch is called with a nil relevantTxs argument, and I don't see a place where it attempts to catch wallet transactions in those blocks.

Interesting. Just one thing I want to clarify is that when I've reproduced the bug, the wallet started up at the same best block as when it shut down, and only a bit later (minutes) the tx would be really mined. This would correspond to the rescanPoint == nil case I believe.

@chappjc
Copy link
Member Author

chappjc commented Jul 21, 2023

As per convo with @jrick, there's a very long standing known bug with wallet startup when there are unconfirmed transactions, whereby they are often never confirmed without a rescan. That appears to be what we are seeing.

Assuming the underlying wallet bug isn't fixed, I propose the workaround I called out in lookupTxOutput where we force a cfilter scan if the number of confirmations is 0 and it's an SPV wallet. This should be very low cost. If it is actually unconfirmed, it is very likely the cfilters scan will be a no-op, scanning 0 blocks.

@buck54321
Copy link
Member

Critical fix pulled out in #2555. Closing this in favor of issue #2556

@buck54321 buck54321 closed this Oct 3, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants