Refactor computing the public key package and expose it.

[dvc94ch]

These are useful for other crates. Our particular use case is integrating frost with substrate. So we store the verifiable secret sharing commitments on chain. If a node crashes it can recover and recompute the public key package from the on chain commitments.

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (877e7c3) 77.46% compared to head (afa6e26) 77.46%.
Report is 29 commits behind head on main.

❗ Current head afa6e26 differs from pull request most recent head 69a83a7. Consider uploading reports for the commit 69a83a7 to get more accurate results

Additional details and impacted files

@@           Coverage Diff           @@
##             main     #502   +/-   ##
=======================================
  Coverage   77.46%   77.46%           
=======================================
  Files          30       30           
  Lines        2742     2751    +9     
=======================================
+ Hits         2124     2131    +7     
- Misses        618      620    +2     

Files Changed	Coverage Δ
frost-core/src/frost/keys.rs	93.01% <100.00%> (+0.78%)	⬆️
frost-core/src/frost/keys/dkg.rs	87.20% <100.00%> (-1.06%)	⬇️

... and 5 files with indirect coverage changes

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[conradoplg]

Don't worry about clippy, we'll fix these shortly, some news errors came up in the Rust release last week

[conradoplg]

We have just fixed the clippy errors on main, if you rebase on top of it it should pass CI

[dvc94ch]

Done

[dvc94ch]

Kind of annoying that generate_trusted_dealer uses a different method of computing the public key than dkg. Was very confusing, was going insane trying to figure out how my implementation was not equivalent to the previous one. Obviously it is, it's just that generated_trusted_dealer only generates one set of commitments. Seems like the commitments need to be divided by max_signers so that summing them produces the original commitment. However that would be non compliant with the spec appendix. Any thoughts on what to do about it?

[conradoplg]

Kind of annoying that generate_trusted_dealer uses a different method of computing the public key than dkg. Was very confusing, was going insane trying to figure out how my implementation was not equivalent to the previous one. Obviously it is, it's just that generated_trusted_dealer only generates one set of commitments. Seems like the commitments need to be divided by max_signers so that summing them produces the original commitment. However that would be non compliant with the spec appendix. Any thoughts on what to do about it?

The VerifiableSecretSharingCommitment in trusted dealer keygen and in DKG are semantically different. In trusted dealer all participants receive the same VSSCommitment, which commits to the polynomial generated by the trusted dealer. In DKG each participant has its own value, which commits to the polynomial each of them have generated.

Thus this PR will need two methods: one for deriving a PublicKeyPackage from a single VSSCommitment, to use in trusted dealer, and one for deriving a PublicKeyPackage from a list of VSSCommitments, to use in DKG.

We're currently doing a bunch of refactorings which will conflict with this PR, so if you prefer I can do both the adjustment I mentioned and the rebasing on top of the refactorings.

[dvc94ch]

Yes, I understand that now. But PublicKeyPackage doesn't need two methods, just one. In dkg you can sum all the commitments to a single one which can be used to produce a PublicKeyPackage. This is what I called a group commitment. Not sure if there is a better word for it.

[dvc94ch]

So for a dkg you'd call PublicKeyPackage::from_commitment(compute_group_commitment(&commitments))

[conradoplg]

Good point! But I think it may be confusing to users. Also indeed the name group_commitment conflicts with another group_commitment in the protocol (there is even already a compute_group_commitment in the spec). I'd go with sum_commitments but I think it's better to have two different methods. In any case I'll discuss this with the team.

[conradoplg]

Thank you for this, indeed it makes a lot of sense and it helps organizing things.

I'm closing this in favor of #551, because I made a mess with the history after syncing with main and I didn't want to force push your branch, so I created that instead based on your commits. I made some changes to improve consistency with the rest of the code base but the API should be equivalent.