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

Require all timestamps to align with time_precision (*) #639

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

cjpatton
Copy link
Collaborator

@cjpatton cjpatton commented Jan 14, 2025

Prescribe that all timestamps and time intervals are a multiple of
time_precision.

This is already required in most situations, but we are currently
missing at least one edge case: Aggregators are meant to reject reports
with timestamps that precede the task start time, but the Client is also
supposed to round down its timestamp, meaning it may get rejected even
if upload time is valid. This case is solved by the new requirement.

This also means that we must lift "Clients SHOULD truncate report
timestamps" to "MUST". Following the principle that "MUST must be
enforced", require rejection of reports with malformed timestamps during
aggregation.

Note: this is a breaking change since we're being stricter about
contents on the wire.

@cjpatton cjpatton added bug Something isn't working draft-14 labels Jan 14, 2025
@cjpatton cjpatton marked this pull request as ready for review January 14, 2025 18:19
Copy link
Collaborator

@branlwyd branlwyd left a comment

Choose a reason for hiding this comment

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

Eh, I think this is optional: an implementation/deployment could allow the task_start time to only be a multiple of the time precision, for example -- which is a good idea since, due to client timestamp rounding, time_precision is effectively the smallest meaningful unit of time. Or the deployment could pre-date the task_start time with enough of a "margin" that rounding the client timestamp doesn't break things.

If we do want to keep this, I suggest we drop the task_start - (task_start % time_precision) notation and instead simply suggest rounding down to the nearest multiple of the task's time_precision. I think this would be easier to follow, and would match how we describe the client's behavior: "The Client SHOULD round this value down to the nearest multiple of the task's time_precision in order to ensure that that the timestamp cannot be used to link a report back to the Client that generated it."

@tgeoghegan
Copy link
Collaborator

The proposed change fixes this specific issue, but it's possible there are other places in DAP where we handle time and might make a similar mistake. Rather than fixing these one by one, we could instead assert that all times in DAP (including task_start and expiry) must be rounded to the task's time_precision, which would fix everything at once.

@cjpatton cjpatton changed the base branch from cjpatton/time to main January 15, 2025 15:37
@cjpatton
Copy link
Collaborator Author

@branlwyd the reason I don't think this should be optional is that it's a case where rejection is systemic to the protocol. Clients are encouraged to truncate times tamps, but if they do, then their report might get rejected depending on the value of task_start.

I took your editorial suggestion.

@tgeoghegan nice observation. Let's discuss potential solutions in the DAP sync today.

@cjpatton
Copy link
Collaborator Author

2025/1/15 call: wherever we have a timestamp, round it down to the nearest multiple of time_precission.

@cjpatton cjpatton marked this pull request as draft January 15, 2025 16:38
@cjpatton cjpatton changed the title Round down the task start time when checking the report timestamp Require all timestamps to align with time_precision (*) Jan 15, 2025
@cjpatton
Copy link
Collaborator Author

Alright, ready for another round here. Note that this is not quite a minimal change.

@@ -1876,6 +1908,10 @@ following checks:
1. Check that the input share can be decoded as specified by the VDAF. If not,
the input share MUST be marked as invalid with the error `invalid_message`.

1. Check that the report's timestamp is well-formed as specified in
{{timestamps}}. If not, the Aggregator MUST mark the input share as invalid
with error `invalid_message`.
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do we want a new report error variant here?

Copy link
Collaborator

Choose a reason for hiding this comment

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

I don't think so. It's hard to imagine any client out there that would first try an unrounded timestamp, then gets error your_timestamp_sucks and then goes "OK, FINE, I'll round it down!" Transmitting an unrounded timestamp in a report just suggests a defective client that isn't ever going to succeed.

Prescribe that all timestamps and time intervals are a multiple of
`time_precision`.

This is already required in most situations, but we are currently
missing at least one edge case: Aggregators are meant to reject reports
with timestamps that precede the task start time, but the Client is also
supposed to round down its timestamp, meaning it may get rejected even
if upload time is valid. This case is solved by the new requirement.

This also means that we must lift "Clients SHOULD truncate report
timestamps" to "MUST". Following the principle that "MUST must be
enforced", require rejection of reports with malformed timestamps during
aggregation.

Note: this is a breaking change since we're being stricter about
contents on the wire.
@cjpatton cjpatton marked this pull request as ready for review January 15, 2025 22:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working draft-14 wire breaking
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants