-
Notifications
You must be signed in to change notification settings - Fork 42
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
feat: deterministic message hashing #1233
Conversation
Can only be merged once #1230 introduces the |
7c449b7
to
a3ea420
Compare
Remove reviewers as PR is actually still draft |
998e350
to
5f61914
Compare
size-limit report 📦
|
Now ready for review |
* Deterministic Message Hashing as defined in | ||
* [14/WAKU2-MESSAGE](https://rfc.vac.dev/spec/14/#deterministic-message-hashing) | ||
*/ | ||
export function messageHash( |
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.
Interesting part: the deterministic message hash logic
import { toProtoMessage } from "../to_proto_message.js"; | ||
|
||
const log = debug("waku:relay:message-id"); | ||
export function gossipsubMessageId( |
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.
interesting part: the gossipsub message id calculation
export function gossipsubMessageId( | ||
message: Message | ||
): Promise<Uint8Array> | Uint8Array { | ||
const startTime = performance.now(); |
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.
where does performance
come from? can't find an import for 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.
ah, TIL it's a native method as an alternative to Date.now()
💯
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 a JS API to handle performance. Which matters here as the methods need to execute within 100ms.
packages/message-hash/src/index.ts
Outdated
if (message.meta) { | ||
bytesToHash = concat([ | ||
pubsubTopicBytes, | ||
message.payload, | ||
contentTopicBytes, | ||
message.meta, | ||
]); | ||
} else { | ||
bytesToHash = concat([ | ||
pubsubTopicBytes, | ||
message.payload, | ||
contentTopicBytes, | ||
]); | ||
} |
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 bytesToHash
an apt naming?
as this variable does not hold the actual hash of the bytes but only the concatenated version - the hash is returned in the last line
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 holds the bytes that needs to be hashed. The bytes to hash. What's apt?
Could probably just call it bytes
build | ||
bundle | ||
dist | ||
node_modules |
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.
Let's add CHANGELOG to ignore
1f8030c
to
65d6dc3
Compare
TODO: Check the API around the seen cache key and ensure MUID is used for it. |
Discussing with @jm-clius, we do not want to do the change on Relay at this stage. We will want to coordinate across implementations when we do it. |
65d6dc3
to
11b6450
Compare
11b6450
to
73043f5
Compare
73043f5
to
c8c2cbf
Compare
Ref #1208
After checking with @jm-clius, we should not change the gossipsub message id just yet. This change will have to be coordinated across the implementations.
However, we can provide the MUID calculation logic for now. This will be first use by nwaku to remove store duplicates.