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

feat: amending computeDigest func. + related test cases #2132

Merged
merged 4 commits into from
Oct 19, 2023

Conversation

ABresting
Copy link
Contributor

Description

Amending the computeDigest function to add pubSubTopic as a function parameter, will help introduce a new messageHash attribute. The corresponding queue driver code is updated to accommodate the amended function.

Amending the existing test cases that use the older version of the computeDigest function.

Changes

  • computeDigest function amended
  • related existing test cases amended
  • queue driver code adjusted to the new change

Issue

#2112

@github-actions
Copy link

github-actions bot commented Oct 17, 2023

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2132

Built from 414d10a

Copy link
Member

@vpavlin vpavlin left a comment

Choose a reason for hiding this comment

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

If I am not mistaken, this does not really change the DB schema or how messages are stored or searched for. It may result in some duplicate messages being stored after the node upgrades - as the unique index values will change for the same message after upgrade, but that should not be a problem.

Thanks for updating the tests!

LGTM

Copy link
Contributor

@alrevuelta alrevuelta left a comment

Choose a reason for hiding this comment

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

lgtm.

I actually dont understant why the pubsub topic wasnt part of the digest if the RFC specifies so: https://rfc.vac.dev/spec/14/#deterministic-message-hashing

And related in case you want to have a look. Seems we have two hashed, one mainly used for store (the one you modified) and one for relay defaultMessageIdProvider. Wondering if we should unify both.

proc defaultMessageIdProvider*(message: messages.Message): Result[MessageID, ValidationResult] =
let hash = sha256.digest(message.data)
ok(@(hash.data))
## Waku message Unique ID provider
# TODO: Add here the MUID provider once `meta` field RFC PR is merged

@ABresting
Copy link
Contributor Author

If I am not mistaken, this does not really change the DB schema or how messages are stored or searched for. It may result in some duplicate messages being stored after the node upgrades - as the unique index values will change for the same message after upgrade, but that should not be a problem.

The schema is going to be changed, we aim to replace id attribute to the messageHash attribute this will improve the storage and searching of messages. The schema change code I will push today as it was suggested by Ivan to push changes incrementally based on some layer that they impact, such as Queue driver, SQLite, Postgres, today I will push SQLite related changes.

Thanks for updating the tests!

LGTM

@ABresting
Copy link
Contributor Author

lgtm.

I actually dont understant why the pubsub topic wasnt part of the digest if the RFC specifies so: https://rfc.vac.dev/spec/14/#deterministic-message-hashing

And related in case you want to have a look. Seems we have two hashed, one mainly used for store (the one you modified) and one for relay defaultMessageIdProvider. Wondering if we should unify both.

proc defaultMessageIdProvider*(message: messages.Message): Result[MessageID, ValidationResult] =
let hash = sha256.digest(message.data)
ok(@(hash.data))
## Waku message Unique ID provider
# TODO: Add here the MUID provider once `meta` field RFC PR is merged

AFAIK, the messageID or id attribute will be replaced by messageHash, i.e. the way messageID is computed is being changed along with it's name. Indeed, we add pubSubTopic inside the hash as referred by this rfc

@ABresting ABresting merged commit 1669f71 into master Oct 19, 2023
9 of 10 checks passed
@ABresting ABresting deleted the add-new-message-hash-column branch October 19, 2023 09:59
ABresting added a commit that referenced this pull request Nov 2, 2023
ABresting added a commit that referenced this pull request Nov 8, 2023
omahs pushed a commit to omahs/nwaku that referenced this pull request Nov 21, 2023
* feat: amending computeDigest func. + related test cases

* minor fixes

* minor fixes v1: testcase saga continues

---------

Co-authored-by: Vaclav Pavlin <vaclav@status.im>
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