-
Notifications
You must be signed in to change notification settings - Fork 17
Conversation
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.
Thanks for adding this @jacobheun !
Just found 2 minor things that I would like you to have a look
} | ||
|
||
module.exports = { | ||
messagePublicKey, |
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.
Do we need to export this?
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 don't need to, but I could see value in having a utility for unmarshalling keys from messages, especially once key inlining is supported. We could remove it for now, but I don't think it's necessary.
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 can keep it then
test/pubsub.spec.js
Outdated
SignPrefix, | ||
Message.encode(normalizeOutRpcMessage(message)) | ||
]) | ||
// const bytesToSign = Buffer.concat([ |
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.
Can we remove these lines?
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.
LGTM
This adds an option to pubsub,
strictSigning
which defaults totrue
. Whenvalidate
is called, the signature of the message will be verified, unlessstrictSigning
is disabled.I originally intended to enforce this at the pubsub interface layer so that both Floodsub and Gossipsub could get the behavior out of the box, however, the implementations don't use the same message processing pipeline. Ideally, we should unify this more so that the abstraction layer can do common validation, like signature validation, before moving on to implementation specific message processing.
As it stands,
validate
is going to need to be called in both implementations in their respective pipelines. I will be creating PRs for both implementations.