swap: fix missing nil redemption check#494
Conversation
| // Taker either has not redeemed or the redemption has not reached | ||
| // the required confirmations. If the txid matches the immature | ||
| // redeem, it is considered monitored. | ||
| return takerStatus.redemption != nil && takerStatus.redemption.TxID() == txid |
There was a problem hiding this comment.
redeemStatus and makerRedeemStatus were checking swapStatus.redeemTime.IsZero() before attempting to dereference swapStatus.redemption. This PR adds a redemption != nil before checking takerStatus.redemption.TxID()
| _, takerRedeemDone := s.redeemStatus(makerStatus, takerStatus) | ||
| if takerRedeemDone { | ||
| // Taker has a redemption that has reached required confirms. | ||
| return false // so no longer monitored? |
There was a problem hiding this comment.
I honestly don't follow the reasoning for considering it not monitored if we know about the redemption and it has reached the required confirms, but considering it monitored if it has not reached the required confirms. @buck54321 Thoughts about why we don't just consider a tx monitored if it corresponds to a redemption regardless of the confirmations? After all, if it is found in the Swapper.matches map, the swap is still active. If it is not found, the user would be subject to the usual FundConf requirement.
There was a problem hiding this comment.
I think you've got it right, but I'll note that the matches map is being pruned in processBlocks, so TxMonitored won't hit after MatchComplete anyway. We either need the full DB solution, or Swapper should hang onto matches until FundConf on the redemptions.
There was a problem hiding this comment.
but I'll note that the
matchesmap is being pruned inprocessBlocks, soTxMonitoredwon't hit afterMatchCompleteanyway
That's what I was getting at with "if it is found in the Swapper.matches map, the swap is still active". In case it's not found the user will just have to wait for FundConf.
A DB powered solution has been on the table for a while, although I'm super hesitant to load the DB for with a check of all matches for each order placed that is funded by coins with < FundConf confirms, so the alternative you mentioned of Swapper hanging on to matches until FundConf sounds more appealing.
Anyway, on this current implementation, I think I'll remove the redeemStatus/makerRedeemStatus function calls from TxMonitored since I think we agree that it makes no sense to consider the tx not monitored because the redeem has reached SwapConf. Correct?
In (*Swapper).TxMonitored, redemptions were being considered not monitored once they reached SwapConf even though they were still active swaps (in Swapper.matches map). This removes the confirms check entirely from TxMonitored, instead indicating if there is an active swap with the txid as a known contract or redeem. Also reduce excessive locking in TxMonitored. Eliminate extra "redeem status" functions.
| // Confirm both redeem txns up to SwapConf so they are no longer monitored. | ||
| matchInfo.db.makerRedeem.coin.setConfs(int64(rig.abc.SwapConf)) | ||
| matchInfo.db.takerRedeem.coin.setConfs(int64(rig.xyz.SwapConf)) | ||
| sendBlock(rig.abcNode) // trigger process block to remove them |
There was a problem hiding this comment.
Note this change in behavior. Now the match needs to be pruned from the matches map before it is no longer found by TxMonitored. Meeting the confirmations threshold no longer makes it not monitored.
|
Replaced by #499 |
3 participants
No description provided.