Replace libevent with our own HTTP and socket-handling implementation #32061

pull pinheadmz wants to merge 27 commits into bitcoin:master from pinheadmz:http-rewrite-13march2025 changing 23 files +2043 −648
  1. pinheadmz commented at 7:32 pm on March 13, 2025: member

    This is a major component of removing libevent as a dependency of the project, by replacing the HTTP server used for RPC and REST with one implemented entirely within the Bitcoin Core codebase. The new HTTPServer class runs its own I/O thread, handling socket connections with code based on #30988, but tailored specifically for HTTP.

    Some functional tests were added in #32408 to cover libevent behaviors that remain consistent with this branch.

    Commit strategy:

    • Implement a few helper functions for strings, timestamps, etc needed by HTTP protocol
    • Isolate the existing libevent-based HTTP server in a namespace http_libevent
    • Implement HTTP in a new namespace http_bitcoin (classes like HTTPRequest, HTTPClient, etc…)
    • Switch bitcoind from the libevent server to the new server
    • Clean up (delete http_libevent)

    I am currently seeing about a 10% speed up in the functional tests on my own arm/macos machine.

    Integration testing:

    I am testing the new HTTP server by forking projects that integrate with bitcoin via HTTP and running their integration tests with bitcoind built from this branch (on Github actions). I will continue adding integrations over time, and re-running these CI tests as this branch gets rebased:

  2. DrahtBot commented at 7:32 pm on March 13, 2025: contributor

    The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

    Code Coverage & Benchmarks

    For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32061.

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK laanwj, fjahr, w0xlt, furszy, hodlinator, theStack
    Approach ACK vasild
    Stale ACK romanz

    If your review is incorrectly listed, please copy-paste <!–meta-tag:bot-skip–> into the comment that the bot should ignore.

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #32297 (bitcoin-cli: Add -ipcconnect option by ryanofsky)
    • #26812 (test: add end-to-end tests for CConnman and PeerManager by vasild)
    • #26022 (Add util::ResultPtr class by ryanofsky)
    • #25665 (refactor: Add util::Result failure types and ability to merge result values by ryanofsky)

    If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

    LLM Linter (✨ experimental)

    Possible places where named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

    • mock_client_socket_pipes->send.GetBytes(buf, sizeof(buf), 0) in test/httpserver_tests.cpp

    2026-03-13 19:58:30

  3. pinheadmz marked this as a draft on Mar 13, 2025
  4. DrahtBot added the label CI failed on Mar 13, 2025
  5. DrahtBot commented at 8:29 pm on March 13, 2025: contributor

    🚧 At least one of the CI tasks failed. Debug: https://github.com/bitcoin/bitcoin/runs/38735177073

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  6. laanwj added the label RPC/REST/ZMQ on Mar 14, 2025
  7. laanwj commented at 12:52 pm on March 14, 2025: member
    Concept ACK, nice work
  8. vasild commented at 1:57 pm on March 14, 2025: contributor
    Concept ACK
  9. fjahr commented at 8:29 pm on March 14, 2025: contributor

    Concept ACK

    My understanding from the low-level networking discussion at CoreDev was that this wouldn’t build on top of sockman. I guess the devil is in the details but can you address that in what sense the current approach follows what was discussed there? Thanks!

  10. pinheadmz commented at 0:03 am on March 15, 2025: member

    My understanding from the low-level networking discussion at CoreDev was that this wouldn’t build on top of sockman. I guess the devil is in the details but can you address that in what sense the current approach follows what was discussed there? Thanks!

    Sure, by coredev I had already written most of this implementation (based on sockman) but the performance was bad, and that was part of the motivation behind the deep-dive talk. However, by the end of the week I had reviewed that code in person with smart attendees and not only improved the performance of my code but started to improve performance vs master branch as well! Those updates came in the days just after the deep-dive discussion.

    SOME kind of sockman is needed to replace libevent. The one @vasild wrote does actually seem to work well for this purpose as well as for p2p, and it would be “nice” to only have to maintain one I/O loop structure in bitcoind. @theuni is investigating how a sockman for http could be optimized if it had no other purpose, and I think that is the kind of feedback that will help us decide which path to take.

  11. pinheadmz force-pushed on Mar 18, 2025
  12. pinheadmz force-pushed on Mar 18, 2025
  13. vasild commented at 12:22 pm on March 19, 2025: contributor

    SOME kind of sockman is needed to replace libevent … it would be “nice” to only have to maintain one I/O loop structure in bitcoind.

    :100:

  14. pinheadmz force-pushed on Mar 19, 2025
  15. DrahtBot added the label Needs rebase on Mar 20, 2025
  16. pinheadmz force-pushed on Mar 20, 2025
  17. pinheadmz force-pushed on Mar 20, 2025
  18. DrahtBot removed the label Needs rebase on Mar 20, 2025
  19. pinheadmz force-pushed on Mar 21, 2025
  20. DrahtBot added the label Needs rebase on Mar 24, 2025
  21. pinheadmz force-pushed on Mar 29, 2025
  22. pinheadmz force-pushed on Mar 29, 2025
  23. DrahtBot removed the label CI failed on Mar 30, 2025
  24. DrahtBot removed the label Needs rebase on Mar 30, 2025
  25. pinheadmz force-pushed on Mar 31, 2025
  26. pinheadmz commented at 10:18 am on March 31, 2025: member

    I rebased this branch on a single squashed commit from #30988 essentially just cherry-picking sockman.{h,cpp} by @vasild and leaving out the p2p refactor. This will make rebase maintenance on master a lot easier by reducing conflicting scope, and hopefully also makes review easier. It also means to some extent this PR can be merged independently of #30988, and also gives @theuni some room to rewrite a specific HTTP sockman if a more efficient purpose-focused module can be written. (Will update PR description in a moment)

    I’ve finally gotten all CI to pass so I’m going to mark this PR as ready for review as I move on to integration testing with all the bitcoin client libraries I can find!

  27. pinheadmz marked this as ready for review on Mar 31, 2025
  28. pinheadmz renamed this:
    [draft] Replace libevent with our own HTTP and socket-handling implementation
    Replace libevent with our own HTTP and socket-handling implementation
    on Mar 31, 2025
  29. pinheadmz force-pushed on Apr 3, 2025
  30. pinheadmz commented at 7:14 pm on April 3, 2025: member
    rebase to a225633e34 includes a new test for “pipelining” HTTP requests (thanks @theuni for pointing out this oversight) and also adds a queue of requests to each HTTPClient to ensure requests are processed in series, in the order they were received.
  31. pinheadmz force-pushed on Apr 3, 2025
  32. DrahtBot added the label CI failed on Apr 3, 2025
  33. DrahtBot removed the label CI failed on Apr 4, 2025
  34. w0xlt commented at 7:06 pm on April 17, 2025: contributor
    Concept ACK.
  35. in src/common/sockman.cpp:396 in cfe5eba446 outdated
    391+        LogPrintf("connection from %s dropped: non-selectable socket\n", them.ToStringAddrPort());
    392+        return;
    393+    }
    394+
    395+    // According to the internet TCP_NODELAY is not carried into accepted sockets
    396+    // on all platforms.  Set it again here just to be sure.
    


    romanz commented at 5:44 am on April 21, 2025:
    nit: this comment seems to be incorrect, since TCP_NODELAY is only set once by SockMan.

    pinheadmz commented at 1:53 pm on April 28, 2025:

    This comment may need to be applied upstream in #30988

    FWIW, the Sockman backend might not be what we end up going with, as @theuni and I are considering an alternative.

  36. in src/util/time.cpp:127 in aeb8352a9d outdated
    118@@ -116,6 +119,20 @@ std::optional<int64_t> ParseISO8601DateTime(std::string_view str)
    119     return int64_t{TicksSinceEpoch<std::chrono::seconds>(tp)};
    120 }
    121 
    122+std::string FormatRFC7231DateTime(int64_t nTime)
    123+{
    124+    const std::chrono::sys_seconds secs{std::chrono::seconds{nTime}};
    125+    const auto days{std::chrono::floor<std::chrono::days>(secs)};
    126+    // 1970-01-01 was a Thursday
    127+    std::string weekday{weekdays[(days.time_since_epoch().count() + 4) % 7]};
    


    romanz commented at 5:49 am on April 21, 2025:
    nit: weekday and month can be a string_view.

    pinheadmz commented at 5:38 pm on April 28, 2025:
    done
  37. in src/util/strencodings.cpp:503 in a1e151c774 outdated
    496@@ -497,3 +497,10 @@ std::optional<uint64_t> ParseByteUnits(std::string_view str, ByteUnit default_mu
    497     }
    498     return *parsed_num * unit_amount;
    499 }
    500+
    501+std::vector<std::byte> StringToBuffer(const std::string& str)
    502+{
    503+    return std::vector<std::byte>(
    


    romanz commented at 6:03 am on April 21, 2025:

    nit: maybe using std::span will be simpler? For example:

    0    auto span = std::as_bytes(std::span(str));
    1    return {span.begin(), span.end()};
    

    pinheadmz commented at 3:38 pm on April 28, 2025:
    Great thanks, done.
  38. in src/util/string.cpp:30 in a1e151c774 outdated
    25+
    26+    auto line_start = it;
    27+    std::string line{};
    28+    while (it != end) {
    29+        char c = static_cast<char>(*it);
    30+        line += c;
    


    romanz commented at 6:33 am on April 21, 2025:

    Maybe it would be better (performance-wise) to create the string after the loop is over?

     0std::optional<std::string> LineReader::ReadLine()
     1{
     2    if (it == end) {
     3        return std::nullopt;
     4    }
     5
     6    auto line_start = it;
     7    size_t count = 0;
     8    while (it != end) {
     9        char c = static_cast<char>(*it);
    10        ++it;
    11        ++count;
    12        if (c == '\n') break;
    13        if (count >= max_read) throw std::runtime_error("max_read exceeded by LineReader");
    14    }
    15    const std::byte *data = &*line_start;
    16    std::string line{reinterpret_cast<const char *>(data), count};
    17    line = TrimString(line); // delete trailing \r and/or \n
    18    return line;
    19}
    

    pinheadmz commented at 3:43 pm on April 28, 2025:
    awesome, done
  39. in src/util/string.cpp:36 in a1e151c774 outdated
    31+        ++it;
    32+        if (c == '\n') break;
    33+        if ((size_t)std::distance(line_start, it) >= max_read) throw std::runtime_error("max_read exceeded by LineReader");
    34+    }
    35+
    36+    line = TrimString(line); // delete trailing \r and/or \n
    


    romanz commented at 6:35 am on April 21, 2025:
    Can we return a string_view to the internal buffer? (preventing allocation & copy)

    pinheadmz commented at 5:31 pm on April 28, 2025:
    by internal buffer I assume you mean m_recv_buffer which im trying to copy from into the HTTPRequest object, then clear the buffer. I left another response about maybe changing the type of that buffer to reduce unnecessary copies, but am going to leave it alone for this current rebase. What I did here though was swap out TrimString() with TrimStringView() so at least that’s one less.
  40. in src/util/string.cpp:45 in a1e151c774 outdated
    40+// Ignores max_read but won't overflow
    41+std::string LineReader::ReadLength(size_t len)
    42+{
    43+    if (len == 0) return "";
    44+    if (Left() < len) throw std::runtime_error("Not enough data in buffer");
    45+    std::string out(reinterpret_cast<const char*>(&(*it)), len);
    


    romanz commented at 6:37 am on April 21, 2025:
    Can we return a string_view to the internal buffer? (preventing allocation & copy)

    pinheadmz commented at 5:36 pm on April 28, 2025:
    I think I addressed this already by taking your suggestion here https://github.com/bitcoin/bitcoin/pull/32061/files#r2052024440

    romanz commented at 10:15 am on May 3, 2025:

    7d30118401 seems to still return a std::string:

    0$ git grep ReadLength
    1src/util/string.cpp:  std::string LineReader::ReadLength(size_t len)
    2src/util/string.h:    std::string ReadLength(size_t len);
    

    pinheadmz commented at 1:43 pm on May 9, 2025:
    Oh I see I misunderstood. For now I want LineReader to return new string objects because it reads directly from the HTTPClient’s m_recv_buffer which is then erased. I know we can prevent that copy by being more clever with m_recv_buffer and I still plan on exploring that, maybe as a follow up? It’s nice and simple now and there is a clean separation between the socket layer receiving data and the HTTPRequest

    romanz commented at 1:46 pm on May 9, 2025:
    Sounds good, thanks for the context :)
  41. in src/httpserver.cpp:787 in 70d003ca10 outdated
    780@@ -781,3 +781,69 @@ void UnregisterHTTPHandler(const std::string &prefix, bool exactMatch)
    781         pathHandlers.erase(i);
    782     }
    783 }
    784+
    785+
    786+namespace http_bitcoin {
    787+std::optional<std::string> HTTPHeaders::Find(const std::string key) const
    


    romanz commented at 6:48 am on April 21, 2025:
    nit: consider using string_view instead of string here and below.

    pinheadmz commented at 7:42 pm on April 28, 2025:
    done.

    romanz commented at 10:23 am on May 3, 2025:
    nit: key & value can also be a string_view (here and below)

    pinheadmz commented at 1:48 pm on May 9, 2025:
    Cool will address this nit on next rebase thanks!

    vasild commented at 5:25 pm on May 14, 2025:

    Bikeshed, feel free to ignore.

    It is redundant to have “http” in both the namespace and in the class name: http_bitcoin::HTTPHeaders::Find(). Also no need to suffix anything with _bitcoin - if we would do that, then we would have to have e.g. namespace common_bitcoin, namespace i2p_bitcoin, namespace node_bitcoin etc. So http_bitcoin::HTTPHeaders::Find() could be shortened to http::Headers::Find().


    pinheadmz commented at 6:09 pm on May 14, 2025:
    Happy to change the names, I just needed to separate the old libevent server from the new one in the PR in the series of commits where each namespace has classes like HTTPRequest
  42. in src/httpserver.cpp:840 in 70d003ca10 outdated
    835+}
    836+
    837+std::string HTTPHeaders::Stringify() const
    838+{
    839+    std::string out;
    840+    for (auto it = m_map.begin(); it != m_map.end(); ++it) {
    


    romanz commented at 4:11 pm on April 21, 2025:

    nit: maybe the following would be simpler?

    0    for (const auto& [k, v] : m_map) {
    1        out += k + ": " + v + "\r\n";
    2    }
    

    pinheadmz commented at 7:41 pm on April 28, 2025:
    thanks I’ll take it!
  43. in src/httpserver.cpp:920 in 482382bd14 outdated
    915+    if (!req->LoadBody(reader)) return false;
    916+
    917+    // Remove the bytes read out of the buffer.
    918+    // If one of the above calls throws an error, the caller must
    919+    // catch it and disconnect the client.
    920+    m_recv_buffer.erase(
    


    romanz commented at 8:49 am on April 27, 2025:
    nit: maybe it’s better to return a std::span from m_recv_buffer to avoid a copy here.

    pinheadmz commented at 3:21 pm on April 28, 2025:
    we’d still want to erase the data after fulfilling the request though (if I’m understanding your comment correctly). Would it make more sense if m_recv_buffer was a std::deque or something where we can truncate the structure without copying/moving/shifting left?

    romanz commented at 5:53 am on April 30, 2025:

    Would it make more sense if m_recv_buffer was a std::deque or something where we can truncate the structure without copying/moving/shifting left?

    Sounds good :)

  44. in src/httpserver.cpp:944 in 73c3c2e3d3 outdated
    940@@ -940,6 +941,92 @@ bool HTTPRequest::LoadBody(LineReader& reader)
    941     }
    942 }
    943 
    944+void HTTPRequest::WriteReply(HTTPStatusCode status, std::span<const std::byte> reply_body)
    


    romanz commented at 9:02 am on April 27, 2025:
    Would it be possible to allow writing a reply composed of several span<byte>? It should allow sending while serializing a response object.

    pinheadmz commented at 2:33 pm on April 28, 2025:
    Could this be done in a followup? I actually just checked #30321 to see why we didn’t do this back then – I remember when that was authored and merged but I didn’t realize you were the author!

    romanz commented at 5:54 am on April 30, 2025:
    Sure, thanks :)
  45. in src/httpserver.cpp:1014 in 73c3c2e3d3 outdated
    1009+        m_client->m_send_buffer.insert(m_client->m_send_buffer.end(), headers_bytes.begin(), headers_bytes.end());
    1010+
    1011+        // We've been using std::span up until now but it is finally time to copy
    1012+        // data. The original data will go out of scope when WriteReply() returns.
    1013+        // This is analogous to the memcpy() in libevent's evbuffer_add()
    1014+        m_client->m_send_buffer.insert(m_client->m_send_buffer.end(), reply_body.begin(), reply_body.end());
    


    romanz commented at 9:04 am on April 27, 2025:
    Maybe use a list of buffers (instead a single FIFO buffer)?

    pinheadmz commented at 3:06 pm on April 28, 2025:
    I don’t understand – would that be an optimization? It’s not really a FIFO, since we insert at the end of the buffer here but send data from the begin() of the buffer in SendBytesFromBuffer()

    romanz commented at 5:57 am on April 30, 2025:

    I suggest considering a similar approach to libevent’s evbuffers:

    evbuffers are represented using a linked list of memory chunks, with pointers to the first and last chunk in the chain.

    https://libevent.org/doc/buffer_8h.html


    romanz commented at 6:45 am on May 1, 2025:
    Thinking about it, doing the copy here is definitely fine for now (we can optimize it in a following PR).
  46. in src/httpserver.h:275 in 73c3c2e3d3 outdated
    270+    // Response headers may be set in advance before response body is known
    271+    HTTPHeaders m_response_headers;
    272+    void WriteReply(HTTPStatusCode status, std::span<const std::byte> reply_body = {});
    273+    void WriteReply(HTTPStatusCode status, const char* reply_body) {
    274+        auto reply_body_view = std::string_view(reply_body);
    275+        std::span<const std::byte> byte_span(reinterpret_cast<const std::byte*>(reply_body_view.data()), reply_body_view.size());
    


    romanz commented at 9:16 am on April 27, 2025:

    nit: can be simplified using

    0    void WriteReply(HTTPStatusCode status, std::string_view reply_body_view)
    1    {
    2        WriteReply(status, std::as_bytes(std::span{reply_body_view}));
    3    }
    

    pinheadmz commented at 3:36 pm on April 28, 2025:
    awesome thanks
  47. in src/httpserver.h:137 in 6a6285d268 outdated
    170+    {
    171+        auto reply_body_view = std::string_view(reply_body);
    172+        std::span<const std::byte> byte_span(reinterpret_cast<const std::byte*>(reply_body_view.data()), reply_body_view.size());
    173+        WriteReply(status, byte_span);
    174+    }
    175+    void WriteReply(HTTPStatusCode status, const std::string& reply_body)
    


    romanz commented at 9:20 am on April 27, 2025:
    Can be removed if we’ll add WriteReply(HTTPStatusCode status, std::string_view reply_body_view) (see above).

    pinheadmz commented at 3:36 pm on April 28, 2025:
    👍
  48. in src/httpserver.cpp:246 in 90761d5026 outdated
    242@@ -243,18 +243,18 @@ static bool InitHTTPAllowList()
    243 }
    244 
    245 /** HTTP request method as string - use for logging only */
    246-std::string RequestMethodString(HTTPRequest::RequestMethod m)
    247+std::string RequestMethodString(HTTPRequestMethod m)
    


    romanz commented at 9:43 am on April 27, 2025:

    nit:

    0std::string_view RequestMethodString(HTTPRequestMethod m)
    

    pinheadmz commented at 1:59 pm on April 28, 2025:
    done
  49. in src/httpserver.cpp:980 in b828fa1e29 outdated
    975+    if (end == std::string::npos) {
    976+        end = decoded_uri.length();
    977+    }
    978+    const std::string query{decoded_uri.substr(start + 1, end - start - 1)};
    979+    // find requested parameter in query
    980+    const std::vector<std::string> params{SplitString(query, "&")};
    


    romanz commented at 9:44 am on April 27, 2025:
    nit: wouldn’t it copy the substrings?

    pinheadmz commented at 2:12 pm on April 28, 2025:
    You’re right thanks, I haven’t been dilligent about using string_view when copying isn’t needed. Fixed that here and also in HTTPRequest::LoadControlData()
  50. in src/httpserver.cpp:994 in b828fa1e29 outdated
    989+        }
    990+    }
    991+    return std::nullopt;
    992+}
    993+
    994+std::pair<bool, std::string> HTTPRequest::GetHeader(const std::string& hdr) const
    


    romanz commented at 9:45 am on April 27, 2025:

    nit:

    0std::pair<bool, std::string_view> HTTPRequest::GetHeader(const std::string& hdr) const
    

    pinheadmz commented at 2:28 pm on April 28, 2025:
    thanks, for this fix I added a new commit instead of a fixup. The reason is because in the “story” of the commits, GetHeader() is already defined and used by the legacy HTTP server returning a string, and the goal at this point is to match the exiting API. But we can optimize it after the big “switch HTTP servers” commit.
  51. romanz commented at 1:00 pm on April 27, 2025: contributor
    LGTM, many thanks! Added some comments/suggestions above :)
  52. pinheadmz force-pushed on Apr 29, 2025
  53. pinheadmz commented at 1:23 pm on April 29, 2025: member
    Thanks so much for the great review @romanz I took most of your suggestions, there’s still an open question about m_recv_buffer but I really appreciate all your attention to string copying!
  54. romanz commented at 7:54 am on May 3, 2025: contributor
  55. DrahtBot requested review from laanwj on May 3, 2025
  56. DrahtBot requested review from fjahr on May 3, 2025
  57. DrahtBot requested review from vasild on May 3, 2025
  58. in test/functional/interface_http.py:132 in 7d30118401 outdated
    127+        req2 += body2
    128+        # Get the underlying socket from HTTP connection so we can send something unusual
    129+        conn = http.client.HTTPConnection(urlNode2.hostname, urlNode2.port)
    130+        conn.connect()
    131+        sock = conn.sock
    132+        sock.settimeout(1)
    


    vasild commented at 7:41 am on May 15, 2025:
    1 second timeout to send or receive seems more than enough for local testing on a dev machine. However, CI virtual machines sometimes are surprisingly slow. To avoid unnecessary test failures maybe it would be better to have this be 5 or 10 seconds for the sendall() calls and then set to 1 for the recv() call which we expect to timeout.

    pinheadmz commented at 2:27 pm on May 18, 2025:
    I should have mentioned the initial test commits have been split off into #32408 and the test as written here failed CI, so has been modified with rpcservertimeout=2 and then expects a timeout between 1 and 4 seconds

    pinheadmz commented at 7:04 pm on June 17, 2025:
    Addressed in #32408
  59. in test/functional/interface_http.py:140 in 7d30118401 outdated
    135+        sock.sendall(req2.encode("utf-8"))
    136+        try:
    137+            # The server should not respond to the fast, second request
    138+            # until the (very) slow first request has been handled:
    139+            res = sock.recv(1024)
    140+            assert not res
    


    vasild commented at 7:53 am on May 15, 2025:

    Shouldn’t this be assert False? Here the expectation is that the recv() will throw an exception due to timeout.

    https://docs.python.org/3/library/socket.html#socket.socket.recv

    A returned empty bytes object indicates that the client has disconnected.

    An “empty bytes object” will not trigger the assert assert not res but if that happens (= disconnect) then the test should fail.

    suggestion:

    0-            assert not res
    1+            assert False
    

    pinheadmz commented at 7:04 pm on June 17, 2025:
    Addressed in #32408
  60. in test/functional/interface_http.py:217 in 7d30118401 outdated
    212+        try:
    213+            conn.request('GET', '/')
    214+            conn.getresponse()
    215+        #       macos/linux           windows
    216+        except (ConnectionResetError, ConnectionAbortedError):
    217+            pass
    


    vasild commented at 10:21 am on May 15, 2025:
    This will also pass if no exception is thrown. Either add assert False after line 214 or have a boolean variable to false before the try and set it to true inside except and assert that it is true afterwards.

    pinheadmz commented at 7:04 pm on June 17, 2025:
    Addressed in #32408
  61. in test/functional/interface_http.py:169 in 7d30118401 outdated
    164+            b'0A' * 1000000,
    165+            b'0B' * 1000000,
    166+            b'0C' * 1000000,
    167+            b'0D' * 1000000,
    168+            b'"]}'
    169+        ]
    


    vasild commented at 11:02 am on May 15, 2025:
    Is the intention here to send 8MB of data?

    pinheadmz commented at 7:04 pm on June 17, 2025:
    Addressed in #32408
  62. in test/functional/interface_http.py:179 in 7d30118401 outdated
    174+            url='/',
    175+            body=iter(body_chunked),
    176+            headers=headers_chunked,
    177+            encode_chunked=True)
    178+        out1 = conn.getresponse().read()
    179+        assert out1 == b'{"result":"high-hash","error":null}\n'
    


    vasild commented at 11:05 am on May 15, 2025:

    Here and elsewhere in the added tests, assert_equal() produces a better error message:

    assert (value of out1 is not printed):

    0    assert out1 == b'{"result":"high-hash","error":null}\n'
    1           ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    2AssertionError
    

    vs

    assert_equal():

    0AssertionError: not(b'{"result":null,"error":{"code":-32700,"message":"Parse error"},"id":null}\n' == b'{"result":"high-hash","error":null}\n')
    

    pinheadmz commented at 7:04 pm on June 17, 2025:
    Addressed in #32408
  63. in src/test/util_tests.cpp:1073 in 7d30118401 outdated
    1068+    BOOST_CHECK(!ParseUInt64Hex("", &n));
    1069+    BOOST_CHECK(!ParseUInt64Hex("-1", &n));
    1070+    BOOST_CHECK(!ParseUInt64Hex("10 00", &n));
    1071+    BOOST_CHECK(!ParseUInt64Hex("1 ", &n));
    1072+    BOOST_CHECK(!ParseUInt64Hex("0xAB", &n));
    1073+}
    


    vasild commented at 12:41 pm on May 15, 2025:

    Add some more test cases and avoid BOOST_CHECK(A && B) because when that fails it is unclear which one of A or B was false. Prefer BOOST_CHECK(A); BOOST_CHECK(B);. Also, because B is n == 123, do BOOST_CHECK_EQUAL(n, 123):

     0BOOST_AUTO_TEST_CASE(test_ParseUInt64Hex)
     1{
     2    uint64_t n;
     3    // Valid values
     4    BOOST_CHECK(ParseUInt64Hex("1234", nullptr));
     5    BOOST_CHECK(ParseUInt64Hex("1234", &n));
     6    BOOST_CHECK_EQUAL(n, 0x1234);
     7    BOOST_CHECK(ParseUInt64Hex("a", &n));
     8    BOOST_CHECK_EQUAL(n, 0xA);
     9    BOOST_CHECK(ParseUInt64Hex("0000000a", &n));
    10    BOOST_CHECK_EQUAL(n, 0xA);
    11    BOOST_CHECK(ParseUInt64Hex("100", &n));
    12    BOOST_CHECK_EQUAL(n, 0x100);
    13    BOOST_CHECK(ParseUInt64Hex("DEADbeef", &n));
    14    BOOST_CHECK_EQUAL(n, 0xDEADbeef);
    15    BOOST_CHECK(ParseUInt64Hex("FfFfFfFf", &n));
    16    BOOST_CHECK_EQUAL(n, 0xFfFfFfFf);
    17    BOOST_CHECK(ParseUInt64Hex("123456789", &n));                                      
    18    BOOST_CHECK_EQUAL(n, 0x123456789ULL);
    19    BOOST_CHECK(ParseUInt64Hex("0", &n));
    20    BOOST_CHECK_EQUAL(n, 0);
    21    BOOST_CHECK(ParseUInt64Hex("FfFfFfFfFfFfFfFf", &n));
    22    BOOST_CHECK_EQUAL(n, 0xFfFfFfFfFfFfFfFfULL);
    23    // Invalid values
    24    BOOST_CHECK(!ParseUInt64Hex("", &n));
    25    BOOST_CHECK(!ParseUInt64Hex("-1", &n));
    26    BOOST_CHECK(!ParseUInt64Hex("10 00", &n));
    27    BOOST_CHECK(!ParseUInt64Hex("1 ", &n));
    28    BOOST_CHECK(!ParseUInt64Hex("0xAB", &n));
    29    BOOST_CHECK(!ParseUInt64Hex("FfFfFfFfFfFfFfFf0", &n));
    30}
    

    pinheadmz commented at 7:09 pm on June 17, 2025:
    taken, thanks
  64. in src/util/strencodings.cpp:270 in 7d30118401 outdated
    265+    }
    266+    if (out != nullptr) {
    267+        *out = total;
    268+    }
    269+    return true;
    270+}
    


    vasild commented at 12:46 pm on May 15, 2025:

    This would not recognize numbers larger than FFFFFFFF (4 bytes, 8 chars). That should be if (str.size() > 16) return false; instead of 8.

    But better not roll our own and use std::from_chars() like the others:

     0diff --git i/src/util/strencodings.cpp w/src/util/strencodings.cpp
     1index d923bdd121..05a5dced62 100644
     2--- i/src/util/strencodings.cpp
     3+++ w/src/util/strencodings.cpp
     4@@ -199,21 +199,21 @@ std::optional<std::vector<unsigned char>> DecodeBase32(std::string_view str)
     5 
     6     return ret;
     7 }
     8 
     9 namespace {
    10 template <typename T>
    11-bool ParseIntegral(std::string_view str, T* out)
    12+bool ParseIntegral(std::string_view str, T* out, size_t base = 10)
    13 {
    14     static_assert(std::is_integral_v<T>);
    15     // Replicate the exact behavior of strtol/strtoll/strtoul/strtoull when
    16     // handling leading +/- for backwards compatibility.
    17     if (str.length() >= 2 && str[0] == '+' && str[1] == '-') {
    18         return false;
    19     }
    20-    const std::optional<T> opt_int = ToIntegral<T>((!str.empty() && str[0] == '+') ? str.substr(1) : str);
    21+    const std::optional<T> opt_int = ToIntegral<T>((!str.empty() && str[0] == '+') ? str.substr(1) : str, base);
    22     if (!opt_int) {
    23         return false;
    24     }
    25     if (out != nullptr) {
    26         *out = *opt_int;
    27     }
    28@@ -250,26 +250,13 @@ bool ParseUInt64(std::string_view str, uint64_t* out)
    29 {
    30     return ParseIntegral<uint64_t>(str, out);
    31 }
    32 
    33 bool ParseUInt64Hex(std::string_view str, uint64_t* out)
    34 {
    35-    if (str.size() > 8) return false;
    36-    if (str.size() < 1) return false;
    37-    uint64_t total{0};
    38-    auto it = str.begin();
    39-    while (it != str.end()) {
    40-        auto v = HexDigit(*(it++));
    41-        if (v < 0) return false;
    42-        total <<= 4;
    43-        total |= v;
    44-    }
    45-    if (out != nullptr) {
    46-        *out = total;
    47-    }
    48-    return true;
    49+    return ParseIntegral<uint64_t>(str, out, 16);
    50 }
    51 
    52 std::string FormatParagraph(std::string_view in, size_t width, size_t indent)
    53 {
    54     assert(width >= indent);
    55     std::stringstream out;
    56diff --git i/src/util/strencodings.h w/src/util/strencodings.h
    57index b95b21dafd..d83fa3c841 100644
    58--- i/src/util/strencodings.h
    59+++ w/src/util/strencodings.h
    60@@ -173,17 +173,17 @@ constexpr inline bool IsSpace(char c) noexcept {
    61  * is `-?[0-9]+`. The minus sign is only permitted for signed integer types.
    62  *
    63  * [@returns](/bitcoin-bitcoin/contributor/returns/) std::nullopt if the entire string could not be parsed, or if the
    64  *   parsed value is not in the range representable by the type T.
    65  */
    66 template <typename T>
    67-std::optional<T> ToIntegral(std::string_view str)
    68+std::optional<T> ToIntegral(std::string_view str, size_t base = 10)
    69 {
    70     static_assert(std::is_integral_v<T>);
    71     T result;
    72-    const auto [first_nonmatching, error_condition] = std::from_chars(str.data(), str.data() + str.size(), result);
    73+    const auto [first_nonmatching, error_condition] = std::from_chars(str.data(), str.data() + str.size(), result, base);
    74     if (first_nonmatching != str.data() + str.size() || error_condition != std::errc{}) {
    75         return std::nullopt;
    76     }
    77     return result;
    78 }
    79 
    

    pinheadmz commented at 7:39 pm on June 17, 2025:
    Due to #32520 I had to change this entire commit anyway. Took your suggestion about adding base argument here and just dropped the wrapper function. Will call ToIntegral( ... 16) directly when reading chunk size.
  65. vasild commented at 1:57 pm on May 15, 2025: contributor
    Posting review midway. Reviewed up to and including 144a777f86 string: add CaseInsensitiveComparator. I like that the PR starts with some new tests to enforce correct behavior.
  66. DrahtBot requested review from vasild on May 15, 2025
  67. in src/util/time.cpp:127 in 7d30118401 outdated
    118@@ -116,6 +119,20 @@ std::optional<int64_t> ParseISO8601DateTime(std::string_view str)
    119     return int64_t{TicksSinceEpoch<std::chrono::seconds>(tp)};
    120 }
    121 
    122+std::string FormatRFC7231DateTime(int64_t nTime)
    123+{
    124+    const std::chrono::sys_seconds secs{std::chrono::seconds{nTime}};
    125+    const auto days{std::chrono::floor<std::chrono::days>(secs)};
    126+    // 1970-01-01 was a Thursday
    127+    std::string_view weekday{weekdays[(days.time_since_epoch().count() + 4) % 7]};
    


    vasild commented at 12:13 pm on May 16, 2025:

    Passing -1717429609 would make this code try to access weekdays[-1] :fire:

    For modern style, better use std::array instead of C arrays and, more importantly, use the array’s at() method which has boundary checks, just in case. Here is a change that adds some more tests and fixes the out-of-bounds access:

     0diff --git i/src/test/util_tests.cpp w/src/test/util_tests.cpp
     1index 387493152d..ebb40dd713 100644
     2--- i/src/test/util_tests.cpp
     3+++ w/src/test/util_tests.cpp
     4@@ -384,15 +384,23 @@ BOOST_AUTO_TEST_CASE(util_FormatISO8601Date)
     5     BOOST_CHECK_EQUAL(FormatISO8601Date(0), "1970-01-01");
     6     BOOST_CHECK_EQUAL(FormatISO8601Date(1317425777), "2011-09-30");
     7 }
     8 
     9 BOOST_AUTO_TEST_CASE(util_FormatRFC7231DateTime)
    10 {
    11+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(std::numeric_limits<int64_t>::max()), "");
    12+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(253402300800), "");
    13+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(253402300799), "Fri, 31 Dec 9999 23:59:59 GMT");
    14     BOOST_CHECK_EQUAL(FormatRFC7231DateTime(253402214400), "Fri, 31 Dec 9999 00:00:00 GMT");
    15     BOOST_CHECK_EQUAL(FormatRFC7231DateTime(1717429609), "Mon, 03 Jun 2024 15:46:49 GMT");
    16     BOOST_CHECK_EQUAL(FormatRFC7231DateTime(0), "Thu, 01 Jan 1970 00:00:00 GMT");
    17+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(-1), "Wed, 31 Dec 1969 23:59:59 GMT");
    18+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(-1717429609), "Sat, 31 Jul 1915 08:13:11 GMT");
    19+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(-62167219200), "Sat, 01 Jan 0000 00:00:00 GMT");
    20+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(-62167219201), "");
    21+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(std::numeric_limits<int64_t>::min()), "");
    22 }
    23 
    24 BOOST_AUTO_TEST_CASE(util_FormatMoney)
    25 {
    26     BOOST_CHECK_EQUAL(FormatMoney(0), "0.00");
    27     BOOST_CHECK_EQUAL(FormatMoney((COIN/10000)*123456789), "12345.6789");
    28diff --git i/src/util/time.cpp w/src/util/time.cpp
    29index 9b9167d19a..c0f375956a 100644
    30--- i/src/util/time.cpp
    31+++ w/src/util/time.cpp
    32@@ -7,21 +7,22 @@
    33 
    34 #include <compat/compat.h>
    35 #include <tinyformat.h>
    36 #include <util/check.h>
    37 #include <util/strencodings.h>
    38 
    39+#include <array>
    40 #include <atomic>
    41 #include <chrono>
    42 #include <optional>
    43 #include <string>
    44 #include <string_view>
    45 #include <thread>
    46 
    47-static const std::string weekdays[7] = {"Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"};
    48-static const std::string months[12] = {"Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"};
    49+static constexpr std::array<std::string_view, 7> weekdays{"Thu", "Fri", "Sat", "Sun", "Mon", "Tue", "Wed"}; // 1970-01-01 was a Thursday.
    50+static constexpr std::array<std::string_view, 12> months{"Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"};
    51 
    52 void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); }
    53 
    54 static std::atomic<std::chrono::seconds> g_mock_time{}; //!< For testing
    55 std::atomic<bool> g_used_system_time{false};
    56 static std::atomic<std::chrono::milliseconds> g_mock_steady_time{}; //!< For testing
    57@@ -116,20 +117,24 @@ std::optional<int64_t> ParseISO8601DateTime(std::string_view str)
    58     }
    59     const auto time{std::chrono::hours{*hour} + std::chrono::minutes{*min} + std::chrono::seconds{*sec}};
    60     const auto tp{std::chrono::sys_days{ymd} + time};
    61     return int64_t{TicksSinceEpoch<std::chrono::seconds>(tp)};
    62 }
    63 
    64-std::string FormatRFC7231DateTime(int64_t nTime)
    65+std::string FormatRFC7231DateTime(int64_t time)
    66 {
    67-    const std::chrono::sys_seconds secs{std::chrono::seconds{nTime}};
    68+    if (time < -62167219200 || 253402300799 < time) {
    69+        // RFC7231 mandates 4-digit year, so only support years 0 to 9999
    70+        return "";
    71+    }
    72+    const std::chrono::sys_seconds secs{std::chrono::seconds{time}};
    73     const auto days{std::chrono::floor<std::chrono::days>(secs)};
    74-    // 1970-01-01 was a Thursday
    75-    std::string_view weekday{weekdays[(days.time_since_epoch().count() + 4) % 7]};
    76+    const auto w{days.time_since_epoch().count() % 7}; // will be in the range [-6, 6]
    77+    std::string_view weekday{weekdays.at(w >= 0 ? w : w + 7)};
    78     const std::chrono::year_month_day ymd{days};
    79-    std::string_view month{months[unsigned{ymd.month()} - 1]};
    80+    std::string_view month{months.at(unsigned{ymd.month()} - 1)};
    81     const std::chrono::hh_mm_ss hms{secs - days};
    82     // examples: Mon, 27 Jul 2009 12:28:53 GMT
    83     //           Fri, 31 May 2024 19:18:04 GMT
    84     return strprintf("%03s, %02u %03s %04i %02i:%02i:%02i GMT", weekday, unsigned{ymd.day()}, month, signed{ymd.year()}, hms.hours().count(), hms.minutes().count(), hms.seconds().count());
    85 }
    86 
    

    pinheadmz commented at 7:56 pm on June 17, 2025:
    ah really really great suggestion thanks, taken
  68. in src/httpserver.cpp:314 in 7d30118401 outdated
    327 
    328-bool InitHTTPServer(const util::SignalInterrupt& interrupt)
    329+namespace http_bitcoin {
    330+using util::Split;
    331+
    332+std::optional<std::string_view> HTTPHeaders::Find(const std::string key) const
    


    vasild commented at 1:40 pm on May 16, 2025:

    e95c6f5b6511ae35141b1e440e1f22e1004d3de6

    Copying strings is expensive:

    0std::optional<std::string_view> HTTPHeaders::Find(const std::string& key) const
    

    pinheadmz commented at 4:12 pm on June 19, 2025:
    👍
  69. in src/httpserver.cpp:321 in 7d30118401 outdated
    340 
    341-    // Redirect libevent's logging to our own log
    342-    event_set_log_callback(&libevent_log_cb);
    343-    // Update libevent's log handling.
    344-    UpdateHTTPServerLogging(LogInstance().WillLogCategory(BCLog::LIBEVENT));
    345+void HTTPHeaders::Write(const std::string key, const std::string value)
    


    vasild commented at 1:41 pm on May 16, 2025:
    0void HTTPHeaders::Write(const std::string& key, const std::string& value)
    

    pinheadmz commented at 4:29 pm on June 19, 2025:
    :+1:
  70. in src/httpserver.cpp:332 in 7d30118401 outdated
    356-#ifdef WIN32
    357-    evthread_use_windows_threads();
    358-#else
    359-    evthread_use_pthreads();
    360-#endif
    361+void HTTPHeaders::Remove(const std::string key)
    


    vasild commented at 1:43 pm on May 16, 2025:
    0void HTTPHeaders::Remove(const std::string& key)
    

    pinheadmz commented at 4:30 pm on June 19, 2025:
    :+1:
  71. in src/httpserver.h:70 in 7d30118401 outdated
     96+{
     97+public:
     98+    std::optional<std::string_view> Find(const std::string key) const;
     99+    void Write(const std::string key, const std::string value);
    100+    void Remove(const std::string key);
    101+    bool Read(util::LineReader& reader);
    


    vasild commented at 2:38 pm on May 16, 2025:
    Would be good to document when Read() throws, returns true and false.

    pinheadmz commented at 4:36 pm on June 19, 2025:
    sure will add
  72. vasild commented at 2:44 pm on May 16, 2025: contributor
    Posting review midway. Reviewed up to and including e95c6f5b6511ae35141b1e440e1f22e1004d3de6 http: Implement HTTPHeaders class
  73. DrahtBot requested review from vasild on May 16, 2025
  74. DrahtBot added the label Needs rebase on May 20, 2025
  75. in src/rpc/protocol.h:26 in 7d30118401 outdated
    19@@ -20,6 +20,20 @@ enum HTTPStatusCode
    20     HTTP_SERVICE_UNAVAILABLE   = 503,
    21 };
    22 
    23+// Copied from libevent http.c success_phrases[] and client_error_phrases[]
    24+// TODO: Should HTTPStatusCode and HTTPReason be moved since they are not RPC protocols?
    25+const std::map<HTTPStatusCode, std::string> HTTPReason{
    26+    {HTTP_OK, "OK"},
    


    vasild commented at 11:24 am on May 22, 2025:

    TODO: Should HTTPStatusCode and HTTPReason be moved since they are not RPC protocols?

    I think no because this file already contains generic HTTP constants, e.g. HTTP_SERVICE_UNAVAILABLE = 503 above.


    pinheadmz commented at 2:28 pm on June 18, 2025:
    I’m asking if both should be moved, including the generic HTTP constants. They are also used in rest.cpp which has to #include <rpc/protocol.h> even though REST is not RPC protocol. I’ll remove the comment for now.
  76. in src/httpserver.h:96 in 7d30118401 outdated
    129-private:
    130-    struct evhttp_request* req;
    131-    const util::SignalInterrupt& m_interrupt;
    132-    bool replySent;
    133+public:
    134+    std::string m_method;
    


    vasild commented at 1:20 pm on May 22, 2025:
    Maybe enum is better for this?

    pinheadmz commented at 2:38 pm on June 18, 2025:
    yeah good call, I will add that in to “define HTTP request methods at module level outside of class” (currently 7a1359064e326f2b28a1c57aaa94fdbe17f9dee2) which is a few commits ahead
  77. in src/test/httpserver_tests.cpp:303 in 7d30118401 outdated
    301+        HTTPRequest req;
    302+        std::vector<std::byte> buffer{StringToBuffer(bad_content_length)};
    303+        LineReader reader(buffer, MAX_HEADERS_SIZE);
    304+        BOOST_CHECK(req.LoadControlData(reader));
    305+        BOOST_CHECK(req.LoadHeaders(reader));
    306+        BOOST_CHECK_THROW(req.LoadBody(reader), std::runtime_error);
    


    vasild commented at 1:45 pm on May 22, 2025:

    It is fine as it is. I am just wondering if it is not more appropriate for LoadHeaders() to throw instead of succeeding because the header Content-Length: eleven is already not according to the spec: https://httpwg.org/specs/rfc9110.html#field.content-length:

    Content-Length = 1*DIGIT


    pinheadmz commented at 2:45 pm on June 18, 2025:
    Hm yeah I see your point, we don’t validate individual headers in LoadHeaders(). I’ll leave as is for now, for what it’s worth, libevent doesn’t appear to check this until evhttp_get_body_length() as well…
  78. in src/httpserver.cpp:462 in 7d30118401 outdated
    512+        if (!ParseUInt64(content_length_value.value(), &content_length)) throw std::runtime_error("Cannot parse Content-Length value");
    513+
    514+        // Not enough data in buffer for expected body
    515+        if (reader.Left() < content_length) return false;
    516+
    517+        m_body = reader.ReadLength(content_length);
    


    vasild commented at 2:13 pm on May 22, 2025:

    If the body comes in pieces, then this parsing will be repeated for every piece and the return on line 460 will be executed a few times until enough data is received. This is suboptimal - to parse the same thing multiple times. I guess it is fine because we are not building a top performance HTTP server.

    If this becomes an issue at some point, then it would be better to remember that it has been parsed until e.g. byte 456 and only continue parsing from there as new data is received over the socket. I guess this will complicate the current implementation significantly.


    pinheadmz commented at 6:40 pm on June 19, 2025:

    Yeah its true, a client could send a request with a huge list of HTTP headers and an incomplete body and we would parse the headers over and over again from m_recv_buffer until the body was finally completed, when we finally pinch off a HTTPRequest and erase the data from the buffer.

    There certainly is an optimization there, keeping an in-progress request attached to HTTPClient with a “where we left off” pointer. It sounds like you would be ok with seeing that in a follow-up PR but let me know if its blocking for you here.

  79. in src/test/httpserver_tests.cpp:353 in 7d30118401 outdated
    351+        accepted_sockets->Push(std::move(connected_socket));
    352+
    353+        // Prepare a request handler that just stores received requests so we can examine them
    354+        // Mutex is required to prevent a race between this test's main thread and the Sockman I/O loop.
    355+        Mutex requests_mutex;
    356+        std::deque<std::unique_ptr<HTTPRequest>> requests;
    


    vasild commented at 2:50 pm on May 22, 2025:
    0        Mutex requests_mutex;
    1        std::deque<std::unique_ptr<HTTPRequest>> requests GUARDED_BY(requests_mutex);
    

    pinheadmz commented at 2:48 pm on June 18, 2025:

    Compiler didn’t like this actually:

    warning: ‘guarded_by’ attribute only applies to non-static data members and global variables [-Wignored-attributes]

  80. in src/test/httpserver_tests.cpp:354 in 7d30118401 outdated
    352+
    353+        // Prepare a request handler that just stores received requests so we can examine them
    354+        // Mutex is required to prevent a race between this test's main thread and the Sockman I/O loop.
    355+        Mutex requests_mutex;
    356+        std::deque<std::unique_ptr<HTTPRequest>> requests;
    357+        auto StoreRequest = [&](std::unique_ptr<HTTPRequest> req) {
    


    vasild commented at 2:54 pm on May 22, 2025:
    Passing unique_ptr by value looks a bit strange since it is not supposed to be copied. I guess this works when the caller std::moves the object, but isn’t it more clear to use std::unique_ptr<HTTPRequest>&& req?

    pinheadmz commented at 3:03 pm on June 18, 2025:
    Agreed this better, thanks.
  81. vasild commented at 2:57 pm on May 22, 2025: contributor
    Posting review midway. Reviewed up to and including 980a9cd3d38abd0e0da85363056a2be6098d3919 http: read requests from connected clients.
  82. DrahtBot requested review from vasild on May 22, 2025
  83. in src/httpserver.cpp:419 in 7d30118401 outdated
    468+bool HTTPRequest::LoadBody(LineReader& reader)
    469+{
    470+    // https://httpwg.org/specs/rfc9112.html#message.body
    471+
    472+    auto transfer_encoding_header = m_headers.Find("Transfer-Encoding");
    473+    if (transfer_encoding_header && ToLower(transfer_encoding_header.value()) == "chunked") {
    


    vasild commented at 10:18 am on June 5, 2025:
    Would be good to have tests to exercise the chunked transfer encoding from d7778f426c http: support "chunked" Transfer-Encoding.

    pinheadmz commented at 3:22 pm on June 18, 2025:
    That was merged in #32408 unless you mean a unit test?
  84. in src/httpserver.cpp:711 in 7d30118401 outdated
    829+    if (CloseConnection(client->m_node_id)) {
    830+        LogDebug(BCLog::HTTP, "Disconnected HTTP client %s (id=%d)\n", client->m_origin, client->m_node_id);
    831+    } else {
    832+        LogDebug(BCLog::HTTP, "Failed to disconnect non-existent HTTP client %s (id=%d)\n", client->m_origin, client->m_node_id);
    833+    }
    834 }
    


    vasild commented at 11:16 am on June 5, 2025:

    The CloseConnectionInternal() method can and should take just a reference, no need for shared pointer:

    0- void HTTPServer::CloseConnectionInternal(std::shared_ptr<HTTPClient>& client)
    1+ void HTTPServer::CloseConnectionInternal(const HTTPClient& client)
    

    pinheadmz commented at 4:58 pm on June 19, 2025:
    yes thanks

    hodlinator commented at 9:57 am on March 10, 2026:

    in 06c66aaa695e0ee3fca3f06b0b09bc37b353553d “HTTPServer: start an I/O loop in a new thread and accept connections”: We again take a shared_ptr for HTTPServer::CloseConnection() when a const ref would do:

    0- bool HTTPServer::CloseConnection(std::shared_ptr<HTTPClient> http_client)
    1+ bool HTTPServer::CloseConnection(const HTTPClient& http_client)
    2  {
    3      size_t erased = std::erase_if(m_connected,
    4-                                   [&](auto& client){ return client == http_client; });
    5+                                   [&](auto& client){ return client.get() == &http_client; });
    
  85. in src/httpserver.h:180 in 7d30118401 outdated
    222+    std::atomic_bool m_send_ready{false};
    223+
    224+    // Set to true when we receive request data and set to false once m_send_buffer is cleared.
    225+    // Checked during DisconnectClients(). All of these operations take place in the Sockman I/O loop,
    226+    // however it may get set my a worker thread during an "optimistic send".
    227+    std::atomic_bool m_prevent_disconnect{false};
    


    vasild commented at 11:35 am on June 5, 2025:

    Would be good to note that this overrides m_disconnect:

    0// If set, then the client will not be disconnected even if `m_disconnect` is true.
    

    pinheadmz commented at 5:00 pm on June 19, 2025:
    :+1:
  86. in src/httpserver.h:182 in 7d30118401 outdated
    224+    // Set to true when we receive request data and set to false once m_send_buffer is cleared.
    225+    // Checked during DisconnectClients(). All of these operations take place in the Sockman I/O loop,
    226+    // however it may get set my a worker thread during an "optimistic send".
    227+    std::atomic_bool m_prevent_disconnect{false};
    228+
    229+    // Client request to keep connection open after all requests have been responded to.
    


    vasild commented at 11:36 am on June 5, 2025:

    nit:

    0    // Client requested to keep the connection open after all requests have been responded to.
    

    pinheadmz commented at 3:24 pm on June 18, 2025:
    👍
  87. in src/httpserver.cpp:899 in 7d30118401 outdated
    1134-        struct evkeyvalq params_q;
    1135-        evhttp_parse_query_str(query, &params_q);
    1136+bool InitHTTPServer(const util::SignalInterrupt& interrupt)
    1137+{
    1138+    if (!InitHTTPAllowList())
    1139+        return false;
    


    vasild commented at 12:45 pm on June 5, 2025:

    nit:

    0    if (!InitHTTPAllowList()) {
    1        return false;
    2    }
    

    pinheadmz commented at 4:43 pm on June 18, 2025:
    👍
  88. in src/httpserver.cpp:909 in 7d30118401 outdated
    1148+    g_http_server->m_rpcservertimeout = std::chrono::seconds(gArgs.GetIntArg("-rpcservertimeout", DEFAULT_HTTP_SERVER_TIMEOUT));
    1149+
    1150+    // Bind HTTP server to specified addresses
    1151+    std::vector<std::pair<std::string, uint16_t>> endpoints{GetBindAddresses()};
    1152+    bool bind_success{false};
    1153+    for (std::vector<std::pair<std::string, uint16_t> >::iterator i = endpoints.begin(); i != endpoints.end(); ++i) {
    


    vasild commented at 12:48 pm on June 5, 2025:
    0    for (std::vector<std::pair<std::string, uint16_t>>::iterator i = endpoints.begin(); i != endpoints.end(); ++i) {
    

    pinheadmz commented at 4:43 pm on June 18, 2025:
    👍
  89. in src/httpserver.cpp:914 in 7d30118401 outdated
    1153+    for (std::vector<std::pair<std::string, uint16_t> >::iterator i = endpoints.begin(); i != endpoints.end(); ++i) {
    1154+        LogPrintf("Binding RPC on address %s port %i\n", i->first, i->second);
    1155+        const std::optional<CService> addr{Lookup(i->first, i->second, false)};
    1156+        if (addr) {
    1157+            if (addr->IsBindAny()) {
    1158+                LogPrintf("WARNING: the RPC server is not safe to expose to untrusted networks such as the public internet\n");
    


    vasild commented at 12:49 pm on June 5, 2025:

    LogPrintf() is using the Info severity.

    0                LogWarning("The RPC server is not safe to expose to untrusted networks such as the public internet\n");
    

    pinheadmz commented at 4:44 pm on June 18, 2025:
    good catch thanks
  90. in src/httpserver.cpp:984 in 7d30118401 outdated
    1241+    if (g_http_server) {
    1242+        // Disconnect clients as their remaining responses are flushed
    1243+        g_http_server->m_disconnect_all_clients = true;
    1244+        // Wait for all disconnections
    1245+        while (!g_http_server->m_no_clients) {
    1246+            std::this_thread::sleep_for(std::chrono::milliseconds{50});
    


    vasild commented at 1:07 pm on June 5, 2025:

    nit:

    0            std::this_thread::sleep_for(50ms);
    

    pinheadmz commented at 4:49 pm on June 18, 2025:
    :+1:
  91. in src/httpserver.cpp:989 in 7d30118401 outdated
    1246+            std::this_thread::sleep_for(std::chrono::milliseconds{50});
    1247+        }
    1248+        // Break sockman I/O loop: stop accepting connections, sending and receiving data
    1249+        g_http_server->interruptNet();
    1250+        // Wait for sockman threads to exit
    1251+        g_http_server->JoinSocketsThreads();
    


    vasild commented at 1:09 pm on June 5, 2025:
    Could a new client sneak in after the loop while (!g_http_server->m_no_clients) exits? Shouldn’t that loop be after interruptNet() so that new clients cannot be accepted?

    pinheadmz commented at 5:00 pm on June 18, 2025:

    Hm that’s a good catch. I need the loop running to disconnect clients though – although I might be able to call HTTPServer::DisconnectClients() one extra time after interruptNet() to clean up edge cases like that.

    Otherwise I’m not sure if it’s even really a problem. We reject all requests in InterruptHTTPServer() which is called first, so the only risk on the server side would be if a lingering connection means JoinSocketsThreads() blocks forever.

  92. vasild commented at 2:02 pm on June 5, 2025: contributor
    Posting review midway. Reviewed up to and including b90f808e30 http: disconnect after idle timeout (-rpcservertimeout)
  93. DrahtBot requested review from vasild on Jun 5, 2025
  94. in src/httprpc.cpp:47 in 7d30118401 outdated
    48     {
    49-        struct timeval tv;
    50-        tv.tv_sec = millis/1000;
    51-        tv.tv_usec = (millis%1000)*1000;
    52-        ev.trigger(&tv);
    53+        context->scheduler->scheduleFromNow(func, std::chrono::milliseconds(millis));
    


    vasild commented at 4:31 am on June 6, 2025:

    Might use std::chrono::milliseconds for the type of the argument instead of int64_t:

    0    HTTPRPCTimer(NodeContext* context, std::function<void()>& func, std::chrono::milliseconds after)
    1    {
    2        context->scheduler->scheduleFromNow(func, after);
    

    (in commit 2950597694 use CScheduler for HTTPRPCTimer)


    pinheadmz commented at 5:07 pm on June 18, 2025:
    Can I though? Parent class RPCTimerInterface uses int64_t millis and so does our sibling class QtRPCTimerBase because of its QTimer.
  95. in src/httprpc.cpp: in 7d30118401 outdated
    59@@ -62,10 +60,10 @@ class HTTPRPCTimerInterface : public RPCTimerInterface
    60     }
    61     RPCTimerBase* NewTimer(std::function<void()>& func, int64_t millis) override
    62     {
    63-        return new HTTPRPCTimer(base, func, millis);
    64+        return new HTTPRPCTimer(m_context, func, millis);
    65     }
    66 private:
    67-    struct event_base* base;
    68+    NodeContext* m_context;
    


    vasild commented at 4:35 am on June 6, 2025:
    Because the context will always be set, ie will never be nullptr, then it is better to use a reference here instead of a pointer: NodeContext& m_context;

    pinheadmz commented at 5:16 pm on June 18, 2025:

    This makes sense to me but the problem is we are only ever given a pointer to a NodeContext (line 718):

    https://github.com/bitcoin/bitcoin/blob/5e6dbfd14ea9eace1c7e5ee76b140be46a0ec855/src/init.cpp#L710-L723

    It was explicitly changed from a reference to a pointer in #21574

    I tried passing node here and then jsut extracting the &address for HTTPReq_JSONRPC() but that resulted in JSONRPCException: Node context not found (-32603).

    Going to put this daemon back in the bottle for now, but can re-examine in a future rebase or follow-up

  96. in src/httprpc.cpp:133 in 7d30118401 outdated
    129@@ -132,7 +130,7 @@ static bool multiUserAuthorized(std::string strUserPass)
    130     return false;
    131 }
    132 
    133-static bool RPCAuthorized(const std::string& strAuth, std::string& strAuthUsernameOut)
    134+static bool RPCAuthorized(const std::string_view& strAuth, std::string& strAuthUsernameOut)
    


    vasild commented at 4:48 am on June 6, 2025:

    Can simplify the code a few lines below:

    0- std::string_view strUserPass64 = TrimStringView(std::string_view{strAuth}.substr(6));
    1+ std::string_view strUserPass64 = TrimStringView(strAuth.substr(6));
    

    (in commit 54a36093f7 http: using string_view to avoid unnecessary copy in Headers)


    pinheadmz commented at 4:12 pm on June 19, 2025:
    nice catch thanks
  97. in src/httpserver.h:74 in 7d30118401 outdated
    100+    void Remove(const std::string key);
    101+    bool Read(util::LineReader& reader);
    102+    std::string Stringify() const;
    103+
    104+private:
    105+    std::map<std::string, std::string, util::CaseInsensitiveComparator> m_map;
    


    vasild commented at 7:02 am on June 6, 2025:

    std::map would keep the entries sorted by the key in alphabetical order and lookup time is O(log(number of elements in the map)) whereas std::unordered_map does not maintain an order and has a lookup time of O(1). We do not need any order here, right? This probably is irrelevant since we are only going to put a bunch of entries in this map, but anyway, if there is no reason to use std::map I guess we should default to std::unordered_map.

    (from commit e95c6f5b65 http: Implement HTTPHeaders class)


    pinheadmz commented at 5:16 pm on June 19, 2025:
    This is great feedback thanks. I made this change by first editing the "string: add CaseInsensitiveComparator" commit which is now "string: add CaseInsensitive{KeyEqual, Hash} for unordered map" and implements the two operators needed for the unordered map.
  98. in src/httpserver.cpp:318 in 7d30118401 outdated
    333 {
    334-    if (!InitHTTPAllowList())
    335-        return false;
    336+    const auto it = m_map.find(key);
    337+    if (it == m_map.end()) return std::nullopt;
    338+    return std::string_view(it->second);
    


    vasild commented at 7:10 am on June 6, 2025:

    This returns a reference (or a “pointer”) to the outside world (from the point of view of the HTTPHeaders class) of an element from the private m_map member. That map is not immutable. What happens if the strings it contains get moved around if the map is internally resized when adding new entries? Also there is the HTTPHeaders::Remove() method which deletes entries from the map, rendering any returned pointers by Find() as dangling.

    54a36093f7 http: using string_view to avoid unnecessary copy in Headers – if there is no any measurable performance gain from this commit then it would be safer to drop it. If there is a measurable performance gain then hmm…


    pinheadmz commented at 6:30 pm on June 19, 2025:
    Ah that is another great catch thank you. Glad I wrote that as an extra commit! I’ll pop it off, it was added in response to this review comment: #32061 (review)
  99. vasild commented at 7:17 am on June 6, 2025: contributor
    Approach ACK 7d301184016a3f59c2e363dff631263cdbe21da0
  100. achow101 referenced this in commit d978a43d05 on Jun 9, 2025
  101. pinheadmz force-pushed on Jun 21, 2025
  102. DrahtBot removed the label Needs rebase on Jun 21, 2025
  103. DrahtBot added the label CI failed on Jun 21, 2025
  104. DrahtBot commented at 2:50 am on June 21, 2025: contributor

    🚧 At least one of the CI tasks failed. Task fuzzer,address,undefined,integer, no depends: https://github.com/bitcoin/bitcoin/runs/44516172178 LLM reason (✨ experimental): The CI failure is caused by a runtime error due to an implicit integer sign change detected by the sanitizer during fuzzing.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  105. pinheadmz force-pushed on Jun 21, 2025
  106. pinheadmz force-pushed on Jun 22, 2025
  107. DrahtBot removed the label CI failed on Jun 22, 2025
  108. pinheadmz force-pushed on Jun 22, 2025
  109. pinheadmz commented at 11:20 pm on June 22, 2025: member

    Rebased to address comments by @vasild.

    Also switched from #30988 (“Split CConnman”) to #32747 (“SockMan lite”) for the back end.

    That also included a rebase on master where #32408 was merged, cherry picking some commits from this PR.

  110. romanz commented at 7:37 am on June 29, 2025: contributor

    Low-latency HTTP request benchmark improvement - compared with #32541 (review)’s taking ~130ms per request:

     0$ ab -k -c 1 -n 100000 http://localhost:8332/rest/txfromblock/000000000000000000017bfd05b5fa367a424c4a565a4baf7950d9e8605df8ec-5000.bin
     1
     2Document Path:          /rest/txfromblock/000000000000000000017bfd05b5fa367a424c4a565a4baf7950d9e8605df8ec-5000.bin
     3Document Length:        234 bytes
     4
     5Concurrency Level:      1
     6Time taken for tests:   11.554 seconds
     7Complete requests:      100000
     8Failed requests:        0
     9Keep-Alive requests:    100000
    10Total transferred:      31700000 bytes
    11HTML transferred:       23400000 bytes
    12Requests per second:    8655.35 [#/sec] (mean)
    13Time per request:       0.116 [ms] (mean)
    14Time per request:       0.116 [ms] (mean, across all concurrent requests)
    15Transfer rate:          2679.44 [Kbytes/sec] received
    
  111. romanz commented at 8:50 am on June 29, 2025: contributor

    The latency of 404 response has improved too:

     0$ ab -k -c 1 -n 100000 http://localhost:8332/rest/
     1...
     2Time taken for tests:   2.876 seconds
     3Complete requests:      100000
     4Failed requests:        0
     5Non-2xx responses:      100000
     6Keep-Alive requests:    100000
     7Total transferred:      9500000 bytes
     8HTML transferred:       0 bytes
     9Requests per second:    34774.44 [#/sec] (mean)
    10Time per request:       0.029 [ms] (mean)
    11Transfer rate:          3226.14 [Kbytes/sec] received
    

    master branch:

     0$ ab -k -c 1 -n 100000 http://localhost:8332/rest/
     1...
     2Time taken for tests:   3.805 seconds
     3Complete requests:      100000
     4Failed requests:        0
     5Non-2xx responses:      100000
     6Keep-Alive requests:    100000
     7Total transferred:      11400000 bytes
     8HTML transferred:       0 bytes
     9Requests per second:    26277.76 [#/sec] (mean)
    10Time per request:       0.038 [ms] (mean)
    11Transfer rate:          2925.45 [Kbytes/sec] received
    
  112. romanz commented at 8:53 am on June 29, 2025: contributor
    Tested ACK e531a7cd2c
  113. DrahtBot requested review from vasild on Jun 29, 2025
  114. DrahtBot added the label Needs rebase on Jul 8, 2025
  115. in src/httpserver.h:55 in e531a7cd2c outdated
    68+    virtual ~HTTPClosure() = default;
    69+};
    70 
    71-/** Change logging level for libevent. */
    72-void UpdateHTTPServerLogging(bool enable);
    73+namespace http_bitcoin {
    


    ryanofsky commented at 4:32 pm on July 11, 2025:

    Just an idea, and not necessarily a suggestion for this PR since it could be a followup, but I was thinking since httpserver.h and httpserver.cpp files are basically rewritten here it could also be good to rename them to rpc/http/server.h and rpc/http/server.cpp and put them in a matching rpc::http namespace. Reasons for suggesting this:

    • To take some files out of the top level directory.
    • To choose a namespace name rpc::http that matches the directory path rpc/http.
    • To make space for a client library to be located at rpc/http/client.h rpc/http/client.cpp (for tools that may that want to use some bitcoin-cli functionality without shelling out to bitcoin-cli)
    • To make space for other RPC protocols to be implemented places like rpc/grpc/.
    • To make rpc/ and ipc/ layouts more consistent (since latter uses src/ipc/<protocol> structure).

    One reason to not like this suggestion would be if you wouldn’t consider other uses of the HTTP server to be RPC. But I would interpret “rpc” liberally and probably move the related files as well:

    • rest.h -> rpc/http/rest.h
    • rest.cpp -> rpc/http/rest.cpp
    • httprpc.h -> rpc/http/jsonrpc.h
    • httprpc.cpp -> rpc/http/jsonrpc.cpp

    Again not pushing for this, just wanted to mention the idea.

  116. in src/util/strencodings.h:382 in e531a7cd2c
    377+        std::transform(s.begin(), s.end(), lowered.begin(),
    378+            [](char c) {
    379+                // Avoid implicit-integer-sign-change UB by only
    380+                // processing ASCII.
    381+                unsigned char uc = static_cast<unsigned char>(c);
    382+                if (uc >= 128) return c;
    


    vasild commented at 11:30 am on September 3, 2025:

    nit: I think that can be a bit shorter:

    0            [](char c) {
    1                // Avoid implicit-integer-sign-change UB by only
    2                // processing ASCII.
    3                if (c < 0) return c;
    

    pinheadmz commented at 4:18 pm on November 19, 2025:

    I guess I need to specify unsigned char for some platforms ?

    0/home/runner/work/_temp/src/util/strencodings.h:381:23: error: comparison is always false due to limited range of data type [-Werror=type-limits]
    1  381 |                 if (c < 0) return c;
    2      |                     ~~^~~
    3cc1plus: all warnings being treated as errors
    
  117. in src/test/httpserver_tests.cpp:142 in e531a7cd2c outdated
    140+        std::string headers_string{headers.Stringify()};
    141+        BOOST_REQUIRE(headers_string.find("Sandwich: ham\r\n") != std::string::npos);
    142+        BOOST_REQUIRE(headers_string.find("Coffee: black\r\n") != std::string::npos);
    143+        BOOST_REQUIRE(headers_string.find("Cache-Control: no-cache, no-store\r\n") != std::string::npos);
    144+        // No matter what order the headers end up in, it should be terminated by an empty line
    145+        BOOST_REQUIRE(headers_string.ends_with("\r\n\r\n"));
    


    vasild commented at 11:42 am on September 3, 2025:

    Maybe also check that the total length of all headers is as expected. That is, no extra, unexpected, stuff has been returned:

    0         BOOST_REQUIRE(headers_string.ends_with("\r\n\r\n"));
    1+        BOOST_CHECK_EQUAL(headers_string.length(), 67);
    2     }
    
  118. in src/test/httpserver_tests.cpp:411 in e531a7cd2c
    409+                if (actual.ends_with("\r\n\r\n874140\n") &&
    410+                    actual.find("HTTP/1.1 200 OK\r\n") != std::string::npos &&
    411+                    actual.find("Connection: close\r\n") != std::string::npos &&
    412+                    actual.find("Content-Length: 7\r\n") != std::string::npos &&
    413+                    actual.find("Content-Type: text/html; charset=ISO-8859-1\r\n") != std::string::npos &&
    414+                    actual.find("Date: Wed, 11 Dec 2024 00:47:09 GMT\r\n") != std::string::npos
    


    vasild commented at 12:50 pm on September 3, 2025:

    Similar to above:

    0                    actual.find("Date: Wed, 11 Dec 2024 00:47:09 GMT\r\n") != std::string::npos &&
    1                    actual.length() == 146
    
  119. in src/httpserver.h:130 in e531a7cd2c
    163+
    164+    // These methods reimplement the API from http_libevent::HTTPRequest
    165+    // for downstream JSONRPC and REST modules.
    166+    std::string GetURI() const {return m_target;};
    167+    CService GetPeer() const;
    168+    HTTPRequestMethod GetRequestMethod() const {return m_method;};
    


    vasild commented at 12:55 pm on September 3, 2025:

    nit: no need for trailing ; and some missing whitespace:

    0    HTTPRequestMethod GetRequestMethod() const { return m_method; }
    
  120. vasild approved
  121. vasild commented at 1:20 pm on September 3, 2025: contributor

    ACK e531a7cd2c17dfb8d075d02865dbc25f8a832b3a

    All my suggestions above have been addressed or some reasoning has been given why not.

  122. pinheadmz force-pushed on Nov 19, 2025
  123. pinheadmz commented at 4:03 pm on November 19, 2025: member

    Push to 8c81bdf2532aa1c61e25d367a480ce3aa71362ca:

    • Rebase on #32747 which has gotten lots of good feedback, and is also rebased on master
    • Fix four nits from @vasild

    This push is mainly for a CI sanity check, then I’m going to put it in draft while I refactor for @theuni main feedback which will be removing the sockman abstraction and implementing the I/O loop directly in HTTPServer

  124. pinheadmz marked this as a draft on Nov 19, 2025
  125. DrahtBot removed the label Needs rebase on Nov 19, 2025
  126. DrahtBot added the label CI failed on Nov 19, 2025
  127. DrahtBot commented at 5:19 pm on November 19, 2025: contributor

    🚧 At least one of the CI tasks failed. Task previous releases: https://github.com/bitcoin/bitcoin/actions/runs/19507786228/job/55838856410 LLM reason (✨ experimental): Compiler error: -Werror type-limits flags fail the build (comparison always false in strencodings.h), causing CI failure.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  128. ryanofsky commented at 6:35 pm on November 19, 2025: contributor

    re: #32061 (comment)

    This push is mainly for a CI sanity check, then I’m going to put it in draft while I refactor for @theuni main feedback which will be removing the sockman abstraction and implementing the I/O loop directly in HTTPServer

    I agree with theuni it should make this PR easier to review and simpler if current sockman functionality (sockman.cpp) is folded into HTTP code, since sockman’s current sending interface and use of int ids don’t seem very convenient for supporting HTTP.

    I could imagine this changing in the future though. If sockman provided a way to wake the event loop (https://github.com/bitcoin/bitcoin/pull/30988#issuecomment-3549076531) it could work nicely for HTTP. Also in theory, other unused sockman functionality like the ability to listen on i2p or tor hidden service addresses could be potentially useful, if for example you had a node at home and wanted an easy way for a mobile app to be able to connect to it from anywhere.

    I guess I’m saying it probably does make sense for sockman and HTTP code to part ways for simplicity and ease-of-review, but I could imagine them being reconciled at some point later. And we might want to reconsider the situation if #30988 makes more progress.

  129. pinheadmz force-pushed on Nov 25, 2025
  130. pinheadmz force-pushed on Nov 26, 2025
  131. pinheadmz force-pushed on Nov 26, 2025
  132. DrahtBot removed the label CI failed on Nov 26, 2025
  133. pinheadmz marked this as ready for review on Nov 26, 2025
  134. pinheadmz commented at 11:02 am on November 26, 2025: member
    Ok this PR is back up for review. It is divorced from SockMan, and the I/O loop is now implemented inside HTTPServer itself.
  135. pinheadmz force-pushed on Nov 29, 2025
  136. pinheadmz commented at 6:55 pm on November 29, 2025: member

    push to 8106ac5d0f71fbd6445810f0f6388b36a7191b72:

    • multiple nits and suggestions taken from corecheck / sonarcloud
  137. pinheadmz force-pushed on Dec 1, 2025
  138. pinheadmz commented at 11:35 am on December 1, 2025: member

    Push to 5f6842030a4d82b81d640f1109c543dabd0938ef:

    • More nits and suggestions from Sonarcloud
  139. DrahtBot added the label Needs rebase on Dec 2, 2025
  140. pinheadmz force-pushed on Dec 2, 2025
  141. pinheadmz commented at 7:01 pm on December 2, 2025: member

    push to 22ec2f95eecddd5dc6ea53f670f232a7e425a8d0:

  142. DrahtBot removed the label Needs rebase on Dec 2, 2025
  143. DrahtBot added the label Needs rebase on Dec 11, 2025
  144. furszy commented at 2:45 pm on December 15, 2025: member
    Concept ACK. libevent has been a source of issues in the past. Also, the 10% speed up mentioned in the PR description is promising. I assume that was measured on the previous approach, so it would be nice to benchmark it again with the new one.
  145. pinheadmz force-pushed on Dec 18, 2025
  146. pinheadmz commented at 4:55 pm on December 18, 2025: member

    push to 625088468a3f2f1438fd71d1b87210e16904fa58:

    • Fix conflicts with master, mostly LogDebug() conflicts from #29641
    • Silent conflict with a test added in #33657 since the new URL parsing is more tolerant
  147. DrahtBot removed the label Needs rebase on Dec 18, 2025
  148. in src/netbase.h:366 in b656c35c09 outdated
    361@@ -362,4 +362,7 @@ bool IsBadPort(uint16_t port);
    362  */
    363 CService MaybeFlipIPv6toCJDNS(const CService& service);
    364 
    365+/** Get the bind address for a socket as CService. */
    366+CService GetBindAddress(const Sock& sock);
    


    fjahr commented at 4:32 pm on December 25, 2025:
    nit: I think I wouldn’t call the commit move-only because of this.

    pinheadmz commented at 6:32 pm on January 7, 2026:
    👍 re-worded the commit message
  149. in src/netbase.cpp:957 in ed360080c1 outdated
    956-    if (!sock.GetSockName((struct sockaddr*)&sockaddr_bind, &sockaddr_bind_len)) {
    957-        addr_bind.SetSockAddr((const struct sockaddr*)&sockaddr_bind, sockaddr_bind_len);
    958+    sockaddr_storage storage;
    959+    socklen_t len = sizeof(storage);
    960+
    961+    auto sa = static_cast<sockaddr*>(static_cast<void*>(&storage));
    


    fjahr commented at 4:42 pm on December 25, 2025:
    The commit message talks about reinterpret_cast but the code uses static_cast?

    pinheadmz commented at 6:34 pm on January 7, 2026:
    Ah thanks, fixed this commit message

    hodlinator commented at 10:06 am on March 10, 2026:

    The remaining double-static_cast in HTTPServer::BindAndStartListening() and HTTPServer::AcceptConnection() are just jumping through the C-shaped hoop of void* uncertainty. reinterpret_cast is more honest IMO.

    0- auto sa = static_cast<sockaddr*>(static_cast<void*>(&storage));
    1+ auto sa = reinterpret_cast<sockaddr*>(&storage);
    
  150. in src/netbase.cpp:959 in ed360080c1 outdated
    958+    sockaddr_storage storage;
    959+    socklen_t len = sizeof(storage);
    960+
    961+    auto sa = static_cast<sockaddr*>(static_cast<void*>(&storage));
    962+
    963+    if (!sock.GetSockName(sa, &len)) {
    


    fjahr commented at 4:50 pm on December 25, 2025:

    nit: I would prefer something like this because it’s clearer to me when you look at this with c++ eyes like I did at first. Take it only if you like it too.

    0    // POSIX convention: 0 is success
    1    if (sock.GetSockName(sa, &len) == 0) {
    

    pinheadmz commented at 6:36 pm on January 7, 2026:
    sure, taken. every little bit of legibility helps ;-)
  151. in src/util/strencodings.h:179 in cd2fe7fbf2 outdated
    176  * @returns std::nullopt if the entire string could not be parsed, or if the
    177  *   parsed value is not in the range representable by the type T.
    178  */
    179 template <typename T>
    180-std::optional<T> ToIntegral(std::string_view str)
    181+std::optional<T> ToIntegral(std::string_view str, size_t base = 10)
    


    fjahr commented at 4:55 pm on December 25, 2025:
    nit: If we are only plan to use this with base 10 or 16 maybe it would be safer to make this a bool. But I don’t feel too strongly about it.

    pinheadmz commented at 6:38 pm on January 7, 2026:
    I think I will leave as-is, a bool would require explanation and this way we have explicit argument name and default value.
  152. in src/util/strencodings.h:370 in bd924ecf4e outdated
    365+    size_t operator()(const std::string_view& s) const {
    366+        std::string lowered;
    367+        lowered.resize(s.size());
    368+        std::ranges::transform(s, lowered.begin(),
    369+            [](char c) {
    370+                // Avoid implicit-integer-sign-change UB by only
    


    fjahr commented at 5:04 pm on December 25, 2025:
    nit: I would call the classe AsciiCaseInsensitiveHash

    pinheadmz commented at 6:46 pm on January 7, 2026:
    Ok taken. I was doubtful at first but learned about “case folding” in unicode so, the qualification is justified.
  153. in src/util/strencodings.h:357 in bd924ecf4e outdated
    353@@ -353,6 +354,29 @@ struct Hex {
    354 };
    355 } // namespace detail
    356 
    357+struct CaseInsensitiveKeyEqual {
    


    fjahr commented at 5:09 pm on December 25, 2025:
    In the bd924ecf4e29b3ff918def6cb55b959fe19f33dc commit message: Not sure why this talks about libevent comparing in lowercase. This is just following HTTP requirements, right? So I found it a bit confusing and checked if this meant that this behavior was just temporary and maybe removed later together with libevent, but it isn’t…

    pinheadmz commented at 6:50 pm on January 7, 2026:
    The important bit is the case-insensitivity of the HTTP protocol. I reference libevent functions in comments and commit messages throughout the PR because I referenced them when writing and also for reviewers to compare. I’ll reword this commit message to better explain.
  154. in src/util/time.h:140 in 333af4bb58 outdated
    135@@ -136,6 +136,12 @@ std::string FormatISO8601DateTime(int64_t nTime);
    136 std::string FormatISO8601Date(int64_t nTime);
    137 std::optional<int64_t> ParseISO8601DateTime(std::string_view str);
    138 
    139+/**
    140+ * RFC7231 formatting https://www.rfc-editor.org/rfc/rfc7231#section-7.1.1.1
    


    fjahr commented at 5:20 pm on December 25, 2025:
    nit: This seems to be superseeded by RFC9110: https://www.rfc-editor.org/rfc/rfc9110.html#name-date-time-formats

    pinheadmz commented at 7:47 pm on January 7, 2026:
    Replaced the reference to HTTP spec with reference to the actual date/time spec implemented in the function.
  155. in src/util/time.h:143 in 333af4bb58 outdated
    135@@ -136,6 +136,12 @@ std::string FormatISO8601DateTime(int64_t nTime);
    136 std::string FormatISO8601Date(int64_t nTime);
    137 std::optional<int64_t> ParseISO8601DateTime(std::string_view str);
    138 
    139+/**
    140+ * RFC7231 formatting https://www.rfc-editor.org/rfc/rfc7231#section-7.1.1.1
    141+ * Used in HTTP/1.1 responses
    142+ */
    143+std::string FormatRFC7231DateTime(int64_t nTime);
    


    fjahr commented at 5:22 pm on December 25, 2025:
    Not sure if we should have the RFC in the name if it’s ambiguous where the format is specified (see above). Maybe FormatHTTPDate or something like that would be better.

    pinheadmz commented at 7:59 pm on January 7, 2026:
    Great catch, I had to dig through the forest of RFCs again, and rewrote the commit message and function name. I’m going with RFC1123 because that is the RFC referenced by libevent. I also wanted to keep the pattern of naming similar to neighbor functions like FormatISO8601DateTime()
  156. in src/util/string.cpp:33 in 6d5aa51abd outdated
    28+    while (it != end) {
    29+        auto c = static_cast<char>(*it);
    30+        ++it;
    31+        ++count;
    32+        if (c == '\n') break;
    33+        if (count >= max_read) throw std::runtime_error("max_read exceeded by LineReader");
    


    fjahr commented at 5:33 pm on December 25, 2025:
    Shouldn’t this be rather count > max_read? At least that would be what I would expect based on the error here and the docs in the .h: “Will not search for \n past max_read.”. It’s currently more like “up until but not including max_read”, right?

    pinheadmz commented at 8:59 pm on January 7, 2026:

    I think everything is correct as-is, but I refactored the tests in order to make sure I hit the edge correctly. I still could be missing an off-by-one though, so I appreciate your patience and re-review.

    If max_read is 8, I want to read no more than 8 characters looking for \n. So 123567\n is fine, meaning that indeed we read up to AND including max_read.

    However 12345678\n would throw an error because we would consume max_read characters and not find an EOL. In this case the EOL is “past max_read”.


    fjahr commented at 1:28 pm on January 8, 2026:

    Hm, yeah, I think the behavior is fine after looking at this again. I guess the first thing that made this a bit hard to parse is the ordering of the lines but I can see now that this is probably the most efficient way to do it. It may just not be the most readable and I needed some extra time to stare at it :)

    I will add some nits at the appropriate lines.

  157. fjahr commented at 5:38 pm on December 25, 2025: contributor
    Reviewed until 6d5aa51abd7e0ca935d0f016d7350c89edc38c38 . What do you think about splitting off the first 6 commits into it’s own pull (up to and including the line reader). I think these could be reviewed quickly and maybe give you some progress. These should me mergable and completely independent of other changes like #33689 . Speaking of, could you make clear if you would prefer that to be reviewed first and do you consider these changes here to be blocked by it? I am asking because I saw your a-c-k there…
  158. romanz commented at 5:38 pm on December 25, 2025: contributor

    Nice performance improvement in low-latency requests (requesting a non-existing header):

    0$ ab -k -c 1 -n 100000 http://localhost:8332/rest/headers/0000000000000000000000000000000000000000000000000000000000000000.bin
    1
    2Document Length:        0 bytes
    3
    4Concurrency Level:      1
    5Complete requests:      100000
    6Failed requests:        0
    7Keep-Alive requests:    100000
    8Total transferred:      10200000 bytes
    9HTML transferred:       0 bytes
    
    commit avg duration
    this PR 625088468a 25 us
    base 09a1fa190e 32 us
  159. pinheadmz force-pushed on Jan 7, 2026
  160. pinheadmz commented at 11:00 pm on January 7, 2026: member

    Push to 63a9487dc3f1667b66a711ee43ec3c5a6ff181fc:

    Address review feedback from @fjahr on first 6 commits, changes included renaming functions and rewording commit messages for better clarity. I plan on opening a new PR with just these 6 commits shortly, some of that code can also be consumed by #34158

  161. in src/test/util_string_tests.cpp:215 in 892d8effbc outdated
    210+
    211+        // EOL is at position equal to max_read: can read first line up to and including EOL,
    212+        // but then throws because there is no EOL within max_read
    213+        LineReader reader2(input, /*max_read=*/23);
    214+        BOOST_CHECK_EQUAL(reader2.ReadLine(), "once upon a time there");
    215+        BOOST_CHECK_THROW(reader2.ReadLine(), std::runtime_error);
    


    fjahr commented at 1:15 pm on January 8, 2026:

    EOL is at position equal to max_read

    This is a 23-char line so for me EOL is at max_read+1 again, same to what is said above in the 22-char line case:

    EOL is +1 past max_read


    fjahr commented at 1:53 pm on January 8, 2026:

    Also, another edge case is a string without any \n or a last line without it, e.g. instead of EOL there is EOF. If I understand correctly, because of how the loop is structured, a string without \n and length 10 will throw the error if max_read=10 but a string with length 9 without a \n will not throw at all and just return the string. I would expect only a line of 11+ without \n to throw.

    Maybe this ordering of the lines in the loop would do a bit better for this edge case? I didn’t test it yet so it’s possible there are other issues with it, feel free to ignore if that’s the case.

    0while (it != end) {
    1        if (count >= max_read) throw std::runtime_error("max_read exceeded by LineReader");
    2        ++count;
    3
    4        auto c = static_cast<char>(*it);
    5        if (c == '\n') break;
    6
    7        ++it;
    8}
    

    pinheadmz commented at 3:57 pm on January 8, 2026:

    You’re right and I think your original instinct on this was right as well. I modified the logic of the ReadLine() loop, added more comments, and covered the end-of-buffer case with another unit test. So it should be more clear: max_read is the length of the longest line we will return, and the line can be terminated either by \n (not counted) or by the end of the buffer.

    max_read = 8

    "12345678" OK "12345678\n" OK "123456789" bad "123456789\n" bad

  162. in src/util/string.h:276 in 892d8effbc outdated
    271+    explicit LineReader(std::span<const std::byte> buffer, size_t max_read);
    272+
    273+    /**
    274+     * Returns a string from current iterator position up to next \n
    275+     * and advances iterator, does not return trailing \n or \r.
    276+     * Will not search for \n past max_read.
    


    fjahr commented at 2:09 pm on January 8, 2026:
    It sounds a bit like this might just return: “Will throw if a line length exceeds max_read.”

    pinheadmz commented at 3:42 pm on January 8, 2026:
    Good point, updated
  163. fjahr commented at 2:24 pm on January 8, 2026: contributor
    nit-picking a bit more on LineReader edge cases :) Looking forward to the separate PR!
  164. pinheadmz force-pushed on Jan 9, 2026
  165. pinheadmz commented at 5:38 pm on January 9, 2026: member

    Push to b6afd81f58:

    Address feedback from @fjahr about LineReader(), fix edge-case handling. At this point I’m going to split off the first 6 commits to a new PR and we can focus review there instead.

    The new PR is https://github.com/bitcoin/bitcoin/pull/34242

  166. sedited referenced this in commit 0871e104a2 on Jan 23, 2026
  167. DrahtBot added the label Needs rebase on Jan 23, 2026
  168. pinheadmz force-pushed on Jan 23, 2026
  169. DrahtBot removed the label Needs rebase on Jan 23, 2026
  170. pinheadmz commented at 8:43 pm on January 23, 2026: member

    push to 3002fbe0d9:

    • Rebase on master and drop the commits merged into #34242
  171. in src/httpserver.cpp:828 in d549f01caa outdated
    823+        // https://httpwg.org/specs/rfc9110.html#rfc.section.5.6.2
    824+        const size_t pos{line.find(':')};
    825+        if (pos == std::string::npos) throw std::runtime_error("HTTP header missing colon (:)");
    826+
    827+        // Whitespace is optional
    828+        std::string key = util::TrimString(std::string_view(line).substr(0, pos));
    


    fjahr commented at 4:17 pm on January 24, 2026:

    I think this would allow and empty key to be stored as well, which is probably not part of the spec. Maybe add a check like this:

    0if (key.empty()) throw std::runtime_error("Empty HTTP header name");
    

    pinheadmz commented at 7:29 pm on January 30, 2026:
    Good catch thanks, added the check and a test.
  172. in src/httpserver.cpp:798 in d549f01caa outdated
    793+void HTTPHeaders::Write(const std::string& key, const std::string& value)
    794+{
    795+    // If present, append value to list
    796+    const auto existing_value = Find(key);
    797+    if (existing_value) {
    798+        m_map[key] = existing_value.value() + ", " + value;
    


    fjahr commented at 4:49 pm on January 24, 2026:

    Hm, for some keys this is ok, for others it isn’t afaict. But not sure yet where that is handled best. I guess what bothers me so far is that it’s the general Write function that does this internally. But I will need to check how the special cases are handled in the later commits.

    See especially https://www.rfc-editor.org/rfc/rfc9110.html#section-5.3:

    Note: In practice, the “Set-Cookie” header field ([COOKIE]) often appears in a response message across multiple field lines and does not use the list syntax, violating the above requirements on multiple field lines with the same field name. Since it cannot be combined into a single field value, recipients ought to handle “Set-Cookie” as a special case while processing fields. (See Appendix A.2.3 of [Kri2001] for details.)


    pinheadmz commented at 3:24 pm on January 31, 2026:
    This is a really good catch and is big bug in what I had. To match spec and libevent behavior I had to replace the unordered_map with vector <pair of strings> to allow duplicate headers. Libevent’s getters are more like “find first and return early” so I’ll do that as well. In that sense we will match the looser protocol enforcement. This also means that since I’m not using a map anymore, I don’t need the comparator/hasher added in #34242 so I’ll revert that with a new utility commit in this branch. I’m just gonna move the CaseInsensitiveEqual() function from test utilities to src/.
  173. in src/test/httpserver_tests.cpp:114 in 5aae24049f outdated
    109+    HTTPResponse res;
    110+    res.m_version_major = 1;
    111+    res.m_version_minor = 1;
    112+    res.m_status = HTTP_OK;
    113+    res.m_reason = HTTPReason.find(res.m_status)->second;
    114+    res.m_body = StringToBuffer("{\"result\":865793,\"error\":null,\"id\":null\"}");
    


    fjahr commented at 4:53 pm on January 24, 2026:

    nit: There seems to be one too many quotes

    0    res.m_body = StringToBuffer("{\"result\":865793,\"error\":null,\"id\":null}");
    

    pinheadmz commented at 3:27 pm on January 31, 2026:
    thanks, fixed
  174. in src/httpserver.h:208 in 12430fe99e outdated
    201@@ -202,6 +202,15 @@ class HTTPEvent
    202 };
    203 
    204 namespace http_bitcoin {
    205+using util::LineReader;
    206+
    207+//! Shortest valid request line, used by libevent in evhttp_parse_request_line()
    208+static const size_t MIN_REQUEST_LINE_LENGTH{strlen("GET / HTTP/1.0")};
    


    fjahr commented at 5:00 pm on January 24, 2026:
    nit: could be constexpr I think, also below

    pinheadmz commented at 5:58 pm on January 31, 2026:
    Thanks took this, couldn’t keep strlen for compile time but C++17 allows string_view::size(). 5 minutes of research suggest that strlen should be allwed by C++20 but its not on AppleClang? Anyway…
  175. in src/test/httpserver_tests.cpp:253 in 12430fe99e outdated
    248+        BOOST_CHECK_THROW(req.LoadBody(reader), std::runtime_error);
    249+    }
    250+    {
    251+        // Content-Length indicates more data than we have in the buffer.
    252+        // Again, not an error just try again later.
    253+        const std::string excessive_content_length = "GET / HTTP/1.0\r\nContent-Length: 1024\r\n\r\n{\"method\":\"getblockcount\"}";
    


    fjahr commented at 5:07 pm on January 24, 2026:
    Maybe also add a negative value test case for version and content-length.

    pinheadmz commented at 3:06 pm on February 1, 2026:

    good idea, added tests and also changed the type to unsigned int, added extra checks to match libevent. Note that “HTTP/2” does not use text format

    0	if (n != 2 || major > '1' || major < '0' || minor > '9' || minor < '0') {
    1		event_debug(("%s: bad version %s on message %p from %s",
    
  176. in src/httpserver.cpp:984 in 0cbe102bf2 outdated
    979+        return util::Error{strprintf(_("Cannot listen on %s: %s"),
    980+                                        to.ToStringAddrPort(),
    981+                                        NetworkErrorString(WSAGetLastError()))};
    982+    }
    983+
    984+    m_listen.emplace_back(std::move(sock));
    


    fjahr commented at 5:22 pm on January 24, 2026:
    Hm, kind of surprised that htis works because it seems this is adding the unique ptr to a vector of shared ptr.

    pinheadmz commented at 3:39 pm on February 1, 2026:
    Good catch, this is the same move we make in net.cpp::BindListenPort() as well. My understanding from https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr.html (and also GPT) is that what we are actually doing here is constructing a new shared_ptr with the current unique_ptr as an argument. Ownership is transferred and the unique_ptr is nullified
  177. in src/httpserver.cpp:1017 in afe4c81f82 outdated
    1012+        return {};
    1013+    }
    1014+
    1015+    if (!addr.SetSockAddr(sa, len)) {
    1016+        LogDebug(BCLog::HTTP,
    1017+                 "Unknown socket family\n");
    


    fjahr commented at 5:29 pm on January 24, 2026:
    The \n isn’t needed anymore with all the new logging as far as I know, here and in other places.

    pinheadmz commented at 3:40 pm on February 1, 2026:
    Thanks, fixed.
  178. in src/test/util/setup_common.cpp:631 in afe4c81f82 outdated
    626+    // Create the Mock Connected Socket that represents a client.
    627+    // It needs I/O pipes but its queue can remain empty
    628+    std::unique_ptr<DynSock> connected_socket{std::make_unique<DynSock>(connected_socket_pipes, std::make_shared<DynSock::Queue>())};
    629+
    630+    // Push into the queue of Accepted Sockets returned by the local CreateSock()
    631+    m_accepted_sockets->Push(std::move(connected_socket));
    


    fjahr commented at 5:31 pm on January 24, 2026:
    I guess this means it’s not const?

    pinheadmz commented at 7:33 pm on February 1, 2026:

    It does compile without warnings though, because the member is itself a pointer:

    0    std::shared_ptr<DynSock::Queue> m_accepted_sockets{std::make_shared<DynSock::Queue>()};
    

    BUT if you still think I should remove the const label for readability, I will.

  179. in src/httpserver.cpp:1693 in 04a01ed1c1 outdated
    1688+    if (g_http_server) {
    1689+        // Disconnect clients as their remaining responses are flushed
    1690+        g_http_server->DisconnectAllClients();
    1691+        // Wait for all disconnections
    1692+        while (g_http_server->GetConnectionsCount() != 0) {
    1693+            std::this_thread::sleep_for(50ms);
    


    fjahr commented at 10:06 pm on January 24, 2026:
    Should this loop get an interrupt/timeout to not run forever?

    pinheadmz commented at 7:16 pm on February 1, 2026:
    Really good catch and actually as written, this sustains a bug in the libevent implementation on master, documented in #31929 (h/t @hodlinator). That PR adds a maximum 30 second timeout to WaitUntilEmpty() which is analogous to this line here. I’ll hard-code the 30s as well unless someone thinks it needs to be configurable. I’ll try to write a test that covers this as well, something that intentionally and reliably hangs on master during shutdown.
  180. fjahr commented at 10:14 pm on January 24, 2026: contributor

    Still kind working on getting a good hold of the big picture but leaving a few questions/comments I had along the way.

    Curious if this would be a good project for differential fuzzing between the two different http servers. Did that already come up anywhere @pinheadmz ?

  181. pinheadmz commented at 4:07 pm on January 26, 2026: member
    @fjahr great idea about differential fuzzing, I’ll get started on that
  182. pinheadmz commented at 6:41 pm on January 30, 2026: member

    I’ve started fuzzing this branch using Fuzzamoto and plenty of assistance from @brunoerg SO far just crash-test fuzzing, differential fuzzing will come later. Luckily for me fuzzing the HTTP server is one of the example scenarios built in to the program ;-) I ran it for about a day without crashes, and generate a coverage report:

    https://thebitcoinblockclock.com/demo/http_coverage_output/coverage-report/coverage/bitcoin/src/httpserver.cpp.html

    Now that I have the workflow figured out, I’ll run a lot more

  183. pinheadmz force-pushed on Feb 1, 2026
  184. pinheadmz commented at 0:30 am on February 2, 2026: member

    Push to c4b3f6971ef0faf27ca250c109eaca35af53a6f7

    Address review from @fjahr including a few medium-sized approach changes.

    • Switch HTTPHeaders.m_map from unordered_map to vector<pair<string string>>. To allow duplicate field-names and be less protocol-restrictive, like libevent.
      • Find and Remove are now FindFirst and RemoveFirst
      • Moved around string utilities including a partial revert of #34242
    • Reject empty field names in headers
    • Reject invalid http versions
    • Wait 30 seconds for graceful client disconnection during shutdown
      • TODO: still needs an extra test that hangs forever on master

    nits:

    • Remove trailing \n from log messages
    • Use constexpr
    • Prefer BOOST_CHECK_EXCEPTION with error messages over BOOST_THROW
    • Add more edge cases in unit tests
  185. sedited referenced this in commit 4a05825a3f on Feb 11, 2026
  186. DrahtBot added the label Needs rebase on Feb 11, 2026
  187. pinheadmz force-pushed on Feb 12, 2026
  188. pinheadmz commented at 4:34 pm on February 12, 2026: member

    push to d48c18b88c64839fd48647a8bb10db4fa266f3e9

    • Rebase on master and consume the new ThreadPool from #33689
  189. DrahtBot removed the label Needs rebase on Feb 12, 2026
  190. in src/httpserver.cpp:1020 in d48c18b88c
    1249+            if (err == WSAEWOULDBLOCK || err == WSAEMSGSIZE || err == WSAEINTR || err == WSAEINPROGRESS) {
    1250+                return true;
    1251+            }
    1252 
    1253-    return GetQueryParameterFromUri(uri, key);
    1254+            // Unrecoverbale error, log and disconnect client.
    


    maflcko commented at 1:02 pm on February 13, 2026:

    Possible typos and grammar issues:

    Unrecoverbale -> Unrecoverable [spelling error in comment; could cause momentary confusion]
    datasent -> data sent [missing space / concatenated words in comment; minor typo impacting readability]
    

    Possible places where comparison-specific test macros should replace generic comparisons:

    src/test/httpserver_tests.cpp: BOOST_CHECK_THROW(req.LoadHeaders(reader), std::runtime_error) -> Replace with BOOST_CHECK_EXCEPTION(req.LoadHeaders(reader), std::runtime_error, HasReason{"HTTP header missing colon (:)"}
    

    2026-02-12 16:33:30


    pinheadmz commented at 1:14 pm on February 25, 2026:
    Thanks 👍 adding to next rebase.
  191. DrahtBot added the label Needs rebase on Feb 19, 2026
  192. pinheadmz force-pushed on Feb 25, 2026
  193. DrahtBot removed the label Needs rebase on Feb 25, 2026
  194. pinheadmz commented at 2:18 pm on February 25, 2026: member

    push to 5d13d9819db4ff39d18f491853ae5d354aa43c95:

    • address nits from @maflcko
    • rebase on master, fixing conflicts from #34577 in particular, using shared_ptr for HTTPRequest
  195. DrahtBot added the label CI failed on Feb 25, 2026
  196. DrahtBot removed the label CI failed on Feb 25, 2026
  197. in src/httpserver.h:91 in 5d13d9819d
    118+private:
    119+    /**
    120+     * Headers can have duplicate field names, so we use a vector of key-value pairs instead of a map.
    121+     * https://httpwg.org/specs/rfc9110.html#rfc.section.5.2
    122+     */
    123+    std::vector<std::pair<std::string, std::string>> m_map;
    


    vasild commented at 9:45 am on February 26, 2026:
    naming nit: used to be a map, but is not anymore, maybe rename from m_map to e.g. m_headers.

    pinheadmz commented at 1:17 pm on March 2, 2026:
    changed all m_map to m_headers

    hodlinator commented at 10:13 am on March 10, 2026:
    in b53198cbf5e1e1974ae1963effeac9a12c20c781 “http: Implement HTTPHeaders class”: #include <map> is still being added.
  198. in src/httpserver.h:181 in 5d13d9819d outdated
    224+
    225+    explicit HTTPServer(std::function<void(std::unique_ptr<HTTPRequest>&&)> func) : m_request_dispatcher(func) {};
    226+
    227+    virtual ~HTTPServer()
    228+    {
    229+        Assume(!m_thread_socket_handler.joinable()); // Missing call to JoinSocketsThreads()
    


    vasild commented at 11:10 am on February 26, 2026:
    Could also satisfy the condition if detach() has been called on the thread. In that case the thread is still running, but this Assume() will not be triggered, when it should. I think it is fine, just mentioning.

    pinheadmz commented at 1:46 pm on March 2, 2026:
    That’s true, and without a wrapper to disable detach() a future change to the code could trigger that (by not triggering it…) Although if there are still connected clients, the Assume on the next line will throw. So, careless future developer should at least still see that.
  199. in src/httpserver.cpp:768 in 5d13d9819d
    901+            continue;
    902+        }
    903+        const std::shared_ptr<HTTPClient> client{it->second};
    904+
    905+        bool send_ready = events.occurred & Sock::SEND; // Sock::SEND could only be set if ShouldTryToSend() has returned true in GenerateWaitSockets().
    906+        bool recv_ready = events.occurred & Sock::RECV; // Sock::RECV could only be set if ShouldTryToRecv() has returned true in GenerateWaitSockets().
    


    vasild commented at 2:49 pm on February 26, 2026:
    Drop these comments, ShouldTryToSend() does not exist (here).

    pinheadmz commented at 1:50 pm on March 2, 2026:
    yes! good catch thanks, RIP “sockman lite”
  200. in src/httpserver.cpp:765 in 5d13d9819d
    898+
    899+        auto it{io_readiness.httpclients_per_sock.find(sock)};
    900+        if (it == io_readiness.httpclients_per_sock.end()) {
    901+            continue;
    902+        }
    903+        const std::shared_ptr<HTTPClient> client{it->second};
    


    vasild commented at 2:50 pm on February 26, 2026:

    This will create a copy of the shared_ptr, which I think is not needed.

    0        const std::shared_ptr<HTTPClient>& client{it->second};
    

    pinheadmz commented at 1:52 pm on March 2, 2026:
    Good improvement thanks, using a reference instead.
  201. in src/httpserver.cpp:787 in 5d13d9819d
    920+
    921+            const ssize_t nrecv{WITH_LOCK(
    922+                client->m_sock_mutex,
    923+                return client->m_sock->Recv(buf, sizeof(buf), MSG_DONTWAIT);)};
    924+
    925+            if (nrecv < 0) { // In all cases (including -1 and 0) EventIOLoopCompletedForOne() should be executed after this, don't change the code to skip it.
    


    vasild commented at 2:57 pm on February 26, 2026:
    EventIOLoopCompletedForOne()

    pinheadmz commented at 1:53 pm on March 2, 2026:
    removed irrelevant comment here
  202. in src/httpserver.cpp:789 in 5d13d9819d
    922+                client->m_sock_mutex,
    923+                return client->m_sock->Recv(buf, sizeof(buf), MSG_DONTWAIT);)};
    924+
    925+            if (nrecv < 0) { // In all cases (including -1 and 0) EventIOLoopCompletedForOne() should be executed after this, don't change the code to skip it.
    926+                const int err = WSAGetLastError();
    927+                if (err != WSAEWOULDBLOCK && err != WSAEMSGSIZE && err != WSAEINTR && err != WSAEINPROGRESS) {
    


    vasild commented at 2:59 pm on February 26, 2026:
    There is IOErrorIsPermanent() in sock.cpp

    pinheadmz commented at 2:06 pm on March 2, 2026:
    Great, I moved that utility function to the header file and removed static in this commit so we can use it here. This is a slight functional change, the check for WSAEMSGSIZE is not present in the IOErrorIsPermanent() but as far as I can tell, that error is uncommon on recv() and especially uncommon on TCP.
  203. DrahtBot added the label Needs rebase on Feb 26, 2026
  204. in src/test/httpserver_tests.cpp:470 in 5d13d9819d
    468+        {
    469+            LOCK(requests_mutex);
    470+            // Connected client should have one request already from the static content.
    471+            if (requests.size() == 1) {
    472+                // Check the received request
    473+                BOOST_CHECK_EQUAL(requests.front()->m_body, "{\"method\":\"getblockcount\",\"params\":[],\"id\":1}\n");
    


    vasild commented at 9:34 am on February 27, 2026:

    nit: raw strings are cool

    0                BOOST_CHECK_EQUAL(requests.front()->m_body, R"({"method":"getblockcount","params":[],"id":1}\n)");
    

    pinheadmz commented at 2:12 pm on March 2, 2026:
    oh wow those are cool! Thanks. applied here and throughout the http tests

    pinheadmz commented at 2:22 pm on March 2, 2026:

    update: \n in a raw string is two characters, not a newline. So I’ll try

    0R"({"method":"getblockcount","params":[],"id":1})""\n"
    

    which is still a little funny but still more readable than what i had before

  205. in src/httpserver.cpp:399 in 5d13d9819d
    465+        // see evhttp_handle_chunked_read() in libevent http.c
    466+        while (reader.Remaining() > 0) {
    467+            auto maybe_chunk_size = reader.ReadLine();
    468+            if (!maybe_chunk_size) return false;
    469+
    470+            const auto chunk_size{ToIntegral<uint64_t>(maybe_chunk_size.value(), /*base=*/16)};
    


    vasild commented at 10:29 am on February 27, 2026:

    Add support for chunk-extensions:

     0             if (!maybe_chunk_size) return false;
     1
     2-            const auto chunk_size{ToIntegral<uint64_t>(maybe_chunk_size.value(), /*base=*/16)};
     3+            std::string_view chunk_size_noext{maybe_chunk_size.value()};
     4+            const auto semicolon_pos = chunk_size_noext.find(';');
     5+            if (semicolon_pos != chunk_size_noext.npos) {
     6+                chunk_size_noext.remove_suffix(chunk_size_noext.size() - semicolon_pos);
     7+            }
     8+
     9+            const auto chunk_size{ToIntegral<uint64_t>(chunk_size_noext, /*base=*/16)};
    10             if (!chunk_size) throw std::runtime_error("Cannot parse chunk length value");
    

    pinheadmz commented at 2:42 pm on March 2, 2026:
    Sure, added this along with a comment linking to RFC9112. Added coverage to unit tests. We don’t support chunk extensions (I had a hard time finding real protocols that do) but the semicolon is valid, so we accept and ignore.
  206. in src/httpserver.cpp:418 in 5d13d9819d
    484+            // Even though every chunk size is explicitly declared,
    485+            // they are still terminated by a CRLF we don't need.
    486+            auto crlf = reader.ReadLine();
    487+            if (!crlf || !crlf.value().empty()) throw std::runtime_error("Improperly terminated chunk");
    488+
    489+            if (last_chunk) return true;
    


    vasild commented at 10:42 am on February 27, 2026:

    Would be good to parse and ignore (?) any trailer fields:

    https://httpwg.org/specs/rfc9112.html#rfc.section.7.1.2 https://httpwg.org/specs/rfc9110.html#trailer.fields

    Otherwise, if present they will be left in the buffer and will brick the parsing of the next request.


    pinheadmz commented at 3:32 pm on March 2, 2026:
    Great catch thanks, I added a buffer-drainer to the end of chunked parsing and covered with unit test.
  207. in src/httpserver.cpp:481 in 5d13d9819d outdated
    550+            if (needs_body) {
    551+                res.m_headers.Write("Content-Length", strprintf("%d", reply_body.size()));
    552+            }
    553+
    554+            // Default for HTTP/1.1
    555+            res.m_keep_alive = true;
    


    vasild commented at 10:56 am on February 27, 2026:

    If the client sends Connection: close in a HTTP/1.1 request, then I guess the server should close the connection:

    https://httpwg.org/specs/rfc9112.html#persistent.tear-down

    A server that receives a “close” connection option MUST initiate closure of the connection (see below) …


    pinheadmz commented at 3:44 pm on March 2, 2026:

    This line is just setting the default behavior for HTTP/1.1

    https://httpwg.org/specs/rfc9112.html#persistent.connections

    HTTP/1.1 defaults to the use of persistent connections, allowing multiple requests and responses to be carried over a single connection. HTTP implementations SHOULD support persistent connections.

    The “connection: close” is handled below, unless I’m misunderstanding?

    0    auto connection_header{m_headers.FindFirst("Connection")};
    1    if (connection_header && ToLower(connection_header.value()) == "close") {
    2        // Might not exist already but we need to replace it, not append to it
    3        while (res.m_headers.RemoveFirst("Connection")) {} ;
    4
    5        res.m_headers.Write("Connection", "close");
    6        res.m_keep_alive = false;
    7    }
    
  208. in src/httpserver.cpp:1022 in 5d13d9819d
    1250+            // EWOULDBLOCK: The requested operation would block.
    1251+            //              The non-blocking socket operation cannot complete immediately.
    1252+            // EMSGSIZE:    Message too large. The receive buffer is too small for the incoming message.
    1253+            // EINTR:       Interrupted function call. The socket operation was interrupted by another thread.
    1254+            // EINPROGRESS: A socket operation in already progress.
    1255+            if (err == WSAEWOULDBLOCK || err == WSAEMSGSIZE || err == WSAEINTR || err == WSAEINPROGRESS) {
    


    vasild commented at 11:07 am on February 27, 2026:
    Use IOErrorIsPermanent()?

    pinheadmz commented at 3:47 pm on March 2, 2026:
    Thanks, reusing the utility from sock.h
  209. in src/httpserver.cpp:946 in 5d13d9819d outdated
    1159-std::string HTTPRequest::GetURI() const
    1160+void HTTPServer::DisconnectClients()
    1161 {
    1162-    return evhttp_request_get_uri(req);
    1163+    const auto now{Now<SteadySeconds>()};
    1164+    size_t erased = std::erase_if(m_connected,
    


    vasild commented at 11:20 am on February 27, 2026:

    This would just close the socket to the client. Should we take care of the following?

    https://httpwg.org/specs/rfc9112.html#persistent.tear-down

    If a server performs an immediate close of a TCP connection, there is a significant risk that the client will not be able to read the last HTTP response. If the server receives additional data from the client on a fully closed connection, such as another request sent by the client before receiving the server’s response, the server’s TCP stack will send a reset packet to the client; unfortunately, the reset packet might erase the client’s unacknowledged input buffers before they can be read and interpreted by the client’s HTTP parser.


    pinheadmz commented at 6:22 pm on March 2, 2026:

    This comment reminds me of #4432 which I found very difficult to test, but I will try again to write something to inspect data while closing.

    DisconnectClients() is written to avoid just closing a socket that is still in use in most cases (client->m_prevent_disconnect if the read or write buffer is not empty) so there shouldn’t be any abrupt closures.

  210. in src/httpserver.cpp:810 in 5d13d9819d
    943+            } else {
    944+                // Reset idle timeout
    945+                client->m_idle_since = Now<SteadySeconds>();
    946+
    947+                // Prevent disconnect until all requests are completely handled.
    948+                client->m_prevent_disconnect = true;
    


    vasild commented at 11:39 am on February 27, 2026:

    I find it difficult to reason about m_prevent_disconnect and m_disconnect because they are set in quite a few places, not always both at the same time.

    What is the scenario this is trying to address? When could it happen that m_disconnect == true && m_prevent_disconnect == true?

    Consider this:

    • Client connects and sends GET / HTTP/1.0. We will set m_prevent_disconnect = true.
    • Subsequent reads from the client socket return a permanent error. We will set m_disconnect = true.
    • However we would never set m_prevent_disconnect = false because that is only done at the end of sending to the client and in this case we would never send anything to them because MaybeDispatchRequestsFromClient() would keep hitting the break in
    0// Stop reading if we need more data from the client to parse a complete request
    1if (!client->ReadRequest(req)) break;
    

    I probably missed something…


    pinheadmz commented at 8:31 pm on March 2, 2026:

    You’re right these flags are messy. I renamed them and reordered the hierarchy in DisconnectClients() so it should be more clear and cover those edges:

    HTTPClient.m_disconnect: unilateral decision by the server to close, either because of error or “Connection: close” header on a completed request. Overrides everything. May drop data in read or write buffers.

    HTTPClient.m_connection_busy: data still in read or write buffer, prevents sudden disconnection during server shutdown until communication is done. Overrides m_disconnect_all.

    HTTPServer.m_disconnect_all: disconnect clients that are not “busy”, used during shutdown. Overrides m_keep_alive.

    HTTPClient.m_keep_alive: client has requested to keep connection open even if there are no pending requests or data in read or write buffer.

  211. vasild commented at 11:45 am on February 27, 2026: contributor
    Reviewed up to and including 1e0f323aeeaa76ce020c46425652559fa68e6679
  212. in src/httpserver.cpp:204 in 5d13d9819d
    213-    event_base_dispatch(base);
    214-    // Event loop will be interrupted by InterruptHTTPServer()
    215-    LogDebug(BCLog::HTTP, "Exited http event loop\n");
    216+    LogDebug(BCLog::HTTP, "Rejecting request while shutting down");
    217+    hreq->WriteReply(HTTP_SERVICE_UNAVAILABLE);
    218 }
    


    vasild commented at 11:55 am on March 2, 2026:
    naming nit: this function rejects just the given request, maybe RejectRequest() is more suitable name than RejectAllRequests().

    pinheadmz commented at 9:22 pm on March 2, 2026:
    sure, renamed.
  213. in src/httpserver.cpp:1105 in 5d13d9819d
    1348+                LogWarning("Binding RPC on address %s failed: %s", addr->ToStringAddrPort(), util::ErrorString(result).original);
    1349+            } else {
    1350+                bind_success = true;
    1351             }
    1352+        } else {
    1353+            LogWarning("Binding RPC on address %s port %i failed.", i->first, i->second);
    


    vasild commented at 11:58 am on March 2, 2026:
    Would be more helpful to print the reason for the failure. Something like: “cannot lookup the address to bind to: addr:port”.

    pinheadmz commented at 3:32 pm on March 3, 2026:
    Changed to "Could not bind RPC on address %s port %i: Address lookup failed."
  214. in src/httpserver.cpp:1100 in 5d13d9819d outdated
    1343+            if (addr->IsBindAny()) {
    1344+                LogWarning("The RPC server is not safe to expose to untrusted networks such as the public internet");
    1345+            }
    1346+            auto result{g_http_server->BindAndStartListening(addr.value())};
    1347+            if (!result) {
    1348+                LogWarning("Binding RPC on address %s failed: %s", addr->ToStringAddrPort(), util::ErrorString(result).original);
    


    vasild commented at 12:03 pm on March 2, 2026:
    No strong opinion, but I think it would be better to consider any failure to bind or lookup as an error. That is - LogError() and return false on the first failure. As a user, if I request the RPC server on a few addresses, I would like it to stop and tell me if it cannot listen on all of those addresses instead of continuing by listening on some and logging a warning message.

    pinheadmz commented at 3:44 pm on March 3, 2026:

    I knew this sounded familiar:

    #14968

    #15050

    But I can’t tell why these were closed (besides a CI failure and an issue with the getaddrinfo error?)

    There was even a CVE:

    https://github.com/bitcoin-core/bitcoincore.org/pull/637

    https://github.com/advisories/GHSA-rr2v-hv85-27hv

    I think it’s worth taking another stab at this, since those previous attempts mention libevent. Since you have no strong opinion, I’d like to punt on this to a follow-up PR in case it warrants more granular discussion.

  215. in src/httpserver.cpp:1156 in 5d13d9819d
    1417+        g_http_server->DisconnectAllClients();
    1418+        // Wait 30 seconds for all disconnections
    1419+        LogDebug(BCLog::HTTP, "Waiting for HTTP clients to disconnect gracefully");
    1420+        uint16_t timeout{600}; // 50ms ticks for 30 seconds
    1421+        while (g_http_server->GetConnectionsCount() != 0) {
    1422+            if (timeout <= 0) {
    


    vasild commented at 12:08 pm on March 2, 2026:
    Is fine but looks a bit odd to compare unsigned integer with <= 0. It will never be less than 0.

    pinheadmz commented at 3:48 pm on March 3, 2026:
    Changed to an int. I’ve noticed the <= pattern around the codebase before when == seems like it would be enough. But just in case something insane happens and we skip over 0, hey. Belt and suspenders.
  216. in src/httpserver.cpp:1166 in 5d13d9819d
    1427+            --timeout;
    1428+        }
    1429+        // Break sockman I/O loop: stop accepting connections, sending and receiving data
    1430+        g_http_server->InterruptNet();
    1431+        // Wait for sockman threads to exit
    1432+        g_http_server->JoinSocketsThreads();
    


    vasild commented at 12:10 pm on March 2, 2026:

    sockman

    0        // Break I/O loop: stop accepting connections, sending and receiving data
    1        g_http_server->InterruptNet();
    2        // Wait for sockets threads to exit
    3        g_http_server->JoinSocketsThreads();
    

    pinheadmz commented at 3:53 pm on March 3, 2026:
    Its just so hard to let go.
  217. in src/httpserver.h:465 in 5d13d9819d
    574+    explicit HTTPClient(HTTPServer::Id id, const CService& addr, std::unique_ptr<Sock> socket)
    575+        : m_id(id), m_addr(addr), m_origin(addr.ToStringAddrPort()), m_sock{std::move(socket)}
    576+    {
    577+        // Set timeout
    578+        m_idle_since = Now<SteadySeconds>();
    579+    };
    


    vasild commented at 12:14 pm on March 2, 2026:

    Any reason to initialize m_idle_since in the body of the constructor instead of inline:

    0SteadySeconds m_idle_since{Now<SteadySeconds>()};
    

    or

    0HTTPClient(...) : m_id(id), ..., m_idle_since{Now<SteadySeconds>()}, ...
    

    ?


    pinheadmz commented at 4:34 pm on March 3, 2026:
    Fixed the style here, thanks
  218. in src/httpserver.cpp:955 in 5d13d9819d
    1168+                                        if (((client->m_disconnect || m_disconnect_all_clients) &&
    1169+                                            // ...but not if this client is specifically flagged to prevent disconnect!
    1170+                                            // It is probably still busy.
    1171+                                            !client->m_prevent_disconnect) ||
    1172+                                            // No matter what, always disconnect if client has timed out.
    1173+                                            now - client->m_idle_since > m_rpcservertimeout) {
    


    vasild commented at 12:21 pm on March 2, 2026:

    There is this injustice that if a client sends a request that the server processes a long time and the client is just waiting for the server, it could be disconnected as “idle”, even though the client is idling just because it is waiting for the server. What about adding the ability to disable the timeout by setting -rpcservertimeout=0:

    0                                            m_rpcservertimeout > 0 && now - client->m_idle_since > m_rpcservertimeout) {
    

    pinheadmz commented at 7:16 pm on March 6, 2026:

    Great catch. I actually didn’t notice an issue testing with RPCs like waitforblockheight because even though DisconnectClients() erased the shared_ptr after the timeout, there was a second reference to the client in the worker thread keeping the connection alive. The result is the log message "Disconnecting HTTP client..." followed by the RPC response being sent to and received by the client!

    Master branch with libevent appears to ignore the idle timeout while busy with a request and then restart the timer after the reply is sent, so I will implement that as well.

    I wrote a functional test covering this which I’ll open as a PR against master to assert the current behavior, and then rebase this branch on that.


    pinheadmz commented at 11:20 pm on March 7, 2026:
  219. vasild commented at 12:28 pm on March 2, 2026: contributor
    Approach ACK 5d13d9819db4ff39d18f491853ae5d354aa43c95
  220. in src/rpc/node.cpp:1 in 5d13d9819d outdated


    hodlinator commented at 8:23 am on March 3, 2026:

    In 39ca86ba6a691be304ac4c32dd64af38d98e26e7 “http: delete libevent!”:

    IMO we should defer the change to the “logging” RPC until we actually remove the libevent dependency completely from the project, otherwise we are breaking functionality. Gets a bit messy to untangle http_libevent::UpdateHTTPServerLogging from HTTP though. At a minimum we could include a note in the commit message such as “Drops logging integration with libevent ahead of removal of the dependency”. Should also probably drop the enum value from /src/logging/categories.h unless we untangle the logging support from HTTP.

    Would also suggest renaming commit message title to: “http: remove libevent usage from this subsystem”


    pinheadmz commented at 3:25 pm on March 13, 2026:

    Ok I took the commit message change and reverted the change to RPC logging. I think it is still appropriate to remove UpdateHTTPServerLogging() and all calls to event_set_log_callback() and event_enable_debug_logging() from httpserver.cpp so what I left behind can be removed in #34411 or other follow-up, essentially:

    0static RPCHelpMan logging()
    1...
    2if (libevent thing) {
    3    // Comment that this is where libevent logging code would go but nothing uses it now
    4}
    
  221. in src/httpserver.cpp:299 in 5d13d9819d outdated
    342+        auto maybe_line = reader.ReadLine();
    343+        if (!maybe_line) return false;
    344+        const std::string& line = *maybe_line;
    345+
    346+        // An empty line indicates end of the headers section https://www.rfc-editor.org/rfc/rfc2616#section-4
    347+        if (line.length() == 0) break;
    


    hodlinator commented at 8:26 am on March 3, 2026:

    nit: Better matches comment above, less tokens:

    0        if (line.empty()) break;
    

    pinheadmz commented at 5:09 pm on March 10, 2026:
    switch to empty() is cleaner thanks
  222. in src/httpserver.cpp:319 in 5d13d9819d outdated
    363+
    364+        Write(key, value);
    365+    } while (true);
    366 
    367-    raii_event_base base_ctr = obtain_event_base();
    368+    return true;
    


    hodlinator commented at 8:42 am on March 3, 2026:

    nit: Clarify loop:

     0--- a/src/httpserver.cpp
     1+++ b/src/httpserver.cpp
     2@@ -744,13 +744,11 @@ bool HTTPHeaders::Read(util::LineReader& reader)
     3 {
     4     // Headers https://httpwg.org/specs/rfc9110.html#rfc.section.6.3
     5     // A sequence of Field Lines https://httpwg.org/specs/rfc9110.html#rfc.section.5.2
     6-    do {
     7-        auto maybe_line = reader.ReadLine();
     8-        if (!maybe_line) return false;
     9+    while (auto maybe_line = reader.ReadLine()) {
    10         const std::string& line = *maybe_line;
    11 
    12         // An empty line indicates end of the headers section https://www.rfc-editor.org/rfc/rfc2616#section-4
    13-        if (line.length() == 0) break;
    14+        if (line.empty()) return true;
    15 
    16         // Header line must have at least one ":"
    17         // keys are not allowed to have delimiters like ":" but values are
    18@@ -768,9 +766,9 @@ bool HTTPHeaders::Read(util::LineReader& reader)
    19         if (key.empty()) throw std::runtime_error("Empty HTTP header name");
    20 
    21         Write(key, value);
    22-    } while (true);
    23+    }
    24 
    25-    return true;
    26+    return false;
    27 }
    28 
    29 std::string HTTPHeaders::Stringify() const
    

    pinheadmz commented at 3:16 pm on March 11, 2026:
    loop clarified! thanks
  223. in src/test/httpserver_tests.cpp:30 in 5d13d9819d outdated
    27+    "204261736963205831396a6232397261575666587a6f354f4751354f4451334d57"
    28+    "4e6d4e6a67304e7a417a59546b7a4e32457a4e7a6b305a44466c4f4451314e6a5a"
    29+    "6d5954526b5a6a4a694d7a466b596a68684f4449345a4759344d6a566a4f546735"
    30+    "5a4749344f54566c0d0a436f6e74656e742d4c656e6774683a2034360d0a0d0a7b"
    31+    "226d6574686f64223a22676574626c6f636b636f756e74222c22706172616d7322"
    32+    "3a5b5d2c226964223a317d0a").value()};
    


    hodlinator commented at 9:01 am on March 3, 2026:

    These can use hex literals instead:

     0diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp
     1index 1bcfd8f035..c4606518b0 100644
     2--- a/src/test/httpserver_tests.cpp
     3+++ b/src/test/httpserver_tests.cpp
     4@@ -9,6 +9,7 @@
     5 
     6 #include <boost/test/unit_test.hpp>
     7 
     8+using namespace util::hex_literals;
     9 using http_bitcoin::HTTPClient;
    10 using http_bitcoin::HTTPHeaders;
    11 using http_bitcoin::HTTPRequest;
    12@@ -18,7 +19,7 @@ using http_bitcoin::MAX_HEADERS_SIZE;
    13 using util::LineReader;
    14 
    15 // HTTP request captured from bitcoin-cli
    16-const std::vector<std::byte> full_request{TryParseHex<std::byte>(
    17+constexpr auto full_request{
    18     "504f5354202f20485454502f312e310d0a486f73743a203132372e302e302e310d"
    19     "0a436f6e6e656374696f6e3a20636c6f73650d0a436f6e74656e742d547970653a"
    20     "206170706c69636174696f6e2f6a736f6e0d0a417574686f72697a6174696f6e3a"
    21@@ -27,7 +28,7 @@ const std::vector<std::byte> full_request{TryParseHex<std::byte>(
    22     "6d5954526b5a6a4a694d7a466b596a68684f4449345a4759344d6a566a4f546735"
    23     "5a4749344f54566c0d0a436f6e74656e742d4c656e6774683a2034360d0a0d0a7b"
    24     "226d6574686f64223a22676574626c6f636b636f756e74222c22706172616d7322"
    25-    "3a5b5d2c226964223a317d0a").value()};
    26+    "3a5b5d2c226964223a317d0a"_hex};
    27 
    28 BOOST_FIXTURE_TEST_SUITE(httpserver_tests, SocketTestingSetup)
    29 
    30@@ -127,14 +128,14 @@ BOOST_AUTO_TEST_CASE(http_headers_tests)
    31     }
    32     {
    33         // Reading request headers captured from bitcoin-cli
    34-        std::vector<std::byte> buffer{TryParseHex<std::byte>(
    35+        constexpr auto buffer{
    36             "486f73743a203132372e302e302e310d0a436f6e6e656374696f6e3a20636c6f73"
    37             "650d0a436f6e74656e742d547970653a206170706c69636174696f6e2f6a736f6e"
    38             "0d0a417574686f72697a6174696f6e3a204261736963205831396a623239726157"
    39             "5666587a6f7a597a4a6b4e5441784e44466c4d474a69596d56684d5449354f4467"
    40             "334e7a49354d544d334e54526d4e54686b4e6a63324f574d775a5459785a6a677a"
    41             "4e5467794e7a4577595459314f47526b596a566d5a4751330d0a436f6e74656e74"
    42-            "2d4c656e6774683a2034360d0a0d0a").value()};
    43+            "2d4c656e6774683a2034360d0a0d0a"_hex};
    44         util::LineReader reader(buffer, /*max_line_length=*/1028);
    45         HTTPHeaders headers{};
    46         headers.Read(reader);
    

    pinheadmz commented at 10:51 am on March 11, 2026:
    Amazing, thanks! Switching to hex literals, and will apply elsewhere as well.
  224. furszy commented at 3:14 pm on March 3, 2026: member
    It seems decoupling the GetQueryParameterFromUri changes could be a good next step and an easy win. We don’t need to depend on libevent for it.
  225. in src/httpserver.cpp:588 in 5d13d9819d outdated
    670-    LogDebug(BCLog::HTTP, "Stopping HTTP server\n");
    671+    std::optional<std::string> found{m_headers.FindFirst(hdr)};
    672+    if (found.has_value()) {
    673+        return std::make_pair(true, found.value());
    674+    } else
    675+        return std::make_pair(false, "");
    


    hodlinator commented at 9:01 pm on March 5, 2026:

    nit: Less verbose, moves instead of copying std::string:

    0    std::optional<std::string> found{m_headers.FindFirst(hdr)};
    1    return std::pair{found.has_value(), std::move(found).value_or("")};
    

    (Evaluation-order is guaranteed when using braces, see https://eel.is/c++draft/dcl.init.list#4).


    pinheadmz commented at 3:24 pm on March 11, 2026:
    taken, thanks. I see the evaluation order is important to guarantee that we test the value before moving it!
  226. in src/httpserver.cpp:287 in 5d13d9819d outdated
    325+            m_map.erase(it);
    326+            return true;
    327+        }
    328+    }
    329+    return false;
    330+}
    


    hodlinator commented at 9:06 pm on March 5, 2026:

    nit: Covers all uses and is more efficient than repetitively calling RemoveFirst() with the same value as is currently done when closing the connection.

    0void HTTPHeaders::RemoveAll(std::string_view key)
    1{
    2    auto moved = std::ranges::remove_if(m_map, [key] (auto& pair) {
    3        return CaseInsensitiveEqual(key, pair.first);
    4    });
    5    m_map.erase(moved.begin(), moved.end());
    6}
    

    pinheadmz commented at 3:37 pm on March 11, 2026:
    Switched out RemoveFirst for RemoveAll
  227. in src/httpserver.cpp:503 in 5d13d9819d outdated
    572+
    573+    m_client->m_keep_alive = res.m_keep_alive;
    574+
    575+    // Serialize the response headers
    576+    const std::string headers{res.StringifyHeaders()};
    577+    const auto headers_bytes{std::as_bytes(std::span(headers.begin(), headers.end()))};
    


    hodlinator commented at 9:11 pm on March 5, 2026:

    nit:

    0    const auto headers_bytes{std::as_bytes(std::span{headers})};
    

    pinheadmz commented at 3:42 pm on March 11, 2026:
    using span constructor here, thanks
  228. in src/httpserver.h:165 in 5d13d9819d outdated
    199+    std::string GetURI() const {return m_target;};
    200+    CService GetPeer() const;
    201+    HTTPRequestMethod GetRequestMethod() const { return m_method; }
    202+    std::optional<std::string> GetQueryParameter(const std::string& key) const;
    203+    std::pair<bool, std::string> GetHeader(const std::string& hdr) const;
    204+    std::string ReadBody() const {return m_body;};
    


    hodlinator commented at 9:16 pm on March 5, 2026:

    nit: More lightweight types, regular whitespace inside one-line functions, remove trailing ;:

    0    const std::string& GetURI() const { return m_target; }
    1    const CService& GetPeer() const;
    2    HTTPRequestMethod GetRequestMethod() const { return m_method; }
    3    std::optional<std::string> GetQueryParameter(std::string_view key) const;
    4    std::pair<bool, std::string> GetHeader(std::string_view hdr) const;
    5    const std::string& ReadBody() const { return m_body; }
    

    pinheadmz commented at 6:54 pm on March 11, 2026:
    Sorry about the mess! All taken
  229. in src/httpserver.h:136 in 5d13d9819d outdated
    170+    //! Response headers may be set in advance before response body is known
    171+    HTTPHeaders m_response_headers;
    172 
    173+    explicit HTTPRequest(std::shared_ptr<HTTPClient> client) : m_client(client) {};
    174+    //! Construct with a null client for unit tests
    175+    explicit HTTPRequest() : m_client(nullptr) {};
    


    hodlinator commented at 9:17 pm on March 5, 2026:

    meganit: Slightly more efficient to explicitly call constructor that doesn’t need to perform nullptr-check:

    0    explicit HTTPRequest() : m_client{} {}
    

    pinheadmz commented at 6:56 pm on March 11, 2026:
    👍
  230. in src/httpserver.h:134 in 5d13d9819d outdated
    168+    std::shared_ptr<HTTPClient> m_client;
    169+
    170+    //! Response headers may be set in advance before response body is known
    171+    HTTPHeaders m_response_headers;
    172 
    173+    explicit HTTPRequest(std::shared_ptr<HTTPClient> client) : m_client(client) {};
    


    hodlinator commented at 9:18 pm on March 5, 2026:

    nit: Might as well move-construct from copy, remove trailing ;:

    0    explicit HTTPRequest(std::shared_ptr<HTTPClient> client) : m_client{std::move(client)} {}
    

    pinheadmz commented at 6:59 pm on March 11, 2026:
    👍
  231. in src/test/httpserver_tests.cpp:107 in 5d13d9819d outdated
    105+        // Writing response headers
    106+        HTTPHeaders headers{};
    107+        BOOST_CHECK(!headers.FindFirst("Cache-Control"));
    108+        headers.Write("Cache-Control", "no-cache");
    109+        // Check case-insensitive key matching
    110+        BOOST_CHECK_EQUAL(headers.FindFirst("Cache-Control").value(), "no-cache");
    


    hodlinator commented at 9:22 pm on March 5, 2026:

    nit: optional supports comparing with Value, see https://en.cppreference.com/w/cpp/utility/optional/operator_cmp.html operator==( const optional<T>& opt, const U& value ).

    Here and elsewhere:

    0        BOOST_CHECK_EQUAL(headers.FindFirst("Cache-Control"), "no-cache");
    

    pinheadmz commented at 10:55 am on March 11, 2026:
    Thanks, cleaned up .value() and also .has_value() when testing optionals
  232. in src/test/httpserver_tests.cpp:219 in 5d13d9819d outdated
    217+    {
    218+        // Malformed: no spaces between data
    219+        const std::string too_short_request_line = "GET/HTTP/1.0\r\nHost: 127.0.0.1\r\n\r\n";
    220+        HTTPRequest req;
    221+        std::vector<std::byte> buffer{StringToBuffer(too_short_request_line)};
    222+        LineReader reader(buffer, MAX_HEADERS_SIZE);
    


    hodlinator commented at 9:29 pm on March 5, 2026:

    Instantiating strings and then copying them to a vector is needlessly inefficient.

    Could add this to src/test/util/str.h/cpp:

    0/** Returns a span view of the string. */
    1std::span<const std::byte> StringToBytes(std::string_view str LIFETIMEBOUND)
    2{
    3    return std::as_bytes(std::span{str});
    4}
    

    Then we can simplify to:

    0        std::span too_short_request_line{StringToBytes("GET/HTTP/1.0\r\nHost: 127.0.0.1\r\n\r\n")};
    1        HTTPRequest req;
    2        LineReader reader(too_short_request_line, MAX_HEADERS_SIZE);
    

    Doing that everywhere possible I only end up with one remaining use of StringToBuffer, in http_response_tests.


    pinheadmz commented at 3:08 pm on March 11, 2026:
    Great improvement thanks, span and view make way more sense. Should’ve done this in #34242 I will apply the change to those tests as well, and maybe split that off into a smaller pre-factor PR
  233. in src/test/util/str.h:48 in 5d13d9819d outdated
    42@@ -42,4 +43,13 @@ void ForEachNoDup(CharType (&string)[StringLength], CharType min_char, CharType
    43     }
    44 }
    45 
    46+/**
    47+ * Returns a byte vector filled with data from a string. Used to test string-encoded
    48+ * data from a socket like HTTP headers.
    


    hodlinator commented at 9:33 pm on March 5, 2026:

    meganit: Documenting where a helper function for tests is used seems a bit much?

    0 * Returns a byte vector filled with data from a string.
    

    pinheadmz commented at 10:49 am on March 11, 2026:
    Ok removed the example from the comment.
  234. in src/httpserver.h:233 in c44b1c206d
    224@@ -224,6 +225,20 @@ class HTTPHeaders
    225      */
    226     std::vector<std::pair<std::string, std::string>> m_map;
    227 };
    228+
    229+class HTTPResponse
    230+{
    231+public:
    232+    int m_version_major;
    233+    int m_version_minor;
    


    hodlinator commented at 9:37 pm on March 5, 2026:

    nit in 4ca42bd8019d854a082d9a4032bf182d960dcddf “http: Implement HTTPResponse class”: Might as well use correct types from the start instead of changing later:

    0-    int m_version_major;
    1-    int m_version_minor;
    2+    uint8_t m_version_major;
    3+    uint8_t m_version_minor;
    

    pinheadmz commented at 11:35 am on March 13, 2026:
    thanks, made this always uint8_t i think this was an artifact of a previous rebase.
  235. in src/rpc/protocol.h:25 in 5d13d9819d outdated
    19@@ -20,6 +20,20 @@ enum HTTPStatusCode
    20     HTTP_SERVICE_UNAVAILABLE   = 503,
    21 };
    22 
    23+//! Mapping of HTTP status codes to short string explanation.
    24+//! Copied from libevent http.c success_phrases[] and client_error_phrases[]
    25+const std::map<HTTPStatusCode, std::string> HTTPReason{
    


    hodlinator commented at 9:43 pm on March 5, 2026:

    nit: Could shave off some static initialization overhead by making constexpr, and use more common naming convention:

    0constexpr std::pair<HTTPStatusCode, const char*> g_http_reasons[]{
    

    Downside: requires more verbose queries in 2 places unless extracted to a helper:

    0-    res.m_reason = HTTPReason.find(res.m_status)->second;
    1+    res.m_reason = std::ranges::find_if(g_http_reasons, [&] (auto& pair) { return pair.first == res.m_status; })->second;
    

    pinheadmz commented at 5:16 pm on March 10, 2026:
    Took the extra step here to create a struct with its own lookup method. The table is compile-time and the getter handles missing values to address #32061 (review)
  236. in src/test/util/setup_common.h:280 in 5d13d9819d outdated
    275+private:
    276+    //! Save the original value of CreateSock here and restore it when the test ends.
    277+    decltype(CreateSock) m_create_sock_orig;
    278+
    279+    //! Queue of connected sockets returned by listening socket (represents network interface)
    280+    std::shared_ptr<DynSock::Queue> m_accepted_sockets{std::make_shared<DynSock::Queue>()};
    


    hodlinator commented at 1:54 pm on March 6, 2026:

    Ownership/lifetime should be precisely declared if possible. We know that the TestingSetup will be created before and destroyed after the running of the tests.

    0    DynSock::Queue m_accepted_sockets;
    

    This would also push SocketTestingSetup::ConnectClient() to be non-const, which would resolve #32061 (review).

    I would also modify the test types to further enforce it:

     0    net: Nail down relation between DynSock & Queue
     1    
     2    Queue owner ensures DynSock::Accept() & Wait() are only called while the Queue exists.
     3
     4diff --git a/src/test/util/net.cpp b/src/test/util/net.cpp
     5index 73e5a58d5d..653e21f041 100644
     6--- a/src/test/util/net.cpp
     7+++ b/src/test/util/net.cpp
     8@@ -346,7 +346,7 @@ void DynSock::Pipe::WaitForDataOrEof(UniqueLock<Mutex>& lock)
     9     });
    10 }
    11 
    12-DynSock::DynSock(std::shared_ptr<Pipes> pipes, std::shared_ptr<Queue> accept_sockets)
    13+DynSock::DynSock(std::shared_ptr<Pipes> pipes, Queue* accept_sockets)
    14     : m_pipes{pipes}, m_accept_sockets{accept_sockets}
    15 {
    16 }
    17@@ -403,7 +403,7 @@ bool DynSock::WaitMany(std::chrono::milliseconds timeout, EventsPerSock& events_
    18             if ((events.requested & Sock::RECV) != 0) {
    19                 auto dyn_sock = reinterpret_cast<const DynSock*>(sock.get());
    20                 uint8_t b;
    21-                if (dyn_sock->m_pipes->recv.GetBytes(&b, 1, MSG_PEEK) == 1 || !dyn_sock->m_accept_sockets->Empty()) {
    22+                if (dyn_sock->m_pipes->recv.GetBytes(&b, 1, MSG_PEEK) == 1 || (dyn_sock->m_accept_sockets && !dyn_sock->m_accept_sockets->Empty())) {
    23                     events.occurred |= Sock::RECV;
    24                     at_least_one_event_occurred = true;
    25                 }
    26diff --git a/src/test/util/net.h b/src/test/util/net.h
    27index 247eba8ea0..14f29680e5 100644
    28--- a/src/test/util/net.h
    29+++ b/src/test/util/net.h
    30@@ -5,6 +5,7 @@
    31 #ifndef BITCOIN_TEST_UTIL_NET_H
    32 #define BITCOIN_TEST_UTIL_NET_H
    33 
    34+#include <attributes.h>
    35 #include <compat/compat.h>
    36 #include <netmessagemaker.h>
    37 #include <net.h>
    38@@ -336,7 +337,7 @@ public:
    39      * [@param](/bitcoin-bitcoin/contributor/param/)[in] pipes Send/recv pipes used by the Send() and Recv() methods.
    40      * [@param](/bitcoin-bitcoin/contributor/param/)[in] accept_sockets Sockets to return by the Accept() method.
    41      */
    42-    explicit DynSock(std::shared_ptr<Pipes> pipes, std::shared_ptr<Queue> accept_sockets);
    43+    explicit DynSock(std::shared_ptr<Pipes> pipes, Queue* accept_sockets LIFETIMEBOUND);
    44 
    45     ~DynSock();
    46 
    47@@ -356,7 +357,7 @@ private:
    48     DynSock& operator=(Sock&&) override;
    49 
    50     std::shared_ptr<Pipes> m_pipes;
    51-    std::shared_ptr<Queue> m_accept_sockets;
    52+    Queue* const m_accept_sockets;
    53 };
    54 
    55 template <typename... Args>
    

    pinheadmz commented at 7:04 pm on March 10, 2026:
    Good call, I know how much you hate shared_ptr’s already ;-) I’ll open a small PR with changes to DynSock. So far only this PR and #30988 use it, nothing on master! I’m going to make the Queue optional as well since it’s not always needed.
  237. in src/httpserver.cpp:596 in 5d13d9819d outdated
    684 
    685-    // Unlisten sockets, these are what make the event loop running, which means
    686-    // that after this and all connections are closed the event loop will quit.
    687-    for (evhttp_bound_socket *socket : boundSockets) {
    688-        evhttp_del_accept_socket(eventHTTP, socket);
    689+util::Result<void> HTTPServer::BindAndStartListening(const CService& to)
    


    hodlinator commented at 1:59 pm on March 6, 2026:

    Since we now have Expected which is a simpler type than Result and closer to std::expected, it should be used if possible. The error strings are never consumed in their translated form, so we can use plain std::string for errors.

    0util::Expected<void, std::string> HTTPServer::BindAndStartListening(const CService& to)
    

    Errors:

    0-        return util::Error{Untranslated(strprintf("Bind address family for %s not supported", to.ToStringAddrPort()))};
    1+        return util::Unexpected{strprintf("Bind address family for %s not supported", to.ToStringAddrPort())};
    

    pinheadmz commented at 6:16 pm on March 11, 2026:
    Took this suggestion after some thought. Result seemed simpler to me and reporting errors to the user is mentioned in the comments as the advantage over Expected. However as you pointed out we don’t translate this error message so it makes sense to use the simpler type.
  238. in src/httpserver.h:322 in 5d13d9819d outdated
    383+     * @param[in] listen_sock Socket on which to accept the connection.
    384+     * @param[out] addr Address of the peer that was accepted.
    385+     * @return Newly created socket for the accepted connection.
    386      */
    387-    std::pair<bool, std::string> GetHeader(const std::string& hdr) const;
    388+    std::unique_ptr<Sock> AcceptConnection(const Sock& listen_sock, CService& addr);
    


    hodlinator commented at 2:05 pm on March 6, 2026:

    meganit: Could use pair instead of out-reference-argument.

    0    * [@return](/bitcoin-bitcoin/contributor/return/) Newly created socket for the accepted connection and address for the peer.
    1    */
    2    std::optional<std::pair<std::unique_ptr<Sock>, CService>> AcceptConnection(const Sock& listen_sock);
    

    pinheadmz commented at 7:18 pm on March 11, 2026:
    I’m going to keep this as-is but open to pushback. My reason is that the entire stack keeps the sockaddr and socket separate until they are finally joined in an HTTPClient: accept() -> Sock::Accept() -> AcceptConnection() -> NewSockAccepted() -> HTTPClient
  239. in src/httpserver.h:392 in 5d13d9819d outdated
    492+
    493+    //! Remote address of connected client
    494+    CService m_addr;
    495+
    496+    //! IP:port of connected client, cached for logging purposes
    497+    std::string m_origin;
    


    hodlinator commented at 2:17 pm on March 6, 2026:

    Since HTTPClient disallows copies (and implictly disallows moves), we might as well introduce these members as const.

    0    const HTTPServer::Id m_id;
    1
    2    //! Remote address of connected client
    3    const CService m_addr;
    4
    5    //! IP:port of connected client, cached for logging purposes
    6    const std::string m_origin;
    

    pinheadmz commented at 7:20 pm on March 11, 2026:
    made these members const
  240. in src/httpserver.h:177 in 5d13d9819d outdated
    220+    /**
    221+     * Each connection is assigned an unique id of this type.
    222+     */
    223+    using Id = int64_t;
    224+
    225+    explicit HTTPServer(std::function<void(std::unique_ptr<HTTPRequest>&&)> func) : m_request_dispatcher(func) {};
    


    hodlinator commented at 2:20 pm on March 6, 2026:

    nit: Move, trailing ;.

    0    explicit HTTPServer(std::function<void(std::unique_ptr<HTTPRequest>&&)> func) : m_request_dispatcher{std::move(func)} {}
    

    pinheadmz commented at 7:24 pm on March 11, 2026:
    👍
  241. in src/httpserver.h:335 in b93361bf36
    336      */
    337-    std::unique_ptr<Sock> AcceptConnectionFromListeningSocket(CService& addr)
    338-    {
    339-        return AcceptConnection(*m_listen.front(), addr);
    340-    }
    341+    std::shared_ptr<HTTPClient> GetFirstConnection() { return m_connected.front(); }
    


    hodlinator commented at 2:32 pm on March 6, 2026:

    nit in 06c66aaa695e0ee3fca3f06b0b09bc37b353553d “HTTPServer: start an I/O loop in a new thread and accept connections”: No need to have full shared_ptr, can return by const ref:

    0-    std::shared_ptr<HTTPClient> GetFirstConnection() { return m_connected.front(); }
    1+    const HTTPClient& GetFirstConnection() { return *m_connected.front(); }
    

    pinheadmz commented at 2:49 pm on March 13, 2026:
    Switched this from ptr to ref, thanks. Also had to change the argument to CloseConnection() which is not a temporary method like GetFirstConnection() but it makes sense there as well. That one could even just accept a HTTPServer::Id but I’ll keep it as a HTTPClient& for now. (also addresses #32061 (review))
  242. in src/httpserver.cpp:894 in 5d13d9819d outdated
    1092+        DisconnectClients();
    1093+    }
    1094 }
    1095 
    1096-CService HTTPRequest::GetPeer() const
    1097+void HTTPServer::MaybeDispatchRequestsFromClient(std::shared_ptr<HTTPClient> client) const
    


    hodlinator commented at 2:34 pm on March 6, 2026:

    Can avoid copying shared_ptr:

    0void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptr<HTTPClient>& client) const
    

    pinheadmz commented at 6:21 pm on March 11, 2026:
    using reference here, thanks
  243. pinheadmz force-pushed on Mar 7, 2026
  244. pinheadmz commented at 2:40 pm on March 7, 2026: member

    push to 7603efcf67490e5daf28121ef6db6c30f2b44502:

    • Rebase on master to fix conflict from #34562 moving HasReason() to common

    Address feedback from @vasild including nits, refactors, feature extensions and a few approach changes:

    • rename m_map -> m_headers
    • use raw strings in tests
    • deduplicate permanent socket error-checking code
    • flesh out chunked transfer encoding from HTTP spec
    • clean up HTTPClient disconnect flags
    • fix regression if server is busy past idle timeout (will soon open a test on master to assert current behavior) @vasild - amazing feedback! Thank you for the thorough review and for following along with the HTTP spec as well <3
  245. pinheadmz force-pushed on Mar 7, 2026
  246. DrahtBot added the label CI failed on Mar 7, 2026
  247. DrahtBot commented at 2:48 pm on March 7, 2026: contributor

    🚧 At least one of the CI tasks failed. Task 32 bit ARM: https://github.com/bitcoin/bitcoin/actions/runs/22801000447/job/66142652455 LLM reason (✨ experimental): Build failed due to -Werror treating a misleading-indentation warning in httpserver.cpp (duplicate throw line) as an error.

    Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

    • Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.

    • A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.

    • An intermittent issue.

    Leave a comment here, if you need help tracking down a confusing failure.

  248. pinheadmz commented at 2:48 pm on March 7, 2026: member

    push to 39ca86ba6a:

    Fix embarrassing rebase mistake

  249. DrahtBot removed the label Needs rebase on Mar 7, 2026
  250. DrahtBot removed the label CI failed on Mar 7, 2026
  251. in src/httpserver.cpp:1122 in 39ca86ba6a
    1365+    g_http_server->SetServerTimeout(std::chrono::seconds(gArgs.GetIntArg("-rpcservertimeout", DEFAULT_HTTP_SERVER_TIMEOUT)));
    1366+
    1367+    // Bind HTTP server to specified addresses
    1368+    std::vector<std::pair<std::string, uint16_t>> endpoints{GetBindAddresses()};
    1369+    bool bind_success{false};
    1370+    for (std::vector<std::pair<std::string, uint16_t>>::iterator i = endpoints.begin(); i != endpoints.end(); ++i) {
    


    hodlinator commented at 9:11 am on March 10, 2026:

    nit: Would be nice to refactor, but I understand not wanting to add additional commits for that reason:

    0    for (const auto& [address, port] : endpoints) {
    

    pinheadmz commented at 6:33 pm on March 11, 2026:
    Great cleanup, not sure why the original I copied from didn’t even use auto
  252. in src/httpserver.cpp:265 in 39ca86ba6a
    296-        LogWarning("libevent: %s", msg);
    297-        break;
    298-    default: // EVENT_LOG_ERR and others are mapped to error
    299-        LogError("libevent: %s", msg);
    300-        break;
    301+    for (const auto &item : m_headers) {
    


    hodlinator commented at 9:20 am on March 10, 2026:

    meganit: More common symbol placing.

    0    for (const auto& item : m_headers) {
    

    pinheadmz commented at 6:34 pm on March 11, 2026:
    👍
  253. in src/httpserver.h:70 in 39ca86ba6a
     97+     * @param[in] key The field-name of the header to search for
     98+     * @returns The value of the first header that matches the provided key
     99+     *          nullopt if key is not found
    100+     */
    101+    std::optional<std::string> FindFirst(const std::string& key) const;
    102+    void Write(const std::string& key, const std::string& value);
    


    hodlinator commented at 9:24 am on March 10, 2026:

    nits:

    • FindFirst() + RemoveFirst(): Could use more lightweight type.
    • Write(): Avoid extra heap allocations through &&-sink and std::move()ing:
    0    std::optional<std::string> FindFirst(std::string_view key) const;
    1    void Write(std::string&& key, std::string&& value);
    

    pinheadmz commented at 2:37 pm on March 12, 2026:
    using temporary object reference here, thanks
  254. hodlinator commented at 11:10 am on March 10, 2026: contributor

    Concept ACK 39ca86ba6a691be304ac4c32dd64af38d98e26e7

    Only managed to deliver fairly surface-level review so far. Not sure I’ll be able to deliver deeper HTTP protocol feedback, but glad to see some has already been posted. Good to see benchmarks as well.

    Created a branch with my inline comment suggestions embedded: https://github.com/hodlinator/bitcoin/tree/pr/32061_suggestions_39ca86b

  255. in src/httpserver.cpp:723 in b53198cbf5
    718+{
    719+    for (const auto &item : m_headers) {
    720+        if (CaseInsensitiveEqual(key, item.first)) {
    721+            return item.second;
    722+        }
    723+    }
    


    theStack commented at 4:35 pm on March 10, 2026:

    style nit: could take use std::ranges::find_if to avoid coding the loop manually, e.g.

    0    auto it = std::ranges::find_if(m_headers, [&](const auto& item) { return CaseInsensitiveEqual(key, item.first); });
    1    if (it != m_headers.end()) return it->second;
    

    (maybe more a matter of personal taste though, probably the current code is simple enough and fine as well)


    pinheadmz commented at 3:55 pm on March 13, 2026:
    I think I’ll keep as is for now, but feel free to push back
  256. in src/httpserver.cpp:923 in e0052cf949
    918+    res.m_version_major = m_version_major;
    919+    res.m_version_minor = m_version_minor;
    920+
    921+    // Add response code and look up reason string
    922+    res.m_status = status;
    923+    res.m_reason = HTTPReason.find(status)->second;
    


    theStack commented at 5:04 pm on March 10, 2026:

    could check if the status key is in HTTPReason before dereferencing the iterator to avoid potential UB? (though this is currently not an issue, and a a call-site would need to apply an explicit cast, e.g.

     0diff --git a/src/test/httpserver_tests.cpp b/src/test/httpserver_tests.cpp
     1index 0db45a7b0a..f4379d183d 100644
     2--- a/src/test/httpserver_tests.cpp
     3+++ b/src/test/httpserver_tests.cpp
     4@@ -498,7 +498,7 @@ BOOST_AUTO_TEST_CASE(http_server_socket_tests)
     5                 BOOST_CHECK_EQUAL(client->m_origin, "5.5.5.5:6789");
     6
     7                 // Respond to request
     8-                requests.front()->WriteReply(HTTP_OK, "874140\n");
     9+                requests.front()->WriteReply(static_cast<HTTPStatusCode>(1337), "874140\n");
    10
    11                 break;
    12             }
    

    but just for belts and suspenders)


    pinheadmz commented at 5:50 pm on March 13, 2026:
    Fixed with a custom lookup function that returns "" if code is missing, as part of addressing #32061 (review)
  257. theStack commented at 5:11 pm on March 10, 2026: contributor

    Concept ACK

    Planning to do some related RFC reading over the next days.

    meta nit: the commit message in 0f72bdbf7b3e16fbe566083418a5d38ef4adcc34 is outdated, still mentioning unordered_map for HTTPHeaders

  258. string: replace AsciiCaseInsensitiveKeyEqual with CaseInsensitiveEqual
    This reverts commit eea38787b9be99c3f192cb83fc18358397e4ab52 from PR #34242
    
    We do not need comparators for HTTPHeaders since it is not using unordered_map anymore.
    We only need a simple, locale-independent, ascii-only compare function for
    a vector of key-value pairs of strings.
    
    We have CaseInsensitiveEqual already in test utils, this commit moves
    it to the strencodings module for use in the application code.
    67e0364ed2
  259. test: util: implement and use StringToBytes instead of StringToBuffer
    Co-Authored by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
    6caf03574d
  260. Make DynSock accepted sockets queue optional, with precise lifetime
    When DynSock is used to represent a connected socket (e.g. a client)
    the data I/O pipes are needed but not the m_accepted_sockets Queue,
    because connected sockets do not create more connected sockets.
    
    When DynSock is used to represent a listening socket, the Queue
    is necessary to create connected sockets upon mocked connection, but
    the Queue does not need to be a std::shared_ptr as long as it
    is guaranteed to live as long as the DynSock.
    
    Co-Authored by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
    4b2500b68c
  261. http: enclose libevent-dependent code in a namespace
    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.
    adf6ea1a8c
  262. http: Implement HTTPHeaders class
    see:
    https://www.rfc-editor.org/rfc/rfc2616#section-4.2
    https://www.rfc-editor.org/rfc/rfc7231#section-5
    https://www.rfc-editor.org/rfc/rfc7231#section-7
    https://httpwg.org/specs/rfc9111.html#header.field.definitions
    a00fb20543
  263. http: Implement HTTPResponse class
    HTTP Response message:
    https://datatracker.ietf.org/doc/html/rfc1945#section-6
    
    Status line (first line of response):
    https://datatracker.ietf.org/doc/html/rfc1945#section-6.1
    
    Status code definitions:
    https://datatracker.ietf.org/doc/html/rfc1945#section-9
    380fb8119a
  264. pinheadmz force-pushed on Mar 13, 2026
  265. pinheadmz force-pushed on Mar 13, 2026
  266. DrahtBot added the label CI failed on Mar 13, 2026
  267. http: Implement HTTPRequest class
    HTTP Request message:
    https://datatracker.ietf.org/doc/html/rfc1945#section-5
    
    Request Line aka Control Line aka first line:
    https://datatracker.ietf.org/doc/html/rfc1945#section-5.1
    
    See message_read_status() in libevent http.c for how
    `MORE_DATA_EXPECTED` is handled there
    2f88838b53
  268. http: Introduce HTTPServer class and implement binding to listening socket
    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>
    beee5bb850
  269. HTTPServer: implement and test AcceptConnection()
    AcceptConnection() is mostly copied from CConmann in net.cpp
    and then modernized.
    
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    f051ae2201
  270. HTTPServer: generate sequential Ids for each newly accepted connection
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    1cf0284954
  271. http: Introduce HTTPClient class 6ecc6ffe59
  272. HTTPServer: start an I/O loop in a new thread and accept connections
    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>
    bd9052071c
  273. HTTPServer: read requests from connected clients
    `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>
    aa629b4bdb
  274. HTTPserver: support "chunked" Transfer-Encoding af7d40892d
  275. HTTPServer: compose and send replies to connected clients
    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>
    525a717339
  276. HTTPServer: disconnect clients 86077e6dcd
  277. Allow http workers to send data optimistically as an optimization 0559746eb7
  278. HTTPServer: use a queue to pipeline requests from each connected client
    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.
    2d81e29c0e
  279. define HTTP request methods at module level outside of class
    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.
    1c5165edae
  280. Add helper methods to HTTPRequest to match original API
    These methods are called by http_request_cb() and are present in the
    original http_libevent::HTTPRequest.
    036f87335f
  281. refactor: split http_request_cb into libevent callback and dispatch
    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.
    bc1d513201
  282. refactor: split HTTPBindAddresses into config parse and libevent setup
    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.
    6f76c5983f
  283. HTTPServer: implement control methods to match legacy API bcf26e3be9
  284. HTTPServer: disconnect after idle timeout (-rpcservertimeout) 005d51e4c4
  285. http: switch servers from libevent to bitcoin 316d9d729f
  286. fuzz: switch http_libevent::HTTPRequest to http_bitcoin::HTTPRequest 8282801cac
  287. http: remove libevent usage from this subsystem fe18c0b1ed
  288. pinheadmz force-pushed on Mar 13, 2026
  289. pinheadmz commented at 8:06 pm on March 13, 2026: member

    push to fe18c0b1ed24bbf4ee15775590bdadcc57a43ddf:

    Address feedback from @hodlinator. Wow, lots of really great comments thank you. Almost everything was code clean ups and C++ style improvements like avoiding data copying and shared_ptr’s, using string_view instead of string, and using somewhat newer codebase utilities like hex literals and Expected.

    Also addressed feedback from @theStack which overlapped a bit with hodlinator.

    Some of these improvements require extra pre-factor commits, which are at the start of the PR now. With enough foresight they perhaps could have been applied in #34242 and #30205. These commits should be easy to review on their own PR which I’ll open soon.

    Meta note: this PR is getting LOOOooooong and github is struggling in some cases to find comments. So I hope I didn’t miss anything. Might be time to open a fresh pull request soon?

  290. DrahtBot removed the label CI failed on Mar 13, 2026

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-03-15 09:13 UTC

This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me