Organize BLS message into two types and reorder signature #91
Conversation
samkim-crypto
force-pushed
the
bls-message
branch
from
85c6bae to
9d3ab61
Compare
program/src/bls_message.rs
Outdated
| /// The message data | ||
| pub message_data: BLSMessageData, | ||
| /// BLS vote message to be sent all to all in Alpenglow | ||
| pub struct BLSVoteMessage { |
There was a problem hiding this comment.
Hmm, I'm generally fine with this PR. However, we will have BLS vote and certs on the same port. Are you saying we should put them into different ports? Or we should try deserialize as vote first, then try cert?
There was a problem hiding this comment.
Hm, I think it is fine to use the same ports, but the validator logic does need to process Vote and Certificate differently upon receiving them before verifying the signatures:
- on
Votemessage, fetch the corresponding BLS public key pertaining to the validator rank - on
Certificatemessage, build an aggregate BLS public key corresponding to the bit-vector
The certificate case is much more expensive compared to simple votes.
The certificate will also be generally larger due to the bit-vector. So maybe we can check the length to determine which type of BLS message it is. Excluding the signatures, Vote seems to be at most 82 bytes (8 bytes slot x 2 + 32 bytes hash x 2) while a certificate will contain:
- 1 byte for certificate type
- 8 bytes for slot number
- 32 bytes for block id
- 32 bytes for replayed bank hash
- variable length bit vector
for a total of 73 bytes excluding bit vector. So when we truncate the bit-vector we can make sure that the certificate message will be at least 83 bytes to make it easily distinguishable from a vote message.
I guess another simple option is to just add a byte identifier at the beginning of Vote and Cert messages to easily distinguish between the two.
There was a problem hiding this comment.
Which is why I made it enum in the first place. I think relying on the size is a little bit shady, as we might invent smart ways to make certificate small later. Any problem you can see with enum?
@0x0ece what do you think? What's more convenient for Jump?
There was a problem hiding this comment.
I would still advocate having two separate types for vote and certificate since many functions make sense only for one of vote or certificate. For example, vote_count only makes sense for a certificate message, but we need to define it for BLSMessage as of right now (considered defining it for the Certificate type, but it is a bit awkward...). We can still add a byte to indicate which type of BLS message it is (in this case, it would be the same serialization as using enums, but just using different types in rust). Or I think it also makes sense to just send it to different ports without the byte.
But I would also be interested to hear what @0x0ece thinks as well though this is more of a rust type issue than an encoding issue.
There was a problem hiding this comment.
I'm fine with that, is it possible to define the extra byte in here (and define serialize/deserialize here if possible) so we don't get out of sync?
There was a problem hiding this comment.
Okay, I merged signatures into each of the VoteMessageData (now called VoteMessage) and Certificate. We still have a single BLSMessage, but since signatures are part of VoteMessage and Certificate, it will be easier to create dedicated functions for each of these types.
…ier organization
samkim-crypto
force-pushed
the
bls-message
branch
from
9d3ab61 to
5e64142
Compare
program/src/bls_message.rs
Outdated
| Vote(BLSMessageVoteData), | ||
| Vote(VoteMessage), | ||
| /// Certificate message | ||
| Certificate(Certificate), |
There was a problem hiding this comment.
To be consistent, do you want to rename Certificate(Certificate) to Certificate(CertificateMessage) ?
There was a problem hiding this comment.
Okay, I divided Certificate into Certificate excluding the signature and bit vector and then CertificateMessage, which includes them. I think this is the most consistent with Vote. Let me know what you think.
I did realize that we would ultimately need to make additional modifications to the Certificate format for the fallback certificates where we use a base3 encoding to signify the validators that signed.
There was a problem hiding this comment.
Hmm, can we just pack the base3 encoding into the bitvec? Do we need other data structures?
There was a problem hiding this comment.
Yeah the encoding itself will be the same. At the end of the day, BitVec is just encoded in the same way as Vec<u8> (in bincode, length prefix followed by bytes). A base3 encoding should also be encoded the same in binary.
We will probably need to replace BitVec with a dedicated type for base3 encoding though so that we have a nice interface to easily add/remove base3 elements and to also check which elements have been added. This is just for fallback/skip certificates.
Alternatively, we can replace BitVec with just Vec<u8> here. Then in consumer logic (in alpenglow), we can just interpret/convert Vec<u8> into a base3 encoding type before we do logic.
There was a problem hiding this comment.
@0x0ece what do you think? Which is more convenient for Firedancer? I think Vec is probably easier?
Currently, the type BLSMessage can be either a vote message or a certificate message. However, the processing logic for these two types of messages are verify different and we generally need to write functions for one type or the other. It is much more convenient if these two types of BLS messages is formally split into two types BLSVoteMessage and BLSCertificateMessage.
I split the BLSMessage into the two types. I also moved the signature field so that it precedes the vote and certificate data. Since the certificate data contains the bit vector, it would be much easier to truncate the bit vector if it comes last in the message format.