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

Optimistic signature aggregation by leader #4123

Merged
merged 26 commits into from
Jun 27, 2022
Merged

Optimistic signature aggregation by leader #4123

merged 26 commits into from
Jun 27, 2022

Conversation

ssd04
Copy link
Contributor

@ssd04 ssd04 commented May 27, 2022 •
edited
Loading

Optimistic signature aggregation on consensus:

  • the consensus nodes will not verify signature share anymore, since the leader will verify the aggregated signature anyway
  • if the agg sig verifcation fail, the leader will check every signature share for each node, and if the number of valid signature shares will be higher than the threshold, it will send the partial agg signature
  • unit tests

@ssd04 ssd04 self-assigned this Jun 6, 2022
@codecov-commenter
Copy link

codecov-commenter commented Jun 6, 2022 •
edited
Loading

Codecov Report

❗ No coverage uploaded for pull request base (feat/optimise-consensus-sigcheck@5831290). Click here to learn what that means.
The diff coverage is n/a.

@@                         Coverage Diff                         @@
##             feat/optimise-consensus-sigcheck    #4123   +/-   ##
===================================================================
  Coverage                                    ?   75.83%           
===================================================================
  Files                                       ?      644           
  Lines                                       ?    85099           
  Branches                                    ?        0           
===================================================================
  Hits                                        ?    64539           
  Misses                                      ?    15770           
  Partials                                    ?     4790

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5831290...31114b8. Read the comment docs.

AdoAdoAdo
AdoAdoAdo reviewed Jun 7, 2022
View reviewed changes
consensus/spos/bls/subroundEndRound.go Outdated Show resolved Hide resolved
consensus/spos/bls/subroundEndRound.go Outdated Show resolved Hide resolved
ssd04 added 13 commits June 7, 2022 16:24
@ssd04
use signature subround jobs state instead of using custom bitmap
4b89afd
@ssd04
added unit tests for verify signature shared on agg sig verification …
b04812a 
…fail
@ssd04
use new temp multisigner to store only valid sig shares
33e778b
@ssd04
fix issues with multisigner saved state
fcfee88
@ssd04
avoid resetting multiSigner data
c222360
@ssd04
return updated bitmap and aggregated signature
a3d5e90
@ssd04
remove store signature since multisigner is not reset anymore
f649a5f
@ssd04
update unit test to return more variables
331bea8
@ssd04
added separate function for agg sig after verification fail
9973538
@ssd04
handle lower than threshold case when verification fail; remove log d…
4650415 
…ebug messages
@ssd04
unit tests for compute agg sig on valid nodes
1b77058
@ssd04
cleanup log debug messages
c5a24ad
@ssd04
added TODO for invalid sig shares slashing
cdc7a39
@ssd04 ssd04 marked this pull request as ready for review June 22, 2022 08:09
@ssd04 ssd04 changed the title verify aggregated sig on end round by leader Optimistic signature aggregation by leader Jun 22, 2022
@ssd04 ssd04 requested a review from AdoAdoAdo June 22, 2022 09:41
AdoAdoAdo
AdoAdoAdo reviewed Jun 24, 2022
View reviewed changes
consensus/spos/bls/subroundEndRound.go Outdated Show resolved Hide resolved
consensus/spos/bls/subroundEndRound.go Outdated Show resolved Hide resolved
@ssd04
fixes after review:
4961c7a 
- do not pass multisigner as parameter since it is not needed anymore
- no need to verify agg sig when created only with valid sigs
AdoAdoAdo
AdoAdoAdo previously approved these changes Jun 24, 2022
View reviewed changes
raduchis
raduchis reviewed Jun 24, 2022
View reviewed changes
consensus/spos/bls/subroundEndRound.go
if err != nil {
isSuccessfull = false

err = sr.SetJobDone(pk, SrSignature, false)
Copy link
Contributor

Choose a reason for hiding this comment

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

also the sr.PeerHonestyHandler() should be decreased? maybe twice? It was increased once on the receivedSignature and now it was found it is invalid.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍 will include in the next PR when slashing will be applied

Copy link
Contributor Author

Choose a reason for hiding this comment

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

as discussed, decreased peerHonesty here to be in line with the old setup, since the peerHonesty has been increased optimistically for the malicious signer

raduchis
raduchis previously approved these changes Jun 24, 2022
View reviewed changes
Copy link
Contributor

@raduchis raduchis left a comment

Choose a reason for hiding this comment

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

will be done on the slashing PR

@ssd04 ssd04 dismissed stale reviews from raduchis and AdoAdoAdo via 291b6f9 June 24, 2022 14:25
AdoAdoAdo
AdoAdoAdo previously approved these changes Jun 24, 2022
View reviewed changes
@AdoAdoAdo AdoAdoAdo merged commit cdc7507 into feat/optimise-consensus-sigcheck Jun 27, 2022
@AdoAdoAdo AdoAdoAdo deleted the optimistic-sig-aggregation-on-consensus branch June 27, 2022 08:40
@schimih schimih added the ignore-for-release-notes Do not include item in release notes label May 25, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@raduchis raduchis raduchis approved these changes

@AdoAdoAdo AdoAdoAdo AdoAdoAdo approved these changes

Assignees

@ssd04 ssd04

Labels
ignore-for-release-notes Do not include item in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@ssd04 @codecov-commenter @raduchis @AdoAdoAdo @schimih