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

Possible states of PrepareSteps sent in an aggregation continue request are ambiguous. #438

Closed
divergentdave opened this issue Apr 18, 2023 · 4 comments

Comments

@divergentdave
Copy link
Collaborator

The spec says in the helper continuation section that helpers should be prepared for leaders to send PrepareSteps in the failed state.

If the status is failed, then mark the report as failed and reply with a failed PrepareStep to the Leader.

In the leader continuation section, it says of the same continue request message that,

The prepare_steps field MUST be a sequence of PrepareSteps in the continued state containing the corresponding inbound prepare message.

We should be clear about whether non-continue PrepareStepStates are allowed in these requests. It might be useful to allow failed in AggregationJobContinueReq steps, because then it could carry a ReportShareError, so that the helper could get visibility of error codes, as the leader does. (The helper can already notice that a report share was filtered out from one request to the next, and infer that there was some error)

@cjpatton
Copy link
Collaborator

Yup, we definitely need to clarify this, and I agree that an explicit signal of rejection is most useful. For what it's worth Daphne Helper will abort if it encounters a failure in the AggregateContReq: https://github.com/cloudflare/daphne/blob/main/daphne/src/vdaf/mod.rs#L755

@branlwyd
Copy link
Collaborator

I agree this merits clarification, and also vote for an explicit signal of rejection (i.e. the Leader sends Failed PrepareSteps to the Helper on error).

@tgeoghegan
Copy link
Collaborator

I believe this issue is obsolete, because the changes in #393 mean that it's no longer possible for AggregationJobContinueReq to contain a failure or share rejection message. However, what remains is the idea that the leader should explicitly signal preparation failure to the helper. I don't think we have a strong enough case for this yet, especially since deployments can use some means out of band of DAP to share error information between aggregators. So we should keep this open to eventually discuss an in-band mechanism for leader-to-helper error reporting, but I don't think there's anything to do for draft-ietf-ppm-dap-05 anymore (except to update a TODO which referenced the wrong issue number).

tgeoghegan added a commit that referenced this issue Jun 26, 2023
cjpatton pushed a commit that referenced this issue Jul 3, 2023
@cjpatton
Copy link
Collaborator

Closing with no action. For the moment we don't have a compelling reason for a fancier error handling mechanism. If we want to discuss this further, let's open a fresh issue with a more refined problem statement.

@cjpatton cjpatton closed this as not planned Won't fix, can't repro, duplicate, stale Oct 24, 2023
cjpatton added a commit that referenced this issue Sep 20, 2024
* Add a submission type (IETF).
* Remove unused references.
* Remove square brackets, which markdown wants to interpret as a link.
  In one case we escape the brackets, e.g., `\[`, `\]`, in other cases
  we rework the text.
* Fix all instances of art being too wide.
* Remove OPEN ISSUE about the size of the HPKE config ID, as there's
  no reason to change this at this point.
* Remove OPEN ISSUE referencing #217 (relax ordering of prep inits).
* Remove TODO referencing #384 (latest discussion is #581).
* Remove OPEN ISSUE referencing #438 (Leader signals rejection).
* Remove OPEN ISSUE referencing #401 (aggregation job rewinding).
* Remove OPEN ISSUE referencing #195 (collection flexibility).
cjpatton added a commit that referenced this issue Sep 23, 2024
* Add a submission type (IETF).
* Remove unused references.
* Remove square brackets, which markdown wants to interpret as a link.
  In one case we escape the brackets, e.g., `\[`, `\]`, in other cases
  we rework the text.
* Fix all instances of art being too wide.
* Remove OPEN ISSUE about the size of the HPKE config ID, as there's
  no reason to change this at this point.
* Remove OPEN ISSUE referencing #217 (relax ordering of prep inits).
* Remove TODO referencing #384 (latest discussion is #581).
* Remove OPEN ISSUE referencing #438 (Leader signals rejection).
* Remove OPEN ISSUE referencing #401 (aggregation job rewinding).
* Remove OPEN ISSUE referencing #195 (collection flexibility).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants