Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Orders are still in order book after successful swap on recovery stage #1794

Closed
raladev opened this issue Aug 6, 2020 · 1 comment · Fixed by #1822
Closed

Orders are still in order book after successful swap on recovery stage #1794

raladev opened this issue Aug 6, 2020 · 1 comment · Fixed by #1822
Assignees
Labels
bug Something isn't working order book P2 mid priority swaps

Comments

@raladev
Copy link
Contributor

raladev commented Aug 6, 2020

Steps:

  1. Place 8 sell BTC/ETH orders as a maker
  2. Send 1 buy order as a maker that will be matched with all 8 orders (
    Screenshot from 2020-08-06 18-28-00
    )
  3. Wait first successful match in xud log
  4. docker rm -f simnet_lndbtc_1 for maker (after this, channel of taker that was connected to simnet node was closed with no reason)
  5. Wait first recovery attempt from maker -> all recovery attempts were failed because of disabled BTC node
  6. Return lndbtc node back
  7. Wait second recovery attempt -> success for several orders and fail for other
  8. Check listorders for maker and taker

Actual result:
Most of orders are still in order book but both users have them in tradehistory (successful orders).
Screenshot from 2020-08-06 17-40-20

returned_orders_xud_taker.log
returned_orders_xud_maker.log

Note:
For me it looks like only first order was removed because it was matched before lndbtc restart, but others was not removed because swap flow of them was finished on recovery stage.

@raladev raladev added bug Something isn't working order book P2 mid priority swaps labels Aug 6, 2020
@raladev raladev changed the title Orders are still in order book after successful/failed swaps and recovery Orders are still in order book after successful swap on recovery stage Aug 6, 2020
@sangaman
Copy link
Collaborator

sangaman commented Aug 6, 2020

Fixing this will require using the swap recovery module only for when xud crashes or is otherwise restarted while a swap is ongoing, and then having similar functionality within Swaps that keeps track of orders, swap successes, et.c..

sangaman added a commit that referenced this issue Aug 21, 2020
This monitors all swap client payments until their resolution without
putting deals into `SwapRecovery`. Previously, if a call to send payment
failed but the payment was still in pending status (as has been the case
with Connext), then we would fail the swap deal and monitor the payment
in `SwapRecovery`. This had several downsides, namely:

1. Since the deal is marked as having failed in the database, if xud
restarts while payment monitoring is ongoing, it won't resume monitoring
because it sees the swap as having failed in the database. We only
recover swaps that were "active" at the time xud shut down. See #1799.

2. When a deal is failed, the maker order it attempted to swap re-enters
the order book and is available to be matched again. However, since
the payment for the original deal is still pending, it may still go
through, meaning that the order can be "double filled" in such a case.
See #1794.

By monitoring all pending payments to their resolution, we ensure that
we don't fail deals that wind up completing.

Fixes #1799. Fixes #1794.
sangaman added a commit that referenced this issue Aug 25, 2020
This monitors all swap client payments until their resolution without
putting deals into `SwapRecovery`. Previously, if a call to send payment
failed but the payment was still in pending status (as has been the case
with Connext), then we would fail the swap deal and monitor the payment
in `SwapRecovery`. This had several downsides, namely:

1. Since the deal is marked as having failed in the database, if xud
restarts while payment monitoring is ongoing, it won't resume monitoring
because it sees the swap as having failed in the database. We only
recover swaps that were "active" at the time xud shut down. See #1799.

2. When a deal is failed, the maker order it attempted to swap re-enters
the order book and is available to be matched again. However, since
the payment for the original deal is still pending, it may still go
through, meaning that the order can be "double filled" in such a case.
See #1794.

By monitoring all pending payments to their resolution, we ensure that
we don't fail deals that wind up completing.

Fixes #1799. Fixes #1794.
sangaman added a commit that referenced this issue Aug 25, 2020
This monitors all swap client payments until their resolution without
putting deals into `SwapRecovery`. Previously, if a call to send payment
failed but the payment was still in pending status (as has been the case
with Connext), then we would fail the swap deal and monitor the payment
in `SwapRecovery`. This had several downsides, namely:

1. Since the deal is marked as having failed in the database, if xud
restarts while payment monitoring is ongoing, it won't resume monitoring
because it sees the swap as having failed in the database. We only
recover swaps that were "active" at the time xud shut down. See #1799.

2. When a deal is failed, the maker order it attempted to swap re-enters
the order book and is available to be matched again. However, since
the payment for the original deal is still pending, it may still go
through, meaning that the order can be "double filled" in such a case.
See #1794.

By monitoring all pending payments to their resolution, we ensure that
we don't fail deals that wind up completing.

Fixes #1799. Fixes #1794.
sangaman added a commit that referenced this issue Aug 25, 2020
This monitors all swap client payments until their resolution without
putting deals into `SwapRecovery`. Previously, if a call to send payment
failed but the payment was still in pending status (as has been the case
with Connext), then we would fail the swap deal and monitor the payment
in `SwapRecovery`. This had several downsides, namely:

1. Since the deal is marked as having failed in the database, if xud
restarts while payment monitoring is ongoing, it won't resume monitoring
because it sees the swap as having failed in the database. We only
recover swaps that were "active" at the time xud shut down. See #1799.

2. When a deal is failed, the maker order it attempted to swap re-enters
the order book and is available to be matched again. However, since
the payment for the original deal is still pending, it may still go
through, meaning that the order can be "double filled" in such a case.
See #1794.

By monitoring all pending payments to their resolution, we ensure that
we don't fail deals that wind up completing.

Fixes #1799. Fixes #1794.
sangaman added a commit that referenced this issue Aug 26, 2020
This monitors all swap client payments until their resolution without
putting deals into `SwapRecovery`. Previously, if a call to send payment
failed but the payment was still in pending status (as has been the case
with Connext), then we would fail the swap deal and monitor the payment
in `SwapRecovery`. This had several downsides, namely:

1. Since the deal is marked as having failed in the database, if xud
restarts while payment monitoring is ongoing, it won't resume monitoring
because it sees the swap as having failed in the database. We only
recover swaps that were "active" at the time xud shut down. See #1799.

2. When a deal is failed, the maker order it attempted to swap re-enters
the order book and is available to be matched again. However, since
the payment for the original deal is still pending, it may still go
through, meaning that the order can be "double filled" in such a case.
See #1794.

By monitoring all pending payments to their resolution, we ensure that
we don't fail deals that wind up completing.

This also fixes a case where the maker would not cancel its invoice when
an outgoing payment would fail and the very first `lookupPayment` call
would confirm that it had failed.

Fixes #1799. Fixes #1794. Fixes #1708.
sangaman added a commit that referenced this issue Aug 26, 2020
This monitors all swap client payments until their resolution without
putting deals into `SwapRecovery`. Previously, if a call to send payment
failed but the payment was still in pending status (as has been the case
with Connext), then we would fail the swap deal and monitor the payment
in `SwapRecovery`. This had several downsides, namely:

1. Since the deal is marked as having failed in the database, if xud
restarts while payment monitoring is ongoing, it won't resume monitoring
because it sees the swap as having failed in the database. We only
recover swaps that were "active" at the time xud shut down. See #1799.

2. When a deal is failed, the maker order it attempted to swap re-enters
the order book and is available to be matched again. However, since
the payment for the original deal is still pending, it may still go
through, meaning that the order can be "double filled" in such a case.
See #1794.

By monitoring all pending payments to their resolution, we ensure that
we don't fail deals that wind up completing.

This also fixes a case where the maker would not cancel its invoice when
an outgoing payment would fail and the very first `lookupPayment` call
would confirm that it had failed.

Fixes #1799. Fixes #1794. Fixes #1708.
sangaman added a commit that referenced this issue Aug 26, 2020
This monitors all swap client payments until their resolution without
putting deals into `SwapRecovery`. Previously, if a call to send payment
failed but the payment was still in pending status (as has been the case
with Connext), then we would fail the swap deal and monitor the payment
in `SwapRecovery`. This had several downsides, namely:

1. Since the deal is marked as having failed in the database, if xud
restarts while payment monitoring is ongoing, it won't resume monitoring
because it sees the swap as having failed in the database. We only
recover swaps that were "active" at the time xud shut down. See #1799.

2. When a deal is failed, the maker order it attempted to swap re-enters
the order book and is available to be matched again. However, since
the payment for the original deal is still pending, it may still go
through, meaning that the order can be "double filled" in such a case.
See #1794.

By monitoring all pending payments to their resolution, we ensure that
we don't fail deals that wind up completing.

This also fixes a case where the maker would not cancel its invoice when
an outgoing payment would fail and the very first `lookupPayment` call
would confirm that it had failed.

Fixes #1799. Fixes #1794. Fixes #1708.
sangaman added a commit that referenced this issue Aug 26, 2020
This monitors all swap client payments until their resolution without
putting deals into `SwapRecovery`. Previously, if a call to send payment
failed but the payment was still in pending status (as has been the case
with Connext), then we would fail the swap deal and monitor the payment
in `SwapRecovery`. This had several downsides, namely:

1. Since the deal is marked as having failed in the database, if xud
restarts while payment monitoring is ongoing, it won't resume monitoring
because it sees the swap as having failed in the database. We only
recover swaps that were "active" at the time xud shut down. See #1799.

2. When a deal is failed, the maker order it attempted to swap re-enters
the order book and is available to be matched again. However, since
the payment for the original deal is still pending, it may still go
through, meaning that the order can be "double filled" in such a case.
See #1794.

By monitoring all pending payments to their resolution, we ensure that
we don't fail deals that wind up completing.

This also fixes a case where the maker would not cancel its invoice when
an outgoing payment would fail and the very first `lookupPayment` call
would confirm that it had failed.

Fixes #1799. Fixes #1794. Fixes #1708.
sangaman added a commit that referenced this issue Aug 27, 2020
This monitors all swap client payments until their resolution without
putting deals into `SwapRecovery`. Previously, if a call to send payment
failed but the payment was still in pending status (as has been the case
with Connext), then we would fail the swap deal and monitor the payment
in `SwapRecovery`. This had several downsides, namely:

1. Since the deal is marked as having failed in the database, if xud
restarts while payment monitoring is ongoing, it won't resume monitoring
because it sees the swap as having failed in the database. We only
recover swaps that were "active" at the time xud shut down. See #1799.

2. When a deal is failed, the maker order it attempted to swap re-enters
the order book and is available to be matched again. However, since
the payment for the original deal is still pending, it may still go
through, meaning that the order can be "double filled" in such a case.
See #1794.

By monitoring all pending payments to their resolution, we ensure that
we don't fail deals that wind up completing.

This also fixes a case where the maker would not cancel its invoice when
an outgoing payment would fail and the very first `lookupPayment` call
would confirm that it had failed.

Fixes #1799. Fixes #1794. Fixes #1708.
sangaman added a commit that referenced this issue Aug 27, 2020
This monitors all swap client payments until their resolution without
putting deals into `SwapRecovery`. Previously, if a call to send payment
failed but the payment was still in pending status (as has been the case
with Connext), then we would fail the swap deal and monitor the payment
in `SwapRecovery`. This had several downsides, namely:

1. Since the deal is marked as having failed in the database, if xud
restarts while payment monitoring is ongoing, it won't resume monitoring
because it sees the swap as having failed in the database. We only
recover swaps that were "active" at the time xud shut down. See #1799.

2. When a deal is failed, the maker order it attempted to swap re-enters
the order book and is available to be matched again. However, since
the payment for the original deal is still pending, it may still go
through, meaning that the order can be "double filled" in such a case.
See #1794.

By monitoring all pending payments to their resolution, we ensure that
we don't fail deals that wind up completing.

This also fixes a case where the maker would not cancel its invoice when
an outgoing payment would fail and the very first `lookupPayment` call
would confirm that it had failed.

Fixes #1799. Fixes #1794. Fixes #1708.
raladev pushed a commit that referenced this issue Aug 28, 2020
* feat(swaps): monitor pending payments before fail

This monitors all swap client payments until their resolution without
putting deals into `SwapRecovery`. Previously, if a call to send payment
failed but the payment was still in pending status (as has been the case
with Connext), then we would fail the swap deal and monitor the payment
in `SwapRecovery`. This had several downsides, namely:

1. Since the deal is marked as having failed in the database, if xud
restarts while payment monitoring is ongoing, it won't resume monitoring
because it sees the swap as having failed in the database. We only
recover swaps that were "active" at the time xud shut down. See #1799.

2. When a deal is failed, the maker order it attempted to swap re-enters
the order book and is available to be matched again. However, since
the payment for the original deal is still pending, it may still go
through, meaning that the order can be "double filled" in such a case.
See #1794.

By monitoring all pending payments to their resolution, we ensure that
we don't fail deals that wind up completing.

This also fixes a case where the maker would not cancel its invoice when
an outgoing payment would fail and the very first `lookupPayment` call
would confirm that it had failed.

Fixes #1799. Fixes #1794. Fixes #1708.

* test(sim): handle no force close for taker

This modifies the simulation tests to account for the maker properly
canceling the invoice for its incoming payment once its outgoing payment
htlc expires. Rather than expecting the channel with the taker to maker
payment to expire and force close, it expects the invoice to be canceled
and for the channel to remain open. It ensures that the balances on the
channel do not change.
rsercano pushed a commit that referenced this issue Sep 11, 2020
* feat(swaps): monitor pending payments before fail

This monitors all swap client payments until their resolution without
putting deals into `SwapRecovery`. Previously, if a call to send payment
failed but the payment was still in pending status (as has been the case
with Connext), then we would fail the swap deal and monitor the payment
in `SwapRecovery`. This had several downsides, namely:

1. Since the deal is marked as having failed in the database, if xud
restarts while payment monitoring is ongoing, it won't resume monitoring
because it sees the swap as having failed in the database. We only
recover swaps that were "active" at the time xud shut down. See #1799.

2. When a deal is failed, the maker order it attempted to swap re-enters
the order book and is available to be matched again. However, since
the payment for the original deal is still pending, it may still go
through, meaning that the order can be "double filled" in such a case.
See #1794.

By monitoring all pending payments to their resolution, we ensure that
we don't fail deals that wind up completing.

This also fixes a case where the maker would not cancel its invoice when
an outgoing payment would fail and the very first `lookupPayment` call
would confirm that it had failed.

Fixes #1799. Fixes #1794. Fixes #1708.

* test(sim): handle no force close for taker

This modifies the simulation tests to account for the maker properly
canceling the invoice for its incoming payment once its outgoing payment
htlc expires. Rather than expecting the channel with the taker to maker
payment to expire and force close, it expects the invoice to be canceled
and for the channel to remain open. It ensures that the balances on the
channel do not change.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working order book P2 mid priority swaps
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants