server/auth,multi: connect resp excludes revoked match requests#653
Conversation
The connect response includes a list of active matches, and it triggers sending pending requests and messages. This excludes match-related requests from being sent if the match in question is not included in the list of active matches. Only pending revoke_match requests are allowed in this case, purely as a courtesy to the client. Similarly, the connect timeout for revoke_match requests is nown 4 hours. Test RequestWhenConnected/SendWhenConnected.
chappjc
changed the title
| if len(extras) > 0 { | ||
| details := fmt.Sprintf("%d matches reported by %s were not found for %s.", len(extras), host, t.token()) | ||
| t.notify(newOrderNote("Match resolution error", details, db.ErrorLevel, corder)) | ||
| // t.negotiate(extras) // missed match message request, but match not revoked (?!) |
There was a problem hiding this comment.
Just a thought. If the match isn't revoked yet, the pending match request should be coming, so this should not be necessary though.
| if t.matches[mid] != nil { | ||
| log.Warnf("Skipping match %v that is already negotiating.", mid) | ||
| continue | ||
| } |
There was a problem hiding this comment.
I don't see a way this could happen at present, but starting up a new matchTracker and overwriting an existing one would be bad news.
| }() | ||
|
|
||
| s.log.Info("HandleConnect done.") | ||
| s.log.Trace("HandleConnect done.") |
There was a problem hiding this comment.
Sorry, never should have been Info. However, with ws.UseLogger added to client/core/log.go, we can get a lot more information about what the frontend is doing when it spins on market load.
| return handler(client.acct.ID, msg) | ||
| msgErr := handler(client.acct.ID, msg) | ||
| if msgErr != nil { | ||
| log.Debugf("Handling of '%s' request for user %v failed: %v", route, client.acct.ID, msgErr) |
There was a problem hiding this comment.
Auth route handlers like handleInit can respond to the client with an error, but we never knew what the errors were, making debugging from the server side difficult.
| mid, err := pr.req.ExtractMatchID() | ||
| if err != nil { | ||
| log.Errorf("Failed to read matchid field from '%s' payload: %v", pr.req.Route, err) | ||
| // just send it | ||
| } else if mid != nil && pr.req.Route != msgjson.RevokeMatchRoute { |
There was a problem hiding this comment.
Non-nil error is an unmarshal error, but matchid is expected to be nil for payloads with no "matchid" (not an error).
| // back into the pending requests. | ||
| connectTimeout := DefaultConnectTimeout | ||
| if pr.req.Route == msgjson.RevokeMatchRoute { | ||
| connectTimeout = 4 * time.Hour // a little extra time for revoke_match to be courteous |
There was a problem hiding this comment.
It's little cost to queue thousands of these for hours, and it helps the client when they do reconnect, especially if their dexc never restarted, only reconnected after prolonged outage (laptop suspend or network off).
buck54321
left a comment
buck54321
left a comment
There was a problem hiding this comment.
Let me know about moving the one function, but otherwise g2g.
| // ExtractMatchID attempts to extract a "matchid" value from a request-typed | ||
| // Message's Payload. The request must have one of the following routes: | ||
| // AuditRoute, RedemptionRoute, RedeemRoute, MatchRoute, or RevokeMatchRoute. | ||
| // A non-nil error is only returned if unmarshalling the payload fails. | ||
| func (msg *Message) ExtractMatchID() (Bytes, error) { | ||
| // Name the targeted request routes. |
There was a problem hiding this comment.
This appears to only be used in one place in auth.go. Can this be an auth package function rather than a *Message method?
There was a problem hiding this comment.
Sure. I actually started with this simply inline in auth.handleConnect and for some reason thought it better in msgjson. Not seeing that anymore though.. switching back.
There was a problem hiding this comment.
Looking again, the reasoning for a msgjson package method:
- the field
"matchid"must match what's used in msgjson - the named routes must be current (and the exclusion of RevokeMatchRoute is the one auth-specific detail)
Changes to msgjson are likely to overlook an auth function that expects certain things such as the specific json tag and a list of relevant request routes with a relevant payload.
There was a problem hiding this comment.
Fair enough. I'm just policing the interface a little. In general, seeing Message methods that apply to only certain routes raises flags for me. But as long as there's a good justification, go for it.
| sort.Slice(msgs, func(i, j int) bool { | ||
| return msgs[i].msg.ID < msgs[j].msg.ID | ||
| }) |
There was a problem hiding this comment.
We're using a uint64 for these msg IDs, so at a billion messages a day, once the DEX has been running without interruption for 50,539,024 years, the IDs are going to start over at zero and this is going to break.
3 participants
server/auth:
'connect'response excludes match-related requests for revoked matchesThe
'connect'response includes a list of active matches. connect also triggerssending pending requests and messages. This change excludes match-
related requests from being sent if the match in question is not included
in the list of active matches. Only pending
revoke_matchrequests areallowed in this case, purely as a courtesy to the client. Similarly, the connect
timeout for
revoke_matchrequests is now 4 hours.Test
RequestWhenConnected/SendWhenConnected.dex/msgjson: add
(*Message).ExtractMatchIDclient/core: missing matches are assumed revoked
Also improve logging on the most important client and server events. INF level for a trade (both sides):
Maker, sell 20 DCR
Taker, buy 20 DCR