streams: Drop confusing DataStream::Serialize method and << operator #27800
Conversation
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 bitcoin#27790 (comment)) so this PR just drops the DataStream::Serialize method.
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee 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. |
|
Concept ACK. I agree this operator is confusing and inconsistent with the properties you'd expect from serialization. |
| DataStream finalAlert{ParseHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50")}; | ||
| m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make("alert", finalAlert)); | ||
| const auto finalAlert{ParseHex("60010000000000000000000000ffffff7f00000000ffffff7ffeffff7f01ffffff7f00000000ffffff7f00ffffff7f002f555247454e543a20416c657274206b657920636f6d70726f6d697365642c2075706772616465207265717569726564004630440220653febd6410f470f6bae11cad19c48413becb1ac2c17f908fd0fd53bdc3abd5202206d0e9c96fe88d4a0f01ed9dedae2b6f9e00da94cad0fecaae66ecf689bf71b50")}; | ||
| m_connman.PushMessage(&pfrom, CNetMsgMaker(greatest_common_version).Make("alert", Span{finalAlert})); |
There was a problem hiding this comment.
does this need to be a Span? Wouldn't be the same to just pass finalAlert?
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Makes sense even absent the confusion, because Span also avoids a temporary copy to a std::vector (in DataStream).
lgtm ACK 5cd0717 🌙
Show signature
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==
| template<typename Stream> | ||
| void Serialize(Stream& s) const | ||
| { | ||
| // Special case: stream << stream concatenates like stream += stream |
…thod and << operator 5cd0717 streams: Drop confusing DataStream::Serialize method and << operator (Ryan Ofsky) Pull request description: 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 bitcoin#27790 (comment)) so this PR just drops the DataStream::Serialize method. ACKs for top commit: furszy: lgtm ACK 5cd0717 MarcoFalke: lgtm ACK 5cd0717 🌙 Tree-SHA512: 49dd117de266f091a5336b13a91c5d8658abe1b3a0a9c51c8b5f6a2e0e814781b73afc39256353e79dade603a8a2761e8536716d1a48499720c266f4500477e2
…alize method and << operator" This reverts commit e1a955a.
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 (comment)) so this PR just drops the DataStream::Serialize method.