SignedSSVMessage #374
SignedSSVMessage #374
Conversation
|
@y0sher I changed the encoding to match with the implementation one |
p2p/validation/msg_validation.go
Outdated
| type MessageValidator interface { | ||
| ValidateSignedSSVMessage(signedSSVMessage *types.SignedSSVMessage) error | ||
| ValidateSignature(signedSSVMessage *types.SignedSSVMessage) error | ||
| } |
There was a problem hiding this comment.
Add comments:
- Open a github issue that we need to add spec for MsgValidation and reference for
ValidateSignedSSVMessage. Comment with the link to the issue. - For
ValidateSignaturecomment that it is an RSA sig with PKCSv1.5 padding. You can link to the RFC... I think the link I gave is good, you can double-check 😛
types/messages.go
Outdated
| type SignedSSVMessage struct { | ||
| OperatorID OperatorID | ||
| Signature []byte `ssz-max:"512"` // Created by the operator's private key. Max size allow keys up to 512*8 = 4096 bits | ||
| Data []byte `ssz-max:"6291893"` // Max size extracted from SSVMessage | ||
| } |
There was a problem hiding this comment.
signature is the first field in impl
let's do it exactly the same...
Also remove the ssz notations
types/messages_encoding.go
Outdated
| // Field (0) 'OperatorID' | ||
| s.OperatorID = OperatorID(ssz.UnmarshallUint64(buf[0:8])) | ||
|
|
||
| // Offset (1) 'Signature' | ||
| if o1 = ssz.ReadOffset(buf[8:12]); o1 > size { | ||
| return ssz.ErrOffset | ||
| } | ||
|
|
||
| if o1 < 16 { | ||
| return ssz.ErrInvalidVariableOffset | ||
| } | ||
|
|
||
| // Offset (2) 'Data' | ||
| if o2 = ssz.ReadOffset(buf[12:16]); o2 > size || o1 > o2 { | ||
| return ssz.ErrOffset | ||
| } | ||
|
|
||
| // Field (1) 'Signature' | ||
| { | ||
| buf = tail[o1:o2] | ||
| if len(buf) > 512 { | ||
| return ssz.ErrBytesLength | ||
| } | ||
| if cap(s.Signature) == 0 { | ||
| s.Signature = make([]byte, 0, len(buf)) | ||
| } | ||
| s.Signature = append(s.Signature, buf...) | ||
| } | ||
|
|
||
| // Field (2) 'Data' | ||
| { | ||
| buf = tail[o2:] | ||
| if len(buf) > 6291893 { | ||
| return ssz.ErrBytesLength | ||
| } | ||
| if cap(s.Data) == 0 { | ||
| s.Data = make([]byte, 0, len(buf)) | ||
| } | ||
| s.Data = append(s.Data, buf...) | ||
| } | ||
| return err | ||
| } | ||
|
|
||
| // SizeSSZ returns the ssz encoded size in bytes for the SignedSSVMessage object | ||
| func (s *SignedSSVMessage) SizeSSZ() (size int) { | ||
| size = 16 | ||
|
|
||
| // Field (1) 'Signature' | ||
| size += len(s.Signature) | ||
|
|
||
| // Field (2) 'Data' | ||
| size += len(s.Data) | ||
|
|
||
| return | ||
| } | ||
|
|
||
| // HashTreeRoot ssz hashes the SignedSSVMessage object | ||
| func (s *SignedSSVMessage) HashTreeRoot() ([32]byte, error) { | ||
| return ssz.HashWithDefaultHasher(s) | ||
| } | ||
|
|
||
| // HashTreeRootWith ssz hashes the SignedSSVMessage object with a hasher | ||
| func (s *SignedSSVMessage) HashTreeRootWith(hh ssz.HashWalker) (err error) { | ||
| indx := hh.Index() | ||
|
|
||
| // Field (0) 'OperatorID' | ||
| hh.PutUint64(uint64(s.OperatorID)) | ||
|
|
||
| // Field (1) 'Signature' | ||
| { | ||
| elemIndx := hh.Index() | ||
| byteLen := uint64(len(s.Signature)) | ||
| if byteLen > 512 { | ||
| err = ssz.ErrIncorrectListSize | ||
| return | ||
| } | ||
| hh.PutBytes(s.Signature) | ||
| hh.MerkleizeWithMixin(elemIndx, byteLen, (512+31)/32) | ||
| } | ||
|
|
||
| // Field (2) 'Data' | ||
| { | ||
| elemIndx := hh.Index() | ||
| byteLen := uint64(len(s.Data)) | ||
| if byteLen > 6291893 { | ||
| err = ssz.ErrIncorrectListSize | ||
| return | ||
| } | ||
| hh.PutBytes(s.Data) | ||
| hh.MerkleizeWithMixin(elemIndx, byteLen, (6291893+31)/32) | ||
| } | ||
|
|
||
| hh.Merkleize(indx) | ||
| return | ||
| } | ||
|
|
||
| // GetTree ssz hashes the SignedSSVMessage object | ||
| func (s *SignedSSVMessage) GetTree() (*ssz.Node, error) { | ||
| return ssz.ProofTree(s) | ||
| } |
There was a problem hiding this comment.
won't this be regenrated automatically?
we either need to make an exception for the type or move it to a file that won't cause automatic generation
There was a problem hiding this comment.
It is not automatically generated because the encoding of the messages.go file is not included in the generate file. Adding a remark anyway.
| ) | ||
|
|
||
| // Valid tests a valid SignedSSVMessageTest | ||
| func Valid() *SignedSSVMessageTest { |
There was a problem hiding this comment.
Now that you added MessageValidator maybe sig tests should be in MessageValidatorTest... (edit)
The issue is since our MessageValidator isn't aligned with implementation not sure how this will work out for the SSV team.
wdyt @y0sher and @moshe-blox
There was a problem hiding this comment.
I agree a lot has to be done regarding Message Validation. Plus, tests for it and SignedSSVMessage are related. But I think we could do it in a later PR, since (if I'm not mistaken) it's not a top priority and we're overheaded with other tasks.
There was a problem hiding this comment.
@GalRogozinski @MatheusFranco99
I think this test should just use message validator as a blackbox, this means this test should check that all the behavior we expect to happen or not happen based on the msg validator response should be tested under SignedSSVMsg tests, all the stuff regarding message validation (signatures and other validations) should happen on its own place.
For this test, what message validator did doesn't matter , it should just test what it expects to see if message validator returned valid/invalid. could use a mock imo.
p2p/validation/msg_validation.go
Outdated
| // To-Do: better spec the intended Message Validation behavior. Issue #375: https://github.com/bloxapp/ssv-spec/issues/375 | ||
| type MessageValidator interface { | ||
| ValidateSignedSSVMessage(runner ssv.Runner, signedSSVMessage *types.SignedSSVMessage) error | ||
| ValidateSignature(runner ssv.Runner, signedSSVMessage *types.SignedSSVMessage) error // Verifies the message's RSA signature with PKCSv1.5 encoding. Ref: https://datatracker.ietf.org/doc/html/rfc8017#section-8.2.2 |
There was a problem hiding this comment.
| ValidateSignature(runner ssv.Runner, signedSSVMessage *types.SignedSSVMessage) error // Verifies the message's RSA signature with PKCSv1.5 encoding. Ref: https://datatracker.ietf.org/doc/html/rfc8017#section-8.2.2 | |
| VerifySignature(runner ssv.Runner, signedSSVMessage *types.SignedSSVMessage) error // Verifies the message's RSA signature with PKCSv1.5 encoding. Ref: https://datatracker.ietf.org/doc/html/rfc8017#section-8.2.2 |
There was a problem hiding this comment.
I don't object
I just want to admit that I use the words as synonyms and never understood the difference
p2p/validation/msg_validation.go
Outdated
| type MessageValidator interface { | ||
| ValidateSignedSSVMessage(runner ssv.Runner, signedSSVMessage *types.SignedSSVMessage) error | ||
| ValidateSignature(runner ssv.Runner, signedSSVMessage *types.SignedSSVMessage) error | ||
| ValidateSignature(runner ssv.Runner, signedSSVMessage *types.SignedSSVMessage) error // Verifies the message's RSA signature with PKCSv1.5 encoding. Ref: https://datatracker.ietf.org/doc/html/rfc8017#section-8.2.2 |
There was a problem hiding this comment.
nit, can you put the comment above the function please?
Overview
Introduce
SignedSSVMessage, which encapsulatesSSVMessagealong with asignatureand a senderidentifierfields.Changes
SignedSSVMesagestructure intypesSignedSSVMesageDecodePubsubMsgnow returns aSignedSSVMesageMsgValidationadapted to receive aSignedSSVMesageinstead of aSSVMessageNote: though
SSVMessageis no longer the transmitted message in the network, theRunnerstill processes the typeSSVMessagein theProcessMessagefunction.Tests
Note: this is a non-fork version of #345