Detect unmatched swaps

[lukechilds]

There is no event emitted on the socket for an unmatched swap. Are you able to add this?

If not, what's the correct way for us to detect it? When we place the order we get timeleft in the response. After timeleft seconds have passed we could check if the connected event has been emitted yet.

Is that the correct event to check for? Would that be reliable?

[jl777]

probably as good as can be done with current limitations.

after timeleft seconds are elapsed if the aliceid didnt get any events, that swap is likely not going anywhere.

[lukechilds]

We get request events almost as soon as we place the swap. Is there a specific event we get that we can consider a trade matched with before the timeleft limit?

What logic are you using internally? I'm assuming if timeleft is 60 seconds and bob tried to connect 10 minutes later you wont let him. Whatever you're doing internally to decide whether a trade is open to be matched or not we need to mirror in the UI to guarantee accuracy.

probably as good as can be done with current limitations.

What are the current limitations? We need to make sure this is accurate. It's going to be really bad if the UI shows the swap as not matched but mm executes the trade.

[jl777]

the request event is what is sent out, so you should see that.
if you dont, then there was some connectivity issue with the network and the swap request is most likely not seen by any bob and it will just timeout

to get a trade, you need to get back at least one requested
then a connect is sent and finally a connected is received

all that has to happen for a swap to commence. If there is no requested that comes back within the timelimit, the swap is not going to happen.

however as soon as you are getting requested messages, you should now have requestid/quoteid and if you see a connect message that your node sent out, you are just waiting on bob to respond. in such cases, bob can respond later than the timeleft as it is considered a swap negotiation in progress.

it would be best to use swapstatus(requestid, quoteid) as soon as those fields are available as internally it switches to a different process to track swaps. there is a 2 minutes time from the connect message to a swap being abandoned, you should see a "failed" message

I know you want this to all be nice and simple, but the reality is that it is a messy and unpredictable process...

[lukechilds]

If there is no requested that comes back within the timelimit, the swap is not going to happen.

What do you mean by "requested"? For a successful trade I'm not seeing a message with a requested method. I get these method values: request, reserved, connect, connected, update, tradestatus.

Are you referring to reserved or connect? Or are we missing messages?

in such cases, bob can respond later than the timeleft as it is considered a swap negotiation in progress.

So in those cases, when swap negotiation is in progress, are we guaranteed to receive a failed message if something goes wrong? Or if bob misbehaves are there things we need to handle that mm won't notify us of?

[jl777]

I am adding uuid to all trade related JSON, hopefully when that works it will solve all these related issues

[lukechilds]

The uuid has helped loads with tracking the swaps, however it doesn't resolve the failed swap issue.

When we place a swap we set it as "pending". On future messages we update the status. However if a swap never gets matched, we don't get new messages to allow us to update the state so it stays in our UI as "pending" forever.

Can you provide concise steps on how we can handle this and correctly update failed swaps.

If there is no requested that comes back within the timelimit, the swap is not going to happen.

So if we don't have the "requested" message before timelimit we can consider it failed. What message is this? reserved, connect or something else?

if you see a connect message that your node sent out, you are just waiting on bob to respond. in such cases, bob can respond later than the timeleft as it is considered a swap negotiation in progress.

there is a 2 minutes time from the connect message to a swap being abandoned, you should see a "failed" message

So if timeleft has expired the swap has failed unless we have a connect message, in that case, wait for an extra 2 minutes. If we don't have a connected message by the end of those 2 minutes, the swap has failed.

Is that the correct logic for accurately detecting failures?

[jl777]

I think as each message comes in, extend the timeout by 2 minutes.
ie. a reserved coming in gives hope, so set the timeout back to 2 minutes
a connect going out means it is very close, reset the timeout back to 2 minutes
a connected is very clear indication a swap should start soon, but just in case set the timeout back to 2 minutes

[lukechilds]

Is that the logic you're using internally? We need it to be identical so there's no possibility of the UI swap state differing from the mm swap state.

a connected is very clear indication a swap should start soon, but just in case set the timeout back to 2 minutes

So after those two minutes since connected have expired, what is the earliest message we can use to confirm the swap has started? Have at least one update message?

[sindresorhus]

There is no event emitted on the socket for an unmatched swap. Are you able to add this?

@jl777 It seems there are a lot of heuristics needed to detect a failed swap. I think it would be better for marketmaker to emit a failed event, based on the heuristics you've mentioned, than for every app to try to imitate it and get it subtly wrong.

[jl777]

other than the one edge case so far, the failed messages invariably mean the swap failed. It would be too major of a change to overhaul the entire swap negotiation and swap processing code to eliminate this one edge case.

Of course, if you are able to describe to me how exactly I can do this safely, then I would be willing to try. Last time we tried to do something I felt was too dangerous, it turned out I was right. Will this time be different?

[jl777]

@lukechilds since this is a p2p system, the other side is not guaranteed to follow any specific timing. The current timeouts are 2 minutes and 5 minutes depending on where in the process, but that is subject to change.

I think if you had the logic of "uuid got some change" to push back the expiration it would be a very good heuristic. Then after some GUI determined amount of time if the swapstatus also says it is finished, then it is finished.

if a uuid has activity of any kind, it isnt finished.
If swapstatus isnt "finished" it isnt finished

[lukechilds]

I think there may be some misunderstandings here.

other than the one edge case so far, the failed messages invariably mean the swap failed.

This issue is completely unrelated to the failed/completed edge case in #756. We are talking about how we can detect unmatched swaps, not failed swaps. e.g If it doesn't transition from pending to matched, we need to show the trade as failed.

It would be too major of a change to overhaul the entire swap negotiation and swap processing code to eliminate this one edge case.

Of course, if you are able to describe to me how exactly I can do this safely, then I would be willing to try.

Regarding the unmatched event, this isn't something we were asking you to add to the swap negotiation, you wouldn't need to change that at all.

You must have some sort of internal logic that decides when a pending trade has expired and can no longer be matched. Whenever that happens if you could send a message over the socket (to our GUI not to other peers) that would instantly solve the issue, prevent the chance of the GUI going out of sync with mm and wouldn't require any changes to swap negotiation.

This would be the ideal solution, but if that's not possible then we can try and replicate your behaviour as close as possible.

The current timeouts are 2 minutes and 5 minutes depending on where in the process, but that is subject to change.

So if these timeouts are going to change would it be possible to maybe get the time to wait for the next message in each message? So every single message we get, we update to a new expired timestamp from that message, then if we ever hit the timestamp we know it's timed out. This would be a backwards compatible change and give you the freedom to change the timeout amounts without breaking GUIs.

For example the socket message (and response from buy/sell) would be:

{
  uuid: 923572309092387598347528734,
  expires: 1523532765,
  ...
}

after 1523532765 if we don't have another message, the swap has failed. If we get another message before then, it should have a new expires value that we wait for.

if a uuid has activity of any kind, it isnt finished.
If swapstatus isnt "finished" it isnt finished

That's not the problem, it's not about detecting when it's finished. It's detecting if it never matched. If a swap doesn't get matched it will never have a swapstatus of "finished", but that doesn't mean it's pending forever.

[jl777]

Sorry, I misunderstood.
I can add such an event that should work most of the time, but it wont be 100%
I might have time today for this

[lukechilds]

I can add such an event that should work most of the time

Awesome, thanks!

Ping me when it's ready to test.

but it wont be 100%

Why don't you think it'll be 100%? Just need to know if there are any edge cases I should look out for.

[jl777]

just a feeling I have. this sort of thing isnt 100% as we have different threads running different parts, so while one thread is thinking one thing, the other thread sees a different state.

I didnt want to put locks around it as that would really increase latency and the cost is the occasional false positive

[jl777]

updated with a special failed message -9999 but it might give spurious events if after other messages have come in. Not sure it will, but not sure it wont.

However for the state of having issued a buy/sell and it expires, it should be pretty reliable indicator. I would set a timer after you get this event and if nothing else comes in after 10 seconds, it would seem pretty safe that nothing ever will

[lukechilds]

updated with a special failed message -9999 but it might give spurious events if after other messages have come in. Not sure it will, but not sure it wont.

However for the state of having issued a buy/sell and it expires, it should be pretty reliable indicator.

Appears to be working great, thanks James!

I would set a timer after you get this event and if nothing else comes in after 10 seconds, it would seem pretty safe that nothing ever will

We've already implemented a 10 min delay after a failed event before removing our event listener to resolve #756 (comment). So hopefully that should be sufficient for this too.

[lukechilds]

@artemii235 Any chance we could get a new build with these changes in?

[artemii235]

@lukechilds Hi, I merged changes from dev branch and waiting for CI build to pass, if everything is fine new release will be published in 30 minutes.

[lukechilds]

@artemii235 Perfect, thanks!

[artemii235]

@lukechilds https://github.com/artemii235/SuperNET/releases/tag/v1.0.141 - here is new release, additional tests might be required as only BEER/ETH swap was done by CI.