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

Spark modified frost implemenation #1

Open
wants to merge 6 commits into
base: secp256k1-tr-base
Choose a base branch
from

Conversation

zhenlu
Copy link

@zhenlu zhenlu commented Sep 25, 2024

No description provided.

@zhenlu zhenlu changed the base branch from main to secp256k1-tr-base September 25, 2024 19:42
Comment on lines +284 to +291
let outer_lambda_i = match (outer_coef_set_x, outer_signer_id) {
(Some(outer_coef_set_x), Some(outer_signer_id)) => {
// If the user's key is a SSS shard, we need to compute the outer lambda_i
frost::compute_lagrange_coefficient(outer_coef_set_x, None, *outer_signer_id)?
}
// Otherwise, we use additive approach for user's key and SO's keys.
_ => <<C::Group as Group>::Field>::one(),
};
Copy link
Collaborator

@conduition conduition Sep 27, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I see your approach here is to assume we have two distinct sets of signer IDs, the inner and the outer sets. This should work, but presently the code assumes the two sets don't intersect.

Let's say we set outer_coef_set_x = {1, 2} and inner_coef_set_x = {1, 2, 3, 4, 5, ...}. Signer 1 is the SE and signer 2 is the user. The signing_package stores all commitments, inner and outer, in a BTreeMap<Identifier<C>, round1::SigningCommitments<C>>.

If the inner signer (SO) with identifier 2 participates in the signing session, their commitment will overwrite the user's commitment, or vise-versa. Both commitments can't exist in the BTree at once.

My first thought is to adjust the structure of the SigningPackage or SigningParameters to include the outer signers' commitments as an extra field, rather than lumping them in with the inner signers' commitments.

How do you think that we should handle this?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually I should include this script on how to use this to make this more clear
https://gist.github.com/zhenlu/6183156fd6151bc82dedca1f6888c24e

So my approach is that each signer have 2 identifiers here, inner_id and outer_id. The SigningCommitments here is only using the inner_id, so the inner_id should be different for each signers. However, the outer_id is only used for calculate the Lagrange coefficient for the second round, so it's ok to intersect with inner.

In the above script, I have the inner_ids for the group as 1, 2, ..., n, and assume user's inner_id as 100 (Note this is only a hack for now, assuming we have less than 100 participants in the group.) And always set group's outer_id to 1, user's outer_id to 2 (I hardcoded this since our use case only has two participants in this round).

I think your approach might work better since we don't need these extra parameters if we include the outer signers' commitments as an extra field, (probably still need the outer signer's identifier to be able to calculate Lagrange coefficient for outer signers). My approach here is just to make the minimum change needed to test this works.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense, as long as we're aware of that limitation and can guard against it 👍

@conduition
Copy link
Collaborator

Hey @zhenlu, hope you're doing well. Just wanted to update you that the upstream PR in the ZF FROST repo has undergone a major restructure by conrado, including some API changes. The API will likely be changing again so for now I'd recommend holding off on upgrading your fork to match. Will keep you appraised as we work out the kinks and bring that PR closer to merging 👍

@zhenlu
Copy link
Author

zhenlu commented Oct 28, 2024

Thanks @conduition. Now we have completed the demo I have time to clean this up for the next step. Please keep me updated and I'll update this once you merged.

@zhenlu
Copy link
Author

zhenlu commented Nov 8, 2024

@conduition do you know the timeline for your PR to merge? I need to do some clean up on this and probably add more validation on aggregation and add a partial aggregation on the SO side + cheater detection.

@conduition
Copy link
Collaborator

Mentioned this in DM with Kevin today, things are in motion on ZcashFoundation/frost#730 but I can't say for sure when it will be merged. There might be further review which could change the code and cause further delays.

If i had to guess, I'd say it'll be merged before christmas and probably released in a semver tag by early/mid 2025. But i'm not sure, Conrado would be the better person to ask there.

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

Successfully merging this pull request may close these issues.

2 participants