Clearing the landing for Handover Protocol#186
Merged
cygnusv merged 49 commits intonucypher:rocknrollfrom Apr 14, 2025
Merged
Conversation
6a26e82 to
57f46bd
Compare
9ffb75c to
580fcc2
Compare
704bdc0 to
2304710
Compare
Co-authored-by: Piotr Roslaniec <p.roslaniec@gmail.com>
…nding Or more correctly, the problem must be we're not unblinding shares in tests. This makes sense because the new verifiability features for updates were designed to work on top of blinded shares, and these tests were written for the previous share update code
As suspected 2 commits ago
For the moment, just validating share update commitments
For some reason, it was part of WASM bindings too
See issue #192
See issue #196
Also, no need to compute the public from from the transcripts, since we can just take it from the aggregate
Two reasons: * We don't consider the case for num_validators != shares_num, so all the places that account for this can be refactored (see #197) * We always need to consider the case when threshold == shares_num because some tests have failed in the past in this situation.
* PublicDecryptionContextFast was unused (we use the simple variant) * Remove h parameter in the simple one, which has some simplifying downstream effects
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## rocknroll #186 +/- ##
=============================================
- Coverage 79.76% 72.32% -7.44%
=============================================
Files 24 24
Lines 5856 5460 -396
=============================================
- Hits 4671 3949 -722
- Misses 1185 1511 +326 ☔ View full report in Codecov by Sentry. |
We can always obtain it as a public parameter
derekpierre
approved these changes
Apr 3, 2025
Member
derekpierre
left a comment
There was a problem hiding this comment.
🎸 - I don't know if I understood all of the change, but looks reasonable.
ferveo-tdec/src/lib.rs
Outdated
| g_inv, | ||
| ) | ||
| .is_err()); | ||
| assert!(decrypt_with_shared_secret(&ciphertext, aad, shared_secret,) |
Member
There was a problem hiding this comment.
Should we just get rid of the trailing comma (one other spot on L261)?
Suggested change
| assert!(decrypt_with_shared_secret(&ciphertext, aad, shared_secret,) | |
| assert!(decrypt_with_shared_secret(&ciphertext, aad, shared_secret) |
Member
Author
There was a problem hiding this comment.
It seems both options (with or without comma) are accepted by the linter. Anyway, I'll remove them.
Additional cleaning
theref
approved these changes
Apr 8, 2025
This file contains hidden or 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
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
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.
Type of PR:
Required reviews:
What this does:
... How do I even begin? 😅
The goals of this PR have shifted over time, so sorry for the twists and turns. Initially this PR was intended to implement Refresh & Recovery protocols (see this comment from the this PRs of the R&R epic here #175 (review)) , but I detected several problems along the way with some low-level Ferveo abstractions that weren't used correctly. At roughly the same time I realized two additional things, one bad and another good: (👎) the recovery protocol was more complex than expected since one of their steps requires encrypted protocol messages, which hinders verifiability of protocol objects, and (👍) I came up with the handover protocol that greatly simplifies most of the situations that we wanted to solve via recovery. This forced us to shift again the focus of this PR towards refresh & handover, instead of refresh & recovery.
Some additional context:
The current objectives of this PR are:
Issues fixed/closed:
None 😅 Even though this PR does a ton of preparatory work, most of it was surfaced while trying to implement refresh & recovery protocols and realizing necessary refactors. In hindsight, it was a bad decision not to open issues along the way instead of trying to fix them in this living PR.
Why it's needed:
This PR is very important since the handover protocol could only be implemented on top of these changes. See #198
Notes for reviewers:
This PR is difficult to review, unless you have relatively deep knowledge of the codebase. Still, there's some commits that are very revealing:
UpdateTranscript, the basic protocol object during refresh, recovery and, potentially, handover protocols