-
Notifications
You must be signed in to change notification settings - Fork 55
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
allow to omit peerId and seqno #372
Conversation
@@ -1031,11 +1031,21 @@ method rpcHandler*(g: GossipSub, | |||
|
|||
g.mcache.put(msgId, msg) | |||
|
|||
if g.verifySignature and not msg.verify(peer.peerId): | |||
if (msg.signature.len > 0 or g.verifySignature) and not msg.verify(peer.peerId): |
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.
peer.peerId
is not really needed for verify, not sure why it's there..?
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.
It's indeed not used
Seems like this is changing again as per libp2p/specs#294 (comment). We should probably hold off on it for the time being, or perhaps adopt a lax approach? |
Yeah, but it's currently indeed lax so it should be just fine and useful for NBC. |
yes, the lax mode is an intermediary step that is useful now |
This PR was not complete, I said I needed a couple more hours on it. |
Since it's merged I will continue next week. As there is something weird going on with floodsub too. |
Add "[WIP]" at the start of the title, while your PR is still "work in progress". |
right, forgot about that - need this for nbc to do something more reasnable soon - it's not affected at this point so., |
* allow to omit peerId and seqno * small refactor * wip * fix message encoding * improve rpc signature logic * remove peerid from verify * trace fixes * fix message test * fix gossip 1.0 tests
related to ethereum/consensus-specs#1981