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

nit: this comment seems to be incorrect, since TCP_NODELAY is only set once by SockMan.

This comment may need to be applied upstream in #30988

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

nit: weekday and month can be a string_view.

done

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

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

Great thanks, done.

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

std::optional<std::string> LineReader::ReadLine()
{
    if (it == end) {
        return std::nullopt;
    }

    auto line_start = it;
    size_t count = 0;
    while (it != end) {
        char c = static_cast<char>(*it);
        ++it;
        ++count;
        if (c == '\n') break;
        if (count >= max_read) throw std::runtime_error("max_read exceeded by LineReader");
    }
    const std::byte *data = &*line_start;
    std::string line{reinterpret_cast<const char *>(data), count};
    line = TrimString(line); // delete trailing \r and/or \n
    return line;
}

awesome, done

Can we return a string_view to the internal buffer? (preventing allocation & copy)

by internal buffer I assume you mean m_recv_buffer which im trying to copy from into the HTTPRequest object, then clear the buffer. I left another response about maybe changing the type of that buffer to reduce unnecessary copies, but am going to leave it alone for this current rebase. What I did here though was swap out TrimString() with TrimStringView() so at least that's one less.

Can we return a string_view to the internal buffer? (preventing allocation & copy)

I think I addressed this already by taking your suggestion here https://github.com/bitcoin/bitcoin/pull/32061/files#r2052024440

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

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

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

Sounds good, thanks for the context :)

nit: consider using string_view instead of string here and below.

done.

nit: key & value can also be a string_view (here and below)

Cool will address this nit on next rebase thanks!

Bikeshed, feel free to ignore.

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

Happy to change the names, I just needed to separate the old libevent server from the new one in the PR in the series of commits where each namespace has classes like HTTPRequest

nit: maybe the following would be simpler?

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

thanks I'll take it!

nit: maybe it's better to return a std::span from m_recv_buffer to avoid a copy here.

we'd still want to erase the data after fulfilling the request though (if I'm understanding your comment correctly). Would it make more sense if m_recv_buffer was a std::deque or something where we can truncate the structure without copying/moving/shifting left?

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

Sounds good :)

Would it be possible to allow writing a reply composed of several span<byte>? It should allow sending while serializing a response object.

Could this be done in a followup? I actually just checked #30321 to see why we didn't do this back then -- I remember when that was authored and merged but I didn't realize you were the author!

Sure, thanks :)

Maybe use a list of buffers (instead a single FIFO buffer)?

I don't understand -- would that be an optimization? It's not really a FIFO, since we insert at the end of the buffer here but send data from the begin() of the buffer in SendBytesFromBuffer()

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

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

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

Thinking about it, doing the copy here is definitely fine for now (we can optimize it in a following PR).

nit: can be simplified using

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

awesome thanks

Can be removed if we'll add WriteReply(HTTPStatusCode status, std::string_view reply_body_view) (see above).

👍

nit:

std::string_view RequestMethodString(HTTPRequestMethod m)

done

nit: wouldn't it copy the substrings?

You're right thanks, I haven't been dilligent about using string_view when copying isn't needed. Fixed that here and also in HTTPRequest::LoadControlData()

nit:

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

thanks, for this fix I added a new commit instead of a fixup. The reason is because in the "story" of the commits, GetHeader() is already defined and used by the legacy HTTP server returning a string, and the goal at this point is to match the exiting API. But we can optimize it after the big "switch HTTP servers" commit.

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

I should have mentioned the initial test commits have been split off into #32408 and the test as written here failed CI, so has been modified with rpcservertimeout=2 and then expects a timeout between 1 and 4 seconds

Addressed in #32408

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

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

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

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

suggestion:

-            assert not res
+            assert False

Addressed in #32408

This will also pass if no exception is thrown. Either add assert False after line 214 or have a boolean variable to false before the try and set it to true inside except and assert that it is true afterwards.

Addressed in #32408

Is the intention here to send 8MB of data?

Addressed in #32408

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

assert (value of out1 is not printed):

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

vs

assert_equal():

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

Addressed in #32408

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

BOOST_AUTO_TEST_CASE(test_ParseUInt64Hex)
{
    uint64_t n;
    // Valid values
    BOOST_CHECK(ParseUInt64Hex("1234", nullptr));
    BOOST_CHECK(ParseUInt64Hex("1234", &n));
    BOOST_CHECK_EQUAL(n, 0x1234);
    BOOST_CHECK(ParseUInt64Hex("a", &n));
    BOOST_CHECK_EQUAL(n, 0xA);
    BOOST_CHECK(ParseUInt64Hex("0000000a", &n));
    BOOST_CHECK_EQUAL(n, 0xA);
    BOOST_CHECK(ParseUInt64Hex("100", &n));
    BOOST_CHECK_EQUAL(n, 0x100);
    BOOST_CHECK(ParseUInt64Hex("DEADbeef", &n));
    BOOST_CHECK_EQUAL(n, 0xDEADbeef);
    BOOST_CHECK(ParseUInt64Hex("FfFfFfFf", &n));
    BOOST_CHECK_EQUAL(n, 0xFfFfFfFf);
    BOOST_CHECK(ParseUInt64Hex("123456789", &n));                                      
    BOOST_CHECK_EQUAL(n, 0x123456789ULL);
    BOOST_CHECK(ParseUInt64Hex("0", &n));
    BOOST_CHECK_EQUAL(n, 0);
    BOOST_CHECK(ParseUInt64Hex("FfFfFfFfFfFfFfFf", &n));
    BOOST_CHECK_EQUAL(n, 0xFfFfFfFfFfFfFfFfULL);
    // Invalid values
    BOOST_CHECK(!ParseUInt64Hex("", &n));
    BOOST_CHECK(!ParseUInt64Hex("-1", &n));
    BOOST_CHECK(!ParseUInt64Hex("10 00", &n));
    BOOST_CHECK(!ParseUInt64Hex("1 ", &n));
    BOOST_CHECK(!ParseUInt64Hex("0xAB", &n));
    BOOST_CHECK(!ParseUInt64Hex("FfFfFfFfFfFfFfFf0", &n));
}

taken, thanks

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

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

<details> <summary>[patch] Use ParseIntegral() which uses std::from_chars(), like the other Parse*() functions</summary>

diff --git i/src/util/strencodings.cpp w/src/util/strencodings.cpp
index d923bdd121..05a5dced62 100644
--- i/src/util/strencodings.cpp
+++ w/src/util/strencodings.cpp
@@ -199,21 +199,21 @@ std::optional<std::vector<unsigned char>> DecodeBase32(std::string_view str)
 
     return ret;
 }
 
 namespace {
 template <typename T>
-bool ParseIntegral(std::string_view str, T* out)
+bool ParseIntegral(std::string_view str, T* out, size_t base = 10)
 {
     static_assert(std::is_integral_v<T>);
     // Replicate the exact behavior of strtol/strtoll/strtoul/strtoull when
     // handling leading +/- for backwards compatibility.
     if (str.length() >= 2 && str[0] == '+' && str[1] == '-') {
         return false;
     }
-    const std::optional<T> opt_int = ToIntegral<T>((!str.empty() && str[0] == '+') ? str.substr(1) : str);
+    const std::optional<T> opt_int = ToIntegral<T>((!str.empty() && str[0] == '+') ? str.substr(1) : str, base);
     if (!opt_int) {
         return false;
     }
     if (out != nullptr) {
         *out = *opt_int;
     }
@@ -250,26 +250,13 @@ bool ParseUInt64(std::string_view str, uint64_t* out)
 {
     return ParseIntegral<uint64_t>(str, out);
 }
 
 bool ParseUInt64Hex(std::string_view str, uint64_t* out)
 {
-    if (str.size() > 8) return false;
-    if (str.size() < 1) return false;
-    uint64_t total{0};
-    auto it = str.begin();
-    while (it != str.end()) {
-        auto v = HexDigit(*(it++));
-        if (v < 0) return false;
-        total <<= 4;
-        total |= v;
-    }
-    if (out != nullptr) {
-        *out = total;
-    }
-    return true;
+    return ParseIntegral<uint64_t>(str, out, 16);
 }
 
 std::string FormatParagraph(std::string_view in, size_t width, size_t indent)
 {
     assert(width >= indent);
     std::stringstream out;
diff --git i/src/util/strencodings.h w/src/util/strencodings.h
index b95b21dafd..d83fa3c841 100644
--- i/src/util/strencodings.h
+++ w/src/util/strencodings.h
@@ -173,17 +173,17 @@ constexpr inline bool IsSpace(char c) noexcept {
  * is `-?[0-9]+`. The minus sign is only permitted for signed integer types.
  *
  * [@returns](/bitcoin-bitcoin/contributor/returns/) std::nullopt if the entire string could not be parsed, or if the
  *   parsed value is not in the range representable by the type T.
  */
 template <typename T>
-std::optional<T> ToIntegral(std::string_view str)
+std::optional<T> ToIntegral(std::string_view str, size_t base = 10)
 {
     static_assert(std::is_integral_v<T>);
     T result;
-    const auto [first_nonmatching, error_condition] = std::from_chars(str.data(), str.data() + str.size(), result);
+    const auto [first_nonmatching, error_condition] = std::from_chars(str.data(), str.data() + str.size(), result, base);
     if (first_nonmatching != str.data() + str.size() || error_condition != std::errc{}) {
         return std::nullopt;
     }
     return result;
 }
 

Due to #32520 I had to change this entire commit anyway. Took your suggestion about adding base argument here and just dropped the wrapper function. Will call ToIntegral( ... 16) directly when reading chunk size.

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

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

<details> <summary>[patch] FormatRFC7231DateTime()</summary>

diff --git i/src/test/util_tests.cpp w/src/test/util_tests.cpp
index 387493152d..ebb40dd713 100644
--- i/src/test/util_tests.cpp
+++ w/src/test/util_tests.cpp
@@ -384,15 +384,23 @@ BOOST_AUTO_TEST_CASE(util_FormatISO8601Date)
     BOOST_CHECK_EQUAL(FormatISO8601Date(0), "1970-01-01");
     BOOST_CHECK_EQUAL(FormatISO8601Date(1317425777), "2011-09-30");
 }
 
 BOOST_AUTO_TEST_CASE(util_FormatRFC7231DateTime)
 {
+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(std::numeric_limits<int64_t>::max()), "");
+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(253402300800), "");
+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(253402300799), "Fri, 31 Dec 9999 23:59:59 GMT");
     BOOST_CHECK_EQUAL(FormatRFC7231DateTime(253402214400), "Fri, 31 Dec 9999 00:00:00 GMT");
     BOOST_CHECK_EQUAL(FormatRFC7231DateTime(1717429609), "Mon, 03 Jun 2024 15:46:49 GMT");
     BOOST_CHECK_EQUAL(FormatRFC7231DateTime(0), "Thu, 01 Jan 1970 00:00:00 GMT");
+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(-1), "Wed, 31 Dec 1969 23:59:59 GMT");
+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(-1717429609), "Sat, 31 Jul 1915 08:13:11 GMT");
+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(-62167219200), "Sat, 01 Jan 0000 00:00:00 GMT");
+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(-62167219201), "");
+    BOOST_CHECK_EQUAL(FormatRFC7231DateTime(std::numeric_limits<int64_t>::min()), "");
 }
 
 BOOST_AUTO_TEST_CASE(util_FormatMoney)
 {
     BOOST_CHECK_EQUAL(FormatMoney(0), "0.00");
     BOOST_CHECK_EQUAL(FormatMoney((COIN/10000)*123456789), "12345.6789");
diff --git i/src/util/time.cpp w/src/util/time.cpp
index 9b9167d19a..c0f375956a 100644
--- i/src/util/time.cpp
+++ w/src/util/time.cpp
@@ -7,21 +7,22 @@
 
 #include <compat/compat.h>
 #include <tinyformat.h>
 #include <util/check.h>
 #include <util/strencodings.h>
 
+#include <array>
 #include <atomic>
 #include <chrono>
 #include <optional>
 #include <string>
 #include <string_view>
 #include <thread>
 
-static const std::string weekdays[7] = {"Sun", "Mon", "Tue", "Wed", "Thu", "Fri", "Sat"};
-static const std::string months[12] = {"Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"};
+static constexpr std::array<std::string_view, 7> weekdays{"Thu", "Fri", "Sat", "Sun", "Mon", "Tue", "Wed"}; // 1970-01-01 was a Thursday.
+static constexpr std::array<std::string_view, 12> months{"Jan", "Feb", "Mar", "Apr", "May", "Jun", "Jul", "Aug", "Sep", "Oct", "Nov", "Dec"};
 
 void UninterruptibleSleep(const std::chrono::microseconds& n) { std::this_thread::sleep_for(n); }
 
 static std::atomic<std::chrono::seconds> g_mock_time{}; //!< For testing
 std::atomic<bool> g_used_system_time{false};
 static std::atomic<std::chrono::milliseconds> g_mock_steady_time{}; //!< For testing
@@ -116,20 +117,24 @@ std::optional<int64_t> ParseISO8601DateTime(std::string_view str)
     }
     const auto time{std::chrono::hours{*hour} + std::chrono::minutes{*min} + std::chrono::seconds{*sec}};
     const auto tp{std::chrono::sys_days{ymd} + time};
     return int64_t{TicksSinceEpoch<std::chrono::seconds>(tp)};
 }
 
-std::string FormatRFC7231DateTime(int64_t nTime)
+std::string FormatRFC7231DateTime(int64_t time)
 {
-    const std::chrono::sys_seconds secs{std::chrono::seconds{nTime}};
+    if (time < -62167219200 || 253402300799 < time) {
+        // RFC7231 mandates 4-digit year, so only support years 0 to 9999
+        return "";
+    }
+    const std::chrono::sys_seconds secs{std::chrono::seconds{time}};
     const auto days{std::chrono::floor<std::chrono::days>(secs)};
-    // 1970-01-01 was a Thursday
-    std::string_view weekday{weekdays[(days.time_since_epoch().count() + 4) % 7]};
+    const auto w{days.time_since_epoch().count() % 7}; // will be in the range [-6, 6]
+    std::string_view weekday{weekdays.at(w >= 0 ? w : w + 7)};
     const std::chrono::year_month_day ymd{days};
-    std::string_view month{months[unsigned{ymd.month()} - 1]};
+    std::string_view month{months.at(unsigned{ymd.month()} - 1)};
     const std::chrono::hh_mm_ss hms{secs - days};
     // examples: Mon, 27 Jul 2009 12:28:53 GMT
     //           Fri, 31 May 2024 19:18:04 GMT
     return strprintf("%03s, %02u %03s %04i %02i:%02i:%02i GMT", weekday, unsigned{ymd.day()}, month, signed{ymd.year()}, hms.hours().count(), hms.minutes().count(), hms.seconds().count());
 }
 

</details>

ah really really great suggestion thanks, taken

e95c6f5b6511ae35141b1e440e1f22e1004d3de6

Copying strings is expensive:

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

👍

void HTTPHeaders::Write(const std::string& key, const std::string& value)

:+1:

void HTTPHeaders::Remove(const std::string& key)

:+1:

Would be good to document when Read() throws, returns true and false.

sure will add

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

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

I'm asking if both should be moved, including the generic HTTP constants. They are also used in rest.cpp which has to #include <rpc/protocol.h> even though REST is not RPC protocol. I'll remove the comment for now.

Maybe enum is better for this?

yeah good call, I will add that in to "define HTTP request methods at module level outside of class" (currently 7a1359064e326f2b28a1c57aaa94fdbe17f9dee2) which is a few commits ahead

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

Content-Length = 1*DIGIT

Hm yeah I see your point, we don't validate individual headers in LoadHeaders(). I'll leave as is for now, for what it's worth, libevent doesn't appear to check this until evhttp_get_body_length() as well...

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

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

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

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

        Mutex requests_mutex;
        std::deque<std::unique_ptr<HTTPRequest>> requests GUARDED_BY(requests_mutex);

Compiler didn't like this actually:

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

Passing unique_ptr by value looks a bit strange since it is not supposed to be copied. I guess this works when the caller std::moves the object, but isn't it more clear to use std::unique_ptr<HTTPRequest>&& req?

Agreed this better, thanks.

Would be good to have tests to exercise the chunked transfer encoding from d7778f426c http: support "chunked" Transfer-Encoding.

That was merged in #32408 unless you mean a unit test?

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

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

yes thanks

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

- bool HTTPServer::CloseConnection(std::shared_ptr<HTTPClient> http_client)
+ bool HTTPServer::CloseConnection(const HTTPClient& http_client)
  {
      size_t erased = std::erase_if(m_connected,
-                                   [&](auto& client){ return client == http_client; });
+                                   [&](auto& client){ return client.get() == &http_client; });

Would be good to note that this overrides m_disconnect:

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

:+1:

nit:

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

👍

nit:

    if (!InitHTTPAllowList()) {
        return false;
    }

👍

    for (std::vector<std::pair<std::string, uint16_t>>::iterator i = endpoints.begin(); i != endpoints.end(); ++i) {

👍

LogPrintf() is using the Info severity.

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

good catch thanks

nit:

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

:+1:

Could a new client sneak in after the loop while (!g_http_server->m_no_clients) exits? Shouldn't that loop be after interruptNet() so that new clients cannot be accepted?

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

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

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

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

(in commit 2950597694 use CScheduler for HTTPRPCTimer)

Can I though? Parent class RPCTimerInterface uses int64_t millis and so does our sibling class QtRPCTimerBase because of its QTimer.

Because the context will always be set, ie will never be nullptr, then it is better to use a reference here instead of a pointer: NodeContext& m_context;

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

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

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

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

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

Can simplify the code a few lines below:

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

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

nice catch thanks

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

(from commit e95c6f5b65 http: Implement HTTPHeaders class)

This is great feedback thanks. I made this change by first editing the "string: add CaseInsensitiveComparator" commit which is now "string: add CaseInsensitive{KeyEqual, Hash} for unordered map" and implements the two operators needed for the unordered map.

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

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

Ah that is another great catch thank you. Glad I wrote that as an extra commit! I'll pop it off, it was added in response to this review comment: #32061 (review)

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

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

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

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

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

nit: I think that can be a bit shorter:

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

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

/home/runner/work/_temp/src/util/strencodings.h:381:23: error: comparison is always false due to limited range of data type [-Werror=type-limits]
  381 |                 if (c < 0) return c;
      |                     ~~^~~
cc1plus: all warnings being treated as errors

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

         BOOST_REQUIRE(headers_string.ends_with("\r\n\r\n"));
+        BOOST_CHECK_EQUAL(headers_string.length(), 67);
     }

Similar to above:

                    actual.find("Date: Wed, 11 Dec 2024 00:47:09 GMT\r\n") != std::string::npos &&
                    actual.length() == 146

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

    HTTPRequestMethod GetRequestMethod() const { return m_method; }

nit: I think I wouldn't call the commit move-only because of this.

👍 re-worded the commit message

The commit message talks about reinterpret_cast but the code uses static_cast?

Ah thanks, fixed this commit message

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

- auto sa = static_cast<sockaddr*>(static_cast<void*>(&storage));
+ auto sa = reinterpret_cast<sockaddr*>(&storage);

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

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

sure, taken. every little bit of legibility helps ;-)

nit: If we are only plan to use this with base 10 or 16 maybe it would be safer to make this a bool. But I don't feel too strongly about it.

I think I will leave as-is, a bool would require explanation and this way we have explicit argument name and default value.

nit: I would call the classe AsciiCaseInsensitiveHash

Ok taken. I was doubtful at first but learned about "case folding" in unicode so, the qualification is justified.

In the bd924ecf4e29b3ff918def6cb55b959fe19f33dc commit message: Not sure why this talks about libevent comparing in lowercase. This is just following HTTP requirements, right? So I found it a bit confusing and checked if this meant that this behavior was just temporary and maybe removed later together with libevent, but it isn't...

The important bit is the case-insensitivity of the HTTP protocol. I reference libevent functions in comments and commit messages throughout the PR because I referenced them when writing and also for reviewers to compare. I'll reword this commit message to better explain.

nit: This seems to be superseeded by RFC9110: https://www.rfc-editor.org/rfc/rfc9110.html#name-date-time-formats

Replaced the reference to HTTP spec with reference to the actual date/time spec implemented in the function.

Not sure if we should have the RFC in the name if it's ambiguous where the format is specified (see above). Maybe FormatHTTPDate or something like that would be better.

Great catch, I had to dig through the forest of RFCs again, and rewrote the commit message and function name. I'm going with RFC1123 because that is the RFC referenced by libevent. I also wanted to keep the pattern of naming similar to neighbor functions like FormatISO8601DateTime()

Shouldn't this be rather count > max_read? At least that would be what I would expect based on the error here and the docs in the .h: "Will not search for \n past max_read.". It's currently more like "up until but not including max_read", right?

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

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

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

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

I will add some nits at the appropriate lines.

EOL is at position equal to max_read

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

EOL is +1 past max_read

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

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

while (it != end) {
        if (count >= max_read) throw std::runtime_error("max_read exceeded by LineReader");
        ++count;

        auto c = static_cast<char>(*it);
        if (c == '\n') break;

        ++it;
}

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

max_read = 8

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

It sounds a bit like this might just return: "Will throw if a line length exceeds max_read."

Good point, updated

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

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

Good catch thanks, added the check and a test.

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

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

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

This is a really good catch and is big bug in what I had. To match spec and libevent behavior I had to replace the unordered_map with vector <pair of strings> to allow duplicate headers. Libevent's getters are more like "find first and return early" so I'll do that as well. In that sense we will match the looser protocol enforcement. This also means that since I'm not using a map anymore, I don't need the comparator/hasher added in #34242 so I'll revert that with a new utility commit in this branch. I'm just gonna move the CaseInsensitiveEqual() function from test utilities to src/.

nit: There seems to be one too many quotes

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

thanks, fixed

nit: could be constexpr I think, also below

Thanks took this, couldn't keep strlen for compile time but C++17 allows string_view::size(). 5 minutes of research suggest that strlen should be allwed by C++20 but its not on AppleClang? Anyway...

Maybe also add a negative value test case for version and content-length.

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

	if (n != 2 || major > '1' || major < '0' || minor > '9' || minor < '0') {
		event_debug(("%s: bad version %s on message %p from %s",

Hm, kind of surprised that htis works because it seems this is adding the unique ptr to a vector of shared ptr.

Good catch, this is the same move we make in net.cpp::BindListenPort() as well. My understanding from https://en.cppreference.com/w/cpp/memory/shared_ptr/shared_ptr.html (and also GPT) is that what we are actually doing here is constructing a new shared_ptr with the current unique_ptr as an argument. Ownership is transferred and the unique_ptr is nullified

The \n isn't needed anymore with all the new logging as far as I know, here and in other places.

Thanks, fixed.

I guess this means it's not const?

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

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

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

Should this loop get an interrupt/timeout to not run forever?

Really good catch and actually as written, this sustains a bug in the libevent implementation on master, documented in #31929 (h/t @hodlinator). That PR adds a maximum 30 second timeout to WaitUntilEmpty() which is analogous to this line here. I'll hard-code the 30s as well unless someone thinks it needs to be configurable. I'll try to write a test that covers this as well, something that intentionally and reliably hangs on master during shutdown.

Possible typos and grammar issues:

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

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

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

2026-02-12 16:33:30

Thanks 👍 adding to next rebase.

naming nit: used to be a map, but is not anymore, maybe rename from m_map to e.g. m_headers.

changed all m_map to m_headers

in b53198cbf5e1e1974ae1963effeac9a12c20c781 "http: Implement HTTPHeaders class": #include <map> is still being added.

Could also satisfy the condition if detach() has been called on the thread. In that case the thread is still running, but this Assume() will not be triggered, when it should. I think it is fine, just mentioning.

That's true, and without a wrapper to disable detach() a future change to the code could trigger that (by not triggering it...) Although if there are still connected clients, the Assume on the next line will throw. So, careless future developer should at least still see that.

Drop these comments, ShouldTryToSend() does not exist (here).

yes! good catch thanks, RIP "sockman lite"

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

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

Good improvement thanks, using a reference instead.

EventIOLoopCompletedForOne()

removed irrelevant comment here

There is IOErrorIsPermanent() in sock.cpp

Great, I moved that utility function to the header file and removed static in this commit so we can use it here. This is a slight functional change, the check for WSAEMSGSIZE is not present in the IOErrorIsPermanent() but as far as I can tell, that error is uncommon on recv() and especially uncommon on TCP.

nit: raw strings are cool

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

oh wow those are cool! Thanks. applied here and throughout the http tests

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

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

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

Add support for chunk-extensions:

             if (!maybe_chunk_size) return false;

-            const auto chunk_size{ToIntegral<uint64_t>(maybe_chunk_size.value(), /*base=*/16)};
+            std::string_view chunk_size_noext{maybe_chunk_size.value()};
+            const auto semicolon_pos = chunk_size_noext.find(';');
+            if (semicolon_pos != chunk_size_noext.npos) {
+                chunk_size_noext.remove_suffix(chunk_size_noext.size() - semicolon_pos);
+            }
+
+            const auto chunk_size{ToIntegral<uint64_t>(chunk_size_noext, /*base=*/16)};
             if (!chunk_size) throw std::runtime_error("Cannot parse chunk length value");

Sure, added this along with a comment linking to RFC9112. Added coverage to unit tests. We don't support chunk extensions (I had a hard time finding real protocols that do) but the semicolon is valid, so we accept and ignore.

I reread the spec again. Looks like some white space is allowed and the server is expected to ignore it.

https://www.rfc-editor.org/rfc/rfc9112.html#name-chunk-extensions https://www.rfc-editor.org/rfc/rfc9110.html#name-whitespace

for example: 0f ; whatever = 123 would be valid. The current code already ignores everything after the ;, but the whitespace before it would end up as a trailing to the hex digit which would fail the parsing by ToIntegral(), 0f in the example.