The original function was already naturally split into two chunks:
First, we parse and validate the users' RPC configuration for IPs and
ports. Next we bind libevent's http server to the appropriate
endpoints.
This commit splits these chunks into two separate functions, leaving
the argument parsing in the common space of the module and moving the
libevent-specific binding into the http_libevent namespace.
A future commit will implement http_bitcoin::HTTPBindAddresses to
bind the validate list of endpoints by the new HTTP server.
The original function is passed to libevent as a callback when HTTP
requests are received and processed. It wrapped the libevent request
object in a http_libevent::HTTPRequest and then handed that off to
bitcoin for basic checks and finally dispatch to worker threads.
In this commit we split the function after the
http_libevent::HTTPRequest is created, and pass that object to a new
function that maintains the logic of checking and dispatching.
This will be the merge point for http_libevent and http_bitcoin,
where HTTPRequest objects from either namespace have the same
downstream lifecycle.
This is a refactor to prepare for matching the API of HTTPRequest
definitions in both namespaces http_bitcoin and http_libevent. In
particular, to provide a consistent return type for GetRequestMethod()
in both classes.
See https://www.rfc-editor.org/rfc/rfc7230#section-6.3.2
> A server MAY process a sequence of pipelined requests in
parallel if they all have safe methods (Section 4.2.1 of [RFC7231]),
but it MUST send the corresponding responses in the same order that
the requests were received.
We choose NOT to process requests in parallel. They are executed in
the order recevied as well as responded to in the order received.
This prevents race conditions where old state may get sent in response
to requests that are very quick to process but were requested later on
in the queue.
Sockets-touching bits copied and adapted from `CConnman::SocketSendData()`
Testing this requires adding a new feature to the SocketTestingSetup,
returning the DynSock I/O pipes from the mock socket so the received
data can be checked.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
`SocketHandlerConnected()` adapted from CConnman
Testing this requires adding a new feature to the SocketTestingSetup,
inserting a "request" payload into the mock client that connects
to us.
This commit also moves IOErrorIsPermanent() from sock.cpp to sock.h
so it can be called from the socket handler in httpserver.cpp
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Socket handling methods are copied from CConnMan:
`CConnman::GenerateWaitSockets()`
`CConnman::SocketHandlerListening()`
`CConnman::ThreadSocketHandler()` and `CConnman::SocketHandler()` are combined into ThreadSocketHandler()`.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
Introduce a new low-level socket managing class `HTTPServer`.
BindAndStartListening() was copied from CConnMan's BindListenPort()
in net.cpp and modernized.
Unit-test it with a new class `SocketTestingSetup` which mocks
`CreateSock()` and will enable mock client I/O in future commits.
Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
This commit is a no-op to isolate HTTP methods and objects that
depend on libevent. Following commits will add replacement objects
and methods in a new namespace for testing and review before
switching over the server.
726b3663cc http: properly respond to HTTP request during shutdown (furszy)
59d24bd5dd threadpool: make Submit return Expected instead of throwing (furszy)
Pull request description:
Fixes#34573.
As mentioned in https://github.com/bitcoin/bitcoin/issues/34573#issuecomment-3891596958, the ThreadPool PR (#33689) revealed an existing issue.
Before that PR, we were returning an incorrect error "Request rejected because http work queue depth exceeded" during shutdown for unhandled requests (we were not differentiating between "queue depth exceeded" and "server interrupted" errors). Now, with the ThreadPool inclusion, we return the proper error but we don't handle it properly.
This PR improves exactly that. Handling the missing error and properly returning it to the user.
The race can be reproduced as follows:
1) The server receives an http request.
2) Processing of the request is delayed, and shutdown is triggered in the meantime.
3) During shutdown, the libevent callback is unregistered and the threadpool interrupted.
4) The delayed request (step 2) resumes and tries to submit a task to the now-interrupted server.
Reproduction test can be found https://github.com/bitcoin/bitcoin/pull/34577#issuecomment-3902672521.
Also, to prevent this kind of issue from happening again, this PR changes task submission
to return the error as part of the function's return value using `util::Expected` instead of
throwing the exception. Unlike exceptions, which require extra try-catch blocks and can be
ignored, returning `Expected` forces callers to explicitly handle failures, and attributes
like `[[nodiscard]]` allow us catch unhandled ones at compile time.
ACKs for top commit:
achow101:
ACK 726b3663cc
sedited:
ACK 726b3663cc
pinheadmz:
re-ACK 726b3663cc
andrewtoth:
ACK 726b3663cc
hodlinator:
re-ACK 726b3663cc
Tree-SHA512: ef026e299adde1148c9fc575e7d937e957bf0ddedfc1cf081941b568736417c2eefcd8bc8c8aea795d7347040ed05da4371bddcdbda7d385e04bf4dc8d875780
Makes sure we respond to the client as the HTTP request attempts to submit a task to
the thread pool during server shutdown.
Roughly what happens:
1) The server receives an HTTP request and starts calling http_request_cb().
2) Meanwhile on another thread, shutdown is triggered which calls InterruptHTTPServer()
and unregisters libevent http_request_cb() callback and interrupts the thread pool.
3) The request (step 1) resumes and tries to submit a task to the now-interrupted server.
This fix detects failed submissions immediately, and the server responds with
HTTP_SERVICE_UNAVAILABLE.
38fd85c676 http: replace WorkQueue and threads handling for ThreadPool (furszy)
c323f882ed fuzz: add test case for threadpool (TheCharlatan)
c528dd5f8c util: introduce general purpose thread pool (furszy)
6354b4fd7f tests: log node JSON-RPC errors during test setup (furszy)
45930a7941 http-server: guard against crashes from unhandled exceptions (furszy)
Pull request description:
This has been a recent discovery; the general thread pool class created for #26966, cleanly
integrates into the HTTP server. It simplifies init, shutdown and requests execution logic.
Replacing code that was never unit tested for code that is properly unit and fuzz tested.
Although our functional test framework extensively uses this RPC interface (that’s how
we’ve been ensuring its correct behavior so far - which is not the best).
This clearly separates the responsibilities:
The HTTP server now focuses solely on receiving and dispatching requests, while ThreadPool handles
concurrency, queuing, and execution.
This will also allows us to experiment with further performance improvements at the task queuing and
execution level, such as a lock-free structure or task prioritization or any other implementation detail
like coroutines in the future, without having to deal with HTTP code that lives on a different layer.
Note:
The rationale behind introducing the ThreadPool first is to be able to easily cherry-pick it across different
working paths. Some of the ones that are benefited from it are #26966 for the parallelization of the indexes
initial sync, #31132 for the parallelization of the inputs fetching procedure, #32061 for the libevent replacement,
the kernel API #30595 (https://github.com/bitcoin/bitcoin/pull/30595#discussion_r2413702370) to avoid blocking validation among others use cases not publicly available.
Note 2:
I could have created a wrapper around the existing code and replaced the `WorkQueue` in a subsequent
commit, but it didn’t seem worth the extra commits and review effort. The `ThreadPool` implements
essentially the same functionality in a more modern and cleaner way.
ACKs for top commit:
Eunovo:
ReACK 38fd85c676
sedited:
Re-ACK 38fd85c676
pinheadmz:
ACK 38fd85c676
Tree-SHA512: a0330e54ed504330ca874c42d4e318a909f548b2fb9ac46db8badf5935b9eec47dc4ed503d1b6f98574418e3473420ea45f60498be05545c4325cfa89dcca689
Replace the HTTP server's WorkQueue implementation and single threads
handling code with ThreadPool for processing HTTP requests. The
ThreadPool class encapsulates all this functionality on a reusable
class, properly unit and fuzz tested (the previous code was not
unit nor fuzz tested at all).
This cleanly separates responsibilities:
The HTTP server now focuses solely on receiving and dispatching requests,
while ThreadPool handles concurrency, queuing, and execution.
It simplifies init, shutdown and requests tracking.
This also allows us to experiment with further performance improvements at
the task queuing and execution level, such as a lock-free structure, task
prioritization or any other performance improvement in the future, without
having to deal with HTTP code that lives on a different layer.
Currently, if an exception is thrown at the top-level HTTP request
handler (prior to invoking the command), the program crashes.
Ideally, each handler should catch all exceptions internally and
be responsible for sanitizing them and crafting the client response.
This is because only the handler knows the correct response format,
which differs per server type. However, because this cannot always
be guaranteed, it is safer to also catch exceptions in the top-level
server code, log the unexpected error, and disconnect the socket.
This both guards against crashes caused by uncaught exceptions and
prevents the client from hanging indefinitely while waiting for a
response that will never arrive.
The following diff can be used to trigger the crash in master
(just run single node functional tests like feature_shutdown.py):
```
diff --git a/src/httprpc.cpp b/src/httprpc.cpp
--- a/src/httprpc.cpp
+++ b/src/httprpc.cpp
@@ -103,6 +103,9 @@
static bool HTTPReq_JSONRPC(const std::any& context, HTTPRequest* req)
{
+ static int i = 0; // skip initial requests as they are used in the RPC warmup phase.
+ if (i++ > 3) throw std::runtime_error("error from json rpc handler");
+
// JSONRPC handles only POST
if (req->GetRequestMethod() != HTTPRequest::POST) {
req->WriteReply(HTTP_BAD_METHOD, "JSONRPC server handles only POST requests");
```
Note:
This leaves a TODO in the code because error responses should eventually
be specialized per server type. REST clients expect plain text responses,
while JSON-RPC clients expect a JSON error object.
The TODO is there because this is not consistently enforced everywhere
in the current codebase, and we should tackle them all at once.
fad7bd9ba3 noui: Remove always empty caption while formatting (MarcoFalke)
fa8ebeb332 refactor: [gui] Document that the title is always empty for node message (MarcoFalke)
fafe71b743 refactor: Remove empty caption from ThreadSafeMessageBox (MarcoFalke)
fa8d0088e7 refactor: Remove empty caption from ThreadSafeQuestion (MarcoFalke)
fa0195499c refactor: [gui] Use lambdas over std::bind (MarcoFalke)
eeee1e341f refactor: Remove trailing semicolon after ADD_SIGNALS_DECL_WRAPPER (MarcoFalke)
Pull request description:
Currently, the user interface (noui, gui) has a caption for each message. However, the caption has many issues:
* It is always hard-coded to the empty string.
* This is confusing and tedious when reading or maintaining the code.
* It is redundant, because `noui` will ignore the caption and set the logging prefix (error, warning, info) based on the `style`.
* The gui does prefer to set the title based on the caption, but since it the caption is always empty, the fallback will always be used.
Fix all issues by removing it.
ACKs for top commit:
hebasto:
ACK fad7bd9ba3, I have reviewed the code and it looks OK. Tested on Ubuntu 25.10.
sedited:
ACK fad7bd9ba3
Tree-SHA512: 58ef538b9b3e1cfdcf2955f6de9b8cee335edbf6339723cb693cb4d584817904c962dac5199ee44d7e2860a5332dec1a6abf47e621eb5cf919aa1cdae271b55f
faa59b3679 util: Add Expected::swap() (MarcoFalke)
fabb47e4e3 util: Implement Expected::operator*()&& (MarcoFalke)
fab9721430 util: Implement Expected::value()&& and Expected::error()&& (MarcoFalke)
fac4800959 util: Add Expected<void, E> specialization (MarcoFalke)
fa6575d6c2 util: Make Expected::value() throw (MarcoFalke)
fa1de1103f util: Add Unexpected::error() (MarcoFalke)
faa109f8be test: refactor: Use BOOST_CHECK_EQUAL over BOOST_CHECK == (MarcoFalke)
fad4a9fe2b Set bugprone-unused-return-value.AllowCastToVoid (MarcoFalke)
Pull request description:
Reviewers requested more member functions In https://github.com/bitcoin/bitcoin/pull/34006.
They are currently unused, but bring the port closer to the original `std::expected` implementation:
* Make `Expected::value()` throw when no value exists
* Add `Unexpected::error()` methods
* Add `Expected<void, E>` specialization
* Add `Expected::value()&&` and `Expected::error()&&` methods
* Add `Expected::swap()`
Also, include a tiny tidy fixup:
* tidy: Set `AllowCastToVoid` in the `bugprone-unused-return-value` check
ACKs for top commit:
stickies-v:
re-ACK faa59b3679
ryanofsky:
Code review ACK faa59b3679. Thanks for the update. The commit I objected to is fixed now and the rest of the implementation seems good enough for code that's probably temporary.
hodlinator:
re-ACK faa59b3679
Tree-SHA512: b6ac28c1e7241837d9db83fe7534d713ca1283c20a77d2273743157d329f041ec0b503658d14b2f4425211808b61a88fed115d77149e0546825acd3bd9198edf
The caption was empty for all call-sites, so this refactor does not
change any behavior.
Note that noui_ThreadSafeMessageBoxRedirect is test-only, so no end-user
behavior is changed here.
Avoids ratelimiting unconditional log statements when debug logging
is enabled. Introduces slight behaviour change by removing
the category from unconditional logs, making them more uniform
with the other unconditional logs in the codebase.
Also, in a slight behavior change, prefix the info-level (and higher)
messages with "libevent:".
This requires some small refactors to silence false-positive warnings.
Also, expand the bugprone-unused-return-value.CheckedReturnTypes option
to include util::Result, and util::Expected.
These methods in the Sock class wrap corresponding syscalls,
accepting void* arguments and casting to char* internally, which is
needed for Windows support and ignored on other platforms because
the syscall itself accepts void*:
Send()
Recv()
GetSockOpt()
SetSockOpt()
12ff4be9c7 test: ensure -rpcallowip is compatible with RFC4193 (Matthew Zipkin)
c02bd3c187 config: Explain RFC4193 and CJDNS interaction in help and init error (Matthew Zipkin)
f728b6b111 init: Configure reachable networks before we start the RPC server (Matthew Zipkin)
Pull request description:
Closes https://github.com/bitcoin/bitcoin/issues/32433
`MaybeFlipIPv6toCJDNS()` relies on `g_reachable_nets` to distinguish between CJDNS addresses and other IPv6 addresses. In particular, [RFC4193](https://www.rfc-editor.org/rfc/rfc4193#section-3.1) address or "Unique Local Address" with the L-bit unset also begins with the `fc` prefix. #32433 highlights a use case for these addresses that have nothing to do with CJDNS.
On master we don't parse init flags like `-cjdnsreachable` until *after* the HTTP server has started, causing conflicts with `-rpcallowip` because CJDNS doesn't support subnets.
This PR ensures that `NET_CJDNS` is only present in the reachable networks list if set by `-cjdnsreachable` *before* `-rpcallowip` is checked. If it is set all `fc` addresses are assumed to be CJDNS, can not have subnets, and can't be set for `-rpcallowip`.
I also noted this specific parameter interaction in the init help as well as the error message if configured incorrectly.
This can be tested locally:
`bitcoind -regtest -rpcallowip=fc00:dead:beef::/64 -rpcuser=u -rpcpassword=p`
On master this will just throw an error that doesn't even mention IPv6 at all.
On the branch, this will succeed and can be tested by adding the ULA to a local interface.
On linux: `sudo ip -6 addr add fc00:dead:beef::1/64 dev lo`
On macos: `sudo ifconfig lo0 inet6 fc00:dead:beef::1/128 add`
then: `curl -v -g -6 --interface fc00:dead:beef::1 u:p@[::1]:18443 --data '{"method":"getblockcount"}'`
If the `rpcallowip` option is removed, the RPC request will fail to authorize.
Finally, adding `-cjdnsreachable` to the start up command will throw an error and specify the incompatibility:
> RFC4193 is allowed only if -cjdnsreachable=0.
ACKs for top commit:
achow101:
ACK 12ff4be9c7
tapcrafter:
tACK 12ff4be9c7
ryanofsky:
Code review ACK 12ff4be9c7
willcl-ark:
ACK 12ff4be9c7
Tree-SHA512: a4dd70ca2bb9f6ec2c0a9463fd73985d1ed80552c674a9067ac9a86662d1c018cc275ba757cebb2993c5f3971ecf4778b95d35fe7a7178fb41b1d18b601c9960
We don't add or maintain these, and they are of little value, as
well as having the effect of polluting diffs.
They are also wrong, i.e DEFAULT_SCRIPTCHECK_THREADS is not in
validation.h.
Treat specifying -norpcbind and -norpcallowip the same as not specifying
-rpcbind or -rpcallowip, instead of failing to bind to localhost and failing to
show warnings.
Also add code comment to clarify what intent of existing code is.
This makes code more consistent and makes it easier to add compile-time checking to
enforce that format strings contain the right specifiers, because it stops
using Untranslated() to create the format string, so the Untranslated()
function will not need to get involved in formatting.
-BEGIN VERIFY SCRIPT-
quote='"[^"]+"'
quotes="(?:$quote|\\s)*"
nonparens="[^()]*"
single_level_paren="\($nonparens\)"
double_level_paren="\($nonparens\($nonparens\)$nonparens\)"
exprs="(?:$double_level_paren|$single_level_paren|$nonparens)*"
git grep -l 'Untranslated' | xargs perl -0777 -i -pe "s/strprintf\((\\W*)Untranslated\(($quotes)\)($exprs)(\))/Untranslated(\1strprintf(\2\3))/gs"
-END VERIFY SCRIPT-
Adds invalid rpcbind port checking to
`HTTPBindAddresses()`. While movement of
`CheckHostPortOptions()` in the previous
commit handles rcpbind port errors, updating
`HTTPBindAddresses()` port checking adds
a defensive measure for potential future
changes.
03d49d0f25 http: set TCP_NODELAY when creating HTTP server (Roman Zeyde)
Pull request description:
Otherwise, the default HTTP server config may result in high latency, due to Nagle's algorithm (on the server) and delayed ACK (on the client):
[1] https://www.extrahop.com/blog/tcp-nodelay-nagle-quickack-best-practices
[2] https://eklitzke.org/the-caveats-of-tcp-nodelay
Without the fix, fetching a small block takes ~40ms (when connection keep-alive is enabled):
```
$ ab -k -c 1 -n 100 http://localhost:8332/rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin
Server Software:
Server Hostname: localhost
Server Port: 8332
Document Path: /rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin
Document Length: 25086 bytes
Concurrency Level: 1
Time taken for tests: 4.075 seconds
Complete requests: 100
Failed requests: 0
Keep-Alive requests: 100
Total transferred: 2519200 bytes
HTML transferred: 2508600 bytes
Requests per second: 24.54 [#/sec] (mean)
Time per request: 40.747 [ms] (mean)
Time per request: 40.747 [ms] (mean, across all concurrent requests)
Transfer rate: 603.76 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 0 41 4.1 41 42
Waiting: 0 0 0.1 0 1
Total: 0 41 4.1 41 42
Percentage of the requests served within a certain time (ms)
50% 41
66% 41
75% 41
80% 41
90% 42
95% 42
98% 42
99% 42
100% 42 (longest request)
```
With the fix, it takes ~0.2ms:
```
$ ab -k -c 1 -n 1000 http://localhost:8332/rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin
Benchmarking localhost (be patient)
Completed 100 requests
Completed 200 requests
Completed 300 requests
Completed 400 requests
Completed 500 requests
Completed 600 requests
Completed 700 requests
Completed 800 requests
Completed 900 requests
Completed 1000 requests
Finished 1000 requests
Server Software:
Server Hostname: localhost
Server Port: 8332
Document Path: /rest/block/00000000000002b5898f7cdc80d9c84e9747bc6b9388cc989971d443f05713ee.bin
Document Length: 25086 bytes
Concurrency Level: 1
Time taken for tests: 0.194 seconds
Complete requests: 1000
Failed requests: 0
Keep-Alive requests: 1000
Total transferred: 25192000 bytes
HTML transferred: 25086000 bytes
Requests per second: 5147.05 [#/sec] (mean)
Time per request: 0.194 [ms] (mean)
Time per request: 0.194 [ms] (mean, across all concurrent requests)
Transfer rate: 126625.50 [Kbytes/sec] received
Connection Times (ms)
min mean[+/-sd] median max
Connect: 0 0 0.0 0 0
Processing: 0 0 0.0 0 0
Waiting: 0 0 0.0 0 0
Total: 0 0 0.0 0 0
Percentage of the requests served within a certain time (ms)
50% 0
66% 0
75% 0
80% 0
90% 0
95% 0
98% 0
99% 0
100% 0 (longest request)
```
ACKs for top commit:
achow101:
ACK 03d49d0f25
theStack:
re-ACK 03d49d0f25
tdb3:
ACK 03d49d0f25
Tree-SHA512: bbf3d78b8521f569430850ec4315a75711303547df1a3de213a4ad34c9700105e374e0a649352fd05f8e4badb5b59debd3720e1c5d392c5113d7816648f7fcaa
