fix(orderbook): prevent stuck replace order holds #1842
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This fixes a bug whereby an order could be placed on hold without that hold ever being removed. When replacing an order, we place the existing order on hold until the replacement order has finished matching, and only then do we remove the original order. However, the order was being placed on hold before the checks for sufficient balance to fill this order, and in case those checks failed the remainder of the `placeOrder` routine would be skipped including the part that removes the original order. In such a case, the original order would be put on hold indefinitely. Instead, we place the existing order on hold immediately before we begin matching and after any checks on the validity of the new order are passed. If the checks fail, then the new order is rejected and the order it was intended to replace remains in the order book without a hold, allowing it to be replaced or removed again by a future call. Fixes #1835.
ghost
approved these changes
Aug 27, 2020
|
Do you have a way to test this? Otherwise I have a node that regularly gets itself into such a situation and can run this branch there @raladev |
The way to test (and reproduce the issue on master) would be to place an order that replaces another order but exceeds your channel capacity, more than your L2 balance in other words. The replaced order would become permanently on hold. On this branch there should be no hold. |
|
Ok, but why would xud allow me to place an order exceeding my L2 balance in the first place? EDIT: got it
This PR should fix it. |
raladev
approved these changes
Aug 27, 2020
rsercano
pushed a commit
that referenced
this pull request
Sep 11, 2020
This fixes a bug whereby an order could be placed on hold without that hold ever being removed. When replacing an order, we place the existing order on hold until the replacement order has finished matching, and only then do we remove the original order. However, the order was being placed on hold before the checks for sufficient balance to fill this order, and in case those checks failed the remainder of the `placeOrder` routine would be skipped including the part that removes the original order. In such a case, the original order would be put on hold indefinitely. Instead, we place the existing order on hold immediately before we begin matching and after any checks on the validity of the new order are passed. If the checks fail, then the new order is rejected and the order it was intended to replace remains in the order book without a hold, allowing it to be replaced or removed again by a future call. Fixes #1835. added currency check to walletwithdraw
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This fixes a bug whereby an order could be placed on hold without that hold ever being removed.
When replacing an order, we place the existing order on hold until the replacement order has finished matching, and only then do we remove the original order. However, the order was being placed on hold before the checks for sufficient balance to fill this order, and in case those checks failed the remainder of the
placeOrderroutine would be skipped including the part that removes the original order. In such a case, the original order would be put on hold indefinitely.Instead, we place the existing order on hold immediately before we begin matching and after any checks on the validity of the new order are passed. If the checks fail, then the new order is rejected and the order it was intended to replace remains in the order book without a hold, allowing it to be replaced or removed again by a future call.
Fixes #1835.