[votor] Replace `VoteCertificate` with either `VoteCertificateBuilder` or `CertificateMessage` #304

samkim-crypto · 2025-07-22T03:00:16Z

Problem

The existing BLS vote certificate logic had an inefficiency related to costly type conversions during signature aggregation. The VoteCertificate type, a wrapper around CertificateMessage, stored its signature in the compact "affine" form. However, because signature aggregation is performed in "projective" form, each call to aggregate required the existing signature to be converted from affine to projective, incurring a non-trivial performance cost on a critical path.

Summary of Changes

To address this, I added a VoteCertificateBuilder type which holds the certificate signature in its projective form. This results in much cleaner and more performance aggregation function. Once all votes are aggregated, VoteCertificateBuilder::build can be called to produce the final CertificateMessage, converting the signature back to the compact affine form only once at the end.

With the new builder, I removed the redundant VoteCertificate, as its role is now cleanly handled by VoteCertificateBuilder for aggregation and CertificateMessage for the final product.

Additionally, I added both checked (aggregate) and unchecked (aggregate_unchecked) versions of the aggregation function. Since signatures are pre-verified during the sigverify phase, the consensus logic can use the more performant unchecked version.

Fixes #

wen-coding · 2025-07-22T03:03:33Z

votor/src/certificate_pool/vote_certificate.rs

-        *signature = Signature::from(current_signature);
-        Ok(())
+        let signature_iter = messages.iter().map(|vote_message| &vote_message.signature);
+        Ok(self.signature.aggregate_with(signature_iter)?)


Sorry I forgot to ask, do you think this function should really go into CertificateMessage?

Sorry are you stating that the aggregate function should go into CertificateMessage instead of VoteCertificate (now VoteCertificateBuilder) or are you stating that maybe we should remove aggregate from votor entirely?

The former, now all we need from VoteCertificate is the aggregate function, I wonder whether we should just move that into BLSMessage::CertificateMessage in the vote program and remove VoteCertificate entirely. What do you think?

Oh yeah this is actually what I just addressed in this PR.

I removed VoteCertificate and replaced it with CertificateMessage in most places. For example see here.

But I did add VoteCertificateBuilder (maybe I can rename this to CertificateMessageBuilder instead) that is primarily used for signature aggregation. The issue is that CertificateMessage stores the signature in a more compact Signature form while signature aggregation can only be done in SignatureProjective form. So the previous aggregation function took the Signature type, converted it to SignatureProjective, then convert it back to Signature type after aggregation.

The VoteCertificateBuilder now just holds the SignatureProjective type from the start. Once the signatures are all aggregated, then it can use the build function to create CertificateMessage. It should be more efficient this way since it removes unnecessary costs related to type conversions.

I was planning on writing a detailed PR description, but I just wanted to check if the CI check passed first.

wen-coding · 2025-07-22T15:48:26Z

votor/src/certificate_pool.rs

    #[cfg(test)]
    pub fn slot_notarized_fallback(&self, slot: Slot) -> Option<usize> {
+        // TODO: remove this line once `CertificateMessage` is moved into the repo`
+        use crate::certificate_pool::vote_certificate::get_vote_count;


I don't think we need get_vote_count any more, it was only used in tests and those tests have since been removed, I'm removing it in #300

Okay great! I removed this during the rebase.

wen-coding · 2025-07-22T15:50:40Z

votor/src/certificate_pool.rs

            .find_map(|(cert_id, cert)| {
                matches!(cert_id, CertificateId::NotarizeFallback(s,_,_) if *s == slot)
-                    .then_some(cert.vote_count())
+                    .then_some(get_vote_count(cert))


We don't need to return vote count here as I mentioned above, that test only checks if the certificate exists.

wen-coding · 2025-07-22T15:52:40Z

votor/src/certificate_pool/vote_certificate.rs

+/// A builder for creating a `CertificateMessage` by efficiently aggregating BLS signatures.
 #[derive(Clone)]
-pub struct VoteCertificate(CertificateMessage);
+pub struct VoteCertificateBuilder {


Nice, let's change the file name as well.

wen-coding · 2025-07-22T15:53:40Z

votor/src/certificate_pool/vote_certificate.rs

-        // aggregate the votes
-        let bitmap = &mut self.0.bitmap;
+    /// Aggregates a slice of `VoteMessage`s into the builder.
+    #[allow(dead_code)]


If we will only use aggregate_unchecked in the future, we can remove this one.

Related to your comment below, I just removed the unchecked version and kept the safe one. A potential panic will now happen in add_to_certificate.

wen-coding · 2025-07-22T15:57:16Z

votor/src/certificate_pool/vote_pool.rs

-        output.aggregate(self.vote_entry.transactions.iter())?;
-        Ok(())
+    pub fn add_to_certificate(&self, output: &mut VoteCertificateBuilder) {
+        output.aggregate_unchecked(&self.vote_entry.transactions)


Instead of calling a function which panics inside, can we use output.aggregate which returns an error and panic here? A bit easier for constructing tests.

carllin · 2025-07-23T03:05:36Z

votor/src/certificate_pool/vote_pool.rs

+    pub fn add_to_certificate(&self, output: &mut VoteCertificateBuilder) {
+        output
+            .aggregate(&self.vote_entry.transactions)
+            .expect("Incoming vote message signatures are assumed to be valid")


just double checking, we've made all the appropriate checks in the vote pool such that the two errors in aggregate can't happen right?

alpenglow/votor/src/certificate_pool/vote_certificate_builder.rs

Line 64 in eba6a91

return Err(CertificateError::ValidatorDoesNotExist(

and

alpenglow/votor/src/certificate_pool/vote_certificate_builder.rs

Line 71 in eba6a91

Ok(self.signature.aggregate_with(signature_iter)?)

Yes, they should not happen unless our check was screwed up, in that case we can panic.

Yeah both of these checks should be guaranteed by the sigverify stage.

If the sigverify stage cannot find a valid public key to verify the signature, then it will discard the vote or cert packet

The only way the aggregate_with function will fail is if the signature is invalid. The sigverify stage will obviously guarantee this as well

…rtificateMessage`

…` or `CertificateMessage` (anza-xyz#304)

wen-coding reviewed Jul 22, 2025

View reviewed changes

samkim-crypto marked this pull request as ready for review July 22, 2025 13:41

samkim-crypto requested review from AshwinSekar, carllin and wen-coding July 22, 2025 13:41

wen-coding reviewed Jul 22, 2025

View reviewed changes

samkim-crypto force-pushed the bls-cert branch from ddf1e30 to eba6a91 Compare July 23, 2025 02:02

wen-coding approved these changes Jul 23, 2025

View reviewed changes

carllin reviewed Jul 23, 2025

View reviewed changes

samkim-crypto added 3 commits July 28, 2025 22:32

replace VoteCertificate with either VoteCertificateBuilder or `Ce…

cd96f90

…rtificateMessage`

rename vote_certificate.rs to vote_certificate_builder.rs

8c453b4

fix error from rebase

659f588

samkim-crypto force-pushed the bls-cert branch from eba6a91 to 659f588 Compare July 28, 2025 13:43

samkim-crypto merged commit 70e4858 into anza-xyz:master Jul 28, 2025
7 checks passed

wen-coding pushed a commit to wen-coding/alpenglow that referenced this pull request Jul 29, 2025

[votor] Replace VoteCertificate with either `VoteCertificateBuilder…

d9db2ba

…` or `CertificateMessage` (anza-xyz#304)

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 1, 2025

[votor] Replace VoteCertificate with either `VoteCertificateBuilder…

efe3238

…` or `CertificateMessage` (anza-xyz#304)

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 1, 2025

[votor] Replace VoteCertificate with either `VoteCertificateBuilder…

8e5f9f0

…` or `CertificateMessage` (anza-xyz#304)

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 1, 2025

[votor] Replace VoteCertificate with either `VoteCertificateBuilder…

e1275af

…` or `CertificateMessage` (anza-xyz#304)

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 1, 2025

[votor] Replace VoteCertificate with either `VoteCertificateBuilder…

74f3c37

…` or `CertificateMessage` (anza-xyz#304)

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 1, 2025

[votor] Replace VoteCertificate with either `VoteCertificateBuilder…

0c15787

…` or `CertificateMessage` (anza-xyz#304)

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 1, 2025

[votor] Replace VoteCertificate with either `VoteCertificateBuilder…

744fd59

…` or `CertificateMessage` (anza-xyz#304)

bw-solana pushed a commit to bw-solana/alpenglow that referenced this pull request Aug 2, 2025

[votor] Replace VoteCertificate with either `VoteCertificateBuilder…

f3707c3

…` or `CertificateMessage` (anza-xyz#304)

Uh oh!

[votor] Replace VoteCertificate with either VoteCertificateBuilder or CertificateMessage #304

[votor] Replace VoteCertificate with either VoteCertificateBuilder or CertificateMessage #304

Uh oh!

Conversation

samkim-crypto commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Summary of Changes

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

[votor] Replace `VoteCertificate` with either `VoteCertificateBuilder` or `CertificateMessage` #304

[votor] Replace `VoteCertificate` with either `VoteCertificateBuilder` or `CertificateMessage` #304

samkim-crypto commented Jul 22, 2025 •

edited

Loading