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

app: auto-revoke matches when conditions warrant #655

Closed

Conversation

itswisdomagain
Copy link
Member

@itswisdomagain itswisdomagain commented Sep 1, 2020

Add a trackedTrade.maybeRevoke method to auto-detect and revoke matches that remain in a status longer than expected, and with the expectation/assumption that such matches would have been revoked by the server.

This is an initial attempt at handling cases where a client misses revoke_match messages as described in #643. As mentioned in #643, a complete resolution would likely require adding a new match_status route for detecting revoked matches.

This first pass helps to resolve the following issues identified in #643 relating to revoked matches being considered active by a client:

  • prevents clients from taking their next action on a match if the match has been idle for longer than expected.
  • makes it possible for clients to auto-redeem counter swaps sooner than later; auto-redemption are not executed for unrevoked matches.
  • enable early unlocking of funding and change coins when the last match of an order is revoked before the final swap was sent but the revoke_match request was missed. Without detecting the revocation, such coins would remain locked until client restart. Related client/core,asset: ensure coins are unlocked when no longer needed #648.

Also resolves #629 by triggering swap confirmation checks on tip change rather than on tick.


// checkSwapConfirmations checks for matches whose maker or taker swap have
// received the reqiured confirmations and records the swap confirmation time.
func (t *trackedTrade) checkSwapConfirmations() {
Copy link
Member Author

Choose a reason for hiding this comment

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

Currently, this is only called on tip change as discussed in #629, and while that is preferable, it just occurred to me that trigger on tip change alone might not be sufficient.

If this checkSwapConfirmations method is not called soon after a new block is mined (e.g. if called after 2 new blocks), this client's next step on this match may be delayed beyond the permitted max action window, causing the match to be revoked.

While I'd prefer to not check swap confirmation count multiple times every broadcastTimeout (currently every 20 seconds given a default broadcastTimeout of 1 minute), it might be the safest thing to do. Thoughts?

Unless it is possible for Core to miss a tip change, it is pointless
to continuously check swap confirmations every tick.
revoke_match messages may be missed in unusual cases (such as poor connectivity
for extended periods). If the client app is not restarted, match negotiations
will continue for such matches (when connectivity is restored, for e.g.) and
this could cause a client to take their next action such as sending a swap even
though the match has been revoked.

Also, missing a revoke_match may cause other issues such as funding and change
coins remaining locked until client restart.

This adds a trackedTrade.maybeRevoke method to auto-detect matches that remain
in a status longer than expected, and with the expectation/assumption that such
matches would be revoked by the server, the client marks them as revoked.
@chappjc
Copy link
Member

chappjc commented Sep 1, 2020

I suspect this quickly become obsolete with match_status, which could be implemented with no more complexity than the present solution. Thoughts about the need for these heuristics remaining even with a match_status capability?

Let's pull it out of #513 though, as I think you were planning. Noting that #653 has a commit will set missing matches as revoked on startup. While that doesn't cover zany use cases like computer suspend/resume, I think such an approach is adequate in the interim.

@itswisdomagain
Copy link
Member Author

The only use case I foresee for these heuristics is determining when to send a match_status request. I believe only when there's reason to suspect that a match is idle for too long should we send a match_status request to check if the match is still active or has been revoked. How long to wait / how much idleness is suspicious, is where these heuristics might become relevant with match_status.

@itswisdomagain
Copy link
Member Author

@buck54321 has got a plan for implementing match_status client-side that will not require the estimations done here. I agree this will become largely obsolete once match_status is implemented client-side, so will be closing for now until (if) a use case resurfaces post-match_status-implementation.

Something I'm eager to see, which would not necessarily be resolved by the match_status impl. is preventing clients from taking their next action on a match if the match has been idle for longer than expected. #663 is on course to resolve that.

@itswisdomagain itswisdomagain deleted the auto-revoke-match branch September 14, 2020 08:20
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.

client/core: silence the not yet swappable/redeemable messages based on confs already checked
2 participants