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

Add a DateTime32 type for 32-bit serialized times #2210

Merged
merged 6 commits into from
May 31, 2021

Conversation

teor2345
Copy link
Contributor

@teor2345 teor2345 commented May 27, 2021

Motivation

In #2178, we want to modify the last seen times gossiped by peers.

But the chrono::DateTime<Utc> type that we're using has a ~56 bit range, so we can easily generate times that don't serialize when we go to send them to peers.

This breaks one of Zebra's design principles: invalid data should be unrepresentable.

Solution

Implement part of #2171:

  • add a DateTime32 type
  • use it for MetaAddr.last_seen

The code in this pull request has:

  • Documentation Comments
  • Existing Unit Tests and Property Tests

Review

@jvff can review this PR, it helps simplify #2178.

Follow Up Work

Replace Zebra's other 32-bit times with DateTime32 #2211

  • these times are lower risk, because we don't modify or generate them yet

@teor2345 teor2345 added A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup P-Medium A-network Area: Network protocol updates or fixes labels May 27, 2021
@teor2345 teor2345 added this to the 2021 Sprint 10 milestone May 27, 2021
@teor2345 teor2345 requested a review from jvff May 27, 2021 04:26
@teor2345 teor2345 self-assigned this May 27, 2021
@teor2345 teor2345 added C-bug Category: This is a bug and removed C-enhancement Category: This is an improvement labels May 27, 2021
@mpguerra mpguerra removed this from the 2021 Sprint 10 milestone May 27, 2021
@teor2345 teor2345 added this to the 2021 Sprint 10 milestone May 27, 2021
@mpguerra mpguerra removed this from the 2021 Sprint 10 milestone May 27, 2021
jvff
jvff previously approved these changes May 27, 2021
Copy link
Contributor

@jvff jvff left a comment

Choose a reason for hiding this comment

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

This looks great! I added two minor suggestions, but they are optional.

One question though, would it make sense to also add Add and Sub implementations? I can do that in a separate PR if you want 👍

zebra-chain/src/serialization/date_time.rs Outdated Show resolved Hide resolved
zebra-network/src/meta_addr.rs Outdated Show resolved Hide resolved
Copy link
Contributor

@dconnolly dconnolly left a comment

Choose a reason for hiding this comment

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

Made some suggestions per @jvff's comment but otherwise looking good 😎

@teor2345
Copy link
Contributor Author

One question though, would it make sense to also add Add and Sub implementations? I can do that in a separate PR if you want 👍

I'd prefer to do the calculations in chrono::DateTime (~56 bits) or i64. Then we don't have to worry about overflow until we convert back to DateTime32.

Also the chrono::DateTime API is a lot nicer, it has functions for minutes, hours, etc.

@teor2345 teor2345 merged commit ebe1c9f into ZcashFoundation:main May 31, 2021
@teor2345
Copy link
Contributor Author

Looks like Deirdre is working on other things right now, so I went ahead and merged this PR so we can make progress.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-consensus Area: Consensus rule updates A-network Area: Network protocol updates or fixes A-rust Area: Updates to Rust code C-bug Category: This is a bug C-cleanup Category: This is a cleanup
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants