rfc3339 direct ser/deser fix for protobuf Timestamp (#666)

* Implemented rfc3339 direct serialization/deserialization for protobuf Timestamp
* CHANGELOG update
* Removed custom Time serializer
* Fix whitespace in changelog

Co-authored-by: Romain Ruetschi <romain@informal.systems>

CHANGELOG.md

@@ -15,6 +15,8 @@
   deemed valid instead of invalid ([#650])
 - `[light-client]` Revert a change introduced in [#652] that would enable DoS attacks,
   where full nodes could spam the light client with massive commits (eg. 10k validators).
+- `[tendermint]` (Since RC1) Time serialization fix (part of [#665])
+- `[tendermint-proto]` (Since RC1) Timestamp serialization fix (part of [#665])
 
 ### FEATURES:
 
@@ -25,6 +27,7 @@
 [#652]: https://github.com/informalsystems/tendermint-rs/pulls/652
 [#639]: https://github.com/informalsystems/tendermint-rs/pull/639
 [#660]: https://github.com/informalsystems/tendermint-rs/issues/660
+[#665]: https://github.com/informalsystems/tendermint-rs/issues/665
 
 ## v0.17.0-rc1
 

light-client/src/predicates/errors.rs

@@ -16,10 +16,8 @@ pub enum VerificationError {
     #[error("header from the future: header_time={header_time} now={now}")]
     HeaderFromTheFuture {
         /// Time in the header
-        #[serde(with = "tendermint::serializers::time")]
         header_time: Time,
         /// Current time
-        #[serde(with = "tendermint::serializers::time")]
         now: Time,
     },
 

light-client/src/tests.rs

@@ -38,7 +38,6 @@ pub struct Initial {
     pub signed_header: SignedHeader,
     pub next_validator_set: ValidatorSet,
     pub trusting_period: DurationStr,
-    #[serde(with = "tendermint::serializers::time")]
     pub now: Time,
 }
 
@@ -49,7 +48,6 @@ pub struct TestBisection<LB> {
     pub primary: Provider<LB>,
     pub witnesses: Vec<WitnessProvider<LB>>,
     pub height_to_verify: HeightStr,
-    #[serde(with = "tendermint::serializers::time")]
     pub now: Time,
     pub expected_output: Option<String>,
     pub expected_num_of_bisections: usize,

light-client/tests/model_based.rs

@@ -62,7 +62,6 @@ pub struct SingleStepTestCase {
 pub struct BlockVerdict {
     block: AnonLightBlock,
     testgen_block: TestgenLightBlock,
-    #[serde(with = "tendermint::serializers::time")]
     now: Time,
     verdict: LiteVerdict,
 }
@@ -119,7 +118,8 @@ struct HeaderVersionFuzzer {}
 impl SingleStepTestFuzzer for HeaderVersionFuzzer {
     // TODO: rehash the header and re-compute commit with it
     // TODO: Unlike in tendermint-go, we don't assert for a particular version in rust
-    // TODO: Either add this check in verification or remove this test because otherwise there's no point of it
+    // TODO: Either add this check in verification or remove this test because otherwise there's no
+    // point of it
     fn fuzz_input(input: &mut BlockVerdict) -> (String, bool) {
         let mut rng = rand::thread_rng();
 

proto-compiler/src/constants.rs

@@ -18,7 +18,7 @@ const QUOTED: &str = r#"#[serde(with = "crate::serializers::from_str")]"#;
 const QUOTED_WITH_DEFAULT: &str = r#"#[serde(with = "crate::serializers::from_str", default)]"#;
 const HEXSTRING: &str = r#"#[serde(with = "crate::serializers::bytes::hexstring")]"#;
 const BASE64STRING: &str = r#"#[serde(with = "crate::serializers::bytes::base64string")]"#;
-const TIMESTAMP: &str = r#"#[serde(with = "crate::serializers::option_timestamp")]"#;
+const OPTIONAL: &str = r#"#[serde(with = "crate::serializers::optional")]"#;
 const VEC_SKIP_IF_EMPTY: &str =
     r#"#[serde(skip_serializing_if = "Vec::is_empty", with = "serde_bytes")]"#;
 const NULLABLEVECARRAY: &str = r#"#[serde(with = "crate::serializers::txs")]"#;
@@ -50,7 +50,6 @@ pub static CUSTOM_TYPE_ATTRIBUTES: &[(&str, &str)] = &[
     (".tendermint.types.Vote", SERIALIZED),
     (".tendermint.types.BlockID", SERIALIZED),
     (".tendermint.types.PartSetHeader", SERIALIZED),
-    (".google.protobuf.Timestamp", SERIALIZED),
     (".tendermint.types.LightClientAttackEvidence", SERIALIZED),
     (".tendermint.types.LightBlock", SERIALIZED),
     (".tendermint.types.SignedHeader", SERIALIZED),
@@ -100,7 +99,7 @@ pub static CUSTOM_FIELD_ATTRIBUTES: &[(&str, &str)] = &[
     ),
     (".tendermint.types.PartSetHeader.hash", HEXSTRING),
     (".tendermint.types.Header.height", QUOTED),
-    (".tendermint.types.Header.time", TIMESTAMP),
+    (".tendermint.types.Header.time", OPTIONAL),
     (".tendermint.types.Header.last_commit_hash", HEXSTRING),
     (".tendermint.types.Header.data_hash", HEXSTRING),
     (".tendermint.types.Header.validators_hash", HEXSTRING),
@@ -115,14 +114,14 @@ pub static CUSTOM_FIELD_ATTRIBUTES: &[(&str, &str)] = &[
     (".tendermint.types.Commit.height", QUOTED),
     (".tendermint.types.Commit.signatures", NULLABLE),
     (".tendermint.types.CommitSig.validator_address", HEXSTRING),
-    (".tendermint.types.CommitSig.timestamp", TIMESTAMP),
+    (".tendermint.types.CommitSig.timestamp", OPTIONAL),
     (".tendermint.types.CommitSig.signature", BASE64STRING),
     (".tendermint.types.Vote.round", QUOTED),
     (".tendermint.types.Vote.height", QUOTED),
     (".tendermint.types.Vote.validator_index", QUOTED),
     (".tendermint.types.Vote.validator_address", HEXSTRING),
     (".tendermint.types.Vote.signature", BASE64STRING),
-    (".tendermint.types.Vote.timestamp", TIMESTAMP),
+    (".tendermint.types.Vote.timestamp", OPTIONAL),
     (".tendermint.types.Validator.address", HEXSTRING),
     (
         ".tendermint.types.Validator.voting_power",

proto/src/prost/tendermint.types.rs

@@ -75,7 +75,7 @@ pub struct Header {
     #[serde(with = "crate::serializers::from_str")]
     pub height: i64,
     #[prost(message, optional, tag="4")]
-    #[serde(with = "crate::serializers::option_timestamp")]
+    #[serde(with = "crate::serializers::optional")]
     pub time: ::std::option::Option<super::super::google::protobuf::Timestamp>,
     /// prev block info
     #[prost(message, optional, tag="5")]
@@ -155,7 +155,7 @@ pub struct Vote {
     #[prost(message, optional, tag="4")]
     pub block_id: ::std::option::Option<BlockId>,
     #[prost(message, optional, tag="5")]
-    #[serde(with = "crate::serializers::option_timestamp")]
+    #[serde(with = "crate::serializers::optional")]
     pub timestamp: ::std::option::Option<super::super::google::protobuf::Timestamp>,
     #[prost(bytes, tag="6")]
     #[serde(with = "crate::serializers::bytes::hexstring")]
@@ -197,7 +197,7 @@ pub struct CommitSig {
     #[serde(with = "crate::serializers::bytes::hexstring")]
     pub validator_address: std::vec::Vec<u8>,
     #[prost(message, optional, tag="3")]
-    #[serde(with = "crate::serializers::option_timestamp")]
+    #[serde(with = "crate::serializers::optional")]
     pub timestamp: ::std::option::Option<super::super::google::protobuf::Timestamp>,
     #[prost(bytes, tag="4")]
     #[serde(with = "crate::serializers::bytes::base64string")]

proto/src/protobuf.rs

@@ -17,6 +17,7 @@
 /// restricting to that range, we ensure that we can convert to and from [RFC
 /// 3339](https://www.ietf.org/rfc/rfc3339.txt) date strings.
 #[derive(Clone, PartialEq, ::prost::Message, ::serde::Deserialize, ::serde::Serialize)]
+#[serde(from = "crate::serializers::timestamp::Rfc3339", into = "crate::serializers::timestamp::Rfc3339")]
 pub struct Timestamp {
     /// Represents seconds of UTC time since Unix epoch
     /// 1970-01-01T00:00:00Z. Must be from 0001-01-01T00:00:00Z to

proto/src/serializers.rs

@@ -48,7 +48,7 @@ pub mod bytes;
 pub mod evidence;
 pub mod from_str;
 pub mod nullable;
-pub mod option_timestamp;
 pub mod optional;
 pub mod time_duration;
+pub mod timestamp;
 pub mod txs;

proto/src/serializers/nullable.rs

@@ -1,4 +1,4 @@
-//! Serialize/deserialize Option<T> type.
+//! Serialize/deserialize `nil`able type into T, where nil turns into the default impl.
 use serde::{Deserialize, Deserializer, Serialize, Serializer};
 
 /// Deserialize Option<T>

proto/src/serializers/option_timestamp.rs → proto/src/serializers/timestamp.rs

@@ -1,38 +1,52 @@
-//! Serialize/deserialize Option<Timestamp> type from and into string:
+//! Serialize/deserialize Timestamp type from and into string:
 use serde::{de::Error as _, Deserialize, Deserializer, Serialize, Serializer};
 
 use crate::google::protobuf::Timestamp;
 use chrono::{DateTime, Datelike, LocalResult, TimeZone, Timelike, Utc};
 use serde::ser::Error;
 
+/// Helper struct to serialize and deserialize Timestamp into an RFC3339-compatible string
+/// This is required because the serde `with` attribute is only available to fields of a struct but
+/// not the whole struct.
+#[derive(Serialize, Deserialize)]
+#[serde(transparent)]
+pub struct Rfc3339(#[serde(with = "crate::serializers::timestamp")] Timestamp);
+impl From<Timestamp> for Rfc3339 {
+    fn from(value: Timestamp) -> Self {
+        Rfc3339(value)
+    }
+}
+impl From<Rfc3339> for Timestamp {
+    fn from(value: Rfc3339) -> Self {
+        value.0
+    }
+}
+
 /// Deserialize string into Timestamp
-pub fn deserialize<'de, D>(deserializer: D) -> Result<Option<Timestamp>, D::Error>
+pub fn deserialize<'de, D>(deserializer: D) -> Result<Timestamp, D::Error>
 where
     D: Deserializer<'de>,
 {
     let value_string = String::deserialize(deserializer)?;
     let value_datetime = DateTime::parse_from_rfc3339(value_string.as_str())
         .map_err(|e| D::Error::custom(format!("{}", e)))?;
-    Ok(Some(Timestamp {
+    Ok(Timestamp {
         seconds: value_datetime.timestamp(),
         nanos: value_datetime.timestamp_subsec_nanos() as i32,
-    }))
+    })
 }
 
 /// Serialize from Timestamp into string
-pub fn serialize<S>(value: &Option<Timestamp>, serializer: S) -> Result<S::Ok, S::Error>
+pub fn serialize<S>(value: &Timestamp, serializer: S) -> Result<S::Ok, S::Error>
 where
     S: Serializer,
 {
-    let value = value
-        .as_ref()
-        .ok_or_else(|| S::Error::custom("no time found"))?;
     if value.nanos < 0 {
         return Err(S::Error::custom("invalid nanoseconds in time"));
     }
     match Utc.timestamp_opt(value.seconds, value.nanos as u32) {
         LocalResult::None => Err(S::Error::custom("invalid time")),
-        LocalResult::Single(t) => Ok(to_rfc3999(t)),
+        LocalResult::Single(t) => Ok(to_rfc3339_custom(t)),
         LocalResult::Ambiguous(_, _) => Err(S::Error::custom("ambiguous time")),
     }?
     .serialize(serializer)
@@ -41,7 +55,7 @@ where
 // Due to incompatibilities between the way that `chrono` serializes timestamps
 // and the way that Go does for RFC3339, we unfortunately need to define our
 // own timestamp serialization mechanism.
-fn to_rfc3999(t: DateTime<Utc>) -> String {
+fn to_rfc3339_custom(t: DateTime<Utc>) -> String {
     let nanos = format!(".{}", t.nanosecond());
     format!(
         "{:04}-{:02}-{:02}T{:02}:{:02}:{:02}{}Z",
@@ -95,8 +109,7 @@ mod test {
     fn json_timestamp_precision() {
         #[derive(Serialize, Deserialize)]
         struct TimestampWrapper {
-            #[serde(with = "crate::serializers::option_timestamp")]
-            timestamp: Option<Timestamp>,
+            timestamp: Timestamp,
         }
         let test_timestamps = vec![
             "2020-09-14T16:33:54.21191421Z",

rpc/src/endpoint/net_info.rs

@@ -92,7 +92,7 @@ pub struct Monitor {
     pub active: bool,
 
     /// When the monitor started
-    #[serde(rename = "Start", with = "serializers::time")]
+    #[serde(rename = "Start")]
     pub start: Time,
 
     /// Duration of this monitor

rpc/src/endpoint/status.rs

@@ -46,7 +46,6 @@ pub struct SyncInfo {
     pub latest_block_height: block::Height,
 
     /// Latest block time
-    #[serde(with = "tendermint::serializers::time")]
     pub latest_block_time: Time,
 
     /// Are we catching up?

tendermint/src/genesis.rs

@@ -11,7 +11,6 @@ use tendermint_proto::google::protobuf::Timestamp;
 #[derive(Clone, Debug, Serialize, Deserialize)]
 pub struct Genesis<AppState = serde_json::Value> {
     /// Time of genesis
-    #[serde(with = "crate::serializers::time")]
     pub genesis_time: Time,
 
     /// Chain ID

tendermint/src/serializers.rs

@@ -10,4 +10,3 @@ pub use tendermint_proto::serializers::*;
 pub mod apphash;
 pub mod hash;
 pub mod option_hash;
-pub mod time;