core: don't fail match on misses and redeem errors #772
Conversation
|
@buck54321 made this change because of an issue I encountered - my redeem contract did not broadcast successfully because my wallet was locked. I unlocked the wallet, but dexc never retried the broadcast. Running with the changes in this PR resolved the issue - dexc retried the broadcast until it succeeded. Thanks for the support! |
|
Thanks jholdstock. You had a peculiar issue with bitcoind reporting unlocked via getwalletinfo, which seems like a bitcoind issue, but we'll consider ways to be more robust. Encrypting an unencrypted wallet while dexc is running seems to be trouble that we'll work around too. |
buck54321
force-pushed
the
failerr
branch
2 times, most recently
from
4b92346 to
9c92b98
Compare
|
Just changed base branch to master. We'll cherry-pick from master into release-0.1 instead of the other way around. The conflict will go away when rebasing on master |
client/core/trade.go
Outdated
| @@ -162,6 +179,16 @@ func (t *trackedTrade) broadcastTimeout() time.Duration { | |||
| return time.Millisecond * time.Duration(t.dc.cfg.BroadcastTimeout) | |||
| } | |||
|
|
|||
| // delayTicks sets the tickGovernor to prevent retrying to quickly after an | |||
client/core/trade.go
Outdated
| c.swapMatchGroup(t, []*matchTracker{m}, errs) | ||
| } | ||
| if len(groupables) > 0 { | ||
| c.swapMatchGroup(t, groupables, errs) |
There was a problem hiding this comment.
I'm thinking about if we should have the groupables go first since these are all in series now, and if one of the suspect ones takes too long (something is funny with them after all and the server request or something else could be slower than usual), the ok ones in groupables might take too long to get to.
client/core/trade.go
Outdated
| lastActionTime := match.matchTime() | ||
| if match.Match.Side == order.Taker { | ||
| // It is possible that AuditStamp could be zero if we're | ||
| // recovering during startup. The implications of this is that | ||
| // if we are 1) already in start up recovery, and 2) we fail | ||
| // again, we will not try again even if we have time before the | ||
| // broadcast timeout. | ||
| lastActionTime = encode.UnixTimeMilli(int64(match.MetaData.Proof.Auth.AuditStamp)) |
There was a problem hiding this comment.
during startup -> after reconnect? or just "if we missed the audit request"
The audit request may be missed while disconnected, and we'd get here after reconnect->match_status resolution, not just restart of dexc. I think we just observed this, although in the maker redeem situation where the audit was of taker's contract.
client/core/trade.go
Outdated
| if time.Since(lastActionTime) < t.broadcastTimeout() { | ||
| t.delayTicks(match, t.dc.tickInterval*3/4) |
There was a problem hiding this comment.
If they missed the audit request, this would jump to setting swapErr even though there's possibly still time to try again.
If we don't know the audit time stamp, we'd ideally figure out the time that the counter party's contract reached swapConf. That's trivial or free of backend RPCs, so we can at least set lastActionTime to match.matchTime() + something or even time.Now - something if we don't have an AuditStamp.
Another idea is to always allow at least 1 retry if AuditStamp is zero. To do that might require putting the bogus value mentioned above in AuditStamp though.
client/core/trade.go
Outdated
| // we will not try again for an hour even if we have time before the | ||
| // broadcast timeout. |
client/core/core.go
Outdated
| (m.suspectSwap && t.wallets.fromAsset.ID == assetID || | ||
| (m.suspectRedeem && t.wallets.toAsset.ID == assetID)) { |
There was a problem hiding this comment.
Missing parenthesis around the m.suspectSwap && t.wallets.fromAsset.ID == assetID.
Perhaps this would read better like:
isFromAsset := t.wallets.fromAsset.ID == assetID
for _, m := range t.matches {
if m.tickGovernor != nil &&
((m.suspectSwap && isFromAsset) || (m.suspectRedeem && !isFromAsset)) {
dexc
core