Skip to content
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

Merged
merged 2 commits into from
Mar 24, 2023
Merged

bug: fix hash size greater than 32 #1621

merged 2 commits into from
Mar 24, 2023

Conversation

alrevuelta
Copy link
Contributor

@alrevuelta alrevuelta commented Mar 22, 2023

  • Uses the message digest to calculate the message hash when logging it.

@@ -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")
Copy link
Contributor

@LNSD LNSD Mar 22, 2023

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

agree, @jm-clius ?

Copy link
Contributor

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.

Copy link
Contributor Author

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?

Copy link
Contributor

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).

@alrevuelta
Copy link
Contributor Author

using now digest as suggested :)

@alrevuelta alrevuelta requested a review from LNSD March 24, 2023 10:14
Copy link
Contributor

@LNSD LNSD left a 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 ✅

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(),
Copy link
Contributor

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?

Copy link
Contributor Author

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

@alrevuelta alrevuelta merged commit c42ac16 into master Mar 24, 2023
@alrevuelta alrevuelta deleted the fix-log-hash branch March 24, 2023 12:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants