getblocktemplate: longpolling support

[luke-jr]

Built on top of #936, this adds support for getblocktemplate longpolling to bitcoind.

[jgarzik]

ACK on longpolling support in general. Long polling turns out to be a useful way to avoid http callbacks, with all the authentication that that entails.

1. it is ugly and fragile to unlock, cv, then relock. Disappointing and would be nice if there were a better solution (note: that is not a NAK)

2. does not pay attention to fShutdown

3. "BIP22 compliance" smells more like self-promotion than a critically required bitcoind feature

[jgarzik]

ACK longpolling support

Change appears mostly ACK-worthy. I worry about adding a new lock deep inside SetBestChain though.

[BitcoinPullTester]

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/f1d42a05fe27d14f258ec5e1b15774dad583d458 for binaries and test log.

[BitcoinPullTester]

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/650ea32bbd60ac149809333131bd887537afa477 for binaries and test log.

[luke-jr]

Rebased

[jgarzik]

Re-rebase requested, now that BIP 22 is merged

[luke-jr]

done

[jgarzik]

1. It accesses hashBestChain outside of locks.

2. pointless wrapping inside do..while(0) block

3. BIP 22 just says "longpollid" is a unique identifier. This code treats it as a block hash, not a job id. Thus, this change seems to hardcode unspoken assumptions about the longpollid.

4. The code does not seem to notice TCP disconnections. Surely you do not want the thread to continue waiting for a new block, if the TCP connection is gone?

[luke-jr]

1. Where?

2. That exists so it can be break'd out of. I suppose it could probably work just as well with yet another nested if, though.

3. "longpollid" is unique per long poll event; bitcoind only has such events when a new block is found, so the previous block hash is a fair fit. Clarified BIP 22 on the nature of "longpollid"'s uniqueness.

4. Good idea, but I'm not sure how practical it is to do portably. Any suggestions?

[BitcoinPullTester]

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/01e0e197ebb96cb14971c672b5704306e3ad0f1f for binaries and test log.

[luke-jr]

Rebased without the while(0). I still don't see any hashBestChain accessing outside of locks, after looking over it again. With regard to TCP disconnects, I did look into this, but it seems not worth the effort considering:

1. boost has no way to detect the socket being closed without reading
2. it would violate the current layer abstraction we have in the RPC implementation
3. while this is a problem for pools (eg, pushpool) with unreliable network clients, bitcoind's RPC is only guaranteed to be usable from localhost, where it's unlikely to occur
4. a few stale sockets/threads that go away every new block shouldn't harm the daemon much

Thoughts?

[jgarzik]

No code objections.

The main question remaining on this, our oldest pullreq: do we want/need it?

[jeremysawicki]

I tested this a while ago and did not find it suitable for deployment as is.

1. The long polling only returns when a new block is found. Ideally it should also return periodically to update the set of transactions. (Do we really want to encourage mining without including a reasonably up-to-date set of transactions?)
2. It doesn't handle application shutdown, so an open long polling request can prevent shutdown.

[luke-jr]

Err, no I didn't :(