Merge rust-bitcoin/rust-bitcoin#4511: Modify `locktime` `serde` implementations

4621d2bde1 Modify locktime serde implemenations (Tobin C. Harding)
200c276315 bitcoin: Make test code spacing uniform (Tobin C. Harding)

Pull request description:

  Patch 1 is preparatory clean up. Patch 2 is the meat and potatoes. See commit log there for full explanation.

  Briefly:

      - Remove `serde` stuff from `units::locktime`
      - Manually implement `serde` traits on `relative::LockTime`
      - Fix the regression test to use the new format

ACKs for top commit:
  apoelstra:
    ACK 4621d2bde14d71b3d6ef0b14258a7479c049ba3b; successfully ran local tests

Tree-SHA512: bc96d79dd4d9f5a124c22f0d3be4750cb7b6e86ba448b31e233982567578337d331b2582c6e1f953c00e8393c4a4552c4b574fe8a55323c911115b269b24090e

bitcoin/tests/serde.rs

@@ -47,6 +47,7 @@ fn serde_regression_block() {
         "data/testnet_block_000000000000045e0b1660b6445b5e5c5ab63c9a4f956be7e1e69be04fa4497b.raw"
     );
     let block: Block = deserialize(segwit).unwrap();
+
     let got = serialize(&block).unwrap();
     let want = include_bytes!("data/serde/block_bincode");
     assert_eq!(got, want)
@@ -55,6 +56,7 @@ fn serde_regression_block() {
 #[test]
 fn serde_regression_absolute_lock_time_height() {
     let t = absolute::LockTime::from_height(741521).expect("valid height");
+
     let got = serialize(&t).unwrap();
     let want = include_bytes!("data/serde/absolute_lock_time_blocks_bincode") as &[_];
     assert_eq!(got, want);
@@ -64,8 +66,8 @@ fn serde_regression_absolute_lock_time_height() {
 fn serde_regression_absolute_lock_time_time() {
     let seconds: u32 = 1653195600; // May 22nd, 5am UTC.
     let t = absolute::LockTime::from_mtp(seconds).expect("valid time");
-    let got = serialize(&t).unwrap();
 
+    let got = serialize(&t).unwrap();
     let want = include_bytes!("data/serde/absolute_lock_time_seconds_bincode") as &[_];
     assert_eq!(got, want);
 }
@@ -73,8 +75,8 @@ fn serde_regression_absolute_lock_time_time() {
 #[test]
 fn serde_regression_relative_lock_time_height() {
     let t = relative::LockTime::from(relative::Height::from(0xCAFE_u16));
-    let got = serialize(&t).unwrap();
 
+    let got = serialize(&t).unwrap();
     let want = include_bytes!("data/serde/relative_lock_time_blocks_bincode") as &[_];
     assert_eq!(got, want);
 }
@@ -82,8 +84,8 @@ fn serde_regression_relative_lock_time_height() {
 #[test]
 fn serde_regression_relative_lock_time_time() {
     let t = relative::LockTime::from(relative::Time::from_512_second_intervals(0xFACE_u16));
-    let got = serialize(&t).unwrap();
 
+    let got = serialize(&t).unwrap();
     let want = include_bytes!("data/serde/relative_lock_time_seconds_bincode") as &[_];
     assert_eq!(got, want);
 }
@@ -91,6 +93,7 @@ fn serde_regression_relative_lock_time_time() {
 #[test]
 fn serde_regression_script() {
     let script = ScriptBuf::from(vec![0u8, 1u8, 2u8]);
+
     let got = serialize(&script).unwrap();
     let want = include_bytes!("data/serde/script_bincode") as &[_];
     assert_eq!(got, want)
@@ -109,6 +112,7 @@ fn serde_regression_txin() {
 #[test]
 fn serde_regression_txout() {
     let txout = TxOut { value: Amount::MAX, script_pubkey: ScriptBuf::from(vec![0u8, 1u8, 2u8]) };
+
     let got = serialize(&txout).unwrap();
     let want = include_bytes!("data/serde/txout_bincode") as &[_];
     assert_eq!(got, want)
@@ -118,6 +122,7 @@ fn serde_regression_txout() {
 fn serde_regression_transaction() {
     let ser = include_bytes!("data/serde/transaction_ser");
     let tx: Transaction = deserialize(ser).unwrap();
+
     let got = serialize(&tx).unwrap();
     let want = include_bytes!("data/serde/transaction_bincode") as &[_];
     assert_eq!(got, want)
@@ -151,6 +156,7 @@ fn serde_regression_address() {
 fn serde_regression_extended_priv_key() {
     let s = include_str!("data/serde/extended_priv_key");
     let key = s.trim().parse::<Xpriv>().unwrap();
+
     let got = serialize(&key).unwrap();
     let want = include_bytes!("data/serde/extended_priv_key_bincode") as &[_];
     assert_eq!(got, want)
@@ -160,6 +166,7 @@ fn serde_regression_extended_priv_key() {
 fn serde_regression_extended_pub_key() {
     let s = include_str!("data/serde/extended_pub_key");
     let key = s.trim().parse::<Xpub>().unwrap();
+
     let got = serialize(&key).unwrap();
     let want = include_bytes!("data/serde/extended_pub_key_bincode") as &[_];
     assert_eq!(got, want)
@@ -182,8 +189,8 @@ fn serde_regression_ecdsa_sig() {
 fn serde_regression_control_block() {
     let s = include_str!("data/serde/control_block_hex");
     let block = ControlBlock::decode(&Vec::<u8>::from_hex(s.trim()).unwrap()).unwrap();
-    let got = serialize(&block).unwrap();
 
+    let got = serialize(&block).unwrap();
     let want = include_bytes!("data/serde/control_block_bincode") as &[_];
     assert_eq!(got, want)
 }
@@ -191,6 +198,7 @@ fn serde_regression_control_block() {
 #[test]
 fn serde_regression_child_number() {
     let num = ChildNumber::Normal { index: 0xDEADBEEF };
+
     let got = serialize(&num).unwrap();
     let want = include_bytes!("data/serde/child_number_bincode") as &[_];
     assert_eq!(got, want)
@@ -199,6 +207,7 @@ fn serde_regression_child_number() {
 #[test]
 fn serde_regression_private_key() {
     let sk = PrivateKey::from_wif("cVt4o7BGAig1UXywgGSmARhxMdzP5qvQsxKkSsc1XEkw3tDTQFpy").unwrap();
+
     let got = serialize(&sk).unwrap();
     let want = include_bytes!("data/serde/private_key_bincode") as &[_];
     assert_eq!(got, want)
@@ -208,6 +217,7 @@ fn serde_regression_private_key() {
 fn serde_regression_public_key() {
     let s = include_str!("data/serde/public_key_hex");
     let pk = s.trim().parse::<PublicKey>().unwrap();
+
     let got = serialize(&pk).unwrap();
     let want = include_bytes!("data/serde/public_key_bincode") as &[_];
     assert_eq!(got, want)
@@ -368,6 +378,7 @@ fn le_bytes() -> [u8; 32] {
 #[test]
 fn serde_regression_work() {
     let work = Work::from_le_bytes(le_bytes());
+
     let got = serialize(&work).unwrap();
     let want = include_bytes!("data/serde/u256_bincode") as &[_];
     assert_eq!(got, want)
@@ -376,6 +387,7 @@ fn serde_regression_work() {
 #[test]
 fn serde_regression_target() {
     let target = Target::from_le_bytes(le_bytes());
+
     let got = serialize(&target).unwrap();
     let want = include_bytes!("data/serde/u256_bincode") as &[_];
     assert_eq!(got, want)

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<E: serde::de::Error>(self, v: u64) -> Result<u32, E> {
-                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<E: serde::de::Error>(self, v: i64) -> Result<u32, E> {
-                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)
     }
 }
 

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<LockTime> for Sequence {
     fn from(lt: LockTime) -> Sequence { lt.to_sequence() }
 }
 
+#[cfg(feature = "serde")]
+impl serde::Serialize for LockTime {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        self.to_consensus_u32().serialize(serializer)
+    }
+}
+
+#[cfg(feature = "serde")]
+impl<'de> serde::Deserialize<'de> for LockTime {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    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)]

units/src/locktime/absolute.rs

@@ -122,27 +122,6 @@ impl From<ParseError> for ParseHeightError {
     fn from(value: ParseError) -> Self { Self(value) }
 }
 
-#[cfg(feature = "serde")]
-impl<'de> serde::Deserialize<'de> for Height {
-    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
-    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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    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<D>(deserializer: D) -> Result<Self, D::Error>
-    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<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
-    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() {

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<S>(&self, s: S) -> Result<S::Ok, S::Error>
-    where
-        S: Serializer,
-    {
-        u16::serialize(&self.to_height(), s)
-    }
-}
-
-#[cfg(feature = "serde")]
-impl<'de> Deserialize<'de> for NumberOfBlocks {
-    #[inline]
-    fn deserialize<D>(d: D) -> Result<Self, D::Error>
-    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<S>(&self, s: S) -> Result<S::Ok, S::Error>
-    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: D) -> Result<Self, D::Error>
-    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() {

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,
         }
     }
 }