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.

Sign up for GitHub

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

Jump to bottom

Implement threshold cryptography #219

Merged
merged 26 commits into from
Sep 27, 2022
Merged

Implement threshold cryptography #219

merged 26 commits into from
Sep 27, 2022

Conversation

abread
Copy link
Contributor

@abread abread commented Sep 9, 2022

This PR introduces threshcrypto - a new module for working with threshold signatures in Mir, and an implementation using BLS12-381 threshold signatures (using the same underlying library as drand).

There was some discussion on Slack on how to go about this.

It's based on the existing crypto module: the dummy and pseudo implementations were kept for testing purposes, but I did not reimplement the DefaultImpl as no API consumers exist yet and there's no "real" implementations besides TBLS to guide its design.

@xosmig xosmig self-requested a review September 12, 2022 06:42
xosmig
xosmig reviewed Sep 12, 2022
View reviewed changes
Copy link
Contributor

@xosmig xosmig left a comment

Choose a reason for hiding this comment

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

Thanks a lot for the PR! I left a few minor cosmetic comments and a couple of comments w.r.t. to the interface.

pkg/threshcrypto/api.go Outdated Show resolved Hide resolved
pkg/threshcrypto/dummy.go Outdated Show resolved Hide resolved
pkg/threshcrypto/dummy.go Outdated Show resolved Hide resolved
pkg/threshcrypto/dummy.go Outdated Show resolved Hide resolved
pkg/threshcrypto/dummy.go Outdated Show resolved Hide resolved
pkg/threshcrypto/pseudo.go Outdated Show resolved Hide resolved
pkg/threshcrypto/pseudo.go Outdated Show resolved Hide resolved
pkg/threshcrypto/tbls.go Show resolved Hide resolved
pkg/threshcrypto/tbls.go Outdated Show resolved Hide resolved
pkg/threshcrypto/api.go Outdated Show resolved Hide resolved
@abread
Copy link
Contributor Author

abread commented Sep 14, 2022

@xosmig Thank you for the thorough review! I believe I have integrated all the suggestions

matejpavlovic
matejpavlovic approved these changes Sep 15, 2022
View reviewed changes
Copy link
Contributor

@matejpavlovic matejpavlovic left a comment

Choose a reason for hiding this comment

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

Very nice, thanks a lot for the contribution André!

I only added a few very minor cosmetic comments that you can use to polish the code, but I think the code is ready for merging even as-is. Great job! 👍

A big thanks also to @xosmig for the initial thorough review and comments!

pkg/threshcrypto/api.go Outdated Show resolved Hide resolved
pkg/threshcrypto/dummy.go Outdated Show resolved Hide resolved
pkg/threshcrypto/dummy.go Outdated Show resolved Hide resolved
pkg/threshcrypto/pseudo.go Outdated Show resolved Hide resolved
pkg/threshcrypto/pseudo.go Outdated Show resolved Hide resolved
pkg/threshcrypto/tbls_test.go Outdated Show resolved Hide resolved
@abread
Copy link
Contributor Author

abread commented Sep 15, 2022

Thank you so much, Matej!
I've updated the PR with your suggestions (and preallocated an array as suggested by the linter)

xosmig
xosmig approved these changes Sep 16, 2022
View reviewed changes
Copy link
Contributor

@xosmig xosmig left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Thanks a lot!
UPD: I added a few more comments. Most are trivial, but the question about the minimum threshold is important.

xosmig
xosmig reviewed Sep 16, 2022
View reviewed changes
pkg/threshcrypto/tbls.go Outdated Show resolved Hide resolved
xosmig
xosmig reviewed Sep 16, 2022
View reviewed changes
pkg/threshcrypto/tbls.go Outdated Show resolved Hide resolved
xosmig
xosmig reviewed Sep 16, 2022
View reviewed changes
pkg/threshcrypto/tbls.go Outdated Show resolved Hide resolved
xosmig
xosmig reviewed Sep 16, 2022
View reviewed changes
pkg/threshcrypto/tbls.go Outdated Show resolved Hide resolved
xosmig
xosmig reviewed Sep 16, 2022
View reviewed changes
pkg/threshcrypto/tbls.go Outdated Show resolved Hide resolved
xosmig
xosmig reviewed Sep 16, 2022
View reviewed changes
pkg/threshcrypto/tbls.go Outdated Show resolved Hide resolved
xosmig
xosmig reviewed Sep 16, 2022
View reviewed changes
pkg/threshcrypto/tbls.go Outdated Show resolved Hide resolved
@abread
Copy link
Contributor Author

abread commented Sep 19, 2022

I've started playing around with this, and ended up implementing some DSL utilities akin to the availability module.

They don't require much thought to implement so I went ahead and included them here

@matejpavlovic matejpavlovic mentioned this pull request Sep 24, 2022
Closed
@xosmig
Copy link
Contributor

xosmig commented Sep 26, 2022

@abread sorry that you had to do it several times already, but could you please merge the latest main branch into your branch?
Also, would it be OK if we use the "squash and merge" option? (in the main branch, there will be just 1 "Implement threshold cryptography" commit, which will include all the changes from this PR). Otherwise, you would need to clean up the commit history and rebase it over the current main (as opposed to merging main into your branch).

@abread
Copy link
Contributor Author

abread commented Sep 26, 2022 •
edited
Loading

sorry that you had to do it several times already, but could you please merge the latest main branch into your branch?

Ah, it was already done (when I added the DSL stuff) :)

Also, would it be OK if we use the "squash and merge" option?

Fine by me!

@abread
Copy link
Contributor Author

abread commented Sep 26, 2022

Weird, it gives me "Already up to date".
I'll try to rebase it against main

abread and others added 16 commits September 26, 2022 22:07
@abread
tbls: Validate signature share origin in VerifyShare
7611e48
@abread
Only accept signature shares from distinct nodes in Recover
ac98ac5 
Upon looking at the tbls implementation, it is clear that it is not ready
to handle multiple/duplicate signature shares from the same node.

This is something that can be done ahead of time with greater efficiency
leading to less calls to `Recover`.

Note: anonymous threshold signature schemes cannot validate share origin,
and therefore may require modifying our requirements
@abread
tbls: do not validate signature shares in Recover
ca7b7ea
@abread
Improve documentation
28fe9e1
@abread @matejpavlovic
Update pkg/threshcrypto/dummy.go
a0fe4fe 
Co-authored-by: Matej Pavlovic <matopavlovic@gmail.com>
@abread
Remove obsolete copyright notices
eec2e0b
@abread
Use types.NewNodeIDFromInt
94630e0
@abread
tbls: Preallocate fixed size array
4a5b11b
@abread @matejpavlovic
Update pkg/threshcrypto/pseudo.go
f50c080 
Co-authored-by: Matej Pavlovic <matopavlovic@gmail.com>
@abread
fix formatting
c0b214f
@abread @xosmig
Apply suggestions from code review
9fb7667 
Co-authored-by: Andrei Tonkikh <andrei.tonkikh@gmail.com>
@abread
Remove unused parameter in tbls_test::keygen
88abf30
@abread
Fix TBLS safety checks
d774258
@abread
threshcrypto: Add DSL module
9baa9fd
@abread
Document DSL package
78dd9ca
@abread
Remove threshold check
f74809f
@abread
Copy link
Contributor Author

abread commented Sep 26, 2022

Rebased to main, should be alright now!

@xosmig
Copy link
Contributor

xosmig commented Sep 26, 2022

Weird, it gives me "Already up to date".

Yes, sorry, it was my fault, I had the wrong option selected in github and the button for merging was gray 😩
I removed my comment after I figured that out, but, apparently, too late.
I'm just waiting for the final OK from @matejpavlovic before merging.

@abread
Copy link
Contributor Author

abread commented Sep 26, 2022

Ah dw, it was quick to do!

@matejpavlovic
Copy link
Contributor

Merging. Thanks for your contribution!!

@matejpavlovic matejpavlovic merged commit 7fbee12 into consensus-shipyard:main Sep 27, 2022
@abread abread deleted the threshcrypto branch September 27, 2022 17:10
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xosmig xosmig xosmig approved these changes

@matejpavlovic matejpavlovic matejpavlovic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@abread @xosmig @matejpavlovic