Yet another batch of improvements for Coordinator#107
Merged
KPrasch merged 16 commits intonucypher:developmentfrom Aug 27, 2023
Merged
Yet another batch of improvements for Coordinator#107KPrasch merged 16 commits intonucypher:developmentfrom
KPrasch merged 16 commits intonucypher:developmentfrom
Conversation
|
|
||
| address authority; | ||
| uint16 dkgSize; | ||
| uint16 threshold; |
Contributor
There was a problem hiding this comment.
This is going to help declutter API in nucypher-ts
piotr-roslaniec
approved these changes
Aug 24, 2023
…or the moment We currently define it as the lowest value that produces a threshold set that's strictly greater than the 50% of the overall size. See nucypher/nucypher#3095
We can revert to a composition model later, but for the moment this will make our lives easier
manumonti
approved these changes
Aug 25, 2023
Contributor
manumonti
left a comment
There was a problem hiding this comment.
LGTM! 👌
Just a few questions/suggestions...
| uint32 endTimestamp; | ||
| uint16 totalTranscripts; | ||
| uint16 totalAggregations; | ||
| // |
Contributor
There was a problem hiding this comment.
It's not a important thing, but why this empty line is commented?
| uint16 dkgSize; | ||
| uint16 threshold; | ||
| bool aggregationMismatch; | ||
| // |
Contributor
There was a problem hiding this comment.
It's not a important thing, but why this empty line is commented?
| function getRitualIdFromPublicKey( | ||
| BLS12381.G1Point memory dkgPublicKey | ||
| ) external view returns (uint32 ritualId) { | ||
| // If public key is not registered, result will produce underflow error |
Contributor
There was a problem hiding this comment.
An underflow error sounds like a too cryptic message if you just used a wrong ritualId, doesn't it?
What about using a require here? Something like
Suggested change
| // If public key is not registered, result will produce underflow error | |
| require(getRitualState(ritualId) != RitualState.INVALID, "Ritual not found") |
KPrasch
approved these changes
Aug 27, 2023
derekpierre
reviewed
Aug 29, 2023
| keccak256(ritual.aggregatedTranscript) != aggregatedTranscriptDigest | ||
| ) { | ||
| ritual.aggregationMismatch = true; | ||
| delete ritual.publicKey; |
| bytes32 digest // signed message hash | ||
| uint32 ritualId, | ||
| bytes memory evidence, // supporting evidence for authorization | ||
| bytes memory ciphertextHeader // data to be signed by authorized |
Member
There was a problem hiding this comment.
👏 .
@cygnusv Is the ciphertext header a fixed size? If so, any benefits that we can leverage from that fact like bytes32 or ...?
This was referenced Sep 18, 2023
Closed
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.
1 + dkg size/2. See Should DKG Threshold be specified for Ritual nucypher#3095FeeModelandCoordinatorGlobalAllowlist (Closes Optimze allow list storage lookups #97)IEncryptionAuthorizerAPI to requireciphertext_headerinstead ofdigest, to further strengthen verification.