streams: Drop confusing DataStream::Serialize method and << operator #27800

ryanofsky commented at 2:39 PM on June 1, 2023: contributor

DataStream Serialize method has surprising behavior because it just serializes raw bytes without a length prefix. When you serialize a string or vector, a length prefix is serialized before the raw object contents so the object can be unambiguously deserialized later. But DataStreams don't support deserializing at all and just dump the raw bytes.

Having this inconsistency is not necessary and could be confusing (see #27790 (review)) so this PR just drops the DataStream::Serialize method.

streams: Drop confusing DataStream::Serialize method and << operator

DataStream Serialize method has surprising behavior because it just serializes
raw bytes without a length prefix. When you serialize a string or vector, a
length prefix is serialized before the raw object contents so the object can be
unambiguously deserialized later. But DataStreams don't support deserializing
at all and just dump the raw bytes.

Having this inconsistency is not necessary and could be confusing (see
https://github.com/bitcoin/bitcoin/pull/27790#discussion_r1212315030) so this
PR just drops the DataStream::Serialize method.

5cd0717a54

DrahtBot commented at 2:39 PM on June 1, 2023: 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	furszy, MarcoFalke
Concept ACK	sipa

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

sipa commented at 2:53 PM on June 1, 2023: member

Concept ACK. I agree this operator is confusing and inconsistent with the properties you'd expect from serialization.

ryanofsky marked this as ready for review on Jun 1, 2023

in src/net_processing.cpp:3422 in 5cd0717a54

3417 | @@ -3418,8 +3418,8 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
3418 |  
3419 |          // If the peer is old enough to have the old alert system, send it the final alert.
3420 |          if (greatest_common_version <= 70012) {
3421 | -            DataStream finalAlert{ParseHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50")};
3422 | -            m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make("alert", finalAlert));
3423 | +            const auto finalAlert{ParseHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50")};
3424 | +            m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make("alert", Span{finalAlert}));

furszy commented at 9:03 PM on June 1, 2023:

does this need to be a Span? Wouldn't be the same to just pass finalAlert?

ryanofsky commented at 9:35 PM on June 1, 2023:

does this need to be a Span? Wouldn't be the same to just pass finalAlert?

Because finalAlert is a vector of bytes, serializing the vector would first write the number of bytes in the vector to the stream, followed by the bytes themselves. Since spans unlike vectors are fixed length, serializing a span just writes the raw bytes to the stream without a length prefix.

furszy commented at 9:59 PM on June 1, 2023:

ah ok. I mixed up stuff by reading the CVectorWriter class description -> "Minimal stream for overwriting and/or appending to an existing byte vector" (it doesn't mention the serialization..). I should have checked the constructor. My bad, thanks.

furszy commented at 9:32 PM on June 1, 2023: member

lgtm ACK 5cd0717a

in src/streams.h:299 in 5cd0717a54

 292 | @@ -293,14 +293,6 @@ class DataStream
 293 |          vch.insert(vch.end(), src.begin(), src.end());
 294 |      }
 295 |  
 296 | -    template<typename Stream>
 297 | -    void Serialize(Stream& s) const
 298 | -    {
 299 | -        // Special case: stream << stream concatenates like stream += stream

maflcko commented at 6:41 AM on June 2, 2023:

+= removed in faa96f841fe45bc49ebb6e07ac82a129fa9c40bf

maflcko approved

maflcko commented at 6:42 AM on June 2, 2023: member

Makes sense even absent the confusion, because Span also avoids a temporary copy to a std::vector (in DataStream).

lgtm ACK 5cd0717a54ce7a2065b29d90717aa2eb1c5e302d 🌙

<details><summary>Show signature</summary>

Signature:

untrusted comment: signature from minisign secret key on empty file; verify via: minisign -Vm "${path_to_any_empty_file}" -P RWTRmVTMeKV5noAMqVlsMugDDCyyTSbA3Re5AkUrhvLVln0tSaFWglOw -x "${path_to_this_whole_four_line_signature_blob}"
RUTRmVTMeKV5npGrKx1nqXCw5zeVHdtdYURB/KlyA/LMFgpNCs+SkW9a8N95d+U4AP1RJMi+krxU1A3Yux4bpwZNLvVBKy0wLgM=
trusted comment: lgtm ACK 5cd0717a54ce7a2065b29d90717aa2eb1c5e302d 🌙
x1wRNoDWKp74p2vgJQphtf4s6Ofy7DBQJHpDr5i8p0Fvt0i6DvmcRSSpOi1ZNES9Cs+6hr5vFRsJ9+5h2nQ8Dg==

</details>

fanquake merged this on Jun 2, 2023

fanquake closed this on Jun 2, 2023

sidhujag referenced this in commit e1a955acce on Jun 2, 2023

sidhujag referenced this in commit 6297d2bb4e on Jun 2, 2023

bitcoin locked this on Jun 1, 2024