Proposed depth cache change:

[miketikh]

Currently depth cache error handling (in depthHandler) works like this:

 else if ( context.lastEventUpdateId && depth.U !== context.lastEventUpdateId + 1 ) {
const msg = 'depthHandler: ['+symbol+'] Incorrect update ID. The depth cache is out of sync.';
            if ( options.verbose ) options.log(msg);
            throw new Error(msg);
}

In other words, if the first new depth event ID is not equal to context's lastEventUpdatedID + 1, it logs an error message and throws an error.

Let's go through an example:

First depth update:

• context lastUpdate: 0;
• first new depth event ID: 1
• last new depth event ID: 2
• depth events:
  1. set bids { 1: 0000 }
  2. set bids { 1.25: 0002 }

Second depth update:

• context lastUpdateID: 2
• first new depth event ID: 1
• last new depth event ID: 3
  1. set bids { 1: 0000 }
  2. set bids { 1.25: 0002 }
  3. set ask { 2: 1 }

Currently the second update would throw an error, because the first new depth ID != context lastUpdate ID + 1. That means you're receiving duplicate updates.

I think this is an incorrect action, because the second depth update contains all the old updates AND new updates. Unless I'm misunderstanding how setting the local cache works, it does not reduce accuracy to put in old updates in twice (ie. setting bids { 1: 00 } two times), but it does skipping new depth updates.

Using our example above, on the second depth update, there is no harm in putting events 1 and 2 into the local cache again, but there is a harm in skipping event 3.

I would propose changing it to:

• Throw an error ONLY if context.lastUpdateEventID > depth.u (if the last update recorded is newer than the last update being sent, so all the information sent is old)
• Continuing to set all updates otherwise

Or, this:

 } else if ( context.lastEventUpdateId && context.lastEventUpdateId > depth.u) {
            const msg = '**** depthHandler: ['+symbol+'] Incorrect update ID. The depth cache is out of sync.';
            if ( options.verbose ) options.log(msg);
            throw new Error(msg);
        } else {
            for ( obj of depth.b ) { //bids

[jaggedsoft]

I agree. Good observation and thank you for your contribution

[miketikh]

Sorry, posted a little prematurely! There is one tweak I would add:

The cache should throw an out of sync error if:

1. context.lastUpdatedID is > depth.u
   2. context.lastUpdatedID < depth.U - 1

The first case, that means all the updates are old and shouldn't be stored. The second case means there was a gap between updates and some orders weren't received.

It should update only update the local cache if depth.U <= context.lastUpdatedID <= depth.u, otherwise it should throw an out of sync error:

} else if ( context.lastEventUpdateId && (context.lastEventUpdateId < depth.U - 1 || context.lastEventUpdateId > depth.u)) {
            const msg = '**** depthHandler: ['+symbol+'] Incorrect update ID. The depth cache is out of sync.';
            if ( options.verbose ) options.log(msg);
            throw new Error(msg);
        } else {
            for ( obj of depth.b ) { //bids

Hopefully that logic is correct, thanks for the great api!

[bkrypt]

Thanks @learnathoner, this looks great. I'll implement the change first thing in the morning.

[bkrypt]

Hi @learnathoner. Thanks again for this. After investigating things a bit further this morning, I'd just like to confirm a few things. Have you tested this change locally at all, and confirmed the data is packaged as you expect in your examples? I'm just asking because this doesn't follow the documentation on how to manage a depth cache locally as appears here.

In your example, the second event, with it's overlapping update IDs, would violate the protocol definition as per instruction 6:

6. While listening to the stream, each new event's U should be equal to the previous event's u+1

In my tests, I've never seen an update not conform to that documented pattern, and have only seen cases where the current update's U is greater than the previous update u + 1, meaning the local cache has missed updates and now has stale data sitting in it.

I am currently looking to sort the depthCache logic out with ideas I've had recently that are the cause of the issues we've been seeing, and will definitely keep the logic you've posted here in mind. If you are able to post concrete examples of overlapping update data though (as received from Binance directly), that would be a huge help.

NOTE: the crucial thing, is that this overlap occurs before the first out of sync error. If it only appears after a "reconnect" due to out of sync, the cause is something different and will be removed by the fix I'm currently implementing.

[miketikh]

Hi Keith,

I wish I had done taken more screenshots yesterday, since today the socket appears to be working much better.

You can find an example in the screenshot below of a duplicate update:

First update:

• context.lastEventUpdatedId: 5606 6208
• depth.U: 5606 6209
• depth.u: 5606 6211
• Information for 209 - 211 is stored in the depth cache, context.lastEventUpdatedId was set to 211.

Second update:

• context.lastEventUpdatedId: 5606 6211
• depth.U: 5606 6209
• depth.u: 5606 6211
• Error thrown, no information stored. Continues from next depth update.

In that example the update is a duplicate with no new information. It wouldn't hurt to update the cache again, but it also wouldn't change anything.

Yesterday I was also running many situations with an overlap, where for example:

• context.lastEventUpdatedId: 3
• depth.U: 2
• depth.u: 5
• Error thrown, no information stored

Then the next depth update might start at U: 6, so the information for updates 4 and 5 would have been lost. I'll log everything over the next week and try to get some screenshots of this occurring.

The error should not be due to reconnecting, since I have reconnect set to false.

On Sun, May 20, 2018 at 3:43 AM, Keith Kirton notifications@github.com wrote:

Hi @learnathoner https://github.com/learnathoner. Thanks again for this. After investigating things a bit further this morning, I'd just like to confirm a few things. Have you tested this change locally at all, and confirmed the data is packaged as you expect in your examples? I'm just asking because this doesn't follow the documentation on how to manage a depth cache locally as appears here https://github.com/binance-exchange/binance-official-api-docs/blob/master/web-socket-streams.md#how-to-manage-a-local-order-book-correctly .

In your example, the second event, with it's overlapping update IDs, would violate the protocol definition as per instruction 6:

1. While listening to the stream, each new event's U should be equal to the previous event's u+1

In my tests, I've never seen an update not conform to that documented pattern, and have only seen cases where the current update's U is greater than the previous update u + 1, meaning the local cache has missed updates and now has stale data sitting in it.

I am currently looking to sort the depthCache logic out with ideas I've had recently that are the cause of the issues we've been seeing, and will definitely keep the logic you've posted here in mind. If you are able to post concrete examples of overlapping update data though (as received from Binance directly), that would be a huge help.

NOTE: the crucial thing, is that this overlap occurs before the first out of sync error. If it only appears after a "reconnect" due to out of sync, the cause is something different and will be removed by the fix I'm currently implementing.

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jaggedsoft/node-binance-api/issues/218#issuecomment-390466584, or mute the thread https://github.com/notifications/unsubscribe-auth/AbI8L5J439kPImJKtjvbPjvhVP7aFG14ks5t0SzEgaJpZM4UFyil .

[bkrypt]

@learnathoner That's actually perfect, thank you. I imagine it may be the exchange sort of "blanket" covering a range of updates when it detects it may not have delivered some update in the last range N or something. Since this happens so infrequently, and would explain why I've not run into it personally yet.

I am with you that updating the cache again with some duplicate data isn't a problem. Ultimately it's only when depth.U is greater than context.lastEventUpdateId + 1 that we actually have a true out of sync problem. I'll add the logic to deal with this to the current PR I have open. Thanks again for all your data re this issue, I appreciate it.

[jaggedsoft]

You are both rock stars. Thank you for your help, we would be lost without you 🏆 available in v0.6.0

[miketikh]

Glad to help, and once again, thanks for the well made API!

It's saved me a lot of headache working with binance's order books, keep up the great work.

On Sun, May 20, 2018 at 1:09 PM, Jon Eyrick notifications@github.com wrote:

You are both rock stars. Thank you for your help, we would be lost without you 🏆 available in v0.6.0

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/jaggedsoft/node-binance-api/issues/218#issuecomment-390500340, or mute the thread https://github.com/notifications/unsubscribe-auth/AbI8LzW0XBeqzFYCdC2JwI0w-rsspQ0wks5t0bFhgaJpZM4UFyil .