[20 BTC] Multithreaded Keep-alive Implementation in Bitcoind

[somebadger]

i just merged your diff with the latest from the official bitcoin git,

https://github.com/somebadger/bitcoin4pools

I also added the following to --help

Code:

  -pollpidfile=<file>      Pushpoold Pid file for long polling support
  -hub=<n>      Hub mode for pool operators

other than some manual intervention and fixing CReserveKey::CReserveKey(CWallet*) syntax for the latest build it wasnt too much of a headache. enough though that i'm keeping it in git now

ps. i hate manually doing .rej's lol

[1713507319]

1713507319

[1713507319]

1713507319

[JoelKatz]

If you get the chance, a pull request for the two resource leak fixes would be nice.

That would be this change to net.cpp:

     // Send and receive from sockets, accept connections
-    pthread_t hThreadSocketHandler = CreateThread(ThreadSocketHandler, NULL, true);
+    CreateThread(ThreadSocketHandler, NULL);

And this change to util.h:

         return (pthread_t)0;
     }
     if (!fWantHandle)
+    {
+        pthread_detach(hthread);
         return (pthread_t)-1;
+    }

[somebadger]

If you get the chance, a pull request for the two resource leak fixes would be nice.

That would be this change to net.cpp:

     // Send and receive from sockets, accept connections
-    pthread_t hThreadSocketHandler = CreateThread(ThreadSocketHandler, NULL, true);
+    CreateThread(ThreadSocketHandler, NULL);

And this change to util.h:

         return (pthread_t)0;
     }
     if (!fWantHandle)
+    {
+        pthread_detach(hthread);
         return (pthread_t)-1;
+    }

Patched and submitted pull request
https://github.com/bitcoin/bitcoin/pull/385

[JoelKatz]

Thanks.

[somebadger]

No probs

[TeraPool]

i just merged your diff with the latest from the official bitcoin git,

https://github.com/somebadger/bitcoin

I also added the following to --help

Code:

  -pollpidfile=<file>      Pushpoold Pid file for long polling support
  -hub=<n>      Hub mode for pool operators

other than some manual intervention and fixing CReserveKey::CReserveKey(CWallet*) syntax for the latest build it wasnt too much of a headache. enough though that i'm keeping it in git now

ps. i hate manually doing .rej's lol

QUESTION

JoelKatz's optimizations here will probably never make it into the official bitcoin github account, will they? Because they typically only make sense for pool operators or am I wrong?

The patches I believe will definitely make it in because that can benefit everybody. (Great work!)

So if anyone downloads the latest "v0.3.24rc3" or later from somebadger's github account, then it will contain JoelKatz's latest "diff" as seen here http://davids.webmaster.com/~davids/bitcoin-3diff.txt

Will somebadger's fork always contain the new pool related code? Just wondering if I should just make my own github fork of bitcoin just for my own pool so I can keep track of this and any future similar changes.

Great work JoelKatz. Thank you for that contribution to the community!

[somebadger]

QUESTION

JoelKatz's optimizations here will probably never make it into the official bitcoin github account, will they? Because they typically only make sense for pool operators or am I wrong?

The patches I believe will definitely make it in because that can benefit everybody. (Great work!)

So if anyone downloads the latest "v0.3.24rc3" or later from somebadger's github account, then it will contain JoelKatz's latest "diff" as seen here http://davids.webmaster.com/~davids/bitcoin-3diff.txt

Will somebadger's fork always contain the new pool related code? Just wondering if I should just make my own github fork of bitcoin just for my own pool so I can keep track of this and any future similar changes.

Great work JoelKatz. Thank you for that contribution to the community!

My plan is to keep the fork upto date with any improvements for pool operators, i'll also be adding bitcoin pulls that help pool performance

ps. its patched based on http://davids.webmaster.com/~davids/bitcoin-4diff.txt and modifications to make it apply to the latest build

[JoelKatz]

JoelKatz's optimizations here will probably never make it into the official bitcoin github account, will they? Because they typically only make sense for pool operators or am I wrong?

Some of them may not, some of them might. Once I'm convinced this code is stable, I can work on getting some of them into the main distribution. I think the hub mode changes and native long polling could get in. The RPC turbocharge and keepalive fixes could.

Some of the stuff in the latest diff might get in too. Stashing the RPC user/pass, avoiding re-processing partial blocks, faster base64 decode, and faster bin2hex all might get in. (Change 3diff to 4diff to see them.)

Some code should never get in. The JSON bypass code is awful. Perhaps a change to using Jansson is possible. But that's a pretty big change.

The patches I believe will definitely make it in because that can benefit everybody. (Great work!)
...
Great work JoelKatz. Thank you for that contribution to the community!

Thanks.

[MrSam]

Thanks for that report. I'll check the code and see if I can get the SIGUSR1 to be sent faster. I think that will solve half the problem though. While pushpool will issue fewer blocks that will turn into stale shares, bitcoind will still be processing the new block when all the new 'getwork' requests start piling in, and you'll get miner idles instead.

I have a fix for this that has other benefits as well, but the first time I tried to implement it, it was a disaster. I think I know the right way to do it now, having run out of wrong ways to do it already. What I'd like to do is pre-compute the skeleton 'getwork' block when a new block or new transaction comes in, rather than waiting to hijack the next 'getwork' request to do it. But to make this work, the locking has to be exactly right. The funny thing is, in my day job ten years ago, this was exactly the type optimization I specialized in. And now I can't seem to quite get it right.

I solved it with what i call the "pushpool feelgood patch". This patch accepts solutions that he knows are from the prev block, and marks them that way in the database rather then rejecting them to the miner.

This way the users have 0 rejected shares, and they feel good!

Thanks again for all your work, and i'll be following all of your updates

[Jine]

So you're actually lying to your users, about their amount of shares?  
Or are you paying for those invalid shares as well?

/ Jim

EDIT: I feel rather bad about that, to be honest.

[MrSam]

So you're actually lying to your users, about their amount of shares? 
Or are you paying for those invalid shares as well?

Well the idea of pushpool shares at difficulty 1 is that it serves as proof of work, and i think that it is a proof of work, it's not their fault that they submit work that has become invalid.

So yes, i accept this type 'recentstale' for payout.

Why feel bad about this ? proof of work is proof of work ?

[talpan]

Hello,

I've patched the official .23 with patch 4.
so far so good, no errors during compiling.
But when i start bitcoind the cpu spikes after 40-50s through the roof, without using it.

regards, talpan

[Jine]

So yes, i accept this type 'recentstale' for payout.

So i would be able to resend the same stale-share, over and over again? Or just keep sending stales - it doesn't help the pool at ALL, but i get paid more?
No seriously, I'm disappointed.

[MrSam]

So yes, i accept this type 'recentstale' for payout.

So i would be able to resend the same stale-share, over and over again? Or just keep sending stales - it doesn't help the pool at ALL, but i get paid more?
No seriously, I'm disappointed.

I never said that i accept stales .. only those specific recentstales ..

Happy disappointment day !

https://www.triplemining.com - Probably the best pool in the world

[JoelKatz]

Hello,

I've patched the official .23 with patch 4.
so far so good, no errors during compiling.
But when i start bitcoind the cpu spikes after 40-50s through the roof, without using it.

regards, talpan

Hmm, that's pretty strange. Can you confirm you're using version 0.95?

[talpan]

Hi,

Sorry I made a mistake, I'm using a custom .23 with a little patch.
Without this patch and with the patch from everythings fine.
But I can't figure out whats wrong with this patch:

Code:

diff -up orig/rpc.cpp new/rpc.cpp
--- orig/rpc.cpp
+++ new/rpc.cpp
@@ -184,6 +184,149 @@
 }
 
 
+Value BlockToValue(CBlock &block)
+{
+    Object obj;
+    obj.push_back(Pair("hash", block.GetHash().ToString().c_str()));
+    obj.push_back(Pair("version", block.nVersion));
+    obj.push_back(Pair("prev_block", block.hashPrevBlock.ToString().c_str()));
+    obj.push_back(Pair("mrkl_root", block.hashMerkleRoot.ToString().c_str()));
+    obj.push_back(Pair("time", (uint64_t)block.nTime));
+    obj.push_back(Pair("bits", (uint64_t)block.nBits));
+    obj.push_back(Pair("nonce", (uint64_t)block.nNonce));
+    obj.push_back(Pair("n_tx", (int)block.vtx.size()));
+    obj.push_back(Pair("size", (int)::GetSerializeSize(block, SER_NETWORK)));
+
+    Array tx;
+    for (int i = 0; i < block.vtx.size(); i++) {
+    	Object txobj;
+
+	txobj.push_back(Pair("hash", block.vtx[i].GetHash().ToString().c_str()));
+	txobj.push_back(Pair("version", block.vtx[i].nVersion));
+	txobj.push_back(Pair("lock_time", (uint64_t)block.vtx[i].nLockTime));
+	txobj.push_back(Pair("size",
+		(int)::GetSerializeSize(block.vtx[i], SER_NETWORK)));
+
+	Array tx_vin;
+	for (int j = 0; j < block.vtx[i].vin.size(); j++) {
+	    Object vino;
+
+	    Object vino_outpt;
+
+	    vino_outpt.push_back(Pair("hash",
+	    	block.vtx[i].vin[j].prevout.hash.ToString().c_str()));
+	    vino_outpt.push_back(Pair("n", (uint64_t)block.vtx[i].vin[j].prevout.n));
+
+	    vino.push_back(Pair("prev_out", vino_outpt));
+
+	    if (block.vtx[i].vin[j].prevout.IsNull())
+	    	vino.push_back(Pair("coinbase", HexStr(
+			block.vtx[i].vin[j].scriptSig.begin(),
+			block.vtx[i].vin[j].scriptSig.end(), false).c_str()));
+	    else
+	    	vino.push_back(Pair("scriptSig", 
+			block.vtx[i].vin[j].scriptSig.ToString().c_str()));
+	    if (block.vtx[i].vin[j].nSequence != UINT_MAX)
+	    	vino.push_back(Pair("sequence", (uint64_t)block.vtx[i].vin[j].nSequence));
+
+	    tx_vin.push_back(vino);
+	}
+
+	Array tx_vout;
+	for (int j = 0; j < block.vtx[i].vout.size(); j++) {
+	    Object vouto;
+
+	    vouto.push_back(Pair("value",
+	    	(double)block.vtx[i].vout[j].nValue / (double)COIN));
+	    vouto.push_back(Pair("scriptPubKey", 
+		block.vtx[i].vout[j].scriptPubKey.ToString().c_str()));
+
+	    tx_vout.push_back(vouto);
+	}
+
+	txobj.push_back(Pair("in", tx_vin));
+	txobj.push_back(Pair("out", tx_vout));
+
+	tx.push_back(txobj);
+    }
+
+    obj.push_back(Pair("tx", tx));
+
+    Array mrkl;
+    for (int i = 0; i < block.vMerkleTree.size(); i++)
+    	mrkl.push_back(block.vMerkleTree[i].ToString().c_str());
+
+    obj.push_back(Pair("mrkl_tree", mrkl));
+
+    return obj;
+}
+
+
+Value getblockbycount(const Array& params, bool fHelp)
+{
+    if (fHelp || params.size() != 1)
+        throw runtime_error(
+            "getblockbycount height\n"
+            "Dumps the block existing at specified height");
+
+    int64 height = params[0].get_int64();
+    if (height > nBestHeight)
+        throw runtime_error(
+            "getblockbycount height\n"
+            "Dumps the block existing at specified height");
+
+    string blkname = strprintf("blk%d", height);
+
+    CBlockIndex* pindex;
+    bool found = false;
+
+    for (map<uint256, CBlockIndex*>::iterator mi = mapBlockIndex.begin();
+         mi != mapBlockIndex.end(); ++mi)
+    {
+    	pindex = (*mi).second;
+	if ((pindex->nHeight == height) && (pindex->IsInMainChain())) {
+		found = true;
+		break;
+	}
+    }
+
+    if (!found)
+        throw runtime_error(
+            "getblockbycount height\n"
+            "Dumps the block existing at specified height");
+
+    CBlock block;
+    block.ReadFromDisk(pindex);
+    block.BuildMerkleTree();
+
+    return BlockToValue(block);
+}
+
+
+Value getblockbyhash(const Array& params, bool fHelp)
+{
+    if (fHelp || params.size() != 1)
+        throw runtime_error(
+            "getblockbyhash hash\n"
+            "Dumps the block with specified hash");
+
+    uint256 hash;
+    hash.SetHex(params[0].get_str());
+
+    map<uint256, CBlockIndex*>::iterator mi = mapBlockIndex.find(hash);
+    if (mi == mapBlockIndex.end())
+        throw JSONRPCError(-18, "hash not found");
+
+    CBlockIndex* pindex = (*mi).second;
+
+    CBlock block;
+    block.ReadFromDisk(pindex);
+    block.BuildMerkleTree();
+
+    return BlockToValue(block);
+}
+
+
 Value getconnectioncount(const Array& params, bool fHelp)
 {
     if (fHelp || params.size() != 0)
@@ -1423,6 +1566,8 @@
 {
     make_pair("help",                  &help),
     make_pair("stop",                  &stop),
+    make_pair("getblockbycount",       &getblockbycount),
+    make_pair("getblockbyhash",        &getblockbyhash),
     make_pair("getblockcount",         &getblockcount),
     make_pair("getblocknumber",        &getblocknumber),
     make_pair("getconnectioncount",    &getconnectioncount),
@@ -2113,6 +2258,7 @@
         if (strMethod == "listtransactions"       && n > 1) ConvertTo<boost::int64_t>(params[1]);
         if (strMethod == "listtransactions"       && n > 2) ConvertTo<boost::int64_t>(params[2]);
         if (strMethod == "listaccounts"           && n > 0) ConvertTo<boost::int64_t>(params[0]);
+        if (strMethod == "getblockbycount"        && n > 0) ConvertTo<boost::int64_t>(params[0]);
         if (strMethod == "sendmany"               && n > 1)
         {
             string s = params[1].get_str();

regards, talpan

[JoelKatz]

That patch looks safe to me. I don't think that explains your issue.

[talpan]

I can't find the problem.
I've compiled it many times from scratch and every time the same result.
Can you test it on your end?

[Matt Corallo]

Some of them may not, some of them might. Once I'm convinced this code is stable, I can work on getting some of them into the main distribution. I think the hub mode changes and native long polling could get in. The RPC turbocharge and keepalive fixes could.

Some of the stuff in the latest diff might get in too. Stashing the RPC user/pass, avoiding re-processing partial blocks, faster base64 decode, and faster bin2hex all might get in. (Change 3diff to 4diff to see them.)

Some code should never get in. The JSON bypass code is awful. Perhaps a change to using Jansson is possible. But that's a pretty big change.

1) Micro-optimizations
Havent had much of a chance to really look through these, but some of them look pull-able others maybe not, but I haven't looked too closely.

2) Native Long Polling Support
This one I'm not sure about, its a useful feature to be sure, but I'm not sure about pulling yet.  When the client codebase gets cleaned up a ton, features like this could very well be pulled, but IMHO not so much yet.

3) A fix from Luke Dash Jr.
Almost certainly can be pulled.

4) Hub Mode
No, this is really not a good change for several reasons.  A. the percent of nodes accepting incoming connections is already low enough, encouraging people to start making a ridiculous number of outgoing connections and filling up the few nodes who do accept incoming connection's connections slots is not a good idea at all.  B. Not even pool operators should be using this, a pool bitcoind should be accepting incoming connections, and if you do that, you will get plenty of connections really fast.  If you want to make sure your blocks are more likely to get accepted quicker, find out the IPs of other large miner nodes and addnode those instead of just blindly making a ton of connections as this patch does.

5) RPC Turbocharge
Absolutely, there is already a asio patch for this waiting as a pull request, so some version of asio/keepalive support/threaded RPC will absolutely be merged at some point in the not-too-distant future.

6) Resource Leak Fix
Absolutely, thanks somebadger for the pull.

[JoelKatz]

No, this is really not a good change for several reasons.  A. the percent of nodes accepting incoming connections is already low enough, encouraging people to start making a ridiculous number of outgoing connections and filling up the few nodes who do accept incoming connection's connections slots is not a good idea at all.

That's not a point I had considered. I think I may make some adjustments.

B. Not even pool operators should be using this, a pool bitcoind should be accepting incoming connections, and if you do that, you will get plenty of connections really fast.

Yes, but from lots of small nodes. That doesn't help you very much.

If you want to make sure your blocks are more likely to get accepted quicker, find out the IPs of other large miner nodes and addnode those instead of just blindly making a ton of connections as this patch does.

Won't those generally be the nodes that are accepting incoming connections?

Thanks for the feedback. You've given me a few ideas for ways to make this work better.