Update string and net utils for future HTTP operations

pinheadmz commented at 6:32 pm on March 23, 2026: member

This is a follow-up to #34242 and is the first few commits of #32061

As review and refinement of the replacement HTTP server progresses, some new utilities were needed and added. This PR updates those utilities as work continues on #32061.

LineReader

In order to enforce strict limits on the total size of headers in HTTPRequest, we add a method to LineReader to give us the total amount of data that has been read from the buffer so far. See #32061 (review)

CaseInsensitiveEqual

HTTP headers are case-insensitive. An early version of #32061 used an unordered_map for this and therefore we needed a comparator struct. However that unordered_map was replaced by a simpler std::vector of std::pair so we can remove the struct and use methods that already exist in the codebase.

StringToBytes

StringToBuffer was introduced in #34242 to test LineReader but review of #32061 indicated that it would be more optimal to return a span of bytes instead of a vector. See #32061 (review)

Split DynSock constructor for two usecases: listening / accepting sockets

See #32061 (review). DynSock was introduced in #30988 and is not used anywhere in master yet. If it’s used as a listening socket, it provides connected sockets. If it’s used as a connected socket, it provides I/O pipes. By making the queue of connected sockets optional we can clean up the ownership / lifetime if the class members.

util: get number of bytes consumed from buffer by LineReader 9a6d5a4243

string: replace AsciiCaseInsensitiveKeyEqual with CaseInsensitiveEqual

This reverts commit eea38787b9be99c3f192cb83fc18358397e4ab52 from PR #34242

We do not need comparators for HTTPHeaders since it is not using unordered_map anymore.
We only need a simple, locale-independent, ascii-only compare function for
a vector of key-value pairs of strings.

We have CaseInsensitiveEqual already in test utils, this commit moves
it to the strencodings module for use in the application code.

8e82b6c43f

test: util: implement and use StringToBytes instead of StringToBuffer

Co-Authored by: Hodlinator <172445034+hodlinator@users.noreply.github.com>

c65b981302

Make DynSock accepted sockets queue optional, with precise lifetime

When DynSock is used to represent a connected socket (e.g. a client)
the data I/O pipes are needed but not the m_accepted_sockets Queue,
because connected sockets do not create more connected sockets.

When DynSock is used to represent a listening socket, the Queue
is necessary to create connected sockets upon mocked connection, but
the Queue does not need to be a std::shared_ptr as long as it
is guaranteed to live as long as the DynSock.

Co-Authored by: Hodlinator <172445034+hodlinator@users.noreply.github.com>

1da6da210c

DrahtBot commented at 6:32 pm on March 23, 2026: contributor

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	vasild
Concept ACK	fjahr

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

in src/test/util/str.cpp:11 in c65b981302 outdated

 6+#include <string>
 7+
 8 #include <test/util/str.h>
 9 
10+/** Returns a span view of the string. */
11+std::span<const std::byte> StringToBytes(std::string_view str LIFETIMEBOUND)

fjahr commented at 8:53 pm on March 24, 2026:

on c65b98130299d08361ca9ac669c8f3e210b410b6:

Not sure about StringToBytes. Why can’t you use MakeByteSpan from span.h? If StringToBytes is needed, why is it only used in the tests and not more widely because we have this std::as_bytes(std::span{str} pattern all over the code base. Personally, I am not sure we even need a funtion for this, it seems fine to just inline this and that seems to be the case all over the code base as well (though maybe it was written before MakeByteSpan). I think I would prefer either option over having another helper that duplicates code that exists elsewhere.

If you want to keep it, it would be good to add a stronger rationale to the commit message :)

vasild commented at 8:43 am on March 25, 2026:

No strong opinion, I would slightly prefer to use std directly:

0std::as_bytes(std::span{s});

instead of

0StringToBytes(s);

pinheadmz commented at 5:01 pm on March 27, 2026:

Question about this: the utility is used with hard-coded string literals in the unit tests. Apparently (TIL) this enforces c-string and includes a null terminator. In other words:

0        std::span<const std::byte> input{std::as_bytes(std::span{"1234"})};
1        LineReader reader(input, /*max_line_length=*/128);
2        BOOST_CHECK_EQUAL(reader.Consumed(), 0);
3        BOOST_CHECK_EQUAL(reader.Remaining(), 4); // FAILS because there are 5 bytes

That’s fixed by forcing string_view:

0        std::span<const std::byte> input{std::as_bytes(std::span{std::string_view("1234")})};

And that’s what we get from StringToBytes that we don’t get from MakeByteSpan.

So, options?

Keep as-is
Write the explicit bytes(span(view as above and delete the now-empty test/util/str.cpp
Add something to span.h like an overload for MakeByteSpan() or new MakeByteSpanFromView() ?

cc: @hodlinator

hodlinator commented at 9:27 pm on March 27, 2026:

It would be great if we could just use MakeByteSpan("c-string"), but as you uncovered, it would include the hidden null terminator. (Makes me itch for there to be a constraint on MakeByteSpan() to not accept string literals, char[N]). I lean towards something like option 3, putting it in span.h, but I prefer the name StringToBytes() over MakeByteSpanFromView().

Tangent¹.

This got me looking at the string_view constructors. It appears that string_view{"c-string"} has linear complexity while "c-string"sv has constant complexity since the literal function receives the size (https://en.cppreference.com/w/cpp/string/basic_string_view/operator%2522%2522sv.html). But not sure the complexity makes any difference at all if construction can happen in a constexpr context with the string_view later being copied at runtime, not sure whether compilers are clever enough for that. ↩︎

hodlinator commented at 9:30 pm on March 27, 2026:

Maybe src/util/string.h would be a better home, if others agree on StringToBytes() being the least bad name.

in src/test/util/str.cpp:7 in 8e82b6c43f outdated

3@@ -4,18 +4,3 @@
4 
5 #include <test/util/str.h>
6 
7-#include <cstdint>

fjahr commented at 9:04 pm on March 24, 2026:

Note to other reviewers: Yes this file is empty now. But something else is added here in the next commit. :)

in src/test/util_string_tests.cpp:11 in 8e82b6c43f outdated

 3@@ -4,11 +4,11 @@
 4 
 5 #include <util/strencodings.h>
 6 #include <util/string.h>
 7-#include <vector>
 8 
 9 #include <boost/test/unit_test.hpp>
10 #include <test/util/common.h>
11 #include <tinyformat.h>
12+#include <test/util/str.h>

fjahr commented at 9:08 pm on March 24, 2026:

nit: ordering

in src/test/util/str.cpp:5 in c65b981302 outdated

1@@ -2,5 +2,13 @@
2 // Distributed under the MIT software license, see the accompanying
3 // file COPYING or http://www.opensource.org/licenses/mit-license.php.
4 
5+#include <cstdint>

fjahr commented at 9:13 pm on March 24, 2026:

I don’t think cstdint is needed. Maybe this was supposed to be cstddef?

fjahr commented at 9:19 pm on March 24, 2026:

By IWYU would need to add #include <attributes.h>, #include <string_view> and #include <span> als well I think.

in src/test/util/str.h:9 in c65b981302 outdated

5@@ -6,6 +6,10 @@
6 #define BITCOIN_TEST_UTIL_STR_H
7 
8 #include <string>
9+#include <vector>

fjahr commented at 9:32 pm on March 24, 2026:

vector is unused here. But could add <cstddef> for std::byte here as well.

Also s/span.h/span/.

in src/test/util/net.cpp:377 in 1da6da210c

373@@ -369,6 +374,7 @@ ssize_t DynSock::Send(const void* buf, size_t len, int) const
374 
375 std::unique_ptr<Sock> DynSock::Accept(sockaddr* addr, socklen_t* addr_len) const
376 {
377+    assert(m_accept_sockets && "Accept() called on non-listening DynSock");

fjahr commented at 9:44 pm on March 24, 2026:

nit: We don’t usually use this pattern, I think Assert() may be more of a common choice here.

vasild commented at 8:48 am on March 25, 2026:

Do you mean to s/assert/Assert/ and keep the && "error string" pattern? Or to change to Assert() and remove && "error string"?

 0$ git grep ' [aA]ssert.*&&.*"'
 1src/policy/fees/block_policy_estimator.cpp:184:    assert(_scale != 0 && "_scale must be non-zero");
 2src/test/fuzz/threadpool.cpp:37:        assert(false && "Unexpected exception type");
 3src/test/fuzz/util/net.cpp:140:    assert(false && "Move of Sock into FuzzedSock not allowed.");
 4src/test/pcp_tests.cpp:101:        assert(false && "Move of Sock into PCPTestSock not allowed.");
 5src/test/util/net.cpp:239:    assert(false && "Move of Sock into ZeroSock not allowed.");
 6src/test/util/net.cpp:260:    assert(false && "Move of Sock into StaticContentsSock not allowed.");
 7src/test/util/net.cpp:425:    assert(false && "Move of Sock into DynSock not allowed.");
 8src/univalue/test/object.cpp:21:            assert(0 && "No exception caught"); \
 9src/univalue/test/object.cpp:24:            assert(0 && "Wrong exception caught"); \
10src/util/subprocess.h:596:    assert (typ == PIPE && "STDOUT/STDERR not allowed");
11src/util/subprocess.h:629:    assert (typ == PIPE && "STDOUT/STDERR not allowed");
12src/util/subprocess.h:660:    assert ((typ == PIPE || typ == STDOUT) && "STDERR not allowed");
13src/wallet/wallet.cpp:765:        assert(copyFrom && "Oldest wallet transaction in range assumed to have been found.");

the pattern && "error string" is indeed not widely used, but that alone is not a reason to avoid it. It is a convenient way to print a custom message if the assertion fails. Is there a particular reason this might be good to avoid?

fjahr commented at 11:32 am on March 25, 2026:

Do you mean to s/assert/Assert/ and keep the && “error string” pattern? Or to change to Assert() and remove && “error string”?

I meant to remove the string as well. Our usual pattern is to put such information into a comment next to the Assert() if necessary.

the pattern && “error string” is indeed not widely used, but that alone is not a reason to avoid it. It is a convenient way to print a custom message if the assertion fails. Is there a particular reason this might be good to avoid?

On Assert() vs assert(): The docs don’t make it clear enough, but here is a comment that makes it clearer: #20255 (comment) In this particular case it doesn’t make a practical difference I think (hence it’s a nit) but of course there is some value in consistency across the code base. On the && pattern: I think it’s not favored because it’s a hack and the string needs to be maintained whereas Assert() should print the necessary context to start debugging automatically. At least that is my understanding.

cc @maflcko , maybe you can make a stronger case here or simply clarify if I got something wrong

fjahr commented at 9:48 pm on March 24, 2026: contributor

Concept ACK

vasild approved

vasild commented at 8:50 am on March 25, 2026: contributor

ACK 1da6da210c0411bf9f78d0b8a9ce08376bdb769b

Happy to re-review if @fjahr’s comments are addressed.

DrahtBot requested review from fjahr on Mar 25, 2026

Update string and net utils for future HTTP operations #34905

LineReader

CaseInsensitiveEqual

StringToBytes

Split DynSock constructor for two usecases: listening / accepting sockets

Reviews