feat: messageHash attaribute added in SQLite + migration script ready #2159
Conversation
ABresting
requested review from
richard-ramos,
vpavlin,
SionoiS,
fbarbu15,
jm-clius and
Ivansete-status
ABresting
added
the
release-notes
|
This PR may contain changes to database schema of one of the drivers. If you are introducing any changes to the schema, make sure the upgrade from the latest release to this change passes without any errors/issues. Please make sure the label |
|
You can find the image built from this PR at Built from 9644a6c |
There was a problem hiding this comment.
It's getting a good shape indeed!
I added a few comments that we need to resolve.
Especially interesting is the one around the "version 7" issue, where we should emphasize the upgrade correctness. i.e. we need to make sure the database gets well updated from sqlite files created in version 0.15.0, for example.
There was a problem hiding this comment.
Apologies that I didn't realize that before but I think it is better to stick to the "digest" concept. Trying to modify so many files to accommodate the name to messageHash isn't great because we would need also to change the Store protocol itself.
Therefore I think it is better to keep using the digest name for that field in the Message database.
@jm-clius - wdyt about naming the new field messageDigest instead of messageHash? I think that would make it more aligned with our current code base. Maybe easier to just rename the "message_hash" to "messageDigest" in https://rfc.vac.dev/spec/14 :)
| @@ -48,7 +48,7 @@ proc isSchemaVersion7*(db: SqliteDatabase): DatabaseResult[bool] = | |||
| return ok(true) | |||
|
|
|||
| else: | |||
| info "Not considered schema version 7" | |||
| info "Not considered schema version 8" | |||
There was a problem hiding this comment.
I think this should be kept to 7. As a reminder, we will keep that until 0.30.0.
| info "Not considered schema version 8" | |
| info "Not considered schema version 7" |
There was a problem hiding this comment.
I think this introduces significant breaking changes. All Store clients operate currently with a view of how the cursors work and should be computed and this will cause queries to fail, afaics.
I don't see the need for introducing breaking changes. The main idea is to make sure that we introduce the authoritative deterministic message hash as set out in RFC 14 (https://rfc.vac.dev/spec/14/#deterministic-message-hashing) and implemented in https://github.com/waku-org/nwaku/blob/master/waku/waku_core/message/digest.nim as a queryable "column" (at least for postgresql, but it probably makes sense in the other archive impls too). To not break backwards compatibility, this should be a new column (I'm happy with messageHash). Since the deterministic message hash is used to populate this column, it MUST be unique and can serve as the (only) primary key. The computeDigest and other functions will indeed eventually be deprecated once we extend the Store protocol itself, but should remain untouched for now.
|
|
||
| proc computeDigest*(msg: WakuMessage): MessageDigest = | ||
| proc computeDigest*(msg: WakuMessage, pubSubTopic: string): MessageHash = |
There was a problem hiding this comment.
This should not be what's used to compute the message hash - this function exists for various historical reasons that I can't fully remember. It must indeed eventually be deprecated, but the authoritative message digest computation is: https://github.com/waku-org/nwaku/blob/master/waku/waku_core/message/digest.nim
There was a problem hiding this comment.
In other words, changing this function is unnecessarily going to break backwards compatibility. We simply want to add the message digest as defined in the RFC and implemented in https://github.com/waku-org/nwaku/blob/master/waku/waku_core/message/digest.nim as the primary key index column. This will allow us to extend the Store protocol in future to operate on these unique message digests. These will also replace our current cursors, but we'll do so in a way that doesn't break backwards compatbility. IMO for now the existing query behaviour, and especially cursors, shouldn't change as it will be very high-impact, unless I'm missing something?
Description
messageHashattribute added to SQLite, which removes theidattribute. An associated test case also added. A migration script also added.Also also unifies the use of
computeDigestas per the RFC. Since it was inconsistent with the remaining code. All the instances ofidattribute replaced bymessageHashwhich includes data types, testcases and library code.It has to be done using a single PR to avoid adding another migration script to remove the legacy
idattribute related code.Wherever possible digest is replaced by
messageHashfor uniformity.Adding folks from goWaku and jsWaku as well since this PR also modifies the nodeRestAPIs and some associated types.
Changes
idattribute replaced bymessageHashid/messageHashcomputation made uniform protocol wide.idattribute replaced bymessageHashidtomessageHashreplacementIssue
#2112