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

Put the challenge at the end #276

Merged
merged 5 commits into from
Jul 10, 2023

Conversation

BasileiosKal
Copy link
Contributor

@BasileiosKal BasileiosKal commented Jul 4, 2023

Fix #265

Move the challenge value c at the end of the proof value. Specifically, the proof becomes

proof = (Abar, Bbar, r2^, r3^, (m^_j1, ..., m^_jU), c)

Also change Abar with Bbar in ProofGen and ProofVerify

@BasileiosKal BasileiosKal force-pushed the vasilis/challenge-index branch 2 times, most recently from 406fc8e to 8951a38 Compare July 6, 2023 11:07
@BasileiosKal BasileiosKal force-pushed the vasilis/challenge-index branch from 8951a38 to 61fe83b Compare July 6, 2023 11:22
@BasileiosKal
Copy link
Contributor Author

Breaking Changes

  1. moved the challenge c value at the end of the proof value (requires an update to the proof serialization/de-serialization operations).
  2. In ProofGen. At step 11 during C calculation, switched Abar with Bbar. I.e.,
    C = Bbar * r2 + Abar * r3 + ...  -->>  C = Abar * r2 + Bbar * r3 + ...
    
    Also in step 15 and 16 of ProofGen, the following changed (e "moved" from r3^ to r2^)
    15. r2^ = r2 + r4 * c      -->>  r2^ = r2 + r4 * e * c
    16. r3^ = r3 + r4 * e * c  -->>  r3^ = r3 + r4 * c
    
  3. In ProofVerify, at step 8, again change Abar with Bbar during C calculation, i.e.,
     C = Bbar * r2^ + Abar * r3^ + ...   -->>   C = Abar * r2^ + Bbar * r3^ + ...
    

@BasileiosKal
Copy link
Contributor Author

@andrewwhitehead, I'm really sorry, but there are some more changes to review 🙏

@christianpaquin
Copy link
Contributor

christianpaquin commented Jul 6, 2023

Is this captured in any test vectors? Never mind, I see they are part of the PR itself.

Copy link
Contributor

@christianpaquin christianpaquin left a comment

Choose a reason for hiding this comment

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

Some minor editorial comments. Implemented the update in my library (PR 23); test vectors pass.

1. (Abar, Bbar, c, r2^, r3^, (m^_1, ..., m^_U)) = proof
2. return serialize((Abar, Bbar, c, r2^, r3^, m^_1, ..., m^_U))
1. (Abar, Bbar, r2^, r3^, (m^_1, ..., m^_U), c) = proof
2. return serialize((Abar, Bbar, r2^, r3^, m^_1, ..., m^_U), c)
Copy link
Contributor

Choose a reason for hiding this comment

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

Why have in inner parenthesis and have the c out of it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch! It was a typo. Fixed. Thanks!

@@ -1122,7 +1122,7 @@ Procedure:
7. if A_i is INVALID or Identity_G1, return INVALID
8. index += octet_point_length

// Scalars (i.e., (c, r2^, r3^, (m^_j1, ..., m^_jU)) in
// Scalars (i.e., (r2^, r3^, (m^_j1, ..., m^_jU), c) in
Copy link
Contributor

Choose a reason for hiding this comment

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

A bit confusing to have the m^_j_i in parenthesis.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed. Thanks!

Copy link
Contributor

@Wind4Greg Wind4Greg left a comment

Choose a reason for hiding this comment

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

I'm not sure why were are doing the swapping of Abar and Bbar. When I try to map this back to the Revisiting BBS paper it seems like the way we currently have it is a close match and this change seems less like the paper.

Is there another version of the paper with this changed?

Is there a reason for this change?

Note that we also don't seem to cite the "Revisiting BBS" paper in the update.

@BasileiosKal
Copy link
Contributor Author

Hey Greg 👋 The swapping is mainly for readability. Note that we just want to calculate Abar * a_random_number + Bbar * another_random_number. It is just a bit more sense to have Abar * r2 + Bbar * r3 instead of Bbar * r2 + Abar * r3.

This does not go against the paper. It is just a naming convention. Another way to think of it, is instead of "swapping" Abar and Bbar we "renaming" r2 to r3.

The reason it is a breaking change is that we have deterministic random number generation for testing.

Copy link
Contributor

@Wind4Greg Wind4Greg left a comment

Choose a reason for hiding this comment

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

Implemented and confirmed all test vectors. Note the high level text description still has the parameters in the old order:

The inputted proof value must consist of the following components, in that order:

1. Two (2) valid points of the G1 subgroup, different from the identity point of G1 (i.e., `Abar, Bbar`, in ProofGen)
2. Three (3) integers representing scalars in the range of 1 to r-1 inclusive (i.e., `c, r2^, r3^`, in ProofGen).
3. A number of integers representing scalars in the range of 1 to r-1 inclusive, corresponding to the undisclosed from the proof messages (i.e., `m^_j1, ..., m^_jU`, in ProofGen, where U the number of undisclosed messages).

@BasileiosKal BasileiosKal force-pushed the vasilis/no-header-ph-fixtures branch from e90be79 to 336fdab Compare July 10, 2023 16:17
@BasileiosKal BasileiosKal force-pushed the vasilis/challenge-index branch from bde5cf3 to 4592170 Compare July 10, 2023 16:37
@BasileiosKal
Copy link
Contributor Author

Discussed on the WG call on the 10th of July. Test vectors are cross validated by multiple implementations. Merging.

@BasileiosKal BasileiosKal merged commit 65baa33 into vasilis/no-header-ph-fixtures Jul 10, 2023
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.

4 participants