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

Simplify signature #620

Merged
merged 6 commits into from
Dec 12, 2022
Merged

Simplify signature #620

merged 6 commits into from
Dec 12, 2022

Conversation

iquerejeta
Copy link
Contributor

Content

This PR removes the verification key and the stake from the signature. These values are no longer required, as individual signatures no longer contain the merkle paths (it is now responsibility of the aggregator to include these values in the certificate). This required some changes in the multi_signer and single_signer, as these folders were verifying single signatures.

Now, in order to verify aggregate signatures, they include not only the individual signatures but also the corresponding RegParty, to be able to verify the batched membership proof (batched merkle path).

Pre-submit checklist

  • Branch
    • Commit sequence broadly makes sense
    • Key commits have useful messages
  • PR
    • No clippy warnings in the CI
    • Self-reviewed the diff
    • Useful pull request description
    • Reviewer requested

Issue(s)

This PR closes #619 .

@github-actions
Copy link

github-actions bot commented Nov 28, 2022

Unit Test Results

    3 files  ±0    28 suites  ±0   2m 45s ⏱️ -11s
339 tests ±0  339 ✔️ ±0  0 💤 ±0  0 ±0 
392 runs  ±0  392 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit c904e71. ± Comparison against base commit de9f2c2.

♻️ This comment has been updated with latest results.

@iquerejeta iquerejeta temporarily deployed to testing-preview November 28, 2022 17:22 Inactive
Copy link
Member

@jpraynaud jpraynaud left a comment

Choose a reason for hiding this comment

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

LGTM 👍

I think this is a breaking change as previous multi-signature will not be compatible with the new format. This means that we will probably have to re-genesis the certificate chains of the Mithril networks. Also all the SPOs will have to recompile their node. It is probably a good time to update the PROTOCOL_VERSION (and maybe enforce versions compatibility)? Let's wait before we merge it, WDYT?


// If there is no reg_party, then we simply received a signature from a non-registered
// party, and we can ignore the request.
if let Some((vk, stake)) = clerk.get_reg_party(&signature.signer_index) {
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
if let Some((vk, stake)) = clerk.get_reg_party(&signature.signer_index) {
let (vk, stake) = clerk.get_reg_party(&signature.signer_index).ok_or_else(|| ProtocolError::UnregisteredParty)?

This should avoid to create the if let and would return an error instead of Ok(())

mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
mithril-stm/src/stm.rs Outdated Show resolved Hide resolved
@jpraynaud jpraynaud added the breaking-change 🔥 Contains a breaking change label Nov 29, 2022
Copy link
Collaborator

@curiecrypt curiecrypt left a comment

Choose a reason for hiding this comment

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

I think the changes look good, the only change I should request is to update the size bench results in the readme. Since we exclude pk and stake from StmSig, we have no longer the size as 176 bytes for a single sig with 1 winning ticket.

We should update the related line as follows:

Note that the size of an individual signature with one valid index is **72 bytes** and increases linearly in the length of valid indices (where an index is 8 bytes).

@iquerejeta
Copy link
Contributor Author

Good catch. But I count 64 bytes + linear increase. 48 from sigma + 8 from the signer index + 8 from at least one index. Do you get the same?
cc: @curiecrypt

@curiecrypt
Copy link
Collaborator

curiecrypt commented Dec 2, 2022

Yes, I got the same. I wrote 72 bytes because if we create a signature it must include at least one winning index. So, 72 bytes = sigma + signer index + winning index @iquerejeta
Sorry, I will look at it again

@curiecrypt
Copy link
Collaborator

The extra 8 bytes coming from the number of indexes are included in serialized version of StmSig. @iquerejeta

@iquerejeta
Copy link
Contributor Author

Right! good catch.

@iquerejeta iquerejeta temporarily deployed to testing-preview December 5, 2022 09:33 Inactive
@github-actions
Copy link

Test Results

    3 files  ±0    28 suites  ±0   2m 55s ⏱️ +19s
341 tests ±0  341 ✔️ ±0  0 💤 ±0  0 ±0 
394 runs  ±0  394 ✔️ ±0  0 💤 ±0  0 ±0 

Results for commit 6c9de9b. ± Comparison against base commit f85f10d.

@iquerejeta iquerejeta temporarily deployed to testing-preview December 12, 2022 09:58 — with GitHub Actions Inactive
@iquerejeta iquerejeta merged commit bc19ee5 into main Dec 12, 2022
@iquerejeta iquerejeta deleted the simplify-signature branch December 12, 2022 11:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking-change 🔥 Contains a breaking change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove VerificationKey and Stake from individual signature
3 participants