Repository: rust-bitcoin/rust-bitcoin
Pull request #2582 (2024-03-13 02:03:56)

fix: Manually implement serde traits
Currently we are deriving the serde traits for the `absolute::{Height, Time}` types, this is incorrect because we maintain an invariant on the inner `u32` of both types that it is above or below the threshold.

Manually implement the serde traits and pass the deserialized `u32` to `from_consensus` to maintain the invariant.

Close: #2559

coveralls: ## Pull Request Test Coverage Report for [Build 8258385134](https://coveralls.io/builds/66251771)


### Details

* **0** of **66**   **(0.0%)**  changed or added relevant lines in **1** file are covered.
* No unchanged relevant lines lost coverage.
* Overall coverage decreased (**-0.2%**) to **83.812%**

---

|  Changes Missing Coverage | Covered Lines | Changed/Added Lines | % |
| :-----|--------------|--------|---: |
| [bitcoin/src/blockdata/locktime/absolute.rs](https://coveralls.io/builds/66251771/source?filename=bitcoin%2Fsrc%2Fblockdata%2Flocktime%2Fabsolute.rs#L482) | 0 | 66 | 0.0%
<!-- | **Total:** | **0** | **66** | **0.0%** | -->


|  Totals | [![Coverage Status](https://coveralls.io/builds/66251771/badge)](https://coveralls.io/builds/66251771) |
| :-- | --: |
| Change from base [Build 8251085872](https://coveralls.io/builds/66237344): |  -0.2% |
| Covered Lines: | 19540 |
| Relevant Lines: | 23314 |

---
##### 💛  - [Coveralls](https://coveralls.io)

apoelstra: Why not just use `serde(try_from = "LockTime")` and `serde(into = "LockTime")]` as suggested in the issue?
apoelstra: >I was under the impression that we eventually get rid of derive feature in serde.

Yeah, we should, but I think that's separate from this, and I'd have an easier time reviewing if we used the `derive` flags which are easy to understand, then add fixed-vector tests, then introduced our own macro system to replace it.
tcharding: > Why not just use `serde(try_from = "LockTime")` and `serde(into = "LockTime")]` as suggested in the issue?

My bad, should have said why before. I was working from a world where `Height`/`Time` live in `units` and `LockTime` lives in `bitcoin` - I realized this morning that I may be the only one currently living in this world :)
tcharding: Rebase only, no other changes. Acknowledging that this is not mergable, there is unresolved discussion about this PR still.
apoelstra: Oh, ok, that makes sense. concept ACK then, though I'm also a little tempted to just drop the `serde` impls on these types.
apoelstra: Rather than using the `Visitor` would it be possible to directly call `u32::deserialize(deserializer)`?
tcharding: > I'm also a little tempted to just drop the serde impls on these types.

I'm down with that. Doesn't hurt `rust-bitcoin` because we do manual impls anyway and since these are by definition `units`, or thin wrappers around integers, users can just convert to the inner unit to serde.

https://github.com/rust-bitcoin/rust-bitcoin/issues/2612
tcharding: After playing with #2612 I think we should continue to consider this PR for merge.
apoelstra: Sure. ACK feff559 but could you try using `u32::deserialize` rather than implementing the whole visitor?

Patch:
diff --git a/units/src/lib.rs b/units/src/lib.rs
index 3b42f1c2..1aeea076 100644
--- a/units/src/lib.rs
+++ b/units/src/lib.rs
@@ -33,6 +33,10 @@
 #[cfg(feature = "serde")]
 pub extern crate serde;
 
+#[cfg(test)]
+#[macro_use]
+mod test_macros;
+
 pub mod amount;
 #[cfg(feature = "alloc")]
 pub mod locktime;
diff --git a/units/src/locktime/absolute.rs b/units/src/locktime/absolute.rs
index 0930f3cd..a85e4571 100644
--- a/units/src/locktime/absolute.rs
+++ b/units/src/locktime/absolute.rs
@@ -4,9 +4,6 @@
 
 use core::fmt;
 
-#[cfg(feature = "serde")]
-use serde::{Deserialize, Serialize};
-
 use internals::write_err;
 
 #[cfg(feature = "alloc")]
@@ -28,7 +25,6 @@
 
 /// An absolute block height, guaranteed to always contain a valid height value.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
-#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 pub struct Height(u32);
 
 impl Height {
@@ -100,13 +96,33 @@ 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)?;
+        Ok(Height::from_consensus(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_consensus_u32().serialize(serializer)
+    }
+}
+
 /// A UNIX timestamp, seconds since epoch, guaranteed to always contain a valid time value.
 ///
 /// Note that there is no manipulation of the inner value during construction or when using
 /// `to_consensus_u32()`. Said another way, `Time(x)` means 'x seconds since epoch' _not_ '(x -
 /// threshold) seconds since epoch'.
 #[derive(Debug, Clone, Copy, PartialEq, Eq, PartialOrd, Ord, Hash)]
-#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
 pub struct Time(u32);
 
 impl Time {
@@ -155,6 +171,27 @@ fn fmt(&self, f: &mut fmt::Formatter) -> fmt::Result { fmt::Display::fmt(&self.0
 
 crate::impl_parse_str!(Time, ParseTimeError, parser(Time::from_consensus));
 
+#[cfg(feature = "serde")]
+impl<'de> serde::Deserialize<'de> for Time {
+    fn deserialize<D>(deserializer: D) -> Result<Self, D::Error>
+    where
+        D: serde::Deserializer<'de>,
+    {
+        let u = u32::deserialize(deserializer)?;
+        Ok(Time::from_consensus(u).map_err(serde::de::Error::custom)?)
+    }
+}
+
+#[cfg(feature = "serde")]
+impl serde::Serialize for Time {
+    fn serialize<S>(&self, serializer: S) -> Result<S::Ok, S::Error>
+    where
+        S: serde::Serializer,
+    {
+        self.to_consensus_u32().serialize(serializer)
+    }
+}
+
 /// Error returned when parsing block time fails.
 #[derive(Debug, Clone, Eq, PartialEq)]
 pub struct ParseTimeError(ParseError);
@@ -364,4 +401,18 @@ fn height_from_str_hex_invalid_hex_should_err() {
         assert!(result.is_err());
     }
 
+    #[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!(Time::MIN);
+        serde_round_trip!(Time::MAX);
+    }
 }


Test patch:
diff --git a/units/src/test_macros.rs b/units/src/test_macros.rs
new file mode 100644
index 00000000..bc2d05e2
--- /dev/null
+++ b/units/src/test_macros.rs
@@ -0,0 +1,16 @@
+// SPDX-License-Identifier: CC0-1.0
+
+//! Bitcoin serde macros.
+//!
+//! This module provides internal macros used for unit tests.
+
+#[cfg(feature = "serde")]
+macro_rules! serde_round_trip (
+    ($var:expr) => ({
+        use serde_json;
+
+        let encoded = serde_json::to_value(&$var).unwrap();
+        let decoded = serde_json::from_value(encoded).unwrap();
+        assert_eq!($var, decoded);
+    })
+);