-
Notifications
You must be signed in to change notification settings - Fork 108
feature/pos-messaging-schema #570
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
feature/pos-messaging-schema #570
Conversation
* Add p2p msgs for votes and timeouts * Implement remaining tests * Add ProposedInView field to vote message * Cleanup * Fix failing TestEnumExtras unit test * Comments * Add validators list to aggregated signature * Fix naming for QC type * Use io.reader * Prefix new file names with 'pos_' * Add deep equality check functions to new types * Add ECDSA public key to new message types * Address Nina's comments * Address nits
* Initial commit * Add tests * Fix broken tests * Better comments and edge case handling * Revert unnecessary comments * Address Nina's feedback * Fix postgres tests * Fix compile error when the relic build tag isn't defined * Address Piotr's comments * Address Nina's comments
* Implement new Bitset data structure * Update bitset constructor to return a pointer * Better comments * Address PR comments
* Migreate AggregateQC to use bitset * Add tests * Cleanup * Fix broken test * Address PR feedback
* Add BLS PK and signature to BlockProducerInfo * Add versioning for BlockProducerInfo * Implement byte encoder and decoder * Add tests * Address nits * Update BlockProducerInfo public key field to PublicKey type * Restrict BlockProducerInfo byte encoding/decoding to version 1 * Rename BlockProducerInfo to MsgDeSoBlockProducerInfo
* rename-bitset-parent-package-to-collections * Fix Dockerfiles * Better comments for Bitset encoding * Gracefully handle nil bitset encoding
* sa/pos-messaging-schema-cleanup * Better comments
* sa/add-bls-signature-verification-for-multiple-payloads * Rename signature verification functions * Make hashingAlgorithm and signingAlgorithm private to the bls package * Fix gofmt error * Address Nina's feedback
* sa/add-block-propser-keys-and-signature-to-block-header * Update tests and block encoding * Add comment to hash function * Simplify hash function for PoS block header * Clean up comments * Address Nina's comments * Add unit test for signature encoding * Add test case for expected hash
2d91cf0 to
bee6e13
Compare
bee6e13 to
603f525
Compare
| // VerifyAggregateSignatureMultiplePayloads takes in a slice of bls.PublicKeys, a bls.Signature, and a slice of payloads. | ||
| // It returns true if each bls.PublicKey at index i has signed its respective payload at index i in the payloads slice. | ||
| // The input bls.Signature is the aggregate signature of each public key's partial bls.Signatures for its respective payload. | ||
| func VerifyAggregateSignatureMultiplePayloads(publicKeys []*PublicKey, signature *Signature, payloadsBytes [][]byte) (bool, error) { |
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.
Hmm... I had a question about this. When I imagined it, it seemed like the signature-verifying function would take a [][]*PublicKey to parallel the payloadBytes. But here I see it's only a single list of public keys.
If this is the case, then how would verification of the QCs work? My understanding was we'd have a situation like the following:
- HighQC1 is signed by {pk1, pk2, pk3}
- HighQC2 is signed by {pk4, pk5}
- HighQC3 is signed by {pk6, pk7, pk8}
In which case I would expect something more like the following:
publicKeys := [][]*PublicKey{
{pk1, pk2, pk3},
{pk4, pk5}
{pk6, pk7, pk8}
}
payloads := [][]byte{
HighQC1,
HighQC2,
HighQC3
}
verified, err := VerifyAggregateSignatureMultiplePayloads(publicKeys, signature, payloads)
I think I'm missing something though. Lmk what you think.
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 see. Is the intention here compression of the payloads? Or is there a limitation in BLS signature verification that requires payload deduplication?
My understanding of all of this is purely based off the UBC researchers' reference implementation here: https://github.com/deso-protocol/hotstuff_pseudocode/blob/diamondhands/REV-02-Jun-06-2023/fast_hotstuff_bls.go#L186.
So I may be missing something too.
My understanding is that, whenever each of those public keys times out, it doesn't sign its high QC, but rather its high QC's view.
Using your example with pk1, pk2, pk3... pk8, when each public key times out for view N, it signs the following:
- pk1 signs the pair <N, HighQC1.View>
- pk2 signs the pair <N, HighQC1.View>
- pk3 signs the pair <N, HighQC2.View>
- ...
- pk8 signs the pair <N, HighQC3.View>
When the leader aggregates a timeout QC for view N, it constructs the timeout QC as follows:
- highestQC = highest QC found from the timeout messages from the timeout messages from the public keys above
- publicKeys = [pk1, pk2, pk3, pk4, pk5, pk6, pk7, pk8]
- all of those public keys have timed out
- payloads = [HighQC1.View, HighQC1.View, HighQC1.View, HighQC2.View, HighQC2.View, HighQC3.View, HighQC3.View, HighQC3.View]
- the index of each element in this slice corresponds to the index of the public key that sent it
- signature = aggregation of all of the partial signatures from pk1, pk2, pk3,...
Then we just call VerifyAggregateSignatureMultiplePayloads(publicKeys, signature, payloads) to verify the aggregate signature.
--
I don't have an opinion on whether each pk signs its high QC vs its high QC's view. My assumption is that the theory behind the safety of signing the high QC view is sound. If that's the case, then signing the view number as above would work with BLS, and results in much smaller aggregate QC sizes.
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.
OK I get what you're doing now. Your understanding of what we need to do is correct, I was just imagining a different schema that "deduplicates" the payload, if that makes sense.
Basically, what you're saying is you aggregate a bunch of (PublicKey, payload) pairs, which I think is the most elegant thing to do. The less elegant thing that I was suggesting is we aggregate ([]PublicKey, payload) pairs, where each tuple will have a unique payload plus all the public keys that signed that particular unique payload. This amounts to the same thing, just grouped differently. Lmk if this doesn't make sense.
I think the way you have it is how we should keep it. It's technically more redundant because we repeat the same view number a lot in ValidatorsTimeoutHighQCViews but we can compress it easily later if it ever becomes an issue (e.g. when we have 10k+ validators or something...).
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.
Correct. And you're right that the payloads are duplicated in my current scheme. I'm actually aligned with you that deduplicating should be the end-game of what we want to implement.
What's interesting is that, even if we do the deduplication as you suggest, we'll still need to unravel the public keys and payloads into individual (publickey, payload) pairs to do the BLS signature verification. This means that the in-memory TimeoutAggregateQuorumCertificate struct's schema is best left as-is.
This also means that the biggest value of deduplicating the payloads is the compression it provides when sending timeout QCs as network messages.
What's nice about the above is that, it means we can implement the compression in the ToBytes/FromBytes functions for the timeout message without changing the struct fields at all. When we get to the point where we want to compress the views, we can increment the MsgDeSoHeader version and implement new byte encoding that has the compression. This would allow us to introduce the networking change without ever having to touch the consensus logic.
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.
Just read through this and I love the idea of pushing the compression into the ToBytes/FromBytes. That actually seems like the perfect place to put it because its benefit is to the network layer, no to the actual logic like you mention.
| NetworkType_TESTNET NetworkType = 2 | ||
| ) | ||
|
|
||
| type MsgDeSoHeaderVersion = uint32 |
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.
Nit: I saw this the first time and thought it's probably fine, but isn't the more normal syntax type MsgDeSoHeaderVersion uint32 with no =?
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.
You're right. Ideally we define the type as type MsgDeSoHeaderVersion uint32 without the =.
There's actually a fundamental difference between the two that makes the current definition with the = easier to work with for us:
- Defining the type with the
=defines it as a type alias foruint32. The two are interchangeable, and values of either type can be assigned to the other. The two types are equivalent which means that all of our strongly typed uint32 byte encoding/decoding functions work as-is - Defining the type without the
=defines it as a brand new 4 byte unsigned integer type. Go's type system sees it as a distinct type fromuint32type.- The advantage of defining the new type is that the type system enforces usage of just the valid values of the new type. You can't assign any uint32 to the version field
- The downside is that we would have to implement new byte encoders and decoders for every newly defined type. Or we would have to unsafe casting when encoding and decoding it
So, it seems like defining it as a type alias is the less bad approach. LMK though
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.
Ah wow I just read through this. Didn't know the type keyword worked like that, but makes sense. Great that we didn't have to rewrite all the code.
Summary of changes in this PR:
MsgDeSoValidatorVotemessage schemaMsgDeSoValidatorTimeoutmessage schemaBitsetData Structure ImplementationMsgDeSoHeadermessage schemaSha256DoubleHashblspackage