feat: supporting meta field in store #2609

gabrielmer · 2024-04-19T15:22:26Z

Description

Adding support for saving the meta field in the db.

Changes

supporting inserting and querying meta field in sqlite
supporting inserting and querying meta field in postgres
added unit tests

Issue

closes #2432

github-actions · 2024-04-19T15:22:39Z

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 release-notes is added to make sure upgrade instructions properly highlight this change.

github-actions · 2024-04-19T15:26:49Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2609-rln-v2-false

Built from aafb959

gabrielmer · 2024-04-22T08:55:46Z

Currently, messages sent before the migration appear with meta field NULL while the ones after the migration are either empty (but not NULL) or with a value. Not sure about this inconsistency. If meta field is empty, should we set it to NULL everywhere? or just as empty string?

cc @NagyZoltanPeter @Ivansete-status

NagyZoltanPeter · 2024-04-22T11:14:52Z

Currently, messages sent before the migration appear with meta field NULL while the ones after the migration are either empty (but not NULL) or with a value. Not sure about this inconsistency. If meta field is empty, should we set it to NULL everywhere? or just as empty string?
cc @NagyZoltanPeter @Ivansete-status

Hm, from pure DB perspective I vote for set it to NULL if meta has no value.

github-actions · 2024-04-26T08:26:51Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2609-rln-v1

Built from 8066dfb

github-actions · 2024-04-26T08:26:59Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2609-rln-v2

Built from 8066dfb

gabrielmer · 2024-04-29T12:32:28Z

waku/factory/validator_signed.nim

@@ -34,6 +34,7 @@ proc msgHash*(pubSubTopic: string, msg: WakuMessage): array[32, byte] =
  ctx.update(msg.payload)
  ctx.update(msg.contentTopic.toBytes())
  ctx.update(msg.timestamp.uint64.toBytes(Endianness.littleEndian))
+  # ctx.update(msg.meta) meta is not included in the message hash, as the signature goes in the meta field


This comment addresses the doubt if meta field should be included in the hash.
Not sure if to delete the comment or not, it's ugly but I feel that it can be useful

In my opinion there's no need to add meta to hash calculation but better to confirm with @jm-clius

as the signature goes in the meta field What do you mean? Which signature we talking here?

I'm referring to the signed validators feature. In order to validate a message, looks like it expects the message hash to be signed and the signature to be attached to the WakuMessage's meta field.

nwaku/waku/factory/validator_signed.nim

Lines 70 to 73 in 6382ded

let recoveredSignature = SkSignature.fromRaw(msg.meta)

if recoveredSignature.isOk():

if recoveredSignature.get.verify(msgHash, protectedTopic.key):

outcome = errors.ValidationResult.Accept

Then yes indeed you can't add meta to the hash otherwise it would break this feature.

Please note that the hash for the signed validator feature is an application-level message hash. It is different from the internal Waku Message hash.
For the signed validator feature, the meta field cannot be included (as pointed out, because it contains the very signature over the application-level hash). This is specified here: https://github.com/vacp2p/rfc-index/blob/main/status/raw/simple-scaling.md#dos-protection
For Waku Message hashes (the ones used in the Store and elsewhere) the meta field forms an essential part in order to allow applications to differentiate messages that may be exactly similar in all other fields: https://github.com/vacp2p/rfc-index/blob/main/waku/standards/core/14/message.md#deterministic-message-hashing

gabrielmer · 2024-04-29T12:33:18Z

waku/waku_archive/driver/postgres_driver/postgres_driver.nim

@@ -31,11 +31,11 @@ type PostgresDriver* = ref object of ArchiveDriver
 const InsertRowStmtName = "InsertRow"
 const InsertRowStmtDefinition = # TODO: get the sql queries from a file
  """INSERT INTO messages (id, messageHash, storedAt, contentTopic, payload, pubsubTopic,
-  version, timestamp) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) ON CONFLICT DO NOTHING;"""
+  version, timestamp, meta) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, CASE WHEN $9 = '' THEN NULL ELSE $9 END) ON CONFLICT DO NOTHING;"""


This CASE statement is only added in the postgres statements, it doesn't seem to be supported in sqlite

When we get the data out of the DB do we process '' the same as NULL ? If not this is a problem.

Great point! So the drivers have to cast the values to the expected Nim data type. So when retrieving the value from the db and setting it to a Nim object, both NULL and '' are converted to an empty seq of bytes

Ivansete-status

LGTM! Thanks for it!
The only interesting point of simplification would be the migration script. I wonder if we could skip the "partitions" part in that migration script and only focus on the actual partitioned table schema change

Ivansete-status · 2024-04-30T07:55:31Z

migrations/message_store_postgres/content_script_version_4.nim

+DO $$
+DECLARE
+     min_storedAt numeric;
+     max_storedAt numeric;
+     min_storedAtSeconds integer = 0;
+     max_storedAtSeconds integer = 0;
+     partition_name TEXT;
+     create_partition_stmt TEXT;
+BEGIN
+    SELECT MIN(storedAt) into min_storedAt
+    FROM messages_backup;
+
+    SELECT MAX(storedAt) into max_storedAt
+    FROM messages_backup;
+


I have the impression that it would be possible to just change the partitioned-table schema and that will automatically reflect that change in the current existing partitions. If that is the case, I think we can simplify the migration logic :)

I just tried it without the partition part (45a35e7) but the migration failed :/
So for now I think it's better to leave it this way

Ivansete-status · 2024-04-30T09:56:33Z

waku/factory/validator_signed.nim

@@ -34,6 +34,7 @@ proc msgHash*(pubSubTopic: string, msg: WakuMessage): array[32, byte] =
  ctx.update(msg.payload)
  ctx.update(msg.contentTopic.toBytes())
  ctx.update(msg.timestamp.uint64.toBytes(Endianness.littleEndian))
+  # ctx.update(msg.meta) meta is not included in the message hash, as the signature goes in the meta field


In my opinion there's no need to add meta to hash calculation but better to confirm with @jm-clius

This reverts commit 45a35e7.

SionoiS

LGTM

SionoiS · 2024-04-30T11:56:20Z

waku/factory/validator_signed.nim

@@ -34,6 +34,7 @@ proc msgHash*(pubSubTopic: string, msg: WakuMessage): array[32, byte] =
  ctx.update(msg.payload)
  ctx.update(msg.contentTopic.toBytes())
  ctx.update(msg.timestamp.uint64.toBytes(Endianness.littleEndian))
+  # ctx.update(msg.meta) meta is not included in the message hash, as the signature goes in the meta field


as the signature goes in the meta field What do you mean? Which signature we talking here?

SionoiS · 2024-04-30T11:58:59Z

waku/waku_archive/driver/postgres_driver/postgres_driver.nim

@@ -31,11 +31,11 @@ type PostgresDriver* = ref object of ArchiveDriver
 const InsertRowStmtName = "InsertRow"
 const InsertRowStmtDefinition = # TODO: get the sql queries from a file
  """INSERT INTO messages (id, messageHash, storedAt, contentTopic, payload, pubsubTopic,
-  version, timestamp) VALUES ($1, $2, $3, $4, $5, $6, $7, $8) ON CONFLICT DO NOTHING;"""
+  version, timestamp, meta) VALUES ($1, $2, $3, $4, $5, $6, $7, $8, CASE WHEN $9 = '' THEN NULL ELSE $9 END) ON CONFLICT DO NOTHING;"""


When we get the data out of the DB do we process '' the same as NULL ? If not this is a problem.

github-actions bot assigned gabrielmer Apr 19, 2024

gabrielmer added the release-notes Issue/PR needs to be evaluated for inclusion in release notes highlights or upgrade instructions label Apr 22, 2024

gabrielmer force-pushed the supporting-meta-field-in-store branch from 6e07898 to 8b09c0f Compare April 26, 2024 08:01

gabrielmer commented Apr 29, 2024

View reviewed changes

gabrielmer added 13 commits April 29, 2024 15:36

Adding meta field to db schema

e2e392d

adding meta field to postgres driver

8b92f78

adding meta field to sqlite

3ebda6a

fix compilation error

b428ded

adding missing comma

f089981

not including meta field in hash for signed topics

87f63f5

fix version numbering

f32228e

inserting meta field null when there's an empty string

f60ec3e

adding meta to store v3 queries

d39fc9e

including meta field in archive test

2434f81

adding test with meta field

bc2dc8d

fix sqlite query

2dfd913

adding test with meta field

a1e2110

gabrielmer force-pushed the supporting-meta-field-in-store branch from f98a103 to a1e2110 Compare April 29, 2024 12:36

gabrielmer requested review from Ivansete-status, SionoiS and NagyZoltanPeter and removed request for Ivansete-status and SionoiS April 29, 2024 12:36

gabrielmer requested review from Ivansete-status and SionoiS April 29, 2024 12:36

gabrielmer marked this pull request as ready for review April 30, 2024 07:43

Ivansete-status approved these changes Apr 30, 2024

View reviewed changes

gabrielmer added 2 commits April 30, 2024 14:19

cleaning migration script

45a35e7

Revert "cleaning migration script"

89124a8

This reverts commit 45a35e7.

SionoiS approved these changes Apr 30, 2024

View reviewed changes

gabrielmer added 2 commits May 3, 2024 13:06

Merge branch 'master' into supporting-meta-field-in-store

5839555

Merge branch 'master' into supporting-meta-field-in-store

00c2a67

gabrielmer merged commit a46d445 into master May 6, 2024
14 of 15 checks passed

gabrielmer deleted the supporting-meta-field-in-store branch May 6, 2024 08:20

Ivansete-status pushed a commit that referenced this pull request May 14, 2024

feat: supporting meta field in store (#2609)

f821791

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat: supporting meta field in store #2609

feat: supporting meta field in store #2609

gabrielmer commented Apr 19, 2024 •

edited

Loading

github-actions bot commented Apr 19, 2024

github-actions bot commented Apr 19, 2024 •

edited

Loading

gabrielmer commented Apr 22, 2024

NagyZoltanPeter commented Apr 22, 2024

github-actions bot commented Apr 26, 2024 •

edited

Loading

github-actions bot commented Apr 26, 2024 •

edited

Loading

gabrielmer Apr 29, 2024

Ivansete-status Apr 30, 2024

SionoiS Apr 30, 2024

gabrielmer Apr 30, 2024

SionoiS Apr 30, 2024

jm-clius May 3, 2024

gabrielmer Apr 29, 2024 •

edited

Loading

SionoiS Apr 30, 2024

gabrielmer Apr 30, 2024 •

edited

Loading

Ivansete-status left a comment

Ivansete-status Apr 30, 2024

gabrielmer Apr 30, 2024

Ivansete-status Apr 30, 2024

SionoiS left a comment

SionoiS Apr 30, 2024

SionoiS Apr 30, 2024

	let recoveredSignature = SkSignature.fromRaw(msg.meta)
	if recoveredSignature.isOk():
	if recoveredSignature.get.verify(msgHash, protectedTopic.key):
	outcome = errors.ValidationResult.Accept

feat: supporting meta field in store #2609

feat: supporting meta field in store #2609

Conversation

gabrielmer commented Apr 19, 2024 • edited Loading

Description

Changes

Issue

github-actions bot commented Apr 19, 2024

github-actions bot commented Apr 19, 2024 • edited Loading

gabrielmer commented Apr 22, 2024

NagyZoltanPeter commented Apr 22, 2024

github-actions bot commented Apr 26, 2024 • edited Loading

github-actions bot commented Apr 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielmer Apr 29, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielmer Apr 30, 2024 • edited Loading

Choose a reason for hiding this comment

Ivansete-status left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

SionoiS left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gabrielmer commented Apr 19, 2024 •

edited

Loading

github-actions bot commented Apr 19, 2024 •

edited

Loading

github-actions bot commented Apr 26, 2024 •

edited

Loading

github-actions bot commented Apr 26, 2024 •

edited

Loading

gabrielmer Apr 29, 2024 •

edited

Loading

gabrielmer Apr 30, 2024 •

edited

Loading