From 4621d2bde14d71b3d6ef0b14258a7479c049ba3b Mon Sep 17 00:00:00 2001 From: "Tobin C. Harding" Date: Thu, 15 May 2025 09:40:15 +1000 Subject: [PATCH] Modify locktime serde implemenations The `units::locktime` types are used for two things: - They are the inner types of `primitives` `LockTime`s - They are used ephemerally for checking satisfaction Neither of these use cases requires `serde` impls for the `units` types. Since we are trying to release 1.0 with minimal amounts of code we should remove them. For `LockTime`s that need to be stored on disk or go over the wire we can manually implement the `serde` traits. For `absolute::LockTime` this is done already and there is no reason the `relative::LockTime` impl cannot be the same [0]. This differs from the current `serde` trait impls but we have already decided that in 0.33 we are going to accept breakage and direct users to use 0.32 to handle it. - Remove `serde` stuff from `units::locktime` - Manually implement `serde` traits on `relative::LockTime` - Fix the regression test to use the new format While we are at it use a uniform terse call in `serialize`. [0] This is because there is an unambiguous encoding for the whole set of locktimes - consensus encoding. --- .../serde/relative_lock_time_blocks_bincode | Bin 6 -> 4 bytes .../serde/relative_lock_time_seconds_bincode | Bin 6 -> 4 bytes primitives/src/locktime/absolute.rs | 23 +------ primitives/src/locktime/relative.rs | 22 +++++- units/src/locktime/absolute.rs | 60 ---------------- units/src/locktime/relative.rs | 65 ------------------ units/tests/data/serde_bincode | Bin 147 -> 135 bytes units/tests/serde.rs | 17 +---- 8 files changed, 26 insertions(+), 161 deletions(-) diff --git a/bitcoin/tests/data/serde/relative_lock_time_blocks_bincode b/bitcoin/tests/data/serde/relative_lock_time_blocks_bincode index 4eb84bb45911c09de9bdc6e7ff57f8fa89090a80..167e25d50c5f161edba7ac04d8bdedd9a8f61a27 100644 GIT binary patch literal 4 Lcmex&ih%(D23i5h literal 6 NcmZQzU|{%n3IGDk0m%RW diff --git a/bitcoin/tests/data/serde/relative_lock_time_seconds_bincode b/bitcoin/tests/data/serde/relative_lock_time_seconds_bincode index b6ed9a62a79422b1e6555e84e8d0a8830abcccae..53442e8bb233bb74ea6fd91e7e5e509de6864ef3 100644 GIT binary patch literal 4 LcmX^2%Ygv^2C4!H literal 6 NcmZQ%U|=}+3jhM50m=XX diff --git a/primitives/src/locktime/absolute.rs b/primitives/src/locktime/absolute.rs index 808ff6c63..53225df3e 100644 --- a/primitives/src/locktime/absolute.rs +++ b/primitives/src/locktime/absolute.rs @@ -413,7 +413,7 @@ impl serde::Serialize for LockTime { where S: serde::Serializer, { - serializer.serialize_u32(self.to_consensus_u32()) + self.to_consensus_u32().serialize(serializer) } } @@ -423,26 +423,7 @@ impl<'de> serde::Deserialize<'de> for LockTime { where D: serde::Deserializer<'de>, { - struct Visitor; - impl serde::de::Visitor<'_> for Visitor { - type Value = u32; - fn expecting(&self, f: &mut fmt::Formatter) -> fmt::Result { f.write_str("a u32") } - // We cannot just implement visit_u32 because JSON (among other things) always - // calls visit_u64, even when called from Deserializer::deserialize_u32. The - // other visit_u*s have default implementations that forward to visit_u64. - fn visit_u64(self, v: u64) -> Result { - v.try_into().map_err(|_| { - E::invalid_value(serde::de::Unexpected::Unsigned(v), &"a 32-bit number") - }) - } - // Also do the signed version, just for good measure. - fn visit_i64(self, v: i64) -> Result { - v.try_into().map_err(|_| { - E::invalid_value(serde::de::Unexpected::Signed(v), &"a 32-bit number") - }) - } - } - deserializer.deserialize_u32(Visitor).map(LockTime::from_consensus) + u32::deserialize(deserializer).map(Self::from_consensus) } } diff --git a/primitives/src/locktime/relative.rs b/primitives/src/locktime/relative.rs index fd58ff1db..5bb2dc1ab 100644 --- a/primitives/src/locktime/relative.rs +++ b/primitives/src/locktime/relative.rs @@ -42,7 +42,6 @@ pub type Time = NumberOf512Seconds; /// * [BIP 68 Relative lock-time using consensus-enforced sequence numbers](https://github.com/bitcoin/bips/blob/master/bip-0065.mediawiki) /// * [BIP 112 CHECKSEQUENCEVERIFY](https://github.com/bitcoin/bips/blob/master/bip-0112.mediawiki) #[derive(Debug, Clone, Copy, PartialEq, Eq, Hash)] -#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] pub enum LockTime { /// A block height lock time value. Blocks(NumberOfBlocks), @@ -372,6 +371,27 @@ impl From for Sequence { fn from(lt: LockTime) -> Sequence { lt.to_sequence() } } +#[cfg(feature = "serde")] +impl serde::Serialize for LockTime { + fn serialize(&self, serializer: S) -> Result + where + S: serde::Serializer, + { + self.to_consensus_u32().serialize(serializer) + } +} + +#[cfg(feature = "serde")] +impl<'de> serde::Deserialize<'de> for LockTime { + fn deserialize(deserializer: D) -> Result + where + D: serde::Deserializer<'de>, + { + u32::deserialize(deserializer) + .and_then(|n| Self::from_consensus(n).map_err(serde::de::Error::custom)) + } +} + /// Error returned when a sequence number is parsed as a lock time, but its /// "disable" flag is set. #[derive(Debug, Clone, Eq, PartialEq)] diff --git a/units/src/locktime/absolute.rs b/units/src/locktime/absolute.rs index c0e2b8417..1787ea089 100644 --- a/units/src/locktime/absolute.rs +++ b/units/src/locktime/absolute.rs @@ -122,27 +122,6 @@ impl From for ParseHeightError { fn from(value: ParseError) -> Self { Self(value) } } -#[cfg(feature = "serde")] -impl<'de> serde::Deserialize<'de> for Height { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - let u = u32::deserialize(deserializer)?; - Height::from_u32(u).map_err(serde::de::Error::custom) - } -} - -#[cfg(feature = "serde")] -impl serde::Serialize for Height { - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - self.to_u32().serialize(serializer) - } -} - #[deprecated(since = "TBD", note = "use `MedianTimePast` instead")] #[doc(hidden)] pub type Time = MedianTimePast; @@ -248,27 +227,6 @@ impl fmt::Display for MedianTimePast { crate::impl_parse_str!(MedianTimePast, ParseTimeError, parser(MedianTimePast::from_u32)); -#[cfg(feature = "serde")] -impl<'de> serde::Deserialize<'de> for MedianTimePast { - fn deserialize(deserializer: D) -> Result - where - D: serde::Deserializer<'de>, - { - let u = u32::deserialize(deserializer)?; - MedianTimePast::from_u32(u).map_err(serde::de::Error::custom) - } -} - -#[cfg(feature = "serde")] -impl serde::Serialize for MedianTimePast { - fn serialize(&self, serializer: S) -> Result - where - S: serde::Serializer, - { - self.to_u32().serialize(serializer) - } -} - /// Error returned when parsing block time fails. #[derive(Debug, Clone, Eq, PartialEq)] pub struct ParseTimeError(ParseError); @@ -500,9 +458,6 @@ impl<'a> Arbitrary<'a> for MedianTimePast { #[cfg(test)] mod tests { - #[cfg(feature = "serde")] - use internals::serde_round_trip; - use super::*; #[test] @@ -554,21 +509,6 @@ mod tests { assert!(is_block_time(500_000_000)); } - #[test] - #[cfg(feature = "serde")] - pub fn encode_decode_height() { - serde_round_trip!(Height::ZERO); - serde_round_trip!(Height::MIN); - serde_round_trip!(Height::MAX); - } - - #[test] - #[cfg(feature = "serde")] - pub fn encode_decode_time() { - serde_round_trip!(MedianTimePast::MIN); - serde_round_trip!(MedianTimePast::MAX); - } - #[test] #[cfg(feature = "alloc")] fn locktime_unit_display() { diff --git a/units/src/locktime/relative.rs b/units/src/locktime/relative.rs index 8aa218db9..f3dfba28a 100644 --- a/units/src/locktime/relative.rs +++ b/units/src/locktime/relative.rs @@ -7,8 +7,6 @@ use core::fmt; #[cfg(feature = "arbitrary")] use arbitrary::{Arbitrary, Unstructured}; -#[cfg(feature = "serde")] -use serde::{Deserialize, Deserializer, Serialize, Serializer}; #[deprecated(since = "TBD", note = "use `NumberOfBlocks` instead")] #[doc(hidden)] @@ -89,28 +87,6 @@ impl fmt::Display for NumberOfBlocks { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) } } -#[cfg(feature = "serde")] -impl Serialize for NumberOfBlocks { - #[inline] - fn serialize(&self, s: S) -> Result - where - S: Serializer, - { - u16::serialize(&self.to_height(), s) - } -} - -#[cfg(feature = "serde")] -impl<'de> Deserialize<'de> for NumberOfBlocks { - #[inline] - fn deserialize(d: D) -> Result - where - D: Deserializer<'de>, - { - Ok(Self::from_height(u16::deserialize(d)?)) - } -} - #[deprecated(since = "TBD", note = "use `NumberOf512Seconds` instead")] #[doc(hidden)] pub type Time = NumberOf512Seconds; @@ -227,28 +203,6 @@ impl fmt::Display for NumberOf512Seconds { fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0, f) } } -#[cfg(feature = "serde")] -impl Serialize for NumberOf512Seconds { - #[inline] - fn serialize(&self, s: S) -> Result - where - S: Serializer, - { - u16::serialize(&self.to_512_second_intervals(), s) - } -} - -#[cfg(feature = "serde")] -impl<'de> Deserialize<'de> for NumberOf512Seconds { - #[inline] - fn deserialize(d: D) -> Result - where - D: Deserializer<'de>, - { - Ok(Self::from_512_second_intervals(u16::deserialize(d)?)) - } -} - /// Error returned when the input time in seconds was too large to be encoded to a 16 bit 512 second interval. #[derive(Debug, Clone, PartialEq, Eq)] pub struct TimeOverflowError { @@ -348,9 +302,6 @@ impl<'a> Arbitrary<'a> for NumberOf512Seconds { #[cfg(test)] mod tests { - #[cfg(feature = "serde")] - use internals::serde_round_trip; - use super::*; use crate::BlockTime; @@ -416,22 +367,6 @@ mod tests { assert!(result.is_err()); } - #[test] - #[cfg(feature = "serde")] - pub fn encode_decode_height() { - serde_round_trip!(NumberOfBlocks::ZERO); - serde_round_trip!(NumberOfBlocks::MIN); - serde_round_trip!(NumberOfBlocks::MAX); - } - - #[test] - #[cfg(feature = "serde")] - pub fn encode_decode_time() { - serde_round_trip!(NumberOf512Seconds::ZERO); - serde_round_trip!(NumberOf512Seconds::MIN); - serde_round_trip!(NumberOf512Seconds::MAX); - } - fn generate_timestamps(start: u32, step: u16) -> [BlockTime; 11] { let mut timestamps = [BlockTime::from_u32(0); 11]; for (i, ts) in timestamps.iter_mut().enumerate() { diff --git a/units/tests/data/serde_bincode b/units/tests/data/serde_bincode index 51d7dc1d09ea388238a66e571ba6b609e2685997..556b15cd794bd56e1ef82aa1bd0f4bf3642fd607 100644 GIT binary patch delta 14 RcmbQt*v>eif&D)O001<=2`vBs delta 26 UcmZo?oXj|(K{(~C?0+->0MlO+egFUf diff --git a/units/tests/serde.rs b/units/tests/serde.rs index 4c93da1f0..dd894aeaa 100644 --- a/units/tests/serde.rs +++ b/units/tests/serde.rs @@ -6,10 +6,7 @@ #![cfg(feature = "serde")] use bincode::serialize; -use bitcoin_units::locktime::{absolute, relative}; -use bitcoin_units::{ - amount, fee_rate, Amount, BlockHeight, BlockInterval, FeeRate, SignedAmount, Weight, -}; +use bitcoin_units::{amount, fee_rate, Amount, BlockHeight, BlockInterval, FeeRate, SignedAmount, Weight}; use serde::{Deserialize, Serialize}; /// A struct that includes all the types that implement or support `serde` traits. @@ -48,11 +45,7 @@ struct Serde { a: BlockHeight, b: BlockInterval, - c: absolute::Height, - d: absolute::MedianTimePast, - e: relative::Height, - f: relative::Time, - g: Weight, + c: Weight, } impl Serde { @@ -81,11 +74,7 @@ impl Serde { a: BlockHeight::MAX, b: BlockInterval::MAX, - c: absolute::Height::MAX, - d: absolute::MedianTimePast::MAX, - e: relative::Height::MAX, - f: relative::Time::MAX, - g: Weight::MAX, + c: Weight::MAX, } } }