chrisleekr / binance-trading-bot

Automated Binance trading bot - Trade multiple cryptocurrencies. Buy low/sell high with Grid Trading. Integrated with TradingView technical analysis
MIT License
5.01k stars 1.09k forks source link

MaxOpentrades are still exceeded #484

Open Rayn0r opened 2 years ago

Rayn0r commented 2 years ago

3OpenTrades

Here are the details from the Slack messages I got, containing the message timestamp plus the transaction time for the order filled from each symbol.

06:34:18.284 GMTUSDT "transactTime": 1661150057973 06:38:27.152 DOGEUSDT "transactTime": 1661150306977 06:38:28.391 GRTUSDT "transactTime": 1661150307467

The last two are only 490 milliseconds apart, so they basically got executed at the same time... I also saved the docker logs content using docker logs a8f6cbf098e6 > tofile.log about 17 minutes after the transactions. I thought it could be pretty printed using: while read line ; do echo $line | python3 -m json.tool - ; done < tofile.log > tofile-pretty_print.log, but it prints a lot of error messages: Expecting ',' delimiter: line 1 column 3540 (char 3539)

What is the right way to pretty print it and what should I look for in the file?

uhliksk commented 2 years ago

@Rayn0r If the delay between those two executions is shorter than the delay between the bot will receive the information about the first execution and the ability to cancel the other one, it will end up with both orders executed. That's just by design and there is no other way to prevent it than to have very fast machine and low latency internet connection. That's the similar thing to when you place the stop limit with price very near to stop limit and if market is changing too fast it can skip your price level and your stop limit order will not be executed at all. You always have to plan your strategy with some margin of error.

uhliksk commented 2 years ago

@Rayn0r If your projected margin of error is 0%, then you can set "Max. Buy Open Orders" to 1. This will prevent multiple buy orders to be executed at the same time perfectly. :)

Rayn0r commented 2 years ago

That's just by design and there is no other way to prevent it than to have very fast machine and low latency internet connection.

I don't think that a fast connection is the key here, but the rate at which the symbols are traded. If there are no trades, then there is nothing to report. Ergo, there is no new message at the web socket. I started a little python script just for fun and have logs of ~300,000 web socket message for a single symbol. The time difference between them ranges from 0.011 to 59.833 seconds. Where most message updates take between 2 and 3 seconds. Out of that ~300,000 messages only 150 had less than 490ms between them. But with multiple symbols coming from the web socket, chances for short intervals increases, I'd say.

@Rayn0r If your projected margin of error is 0%, then you can set "Max. Buy Open Orders" to 1. This will prevent multiple buy orders to be executed at the same time perfectly. :)

I'll give that a try and see how well it goes. Thanks.

uhliksk commented 2 years ago

I don't think that a fast connection is the key here, but the rate at which the symbols are traded.

As soon as the first buy order was executed and an open trade limit was exceeded, the bot queued neccessary requests to check and cancel all other buy orders in Grid Trade 1 immediatelly without waiting for another message at the web socket to be received. If you had a fast machine with low latency connection (and Binance processed API request in time) the second order would be cancelled in less than 490 ms. It's all about the timing.

Maybe what should be a problem is if there are already some other websocket messages queued and the cancellation is processed too late because of this as they are added at the end of actual queue. If this is a problem then it should help in rush hours to add high priority flag to process those buy order cancellations even before all other already queued websocket messages. I'm not sure if this is even possible in current queue implementation. What do you think @habibalkhabbaz @chrisleekr ?

habibalkhabbaz commented 2 years ago

@uhliksk Your explanation makes sense to me. I think that the order executed too much fast for @Rayn0r

And for the second possibility, yes, there is a priority option for queued jobs but the problem is that it has a bad impact on performance based on the bull queue documentation.

priority: number; // Optional priority value. ranges from 1 (highest priority) to MAX_INT  (lowest priority). Note that
  // using priorities has a slight impact on performance, so do not use it if not required.

https://github.com/OptimalBits/bull/blob/develop/REFERENCE.md#queueadd

uhliksk commented 2 years ago

@habibalkhabbaz

Thank you for the documentation. I think the lifo can be used instead of priority in our case without impacting the performance.

 lifo: boolean; // if true, adds the job to the right of the queue instead of the left (default false)
habibalkhabbaz commented 2 years ago

@uhliksk

Sure it's better than setting priority for our case!

However, I just started to rethink about this. What I found is we will not get any benefit from setting priority option or using lifo because the order is already executed at this point based on @Rayn0r case.

What we can do is set max open orders and max open trades to a more meaningful value based on our strategy like what @uhliksk suggested.

uhliksk commented 2 years ago

However, I just started to rethink about this. What I found is we will not get any benefit from setting priority option or using lifo because the order is already executed at this point based on @Rayn0r case.

But he said there was 490 ms between those two executions. Do you think the bot will not be able to cancel the second sell order in this case?

uhliksk commented 2 years ago

@habibalkhabbaz Now I understand what you mean. If there is already sell order execution queued then it is already too late to cancel the order. But if there is just a price update or any other websocket message queued then cancelling before this queued message is processed should help as we can cancel the sell order a few miliseconds faster this way. I think we have nothing to lose by applying lifo there.

uhliksk commented 2 years ago

Another possibility why the sell order hasn't been cancelled is #487.

habibalkhabbaz commented 2 years ago

Hello @uhliksk

The current queue implementation is just pushing an empty job to the symbol queue and its listener will execute the trailing trade steps. No matter if we queue for cancel order or ticker received or anything else, all of them will just call executeTrailingTrade. That's why I think no change will happen if we use lifo.

I think that what happened to @Rayn0r is as follows:

  1. BTCUSDT: Order executed successfully
  2. BTCUSDT: Call executeTrailingTrade
  3. DOTUSDT: While the BTCUSDT is executing the trailing trade, the order is executed successfully
  4. BTCUSDT: Processed the executed order.
  5. BTCUSDT: Queue the other symbols open orders to cancel them (Too late, DOTUSDT order already executed)
Rayn0r commented 2 years ago

I'm not sure, but I think transactTime is the time stamp of when the order has been successfully processed. So if the difference for transactTime of those two is 490ms then they must have run pretty much at the same time. It takes some time before the orders are filled and I guess, that's what happened here. Both got started and once the first one was filled it was too late to cancel the second.

uhliksk commented 2 years ago

Hello @habibalkhabbaz

Does that mean if there are two jobs queued then both of them will process the same data? What's the purpose of the queue then?

Example:

  1. Price change 1 received
  2. Started executeTrailingTrade job 1
  3. Price change 2 received while job 1 is running
  4. Queue the job
  5. Price change 3 received while job 1 is still running
  6. Queue the job
  7. Job 1 finished
  8. Start Job 2 from queue
  9. Job 2 finished
  10. Start Job 3 from queue
  11. Job 3 finished

What price will be applied after step 9 when Job 2 is finished?

habibalkhabbaz commented 2 years ago

Hello @uhliksk

I think my example was not clear. I just updated the example to be more clear. Sorry for that.

Each symbol can have only 1 concurrent job running.

What I am trying to explain is that the case happened to @Rayn0r is possible because when BTCUSDT executing the trailing trade, the other symbol (DOTUSDT) order is already executed and it's too late for BTCUSDT to queue the cancelling job for the open orders.

uhliksk commented 2 years ago

What I am trying to explain is that the case happened to @Rayn0r is possible because when BTCUSDT executing the trailing trade, the other symbol (DOTUSDT) order is already executed and it's too late for BTCUSDT to queue the cancelling job for the open orders.

Ok, now I think I understand. But still I think lifo should help in some cases to process the order cancellation faster if empty job is processed faster. Also maybe we should add some parameter to skip unnecessary steps of executeTrailingTrade when we just want to check max. open trades limit to process it more directly and potentially little bit faster as we don't need to get balances, etc.. Same in opposite way we don't need to check open trades or open orders when just a simple price change occured, etc.

uhliksk commented 2 years ago

If symbol is locked or action is already defined then our attempt to cancel the order will be skipped. We need additional parameter to queue to force process handle-open-orders if we initiated this from buy order executed. And there also lifo will be useful because we want to process this as the highest priority. What do you think @habibalkhabbaz ?

https://github.com/chrisleekr/binance-trading-bot/blob/5ce87116490ac6a585d7f0cd5578b6979251614c/app/cronjob/trailingTrade/step/handle-open-orders.js#L84-L98

habibalkhabbaz commented 2 years ago

@uhliksk You are right 💯 That is one reason. We have to refactor this to force handling open orders. However, I think we can just keep the queue to work as fifo since there is no benefit from setting lifo because executeTrailingTrade will be executed as one shot whatever from where it was queued.

P.S But still there is a possibility for the order to be executed when it is executed (almost) in parallel with another symbol and no solution for that as it is expected to happen.

uhliksk commented 2 years ago

@habibalkhabbaz I agree but also I'm just thinking if we actually still need the symbol locking. If we queue executeTrailingTradeIndicator in the same queue with executeTrailingTrade then we will not need to lock symbols and there will be no skipped executeTrailingTrade because of symbol lock, isn't it?

habibalkhabbaz commented 2 years ago

@habibalkhabbaz I agree but also I'm just thinking if we actually still need the symbol locking. If we queue executeTrailingTradeIndicator in the same queue with executeTrailingTrade then we will not need to lock symbols and there will be no skipped executeTrailingTrade because of symbol lock, isn't it?

Sure, I agree with you too @uhliksk I think we can just skip checking the symbol lock.

There was a reason for that, but now after refactoring to WebSocket, I think we can just skip that.

Explanation

Before refactoring to WebSocket the executeTrailingTradeIndicator was having critical steps like get-indicators, that's why we lock the symbol and wait. But now, after we did the refactoring, the indicators moved directly in executeTrailingTrade. Hence, we can skip checking the lock.

https://github.com/chrisleekr/binance-trading-bot/pull/431

What do you think @chrisleekr & @uhliksk ?

uhliksk commented 2 years ago

@habibalkhabbaz I'll try to blindly test with symbol locking disabled as I don't have a time to check the code right now but your arguments seem right to me. :)