From b699af91c934990afc919fe317718688cdf65362 Mon Sep 17 00:00:00 2001 From: Mikhail Zabaluev Date: Tue, 7 Dec 2021 18:59:00 +0200 Subject: [PATCH] Cherry-pick chrono changes Replace chrono with time 0.3 (#1030) * pbt-gen: Converted from chrono to time 0.3 chrono has soundness issues (see RUSTSEC-2020-0159) and does not seem to be maintained. * Replace dependency on chrono with time 0.3 Change Time implementation to crate time: chrono has soundness issues (see RUSTSEC-2020-0159) and does not seem to be actively maintained. * Add Time methods checked_add and checked_sub These should be used instead of the overloaded operators that broke the operator convention by returning a Result. * proto: Don't use formatting methods of time Drop the "formatting" feature of time, as this brings in std. * pbt-gen: Add arb_datetime_for_rfc3339 With crate time, the range of supported dates stretches to the ancient past beyond the common era, which is not representable in RFC 3339. Add a generator to produce `OffsetDateTime` values that are within the RFC 3339 spec. * Ensure Time can only have values good for RFC 3339 As the time values are meant for human-readable representation in the RFC 3339 format, make it not possible to construct Time with values that fall outside this standard representation. Conversion from time::OffsetDateTime is made fallible with a TryFrom impl replacing the From impl. Conversion from Unix timestamps is also affected. * rpc: Replaced chrono with time 0.3 * testgen: Replaced chrono with time 0.3 * Less allocatey ways of formatting date-times Provide and use another helper in proto mod serializers::timestamp, one that formats into a provided fmt::Write object rather than allocating a string. * light-client: port from chrono to time * Revert to Add/Sub returning Result * light-client: changed the MBTs from chrono to time * tendermint: Remove a comment referencing chrono We use ErrorDetail::DurationOutOfRange without the source error from the time library because that is uninformative in the context. Also move the DateOutOfRange close with DurationOutOfRange since they can occur in the same operations and are generally similar. * light-client: add std feature for time The clock needs OffsetDateTime::now_utc(). * tendermint: Simplify Time::duration_since * testgen: minor code cleanup * Restrict valid Time years to 1-9999 Exclude year 0 because the spec on Google protobuf Timestamp forbids it. Add more helpers in pbt-gen to produce protobuf-safe values in both OffsetDateTime and RFC 3339 strings. Restrict the property tests to only use the protobuf-safe values where expected. * Test Time::checked_add and Time::checked_sub * Changelog entries for #1030 * Improve documentation of tendermint::Time * proto: remove the chrono type conversions The From impls are panicky and do not enforce compliance with protobuf message value restrictions. * tendermint: remove direct uses of chrono types Replace with tendermint::Time. * Harden Timestamp conversions and serde Require the timestamp to be in the validity range (years 1-9999 in UTC) and the nanosecond member value to not exceed 999_999_999. Rename ErrorDetail::TimestampOverflow to TimestampNanosOutOfRange, because the old variant was not very informative (the chained TryFromIntError did not help) and we also use it for the above-range case now which is not an overflow. * Changelog entry about changed error variants * Restore nanosecond range check in Time::from_unix_timestamp Add a unit test to exercise the check. * proto: Improve timestamp::fmt_as_rfc3339_nanos - More ergonomic signature - A non-allocating implementation * Fix component name in changelog for 1030-remove-chrono Co-authored-by: Thane Thomson * time: Use Self instead of the type name in methods Co-authored-by: Thane Thomson * Comment on the inner representation of `Time` * Don't alias crate time in testgen * Document the Time::from_utc helper Co-authored-by: Thane Thomson --- .../breaking-changes/1030-remove-chrono.md | 11 ++++ .../improvements/1030-new-time-api.md | 11 ++++ abci/src/codec.rs | 2 +- proto/Cargo.toml | 2 - proto/src/chrono.rs | 53 --------------- proto/src/lib.rs | 1 - tendermint/Cargo.toml | 4 +- tendermint/src/abci/request/init_chain.rs | 5 +- tendermint/src/abci/types.rs | 10 +-- tendermint/src/time.rs | 66 +++++++++---------- 10 files changed, 65 insertions(+), 100 deletions(-) create mode 100644 .changelog/unreleased/breaking-changes/1030-remove-chrono.md create mode 100644 .changelog/unreleased/improvements/1030-new-time-api.md delete mode 100644 proto/src/chrono.rs diff --git a/.changelog/unreleased/breaking-changes/1030-remove-chrono.md b/.changelog/unreleased/breaking-changes/1030-remove-chrono.md new file mode 100644 index 000000000..7d8385ee5 --- /dev/null +++ b/.changelog/unreleased/breaking-changes/1030-remove-chrono.md @@ -0,0 +1,11 @@ +- `[tendermint]` Reform `tendermint::Time` + ([#1030](https://github.com/informalsystems/tendermint-rs/issues/1030)): + * The struct content is made private. + * The range of acceptable values is restricted to years 1-9999 + (as reckoned in UTC). + * Removed conversions from/to `chrono::DateTime`. + * Changes in error variants: removed `TimestampOverflow`, replaced with + `TimestampNanosOutOfRange`; removed `ChronoParse`, replaced with `TimeParse`. +- `[tendermint-rpc]` Use `OffsetDateTime` and `Date` types provided by the `time` crate + in query operands instead of their `chrono` counterparts. + ([#1030](https://github.com/informalsystems/tendermint-rs/issues/1030)) diff --git a/.changelog/unreleased/improvements/1030-new-time-api.md b/.changelog/unreleased/improvements/1030-new-time-api.md new file mode 100644 index 000000000..a9c269f0d --- /dev/null +++ b/.changelog/unreleased/improvements/1030-new-time-api.md @@ -0,0 +1,11 @@ +- Remove dependencies on the `chrono` crate. + ([#1030](https://github.com/informalsystems/tendermint-rs/issues/1030)) +- `[tendermint]` Improve `tendermint::Time` + ([#1030](https://github.com/informalsystems/tendermint-rs/issues/1030)): + * Restrict the validity range of `Time` to dates with years in the range + 1-9999, to match the specification of protobuf message `Timestamp`. + Add an `ErrorDetail` variant `DateOutOfRange` to report when this + restriction is not met. + * Added a conversion to, and a fallible conversion from, + `OffsetDateTime` of the `time` crate. + * Added `Time` methods `checked_add` and `checked_sub`. diff --git a/abci/src/codec.rs b/abci/src/codec.rs index 8e80736dd..d9fd649c6 100644 --- a/abci/src/codec.rs +++ b/abci/src/codec.rs @@ -146,7 +146,7 @@ where Ok(len) => len, // We've potentially only received a partial length delimiter Err(_) if src_len <= MAX_VARINT_LENGTH => return Ok(None), - //Err(e) => return Err(Error::decode(e)), + // Err(e) => return Err(Error::decode(e)), Err(e) => return Err(e), }; let remaining = tmp.remaining() as u64; diff --git a/proto/Cargo.toml b/proto/Cargo.toml index afd8e8023..aab957435 100644 --- a/proto/Cargo.toml +++ b/proto/Cargo.toml @@ -28,8 +28,6 @@ num-derive = { version = "0.3", default-features = false } # TODO(thane): Remove restrictions once js-sys issue is resolved (causes the Substrate no_std check to fail) time = { version = ">=0.3, <0.3.12", default-features = false, features = ["macros", "parsing"] } flex-error = { version = "0.4.4", default-features = false } -# TODO(hdevalence): re-added while rebasing the ABCI domain types PR again -chrono = { version = "0.4", default-features = false, features = ["serde", "alloc"] } [dev-dependencies] serde_json = { version = "1.0", default-features = false, features = ["alloc"] } diff --git a/proto/src/chrono.rs b/proto/src/chrono.rs deleted file mode 100644 index 3b969f0f0..000000000 --- a/proto/src/chrono.rs +++ /dev/null @@ -1,53 +0,0 @@ -use core::convert::TryInto; - -use chrono::{DateTime, Duration, TimeZone, Utc}; - -use crate::google::protobuf as pb; - -impl From> for pb::Timestamp { - fn from(dt: DateTime) -> pb::Timestamp { - pb::Timestamp { - seconds: dt.timestamp(), - // This can exceed 1_000_000_000 in the case of a leap second, but - // even with a leap second it should be under 2_147_483_647. - nanos: dt - .timestamp_subsec_nanos() - .try_into() - .expect("timestamp_subsec_nanos bigger than i32::MAX"), - } - } -} - -impl From for DateTime { - fn from(ts: pb::Timestamp) -> DateTime { - Utc.timestamp(ts.seconds, ts.nanos as u32) - } -} - -// Note: we convert a protobuf::Duration into a chrono::Duration, not a -// std::time::Duration, because std::time::Durations are unsigned, but the -// protobuf duration is signed. - -impl From for pb::Duration { - fn from(d: Duration) -> pb::Duration { - // chrono's Duration stores the fractional part as `nanos: i32` - // internally but doesn't provide a way to access it, only a way to get - // the *total* number of nanoseconds. so we have to do this cool and fun - // hoop-jumping maneuver - let seconds = d.num_seconds(); - let nanos = (d - Duration::seconds(seconds)) - .num_nanoseconds() - .expect("we computed the fractional part, so there's no overflow") - .try_into() - .expect("the fractional part fits in i32"); - - pb::Duration { seconds, nanos } - } -} - -impl From for Duration { - fn from(d: pb::Duration) -> Duration { - // there's no constructor that supplies both at once - Duration::seconds(d.seconds) + Duration::nanoseconds(d.nanos as i64) - } -} diff --git a/proto/src/lib.rs b/proto/src/lib.rs index 312403de6..49e41fde6 100644 --- a/proto/src/lib.rs +++ b/proto/src/lib.rs @@ -19,7 +19,6 @@ pub mod google { } } -mod chrono; mod error; #[allow(warnings)] mod tendermint; diff --git a/tendermint/Cargo.toml b/tendermint/Cargo.toml index cc50f5cbe..cb4fe8046 100644 --- a/tendermint/Cargo.toml +++ b/tendermint/Cargo.toml @@ -31,7 +31,7 @@ rustdoc-args = ["--cfg", "docsrs"] [dependencies] async-trait = { version = "0.1", default-features = false } -bytes = { version = "1.0", default-features = false } +bytes = { version = "1.0", default-features = false, features = ["serde"] } ed25519 = { version = "1.3", default-features = false } ed25519-dalek = { version = "1", default-features = false, features = ["u64_backend"] } futures = { version = "0.3", default-features = false } @@ -54,8 +54,6 @@ zeroize = { version = "1.1", default-features = false, features = ["zeroize_deri flex-error = { version = "0.4.4", default-features = false } k256 = { version = "0.11", optional = true, default-features = false, features = ["ecdsa", "sha256"] } ripemd160 = { version = "0.9", default-features = false, optional = true } -# TODO(hdevalence): re-added while rebasing the ABCI domain types PR again -chrono = { version = "0.4", default-features = false, features = ["serde", "alloc"] } [features] default = ["std"] diff --git a/tendermint/src/abci/request/init_chain.rs b/tendermint/src/abci/request/init_chain.rs index 8072c2f6e..bb50b553f 100644 --- a/tendermint/src/abci/request/init_chain.rs +++ b/tendermint/src/abci/request/init_chain.rs @@ -1,8 +1,7 @@ use bytes::Bytes; -use chrono::{DateTime, Utc}; use super::super::types::ValidatorUpdate; -use crate::{block, consensus, prelude::*}; +use crate::{block, consensus, prelude::*, Time}; /// Called on genesis to initialize chain state. /// @@ -10,7 +9,7 @@ use crate::{block, consensus, prelude::*}; #[derive(Clone, PartialEq, Eq, Debug)] pub struct InitChain { /// The genesis time. - pub time: DateTime, + pub time: Time, /// The ID of the blockchain. pub chain_id: String, /// Initial consensus-critical parameters. diff --git a/tendermint/src/abci/types.rs b/tendermint/src/abci/types.rs index 4df1a95e7..54344d692 100644 --- a/tendermint/src/abci/types.rs +++ b/tendermint/src/abci/types.rs @@ -8,9 +8,8 @@ use core::convert::{TryFrom, TryInto}; use bytes::Bytes; -use chrono::{DateTime, Utc}; -use crate::{block, prelude::*, vote, Error, PublicKey}; +use crate::{block, prelude::*, vote, Error, PublicKey, Time}; /// A validator address with voting power. /// @@ -80,7 +79,7 @@ pub struct Evidence { /// The height when the offense occurred. pub height: block::Height, /// The corresponding time when the offense occurred. - pub time: DateTime, + pub time: Time, /// Total voting power of the validator set at `height`. /// /// This is included in case the ABCI application does not store historical @@ -246,7 +245,10 @@ impl TryFrom for Evidence { .ok_or_else(Error::missing_validator)? .try_into()?, height: evidence.height.try_into()?, - time: evidence.time.ok_or_else(Error::missing_timestamp)?.into(), + time: evidence + .time + .ok_or_else(Error::missing_timestamp)? + .try_into()?, total_voting_power: evidence.total_voting_power.try_into()?, }) } diff --git a/tendermint/src/time.rs b/tendermint/src/time.rs index 00114efb0..0cb6e330f 100644 --- a/tendermint/src/time.rs +++ b/tendermint/src/time.rs @@ -34,7 +34,7 @@ use crate::{error::Error, prelude::*}; // For memory efficiency, the inner member is `PrimitiveDateTime`, with assumed // UTC offset. The `assume_utc` method is used to get the operational // `OffsetDateTime` value. -#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Hash, Serialize, Deserialize)] +#[derive(Copy, Clone, Debug, Eq, PartialEq, PartialOrd, Ord, Serialize, Deserialize)] #[serde(try_from = "Timestamp", into = "Timestamp")] pub struct Time(PrimitiveDateTime); @@ -117,6 +117,20 @@ impl Time { pub fn to_rfc3339(&self) -> String { timestamp::to_rfc3339_nanos(self.0.assume_utc()) } + + /// Computes `self + duration`, returning `None` if an overflow occurred. + pub fn checked_add(self, duration: Duration) -> Option { + let duration = duration.try_into().ok()?; + let t = self.0.checked_add(duration)?; + Self::from_utc(t.assume_utc()).ok() + } + + /// Computes `self - duration`, returning `None` if an overflow occurred. + pub fn checked_sub(self, duration: Duration) -> Option { + let duration = duration.try_into().ok()?; + let t = self.0.checked_sub(duration)?; + Self::from_utc(t.assume_utc()).ok() + } } impl fmt::Display for Time { @@ -151,19 +165,12 @@ impl Add for Time { type Output = Result; fn add(self, rhs: Duration) -> Self::Output { - // Work around not being able to depend on time 0.3.5 - // https://github.com/informalsystems/tendermint-rs/issues/1047 - let lhs_nanos = self.0.assume_utc().unix_timestamp_nanos(); - let rhs_nanos: i128 = rhs - .as_nanos() - .try_into() - .map_err(|_| Error::duration_out_of_range())?; - let res_nanos = lhs_nanos - .checked_add(rhs_nanos) + let duration = rhs.try_into().map_err(|_| Error::duration_out_of_range())?; + let t = self + .0 + .checked_add(duration) .ok_or_else(Error::duration_out_of_range)?; - let t = OffsetDateTime::from_unix_timestamp_nanos(res_nanos) - .map_err(|_| Error::duration_out_of_range())?; - Self::from_utc(t) + Self::from_utc(t.assume_utc()) } } @@ -171,19 +178,12 @@ impl Sub for Time { type Output = Result; fn sub(self, rhs: Duration) -> Self::Output { - // Work around not being able to depend on time 0.3.5 - // https://github.com/informalsystems/tendermint-rs/issues/1047 - let lhs_nanos = self.0.assume_utc().unix_timestamp_nanos(); - let rhs_nanos: i128 = rhs - .as_nanos() - .try_into() - .map_err(|_| Error::duration_out_of_range())?; - let res_nanos = lhs_nanos - .checked_sub(rhs_nanos) + let duration = rhs.try_into().map_err(|_| Error::duration_out_of_range())?; + let t = self + .0 + .checked_sub(duration) .ok_or_else(Error::duration_out_of_range)?; - let t = OffsetDateTime::from_unix_timestamp_nanos(res_nanos) - .map_err(|_| Error::duration_out_of_range())?; - Self::from_utc(t) + Self::from_utc(t.assume_utc()) } } @@ -376,31 +376,31 @@ mod tests { proptest! { #[test] - fn add_regular((dt, d) in args_for_regular_add()) { + fn checked_add_regular((dt, d) in args_for_regular_add()) { let t: Time = dt.try_into().unwrap(); - let t = (t + d).unwrap(); + let t = t.checked_add(d).unwrap(); let res: OffsetDateTime = t.into(); assert_eq!(res, dt + d); } #[test] - fn sub_regular((dt, d) in args_for_regular_sub()) { + fn checked_sub_regular((dt, d) in args_for_regular_sub()) { let t: Time = dt.try_into().unwrap(); - let t = (t - d).unwrap(); + let t = t.checked_sub(d).unwrap(); let res: OffsetDateTime = t.into(); assert_eq!(res, dt - d); } #[test] - fn add_overflow((dt, d) in args_for_overflowed_add()) { + fn checked_add_overflow((dt, d) in args_for_overflowed_add()) { let t: Time = dt.try_into().unwrap(); - assert!((t + d).is_err()); + assert_eq!(t.checked_add(d), None); } #[test] - fn sub_overflow((dt, d) in args_for_overflowed_sub()) { + fn checked_sub_overflow((dt, d) in args_for_overflowed_sub()) { let t: Time = dt.try_into().unwrap(); - assert!((t - d).is_err()); + assert_eq!(t.checked_sub(d), None); } } }