duplicate swap request (unneeded retries)

[offerm]

I setup my xud to connect only to test1 server (where swap isn't properly configured).

This is what I did:

admins-MacBook-Pro-2:xud-simnet admin$ xud-simnet-start
starting all processes
btcd started
ltcd started
lndbtc started
lndltc started
xud started
sync lndbtc to chain
.
lndbtc synced to chain
sync lndltc to chain
.
lndltc synced to chain
Ready!
admins-MacBook-Pro-2:xud-simnet admin$ xucli getinfo
{
  "version": "1.0.0-alpha.2",
  "nodePubKey": "02db8d5f43940fa1632ad2e2c0761b608e54fcca9c7502f961d17e290e66ad99a6",
  "urisList": [],
  "numPeers": 2,
  "numPairs": 1,
  "orders": {
    "peer": 10,
    "own": 0
  },
  "lndbtc": {
    "error": "",
    "channels": {
      "active": 1,
      "inactive": 0,
      "pending": 0
    },
    "chainsList": [
      "bitcoin"
    ],
    "blockheight": 29394,
    "urisList": [],
    "version": "0.5.0-beta commit=v0.4.2-beta-1247-ge22ae9985b696924d5934004c269c930d3ccba43",
    "alias": "BTC@admins-MacBook-Pro-2.local"
  },
  "lndltc": {
    "error": "",
    "channels": {
      "active": 1,
      "inactive": 0,
      "pending": 0
    },
    "chainsList": [
      "litecoin"
    ],
    "blockheight": 43941,
    "urisList": [],
    "version": "0.5.0-beta commit=v0.4.2-beta-1247-ge22ae9985b696924d5934004c269c930d3ccba43",
    "alias": "LTC@admins-MacBook-Pro-2.local"
  }
}
admins-MacBook-Pro-2:xud-simnet admin$ xucli buy 0.004 LTC/BTC market
{
  "internalMatchesList": [],
  "swapResultsList": []
}
admins-MacBook-Pro-2:xud-simnet admin$ xucli getinfo
{
  "version": "1.0.0-alpha.2",
  "nodePubKey": "02db8d5f43940fa1632ad2e2c0761b608e54fcca9c7502f961d17e290e66ad99a6",
  "urisList": [],
  "numPeers": 2,
  "numPairs": 1,
  "orders": {
    "peer": 5,
    "own": 0
  },
  "lndbtc": {
    "error": "",
    "channels": {
      "active": 1,
      "inactive": 0,
      "pending": 0
    },
    "chainsList": [
      "bitcoin"
    ],
    "blockheight": 29394,
    "urisList": [],
    "version": "0.5.0-beta commit=v0.4.2-beta-1247-ge22ae9985b696924d5934004c269c930d3ccba43",
    "alias": "BTC@admins-MacBook-Pro-2.local"
  },
  "lndltc": {
    "error": "",
    "channels": {
      "active": 1,
      "inactive": 0,
      "pending": 0
    },
    "chainsList": [
      "litecoin"
    ],
    "blockheight": 43941,
    "urisList": [],
    "version": "0.5.0-beta commit=v0.4.2-beta-1247-ge22ae9985b696924d5934004c269c930d3ccba43",
    "alias": "LTC@admins-MacBook-Pro-2.local"
  }
}

Checking the log you can see that some orders executed multiple times.

xud.log

[offerm]

buy 0.004 LTC/BTC market executed like this first match: 0.001@1.10 0.0015@1.11 0.0015@1.12 failed! second match: 0.0005@1.12 0.0025@1.13 0.001@1.14 failed! 3rd match: 0.002 @ 1.14 failed! no more matches

when 0.001@1.10 (first order) failed, we removed that order from the order book. I assume this was on purpose since that peer order can't be traded and is just a noise. Buy if this is the case:

• why we are not removing the complete order at 1.12 when the trade 0.0015@1.12 failed
• why we are not removing all peer orders when the first trade failed?

or maybe it was not on purpose? :-)

[sangaman]

It looks like the root cause here is that if we match part of a peer order, try to swap with it, and then fail, we aren't removing the remainder of the peer order (only the part we tried to swap). Then when we repeat the matching logic for the failed amount, we match again with the same error. Does that sound right to you?

I think the solution is to address your first bullet point and remove the entirety of an order whenever we fail to swap any part of it.

For your second bullet point, are you asking why we don't remove all orders for a given peer if we fail to swap any order from that peer? Right now the only way that would happen is if we fail swaps enough times that we ban them. It might make sense to disconnect from a peer (which would in turn drop all their orders and prevent new ones from coming in) if a swap fails after it is accepted due to lack of a route or issues with lnd, but we wouldn't want to do this if a swap fails because an order we want is no longer available. If they keep reconnecting and failing swaps, they'd eventually be banned.

[kilrau]

• why we are not removing the complete order at 1.12 when the trade 0.0015@1.12 failed

Agree with you both, let's remove the entire order and make this the "to be implemented" of this issue.

• why we are not removing all peer orders when the first trade failed?

Sepaeate topic. Agree with @sangaman , don't understand why we would do that. It could happen that another 3rd peer is outrunning us in 99% of the time, but one time we'll succeed to fill an order. It won't do us much harm to the extend we spend time trying to fill orders of this peer and not others which are more likely to succeed. So definitely not just remove all orders of this peer.

Failed swaps and all serious stuff is already handled by reputation events and banning as @sangaman explained. Question now is do we add rejected dealRequests as negative reputation event or do we treat it as something separate. Rejecting dealReuqests mostly isn't negative behavior, we might be simply on a slower network link than someone else. Nodes with a lot of liquidity will attract a lot of trades and collect a lot of negative reputation events for rejected trades. For now I'm leaning towards something separate and keep reputation events for real negative behavior. Thoughts?

[offerm]

@kilrau the discussion is not about rejected dealRequest. It 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.

[kilrau]

Good point @offerm , we & I should think about this, since it needs to be defined more clearly, I separated it out in a new issue: https://github.com/ExchangeUnion/xud/issues/661

Let's keep this one here plain and simple about "removing the entire order" in case of a generic swap failure.

[kilrau]

As per our call, closing this in favor of #661 @offerm