mining: Add HTTP notify support for DATUM Gateway#307
Conversation
pdath
force-pushed
the
blocknotify-http
branch
6 times, most recently
from
cbb5a51 to
265e8d6
Compare
When using the DATUM gateway you can use the blocknotify command in Bitcoin Knots. An example is: blocknotify=wget -q -O /dev/null http://localhost:7152/NOTIFY This works by shelling out and running wget. I have been profiling the performance of this, and on my slow hardware (Odroid M2 16GB), this takes about 75ms. I expect a lot of home miners running DATUM Gateway will also have low-spec hardware. This is a problem because from the moment that Bitcoin Knots has accepted a new block, any energy spent by a hasher is wasted until DATUM Gateway can get the notification of the new block, issue a getblocktemplate() RPC-JSON call to Bitcoin Knots, get the result, and send a new work unit to the hasher to start working on a new job. Another issue is that the longer it takes for a hasher to get the new block template the more it reduces their chance of solving that block template, simply because of the reduced hashing time working on it. To resolve the latency issue of shelling out I proposed to add a native http notifier in this discussion: bitcoinknots#305 I took onboard the feedback and have changed NotifyBlockTip to check the blocknotify configuration option to see if it starts with http:// and if it does, use the new code in http_notify.cpp (and http_notify.h) to make a single-shot attempt to retrieve the URL, otherwise it runs the existing code which shells out to run the command. Just like normal blocknotify it is run in a thread so is non-blocking. A sample notify command for DATUM Gateway is: blocknotify=http://localhost:7152/NOTIFY When I timed the native http notification I found it was generated in the same microsecond (note, microsecond) that Bitcoin Knots detected the new block tip (compared to 75ms when using the shell method). I have created a unit test httpnotify_test.cpp. I have kept the changes to init.cpp minimal to help reviewers see the structure of the change easier. I have tried to make the changes to the central Bitcoin Knots code minimal. I normally work in C, and have used AI to convert the concepts I wanted to implement into C++. I have access to an Ubuntu build environment and have tested it there. I have not been able to test this on MacOS or Windows. This is my first contribution to Bitcoin Knots. Take it easy on me. :-)
kwsantiago
left a comment
kwsantiago
left a comment
There was a problem hiding this comment.
Two robustness blockers in the socket path, plus a gating inconsistency. The URL parser itself is solid (bounds-checked, no OOB). Details inline.
| #endif | ||
|
|
||
| // Connect | ||
| if (connect(sock, ai_result->ai_addr, ai_result->ai_addrlen) == SOCKET_ERROR) { |
There was a problem hiding this comment.
SO_SNDTIMEO does not bound connect() on Linux, it only applies to send/recv, not the TCP handshake. A blocking connect to an unreachable/firewalled host follows tcp_syn_retries (~127s). Since each block tip spawns a detached thread (init.cpp), a down gateway leaves a thread + fd hung for ~2min per block, and worse on reorgs. That defeats the non-blocking goal of the PR.
Recommend routing through the existing Sock / ConnectDirectly() in netbase.cpp, which already does a non-blocking connect bounded by nConnectTimeout and walks the addrinfo list, fixes this and the next comment together.
| } | ||
|
|
||
| // Create TCP socket | ||
| SOCKET sock = socket(ai_result->ai_family, ai_result->ai_socktype, ai_result->ai_protocol); |
There was a problem hiding this comment.
Only the first getaddrinfo result is tried; ai_next is never walked. With AF_UNSPEC, localhost usually resolves to ::1 first, if IPv6 is disabled on the miner's box the notify is silently dropped even though 127.0.0.1 would work, which is the exact local DATUM setup this targets. Loop over the addrinfo list until one connects.
There was a problem hiding this comment.
Thank you for your valuable feedback. All addresses are now tried till one of them works.
| { | ||
| if (strCommand.empty()) return; | ||
|
|
||
| if (strCommand.starts_with("http://")) { |
There was a problem hiding this comment.
This runs before the #if HAVE_SYSTEM guard, so http:// notify now works on builds compiled without system() , a capability those builds intentionally lacked. But the other hooks (-shutdownnotify/-startupnotify/-alertnotify/-walletnotify) are still wrapped in #if HAVE_SYSTEM, so their http:// paths silently do nothing. Gate uniformly, or scope the help text to blocknotify only.
There was a problem hiding this comment.
I have updated "bitcoind --help".
Note that I have removed the example bitcoin.conf and man page, as Luke said he builds these himself during a release.
| @@ -0,0 +1,604 @@ | |||
| // Copyright (c) 2024 The Bitcoin Core developers | |||
There was a problem hiding this comment.
Tests cover ParseHttpUrl/BuildHttpGetRequest only; the HttpNotify socket path (connect/timeout/addrinfo/send), the platform-dependent part flagged as untested on Mac/Windows, has no coverage. A loopback test (bind 127.0.0.1:0, assert the GET bytes received) plus a connection-refused case that must return promptly would catch the connect-timeout issue above.
There was a problem hiding this comment.
I have added several additional tests.
This addresses four issues in the HTTP block-notification feature: - Fix unbounded connect() on Linux by using non-blocking connect with nConnectTimeout (via Sock::SetNonBlocking, Sock::Wait) - Try all addrinfo entries in order instead of only the first, allowing fallback from IPv6 to IPv4 when the first address family is unavailable - Clarify -blocknotify help text to indicate http:// support is independent of shell command availability on HAVE_SYSTEM builds, and is the only supported form on builds without system() - Add Doxygen documentation for all public functions in httpnotify.h and socket-level tests (loopback listener, connection refused) to httpnotify_tests.cpp Additionally: - Clean up implementation comments to be more descriptive - Generated man page updated to reflect help text changes
5 participants
When using the DATUM gateway you can use the blocknotify command in Bitcoin Knots. An example is:
blocknotify=wget -q -O /dev/null http://localhost:7152/NOTIFY
This works by shelling out and running wget. I have been profiling the performance of this, and on my slow hardware (Odroid M2 16GB), this takes about 75ms. I expect a lot of home miners running DATUM Gateway will also have low-spec hardware.
This is a problem because from the moment that Bitcoin Knots has accepted a new block, any energy spent by a hasher is wasted until DATUM Gateway can get the notification of the new block, issue a getblocktemplate() RPC-JSON call to Bitcoin Knots, get the result, and send a new work unit to the hasher to start working on a new job.
Another issue is that the longer it takes for a hasher to get the new block template the more it reduces their chance of solving that block template, simply because of the reduced hashing time working on it.
To resolve the latency issue of shelling out I proposed to add a native http notifier in this discussion:
#305
I took onboard the feedback and have changed NotifyBlockTip to check the blocknotify configuration option to see if it starts with http:// and if it does, use the new code in http_notify.cpp (and http_notify.h) to make a single-shot attempt to retrieve the URL, otherwise it runs the existing code which shells out to run the command. Just like normal blocknotify it is run in a thread so is non-blocking.
A sample notify command for DATUM Gateway is:
blocknotify=http://localhost:7152/NOTIFY
When I timed the native http notification I found it was generated in the same microsecond (note, microsecond) that Bitcoin Knots detected the new block tip (compared to 75ms when using the shell method).
I have created a unit test httpnotify_test.cpp.
I have kept the changes to init.cpp minimal to help reviewers see the structure of the change easier. I have tried to make the changes to the central Bitcoin Knots code minimal.
I normally work in C, and have used AI to convert the concepts I wanted to implement into C++. I have access to an Ubuntu build environment and have tested it there. I have not been able to test this on MacOS or Windows.
This is my first contribution to Bitcoin Knots. Take it easy on me. :-)