knowm / XChange

XChange is a Java library providing a streamlined API for interacting with 60+ Bitcoin and Altcoin exchanges providing a consistent interface for trading and accessing market data.
http://knowm.org/open-source/xchange/
MIT License
3.84k stars 1.94k forks source link

Nonce for sub-quarter second polling on btce #174

Closed drdozer closed 10 years ago

drdozer commented 10 years ago

Hi,

We've hit the quarter second polling limit for btc-e. Because of how the nonce is generated using a quarter-second granularity, we got our api keys fried. For now we've stuck a throughput-limited queue between calls and the application, but we'd also like a fixed nonce.

There are two options as we see it. First, replace the nextNonce() call with another one. We happen to have one that works ;) Second, add an interface NonceGenerator, and give an instance of this to BTCEBasePollingService at construction time (with a constructor that defaults this to the old behaviour). We've coded up the former:

https://github.com/drdozer/XChange/blob/develop/xchange-btce/src/main/java/com/xeiam/xchange/btce/service/polling/BTCEBasePollingService.java

If you'd like to see code for both options, I can create git-flow feature branches and pull requests for both.

Matthew

timmolter commented 10 years ago

I like the idea of replacing getNonce() with a working one, the first option. @mmazi can you take a close look at it since you've worked on that before??

Thanks!

mmazi commented 10 years ago

Already having a look :)

I've thought about the nonce algorithm @drdozer suggests when I implemented the current one, but I see a problem with it. If, during one lifetime of a BTCEBasePollingService, you query more than four times per second in average, then discard your BTCEBasePollingService and re-create one (eg. by restarting your application), you will start creating lower nonces than the ones you've already used, so they'll be illegal.

drdozer commented 10 years ago

Agreed. So, we could adjust the 250L divisor until you get a number of nonces that is an order of magnitude more frequent than the number of round-trip calls you could perform in serial per second.

mmazi commented 10 years ago

I don't understand exactly what you mean, but I suspect you might hit the long overflow limit too soon then.. (You could do the same with the current implementation, and have the same problem.)

Can you implement what you suggest?

drdozer commented 10 years ago

I see what you mean. My scheme lets you borrow nonces from the future, as long as you wait until all those clock ticks have ticked before you start a new nonce stream for the same api key. The only really robust way to solve this is to have a persisted nonce counter, but that is heavyweight and would definitely require a way to let API users choose an implementation that suited them.

mmazi commented 10 years ago

I agree.

keith-f commented 10 years ago

Hi. Usually 4 requests per second is plenty, and you'll be lucky to achieve this when the server is under load. However, sometimes performing a number of operations in quick succession (eg, balance check, followed by an orderbook query, followed by placing an order) with the current implementation will fail with 'invalid nonce' if the total round trip for one of the requests takes less than 0.25 seconds; the same nonce is generated for two of the requests.

The implementation as suggested by @drdozer would solve this problem, but as you've pointed out, may overflow. An alternative would be to modify the existing implementation to block and wait if a second call occurs within 0.25 seconds of the previous call. This would guarantee a unique nonce per request, and would allow applications to call exchange operations freely without worrying about timings.

drdozer commented 10 years ago

If the API is delaying the invocation, then it should return futures to avoid blocking the invoking thread. This would be a major change though. It would make it look more like how async js frameworks are structured e.g. gwt.

Sent from my android - may contain predictive text entertainment. On 1 Oct 2013 16:53, "keith-f" notifications@github.com wrote:

Hi. Usually 4 requests per second is plenty, and you'll be lucky to achieve this when the server is under load. However, sometimes performing a number of operations in quick succession (eg, balance check, followed by an orderbook query, followed by placing an order) with the current implementation will fail with 'invalid nonce' if the total round trip for one of the requests takes less than 0.25 seconds; the same nonce is generated for two of the requests.

The implementation as suggested by @drdozer https://github.com/drdozerwould solve this problem, but as you've pointed out, may overflow. An alternative would be to modify the existing implementation to block and wait if a second call occurs within 0.25 seconds of the previous call. This would guarantee a unique nonce per request, and would allow applications to call exchange operations freely without worrying about timings.

— Reply to this email directly or view it on GitHubhttps://github.com/timmolter/XChange/issues/174#issuecomment-25462209 .

mmazi commented 10 years ago

@drdozer's original suggestion will work just fine in environments where in average (during a lifetime of a BTCEBasePollingService) less than 4 requests per second are made, even if sometimes many quick successive requests are made. This probably covers most environments -- I imagine making more than 4 requests per second in average over an extended period of time wouldn't even be tolerated by BTC-E..

So actually I think that @drdozer's original suggestion is an improvement over the current implementation (even thought it is theoretically not bullet-proof). Perhaps we should just go with it.

mmazi commented 10 years ago

(Github really needs a confirmation alert for the "Close & Comment" button..)

timmolter commented 10 years ago

I agree with @mmazi 's suggestion to improve the current implementation and call it 'good enough' for now. Please close this issue when it's implemented as I'm going to wait for it before releasing version 1.9.0 of XChnage soon.

timmolter commented 10 years ago

@drdozer I think the consensus is to use your suggested fix for the nonce generation. Can you submit a pull request with the code update? Alternatively, you could point me to the code, and I can manually update since it's just a small change. Thanks!

drdozer commented 10 years ago

I have sent a pull request. I think it got linked into the issue correctly. Never quite sure.

Matthew

On 9 October 2013 10:45, Tim Molter notifications@github.com wrote:

@drdozer https://github.com/drdozer I think the consensus is to use your suggested fix for the nonce generation. Can you submit a pull request with the code update? Alternatively, you could point me to the code, and I can manually update since it's just a small change. Thanks!

— Reply to this email directly or view it on GitHubhttps://github.com/timmolter/XChange/issues/174#issuecomment-25958545 .

Dr Matthew Pocock Turing ate my hamster LTD mailto: turingatemyhamster@gmail.com

Integrative Bioinformatics Group, School of Computing Science, Newcastle University mailto: matthew.pocock@ncl.ac.uk

gchat: turingatemyhamster@gmail.com msn: matthew_pocock@yahoo.co.uk irc.freenode.net: drdozer skype: matthew.pocock tel: (0191) 2566550 mob: +447535664143