Xud: Define & implement behavior for different kind of swap errors

[kilrau]

This issue is about a swap failure after the deal was agreed with the peer and the amount was held by the peer. Providing that the taker is acting reasonably fast once a deal agreed, the only "error" has to do with actual swap configuration and routing.

In such a case, if we get an error from the executeswap and we decide to remove the part of the peer order we tried to swap, we should ask why we are not removing the other part and the other orders.

For example, if we get "unknowPaymentHash" it is clear that the maker is not setup properly. If so, why to remove just that part that we tried to swap? why not to remove all orders and disconnect from the peer until next restart or next connect rpc call?

I think we need to consider this error by error and decide what we should do about it. If the swap fails on noroute from the peer to us, we may be able to swap a smaller amount so removing the current amount may not be the right handling.

[sangaman]

I think it makes sense to disconnect from a peer if we get an error indicating that something is improperly setup, unknown payment hash or no route for example. If we don't, we will probably wind up banning them very quickly if we keep matching with them and failing swaps. This way, we only score one negative reputation event and then give the peer a chance to correct the issue and reconnect to us.

[kilrau]

Let's continue here to list swap errors and proposed behavior each @offerm @sangaman

[sangaman]

As a corollary to this, I was thinking about extending the SwapFailureReason enum we have to cover all the possible reasons a swap can fail. Right now we only set a string error message on swaps when they fail/error. We can keep that for details on the error (like the error message returned from lnd), but it would also be nice to have a clearly defined reason for the failure where we can easily do something like if (swapDeal.failureReason === ____ ) { .... I was thinking that it would make solving this issue much easier, so maybe I'll start working on that first.

[sangaman]

I'm thinking here too, disconnecting from a peer might be too drastic even in case of an unexpected swap failure because it might only be an issue with a particular trading pair. If we have 10 trading pairs active, we don't want to kill trading on the other 9. Then again, we also don't want to keep accepting orders for the trading pair that failed.

We could update our handshake with the peer and drop support for that particular trading pair, but then we'd want a way to re-enable the pair in the future but there's no simple way of handling that I can think of. I'm brainstorming about something like a "sanity swap check" trading 1 satoshi for 1 satoshi that'd we use for verifying we can actually swap a given pair with a given peer.

[erkarl]

I agree on the disconnecting part. No need to disable the other 9 trading pairs. I think we need to rethink the reputation/banning system a little to add support for disabling/banning trading pair.

I'd keep it simple for re-enabling the pair by just using an increasing timeout.

[sangaman]

Re-enable the pair after some time has elapsed? That's fine I think, but it would be nice to have a way to check if swaps are working before we wind up with another failed swap. That's why I'm thinking something like 1 satoshi test swaps might be useful, perhaps even as part of the initial handshake. That's tangential to this issue but if it seems potentially worthwhile I can open a separate issue to discuss/track that.

[offerm]

How about a test swap on every new order ?

On Wed, 14 Nov 2018 at 18:36 Daniel McNally [e-mail]
wrote:

Re-enable the pair after some time has elapsed? That's fine I think, but
it would be nice to have a way to check if swaps are working before we wind
up with another failed swap. That's why I'm thinking something like 1
satoshi test swaps might be useful, perhaps even as part of the initial
handshake. That's tangential to this issue but if it seems potentially
worthwhile I can open a separate issue to discuss/track that.

—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
https://github.com/ExchangeUnion/xud/issues/661#issuecomment-438728553,
or mute the thread
https://github.com/notifications/unsubscribe-auth/AJQ0cnZFl4-V3clO4BYkU1qE5oF5_hx6ks5uvEaLgaJpZM4YYa7s
.

[sangaman]

@offerm I'll respond and share my thoughts in a separate issue later today.

[kilrau]

I like the test swap idea (https://github.com/ExchangeUnion/xud/issues/676#issuecomment-439413320) since this catches tons of errors we don't even know about. Any other opinions? @moshababo @erkarl @michael1011 @sangaman

[kilrau]

Actually test-swaps don't eliminate the need for dealing with different kind of swap errors differently :/
It just makes swap errors less likely to appear on a real trade. So I'd keep this issue open to define swap error + behavior. We will use the defined behavior for test-swaps as well as real swaps. Agreed? Other opinions? @sangaman @offerm

[sangaman]

I moved this to alpha.6 because it depends on #671, but this should be a priority once #671 is merged.