Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Replace chrono with time 0.3 #1030

Merged
merged 32 commits into from
Dec 7, 2021
Merged

Replace chrono with time 0.3 #1030

merged 32 commits into from
Dec 7, 2021

Conversation

mzabaluev
Copy link
Contributor

@mzabaluev mzabaluev commented Nov 26, 2021

Fixes: #1012, #1007, #1008

Important or breaking changes:

  • Removed chrono from dependencies and public API, replaced by time.

  • The inner value of the Time tuple struct is made private.

  • Renamed Time::as_rfc3339 to to_rfc3339 to follow Rust naming conventions (somehow clippy got it wrong at the time of Fix latest clippy assertion failures #910, but it no longer does).

  • Instead of From<chrono::DateTime<Utc>>, Time now implements TryFrom<time::OffsetDateTime>. The reason is that OffsetDateTime supports a wider range of dates than is supported by RFC 3339, and tendermint::Time needs its string conversion to be standard-compliant and infallible.

  • Added Time methods checked_add/checked_sub, to follow the pattern set by SystemTime and time::OffsetDateTime. In a later change, these could replace use of the Add/Sub operator impls, whose Result output types make for poor ergonomics in operator overloading.

  • rpc uses time types OffsetDateTime and Date in query operands instead of their chrono counterparts.

  • Referenced an issue explaining the need for the change

  • Updated all relevant documentation in docs

  • Updated all code comments where relevant

  • Wrote tests

  • Added entry in .changelog/

chrono has soundness issues (see RUSTSEC-2020-0159) and does not
seem to be maintained.
Change Time implementation to crate time: chrono has soundness issues
(see RUSTSEC-2020-0159) and does not seem to be actively maintained.
These should be used instead of the overloaded operators that broke
the operator convention by returning a Result.
Drop the "formatting" feature of time, as this brings in std.
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.
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.
Provide and use another helper in proto mod serializers::timestamp,
one that formats into a provided fmt::Write object rather than
allocating a string.
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.
@mzabaluev mzabaluev added dependencies Pull requests that update a dependency file serialization breaking code-quality Issues relating to linting configuration and general code quality labels Nov 26, 2021
@codecov-commenter
Copy link

codecov-commenter commented Nov 26, 2021

Codecov Report

Merging #1030 (f5e6244) into master (0b52add) will increase coverage by 0.3%.
The diff coverage is 94.5%.

Impacted file tree graph

@@           Coverage Diff            @@
##           master   #1030     +/-   ##
========================================
+ Coverage    62.0%   62.4%   +0.3%     
========================================
  Files         237     236      -1     
  Lines       21169   21352    +183     
========================================
+ Hits        13143   13339    +196     
+ Misses       8026    8013     -13     
Impacted Files Coverage Δ
light-client/src/components/clock.rs 0.0% <0.0%> (ø)
proto/src/lib.rs 100.0% <ø> (ø)
tendermint/src/abci/request/init_chain.rs 0.0% <ø> (ø)
tendermint/src/abci/types.rs 0.0% <0.0%> (ø)
tendermint/src/error.rs 0.0% <0.0%> (ø)
tendermint/src/serializers/time.rs 53.8% <0.0%> (ø)
testgen/src/lib.rs 100.0% <ø> (ø)
testgen/src/header.rs 80.9% <66.6%> (ø)
pbt-gen/src/time.rs 94.9% <97.7%> (-5.1%) ⬇️
proto/src/serializers/timestamp.rs 98.0% <98.2%> (+2.8%) ⬆️
... and 18 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 0b52add...f5e6244. Read the comment docs.

The clock needs OffsetDateTime::now_utc().
@thanethomson
Copy link
Contributor

I'd recommend targeting the v0.23.x branch with this PR for now, since I assume you want this change to make its way into ibc-rs soon?

I was planning on merging #1022 ASAP, then we'll need to backport these changes to master.

@mzabaluev
Copy link
Contributor Author

I assume you want this change to make its way into ibc-rs soon?

No, we only really need it to fix the "broken window" of audit failures. Also, it's a breaking change, so it should properly be included starting from 0.24.x (assuming this project follows cargo's stricter semver rules).

@thanethomson
Copy link
Contributor

We're actually going to allow for breaking changes to the v0.23.x series from one "patch" version to the next. I should add a note that people need to pin a specific version of tendermint-rs in their dependencies.

This is one of the side effects of the versioning strategy we're going with.

Since we're also pre-1.0, I think there's still some leniency here.

So I'd still recommend merging this into v0.23.x and then I can backport the changes once #1022's been merged. I really want to get that PR out of the way without more conflicts, as the work's been ongoing for 7 months now and it's time to push it over the finish line 😅

@thanethomson thanethomson mentioned this pull request Nov 26, 2021
6 tasks
@@ -1,53 +0,0 @@
use core::convert::TryInto;

use chrono::{DateTime, Duration, TimeZone, Utc};
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The whole module is removed, rather than being replaced with time equivalents, because of the problems detailed below. Conversions from/to timestamp::Time can serve as domain-validating, fallible alternatives, until more direct convenience methods are reintroduced here if needed.

fn from(dt: DateTime<Utc>) -> pb::Timestamp {
pb::Timestamp {
seconds: dt.timestamp(),
// This can exceed 1_000_000_000 in the case of a leap second, but
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was against the wire protocol specification.


impl From<pb::Timestamp> for DateTime<Utc> {
fn from(ts: pb::Timestamp) -> DateTime<Utc> {
Utc.timestamp(ts.seconds, ts.nanos as u32)
Copy link
Contributor Author

@mzabaluev mzabaluev Nov 30, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This panicked if members of ts have invalid values (subject to chrono's interpretation which does not readily conform to the protobuf specification).

As the most likely origin of a pb::Timestamp is by having been parsed from an incoming protobuf message by prost without any validation as to its contents, this is an easy DoS bomb under any application developers who would make use of this conversion. Please consider using fallible conversions when dealing with untrusted values originating from the network.

Comment on lines -37 to -42
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");
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This could produce protobuf messages that violate the specification.

The whole approach of having infallible conversions that can only either panic or produce off-spec values for the wire protocol in case the destination's validity requirements are not met, needs to be reconsidered.

impl From<DateTime<Utc>> for pb::Timestamp {
fn from(dt: DateTime<Utc>) -> pb::Timestamp {
pb::Timestamp {
seconds: dt.timestamp(),
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The timestamp's range as represented by chrono::DateTime can exceed the range valid for the protobuf Timestamp messages.

Comment on lines -49 to -52
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)
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is actually possible to replace with an infallible time equivalent (and time does have a one-stop shop constructor for that!), but it felt odd to leave a lone conversion standing when others were sent back to the drawing board.

We should consider introducing a newtype for durations in the tendermint API, with similar range constraint enforcement as maintained in this PR for Time.

@mzabaluev
Copy link
Contributor Author

being able to carry along an additional time zone other than UTC is a misfeature.

This reimplementation does not do so; the inner value is a PrimitiveDateTime, with .assume_utc() used to get the operational OffsetDateTime value where needed.

I don't agree that time API is simpler. It has no way to encode the timezone in the date-time type (with the exception of the oddly-named OffsetDateTime, which hardcodes UTC), so the user has to keep track on their own, and cannot rely on the type system for assurance.

This is clever, but the type parameter of DateTime is also one example of added complexity that not every application would necessarily need. OffsetDateTime does not hardcode UTC, it supports any possible offset dynamically; the closest equivalent in chrono is DateTime<FixedOffset>. tendermint::Time takes the job of normalizing to UTC, among other value sanity checks that are necessary for a valid Tendermint timestamp.

An instance of unwanted complexity that bugs me more about chrono is its handling of leap seconds. It looks like a recipe for a wide variety of bugs, and it's not compatible with Google's definition of a protobuf timestamp, which specifically avoids dealing with leap second times. It's certainly not something I'd feel comfortable basing the protocol library's support for time representation on.

I also don't agree that there is a real soundness issue here. Yes, the OS can cause a segfault if you use it wrong. But there are plenty of other ways that I can cause similar "soundness" issues from safe Rust -- for instance, opening up /proc/mem and writing into it.

That requires actual malicious intent. By contrast, std::env::set_var is a Rust function declared as safe, with only a vague mention of possible induced UB when also using FFI functions, which chrono does not give a hint about doing. It's very easy for a newbie to slip in an environment variable modification unaware of the consequences. That, of course, should also be linted against and warrants a security audit notice when found in a published library.

Are there really many other easy ways to violate soundness from safe Rust? Are we living in an illusion of safety?

Hiding the inner value of Time would help sidestep the debate here; we could feature-gate conversion impls from/to both chrono and time types so you could use whichever you prefer.

This option puts extra configuration burden on the library

There is already a number of dependencies managed with feature gates, so this seems like more of the same.

and forces every user to jump through hoops to get a workable API, which seems like a movement in the opposite direction from the simplification I suggested in #865.

As long as we don't have any other support for:

  • the range and validity constraints imposed by RFC 3339 and the specification of protobuf well-known types;
  • the specific details of RFC 3339 formatting to conform with the Go implementation;
  • implementing the above directly on the DTO types used by prost/tonic;

I think it's necessary to have a newtype with fallible conversions here.

@mzabaluev
Copy link
Contributor Author

Not sure if time fares any better here offhand.

I managed to get the desired behaviour out of it, but unfortunately that currently requires std: time-rs/time#400

@mzabaluev
Copy link
Contributor Author

@thanethomson

I'd recommend targeting the v0.23.x branch with this PR for now, since I assume you want this change to make its way into ibc-rs soon?

That's #1036 now.

I was planning on merging #1022 ASAP, then we'll need to backport these changes to master.

Also done in follow-up commits here.

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.
Add a unit test to exercise the check.
- More ergonomic signature
- A non-allocating implementation
@soareschen
Copy link
Contributor

Can we have a parallel branch in ibc-rs that fix the breaking changes from this branch?

Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a few minor nits and some questions. Otherwise looks great! 🎉

All my questions/comments apply to #1036 as well, where relevant.

tendermint/src/time.rs Outdated Show resolved Hide resolved
tendermint/src/time.rs Outdated Show resolved Hide resolved
}
}

impl Time {
fn from_utc(t: OffsetDateTime) -> Result<Self, Error> {
debug_assert_eq!(t.offset(), offset!(UTC));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there any reason to reserve this check only for debug builds?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's an internal method, all call sites should either produce a value with the UTC offset or convert to it.
The debug check is to catch if a programmer forgot.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have added a comment on the function.

Comment on lines +109 to +114
let mut secfrac = nanos;
let mut secfrac_width = 9;
while secfrac % 10 == 0 {
secfrac /= 10;
secfrac_width -= 1;
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some back-of-the-envelope calculations so far indicate to me that this works for formatting. Neat little algorithm 🙂 Would it not perhaps be worthwhile to add some test cases to test this fmt_as_rfc3339_nanos method out? Or is the best place to test this in the property-based tests for the tendermint::Time struct?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The existing tests exercise to_rfc3339 which uses this method, and they caught me out when I did it wrong, so I assume it is covered.

tendermint/src/time.rs Outdated Show resolved Hide resolved
tendermint/src/time.rs Outdated Show resolved Hide resolved
tendermint/src/time.rs Outdated Show resolved Hide resolved
tendermint/src/time.rs Show resolved Hide resolved
testgen/Cargo.toml Outdated Show resolved Hide resolved
@mzabaluev
Copy link
Contributor Author

Clippy failures should be fixed by #1041.

Copy link
Contributor

@thanethomson thanethomson left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great, I think we're good to go here. Thanks @mzabaluev!

@thanethomson thanethomson merged commit 3381ea7 into master Dec 7, 2021
@thanethomson thanethomson deleted the mikhail/remove-chrono branch December 7, 2021 16:59
hdevalence pushed a commit to penumbra-zone/tendermint-rs that referenced this pull request Aug 18, 2022
Replace chrono with time 0.3 (informalsystems#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 informalsystems#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 <[email protected]>

* time: Use Self instead of the type name in methods

Co-authored-by: Thane Thomson <[email protected]>

* 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 <[email protected]>
hdevalence pushed a commit to penumbra-zone/tendermint-rs that referenced this pull request Sep 20, 2022
Replace chrono with time 0.3 (informalsystems#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 informalsystems#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 <[email protected]>

* time: Use Self instead of the type name in methods

Co-authored-by: Thane Thomson <[email protected]>

* 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 <[email protected]>
hdevalence pushed a commit to penumbra-zone/tendermint-rs that referenced this pull request Sep 20, 2022
Replace chrono with time 0.3 (informalsystems#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 informalsystems#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 <[email protected]>

* time: Use Self instead of the type name in methods

Co-authored-by: Thane Thomson <[email protected]>

* 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 <[email protected]>
hdevalence pushed a commit to penumbra-zone/tendermint-rs that referenced this pull request Sep 20, 2022
Replace chrono with time 0.3 (informalsystems#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 informalsystems#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 <[email protected]>

* time: Use Self instead of the type name in methods

Co-authored-by: Thane Thomson <[email protected]>

* 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 <[email protected]>
hdevalence pushed a commit to penumbra-zone/tendermint-rs that referenced this pull request Sep 20, 2022
Replace chrono with time 0.3 (informalsystems#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 informalsystems#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 <[email protected]>

* time: Use Self instead of the type name in methods

Co-authored-by: Thane Thomson <[email protected]>

* 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 <[email protected]>
mzabaluev added a commit that referenced this pull request Sep 28, 2022
* split A.4.1, proto tooling changes

* split A.2, changelog abci

* split A.1, tendermint and abci changes

* split A.3, abci-in-rpc

* clean up imports in abci crate

* Return to 0.34 ABCI encoding

* 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 <[email protected]>

* time: Use Self instead of the type name in methods

Co-authored-by: Thane Thomson <[email protected]>

* 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 <[email protected]>

* fixup! split A.3, abci-in-rpc

* split A.4.1.1, check_event_attrs

* split A.4.1.2, some slice change?

* split A.4.1.3, b.data.get(0).is_none()

* split A.4.1.4, remove test attrs

* split A.4.1.5, someting optiony

* clippy fix

* Remove unnessarily generated files

* Fix deliver_tx.events KV tests

* Remove Option wart in net_info::Response

* rpc: Revert Event member events to a KV map

The event list change did not make it into tendermint 0.37.

* Revert tests to checking decoded KVs

* rpc: Restore base64 serialization of event KV tags

* Post-merge fixes for rpc tests

* rpc: Use the re-exported serializers mod

* Remove duplicate changelog entries

* Fix kvstore-test build

* Changelog entry for switching to Bytes

* Remove a commented out line

* abci: Restore Client::set_option as deprecated

* Update abci/src/client.rs

Co-authored-by: Henry de Valence <[email protected]>
Co-authored-by: Thane Thomson <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking code-quality Issues relating to linting configuration and general code quality dependencies Pull requests that update a dependency file serialization
Projects
None yet
6 participants