client/core,asset: ensure coins are unlocked when no longer needed

[itswisdomagain]

Coins are currently unlocked when

• a market order or limit order with tif immediate does not get matched
• an order is canceled (i.e. by user action)
• a market is suspended with persist=false

Other times to unlock coins (implemented in this PR):

• when new orders could not be submitted to the server, e.g. if sending the
  request or validating the server's response fails.
• when all matches are either swapped or revoked for unbooked orders, i.e.
  orders that will not receive future matches.
• after sending a swap, unlock the spent outputs to ensure accurate balance
  reporting.

Also revoke orders in UnbookOrder note handler to ensure that coins are
unlocked for server-revoked orders. If such orders remain at status Booked,
their coins will never be unlocked because the expectation would be that the
order would receive new matches in the future.

UPDATE: As @chappjc pointed out, revoking orders in the UnbookOrder note
handler is a hack at best, and not a very good one. The client currently has no
way of detecting revoked orders; with the implication that coins for such orders
will remain perpetually locked.

Resolves #631.

[chappjc]

Some righteous changes here, but I don't think the unbook order interaction is going to work out, mainly because it is subscription based but also because it's awkward to push all unbooked orders through a bogus revoked status.

[chappjc]

After #513 is done and you're back to this one, can you call out any clearly-neglected coin locking sites are fixed? There's a lot of refactoring here, so it's hard to get an idea of which specific cases are resolved.

[itswisdomagain]

Called that out in the PR description but was probably not clear. I've edited the description.
Coin-unlocking cases implemented here are:

• when new orders could not be submitted to the server, e.g. if sending the
  request or validating the server's response fails.
• when all matches are either swapped or revoked for unbooked orders, i.e.
  orders that will not receive future matches.
• after sending a swap, unlock the spent outputs to ensure accurate balance
  reporting.

Will recheck to confirm I haven't missed any, but last I checked, those were it.

[chappjc]

To simplify this, we can allow some cases to fall back to unlocking, possibly redundantly, on retirement of trackedTrades. #665

Perhaps we can simplify the approach in this PR considerably to focus on the low-hanging fruit that does not require unbook ntfn tricks.

[itswisdomagain]

we can allow some cases to fall back to unlocking, possibly redundantly, on retirement of trackedTrades

Makes sense. To further simplify, we can make trade retirement the sole point of coin unlocking - only unlock coins when the associated trade/order is retired.

The only caveat I see there is some trade coins will remain locked for longer if a trade is kept alive simply for refund or auto-redeem reasons, as those recovery actions may take a while. We could ofc unlock coins for such orders (while not retiring them) and set t.coinsLocked = false and t.changeLocked = false on those trades to prevent re-attempting to unlock the coins when the trades are later retired.

If you're okay with having all coin unlocking done from the site of trade retirement, will you want to take that up in #665 or should I pull in your #665 commits and finish up here?

[buck54321]

If you're okay with having all coin unlocking done from the site of trade retirement

I'm pretty sure that would break things. Most importantly, it would prevent someone from using remaining funds from a canceled, but partially filled order until all matches are fully negotiated.

Let's roll with this PR as is.

[itswisdomagain]

A breakdown of ideal coin-unlocking sites, particularly important with commit 48b0e22 stopping coin unlocking on client shutdown. Checked checkboxes with no link to implementation are implemented in this PR.

Orders for which coins should be unlocked:

• New orders that did not make it to the DEX, e.g. if sending the request or validating the server's response fails.
  Coins are now unlocked when an error prevents the order from reaching the DEX.
• Unmatched orders, unmatched either because
  • preimage request was missed (order would be revoked by the server), or
  • preimage was revealed but order was not matched in the epoch matching cycle (for market and tif-immediate limit orders). Coins are unlocked in the no_match notification handler.

All other orders that receive one or more matches have no further use for locked coins when they're Executed, Revoked or Canceled AND swaps have been sent for all of the order's active (unrevoked) matches. These include partially matched market and tif-immediate limit orders (= Executed). Coin unlocking for such orders should be performed when:

• the last set of swaps are sent
• a match is revoked and the revoked match is the last match for which a swap was required
• the order is canceled with no swap-pending match
• the order is revoked because the market is suspended with persist=false, with no swap-pending match
• the order is revoked by the server, with no swap-pending match

The following are cases where coins will remain needlessly locked, and resolution measures to ensure the coins are unlocked as required:

Client is unware that...	Recover and unlock coins...
an order has been revoked because client missed the preimage request	#597 lists ideas for detecting revoked orders on DEX connect
a market or tif-immediate limit order was unmatched because client missed the no_match request	#597 lists ideas for detecting unmatched but executed orders on DEX connect
a match has been revoked (e.g. client missed the revoke_match request)	when the client (re-)connects the DEX, "notices" the revoked matches and confirms that the revoked matches are the last set of matches that would have required sending a swap
a standing limit order has been revoked (due to inaction on a match)	when a revoke_order notification is received (not yet implemented, compare #526). Also, #597 lists ideas for detecting revoked orders on DEX connect
a standing limit order has been canceled (missed match request containing the matched cancel match)	#597 lists ideas for detecting canceled orders on DEX connect

[itswisdomagain]

I'm okay with merging this, leaving further coin unlocking work for the implementation of order status recovery described in #597 (comment).

Is there any merit to temporarily restoring coin unlocking on asset shutdown? Most of those coins will be relocked on client restart, except coins for

• inactive orders (coins for these should have already been unlocked before client shutdown, or is it possible for previously active orders to be immediately considered inactive on client restart?)
• new orders submitted to the DEX but that could not be persisted to db, their coins currently remain locked until client shutdown and they're not locked on restart; could consider unlocking them right away, after all the client ditches the order right away. Only caveat to unlocking the coins right away is subsequent order placements may attempt to re-use the funding coins for the order and would get rejected by the server - until the server ditches the order (preimage request miss) and unlocks the coin, that is.
  

  dcrdex/client/core/core.go

  Lines 2151 to 2155 in 7671c15

  	err = c.db.UpdateOrder(dbOrder)
  	if err != nil {
  	logAbandon(fmt.Sprintf("failed to store order in database: %v", err))
  	return nil, 0, fmt.Errorf("Order abandoned due to database error: %w", err)
  	}

In summary, I don't think client shutdown should be the trigger for unlocking coins. Only unlock coins when they're no longer needed, falling back to unlocking coins when trades are retired. As long as the client considers a trade active, the associated coins should remain locked.

I'm probably gonna want to move the client coin unlocking requirement described in #648 (comment) to an issue to ensure coins don't stay locked against expectations.

[itswisdomagain]

I'm reconsidering this. The client does not save the order to db, nor will it respond to preimage requests for the order. Only advantage to keeping the coins locked is to prevent new orders from using these coins as they're already locked by the server. But we can't keep these locked forever. And there's no client-restart-agnostic reliable way of knowing that the server has also ditched the order, so we can safely unlock coins.

Any objections to unlocking right away?

[chappjc]

This is an odd case. It's hard to imagine the request failing but it actually being booked, but if the client cannot do anything with the order, I don't see a reason to keep the coins locked.