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

Reviews

See the guideline for information on the review process.
A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

• #27479 (BIP324: ElligatorSwift integrations by sipa)

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.

fad126d seems like a nice improvement. It makes existing MakeByteSpan function safer by only allowing it to be called on arrays of characters, and not array of other things like uint16_t that could have platform specific representations. Some thoughts though:

• MakeByteSpan probably deserves to have some documentation since it is one of the more useful function in that file and is called a lot of places.
• I don't like introducing the ByteSpanCast function. It doesn't seem useful except as an implementation detail, and I think it'd be nicer if we got rid of some confusing sounding functions instead of adding another one.
• I don't like adding another use of the AsBytePtr function. As mentioned in util: Allow std::byte and char Span serialization #27927 I think AsBytePtr is unsafe, and less clear than reinterpret_cast.

Would suggest tweaking the PR as follows:

--- a/src/span.h
+++ b/src/span.h
@@ -270,25 +270,28 @@ inline const unsigned char* UCharCast(const std::byte* c) { return reinterpret_c
 
 // Helper function to safely convert a Span to a Span<[const] unsigned char>.
 template <typename T> constexpr auto UCharSpanCast(Span<T> s) -> Span<typename std::remove_pointer<decltype(UCharCast(s.data()))>::type> { return {UCharCast(s.data()), s.size()}; }
-// Helper function to safely convert a Span to a Span<[const] std::byte>.
-template <typename B>
-auto ByteSpanCast(Span<B> s)
-{
-    return Span{AsBytePtr(UCharCast(s.data())), s.size_bytes()}; // Use UCharCast to enforce B is a byte-like type.
-}
 
 /** Like the Span constructor, but for (const) unsigned char member types only. Only works for (un)signed char containers. */
 template <typename V> constexpr auto MakeUCharSpan(V&& v) -> decltype(UCharSpanCast(Span{std::forward<V>(v)})) { return UCharSpanCast(Span{std::forward<V>(v)}); }
 
+// Helper functions to turn arrays of characters into std::byte spans. Usable on
+// C++ std::string, std::string_view, and std::vector<char> objects, as well as
+// on C string literals and C fixed-size char[] arrays.
+//
+// Note that when constructing a span directly from a C string literal, the span
+// will include the \0 null byte. This can be avoided by constructing the span
+// indirectly through std::string_view.
 template <typename V>
 Span<const std::byte> MakeByteSpan(V&& v) noexcept
 {
-    return ByteSpanCast(Span{std::forward<V>(v)});
+    Span data{std::forward<V>(v)};
+    return AsBytes(Span{UCharCast(data.data()), data.size_bytes()});
 }
 template <typename V>
 Span<std::byte> MakeWritableByteSpan(V&& v) noexcept
 {
-    return ByteSpanCast(Span{std::forward<V>(v)});
+    Span data{std::forward<V>(v)};
+    return AsWritableBytes(Span{UCharCast(data.data()), data.size_bytes()});
 }
 
 #endif // BITCOIN_SPAN_H

🐙 This pull request conflicts with the target branch and needs rebase.

Maybe MakeByteSpan should just be removed?

• Currently it (as well as Span) accepts a range that is not borrowed (see https://en.cppreference.com/w/cpp/ranges/borrowed_range), leading to potential lifetime issues. Someone should check if making the Span deduction guidelines safer will automatically make the MakeByteSpan function safer.
• Given that std::as_bytes is in the standard library and we can't prevent devs from using it (or AsBytes) at compile time, it seems pointless trying to offer a safe MakeByteSpan wrapper with only a docstring suggesting to use it. It may be better to just write a clang-tidy plugin to review the code to disallow calling AsBytes on a span of uint16_t (or similar)?

Maybe MakeByteSpan should just be removed?

I don't think I agree. MakeByteSpan(x) is just equivalent to AsBytes(Span{x}) currently, so right now it is pointless. But if we go ahead with this PR and restrict it to only work on character arrays, that will make it safer and actually useful.

it seems pointless trying to offer a safe MakeByteSpan wrapper with only a docstring suggesting to use it.

I don't think it's pointless to have a safe function even though a dangerous alternative exists. If I'm writing or reviewing code that uses a safe implementation of MakeByteSpan I can be more confident it is doing the right thing than if I see code using AsBytes. Just like if I'm reviewing code that is using UCharCast I can be more confident in it than code using an (unsigned char) cast.

I think it would be a good thing to discourage use of AsBytes and provide some safer alternative where possible. That alternative doesn't have to be MakeByteSpan, but if we have a MakeByteSpan function already and can document it and make it safer, I think it'd be good to do that.

If I'm writing or reviewing code that uses a safe implementation of MakeByteSpan I can be more confident it is doing the right thing than if I see code using AsBytes.

Would you be less confident if there was a clang-tidy plugin doing the check? For example, instead of having two functions that do a similar thing, but it is unclear which one is safer/preferred from just looking at the name, we could have an additional UnsafeAsBytes (or similar), where it is clear that using it without looking up what it does is unsafe.

If I'm writing or reviewing code that uses a safe implementation of MakeByteSpan I can be more confident it is doing the right thing than if I see code using AsBytes.

Would you be less confident if there was a clang-tidy plugin doing the check? For example, instead of having two functions that do a similar thing, but it is unclear which one is safer/preferred from just looking at the name, we could have an additional UnsafeAsBytes (or similar), where it is clear that using it without looking up what it does is unsafe.

I think that sounds fine, though I'm not sure what the clang plugin adds exactly. If the goal is just to forbid AsBytes from being called on non-char arrays, we could change the definition of AsBytes to do that (probably using UCharCast internally), and provide an additional UnsafeAsBytes escape hatch for the cases where that doesn't work. But maybe a clang plugin could be useful to prevent std::as_bytes calls?.

I just think as long as we have a MakeByteSpan function and can make it safer, it seems good to do that. Or at least that doing that would be better than getting rid of MakeByteSpan and replacing it with calls to AsBytes when AsBytes is not very safe right now. But if there's something that can we can do to make AsBytes safer, I'd agree there be little use for a safer MakeByteSpan

FWIW, I don't believe AsBytes (or std::as_bytes) are ever undefined behavior; every object has a memory representation, which you're allowed to observe. It is of course always implementation-defined behavior when anything but byte and byte-like arrays are involved, but the point of these functions is allowing access to that.

FWIW, I don't believe AsBytes (or std::as_bytes) are ever undefined behavior

Yes that should be true as long as span is pointing to live memory. I should replace "platform specific or undefined" with "implementation specific" in the #27978 change description

Closing for now to allow for more brainstorming