GetSockName() currently forwards the raw getsockname() return value (0 on success), which forces callers to use an inverted check (!GetSockName(...)).

Can we normalize it so it returns true on success?

bool Sock::GetSockName(sockaddr* name, socklen_t* name_len) const
{
    return getsockname(m_socket, name, name_len) == 0;
}

It would make the code slightly more readable and maintainable.

I agree with you, but this might be better addressed in a separate PR. The entire Sock class wraps POSIX socket management syscalls that return 0 on success (connect, bind, listen, close, etc...)

The idea behind the Sock methods is that they are mockable versions of the OS functions. Zero learning curve for newcomers (assuming familiarity with the OS interface). Sock::Foo(x, y, z) is equivalent to calling the OS's foo(x, y, z).

inverted check

the function returns an integer error code, not a boolean. A check like if (foo() == 0) looks better (less "inverted").

I see. Thanks @vasild. It would be nice to have more documentation on the return codes then. And +1 on the == 0 suggestion.

This line gets "modernized" in the next commit, including a comment and == 0, so I think it's cool for now.

"returns an integer error code" -- oh, I meant this:

https://www.man7.org/linux/man-pages/man2/getsockname.2.html#RETURN_VALUE

RETURN VALUE
On success, zero is returned. On error, -1 is returned, and errno
is set to indicate the error.

or the check could be if (foo() != -1).

The inner cast is unnecessary?

This works with a reinterpret_cast because sockaddr_storage and sockaddr are technically unrelated types even though they overlap... I'll clean it up, thanks. I think I had static_cast there before because of hints from Sonarcloud on corecheck.dev.

taken, thanks

nit: I think C++20 has a short version of this (no extra arrays needed):

std::string FormatRFC1123DateTime(int64_t time)
{
    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}};
    // Example: "Mon, 27 Jul 2009 12:28:53 GMT"
    return std::format("{:%a, %d %b %Y %H:%M:%S} GMT", secs);
}

maybe you already tried it and it doesn't work on all our supported platforms?

This should be locale-independent. I think it is:

https://en.cppreference.com/w/cpp/chrono/system_clock/formatter.html#Format_specification

the default "C" locale if L is not present in the format specification

https://github.com/bitcoin/bitcoin/actions/runs/20963499370/job/60247105398?pr=34242#step:11:1480

/home/admin/actions-runner/_work/_temp/src/util/time.cpp:129:17: error: no member named 'format' in namespace 'std'
  129 |     return std::format("{:%a, %d %b %Y %H:%M:%S} GMT", secs);
      |            ~~~~~^

😢

Indeed, will revert on next push.

too good to be true

In ea993ff:

Seems you can remove this last line. You already do a while (it != end)

Youre right thanks, its gone.

Note that it is incremented inside the body of the loop, after the check in while (it != end). So after the increment it could be equal to end.

    const std::vector<std::byte> input{StringToBuffer("ab")};
    LineReader reader(input, /*max_read=*/1);
    std::optional<std::string> line{reader.ReadLine()};

Before this change the above snippet would have set line to ab and after the change it throws std::runtime_error: max_read exceeded by LineReader. I guess the new behavior is the correct? Maybe worth adding such a test.

Thanks @vasild I added this as a test case, and am scratching my head how it slipped through. Removing the line is not only cleaner code, it is a bug fix. LineReader should not return a line longer than max_read

tiny nit:
could make the termination character customizable.

Going to leave this for now, next dev who needs a custom EOL character can add it in the future ;-)

In 1d24816:

Tiny, non-blocking, idea to see if it makes sense to simplify the LineReader class.
(you can reinterpret std::byte to char in order to make it work with std::istringstream)

// Check three lines terminated by \n, \r\n, and end of buffer, trimming whitespace
const std::string input = "once upon a time\n there was a dog \r\nwho liked food";
std::istringstream stream(input);

std::string line1, line2, line3, line4;
std::getline(stream, line1);
std::getline(stream, line2);
std::getline(stream, line3);
BOOST_CHECK(!std::getline(stream, line4));

BOOST_CHECK_EQUAL(line1, "once upon a time");
BOOST_CHECK_EQUAL(line2, "there was a dog \r"); // TODO: trim whitespaces at the end.
BOOST_CHECK_EQUAL(line3, "who liked food");