From 8c663c8034cc5d4a99de52d53180305dca7bcb1f Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 9 Dec 2025 21:42:17 +0000 Subject: [PATCH 1/2] Fix TryFromIntError when reading Number and Number from binprot The issue occurred when binprot streams contained i64 values outside the i32 range. The binprot library reads all integers as i64 internally and uses try_from to convert to target types, which fails with TryFromIntError for out-of-range values. The fix changes Number and Number to read i64 directly and cast to the target type, avoiding the intermediate conversion that could fail. Writing still uses the original format to maintain wire compatibility. Added regression tests to verify the fix handles: - i64 values larger than i32::MAX - Negative i64 values - All existing test cases continue to pass Co-authored-by: dannywillems <6018454+dannywillems@users.noreply.github.com> --- mina-p2p-messages/src/number.rs | 99 ++++++++++++++++++++++++++++++++- 1 file changed, 97 insertions(+), 2 deletions(-) diff --git a/mina-p2p-messages/src/number.rs b/mina-p2p-messages/src/number.rs index 221589e144..cb20ed824b 100644 --- a/mina-p2p-messages/src/number.rs +++ b/mina-p2p-messages/src/number.rs @@ -197,10 +197,52 @@ macro_rules! binprot_number { binprot_number!(i32, i32); binprot_number!(i64, i64); -binprot_number!(u32, i32); -binprot_number!(u64, i64); binprot_number!(f64, f64); +// Custom implementations for u32 and u64 to handle the full i64 range +// without TryFromIntError. The binprot format stores all integers as i64 +// internally, and conversion can fail if the value is outside the target +// type's range. We read as i64 first, then cast to handle out-of-range values +// gracefully. Writing still uses the smaller type to maintain wire format +// compatibility. +impl binprot::BinProtRead for Number { + fn binprot_read(r: &mut R) -> Result + where + Self: Sized, + { + // Read the value as i64 first to avoid TryFromIntError when the value + // is outside i32 range, then cast to u32 using wrapping semantics. + let value = i64::binprot_read(r)?; + Ok(Self(value as u32)) + } +} + +impl binprot::BinProtWrite for Number { + fn binprot_write(&self, w: &mut W) -> std::io::Result<()> { + // Write as i32 to maintain wire format compatibility + (self.0 as i32).binprot_write(w) + } +} + +impl binprot::BinProtRead for Number { + fn binprot_read(r: &mut R) -> Result + where + Self: Sized, + { + // Read the value as i64 first to handle the full range without errors, + // then cast to u64 using wrapping semantics. + let value = i64::binprot_read(r)?; + Ok(Self(value as u64)) + } +} + +impl binprot::BinProtWrite for Number { + fn binprot_write(&self, w: &mut W) -> std::io::Result<()> { + // Write as i64 to maintain wire format compatibility + (self.0 as i64).binprot_write(w) + } +} + #[cfg(test)] mod tests { use binprot::{BinProtRead, BinProtWrite}; @@ -257,4 +299,57 @@ mod tests { max_number_test!(u32_max, u32); max_number_test!(u64_max, u64); + + /// Test that Number can read i64 values outside i32 range without error. + /// This is a regression test for the TryFromIntError issue. + #[test] + fn u32_read_large_i64() { + // Create a binprot-encoded i64 value larger than i32::MAX + let large_i64 = (i32::MAX as i64) + 1000; + let mut buf = Vec::new(); + large_i64.binprot_write(&mut buf).unwrap(); + + // This should not fail with TryFromIntError + let mut r = buf.as_slice(); + let result = super::Number::::binprot_read(&mut r); + assert!(result.is_ok(), "Should handle i64 values outside i32 range"); + + // The value should wrap as expected when cast from i64 to u32 + let n = result.unwrap(); + assert_eq!(n.0, large_i64 as u32); + } + + /// Test that Number can read negative i64 values without error. + #[test] + fn u32_read_negative_i64() { + // Create a binprot-encoded negative i64 value + let negative_i64 = -1000i64; + let mut buf = Vec::new(); + negative_i64.binprot_write(&mut buf).unwrap(); + + // This should not fail with TryFromIntError + let mut r = buf.as_slice(); + let result = super::Number::::binprot_read(&mut r); + assert!(result.is_ok(), "Should handle negative i64 values"); + + // The value should wrap as expected when cast from i64 to u32 + let n = result.unwrap(); + assert_eq!(n.0, negative_i64 as u32); + } + + /// Test that Number can read i64 values without error. + #[test] + fn u64_read_i64() { + // Test with a negative value (which would fail with try_from) + let negative_i64 = -5000i64; + let mut buf = Vec::new(); + negative_i64.binprot_write(&mut buf).unwrap(); + + let mut r = buf.as_slice(); + let result = super::Number::::binprot_read(&mut r); + assert!(result.is_ok(), "Should handle negative i64 values"); + + let n = result.unwrap(); + assert_eq!(n.0, negative_i64 as u64); + } } From 37d64c5042c71eb769dc891e5529f3c4e867c7c3 Mon Sep 17 00:00:00 2001 From: "copilot-swe-agent[bot]" <198982749+Copilot@users.noreply.github.com> Date: Tue, 9 Dec 2025 21:53:09 +0000 Subject: [PATCH 2/2] 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> --- mina-p2p-messages/src/number.rs | 22 +++++++++++++++++++--- 1 file changed, 19 insertions(+), 3 deletions(-) diff --git a/mina-p2p-messages/src/number.rs b/mina-p2p-messages/src/number.rs index cb20ed824b..1cf67004cd 100644 --- a/mina-p2p-messages/src/number.rs +++ b/mina-p2p-messages/src/number.rs @@ -205,6 +205,15 @@ binprot_number!(f64, f64); // type's range. We read as i64 first, then cast to handle out-of-range values // gracefully. Writing still uses the smaller type to maintain wire format // compatibility. +// +// Note: This introduces intentional asymmetry between read and write for the sake +// of robustness. When reading, we accept any i64 value and cast using wrapping +// semantics. When writing, we cast to the signed type (which may change the sign +// bit for large values). This is acceptable because: +// 1. The wire format is defined by the original OCaml implementation which uses +// signed integers for the binprot encoding +// 2. Rust code reading these values will interpret them correctly via the cast +// 3. This prevents connection drops from malformed or unexpected data impl binprot::BinProtRead for Number { fn binprot_read(r: &mut R) -> Result where @@ -212,6 +221,8 @@ impl binprot::BinProtRead for Number { { // Read the value as i64 first to avoid TryFromIntError when the value // is outside i32 range, then cast to u32 using wrapping semantics. + // This handles cases where the binprot stream contains values outside + // the i32 range (e.g., large nonce values). let value = i64::binprot_read(r)?; Ok(Self(value as u32)) } @@ -219,7 +230,9 @@ impl binprot::BinProtRead for Number { impl binprot::BinProtWrite for Number { fn binprot_write(&self, w: &mut W) -> std::io::Result<()> { - // Write as i32 to maintain wire format compatibility + // Write as i32 to maintain wire format compatibility with the OCaml + // implementation. For values > i32::MAX, the cast will reinterpret + // the bits as a negative i32, which is the expected behavior. (self.0 as i32).binprot_write(w) } } @@ -230,7 +243,8 @@ impl binprot::BinProtRead for Number { Self: Sized, { // Read the value as i64 first to handle the full range without errors, - // then cast to u64 using wrapping semantics. + // then cast to u64 using wrapping semantics. This prevents TryFromIntError + // for negative i64 values or values outside u64::MIN..=i64::MAX. let value = i64::binprot_read(r)?; Ok(Self(value as u64)) } @@ -238,7 +252,9 @@ impl binprot::BinProtRead for Number { impl binprot::BinProtWrite for Number { fn binprot_write(&self, w: &mut W) -> std::io::Result<()> { - // Write as i64 to maintain wire format compatibility + // Write as i64 to maintain wire format compatibility. For values > i64::MAX, + // the cast will reinterpret the bits as a negative i64, matching the OCaml + // implementation's behavior. (self.0 as i64).binprot_write(w) } }