-
Notifications
You must be signed in to change notification settings - Fork 95
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
Spec alignment review #1509
Spec alignment review #1509
Conversation
…s. Fix error string.
@@ -113,6 +113,10 @@ func (i *Instance) Broadcast(logger *zap.Logger, msg *spectypes.SignedSSVMessage | |||
if !i.CanProcessMessages() { | |||
return errors.New("instance stopped processing messages") | |||
} | |||
// TODO: In spec, the below assignment doesn't exist. | |||
// In fact, the msg should already have the correct identifier. Why are we setting it again? | |||
// Also, even though it doesn't change anything, we shouldn't change the message's content |
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.
we are not changing the message content, just creating the messageID.
infact in broadcast i.config.GetNetwork().Broadcast(msgID, msg)
below, msgID is never used, we are using the msgid from the message there.
maybe its time to not use separate msgid in the Broadcast?
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.
Oh true. I didn't realise it. Yup, at least for the scope of this PR, in spec we also use msgID in Broadcast, but from the msg itself.
Regarding changing the Broadcast function, we would also need to do a change in spec. Maybe it's not the scope of this PR but we can add it later on
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.
@y0sher Can I align this to spec, to use the ID from the msg?
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.
For now, I'm aligning it. Afterwards, if we find it necessary, we can go back to copying the ID from the state.
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.
there's no special reason to use the msgid from the state, the message is ok.
In conversation with @moshe-blox , we saw that |
|
Also because the implementation has |
It was used to disable signature checks in message validation IIRC, but if it's always false now, let's remove it |
2015833
into
alan/no-fork-spec-align-0cbc825
Overview
This PR adds suggestions from the spec alignment review. More than the minor suggested changes, I posted below some questions on major differences.
Differences
AggregatorRunner
,CommitteeRunner
, ...), anOperatorSigner
interface is used which is also defined in ssv. In spec, we changed it to a structure.BaseRunner
, there is aDomainTypeProvider
attribute that doesn't exist in spec. In spec, we get theDomainType
fromShare
.Note: The
VerifySignature
method fromConfig
(of QBFT) was dropped since it was unaligned to spec and not being used. Plus, it's a dangerous variable.