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

Clearly identify 32-bit and 64-bit times in serialization and proptests #2171

Closed
1 of 8 tasks
Tracked by #2311
teor2345 opened this issue May 20, 2021 · 3 comments
Closed
1 of 8 tasks
Tracked by #2311
Labels
A-consensus Area: Consensus rule updates A-network Area: Network protocol updates or fixes C-security Category: Security issues I-consensus Zebra breaks a Zcash consensus rule I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data I-panic Zebra panics with an internal error message

Comments

@teor2345
Copy link
Contributor

teor2345 commented May 20, 2021

Scheduling

This risk is acceptable for the stable release, but we need to fix it before we support lightwalletd.

Is your feature request related to a problem? Please describe.

Zcash has two serialized time fields: 32-bit seconds, and 64-bit seconds. Zebra also keeps internal 64-bit times with additional 32-bit nanosecond precision.

In general, it's hard to identify, convert, serialize and proptest these different time types correctly.

This is a follow-up to #1849.

Describe the solution you'd like

Make wrapper types for serialized times:

Tasks for each wrapper type:

  • make arbitrary impls
    • the arbitrary impls should replace the existing datetime_full and datetime_u32 proptest strategies
    • we can use datetime_full for DateTime64 by removing nanoseconds
  • make serialization impls

Replace chrono::DateTime<Utc> with the appropriate types:

Describe alternatives you've considered

It would be nice if Zcash just used 64-bit times throughout the protocol.

@teor2345 teor2345 added C-bug Category: This is a bug S-needs-triage Status: A bug report needs triage P-Low C-security Category: Security issues I-panic Zebra panics with an internal error message I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data labels May 20, 2021
@dconnolly
Copy link
Contributor

It would be nice if Zcash just used 64-bit times throughout the protocol.

💕

@teor2345
Copy link
Contributor Author

I did DateTime32 in #2210 so we could do #2178

@mpguerra mpguerra removed the S-needs-triage Status: A bug report needs triage label May 31, 2021
@teor2345 teor2345 added A-consensus Area: Consensus rule updates A-network Area: Network protocol updates or fixes I-consensus Zebra breaks a Zcash consensus rule P-Medium and removed C-bug Category: This is a bug P-Low labels Dec 16, 2021
@mpguerra mpguerra mentioned this issue Jan 27, 2022
40 tasks
@teor2345
Copy link
Contributor Author

teor2345 commented Mar 1, 2022

This is a useful validation improvement, but it's not needed right now.

@teor2345 teor2345 closed this as completed Mar 1, 2022
@mpguerra mpguerra closed this as not planned Won't fix, can't repro, duplicate, stale May 11, 2023
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 C-security Category: Security issues I-consensus Zebra breaks a Zcash consensus rule I-invalid-data Zebra relies on invalid or untrusted data, or sends invalid data I-panic Zebra panics with an internal error message
Projects
None yet
Development

No branches or pull requests

3 participants