server/swap: better contract expiry edge case handling #1541
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
5ffc955 to
c7548ff
Compare
fc30a4f to
bbce25c
Compare
JoeGruffins
approved these changes
Mar 25, 2022
Comment on lines
+1049
to
+1054
| expectedTakerLockTime := match.matchTime.Add(s.lockTimeTaker) | ||
| if expectedTakerLockTime.Before(now) { | ||
| log.Infof("Revoking match %v at %v because the expected taker swap locktime would be in the past (%v).", | ||
| match.ID(), match.Status, expectedTakerLockTime) | ||
| failMatch(false) | ||
| } |
There was a problem hiding this comment.
Could even add a minute or so? Taker can't be expected to react instantaneously.
There was a problem hiding this comment.
Same thoughts here, but didn't do it because tests use arbitrary and often crazy short lock times.
buck54321
reviewed
Mar 30, 2022
|
I actually still have to run the trade_simnet_test.go test, which I suspect might have issues with exceeded lock times. I'm confident these changes are appropriate, but the tests might need updating.. Also I'm not merging anything of mine until I give proper attention to #1510 and #1523 now that they've gotten reviews from the rest of the team. |
In handleInit and handleRedeem, do not bother to limit the expiration time of the processInit/processRedeem waiters by the broadcast timeout. Both of the TryFuncs (processInit and processRedeem) will respond in error if the match is revoked, so there is no need to tweak the waiter's expire time. This will prevent a client request timeout when the init or redeem is submitted just before the Swapper revokes the match, when the waiter's expire time could be set to the past despite the inaction checks having not yet revoked the match.
This updates Swapper to proactively revoke matches where any known swap contracts are expired, or in the case of MakerSwapCast where the expected taker contract would be created with a lock time in the past. These scenarios are possible if a transaction takes an extremely long time to reach the required number of confirmations. The new checks are done in checkInactionEventBased, which is called on a ticker. The (*Swapper).failMatch method is given a userFault bool input argument to indicate if the actor should be penalized. matchTracker is given an expiredBy method that returns true if the lock time of either party's *known* swap is before the reference time. Normally this will be called with the present time. checkInactionBlockBased now consults expiredBy when calling failMatch to determine if either of the swap contracts are expired, justifying the inaction. NOTE: client should be updated with similar logic to prevent attempting to make expired contracts or contracts that are about to expire. server/db: option to store forgiven revoked matches When a match is revoked due to no fault of the user, it should be saved in the DB with the forgiven flag set.
If the located contract transaction in processInit is found to have a lock time that is in the past (already expired) revoke the match immediately and respond to the user with an error. No penalties are assesed since processInit first verifies that the proper lock time has been used. This would be the case if the maker's swap took a very long time to reach the required number of confirmations, and when the taker's turn to publish their swap came around it was already passed the expected lock time for their swap (matchTime + 8 hrs). NOTE: client should be updated with similar logic to prevent attempting to make expired contracts or contracts that are about to expire.
bbce25c to
e90d192
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This PR contains three closely related updates to
Swapperto improve the handling of matches around broadcast timeout and contract expiration.server/swap: wait for init/redeem for full wait expiration bf2001c
server/swap: revoke when contracts are/would be expired 1be939f
server/swap: do not accept expired contracts in processInit bbce25c