SignedSSVMessage

[MatheusFranco99]

Overview

Introduce SignedSSVMessage, which encapsulates SSVMessage along with a signature and a sender identifier fields.

Changes

• New SignedSSVMesage structure in types
• DecodePubsubMsg now returns a SignedSSVMesage
• MsgValidation adapted to receive a SignedSSVMesage instead of a SSVMessage.

Note: though SSVMessage is no longer the transmitted message in the network, the Runner still processes the type SSVMessage in the ProcessMessage function.

Tests

• [x] Encoding
• [x] Valid SignedSSVMessage
• [x] Invalid SignedSSVMessage with no data
• [x] Invalid SignedSSVMessage with empty signature
• [x] Invalid SignedSSVMessage with zero signer
• [x] Invalid SignedSSVMessage with wrong data

[MatheusFranco99]

@GalRogozinski The current tests only check msg.Validate() which doesn't check the signature. This is because we don't specify here how this signature should be created or validated. I didn't add such a restriction on the type of signature because the message validation module (in p2p/validation) will be removed from spec in a future PR (right?) and the validator or runner don't have access to this signature. What do you think?

[GalRogozinski]

@MatheusFranco99

The spec should achieve 2 objectives in my eyes:

1. Make sure that the node implementation doesn't accidentally change the protocol w.o detection
2. Allow anyone to implement a node, ideally w.o looking at any other source.

Currently the spec doesn't exactly reach those objectives and in the spec rewrite I am hoping to change this. In the meanwhile I wish to do things correctly for new additions :-)

Imagine someone changes the RSA parameters (for better security) and then creates an incompatibility. This should be caught by the spec

[MatheusFranco99]

@GalRogozinski Sure, I'll be adding a check in message validation to verify the RSA signature and tests for it.

[MatheusFranco99]

@y0sher Yes, I'm aware of that. The point of this PR is not only to define a type for this buffer approach that we're using on the node, but it also is a pre-requisite for the next "reduce signatures" PR. This signed message type will now be used in the runner module.

[y0sher]

@y0sher @MatheusFranco99

This is a fork because the implementation puts signature first and the PR puts operator first, right?

If so we should change this to not be a fork because I don't think there is a real advantage here...

Its a fork because the implementation uses its own way of encoding the sig and operators. even if you create the struct in the PR in the same order it doesn't mean the final encoded data will be the same unless you enforce the struct to be encoded in the same way. In your PR the encoding is SSZ, where in the impl it is just raw data bytes on a buffer.

[GalRogozinski]

@moshe-blox @y0sher

There are 2 things we can do here:

1. Delete the ssz encoding and use the same encoding as impl
2. Make this part of the upcoming fork

The only reason I would use SSZ is just for the sake of consistency, since we use SSZ everywhere. Besides this I don't see an advantage. But this is good enough for me to opt for 2. If you guys don't want to change to SSZ then we can go for 1.