-
Notifications
You must be signed in to change notification settings - Fork 51
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
bug: fix hash size greater than 32 #1621
Conversation
waku/v2/node/waku_node.nim
Outdated
@@ -392,10 +392,11 @@ proc registerRelayDefaultHandler(node: WakuNode, topic: PubsubTopic) = | |||
return | |||
|
|||
proc traceHandler(topic: PubsubTopic, data: seq[byte]) {.async, gcsafe.} = | |||
let mh = MultiHash.digest("sha2-256", data).expect("valid hash") |
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.
Is there any requirement for using multihash here? Why don't we use the std/hash sha256
function directly? That would simplify things.
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.
agree, @jm-clius ?
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.
Yes, good catch - we should use only sha256
. We used Multihash before to mimic what we used in the messageIdProvider, but that has also since changed to pure sha256
to match with go-waku.
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.
btw can we use digest
from digest*(pubsubTopic: PubsubTopic, msg: WakuMessage): WakuMessageDigest =
? or what is stopping is from using it?
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.
Afaics we can use it (provided we change the tracehandler to take the WakuMessage so that we don't decode twice). This was originally over the whole message in order to mimic what we do for message IDs in gossipsub, but now that we have specified what deterministic hashing should look like it's probably a good idea to use that specified hash (i.e. the digest).
b7a670a
to
5b14cfb
Compare
using now digest as suggested :) |
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.
A minor request. In general, LGTM ✅
waku/v2/node/waku_node.nim
Outdated
trace "waku.relay received", | ||
peerId=node.peerId, | ||
pubsubTopic=topic, | ||
hash=MultiHash.digest("sha2-256", data).expect("valid hash").data.buffer.to0xHex(), # TODO: this could be replaced by a message UID | ||
hash="0x" & $topic.digest(msg).toHex(), |
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.
A minor request. There is a to0xHex()
proc in stew/byteutils
. Could you replace the string concat here with that method?
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.
sure! fair request. done
e41d042
digest
to calculate the message hash when logging it.