iykyk-syn / unison

Consensus nodes performing in unison!
Apache License 2.0
29 stars 2 forks source link

rebro/gossip: merge Certificate into QuorumCertificate #16

Open abstraking opened 2 months ago

abstraking commented 2 months ago

We separated those for readability, but it increases the cognitive load, in fact. We can simplify and merge these interfaces into one

// QuorumCertificate maintains active Messages and Signatures for them by an arbitrary quorum. It accumulates 
// Messages  propagated over the Broadcaster network by quorum participants and maintains *local
// view* of Certificates.
//
// QuorumCertificate is mutable and append-only until it is finalized.
// It expects an arbitrary number of new Messages and Signatures to be added until finalization is triggered.
// The finalization conditions and quorums are implementation-specific.
type QuorumCertificate interface {
    // Add appends the given message to the Certificate 
    // performing application-specific verification.
    Add(Message) error
    // Get retrieves a Message kept on the Certificate by the MessageID.
    Get(MessageID) (Message, bool)
    // AddSignature appends the Signature for the Message.
    // Signature is expected to be verified beforehand by the Signer.
    // Reports true if enough signatures were collected for the Message.
    AddSignature(MessageID, crypto.Signature) (bool, error)
    // Signatures provides a list of all the signatures for a message by its ID.
    Signatures(MessageID) []crypto.Signature
    // Delete deletes Certificate by the MessageID of the committed MessageData.
    Delete(MessageID) bool
    // List provides all the Messages in the QuorumCertificate.
    List() []Message
    // Finalize attempts to finalize the QuorumCertificate.
    // It reports whether the finalization conditions were met.
    // The finalization conditions are defined by the implementation.
    // It may additionally perform expensive computations, like signature aggregation.
    Finalize() (bool, error)
}
abstraking commented 2 months ago

Maybe, we can also omit Qourum from the name

insiderbyte commented 2 months ago

True, I also thought about it today. Our quorum anyway aware of certificates and their finalization so merging makes sense to me

abstraking commented 2 months ago

Another thing I realized is that we can remove Finalize and make AddSingature report on finalization. Honestly, things like an aggregation of signatures can be done elsewhere by asking for Signatures on the certificate

insiderbyte commented 2 months ago

I'd leave finalization as it fits perfectly to the architecture and has such an ending point that could be useful in the future

abstraking commented 2 months ago

I feel like this is exactly the case of YAGNI and if we can finalize this, we should.