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

Sanity checks before adding to orderbook #675

Closed
ghost opened this issue Nov 15, 2018 · 5 comments
Closed

Sanity checks before adding to orderbook #675

ghost opened this issue Nov 15, 2018 · 5 comments
Labels
enhancement New feature or request order book P1 top priority swaps

Comments

@ghost
Copy link

ghost commented Nov 15, 2018

Started as a result of #240 discussion.

To the extent that this is a valuable safeguard, I think it makes sense to employ it on a broader scope and not just via the cli. Maybe xud should make these sanity checks internally - it's not in our interest to advertise orders that can't be swapped. For example, we shouldn't broadcast orders if we don't have channels open with some minimum capacity. We already do some checks on initialization like verifying lnd is in sync with the chain. I believe checking our incoming capacity would be a bit more complicated - calling listchannels and then iterating over each one - but still doable. Getting this done right wouldn't be simple but it seems worthwhile, and once that's in place we could return a grpc error for the placeorder calls indicating that the order can't be swapped without needing to make an extra call like channelbalance.

High level specs

  • before placeOrder add orderSanityChecks

orderSanityChecks should verify that:

  • inbound capacity to execute the order exists
  • outbound capacity to execute the order exists

if either of the above checks fails - ERROR!

@ghost ghost added order book enhancement New feature or request labels Nov 15, 2018
@sangaman sangaman added the swaps label Nov 15, 2018
@sangaman
Copy link
Collaborator

We could also do this on peer orders potentially, so that we check if capacity exists before we add it to our orderbook. In both cases, I think it makes sense to save the capacity in memory (cached in other words) , and only recalculate it after it gets old, maybe 10 minutes? I'm thinking that doing this every time we place an order will introduce too much of a delay - we really want the order placement and settlement to be as fast as possible.

In fact, maybe we shouldn't tie these checks into the placeOrder flow per se, but rather calculate our capacities on a scheduled basis. Every 5-10 minutes we update the balances in memory, and on new orders we just compare the amount to the balance we've stored in memory. That should add almost no delay to the routine.

Also one extra detail, the lightning spec recommends that 1% of the balance of each channel be held in reserve on each side. So we probably want to make sure that our channel balance is at least 101% of the order amount.

@kilrau
Copy link
Contributor

kilrau commented Nov 16, 2018

Cache channel balances in memory and then check this value on placeOrder sounds good! Since this is an independent job (not affecting the placeOrder performance) I think we can target 1 min updates (unless there is a reason not to @sangaman @erkarl ), looking a bit into the future where channel balances change very fast when trading volume goes in one direction.

Since we want to support AMPs in future, let's prepare aggregating balances of multiple channels. Also wouldn't distinguish between peers for now, meaning let the check succeed as long as one channel to one peer with sufficient capacity exists.
<coin>_channel_in (remote balance of all channels aggregated)
<coin>_channel_out (local balance of all channels aggregated)

Continuation: #680

@ghost
Copy link
Author

ghost commented Mar 4, 2019

We could also do this on peer orders potentially, so that we check if capacity exists before we add it to our orderbook.

Good point.

In fact, maybe we shouldn't tie these checks into the placeOrder flow per se, but rather calculate our capacities on a scheduled basis.

Agreed.

I think we can target 1 min updates...

I'll make the time configurable.

@sangaman
Copy link
Collaborator

sangaman commented Mar 5, 2019

Are you thinking of putting this scheduled capacity check on each lnd client instance? Eventually we'll want similar logic for Raiden so try to make the approach generic enough that it can be reused easily.

@ghost
Copy link
Author

ghost commented Mar 5, 2019

Are you thinking of putting this scheduled capacity check on each lnd client instance? Eventually we'll want similar logic for Raiden so try to make the approach generic enough that it can be reused easily.

Thanks for pointing this out - definitely want to easily support Raiden sanity checks as well.

This issue was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request order book P1 top priority swaps
Projects
None yet
Development

No branches or pull requests

2 participants