fix: add protection in rest service to always publish with timestamp if user doesn't provide it #2261

Ivansete-status · 2023-11-30T10:46:54Z

Description

Before this PR, the relay REST service accepted "publish" events without timestamp info.
Therefore, the Postgres database ended up with the following message set:

That caused pagination issues within the Store protocol because the sender_time attribute (part of the Cursor object) was always set to 0.

Changes

Issue

This was noticed while dogfooding status.test in the context of status-im/infra-status#37

github-actions · 2023-11-30T10:51:42Z

You can find the image built from this PR at

quay.io/wakuorg/nwaku-pr:2261

Built from 6b768ba

jm-clius · 2023-11-30T17:17:09Z

Interesting. Can you expand a little bit more on why the 0 timestamps caused issues with pagination? Although the timestamp attribute is mandatory in the Waku Network (which is a certain restricted way of using Waku), the raw protocol does not require timestamps to be present: https://rfc.vac.dev/spec/14/#message-attributes
The only issue this caused for Store was how to do the sorting in the archive, but for this we introduced a storedAt timestamp which would always be non-zero. As far as I can see, 0 timestamps should not affect cursor creation or lookups? As long as the Index combination remains unique, the value of the fields constituting the cursor should not matter?

gabrielmer · 2023-11-30T17:26:27Z

I think we've discussed before about automatically adding in REST the current timestamp if the user does not set it in the request.
If that's the case, shouldn't we add the timestamp for the user instead of responding with an error? or am I missing something and I'm mixing different things? 😅

Ivansete-status · 2023-12-01T10:42:27Z

Interesting. Can you expand a little bit more on why the 0 timestamps caused issues with pagination? Although the timestamp attribute is mandatory in the Waku Network (which is a certain restricted way of using Waku), the raw protocol does not require timestamps to be present: https://rfc.vac.dev/spec/14/#message-attributes The only issue this caused for Store was how to do the sorting in the archive, but for this we introduced a storedAt timestamp which would always be non-zero. As far as I can see, 0 timestamps should not affect cursor creation or lookups? As long as the Index combination remains unique, the value of the fields constituting the cursor should not matter?

Thanks for the comment!
Let me elaborate more.
Let's imagine a Store request that needs a couple of pages and let's consider that all the messages are archived with sender_time == 0 as shown in the pic in the PR description.

In this case, the very first page will be returned with the following cursor definition (notice that senderTime is zero in the cursor .)

nwaku/waku/node/waku_node.nim

Line 807 in 89dbbd2

    
           cursor: response.cursor.map(proc(cursor: ArchiveCursor): HistoryCursor = HistoryCursor(pubsubTopic: cursor.pubsubTopic, senderTime: cursor.senderTime, storeTime: cursor.storeTime, digest: cursor.digest)),

After that, a go-Store client will ask for the second page of messages from the following code. Notice that the senderTime will be zero in this case too:

https://github.com/waku-org/go-waku/blob/16d59f37d02a8375466c027f876acd05621bc9f6/waku/v2/protocol/store/waku_store_client.go#L457-L459

https://github.com/waku-org/go-waku/blob/16d59f37d02a8375466c027f876acd05621bc9f6/waku/v2/protocol/store/pb/store.pb.go#L196

https://github.com/waku-org/go-waku/blob/16d59f37d02a8375466c027f876acd05621bc9f6/waku/v2/protocol/store/pb/store.pb.go#L126

After that, the following if statement will happen, considering it a wrong request, in nwaku, and therefore the rest of the Store requests won't proceed. In other words, only the very first page is delivered to the store client.

nwaku/waku/waku_store/rpc_codec.nim

Lines 56 to 57 in c86dc44

    
           if not ?pb.getField(3, senderTime): 
        
             return err(ProtobufError.missingRequiredField("sender_time"))

Set now timestamp to the WakuMessage if the client didn't send this information in the request. This could happen in a request like: curl -X POST "http://127.0.0.1:8646/relay/v1/messages/%2Fwaku%2F2%2Fdefault-waku%2Fproto" -H "content-type: application/json" -d '{"payload":"'${payload}'","contentTopic":"my-ctopic"}'

Ivansete-status · 2023-12-01T11:50:05Z

Thanks for the suggestion @gabrielmer !
I've applied your recommendation. I think it gets better now!

SionoiS

Thank you!

SionoiS · 2023-12-01T12:39:22Z

tests/waku_archive/test_waku_archive.nim

@@ -72,7 +72,7 @@ suite "Waku Archive - message handling":

    ## Then
    check:
-      (waitFor driver.getMessagesCount()).tryGet() == 1
+      (waitFor driver.getMessagesCount()).tryGet() == 0


I would change the test name. also it should be deliver not driver 🤣

I would change the test name. also it should be deliver not driver 🤣

Good point! I changed the whole tests' names, which is a little bit beyond the scope of this PR.

gabrielmer

Looks great! Thanks so much! 🤩

jm-clius

Thanks for the detailed response!

After that, the following if statement will happen, considering it a wrong request, in nwaku, and therefore the rest of the Store requests won't proceed. In other words, only the very first page is delivered to the store client.

Ah, I see now why this happens. However, I think the issue here is that nwaku enforces the existence of a senderTimestamp field in violation of the current RFC. We may indeed change to a timestamp-mandatory store implementation, but that would involve an RFC change and a more extensive data schema review (storedAt receiverTimestamp become obsolete), as I mention below. A good time to do that would be in the new, reviewed Store protocol that @ABresting is working on, where timestamp is likely to be mandatory.

I do agree with the REST API adding timestamps on the user's behalf though. For now to proceed, I'd be happy to approve two different PRs:

REST API changes to add timestamp when publishing new messages (received messages should never be modified)
Remove strict requirement for sender timestamp in cursor in nwaku (fixing compliance with RFC).

jm-clius · 2023-12-04T12:50:07Z

waku/waku_api/rest/filter/types.nim

+  var timestamp = msg.timestamp.get(0)
+
+  if timestamp == 0:
+    timestamp = getNanosecondTime(getTime().toUnixFloat())


A bit confused by this. Where is this used? Afaics this is to convert received filter messages (as a client) to Waku Messages, although I can't find where this is called in the implementation. What would be the purpose of modifying timestamps in received messages on the API?

Well, this was thought to be a change aligned to the Store change. I can remove it if that doesn't make sense to you in this PR.

jm-clius · 2023-12-04T12:53:00Z

waku/waku_archive/archive.nim

  if msg.timestamp == 0:
-    return ok()
+    return err(timestampIsZero)


Although I agree with the REST API adding timestamps on behalf of the application when publishing messages. I still don't think the Store should (for now) enforce sender timestamps. Doing so goes against the existing RFC. Furthermore, this obsoletes the entire receiverTimestamp, storedAt, etc. mechanism which is a larger cleanup exercise. We may indeed decide to follow this route (and perhaps should have from the beginning), but deprecating the RFC provision for optional timestamps is a bit larger, involves an RFC change and a more extensive review of the existing data scheme.

Thanks for the comment!
The https://rfc.vac.dev/spec/13/ RFC enforces the existence of senderTime in the Store request, although it is true that the RFC doesn't enforce any value. This can be seen because the senderTime is not optional in the protobuf definition:

On the other hand, in the archive.nim module we are enforcing the senderTime to be within a certain time window:

nwaku/waku/waku_archive/archive.nim

Lines 51 to 60 in aebc912

let

now = getNanosecondTime(getTime().toUnixFloat())

lowerBound = now - MaxMessageTimestampVariance

upperBound = now + MaxMessageTimestampVariance

if msg.timestamp < lowerBound:

return err(invalidMessageOld)

if upperBound < msg.timestamp:

return err(invalidMessageFuture)

... but it is weird that we accept msg.timestamp == 0 , isn't it?

jm-clius

Thanks! LGTM now.

This is needed because we have the following in waku_archive/archive.nim method validate*(validator: DefaultMessageValidator, msg: WakuMessage): ValidationResult = if msg.timestamp == 0: return ok()

Ivansete-status · 2023-12-06T11:44:28Z

After a chat with @jm-clius , we left the waku_archive/archive.nim with the following:

method validate*(validator: DefaultMessageValidator, msg: WakuMessage): ValidationResult =
  if msg.timestamp == 0:
    return ok()

Therefore, messages with timestamp zero are allowed. Kindly ask Hanno for further details as to why this is interesting as he has a better understanding of the system :D

Ivansete-status self-assigned this Nov 30, 2023

Ivansete-status marked this pull request as ready for review November 30, 2023 16:52

Ivansete-status requested review from SionoiS, jm-clius, gabrielmer and richard-ramos November 30, 2023 16:53

Ivansete-status added 4 commits December 1, 2023 12:47

rest relay: avoid publish messages without timestamp information

becb2ce

archive.nim: prevent archiving messages without timestamp info

123087e

test_waku_archive.nim: message can't be stored if sender_time == 0

67704d7

Ivansete-status force-pushed the fix-timestamp-zero-protection branch from 552bc3d to a7213c4 Compare December 1, 2023 11:47

SionoiS approved these changes Dec 1, 2023

View reviewed changes

test_waku_archive: rename tests

bc4620e

Ivansete-status force-pushed the fix-timestamp-zero-protection branch from f541d4b to bc4620e Compare December 1, 2023 13:21

gabrielmer approved these changes Dec 1, 2023

View reviewed changes

jm-clius requested changes Dec 4, 2023

View reviewed changes

undoing changes as per Hanno's suggestion

9c37289

Ivansete-status force-pushed the fix-timestamp-zero-protection branch from aebc912 to 9c37289 Compare December 4, 2023 19:02

jm-clius self-requested a review December 5, 2023 16:18

jm-clius approved these changes Dec 5, 2023

View reviewed changes

Ivansete-status changed the title ~~fix: Disallow archiving messages that don't have timestamp (sender_time) info or is set to 0~~ fix: add protection in rest service to always publish with timestamp if user doesn't provide it Dec 6, 2023

test_waku_archive: store a message without timestamp

499f107

This is needed because we have the following in waku_archive/archive.nim method validate*(validator: DefaultMessageValidator, msg: WakuMessage): ValidationResult = if msg.timestamp == 0: return ok()

Ivansete-status force-pushed the fix-timestamp-zero-protection branch from 4bf048a to 499f107 Compare December 6, 2023 11:41

Ivansete-status merged commit 42f1957 into master Dec 6, 2023
9 of 10 checks passed

Ivansete-status deleted the fix-timestamp-zero-protection branch December 6, 2023 13:02

chair28980 mentioned this pull request Jan 2, 2024

feat: use current timestamp when unset in rest api #2193

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix: add protection in rest service to always publish with timestamp if user doesn't provide it #2261

fix: add protection in rest service to always publish with timestamp if user doesn't provide it #2261

Ivansete-status commented Nov 30, 2023

github-actions bot commented Nov 30, 2023 •

edited

Loading

jm-clius commented Nov 30, 2023

gabrielmer commented Nov 30, 2023

Ivansete-status commented Dec 1, 2023 •

edited

Loading

Ivansete-status commented Dec 1, 2023

SionoiS left a comment

SionoiS Dec 1, 2023

Ivansete-status Dec 1, 2023

gabrielmer left a comment

jm-clius left a comment

jm-clius Dec 4, 2023

Ivansete-status Dec 4, 2023

jm-clius Dec 4, 2023

Ivansete-status Dec 4, 2023

jm-clius left a comment

Ivansete-status commented Dec 6, 2023

	let
	now = getNanosecondTime(getTime().toUnixFloat())
	lowerBound = now - MaxMessageTimestampVariance
	upperBound = now + MaxMessageTimestampVariance

	if msg.timestamp < lowerBound:
	return err(invalidMessageOld)

	if upperBound < msg.timestamp:
	return err(invalidMessageFuture)

fix: add protection in rest service to always publish with timestamp if user doesn't provide it #2261

fix: add protection in rest service to always publish with timestamp if user doesn't provide it #2261

Conversation

Ivansete-status commented Nov 30, 2023

Description

Changes

Issue

github-actions bot commented Nov 30, 2023 • edited Loading

jm-clius commented Nov 30, 2023

gabrielmer commented Nov 30, 2023

Ivansete-status commented Dec 1, 2023 • edited Loading

Ivansete-status commented Dec 1, 2023

SionoiS left a comment

Choose a reason for hiding this comment

SionoiS Dec 1, 2023

Choose a reason for hiding this comment

Ivansete-status Dec 1, 2023

Choose a reason for hiding this comment

gabrielmer left a comment

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

jm-clius Dec 4, 2023

Choose a reason for hiding this comment

Ivansete-status Dec 4, 2023

Choose a reason for hiding this comment

jm-clius Dec 4, 2023

Choose a reason for hiding this comment

Ivansete-status Dec 4, 2023

Choose a reason for hiding this comment

jm-clius left a comment

Choose a reason for hiding this comment

Ivansete-status commented Dec 6, 2023

github-actions bot commented Nov 30, 2023 •

edited

Loading

Ivansete-status commented Dec 1, 2023 •

edited

Loading