-
Notifications
You must be signed in to change notification settings - Fork 31
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
Ed/merge sig agg #1589
Ed/merge sig agg #1589
Conversation
…ment_for_votedata Make the commitment of all VoteData of fixed length
Add bitvec serialization
…pressoSystems/HotShot into sishan/signature_aggregation_with_hsp
Add signature serialization function
…pressoSystems/HotShot into sishan/signature_aggregation_with_hsp
… bit vector proof is relayed for validation
…urve_in_signature parameterizing curve in signature
…in_0821 Sishan/merge sig agg to main 0821
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM!
I get this failure when I run the tests on my machine.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- I thought @dailinsubjam is going to move
SignatureKey
into its own crate under/crates/
and out of/types
? does that affect this PR as well? (or is this PR supposed to eclipse Sishan's?) - I got lost in the many abstractions of crypto-team-defined
QuorumCertificate
, jellyfish'sAggregatableSignature
, and here HotShot'sSignatureKey
,AssembledSignature
,SignedCertificate
, and HotShot's ownQuorumCertificate
, we should clarify them and remove unnecessary layers of abstraction. (maybe in a separate PR)
in fact, I'm lost because I don't know which trait is for what purposes and thus cannot comment on what API I expect it to have. - would be great if @elliedavidson could provide more details in the PR description, at least highlight the main changes, and what changes deserve special attention (if any)
/// List of known node's public keys, including own, sorted by nonce () | ||
pub known_nodes: Vec<K>, | ||
/// List of known node's public keys and stake value for certificate aggregation, serving as public parameter | ||
pub known_nodes_with_stake: Vec<ENTRY>, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
it felt underconstrained w/o specifying the equivalence between the key in ENTRY
and K
.
I wonder if you want to use:
use hotshot-types::traits::stake_table::StakeTableScheme;
pub struct HotShotConfig<K,ST: StakeTableScheme, ELECTIONCONFIG>
where ST::Key: K, {
pub known_nodes: Vec<K>,
pub stake_table: ST,
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
- Yes, that will happen in a separate PR!
- I agree. In general HotShot traits have gotten messy. Here is an issue for this, but we unfortunately have other priorities to work on right now: Remove unnecessary layers of abstraction between crypto primitives and HotShot traits #1595
- Normally a PR would have more details! Sishan's original PR did have more details. I only made this PR since Sishan can't make it while she is off.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Re this particular comment - I agree - good catch. I think we should probably just remove the known_nodes since it effectively the same thing as known_nodes_with_stake. Issue here: #1596
@philippecamacho |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
This PR is a wrapper around this PR: #1568