Tighten language around replay protection #582
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
cjpatton
requested review from
tgeoghegan,
branlwyd,
ekr and
chris-wood
as code owners
tgeoghegan
approved these changes
Sep 22, 2024
cjpatton
force-pushed
the
cjpatton/442
branch
from
5e785ab to
d0cfe1d
Compare
|
Took Tim's suggestions, rebased, and squashed. |
cjpatton
force-pushed
the
cjpatton/442
branch
from
d0cfe1d to
097c039
Compare
branlwyd
reviewed
Sep 23, 2024
branlwyd
approved these changes
Sep 24, 2024
At the moment, we have very little normative/informative language around replay protection. The only normative language is buried in the "input share validation" subsection: > 1. Check if the report has been previously aggregated. If so, the input share > MUST be marked as invalid with the error `report_replayed`. A report is > considered aggregated if its contribution would be included in a relevant > collection job. Another problem is that we never say explicitly to record the IDs of aggregated reports. Here the Aggregator has a choice: it can avoid excess computation by storing the ID of every report it sees in an aggregation job, thereby ensuring reports that are rejected for some other reason aren't processed again; or it can avoid excess storage by only storing the IDs of output shares it wants to aggregate. This change has several parts to it: 1. Add a section to the protocol overview about replay protection (why/how) and a paragraph to the aggregation sub-protocol overview. 1. Remove the replay check from the "input share validation" section. 1. Have the Leader check for replays right after it has picked a candidate set of reports. Have it to store the IDs either before initialization or after completion of the aggregation job. 1. Have the Helper resolve replays (reject replays and update the stored set) either at the beginning of an aggregation job or just before completing it. 3. Be explicit about the use of report IDs for replay protection. This only impacts an implementation note, which envisions multiple schemes for replay protection, like hashing the report. This note has been moved from the "input share validation" section to operational considerations. Co-authored-by: Brandon Pitman <[email protected]>
cjpatton
force-pushed
the
cjpatton/442
branch
from
78ad3b5 to
60d35ec
Compare
|
Rebased and squashed. |
branlwyd
added a commit
that referenced
this pull request
Oct 4, 2024
The related implementation note is removed, too -- it is nowadays duplicative of the text in the Replay Protection section. I think this was an intended part of #582, based on the commit text of that PR.
branlwyd
added a commit
that referenced
this pull request
Oct 7, 2024
The related implementation note is removed, too -- it is nowadays duplicative of the text in the Replay Protection section. I think this was an intended part of #582, based on the commit text of that PR.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Closes #442.
At the moment, we have very little normative/informative language around replay protection. The only normative language is buried in the "input share validation" subsection:
Another problem is that we never say explicitly to record the IDs of aggregated reports. Here the Aggregator has a choice: it can avoid excess computation by storing the ID of every report it sees in an aggregation job, thereby ensuring reports that are rejected for some other reason aren't processed again; or it can avoid excess storage by only storing the IDs of output shares it wants to aggregate.
This change has several parts to it:
Add a section to the protocol overview about replay protection (why/how) and a paragraph to the aggregation sub-protocol overview.
Remove the replay check from the "input share validation" section.
Have the Leader check for replays right after it has picked a candidate set of reports. Have it to store the IDs either before initialization or after completion of the aggregation job.
Have the Helper resolve replays (reject replays and update the stored set) either at the beginning of an aggregation job or just before completing it.
Be explicit about the use of report IDs for replay protection. This only impacts an implementation note, which envisions multiple schemes for replay protection, like hashing the report. This note has been moved from the "input share validation" section to operational considerations.