Adding queue to prevent 'Bitcoin JSON-RPC: Work queue depth exceeded' errors #23

karelbilek · 2017-03-22T23:30:21Z

I have added a queue, fixing recurring "Bitcoin JSON-RPC: Work queue depth exceeded" errors.

The queue size can be set from calling code by options (I will add it to bitcore-node next); by default, 16 is used, since that's the default bitcoind queue size.

Fixes issues:

and maybe others

I am using the same async version as bitcore-node, so it's not installed twice with different versions :)

karelbilek · 2017-03-22T23:31:53Z

I have also fixed the tests (for some reason, they didn't go through; maybe other node version).

I am not sure what will Travis do with it. ....and he will fail. I will edit the tests so they go through both on my PC and on Travis then.

coveralls · 2017-03-22T23:32:17Z

Coverage decreased (-0.8%) to 99.16% when pulling 49af46f on runn1ng:master into da5d5ec on bitpay:master.

karelbilek · 2017-03-22T23:35:43Z

New node in travis fixed the tests

coveralls · 2017-03-22T23:36:23Z

Coverage remained the same at 100.0% when pulling 3a0b96a on runn1ng:master into da5d5ec on bitpay:master.

levino

Overall a correct implementation of the feature. I personally would love the difff to be smaller / the changes to be more lightweight.

There are also some bigger issues as changing the node version which is a no go from my perspective. Needs to be addresses or at least discussed.

levino · 2017-03-24T08:34:51Z

.travis.yml

@@ -1,6 +1,6 @@
 language: node_js
 node_js:
- '0.10'
+- '7'


I think this should be avoided. Changing the node versionin travis is a pretty major change. Why should this be necessary?

Well, I run the tests on my PC, they failed (because node changed the error message on JSON parsing slightly), so I corrected the tests, but then they failed on travis. So I corrected Travis.

I don't think it's a major change, but I don't really care, it's just about different error in the tests. If this is a blocker, I can remove the two commits.

OK, I will remove the test changes (this and the JSON line), I agree that it doesn't belong in this PR

levino · 2017-03-24T08:35:29Z

lib/index.js

@@ -3,6 +3,8 @@
 var http = require('http');
 var https = require('https');

+var async = require('async');


Line 5 is empty. Should not be the case. I would remove it.

yeah good idea

levino · 2017-03-24T08:36:52Z

lib/index.js

@@ -22,6 +24,10 @@ function RpcClient(opts) {
    this.log = RpcClient.loggers[RpcClient.config.logger || 'normal'];
  }

+  var queueSize = opts.queue || 16;


Constant values (like 16) should be set at the top or in a config file like

var DEFAULT_CONCURRENCY=16
and then

var queueSize = opts.queue || DEFAULT_CONCURRENCY

Would be really annoying to go looking for that value.

true. But it's not there for the other values also (user/pass), I didn't want to refactor too much... but yeah I can change that

levino · 2017-03-24T08:37:25Z

lib/index.js

@@ -38,90 +44,100 @@ RpcClient.config = {
  logger: 'normal' // none, normal, debug
 };

-function rpc(request, callback) {
+function rpc(request, originalCallback) {


I see no reason to change this parameter name. Should not be done in my opinion.

Well I can either change callback to originalCallback and then callback stays in the rest of the code (since I need to call both the original callback and the "task" callback).

Or I can leave callback, like you suggest, and then I will need to make newCallback and call newCallback everywhere instead of callback.

levino · 2017-03-24T08:39:23Z

lib/index.js

-    for (var k in self.httpOptions) {
-      options[k] = self.httpOptions[k];
+  var task = function(taskCallback) {
+    var callback = function() {


What about error handling? Can this still be done properly?

It's done exactly as in the original code

levino · 2017-03-24T08:39:56Z

lib/index.js


-  var called = false;
+    var called = false;


Please remove all whitespace changes from the PR. This just bloats the diff and is very annoying when reviewing.

It's not unnecessary whitespace change. The code is in a new function that wasn't there before.

I can make two commits, one without the whitespace change and one that just shifts the lines, but is that really necessary.

levino · 2017-03-24T08:42:10Z

package.json

    "chai": "^1.10.0",
    "coveralls": "^2.11.2",
    "istanbul": "^0.3.5",
    "mocha": "^2.1.0",
    "sinon": "^1.12.2"
  },
+  "dependencies": {
+    "async": "^1.3.0"


I would suggest use fixed deps or yarn. This right here will lead to undeterministic builds and heavy pain in the future. Async is a well maintained library, that is for sure but still they can f**k up a release. Better be safe than sorry.

So for the time being I would make the line so:

"async":"1.3.0"

bitcore-node uses exactly this dependency, "async": "^1.3.0". Given that this is used only in bitcore-node, it doesn't matter really

karelbilek · 2017-03-24T10:40:54Z

Thanks for the feedback. I agree with some, I will do some little changes

I can remove the changes in tests/travis if that's a stopper for merging, I don't really care (it was really only in order for my PC to run the tests, since I have a new node version; but I can change it back).

Anyway bitpay doesn't seem to reply on pull requests or merge them, so I don't know if it matters :)

coveralls · 2017-03-24T10:53:58Z

Coverage decreased (-48.8%) to 51.24% when pulling f6154ce on runn1ng:master into da5d5ec on bitpay:master.

coveralls · 2017-03-24T10:57:43Z

Coverage remained the same at 100.0% when pulling 6c319b5 on runn1ng:master into da5d5ec on bitpay:master.

karelbilek · 2017-03-24T11:01:31Z

OK, I made the patch significantly smaller by making it a new function instead of wrapping it in a function-inside-function.

I kept the async version in package.json, since the ^version is used in the whole bitcore/insight

levino · 2017-03-24T11:17:39Z

@Runn1ng I was just reviewing as a bistander. I actually am not a maintainer of this codebase and cannot block or merge anything.

karelbilek · 2017-03-24T11:24:40Z

Yeah I know. Collabolators got a little badge on github :)

Related to bitpay/bitcoind-rpc#23

elichai · 2017-05-15T14:55:03Z

Why no one is merging it?

coveralls · 2017-09-25T00:59:43Z

Coverage remained the same at 100.0% when pulling 6cb02d6 on runn1ng:master into da5d5ec on bitpay:master.

coveralls · 2017-09-25T01:03:57Z

Coverage remained the same at 100.0% when pulling ee25142 on runn1ng:master into da5d5ec on bitpay:master.

Berndinox · 2017-10-25T07:14:51Z

... years later

adamu · 2017-11-08T01:06:41Z

Why is this PR showing commit ee25142 instead of 6c319b5?

karelbilek · 2017-11-08T11:44:38Z

Oh sorry, I probably pushed the smart fee commit to my branch to my repo by mistake. Thanks for noticing, I will force-push again

gasteve · 2018-01-15T15:23:52Z

I'm not sure this is the right approach to dealing with this issue ... since bitcoind-rpc is used client side, and there can be many concurrent users of a given bitcoind node, the work queue could still be exceeded even with this logic in place. Isn't it better to handle such errors gracefully than to burden this library with logic that would only prevent the issue if a single process was exceeding the bitcoind work queue? Also, a wrapper of some sort that exists outside this repo could accomplish the same thing if it's really desired.

carnesen · 2018-01-15T16:07:22Z

@gasteve I haven't looked into the specifics of the implementation in this PR, but it's common for a client library to provide a mechanism for limiting the number of concurrent requests. For example, https://github.com/brianc/node-postgres uses connection "pooling". It's true that for this to be effective there needs to be some level of coordination between clients and server (at least in terms of configuration) so that each client sets its concurrency appropriately. For example, if the server allows 16 concurrent tasks and there are four clients, then perhaps to be safe each client would self-limit to four concurrent tasks.

levino · 2018-01-16T06:22:38Z

@gasteve Valid point. But this is about fixing a dos vulnerabilty asap. As long as you have no PR that implements your suggested improvements, I would say: Done is better than perfect.

levino · 2018-01-16T06:24:01Z

But anyhow, this will not get merged because no maintainer gives a damn...

levino · 2018-01-16T06:31:09Z

Thinking a bit more about it, I see another issue: This is actually not fixing the dos vulnerability at all. An attacker can still flood the api with carefully designed requests so that the queue will fill up and no other user will be able successfully submit a single request (it will get queued at position 1 mio or so and only be resolved after 5 hours). In order to protect the insight-api against dos we need to queue "per user" on a much higher level.

Also it should be fine to get a "bitcoin rcp work queue depth exceeded" if it is handled gracefully further up in the stack instead of just being sent out straight to the user. One could retry or something.

So all in all I agree with the point that this here is the wrong place to try to fix the issue. Also the proposed changes are NOT fixing the issue at hand, which is: Anyone can take down an insight-api deployment with no cost.

winteraz · 2018-01-16T10:32:00Z

I'm trying to index (freshly) the data and I get this error. I didn't receive a single the API request(i.e. no chance I got DDOS-ed) Does this PR fixes freshly indexes as well?
I have a big server (64GB RAM , SSD etc) and just managed to index it at 6.17%.... this is really frustrating
[2018-01-16T10:11:11.098Z] error: RPCError: Bitcoin JSON-RPC: Work queue depth exceeded
[2018-01-16T10:11:26.100Z] error: RPCError: Bitcoin JSON-RPC: Work queue depth exceeded
[2018-01-16T10:11:41.101Z] error: RPCError: Bitcoin JSON-RPC: Work queue depth exceeded
[2018-01-16T10:12:56.109Z] error: RPCError: Bitcoin JSON-RPC: Work queue depth exceeded
[2018-01-16T10:13:11.109Z] error: RPCError: Bitcoin JSON-RPC: Work queue depth exceeded
[2018-01-16T10:13:26.110Z] error: RPCError: Bitcoin JSON-RPC: Work queue depth exceeded

karelbilek · 2018-01-16T11:50:32Z

@levino yes this does not really solve the DDoS vulnerability.

However it stops "accidental" overflow of the RPC queue.

Related to bitpay/bitcoind-rpc#23

https://github.com/satoshilabs/bitcoind-rpc/pull/1/files and bitpay/bitcoind-rpc#23

levino · 2018-10-31T11:42:51Z

Can someone close this? Thanks.

This was referenced Mar 22, 2017

Bitcoin JSON-RPC: Work queue depth exceeded' while reindexing bitpay/bitcore-node#463

Open

Adding rpc queue options bitpay/bitcore-node#499

Open

levino suggested changes Mar 24, 2017

View reviewed changes

Adding queue to RPC calls

6c319b5

karelbilek mentioned this pull request Mar 29, 2017

Adding queue to RPC calls trezor-graveyard/bitcoind-rpc#1

Merged

karelbilek added a commit to trezor-graveyard/bitcore-node that referenced this pull request Mar 29, 2017

Adding rpc queue options

c67586b

Related to bitpay/bitcoind-rpc#23

losh11 pushed a commit to litecore-archive/litecore-node that referenced this pull request May 11, 2017

Adding rpc queue options

75efe16

Related to bitpay/bitcoind-rpc#23

levino mentioned this pull request Jan 16, 2018

'Bitcoin JSON-RPC: Work queue depth exceeded. Code:429' bitpay/insight-api#492

Open

Bushstar pushed a commit to FeatherCoin/feathercore-node that referenced this pull request Jun 5, 2018

Adding rpc queue options

9a1fb7d

Related to bitpay/bitcoind-rpc#23

Bushstar pushed a commit to fiscalobject/ufocore-node that referenced this pull request Jun 11, 2018

Adding rpc queue options

3b0a763

Related to bitpay/bitcoind-rpc#23

eugene-sy added a commit to meritlabs/lightwallet-stack that referenced this pull request Jul 9, 2018

rpc queue for meritd.js

1b827b9

https://github.com/satoshilabs/bitcoind-rpc/pull/1/files and bitpay/bitcoind-rpc#23

eugene-sy mentioned this pull request Jul 9, 2018

RPC queue for meritd.js meritlabs/lightwallet-stack#1010

Merged

matiu closed this Oct 31, 2018

Adding queue to prevent 'Bitcoin JSON-RPC: Work queue depth exceeded' errors #23

Adding queue to prevent 'Bitcoin JSON-RPC: Work queue depth exceeded' errors #23

Conversation

karelbilek commented Mar 22, 2017 • edited Loading

karelbilek commented Mar 22, 2017 • edited Loading

coveralls commented Mar 22, 2017

karelbilek commented Mar 22, 2017

coveralls commented Mar 22, 2017

levino left a comment

Choose a reason for hiding this comment

levino Mar 24, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelbilek commented Mar 24, 2017

coveralls commented Mar 24, 2017

coveralls commented Mar 24, 2017

karelbilek commented Mar 24, 2017

levino commented Mar 24, 2017

karelbilek commented Mar 24, 2017

elichai commented May 15, 2017 • edited Loading

coveralls commented Sep 25, 2017

coveralls commented Sep 25, 2017

Berndinox commented Oct 25, 2017

adamu commented Nov 8, 2017

karelbilek commented Nov 8, 2017

gasteve commented Jan 15, 2018

carnesen commented Jan 15, 2018

levino commented Jan 16, 2018

levino commented Jan 16, 2018

levino commented Jan 16, 2018

winteraz commented Jan 16, 2018 • edited Loading

karelbilek commented Jan 16, 2018

levino commented Oct 31, 2018

karelbilek commented Mar 22, 2017 •

edited

Loading

karelbilek commented Mar 22, 2017 •

edited

Loading

levino Mar 24, 2017 •

edited

Loading

elichai commented May 15, 2017 •

edited

Loading

winteraz commented Jan 16, 2018 •

edited

Loading