-
Notifications
You must be signed in to change notification settings - Fork 42
[bls-cert-verify] Optimize bls-cert-verify and improve code quality #706
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
base: master
Are you sure you want to change the base?
[bls-cert-verify] Optimize bls-cert-verify and improve code quality #706
Conversation
813ae83 to
6721329
Compare
akhi3030
left a comment
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.
Thanks, looks great!
| fn bench_verify_cert(c: &mut Criterion) { | ||
| let mut group = c.benchmark_group("BLS Cert Verify"); | ||
| let validator_sizes = [500, 1000, 1500, 2000]; | ||
| const TEST_STAKE: u64 = 30; // assume each validator has stake 30 (arbitrary number) |
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.
| const TEST_STAKE: u64 = 30; // assume each validator has stake 30 (arbitrary number) | |
| // assume each validator has stake 30 (arbitrary number) | |
| const TEST_STAKE: u64 = 30; |
nit: this seems like the more rusty way to do comments.
bls-cert-verify/src/cert_verify.rs
Outdated
| } | ||
|
|
||
| /// Verifies the [`signature`] of the [`payload`] which is signed by at most [`max_validators`] validators in the base2 encoded [`ranks`] using the [`rank_map`] to lookup the [`BlsPubkey`]. | ||
| fn get_vote_payloads(cert_type: &CertificateType) -> (Vote, Option<Vote>) { |
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.
| fn get_vote_payloads(cert_type: &CertificateType) -> (Vote, Option<Vote>) { | |
| fn get_vote_payloads(cert_type: CertificateType) -> (Vote, Option<Vote>) { |
nit: if a function takes something as a reference and then clones or copy it, it is better to expose that to the caller.
| /// # Returns | ||
| /// | ||
| /// On success, returns the total stake. | ||
| pub fn verify_certificate( |
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 had originally picked the name verify_cert_get_total_stake and I am happy with this renaming.
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 the original name was fine as well! Just thought to keep it simple.
bls-cert-verify/src/cert_verify.rs
Outdated
|
|
||
| /// Verifies the [`signature`] of the [`payload`] which is signed by at most [`max_validators`] validators in the base2 encoded [`ranks`] using the [`rank_map`] to lookup the [`BlsPubkey`]. | ||
| fn get_vote_payloads(cert_type: &CertificateType) -> (Vote, Option<Vote>) { | ||
| match *cert_type { |
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.
Factoring out this logic from the function does make it easier to read that function. Just a note that now we are having to create an additional Option and we have two branches instead of one. I doubt this is causing any perf hits but something to keep in mind if we feel like we need to squeeze more perf.
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 call. Fixed!
| // `expect` is safe because the `Vote` struct is composed entirely of primitive | ||
| // types (u64, Hash, enums) that are inherently serializable and it is constructed | ||
| // locally within this module. | ||
| bincode::serialize(vote).expect("Vote serialization should never fail for valid Vote structs") |
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 plan on introducing wincode serialization in bls sigverifier. I think I will refactor this at the same time when I do that.
Summary of Changes
Cleaning up the
solana-bls-cert-verifycrate.766cde8: Benches for the certificate verification logic exists in
solana-core, but I thought it made sense to add benches here too to identify any specific bottle necks related to certificate verification.735274f: Updated the public key aggregation logic to take advantage of the mixed point additions.
The
solana-bls-signaturesv3.0 crate added support for mixed point additions (anza-xyz/solana-sdk#506). Previously, we had to convert all points to projective before summing these together to aggregate public keys, but we can now add affine or even unserialized points directly to project points using a simpler group add formula.Using mixed point additions, the bench results seem to improve quite drastically:
I am glad that this gives quite a significant performance boost, but maybe we should have used mixed-point addition from the start.. 🙏
01c9687: For the base3 certificate verification, we have to aggregate two sets of public keys (pertaining to primary votes and fallback votes). Currently, these two sets are aggregated sequentially. If we aggregate these two sets in parallel, then we get decent performance improvements, so I applied this change.
The rest of the commits db2b01e, 315a77c, 8b3615f, f99187d are general refactoring and code quality improvements. In particular, I renamed the function
verify_cert_get_total_staketo justverify_certificatebecause that seemed cleaner and follows a more standard rust naming convention, but I can revert this if others are opposed.Finally, I added more precise bench logic in 6721329 to include:
Here are the results:
Distribution:
Roughly for 1000~1500 validators, the public key aggregation logic takes a little over 50% of the cost of verifying a certificate.
I created in issue #708 to cut down public key aggregation even further. If we take approach 2, then I think public key aggregation can almost be an order of magnitude faster. I'll tackle this issue on a follow-up.