util: Allow std::byte and char Span serialization #27927

Seems odd to require developers to cast all byte-like spans passed to serialization to unsigned char-spans. Fix that by passing and accepting byte-like spans as-is. Finally, add tests and update the code to use just Span where possible.

<!--e57a25ab6845829454e8d69fc972939a-->

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

<!--021abf342d371248e50ceaed478a90ca-->

Reviews

See the guideline for information on the review process.

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

<!--174a7506f384e20aa4161008e828411d-->

Conflicts

No conflicts as of last run.

Concept ACK

Concept ACK. It makes sense to allow Span<std::byte> to serialize fixed size blobs, like Span<unsigned char> already does so there is no need for serialize callers to cast to unsigned char if they are already using std::byte.

I also think it is nice that the implementation uses a Span<T> parameter type instead of Span<std::byte> or Span<unsigned char> so you can tell just by looking at the Serialize declaration that an actual Span argument needs to be passed, not some argument that can be implicitly converted to span. It is true as you pointed out in your earlier comment #27790 (review) that the presence of the Serialize(Stream& os, const T& a) overload below already prevents implicit conversions, but it's nice if you can look directly at a declaration and see that it doesn't allow implicit conversions, without having to look at other overloads.

Looking at surrounding code, I also think there are some followups that could be done to make it cleaner / safer / nicer (but would probably save these for a followup PR):

1. I think it's not ideal that ByteSpanCast is using UCharCast cast internally. Now code is using std::byte, I think would be good to drop the UCharCast function and replace it with a similar function that accepts char, unsigned char and std::byte pointers, but returns a std::byte pointer rather than an unsigned char pointer. Doing this would remove an unnecessary series of casts from byte to char and back to byte again in ByteSpanCast, and also just generally move code further in the direction of using std::byte over char
2. I think it might be good to refuse the serialize Span<char> and Span<unsigned char> arguments, and only serialize Span<std::byte> arguments as fixed size blobs. Again it would push code more in direction of using std::byte over char for binary data. It could also perhaps avoid cases where someone is trying to serialize a variable sized string and ends up through some misunderstood conversions accidentally serializing a fixed size span.
3. I think it would be good to drop the AsBytePtr function, which looks like a safe conversion but actually can cast any pointer to a byte pointer, is not actually safe or useful to call on most object types. I think if suggestion 1 above is implemented, some uses of AsBytePtr would no longer be necessary, and the other uses would be clearer if they just used reinterpret_cast directly instead of AsBytePtr

I think it's not ideal that ByteSpanCast is using UCharCast cast internally. Now code is using std::byte, I think would be good to drop the UCharCast function and replace it with a similar function that accepts char, unsigned char and std::byte pointers, but returns a std::byte pointer rather than an unsigned char pointer. Doing this would remove an unnecessary series of casts from byte to char and back to byte again in ByteSpanCast, and also just generally move code further in the direction of using std::byte over char

I thought about this, but concluded it isn't worth it. UCharCast won't be going away, because it is needed to speak with libbitcoinconsensus or libsecp256k1, see for example the conflicting pull. So adding the same 8 function to only differ in return type seems bloaty and verbose. However, I agree with you that new code shouldn't use UCharCast unless necessary.

I think it might be good to refuse the serialize Span<char> and Span<unsigned char> arguments, and only serialize Span<std::byte> arguments as fixed size blobs. Again it would push code more in direction of using std::byte over char for binary data. It could also perhaps avoid cases where someone is trying to serialize a variable sized string and ends up through some misunderstood conversions accidentally serializing a fixed size span.

This may be a bit too much hand-holding, with a small benefit. Also, it would make code more bloaty, as there are quite a few cases in the current code base where a std::string(_view) is serialized via Span<char>, or a view on unsigned char is serialized via Span<unsigned char>.

Also, Span<B> could be used internally to serialize byte-holding (pre)vectors. In fact this is likely a performance bug-fix, because currently, serialization accepts std::vector<B> for B=signed char, or B=int8_t. (B=char is deleted). However, serialization seems to only apply the optimization to only call write once for B=unsigned char. See https://github.com/bitcoin/bitcoin/blob/f1b4975461364d5d40d2bfafc6b165dd5d7eec30/src/serialize.h#L651 It may be good to treat all byte-like types the same, which can be achieved with Span<B>, or alternatively also delete the other B-types that are not special-cased, or alternatively delete the special-case code to treat all vectors the same.

I think it would be good to drop the AsBytePtr function, which looks like a safe conversion but actually can cast any pointer to a byte pointer, is not actually safe or useful to call on most object types. I think if suggestion 1 above is implemented, some uses of AsBytePtr would no longer be necessary, and the other uses would be clearer if they just used reinterpret_cast directly instead of AsBytePtr

I don't think this is possible either. It is required to speak with libsqlite3, libbdb, and internally in serialize.h. I think we should instead think about nuking the AsBytes span helpers. While they are ported from the standard library, I don't think they are appropriate for us, because they are likely architecture dependent when it comes to endian-ness.

Rebased on #27973 for now, please review that first.

Did a rebase on clean master to drop the unneeded dependency.

Code review ACK fa38d862358b87219b12bf31236c52f28d9fc5d6. This looks great. The second commit really removes a lot of boilerplate and shows why the first commit is useful.

Fix that by introducing a helper to convert any byte-like span to a std::byte-span, which is then accepted by serialization.

Can drop this line from PR description (https://github.com/bitcoin/bitcoin/pull/27927#issue-1767744286)

re: #27927 (comment)

Thanks for the responses. You convinced me there probably little benefit to preventing serialization of char spans, and it would just add boilerplate and not make things safer.

I do think we can drop the AsBytePtr function, though, so I tried to do that in #27978.

I also still think in the longer run it would be good to convert more internal code to use std::byte instead of unsigned char and replace UCharCast with a ByteCast alternative and MakeUCharSpan with a safer MakeUCharSpan alternative. If code calling leveldb or libsecp needs uchar functions, it implement them locally, or we could keep existing functions, but implement them in terms of ByteCast. Not suggesting to do this now. I just think we might end up doing it when more code uses std::byte.

utACK fa38d862358b87219b12bf31236c52f28d9fc5d6

I do wonder: is there any reason why we wouldn't want to support serializing Spans in general (where its serialization is defined as the concatenation of the serialization of its elements)?

I do wonder: is there any reason why we wouldn't want to support serializing Spans in general (where its serialization is defined as the concatenation of the serialization of its elements)?

I think that should be fine to add, once there is a use case. Or is there already one?

ACK fa38d862358b87219b12bf31236c52f28d9fc5d6

I think that should be fine to add, once there is a use case. Or is there already one?

Not that I know, no. I was more wondering why we'd only add a specialized one for byte-like types when a generic one for all spans would work equally well. And I think the answer is that even with a generic one, we'd probably want a more efficient specialization for byte-like types anyway, to avoid processing one byte at a time.

Yeah, I am planning to do a more thorough cleanup with C++20, see my comment directly above and the other one earlier in the thread:

Also, Span<B> could be used internally to serialize byte-holding (pre)vectors. In fact this is likely a performance bug-fix, because currently, serialization accepts std::vector<B> for B=signed char, or B=int8_t. (B=char is deleted). However, serialization seems to only apply the optimization to only call write once for B=unsigned char. See

https://github.com/bitcoin/bitcoin/blob/f1b4975461364d5d40d2bfafc6b165dd5d7eec30/src/serialize.h#L651 It may be good to treat all byte-like types the same, which can be achieved with Span<B>, or alternatively also delete the other B-types that are not special-cased, or alternatively delete the special-case code to treat all vectors the same.