-
Notifications
You must be signed in to change notification settings - Fork 771
Simplex QuorumCertificate and BLS aggregator #4091
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?
Conversation
simplex/qc.go
Outdated
|
||
bytes, err := Codec.Marshal(CodecVersion, serializedQC) | ||
if err != nil { | ||
panic(fmt.Errorf("failed to marshal QC: %w", err)) |
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.
todo: don't panic, update the simplex interface to allow an error return 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.
If we replace the codec usage with canoto, then we won't need to change the interface
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 didn't review the tests yet, will look though the tests after these are addressed.
simplex/qc.go
Outdated
|
||
bytes, err := Codec.Marshal(CodecVersion, serializedQC) | ||
if err != nil { | ||
panic(fmt.Errorf("failed to marshal QC: %w", err)) |
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.
If we replace the codec usage with canoto, then we won't need to change the interface
simplex/qc.go
Outdated
type SerializedQC struct { | ||
Sig []byte `serialize:"true"` | ||
Signers []simplex.NodeID `serialize:"true"` | ||
} |
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.
Why do we serialize the full list of nodeIDs? Shouldn't we be using a bitset to indicate participation similarly to warp?
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.
using a bitset would be nice, but the implementation feels kind of gross. I'd need to store a canonical ordering of nodes in the blsVerifier
to deserialize the bitset, as well as a mapping of nodeIDs to indices in the QC
in order to serialize.
I noticed warp has a IndexedValidator
for creating the bitset, and some helper functions to create a canonical ordering to read the bitset. Is all this complexity needed just to reduce the size of the serializedQC? Even if we have 100 nodes thats only 2000 bytes or ~.002 mbs potentially saved
// Verify checks if the quorum certificate is valid by verifying the aggregated signature against the signers' public keys. | ||
func (qc *QC) Verify(msg []byte) error { | ||
pks := make([]*bls.PublicKey, 0, len(qc.signers)) | ||
quorum := simplex.Quorum(len(qc.verifier.nodeID2PK)) | ||
if len(qc.signers) != quorum { | ||
return fmt.Errorf("%w: expected %d signers but got %d", errUnexpectedSigners, quorum, len(qc.signers)) | ||
} |
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.
Are there checks somewhere else that verify that the same signer isn't included multiple times?
If so we should document that assumption here. If not we need to add that.
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.
Simplex checks for:
During the regular Simplex admission path.
Though now when I quickly skimmed through the code, I think we can process an un-verified notarization through the replication path, oops... @samliok can you confirm?
I think we should be prudent and double check this here in the avalanchego side as well.
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.
So a couple notes after looking a bit deeper.
- We do check if multiple nodes have signed twice in simplex see here. Although it's probably a better idea to do it here in Verify(and potentially remove the check in simplex)?
- We don't verify notarizations/finalizations through the replication path. Created an issue
- Noticed we don't add re-add replication task if block verification fails. Issue
- We may possible add a signature for the wrong digest when creating a notarization. Issue
I'll tackle these issues on this simplex side, plus added checking for double signers in avalanchego.
simplex/qc.go
Outdated
} | ||
|
||
type SerializedQC struct { | ||
Sig []byte `serialize:"true"` |
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.
By using []byte
for the signature, the serialized format will need to include the length of the signature. (or if we switch this to canoto, will at least require us to verify the length).
This is why in warp we specify the length here explicitly:
Sig []byte `serialize:"true"` | |
Sig [bls.SignatureLen]byte `serialize:"true"` |
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.
it weird though bls.SignatureToBytes(*bls.Signature) []byte
doesn't return a fixed length byte array. Shouldn't it always return an array of bls.SignatureLen
🤷
simplex/qc.go
Outdated
// Aggregate aggregates the provided signatures into a quorum certificate. | ||
// It requires at least a quorum of signatures to succeed. | ||
// If any signature is from a signer not in the membership set, it returns an error. | ||
func (a SignatureAggregator) Aggregate(signatures []simplex.Signature) (simplex.QuorumCertificate, 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.
This probably doesn't need to include any handling of duplicates, as it should be unexpected for simplex to provide duplicates here (imo).
if len(signatures) < quorumSize { | ||
return nil, fmt.Errorf("%w: expected %d signatures but got %d", errUnexpectedSigners, quorumSize, len(signatures)) | ||
} |
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.
Is this defence-in-depth? I wouldn't expect simplex to ever call this function without sufficient signers.
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.
yep, simplex checks the signers before calling aggregate
Let's double check the QC is valid (no double signing, enough signers) here as well.
Why this should be merged
Implements the simplex
QuorumCertificate
,QCDeserializer
andSignatureAggregator
interfaces. This allows simplex to parse, aggregate and verify quorum certificates(ex. finalizations and notarizations) during execution.How this works
Codec
How this was tested
Added unit tests to
qc_test.go
.Need to be documented in RELEASES.md?
no