Skip to content

Commit 61d0324

Browse files
Copilotdannywillems
andcommitted
Add detailed documentation about intentional read/write asymmetry
Clarify that the asymmetry between reading (accepting any i64) and writing (casting to signed type) is intentional for robustness and wire format compatibility with the OCaml implementation. Co-authored-by: dannywillems <6018454+dannywillems@users.noreply.github.com>
1 parent f1426d8 commit 61d0324

File tree

1 file changed

+19
-3
lines changed

1 file changed

+19
-3
lines changed

mina-p2p-messages/src/number.rs

Lines changed: 19 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -205,21 +205,34 @@ binprot_number!(f64, f64);
205205
// type's range. We read as i64 first, then cast to handle out-of-range values
206206
// gracefully. Writing still uses the smaller type to maintain wire format
207207
// compatibility.
208+
//
209+
// Note: This introduces intentional asymmetry between read and write for the sake
210+
// of robustness. When reading, we accept any i64 value and cast using wrapping
211+
// semantics. When writing, we cast to the signed type (which may change the sign
212+
// bit for large values). This is acceptable because:
213+
// 1. The wire format is defined by the original OCaml implementation which uses
214+
// signed integers for the binprot encoding
215+
// 2. Rust code reading these values will interpret them correctly via the cast
216+
// 3. This prevents connection drops from malformed or unexpected data
208217
impl binprot::BinProtRead for Number<u32> {
209218
fn binprot_read<R: std::io::Read + ?Sized>(r: &mut R) -> Result<Self, binprot::Error>
210219
where
211220
Self: Sized,
212221
{
213222
// Read the value as i64 first to avoid TryFromIntError when the value
214223
// is outside i32 range, then cast to u32 using wrapping semantics.
224+
// This handles cases where the binprot stream contains values outside
225+
// the i32 range (e.g., large nonce values).
215226
let value = i64::binprot_read(r)?;
216227
Ok(Self(value as u32))
217228
}
218229
}
219230

220231
impl binprot::BinProtWrite for Number<u32> {
221232
fn binprot_write<W: std::io::Write>(&self, w: &mut W) -> std::io::Result<()> {
222-
// Write as i32 to maintain wire format compatibility
233+
// Write as i32 to maintain wire format compatibility with the OCaml
234+
// implementation. For values > i32::MAX, the cast will reinterpret
235+
// the bits as a negative i32, which is the expected behavior.
223236
(self.0 as i32).binprot_write(w)
224237
}
225238
}
@@ -230,15 +243,18 @@ impl binprot::BinProtRead for Number<u64> {
230243
Self: Sized,
231244
{
232245
// Read the value as i64 first to handle the full range without errors,
233-
// then cast to u64 using wrapping semantics.
246+
// then cast to u64 using wrapping semantics. This prevents TryFromIntError
247+
// for negative i64 values or values outside u64::MIN..=i64::MAX.
234248
let value = i64::binprot_read(r)?;
235249
Ok(Self(value as u64))
236250
}
237251
}
238252

239253
impl binprot::BinProtWrite for Number<u64> {
240254
fn binprot_write<W: std::io::Write>(&self, w: &mut W) -> std::io::Result<()> {
241-
// Write as i64 to maintain wire format compatibility
255+
// Write as i64 to maintain wire format compatibility. For values > i64::MAX,
256+
// the cast will reinterpret the bits as a negative i64, matching the OCaml
257+
// implementation's behavior.
242258
(self.0 as i64).binprot_write(w)
243259
}
244260
}

0 commit comments

Comments
 (0)