bmoscon / cryptofeed

Cryptocurrency Exchange Websocket Data Feed Handler
Other
2.21k stars 682 forks source link

incorrect OrderBook for binance #604

Open tristan-murfitt-elw opened 3 years ago

tristan-murfitt-elw commented 3 years ago

Describe the bug Over time, internal binance orderbook can be incorrect because we don't regularly poll the snapshot. In comparison, coinbase WS feed automatically supplies regular snaps (and doesn't truncate the book!)

Tagging @jinusean because their recent PR https://github.com/bmoscon/cryptofeed/pull/580 touched a lot of this file and I'm sure they can help confirm!

More detail When fetching snapshot we truncate depth to 1000 (instead of the max of 5k, self.valid_depths[-1]): https://github.com/bmoscon/cryptofeed/blob/b367c0fdfcc5e4f0be6068c5d51a421cf4b4782b/cryptofeed/exchanges/binance.py#L259

A new snapshot only gets fetched when a message is dropped (missing seq num): https://github.com/bmoscon/cryptofeed/blob/b367c0fdfcc5e4f0be6068c5d51a421cf4b4782b/cryptofeed/exchanges/binance.py#L253

I think this means, after running for a while, the internal OB could be outdated with the real world. The initial 1000 levels are recorded, and we store deltas at all(?) levels, but we will never know about unchanged levels outside of those initial 1k levels. This is a problem if the market moves in one direction over time, and the real book has an unchanged (old) limit order but our internal OB doesn't.

Do you both agree? If so, @bmoscon do you think any other exchanges suffer from this problem?

Solution

Cryptofeed Version latest

tristan-murfitt-elw commented 3 years ago

Binance websocket docs "How to manage a local order book correctly"

Surprises me that they don't mention this potential issue (and tell you to regularly refresh snapshot)

bmoscon commented 3 years ago

@tristan-murfitt-elw - their docs indicate that we should use 1000 as the depth, are you sure that the websocket is providing book updates for all 5000 levels?

tristan-murfitt-elw commented 3 years ago

@bmoscon there are certainly more than 1000 levels in the binance order books so we should use the maximum available (as is done for other exchanges).

To test this, I set depth to 5 and watched the internal OB get larger and larger to 2000+ levels as the updates came in.

That being said, the main concern here is the stale book levels from not regularly refreshing the snapshot. Snapshot is 1k, book is definitely deeper than that, so if the market moves in one direction there will be inconsistency with internal OB vs the exchange

jinusean commented 3 years ago

I ran two scripts for about an hour that stored data from:

  1. Cryptofeed's callback (1000ms).
  2. Polling the RESTapi for the book snapshot (500ms).

There were about 5000+ snapshots for each data source, and 362 matching update_ids. All of the matching IDs had matching snapshots, without a single non-matching row. Perhaps there is a possibility for issues if we were using the full trade streams, but this doesn't seem to be the case in aggTrade consumption.

In regards to the ever expanding internal OB, I don't think this is much of an issue in terms of data integrity since the feed class will prune the extra levels before calling the data back to the user.

Implementing a validity check every 'x' intervals would make the system more robust, but it's not something I want to invest in right now without any empirical data.

tristan-murfitt-elw commented 3 years ago

Just did an experiment to get some empirical data and prove that this is a problem:

Experiment code and explanation here: https://github.com/bmoscon/cryptofeed/pull/606

Steps:

  1. Place some limit sells in a binance book fairly far out from mid (low volume OB is easier to test)
  2. Set the cryptofeed snapshot to only take first 20 levels, start cryptofeed. (I chose to turn off the max depth param because it's slow, non-default, and not necessary if you can just truncate your book after)
  3. Wait for market to move up (towards your limit sells)
  4. Observe that these orders are not in the cryptofeed internal OB

This scenario will happen even if you have 5000 levels in the book, because any prices outside of those first 5k may not be updated (so no deltas pushed from binance over the WS). This wouldn't happen if Binance gave us the full L2 order book. It will happen more often the smaller your initial snap is (hence why 5000 default is better than 1000).

Potential solution:

Tiergarten commented 3 years ago

An ideal solution would utilise the binance updateIds (if this can be performant): https://github.com/binance/binance-spot-api-docs/blob/master/web-socket-streams.md#diff-depth-stream

jinusean commented 3 years ago

@tristan-murfitt-elw sorry where in the code is the book truncated?

I'm currently running 3 scripts:

  1. Polling rest api
  2. Cryptofeed stream
  3. Code from official binance-exchange github

So far all three sources are in sync, I'll keep it running for another day or so.

I also came across this forum question. The UI should not be considered the absolute truth.

@Tiergarten if we can prove that the snapshot data != cached order book data, then we can just replace the cache with the snapshot without bothering to do an update_id check.

tristan-murfitt-elw commented 3 years ago

@jinusean I added max depth of 20 to make it easy for me to simulate the orders on the live exchange and observe the cryptofeed internal OB to be inconsistent. The same problem can occur with a depth of 5k, albeit less frequently (because a price move to that level would take longer to happen, allowing more time for each level to be updated and receive a delta over the WS).

The binance exchange example is also therefore wrong, and suffers the same problem!!

I know the UI is the absolute truth in this example because I placed the orders myself and confirm I didn't get any fills. But these orders were not found in the cryptofeed internal book. So this can absolutely happen (all because Binance don't provide the full L2 book snaps)

we can just replace the cache with the snapshot without bothering to do an update_id check

this is what I propose too, but we should be careful with the timings: receiving updates while polling for the new snapshot

tristan-murfitt-elw commented 3 years ago

@jinusean what cryptofeed snapshot depth are you using for this experiment? the default is now set 5000 since this commit a few days ago https://github.com/bmoscon/cryptofeed/commit/1ce8ab42a253cad17fe1533a1a3c16c5b7518e69

So far all three sources are in sync, I'll keep it running for another day or so.

I wouldn't expect to see a sync error until the price moves past those first N levels.

eg. If I place a BTC sell order right now for $120,936.10 and nobody else updates this price level, when the price moves up to that range, cryptofeed is not going to have knowledge of the order (because initial snap was only 5k levels, and no update delta was received for that level)

jinusean commented 3 years ago

Ok let's just see if a discrepancy between the orderbook and snapshot ever occurs first. There's a chance the snapshots too are invalid, then we'd have to raise the issue to the Binance devs.

And receiving updates while polling for the snapshot shouldn't be an issue since the current implementation already uses a buffer to handle such situations. I just want to make sure the snapshot's data is actually different from the websocket data first.

jinusean commented 3 years ago

@jinusean what cryptofeed snapshot depth are you using for this experiment? the default is now set 5000 since this commit a few days ago 1ce8ab4

It's at 5000.

Oddly the official Binance script truncates the book if it ever goes beyond 1000 levels.

https://github.com/binance-exchange/binance-toolbox-python/blob/aa8cbde68cf7118cbbce7ff1a3d24ab18e0ba9bf/spot/manage_local_order_book.py#L72-L74

tristan-murfitt-elw commented 3 years ago

Ok let's just see if a discrepancy between the orderbook and snapshot ever occurs first.

See my example here https://github.com/bmoscon/cryptofeed/pull/606. If you perform this same test while running your experiment you'll see a discrepancy

Oddly the official Binance script truncates the book if it ever goes beyond 1000 levels.

This is quite common, and the same as what cryptofeed does if you set the max_depth. Most people are only interested in the top levels of the book. However, without regularly refetching the snapshot, there could be the inconsistency I found (as above). Might need to reach out to binance devs so they can at least update their docs/example to notify end users

jinusean commented 3 years ago

I can see the discrepancy in the UI but that doesn't mean there will be one between the snapshot and OB(?) Surely the difference will be caught if the script is run for 24hours.

tristan-murfitt-elw commented 3 years ago

discrepancy is between the UI (and real world!) vs the cryptofeed orderbook.

if you fetch a snapshot from binance again when this happens, the new snapshot is also different to the cryptofeed internal one! (I didn't push this code but it's easy to replicate)

jinusean commented 3 years ago

Can you try replicating the issue with this branch https://github.com/jinusean/cryptofeed/tree/binance-snapshot-depth

You can set the max_depth to whatever you want since the snapshots will always fetch with max_depth=5000 so the internal ob will have the max levels. And all the callback data will only be a pruned copy.

hk-yuan commented 3 years ago

Hi @jinusean , correct me if I'm wrong but the only difference I could see between that branch and @tristan-murfitt-elw 's test branch is this self.max_depth. By setting max_depth as max(self.valid_depths) in your branch it makes the experiment more difficult to assess (who knows whether a price level >5k from mid won't get updated after the market also moves in its direction even exists when you run your script ... (!) ); the issue is more obvious with fewer depths which is why I believe @tristan-murfitt-elw forced it down to 20 and placed his own orders in on the actual exchange. I hope an image might illustrate the issue better:

Screenshot 2021-08-17 at 09 39 29

Dotted box shows snapshot at max_depth (it's shown to be 4 here but can be 5k - doesn't really matter unless it's truly the full orderbook). Let's also assume this is the actual snapshot depth rather than pruned depth. Over time the market can move, in this case we're looking at ASK so let's say it moves up - our bottom levels get dropped and max_depth should encompass levels not present in the first snapshot.

We receive deltas which update the values (I've shown a "change from T0" column just for illustration here). If a particular level, which was outside the original snapshot, receives a delta - that's fine, it will be captured at T1. If a particular level doesn't receive a delta then will we ever see it until another snapshot?

The value of fetch max_depth isn't a problem in of itself, it's the fact that we don't take more regular snapshots to fill the gaps.

jinusean commented 3 years ago

@hk-yuan thank you for clarifying. I was under the impression there were no price levels beyond > 5000 since the assets I tested had no more than 1500 levels.

In that case, we can store a boolean whenever the order book depth hits 5000 (whether it's from the original snapshot or via deltas). And then we can fetch another snapshot once the size drops below a certain level. For added security we can do a timed refetch (every hour?).

@tristan-murfitt-elw what do you think?

tristan-murfitt-elw commented 3 years ago

@hk-yuan much better explanation than mine, thanks!

@jinusean in practice I expect a lot of books to have more than 5k levels (I think coinbase BTC markets can be close to 20k levels per side!)

The main problems are:

Triggering fetch of new snapshot

Processing the new snapshot

  1. Send REST GET request for new snap
  2. While it's in flight, continue sending updates through to internal OB and callbacks
  3. While it's in flight, also buffer and collect the updates
  4. Snapshot response comes back, apply the buffered updates to it. Then loop over every bid/ask level and check if it's in our internal orderbook. If it's in the book it should always match the size exactly (otherwise something went very wrong). Extra levels in our internal book are normal and expected and we want to keep these. Extras in the new snapshot are these stale prices we've been discussing, and these should just be added into the book.
  5. Simulate book "updates" for each of these levels that should be added to the internal OB.
jinusean commented 3 years ago
tristan-murfitt-elw commented 3 years ago

If we set the snapshot level == max_depth, we will never get an update for the (max_depth + 1) level

This isn't the case. If you get 5000 snapshot from Binance, their WS feed will still give you every delta that happens. If I place a BTC sell order at $1,000,000 it will still get an update through the feed.

It's bad practice to set the max_depth field in cryptofeed because it actually diffs the new vs old OB every snapshot, truncates to your max_depth, and then simulates all the necessary updates. Doing this per update is very slow and (from experience) is not a good idea

Can't we just replace our internal OB with the snapshot once the snapshot's update_id is greater than the obs?

Problems with this are:

  1. Our internal OB has extra useful data in it (from WS updates that aren't in the most recent snap)
  2. We still need to simulate updates for any "stale" prices in the new snap which should've been in the internal book (for any downstream subscribers of the book deltas)
jinusean commented 3 years ago

If we set the snapshot level == max_depth, we will never get an update for the (max_depth + 1) level

This isn't the case. If you get 5000 snapshot from Binance, their WS feed will still give you every delta that happens. If I place a BTC sell order at $1,000,000 it will still get an update through the feed.

I meant if we fetch a snapshot with limit=1000 and there are over 1000 levels, we'd never see that level in the deltas since it has already been applied prior to the subscription. Hence we should fetch a snapshot that is higher than the user defined max_depth. Ideally just fetch the maximum level allowed to minimize api calls.

It's bad practice to set the max_depth field in cryptofeed because it actually diffs the new vs old OB every snapshot, truncates to your max_depth, and then simulates all the necessary updates. Doing this per update is very slow and (from experience) is not a good idea

Right, the max_depth should be a soft limit for the user. The internal ob will/should maintain the max 5000 levels.

Can't we just replace our internal OB with the snapshot once the snapshot's update_id is greater than the obs?

Problems with this are:

  1. Our internal OB has extra useful data in it (from WS updates that aren't in the most recent snap)
  2. We still need to simulate updates for any "stale" prices in the new snap which should've been in the internal book (for any downstream subscribers of the book deltas)

Ok gotcha.

One last thing is, if we fetch a snapshot with limit=x, and we get orders sizes that are < x, is it safe to assume that there are/will be no missing orders? Running my test the other day for 24 hours, the orderbook was 100% in sync with new snapshots, albeit the orders size never went beyond 1500.

bmoscon commented 3 years ago

I did change the code to use the maximum book size for the snapshots. Shouldnt this largely correct this issue?

villekuosmanen commented 3 years ago

Based on my understanding the problem still exists in 5000 book size, it's just less likely to occur.

I can start looking into implementing the fix suggested by @tristan-murfitt-elw. Feel free to assign this issue to me. I'm new to working on the upstream branch of cryptofeed, but I've done some work on a fork during the past few weeks.

peedrr commented 3 years ago
  • Periodically fetch a new snapshot from the API (eg. every 30s)

@villekuosmanen - is this the solution you're proposing to implement?

Can I suggest that, rather than a fixed period of time, we instead refresh on a count of delta messages received? By their nature, I am guessing that low volume pairs will not suffer from this problem as much as high volume pairs, therefore do not require such frequent refreshes.

If we request snapshots all at the same time, I'm concerned it will exacerbate another open issue whereby users who subscribe to large numbers of symbols are not able to resubscribe cleanly when their connections get reset at the 24 hour mark, because of the sudden surge of HTTP requests.

@tristan-murfitt-elw - thoughts on this?

villekuosmanen commented 3 years ago

@peedrr doing it based on numbers of messages could work. You could use max orderbook depth to set the max number of delta messages seen before refreshing. If we set the ratio of max delta messages seen before a refresh to 0.5, at the default parameters (book depth of 5000, depth_interval of 100ms, and assuming the book changes at every step) there would be ~ 4 minutes 10 seconds between snapshots. Although changing these parameters could cause strange things to happen, such as depth of 20 and depth_interval of 100ms which would refresh every two seconds which feels excessive.

In any case the exact details on how (and how frequently) a new snapshot is loaded can be worked out later.

tristan-murfitt-elw commented 3 years ago

@peedrr low volume pairs probably suffer with this problem more because books levels are updating less often (fewer limit orders being amended) so there's a higher risk of prices outside the initial 5k levels not triggering WS deltas. edit: with the exception of books which are so shallow Binance can actually return the full book without truncating!

Agree performance & rate limits are a concern.

As contributors to the library, we have a responsibility to ensure the data is correct. The longer we wait between snapshots, the more incorrect data is in the internal OB (starting at the outer layers).

nirvana-msu commented 3 years ago

max_depth parameter's behavior is very misleading. Let's say I set it to something like 10. It is quite likely that, even on the second update, I'll receive less than 10 levels in my callback. Specifically, what happens is:

1) Following first WS update, a snapshot is being fetched for exactly max_depth levels 2) Another WS update comes and it says a level (close to the top of the order book) is removed. 3) Update is applied to the snapshot, removing the level. 4) Order book with less than max_depth levels is pushed to the callback.

The way it is implemented now, max_depth is essentially unusable. I have to avoid setting it, and trim the levels within my callback.

@bmoscon a much more intuitive behavior would be if max_depth only defined the number of levels pushed to the user in the callback - but internally the library should do whatever is needed to maintain an accurate state. That means fetching snapshot for a lot more levels than max_depth, and re-fetching whenever price moves close enough to a level where the true order book state is not known (was not fetched).

bmoscon commented 3 years ago

@nirvana-msu not sure what you are talking about. Are you claiming that when you set max depth to 10 you're getting more than 10 levels in the orderbook? All max_depth does is prevent the orderbook from showing you more than that number of levels, you can get less than that many levels

nirvana-msu commented 3 years ago

@bmoscon no I'm saying the opposite - when I set max_depth to, say, 10 levels, very often I would receive fewer than 10 levels in my callback.

Given that the true order book has many thousand levels, when I set max_depth to 10, I expect to receive exactly 10 top levels, always (unless there really are no 10 levels in the true order book). But I receive fewer than 10.

bmoscon commented 3 years ago

@nirvana-msu please provide sample code or other information so I can reproduce this (exchange, trading pairs, etc). Also provide the version number you are on.

nirvana-msu commented 3 years ago

Sure, let me put it together. It's on the latest version, 2.0.0. Right now I've experienced it for Binance spot, but likely same would apply to other exhanges. I actually described exactly what happens, step-by-step, in my comment above. The main issue is that the snapshot is being fetched for just 10 levels (max_depth). When a subsequent WS update removes one of those levels, I now have just 9 levels - it simply doesn't know anything about the levels beyond 10th because it never fetched them in the snapshot. I'll attach some code in a bit.

bmoscon commented 3 years ago

Oh I see, because of the initial orderbook snapshot that is pulled from the REST api. It seems the fix would be to go one interval higher than what is requested (i.e. if you ask for 10, pull down 25 to start with) and then rely on the refresh code that is in the PR and will likely soon be merged: https://github.com/bmoscon/cryptofeed/pull/606

nirvana-msu commented 3 years ago

There you go:

import logging

from cryptofeed import FeedHandler
from cryptofeed.defines import BINANCE, L2_BOOK
from cryptofeed.exchanges import Binance
from cryptofeed.types import OrderBook

logger = logging.getLogger(__name__)

DEPTH = 10

async def l2_book(ob: OrderBook, receipt_timestamp: float):
    book = ob.book.to_dict()

    if len(book['bid']) < DEPTH:
        logger.warning(f"Received {len(book['bid'])} bid levels for {ob.symbol}!")
    if len(book['ask']) < DEPTH:
        logger.warning(f"Received {len(book['ask'])} ask levels for {ob.symbol}!")

fh = FeedHandler()
fh.add_feed(BINANCE, symbols=Binance().info()['symbols'], channels=[L2_BOOK], callbacks={L2_BOOK: l2_book},
            max_depth=DEPTH, depth_interval='1000ms')

fh.run()

This is running on latest version 2.0.0, subscribing to all Binance spot symbols and setting max_depth to 10.

Sample output:

Received 7 bid levels for GALA-BNB!
Received 9 ask levels for IDEX-USDT!
Received 9 ask levels for SOL-AUD!
Received 9 ask levels for SOL-AUD!
Received 9 ask levels for SOL-AUD!
Received 9 ask levels for SOL-AUD!
Received 8 ask levels for BTC-USDP!
Received 9 bid levels for POLY-BUSD!
Received 9 ask levels for DYDX-BTC!
Received 9 bid levels for BTC-USDP!
Received 9 ask levels for POLY-USDT!
...

Also, even if an update has 10 levels (or less) we can never be sure that there aren't any levels missing. If the level (that was outside the first snapshot) was never updated, then it would simply be missed, unlike e.g. a level deeper that was updated via WS.

nirvana-msu commented 3 years ago

Oh I see, because of the initial orderbook snapshot that is pulled from the REST api. It seems the fix would be to go one interval higher than what is requested (i.e. if you ask for 10, pull down 25 to start with) and then rely on the refresh code that is in the PR and will likely soon be merged: #606

Yes, you need to pull more than requested max_depth. Probably, a lot more. Essentially you need to have a large buffer - and when the price starts to move close to the boundary where the true orderbook state was not snapshotted, you need to re-take snapshot. The timing and size for the snapshots is a rather delicate balance between performance and rate limits. You want snapshots to be large so you could do them less frequently (and avoid rate limits), and you also need to remember those bounds of the previous snapshot and take a new snapshot not just on a timer, but when the price moves close enough to a boundary for that to become a problem soon.

You can also take it a step further and do not push a callback to the user if you believe you do not enough levels fetched in the last snapshot - and instead wait for the new snapshot first. That way you could guarantee data integrity, at the expense of potential delays. And to avoid those delays the code would need to be smart enough to know when to request a new snapshot in advance (but it can still happen on a large price gap move).

villekuosmanen commented 3 years ago

@nirvana-msu The problem that you're mentioning is the same as here, though it seems like when you set max_depth to a certain depth (e.g. 10) you always expect to get that amount of levels in your callback.

The way the Binance API is implemented by the exchanges, this is not easily possible. Unlike many other exchanges (e.g. Kraken, FTX) Binance does not publish "artificial" deltas to maintain order book depth. For example, if you ask a book with depth=10 and the head is filled, your book will only have 9 orders. In FTX for instance, they would publish an artificial delta for the previously 11th level to keep each consumer's order book depth at re requested level (i.e. 10 in this case).

As @bmoscon said the obvious way to solve it will be to ask for more orders than you need and truncate the order book yourselves to a level you need. My PR for periodically re-fetching new snapshots will definitely help, as long as we're fetching the snapshot often enough the risk of the order book going out of date should be minimal. Of course, it's very difficult to determine how often is often enough, @tristan-murfitt-elw probably has a better idea about it than I do...

You can also take it a step further and do not push a callback to the user if you believe you do not enough levels fetched in the last snapshot - and instead wait for the new snapshot first.

This is probably not a good idea due to latency issues, in my opinion.

nirvana-msu commented 3 years ago

Of course, it's very difficult to determine how often is often enough

@villekuosmanen that's what I was trying to explain though - when to refresh shouldn't be based on time passed, it should be based on how close the current price is to the lower/upper bounds (also accounting for the desired max_depth) of the last-fetched snapshot. I have explained that in more detail in my comments in https://github.com/bmoscon/cryptofeed/pull/606.

This is probably not a good idea due to latency issues, in my opinion.

I would say that should ideally be up to a user to decide, Many would not care about small occasional latency, but would care about data integrity (think about capturing market data for research purpose as oposed to live trading). Ideally there should be a flag for the user to tell which one he prefers (integrity vs timeliness).

bmoscon commented 3 years ago

the easiest thing to do would be to take a larger snapshot than needed and refresh it periodically. I used to limit the snapshot size because truncating the book when max depth was set was very time consuming, so smaller snapshots limited that. This is no longer the case, so there isnt really a good reason to not just take 1000 or 5000 levels when starting.

villekuosmanen commented 3 years ago

I feel like tracking price movements and refreshing snapshots based on that adds a fair bit of complexity. @bmoscon are you suggesting that we should always fetch a 1000+ depth snapshot in the background, even when the user requests a book with max_depth of 10, and not truncate it in the book's code? That would solve the issue of having to poll the order book REST API every 10 seconds or so.

bmoscon commented 3 years ago

@villekuosmanen yes, I'm saying always get 1000 unless they ask for more than that (optionally could do something where we ask for 10x more than they ask for so 1-10 levels -> 100 from snap, 10-50 -> 500 snap, 50+ 1000, more than 1000 -> 5000). We'd still need to refresh periodically

nirvana-msu commented 3 years ago

I feel like tracking price movements and refreshing snapshots based on that adds a fair bit of complexity.

@villekuosmanen it might actually even be easier than the timer logic, no? We only need to remember two variables (per symbol) - the "lower-most bid" and the "upper-most ask" prices of the most recently captured snapshot . They define the range of prices where we are 100% confident our re-constructed book is correct,

The logic would then be as follows. After applying each WS update we check:

  1. If ask price at level max_depth (i.e. self._l2_book[std_pair].book.asks.index(self.max_depth-1)) is close to "upper-most ask" OR If bid price at level max_depth (i.e. self._l2_book[std_pair].book.bids.index(self.max_depth-1)) is close to "lower-most bid" THEN re-fetch snapshot.

The only question there is how exactly to define "close" between the two prices. We could either define it in terms of number of ticks, or we could also define it as the number of levels in our internal order book between these two prices (e.g. if number of levels is less than, say, 50, then re-fetch).

bmoscon commented 2 years ago

the PR has been merged, so closing this. Thanks to everyone for the help here

nirvana-msu commented 2 years ago

Should probably re-open until these issues are addressed.

shaunc commented 2 years ago

I'm wondering how to configure to ensure I can maintain the full book with deltas on Binance (including periodic snapshots). I found this issue... is there a current best practice?

bmoscon commented 2 years ago

@shaunc the orderbook object has both in it - full book and the delta. you can use whichever you choose