[20 BTC] Multithreaded Keep-alive Implementation in Bitcoind

[Jine]

Suggestions? I'll try this one in my personal dev-environment and see if i can replicate the issue.

I'm guessing that there's some path that doesn't release the RPC mutex. I put up a new build that might fix it, but it's hard to be sure since I can't replicate the problem.

Update: If that fails, I can just pull the multi-threading stuff and only fix keep-alives. That's a no-brainer (two lines change in trivial ways) and is very unlikely to cause any problems.

Tried out the latest version in my dev-environment - couldn't get the keep-alive to work tho.
It seems to create a new connection for each request.

Tried with the following script...

Code:

jine@srv:~/bitcoin-0.3.23/src/src$ cat test.sh
while [ 1 ]
do
        curl --keepalive-time 60 --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "getwork", "params": [] }' -H 'content-type: text/plain;' http://test:test@127.0.0.1:8332/
done

Directly against bitcoind that is, but i think curl should handle keep-alives well...
Any suggestions? :/ Could NOT get it to crash tho. Tried multiple times with huge number of requests etc... (not tried pushpoold yet tho)

[JoelKatz]

If you start a new curl process for each request, there is no way it can reuse the connection. You'd have to ask a single instance of curl to make multiple requests.

If you try this:
   curl --keepalive-time 60 --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "getwork", "params": [] }'
 -H 'content-type: text/plain;' "http://test:test@127.0.0.1:8332/[00-99]"

You'll see it retrieve 100 times without using 100 connections.

Without my patch: 100 connections, 5 seconds.
With my patch: 1 connection, .067 seconds.
They were all 'access denied' though. I didn't send a password. I'll retest with correct credentials.

[JoelKatz]

Thanks for the idea of using curl to test. I put up a new build. I didn't realize I was still sending 'Connection: close' in the success replies. Doh!

Now I ran this test:

curl -u MyUser:MyPass --keepalive --data-binary '{"jsonrpc": "1.0", "id":"curltest", "method": "getwork", "params": [] }' -H 'content-type: text/plain;' "http://test:test@127.0.0.1:8332/[00-99]"

Without my patch, 100 connections, .2 seconds.
With my patch, 1 connection, .09 seconds.

The [00-99] makes it retrieve 100 URLs -- bitcoind ignores the URL.

The only change between the version you have and my latest is this:

@@ -1571,7 +1571,7 @@ string HTTPReply(int nStatus, const stri
     return strprintf(
             "HTTP/1.1 %d %s\r\n"
             "Date: %s\r\n"
-            "Connection: close\r\n"
+            "Connection: keep-alive\r\n"
             "Content-Length: %d\r\n"
             "Content-Type: application/json\r\n"
             "Server: bitcoin-json-rpc/%s\r\n"

[Caesium]

Looks good! Trying it with a small private pool and no errors to report so far.

Pushpool is happily re-using connections and work is still being dished out - TIME_WAITs have disappeared

For anyone struggling to get this to build, by the way, use 0.3.22, not .23 or git. Any chance of a diff rather than the full file JoelKatz?

[JoelKatz]

Looks good! Trying it with a small private pool and no errors to report so far.

Pushpool is happily re-using connections and work is still being dished out - TIME_WAITs have disappeared

For anyone struggling to get this to build, by the way, use 0.3.22, not .23 or git. Any chance of a diff rather than the full file JoelKatz?

Glad to hear it. Unfortunately, the changes are really invasive. It will be hard to produce a clean diff. I'll see what I can do.

[JoelKatz]

Actually, it proved to be really easy! Here's a diff:
http://davids.webmaster.com/~davids/rpc.diff.txt

If it doesn't patch cleanly against some version you need to patch against, let me know. I should be able to rebase it without too much trouble. By the way, I was using 0.3.23.

[Jine]

I think i just discovered the issue with dying bitcoind, read the log below

I this when bitcoind unexpectedly  died, and i tried to restart it

11:44:15 <@jine> EXCEPTION: N5boost16exception_detail10clone_implINS0_19error_info_injectorINS_6system12sys tem_errorEEEEE
11:44:15 <@jine> Address already in use
11:44:15 <@jine> bitcoin in ThreadRPCServer()
11:44:15 <@jine> terminate called after throwing an instance of 'boost::exception_detail::clone_impl<boost::exception_detail::error_info_injector<boost::system::system_error> >'

What actually happend:

11:48:12 <@jine> I can't start bitcoind while 3000+ connections tries to connect to :8332
(This is due to the fact that pushpoold is using up all ports, including tcp/8332 to try to connect to bitcoind)

11:48:44 <@jine> When 3000+ people tries to connect to pushpoold
11:48:52 <@jine> I cant stop it on the "correct" way
11:48:57 <+cereal7802> so i take it your going to kill pushpool, try the patched bitcoind again, then restart push?
11:49:11 <@jine> Have to kill it with; kill -9 (forcefully)
(So all my previous test have been unsuccuessfull due to pushpoold still running, still using up sockets, which makes bitcoind not start / dying while initializing)

Will try that in a moment, but it looks promising

[JoelKatz]

By default, temporary ports are allocated from 32768 - 61000, so 8332 shouldn't ever conflict (unless you changed that to increase the range). This is controlled by /proc/sys/net/ipv4/ip_local_port_range.

If you increased the range to try to prevent running out of ports, you can blacklist 8332. Use
/proc/sys/net/ipv4/ip_local_reserved_ports.

It may help to add a 'brake' to pushpoold to stop it from doing whatever it's doing that's causing the issue. (Perhaps if a connection to bitcoind fails, do not attempt to reconnect for two seconds. That's pretty easy to hack in.) But I doubt it's that it's using a needed port.

[Jine]

We have tweaked ip_local_port_range

root@bitcoins:/usr/src/bitcoin-0.3.23/src/src# cat /proc/sys/net/ipv4/ip_local_port_range
1024    65535

EDIT: This to allow more sockets to be open at once, due to the amount of connections we're getting

[JoelKatz]

We have tweaked ip_local_port_range

root@bitcoins:/usr/src/bitcoin-0.3.23/src/src# cat /proc/sys/net/ipv4/ip_local_port_range
1024    65535

Ahh, you need to blacklist critical ports then!

See starting from line 604 here:
http://www.mjmwired.net/kernel/Documentation/networking/ip-sysctl.txt

[Jine]

Or modify it back to 32-65k and apply your patch

[JoelKatz]

Or modify it back to 32-65k and apply your patch

Even better!

[Caesium]

Sent you a little something Joel for your efforts

[JoelKatz]

Sent you a little something Joel for your efforts

Thanks. It is much appreciated.

[eleuthria]

Checking in after getting linked back here this morning.  Is the patch now working in a high volume production environment?  If so I'll have to throw some BTC at JoelKatz as well.

[JoelKatz]

Checking in after getting linked back here this morning.  Is the patch now working in a high volume production environment?  If so I'll have to throw some BTC at JoelKatz as well.

Donations will go to prioritizing my next BitCoin-related project, optimizing bitcoind for pooled mining in a few other ways.

First, native long-polling support in bitcoind, eliminating the need for blkmond and speeding up switching to a new block. You'll be able to pass a pidfile to bitcoind, and it will automatically send a SIGUSR1 to the pool manager when a new block is found.

Second, aggressively acquiring and maintaining more connections. The current client actually goes out of its way to keep fewer connections. But if you have more connections, you win two ways. First, you find out about new blocks faster, meaning you won't waste time working on a shorter chain. Second, your found blocks will get out the fastest, increasing the odds that your block will stay in the chain.

Third, optimizing the code paths that miners hit heavily. This includes, for example, more rapidly getting a found block out to all connected clients. (This actually may already be close to optimal. I'm not sure there's much room for improvement here, but it won't hurt to look.)

[simplecoin]

running the diff against .23 I get 10 out of 10 hunks failed.....

does this only work against .22?

[JoelKatz]

running the diff against .23 I get 10 out of 10 hunks failed.....

does this only work against .22?

I made it against .23, and I just checked it against a clean .23 and it applied fine. Make sure you're in the src/src directory. "patch < rpc.diff" should work.

[simplecoin]

hmm, I was... tried with -p0, retrying.

Update: copied to src/src and renamed to rpc.diff. patch < diff worked fine. Thanks!

compiling now.

[JoelKatz]

A preliminary combined diff is now available.
http://davids.webmaster.com/~davids/bitcoin-3diff.txt

Features:

1) RPC enhancements to support JSON parsing in a thread, concurrent handling of multiple RPC connections, and working HTTP keepalives.

2) Native long-polling support. New option "-pollpidfile=/path/to/pidfile". Pass in the path to your pool manager's PID file. (By default with pushpoold, it's "/var/run/pushpoold.pid".) This is faster than blkmond and eliminates the need to run it. (In fact, DO NOT use blkmond with this option as you'll wind up telling your miners to abort valid runs.)

3) More aggressive connection management. Outbound connections are made in their own thread and the client tries more aggressively to get more of them. This should mean both that you find out about new blocks faster and you get your own blocks out faster. This is most noticeable when you just restart bitcoind, as it tries very aggressively on startup to re-establish connectivity. But the benefits should accrue all the time -- you'll be working on a longer chain faster and others will be working on extending your blocks faster.

The changes to 'net.cpp' should not be used on machines with limited net connectivity as more connections will mean more traffic. Currently, these changes are not very tunable. (I plan to add a '-hub' option that will enable these features to make you more of a 'hub' of the BitCoin network.)

Test reports, suggestions, and donations are all welcome.