Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(crypto/bls12381): signature aggregation #11
feat(crypto/bls12381): signature aggregation #11
Changes from 7 commits
ecaeb63
ca71e12
36b7c72
654bc38
4511acd
a535f96
4b2afec
fcb300b
80d8359
141d1a2
94b8fd6
f3b1bb7
6b0b06e
f752e1e
0c7153f
0800a7e
12c0ad1
8f82994
7ed479b
0093df8
cd6f8ad
e5932a4
089d45e
ae6c643
a2e8326
c925031
427e875
87e3312
38f96e3
092cb5b
b56a29e
fb07cde
2b2d2d5
1fd3cd1
12ba08a
87fd68b
8d7ce78
5905680
9c90795
a9349b3
71458b0
641d6ac
21a0137
97cf653
edc92e2
3881bf3
6677c0c
c66534e
f53d1ca
b24e14c
b01383a
ab29251
6a9e7bd
5463386
c8ccadf
0792f63
5605bca
40f5816
8139212
95455d5
de99d54
1f478ec
4fffebf
256a329
3c4c3a8
c71b2cc
82930be
0203b88
701204d
c4a6f43
46423c8
1a36a1b
b9c5a03
2670544
b48a625
42d65c8
5293186
d0de97d
d3c58c7
236829e
cfed49b
8650f2d
81fc562
303a6be
10d7cd9
2e27d2d
d98fefc
9739076
6266c88
15e1cfc
3caca93
ebfba32
ff1a813
c30e164
9af3d65
f314951
b4e8af3
af1d05f
eb5a08d
1217b48
4cead62
fe2b097
d53342f
4f7bd20
603ac3c
b44473b
9d7c8a3
78b03f6
d0c6115
804d022
ffbba5e
bf4c3b5
68ffc1b
1ac3231
2183f4e
15b4667
faedd63
27ccb72
8893054
c3341da
0253b40
4dea906
12195a8
9bb2dfc
156f344
235ba07
b7a9fdf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmmm, I might be wrong, but I don't agree with this.
When vote extensions are enabled, there is one signature for the vote and another signature for the vote extension.
I agree the extension signatures cannot be aggregated, as the extensions can be different.
But the vote themselves can still be aggregated (assuming you remove the timestamp field, or set it always to 0-time).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Good point. But then again, who is going to verify vote extensions? There's no point in aggregating votes if you need to verify vote extensions using individual verification (signature.Verify).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
On the 2nd thought, we don't verify vote extensions in
VerifyCommit
, so I think using them while aggregating vote signatures is okay.6b0b06e
(#11)There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we delete this or modify it so it works with the new scheme?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will remove
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reading one previous comment from Jasmina, I realise we need to add the necessary checks to make sure PBTS is active. Because this code no longer works with BFT Time (even if we decided to leave the timestamps in the commit structure, they would no longer be signed, so any full node on the way can now tamper with its value)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this is probably the biggest breaking change - removing Timestamp from vote sign bytes
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am double checking, I think it is ok because this is only for the canonical vote. Because vote timestamps are used to compute the block time when using BFT time.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we should also remove the timestamp field from both vote structures: the one internal to comet, and the protobuf-generated one. Leaving that field there only adds confusion IMO (let me know if you want to discuss)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should it go hand in hand with making the PBTS the default choice + ripping off median time?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The decision here was to add preemptive panics so we fail early in case CometBFT is misconfigured.