Repository navigation
fix: servers verify each client's opening before summing - #10
Conversation
Matches the paper's server step: every canonical client's opening must open that client's own commitment at the server's position, or the server posts nothing and names the client. Previously a bad opening made an honest server's aggregate fail at the recipient, which then dropped the server.
# Conflicts: # tests/end_to_end.rs
noot
changed the title
There was a problem hiding this comment.
🟡 Changes recommended
Missing per-client bounds let malicious openings cause honest servers to be blamed, while unchecked server indices can prevent correct attribution to the offending client.
2 open issues
What changed in this PR
This PR adds per-client opening verification to server rounds, aiming to identify invalid client input before aggregation rather than blame an honest server at the recipient.
Changes:
- Pass commitment parameters and bulletin commitments into
run_server_round, returningBadOpening(client)for failed verification. - Add regression tests for tampering, swapped commitments, wrong positions, and missing commitments.
- Update callers, benchmarks, examples, and documentation for the new API.
| File | Description |
|---|---|
tests/rs_mode_e2e.rs |
Supply client commitments in RS-mode rounds. |
tests/recipient_e2e.rs |
Update recipient test setup for server verification. |
tests/prony_e2e.rs |
Pass commitments through Prony round tests. |
tests/mse_e2e.rs |
Update MSE tests for the new server API. |
tests/end_to_end.rs |
Add rejection tests and shared round setup; update callers. |
src/scaling_bench.rs |
Include opening verification in server benchmarks. |
src/protocol/server.rs |
Verify individual openings and report offending clients. |
README.md |
Document commitment inputs and rejection behavior. |
examples/profile.rs |
Update profiling rounds to supply commitments. |
examples/demo.rs |
Read bulletin commitments for server verification. |
Review effort: Balanced
| .par_iter() | ||
| .zip(opening_refs.par_iter()) | ||
| .zip(comm_refs.par_iter()) | ||
| .find_map_first(|((cid, op), comm)| { |
There was a problem hiding this comment.
🟡 Changes recommended
The opening check does not validate server_index against position, allowing a client to abort the server round with MalformedOpening instead of being identified by BadOpening(cid).
2 open issues
1 resolved since last review
Review effort: Balanced
ameba23
left a comment
ameba23
left a comment
There was a problem hiding this comment.
Looks great 🌟 - pending the remaining issue from Copilot.
Regarding performance impact, i ran the scaling_bench with and without these changes, and am seeing this sort of impact on total CPU time (adding together client, server and recipient processing):
- Prony with 4kb message: 19% increase
- Prony with 16kb message: 16% increase
- IBLT with 4kb message: 36% increase
- IBLT with 16kb message: 10% increase
with 300 clients, 8 servers.
I don't think its worth scrutinising this too much, as i totally think we need this check. But something to bear in mind.
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
|
i'll look into some performance improvements next! thanks for pointing that out |
The server round now checks every canonical client's opening against that client's own commitment at the server's position before summing. A failing opening returns
ServerRoundError::BadOpening(client)and the server posts nothing.run_server_roundtakes the commitment parameters and the clients' commitments off the bulletin, mirroringrun_rs_node_round.The per-client check uses the fresh norm bounds,
β_cson the randomness andζon the digits, rather than the aggregate bounds the recipient uses.verifyis split into a bounded helper, withverifykeeping the aggregate bounds andverify_freshusing the fresh ones, so anyρ ≤ ρ_maxopenings the server accepts sum to an opening the recipient accepts.paper divergence
In "Constructing Relaxed Anonymous Broadcast" the server procedure decrypts each client's sealed opening, verifies it against that client's commitment at position i, and returns ⊥ for the whole round if any client fails. The code summed the openings unverified and left the check to the recipient, which verifies only the aggregate. The recipient's exclusion loop then treated a failed aggregate as the server's fault, so one bad opening from a client got an honest server dropped, and with the default threshold a few such clients could stall a round. The anonymity proof in "Anonymity against Malicious Clients" does not depend on the abort, so this is robustness and attribution, not privacy. The named client can now be excluded and the round re-run.
The commitment scheme's verify in "Preliminaries" bounds a fresh opening by
β_cs; the code's verify only had the aggregate boundβ_agg. Checking a single client against the aggregate bound would have let a client pass the server with oversized randomness and still push the sum past the recipient's bound.verification
All 150 tests pass. New tests cover a tampered opening (the targeted server names the client, the remaining servers still reach the threshold), a swapped commitment, an opening for another server's position, a missing commitment, and a commitment made with randomness at the aggregate bound that the aggregate check accepts but the fresh check rejects.