Simon/doc eddsa

[SimonRastikian]

Closes #239 .

Additionally, I found that the ECDSA OT based signing phase doc does not follow the implementation that includes a coordinator. This tiny change has been made here.

[codecov-commenter]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 87.91%. Comparing base (e71f423) to head (7dd26b1).
⚠️ Report is 2 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #243      +/-   ##
==========================================
- Coverage   88.46%   87.91%   -0.55%     
==========================================
  Files          50       51       +1     
  Lines        9733     9789      +56     
  Branches     9733     9789      +56     
==========================================
- Hits         8610     8606       -4     
- Misses        673      733      +60     
  Partials      450      450              

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

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[gilcu3]

Main blocker is that we need to update the comments in the implementation, so that each step that we mention in the guide is backed by a comment in the code. Once that is done I can review again to make sure they match. Please also take a look at the markdown suggestions in #245, I think they make reading the document easier as well (so not only making a linter happy).

[SimonRastikian]

@gilcu3 I suggest you do not block the documentation PR for the implementation. Instead, let's open another PR for the implementation comments

[gilcu3]

@gilcu3 I suggest you do not block the documentation PR for the implementation. Instead, let's open another PR for the implementation comments

I am only blocking it because it is part of the documentation IMO. If that's not the case then I am happy removing the block.

[gilcu3]

Looks great to me!

I was able to easily correlate implementation and docs, which is very nice 😄

Left some minor improvement suggestions

[gilcu3]

could we have here a small subsection with notation instead? for example, the variable $\mathcal{P}_0$ is used below but not yet defined

[SimonRastikian]

We will in #18

[gilcu3]

Oh but that might take some time still, and in the mean time this doc will be a bit inconsistent. Something as simple as :

Nottation:

• $t$: threshold, which equals the number_malicious_parties
• $\mathcal{P}_0$: set of participants from the DKG

is enough.

Now that I think about it, wasn't threshold = number_malicious_parties + 1 here? 😱 (that's how we are using it in the node atm afaik)

[SimonRastikian]

I think we clarified the terminology. From now on we pplease use the proper one

[gilcu3]

which one is the proper one? number_malicious_parties? because if that's the case we might have a small bug in the node

[SimonRastikian]

Let's not talk about threshold anymore but about max_malicious

[gilcu3]

No problem with that, although we are still using threshold in the code, so we will be forced to keep using it for a while.

Anyway, the original comment still holds, I still believe we should define $\mathcal{P}_0$ before using it

[gilcu3]

adding the In short, the following sum is executed:... is nice, but not complete as R is also aggregated. I would rather explain all, or not explain it, as we already link the RFC. Else the reader might be clueless about where R comes from.

[SimonRastikian]

Sorry not sure what you mean..

[gilcu3]

That we explain where s comes from, but say nothing about R that is also computed in the same step, unless I missed something when checking the implementation of the aggregation function

[SimonRastikian]

You are absolutely right, the thing is (and I dislike it) is that the RFC aggregates only the s terms but the implementation aggregates also the R term. Now what I dislike is that R is already aggregated in round2::sign and then a second time in round2::aggregate.
I really dislike this inefficiency but it is a price we are paying due to the fact that we are using FROST implementation.
What are your thoughts?

[gilcu3]

Interesting that they decided to do the computation twice, will need to think about what could they possibly be protecting against, cannot believe they simply missed it 😞

EDIT: took a look at the RFC https://datatracker.ietf.org/doc/html/rfc9591#name-signature-share-aggregation

and they seem to be computing the group commitment (R) as well. Are you sure we are not missing something here?

def aggregate(commitment_list, msg, group_public_key, sig_shares):
  # Compute the binding factors
  binding_factor_list = compute_binding_factors(group_public_key,
   commitment_list, msg)

  # Compute the group commitment
  group_commitment = compute_group_commitment(
      commitment_list, binding_factor_list)

  # Compute aggregated signature
  z = Scalar(0)
  for z_i in sig_shares:
    z = z + z_i
  return (group_commitment, z)