http: Make `HTTPRequest` update state internally #36124

pull hodlinator wants to merge 4 commits into bitcoin:master from hodlinator:2026/08/http_refactors changing 4 files +124 −145
  1. hodlinator commented at 7:58 AM on August 31, 2026: contributor

    Issues in order of importance

    #35829 made the fields of HTTPRequest & HTTPRemoteClient private, but still allows anyone with a reference to a request to update the internal state enum. #35829 (review)

    HTTPRemoteClient::TryReadRequest() only needs to be a static method in order to forward a shared_ptr of a client to a new request.

    Commits solving these issues in same order

    • http: Remove HTTPRequest::SetState() and introduce HTTPRequest::Load() - Removes the ability for other classes to directly modify the request state.
    • 2 refactors: Extract HTTPRemoteClient::TryReadRequestInternal() from a combination of HTTPRemoteClient::TryReadRequest() and HTTPRemoteClient::ReadRequest() (the latter is removed)
  2. DrahtBot added the label RPC/REST/ZMQ on Aug 31, 2026
  3. DrahtBot commented at 7:59 AM on August 31, 2026: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--006a51241073e994b41acfe9ec718e94-->

    External sites

    <!--021abf342d371248e50ceaed478a90ca-->

    Reviews

    See the guideline and AI policy for information on the review process.

    Type Reviewers
    ACK pinheadmz
    Concept ACK jeanpablojp
    Stale ACK janb84, winterrdog

    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

    No conflicts as of last run.

    <!--5faf32d7da4f0f540f40219e4f7537a3-->

  4. jeanpablojp commented at 7:38 PM on August 31, 2026: contributor

    Concept ACK

    I ran the flattened ShouldDisconnect() against the version in master over every combination of the flags it reads, with the client inside and outside the idle window and the timeout on and off. Same return value in all 64. The only difference is that master logs the idle timeout even for a client that is already flagged for disconnect.

  5. in src/test/httpserver_tests.cpp:455 in 549cbbf67f outdated
     451 | @@ -484,50 +452,51 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests)
     452 |      public:
     453 |          DummyClient() : HTTPRemoteClient{/*id=*/0, /*addr=*/CService(), /*socket=*/CreateSock(0, 0, 0)} {}
     454 |  
     455 | -        void receive(std::string_view s)
     456 | +        void Receive(std::string_view s)
    


    jeanpablojp commented at 7:38 PM on August 31, 2026:

    The unit tests run ShouldDisconnect() but assert nothing about its first two branches. I broke each one and the suite stayed green both times. interface_http.py does reach both, but it blocks on the socket instead of failing. The test below covers both branches, and the new log line with them.

    <details> <summary>test for both branches and the new log line</summary>

    @@ -746,6 +746,38 @@ BOOST_AUTO_TEST_CASE(http_request_state_tests)
         }
     }
     
    +BOOST_AUTO_TEST_CASE(http_should_disconnect_tests)
    +{
    +    struct Client : HTTPRemoteClient {
    +        Client() : HTTPRemoteClient{/*id=*/0, /*addr=*/CService(), /*socket=*/CreateSock(0, 0, 0)} {}
    +        using HTTPRemoteClient::MutateRecvBuffer;
    +    };
    +    constexpr auto timeout{30s};
    +    const auto now{Now<SteadySeconds>()};
    +
    +    // Nothing to disconnect for yet.
    +    auto idle{std::make_shared<Client>()};
    +    BOOST_CHECK(!idle->ShouldDisconnect(now, timeout, /*disconnect_all=*/false));
    +    // A shutdown waits for a connection that is still busy, and it starts busy.
    +    BOOST_CHECK(!idle->ShouldDisconnect(now, timeout, /*disconnect_all=*/true));
    +    // Past -rpcservertimeout, which a timeout of 0 disables.
    +    BOOST_CHECK(idle->ShouldDisconnect(now + timeout + 1s, timeout, /*disconnect_all=*/false));
    +    BOOST_CHECK(!idle->ShouldDisconnect(now + timeout + 1s, 0s, /*disconnect_all=*/false));
    +
    +    // A request handed to a worker holds the idle timeout off until the reply is sent.
    +    auto busy{std::make_shared<Client>()};
    +    busy->MutateRecvBuffer().append("GET / HTTP/1.0\n\n");
    +    auto request{HTTPRemoteClient::TryReadRequest(busy)};
    +    BOOST_REQUIRE(request);
    +    BOOST_CHECK(!busy->ShouldDisconnect(now + timeout + 1s, timeout, /*disconnect_all=*/false));
    +
    +    // A malformed request flags the client, and that outranks the rest.
    +    auto bad{std::make_shared<Client>()};
    +    bad->MutateRecvBuffer().append("GET / HTTP/1.0\nInvalid header with no colon\n\n");
    +    BOOST_CHECK(!HTTPRemoteClient::TryReadRequest(bad));
    +    BOOST_CHECK(bad->ShouldDisconnect(now, 0s, /*disconnect_all=*/false));
    +}
    +
     BOOST_AUTO_TEST_CASE(http_server_socket_tests)
     {
         // Hard code the timestamp for the Date header in the HTTP response
    @@ -791,6 +823,9 @@ BOOST_AUTO_TEST_CASE(http_server_socket_tests)
         // Create a mock client with pre-loaded request data and add it to the local CreateSock queue.
         // Keep a handle for the mock client's send and receive pipes so we can examine
         // the data it "receives".
    +    // No keep-alive, so the server closes once the reply is flushed, and says so.
    +    DebugLogHelper find_close{"Done sending to client without keep-alive"};
    +
         std::shared_ptr<DynSock::Pipes> mock_client_socket_pipes{ConnectClient(std::as_bytes(std::span(full_request)))};
     
         // Wait up to a minute to find and connect the client in the I/O loop
    

    </details>


    hodlinator commented at 10:07 AM on September 2, 2026:

    Thanks! Added the checks in slightly extended form.


    hodlinator commented at 10:44 AM on September 3, 2026:

    Added co-authorship in #36159.

  6. winterrdog commented at 9:21 AM on September 1, 2026: contributor

    Concept ACK

  7. in src/httpserver.cpp:1211 in 27b5a5cdef outdated
    1217 |          case HTTPRequest::State::Error:
    1218 | -            break;
    1219 | -        }
    1220 | +            return true;
    1221 | +        } // no default case, so the compiler can warn about missing cases
    1222 |      } catch (...) {
    


    winterrdog commented at 3:58 PM on September 1, 2026:

    [!NOTE] i think NONFATAL_UNREACHABLE will do just fine. do not mind the comment below 🤝


    do you think some runtime safety like assert(false) (in debug builds) would be useful here ?

    e.g. similar to how it's done: https://github.com/bitcoin/bitcoin/blob/dc0395c5858a1d55239b82a834e5075cf2069219/src/index/blockfilterindex.cpp#L64-L71

    or here: https://github.com/bitcoin/bitcoin/blob/dc0395c5858a1d55239b82a834e5075cf2069219/src/bitcoin-cli.cpp#L430-L445

  8. hodlinator force-pushed on Sep 2, 2026
  9. in src/httpserver.cpp:1175 in 907d7342b6
    1171 | @@ -1166,47 +1172,35 @@ void HTTPServer::ClearConnectedClients()
    1172 |      m_connected.clear();
    1173 |  }
    1174 |  
    1175 | -void HTTPRemoteClient::ReadRequest(HTTPRequest& req)
    1176 | +bool HTTPRequest::Load(util::LineReader& reader)
    


    janb84 commented at 2:21 PM on September 2, 2026:

    NIT: would it not help the readability to move this function closer to the other load* helpers ? ~ L 378


    hodlinator commented at 9:07 AM on September 3, 2026:

    re #36124 (review): The order of methods in httpserver.cpp is wildly out of sync with the header. I prefer to punt on this for now, to keep the number of commits down.

  10. in src/httpserver.h:162 in 907d7342b6 outdated
     162 | -     * from a receive buffer.
     163 | -     * @param[in]   reader  A LineReader object constructed over a span of data.
     164 | -     * @returns     true    If the request field was parsed.
     165 | -     *              false   If there was not enough data in the buffer to complete the field.
     166 | +     * Try to read an HTTP request. Updates m_state.
     167 | +     * @returns     true    If the request was fully parsed or in error.
    


    janb84 commented at 3:07 PM on September 2, 2026:

    NIT. On error it does not return true. Why not make it void? non-test code already checks getstate() and doing the same in the tests makes the test slightly more strict. The bool loosy communicates 5 states in a bool

         * [@returns](/github-metadata-backup-bitcoin-bitcoin/contributor/returns/)     true    If the request was fully parsed or already complete
    

    hodlinator commented at 9:08 AM on September 3, 2026:

    re #36124 (review): Removed docstring for now as I switched to returning state which seems self-explanatory.

  11. janb84 commented at 3:08 PM on September 2, 2026: contributor

    concept ACK 907d7342b60dfa00ad81e235eccb5808b9c13da6

    The removal of the SetState() makes it a better design (imho), makes the convention based "rule" not to arbitrary set the state, to a a compiler-enforced fact

    Found a few possible NITS.

  12. in src/test/fuzz/http_request.cpp:45 in 907d7342b6


    winterrdog commented at 8:07 AM on September 3, 2026:

    nit: this comment still refers to LoadControlData/LoadHeaders/LoadBody as the call sequence above, but those were replaced by a single Load() call. since the helpers are private now too, it is not immediately clear to a reader where this logic comes from

  13. hodlinator force-pushed on Sep 3, 2026
  14. hodlinator force-pushed on Sep 3, 2026
  15. DrahtBot added the label CI failed on Sep 3, 2026
  16. hodlinator force-pushed on Sep 3, 2026
  17. hodlinator commented at 9:36 AM on September 3, 2026: contributor

    Thanks for the feedback so far! Pushes after 907d7342b60dfa00ad81e235eccb5808b9c13da6 address some nits and expands http_should_disconnect_tests to more completely verify optimistic send behavior.

  18. DrahtBot removed the label CI failed on Sep 3, 2026
  19. hodlinator force-pushed on Sep 3, 2026
  20. hodlinator commented at 10:45 AM on September 3, 2026: contributor

    Decided to split out two PRs from this one as they are quite independent. See #36159 and #36160.

  21. janb84 commented at 1:19 PM on September 3, 2026: contributor

    (re) ACK c43d544b16b61e3182467dba7355cdaa372204c6

    returning the state and not a bool at the load() function, is the better way to go imho :) Have re-reviewed the PR because of the split-off; build tested etc.

    In the commit message, is an API change a behaviour change? super micro nit. The load() documentation block has no @return anymore, is state super clear to everyone ?

  22. DrahtBot requested review from jeanpablojp on Sep 3, 2026
  23. DrahtBot requested review from winterrdog on Sep 3, 2026
  24. winterrdog commented at 8:21 PM on September 5, 2026: contributor

    is state super clear to everyone ?

    had not noticed this. i think we can add the @return with some light documentation.

    State is self-explanatory as a type, but it does not tell a caller that Load() never returns Error (it throws instead), or that the other values mean "keep calling with more data" vs "done", sth that is specific to Load()'s contract. secondly, it keeps consistency with the @throws below and the Doxygen style used elsewhere

  25. hodlinator renamed this:
    http: Make `HTTPRequest` update state internally, etc
    http: Make `HTTPRequest` update state internally
    on Sep 5, 2026
  26. hodlinator force-pushed on Sep 5, 2026
  27. hodlinator commented at 9:00 PM on September 5, 2026: contributor

    Latest push modifies the initial Load() commit only.

    re #36124#pullrequestreview-5102409506:

    In the commit message, is an API change a behaviour change?

    Good point, fixed.

    re #36124#pullrequestreview-5102409506 + #36124 (comment):

    is state super clear to everyone ?

    had not noticed this. i think we can add the @return with some light documentation.

    State is self-explanatory as a type, but it does not tell a caller that Load() never returns Error (it throws instead), or that the other values mean "keep calling with more data" vs "done", sth that is specific to Load()'s contract. secondly, it keeps consistency with the @throws below and the Doxygen style used elsewhere

    Added back @returns. It is true that Load() (re-)throws the first time it sets Error, but subsequent calls will return Error. The return value is only read in unit tests, so not sure if it's worth elaborating further in the doc-string.

  28. winterrdog commented at 9:24 PM on September 5, 2026: contributor

    tACK 62796e659fb79a0c6f1749677ec480a80ec26e6d

    successfully built and tested on this toolchain: Debian/clang++-18/x86_64

  29. DrahtBot requested review from janb84 on Sep 5, 2026
  30. DrahtBot added the label Needs rebase on Sep 10, 2026
  31. hodlinator force-pushed on Sep 10, 2026
  32. DrahtBot removed the label Needs rebase on Sep 10, 2026
  33. DrahtBot added the label Needs rebase on Sep 16, 2026
  34. hodlinator force-pushed on Sep 16, 2026
  35. DrahtBot removed the label Needs rebase on Sep 16, 2026
  36. DrahtBot added the label Needs rebase on Sep 25, 2026
  37. hodlinator force-pushed on Oct 14, 2026
  38. DrahtBot removed the label Needs rebase on Oct 14, 2026
  39. in src/httpserver.h:558 in c84da5e53a outdated
     553 | -     * Updates HTTPRequest.m_state and drains buffer on error.
     554 | -     * @param[in]   req     A HTTPRequest to read into
     555 | -     * @throws std::runtime_error if request is unreadable or violates protocol
     556 | -     */
     557 | -    void ReadRequest(HTTPRequest& req);
     558 | -
    


    winterrdog commented at 10:08 AM on October 15, 2026:

    in commit c84da5e53aa1661a89d522667e7f05c6f6f88a3b

    mostly a concern..

    on a 2nd look, i have started to get some reservations about folding ReadRequest into TryReadRequestInternal. was it mainly removed to simplify things, or was there a problem with keeping it separate ?

    ReadRequest gave us one place to clean up m_recv_buffer after a failed parse. now, each catch block has to remember to do that itself, so it is easy for someone adding a new error case later to forget. keeping the cleanup in one place felt like a nice and easy safety net

    unless there is a reason i am missing, i would lean towards keeping a thin ReadRequest just for the cleanup. it gives us one less thing to worry about when adding new error cases. open to be convinced otherwise. i am mostly trying to understand the direction you want to go


    hodlinator commented at 1:01 PM on October 17, 2026:

    Could potentially add an inner try/catch-block:

    --- a/src/httpserver.cpp
    +++ b/src/httpserver.cpp
    @@ -1104,7 +1104,12 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequestInternal()
         HTTPRequest::State req_state{HTTPRequest::State::Error};
         try {
             // Read data from the buffer into the current request
    -        req_state = m_req->Load(reader);
    +        try {
    +            req_state = m_req->Load(reader);
    +        } catch (...) {
    +            m_recv_buffer.clear();
    +            throw;
    +        }
     
             // Remove the bytes read out of the buffer.
             m_recv_buffer.erase(
    @@ -1119,7 +1124,6 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequestInternal()
                 e.what());
     
             WriteNoStoreErrorReply(*m_req, HTTP_CONTENT_TOO_LARGE);
    -        m_recv_buffer.clear();
             m_disconnect = true;
             return nullptr;
         } catch (const std::runtime_error& e) {
    @@ -1132,7 +1136,6 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequestInternal()
     
             // We failed to read a complete request from the buffer
             WriteNoStoreErrorReply(*m_req, HTTP_BAD_REQUEST);
    -        m_recv_buffer.clear();
             m_disconnect = true;
             return nullptr;
         }
    

    Would prefer doing that over bringing back HTTPRemoteClient::ReadRequest(). WDYT?


    winterrdog commented at 7:16 AM on October 19, 2026:

    i think that will do. let's run with it

    i had 2 alternatives, but both introduced a helper, which i understand you would rather avoid here


    also, i think the reader can be scoped to the outer try-catch instead of function scope, since that is the only scope it is used in, thus ending up with sth like this:

    a/src/httpserver.cpp b/src/httpserver.cpp index 8c9cb5b3ca..5927b16223 100644
    --- a/src/httpserver.cpp
    +++ b/src/httpserver.cpp
    @@ -1099,12 +1099,17 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequest(const std::shared_
     std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequestInternal()
     {
         if (m_recv_buffer.empty()) return nullptr;
    -    LineReader reader(m_recv_buffer, MAX_HEADERS_SIZE);
    
         HTTPRequest::State req_state{HTTPRequest::State::Error};
         try {
             // Read data from the buffer into the current request
    -        req_state = m_req->Load(reader);
    +        LineReader reader(m_recv_buffer, MAX_HEADERS_SIZE);
    +        try {
    +            req_state = m_req->Load(reader);
    +        } catch (...) {
    +            m_recv_buffer.clear();
    +            throw;
    +        }
    
             // Remove the bytes read out of the buffer.
             m_recv_buffer.erase(
    @@ -1119,7 +1124,6 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequestInternal()
                 e.what());
    
             WriteNoStoreErrorReply(*m_req, HTTP_CONTENT_TOO_LARGE);
    -        m_recv_buffer.clear();
             m_disconnect = true;
             return nullptr;
         } catch (const std::runtime_error& e) {
    @@ -1132,7 +1136,6 @@ std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequestInternal()
    
             // We failed to read a complete request from the buffer
             WriteNoStoreErrorReply(*m_req, HTTP_BAD_REQUEST);
    -        m_recv_buffer.clear();
             m_disconnect = true;
             return nullptr;
         }
    

    hodlinator commented at 7:32 AM on October 19, 2026:

    Nice! Narrowed the scope of reader in latest push.

  40. bitcoin deleted a comment on Oct 17, 2026
  41. hodlinator force-pushed on Oct 19, 2026
  42. winterrdog commented at 3:42 PM on October 19, 2026: contributor

    reACK 656ddb44ba0db001d503cce52db5ac6e6db897f1

    LGTM!

  43. in src/httpserver.h:164 in b6deeac911 outdated
     167 |      explicit HTTPRequest() : m_client{} {}
     168 |  
     169 |      /**
     170 | -     * Methods that attempt to parse HTTP request fields line-by-line
     171 | -     * from a receive buffer.
     172 | -     * @param[in]   reader  A LineReader object constructed over a span of data.
    


    pinheadmz commented at 5:06 PM on October 27, 2026:

    b6deeac911e5db6c14f4fe6653bddd329a8e3eed

    nit: why remove the doxygen param?


    hodlinator commented at 7:20 PM on October 27, 2026:

    Was keeping it for the now-private Load*() methods. In the latest push I've instead moved it to the public method and rephrased it to be more informative instead of describing the LineReader type.

  44. pinheadmz commented at 5:39 PM on October 27, 2026: member

    concept ACK

    "Ralph" caught something that I think needs to be addressed: https://review.fish.foo/traces/36124-656ddb44ba0db001d503cce52db5ac6e6db897f1-890-2.html

    Edge case:

    1. m_send_buffer is full (for some reason), so m_req gets held back (throttled) and is not dispatched to a worker
    2. m_recv_buffer is empty because client sent one request and its now in m_req
    3. this returns early every I/O loop and m_req never(*) gets dispatched, even after the send buffer drains:
    std::unique_ptr<HTTPRequest> HTTPRemoteClient::TryReadRequestInternal()
    {
        if (m_recv_buffer.empty()) return nullptr;
    

    (*) = if the client sends more data it will unclog the jam because then m_recv_buffer won't be empty anymore.

    Suggested fix from Claude: skip only the parsing when the buffer is empty, and start req_state from the request's current state. This way a Complete request still gets dispatched to a worker.

      HTTPRequest::State req_state{m_req->GetState()};
      try {
          if (!m_recv_buffer.empty()) {
              LineReader reader(m_recv_buffer, MAX_HEADERS_SIZE);
              ...
          }   
      }   
    
  45. http: Extract HTTPRequest::Load() to remove SetState()
    Removes the ability for other classes to directly modify the request state.
    
    Refactor with slight API change as pre-existing Load*-methods are made private, which mainly affects unit tests.
    05897e53b2
  46. refactor: Extract HTTPRemoteClient::TryReadRequestInternal() from static HTTPRemoteClient::TryReadRequest()
    TryReadRequest() now only remains as an outer method for the sake of passing the client shared_ptr to the new request.
    9692e6f7d0
  47. refactor: Flatten HTTPRemoteClient::ReadRequest() into HTTPRemoteClient::TryReadRequestInternal() 148210fe99
  48. hodlinator force-pushed on Oct 27, 2026
  49. hodlinator commented at 7:26 PM on October 27, 2026: contributor

    Thank you @pinheadmz & Ralph for catching the accidental behavior change/bug. Applied suggestion, making the last commit more of a pure refactor.

  50. pinheadmz commented at 4:16 PM on October 28, 2026: member

    doxygen nit, totally ignorable

    Turns out I had the grouping syntax wrong. Groups need @name to work, otherwise the description is only attached to the first item.

    fixed:

    diff --git a/src/httpserver.h b/src/httpserver.h
    index cff876abb8..bbe7a7c79d 100644
    --- a/src/httpserver.h
    +++ b/src/httpserver.h
    @@ -132,6 +132,7 @@ private:
     
     struct HTTPVersion {
         /**
    +     * [@name](/github-metadata-backup-bitcoin-bitcoin/contributor/name/) Protocol version
          * Default HTTP protocol version 1.1 is used by error responses
          * when a request is unreadable.
          */
    @@ -199,6 +200,7 @@ public:
     
     private:
         /**
    +     * [@name](/github-metadata-backup-bitcoin-bitcoin/contributor/name/) Request parsers
          * Methods that attempt to parse HTTP request fields line-by-line
          * from a receive buffer.
          * [@returns](/github-metadata-backup-bitcoin-bitcoin/contributor/returns/)     true    If the request field was parsed.
    

    <img width="698" height="180" alt="Screenshot 2026-10-28 at 12 14 45 PM" src="https://github.com/user-attachments/assets/98c753fd-7139-4f5a-9efd-3efc11ea25bd" /> <img width="828" height="314" alt="Screenshot 2026-10-28 at 12 14 36 PM" src="https://github.com/user-attachments/assets/b9086376-d6dd-4a1b-9558-49ff883d1d30" />

  51. pinheadmz approved
  52. pinheadmz commented at 4:19 PM on October 28, 2026: member

    ACK 148210fe99bb54583fd47f2c39bf0b9390161185

    Clean refactor, technically a little granular unit test coverage lost from private-izing internal methods but it makes for cleaner classes.

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

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA256
    
    ACK 148210fe99bb54583fd47f2c39bf0b9390161185
    -----BEGIN PGP SIGNATURE-----
    
    iQJPBAEBCAA5FiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmriIDEbFIAAAAAABAAO
    bWFudTIsMi41KzEuMTIsMCwzAAoJEOfimEtiick66N8QALEtD02xBq+MIOB5FIrj
    NyxK4JmDLwfYuCo2Qap6GYIRQrDHgsxPmZzpjwqcQg39PqNhTvfHB91qB8SvB4Uv
    OJoUyhhGwzWRj8GRzgJj5cbj+XazxGpYf9zQUSZbjXSmiX3wecOp7LdEjVpNzKxs
    va/qMjdTUqSfEcRY1vSuCfDCZYKMoioyXvYidOsuErHnwGkHWHpA8llV0L3jNqmz
    QNibmLF/fv5WANaIE5JuDQ6M4rfMa10zJ6QLsMRCTPF6BOWya5gXCY5XswiLh0CQ
    RMQIxTOyn6cBPaR8vw159cHfruv6XN6lZoeKjbF+u/DCRZz4j/k3DI8LyefKN4K7
    IIC10Wqxp+0welsSn/sqk64A5tYCAzFr1a00vodJU/VjSQBn3lUh3sR4Mx+/LQLK
    jpOUSQg9maseCDoxC7aWjvFHhSpJsS8izALyLKT5bFb8vIpeuITo1x2bbciq91kP
    3tIedAEGK8KfstNIz+Dk2GgCxZSUkziAKJvJ3uMFERQHRg7A9rI2DKU0VObA+ntn
    vdEocdYmvcy937SqumDhtfNvnjDE8guOBzey33C2IfmNo3Z8XTCzPOZmiq81fiJ/
    mDYaZ/HC324XZqQhuzzGsigE3C/ekkMwP6qYIOcqFZC3qEL2+cc0SQ7x8RiyIejj
    627R1cmfGJw5GaZnfO/KE27R
    =JgGp
    -----END PGP SIGNATURE-----
    

    pinheadmz's public key is on openpgp.org

    </details>

  53. DrahtBot requested review from winterrdog on Oct 28, 2026
  54. doc: Add missing names to activate grouping syntax 6a5212fb97
  55. in src/test/fuzz/http_request.cpp:54 in 148210fe99 outdated
      53 | @@ -51,9 +54,7 @@ void SingleShotParse(const std::string& http_buffer, FuzzedDataProvider& provide
      54 |      HTTPRequest http_request;
    


    hodlinator commented at 7:51 PM on October 28, 2026:

    inline comment thread in response to @pinheadmz #36124 (comment):

    Found a couple of more groupings:

    commit 94afe512f028f56510a1362ee447383fa7bfa88a (HEAD -> 2026/08/http_refactors)
    Author: Hodlinator <172445034+hodlinator@users.noreply.github.com>
    Date:   Wed Oct 28 20:29:07 2026 +0100
    
        doc: Add missing [@name](/github-metadata-backup-bitcoin-bitcoin/contributor/name/) to activate grouping syntax
    
    diff --git a/src/httpserver.h b/src/httpserver.h
    index cff876abb8..b4c18e7bcb 100644
    --- a/src/httpserver.h
    +++ b/src/httpserver.h
    @@ -132,6 +132,7 @@ private:
     
     struct HTTPVersion {
         /**
    +     * [@name](/github-metadata-backup-bitcoin-bitcoin/contributor/name/) Protocol version
          * Default HTTP protocol version 1.1 is used by error responses
          * when a request is unreadable.
          */
    @@ -199,6 +200,7 @@ public:
     
     private:
         /**
    +     * [@name](/github-metadata-backup-bitcoin-bitcoin/contributor/name/) Request parsers
          * Methods that attempt to parse HTTP request fields line-by-line
          * from a receive buffer.
          * [@returns](/github-metadata-backup-bitcoin-bitcoin/contributor/returns/)     true    If the request field was parsed.
    @@ -408,7 +410,8 @@ private:
          */
         std::thread m_thread_socket_handler;
     
    -    /*
    +    /**
    +     * [@name](/github-metadata-backup-bitcoin-bitcoin/contributor/name/) Request dispatch
          * What to do with HTTP requests once received, validated and parsed.
          * Set in main thread by server start and interrupt but read in
          * worker threads.
    @@ -578,6 +581,7 @@ private:
         std::atomic_bool m_req_busy{false};
     
         /**
    +     * [@name](/github-metadata-backup-bitcoin-bitcoin/contributor/name/) Response data
          * Response data destined for this client.
          * Written to by http worker threads, read and erased by HTTPServer I/O thread
          */
    

    Not entirely happy with how these fields are separated from the others, see screenshot: <img width="1308" height="1380" alt="Image" src="https://github.com/user-attachments/assets/523c6941-dc92-462a-80e7-012fc3173d94" />

    But if you agree I can push that commit on top.


    pinheadmz commented at 7:59 PM on October 28, 2026:

    Too picky for me ;-) I think future devs are more likely to read the .cpp file than the doxygen html anyway so as long as it looks ok in the code, i'm good with it. Happy to re-ack if you decide to change it though.


    hodlinator commented at 7:32 AM on October 29, 2026:

    Since you offered to re-ACK, I pushed the change. People are bound to consume the comments more through code as you say, rather than through this specific doxygen layout, so it feels more complete for it to be correct.

  56. hodlinator force-pushed on Oct 29, 2026
  57. DrahtBot added the label CI failed on Oct 29, 2026
  58. DrahtBot removed the label CI failed on Oct 29, 2026
  59. pinheadmz approved
  60. pinheadmz commented at 1:47 PM on October 29, 2026: member

    ACK 6a5212fb97c2c332fc3cb901f8e9a110622ef841

    Only doxygen changes since last review, thanks for cleaning that all up <3

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

    -----BEGIN PGP SIGNED MESSAGE-----
    Hash: SHA256
    
    ACK 6a5212fb97c2c332fc3cb901f8e9a110622ef841
    -----BEGIN PGP SIGNATURE-----
    
    iQJPBAEBCAA5FiEE5hdzzW4BBA4vG9eM5+KYS2KJyToFAmrjTjQbFIAAAAAABAAO
    bWFudTIsMi41KzEuMTIsMCwzAAoJEOfimEtiick61L0P/RiOzFWc2550oNsQJQUo
    SjqcDQcfztHzPhhUopnoR7xTvC1JdBcMrBANnXJBE9ZIcKL12/sffpCcTZscwK+v
    m4SYWsihpLnIxx1vnyaokJbIdqqWxZsl+fBg0CUpIIh85hOCMRmht7WWb/pJnhWs
    BMsLwhpIoh4WgzU1ctWFqWlLGjfFLeskx2JjVPWSIlur+ThnN3MfcQi99LfKx9qi
    E41sKPwnY1wOmEWMCilhuZy1uwqdvV/qElGGEjn55D2gi4j41uaxnqBS/nsuNaQi
    V5jB/WJ9GiRCLByNqqKSEjlEciVdbR2XEbZ+SYSFw7oaIJQ+953QCR+sGvgAOCrm
    BzLHGG0UF8ZuE9nRhGSBu0gwTV2welFx+hO75FqWZpYbOBv4/l8rjIjvYBECo/df
    7wGepZBDUNgXJDCInBJmfnLin6VutvQL90sMegc7D/vPYNoIt+HjizvKmOTGlnT6
    MKldDakZjILgh4PRoft4OG9qaiq7n6k8n0UuUzn8ZS5Cx97ODzs+Q38ZzSP1F0P3
    ImRxPCfnZFIdIjJ1Amaqt/SYp0sW6iBgTcnPbQyKr83smvuHzPSQwTBVbRi/eozt
    oiZfQJxFzrtXqP1xofNCue0fUVH7iUSMtnxNCVZfayGwnFc66v0V9IGezz24t3mA
    pIS2YXrhHkwlhsp7BeqeBfpp
    =rUPo
    -----END PGP SIGNATURE-----
    

    pinheadmz's public key is on openpgp.org

    </details>

  61. fanquake merged this on Oct 29, 2026
  62. fanquake closed this on Oct 29, 2026

  63. winterrdog commented at 3:08 PM on October 29, 2026: contributor

    Post-merge ACK 6a5212fb97c2c332fc3cb901f8e9a110622ef841


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-10-30 09:40 UTC