ssvlabs / ssv-spec

GNU General Public License v3.0
25 stars 22 forks source link

Replace redundant BLS in QBFT with RSA #372

Closed MatheusFranco99 closed 4 months ago

MatheusFranco99 commented 5 months ago

Overview

This PR adds an RSA check in the Validator.ProcessMessage function and drops redundant BLS signature checks in QBFT.

Changes

Improvement

New total duty time per number of consensus rounds.

1 Round 2 Rounds 3 Rounds
67% 70% 71%

New consensus n-th round time.

1st Round 2nd Round 3rd Round
21% 73% 73%
olegshmuelov commented 5 months ago

since https://github.com/bloxapp/ssv-spec/pull/342 was merged already

can we fix small issue in the current pr?

GalRogozinski commented 5 months ago

since #342 was merged already

can we fix small issue in the current pr?

This PR doesn't change runner at all... Maybe in a subsequent PR when we change runner

olegshmuelov commented 5 months ago

since #342 was merged already can we fix small issue in the current pr?

This PR doesn't change runner at all... Maybe in a subsequent PR when we change runner

created an issue https://github.com/bloxapp/ssv-spec/issues/376

GalRogozinski commented 4 months ago

Ok a few final things after heated discussion @MatheusFranco99 :

After more discussions this is the final structure decided:

type BeaconSigner interface {
    // SignBeaconObject returns signature and root.
    SignBeaconObject(obj ssz.HashRoot, domain spec.Domain, pk []byte, domainType spec.DomainType) (Signature, [32]byte, error)
    // IsAttestationSlashable returns error if attestation is slashable
    IsAttestationSlashable(pk []byte, data *spec.AttestationData) error
    // IsBeaconBlockSlashable returns error if the given block is slashable
    IsBeaconBlockSlashable(pk []byte, slot spec.Slot) error
}

// SSVSigner used for all SSV specific signing
type SSVOperatorSigner interface {
    SignSSVMessage(data []byte, pk []byte) ([]byte, error)
}

type SSVShareSigner interface {
    SignRoot(data Root, sigType SignatureType, pk []byte) (Signature, error)
}

// KeyManager is an interface responsible for all key manager functions
type KeyManager interface {
    BeaconSigner
    SSVShareSigner
    // AddShare saves a share key
    AddShare(shareKey *bls.SecretKey) error
    // RemoveShare removes a share key
    RemoveShare(pubKey string) error
}

Explanation: We don't want to force a single object to hold both RSA and BLS keys even if they are used for the same purpose (authenticating an operator). So we seperate the interfaces

KeyManager is the BLSkey manager. It is less important to keep but it isn't a mistake to keep it as far as I am concerned. So for the sake of less changes it stays.

GalRogozinski commented 4 months ago

Also we need to maybe add an RSAVerify to MsgProcessing test on the broadcasted message to ensure that messages were signed correctly

GalRogozinski commented 4 months ago

Also we need to drop the double verification at the p2p layer now...

y0sher commented 4 months ago

Q: are we signing rsa in consensus ? If we are verifying the we should probably also sign.

GalRogozinski commented 4 months ago

Q: are we signing rsa in consensus ? If we are verifying the we should probably also sign.

In spec we sign in validator.go In impl from my POV you should sign where you pass all spec tests