13/14/16/21: Change in timestamp format #483

s1fr0 · 2022-02-04T22:40:51Z

In nim-waku PR #842 a new type Timestamp for all timestamps in waku v2 is introduced. The type Timestamp is currently set to be an alias for int64 (see utils/time) and all times now have nanosecond resolution.

This PR reflects these changes in specifications. In particular it addresses the following item of #834:

The corresponding specs i.e., Waku2-Store and Waku2-Message should reflect this change

It further addresses Waku2-RPC and Waku2-FTSTORE specifications.

jm-clius

I think the timestamp changes also affect https://rfc.vac.dev/spec/16/

This is a breaking change in the protocol, so I think we should do a major version bump in the protocol-id. WDYT @staheri14 @oskarth ? This will also affect the nim-waku implementation PR, as the protocol-id is configured as a const there.

…aku-org/nwaku#834

s1fr0 · 2022-02-09T23:30:24Z

In order to sign old commits (and later allow merging) I executed a push force: this unfortunately deleted the file change request by @jm-clius. I however assume that @jm-clius wanted to make https://rfc.vac.dev/spec/16/ consistent with the timestamp refactoring of #842: such (new) commit can be found here.

staheri14 · 2022-02-09T23:33:44Z

protocol-id

Yes, I agree, ~~also we may need to update the protocol id of other protocols that directly use WakuMessage e.g., filter and lightpush?~~ @jm-clius @oskarth

Update: I think the store protocol is the only one whose id needs to be changed (not the other protocols I mentioned above)

staheri14 · 2022-02-09T23:40:32Z

@s1fr0 This file also needs to be updated https://rfc.vac.dev/spec/21/

s1fr0 · 2022-02-09T23:49:32Z

Thanks @staheri14 ! Done in 29c89c6.

oskarth

@jm-clius by protocol id what do you mean more precisely?

UPDATE: Pretty sure you meant 2 below, not 1 after re-reading as two separate paragraphs

1) JSON RPC API

If you mean for 16 specifically do you mean "v1" in get_waku_v2_store_v1_messages? I don't see a specific mention of protocol-id though.

Technically speaking, https://rfc.vac.dev/spec/16 is still in draft, which means no end users should rely on it. Practically speaking, this might impact some users. At a minimum, it should be communicated as a breaking change in nim-waku changelog.

Is the suggestion to change "v1" to "v2" here? I'd be fine with it, but it seems like if we want to do it properly we'd also keep v1 around (otherwise for end consumers things break regardless), and not sure the overhead for that is justified?

2) Protocol identifier in store protocol

Assuming we mean e.g. Protocol identifier*: /vac/waku/store/2.0.0-beta3 in https://rfc.vac.dev/spec/13/ it is also in draft, but I think we can bump this to beta4. I don't think a major bump (e.g. 3.0 or 2.1) makes sense given that it still in draft mode. Once store is in stable mode, a new store spec would be a new RFC, and we'd want to give more care to make sure both protocol identifiers are supported by e.g. a single nwaku node.

I don't think the overhead of keepingbeta3 support is justified - easiest would be to drop it completely. Ideally we don't require a coordinated upgrade, so js-waku nodes can keep talking to some old nodes that still support beta3 until they've moved over to use beta4. I'm not sure we have support for this type of deploy (like green-blue production setup), but perhaps it can be done with test/production fleet? This depends a bit on how much js-waku nodes rely on reliable store operation in production cluster.

wdyt @D4nte re ideal upgrade path for these types of changes.

oskarth · 2022-02-10T03:52:31Z

Updated title to indicate it is a breaking change, refactor seems to point to implementation details not changing interface with different type requirements

jm-clius · 2022-02-10T08:33:21Z

@oskarth, yeah I meant 2 indeed - the protocol identifier of the store protocol. It makes sense to me to rename to beta4. We can upgrade test and be sure everyone is on board before we deploy to prod.

jm-clius · 2022-02-10T16:03:08Z

content/docs/rfcs/21/README.md

+  + Timestamp start_time = 5;
+  + Timestamp end_time = 6;


For this to be proto3 I think we have to use proto types, i.e. int64. It is true that in the nim client we define a separate Timestamp type, but when serialising to protobuf, I think we use int64 fields?

That is correct (in reality we serialize to zint64 when writing protobuffers and read as Timestamp). Unless we alias this type as well in utils/time to pbTimestamp. Let me know what do you think it suits best.

Timestamp is a proto3 type: https://developers.google.com/protocol-buffers/docs/proto3 that is actually a timestamp in string format. Is that the type you wish to use?

Any reason why we are using different types for start_time, end_time and timestamp?

I apologize for the misunderstanding. The correct proto3 type is indeed sint64. since I cast our internal Timestamp type (an alias for int64) to zigzag encoding before writing it to the buffer, i.e. I cast to zint64 in libp2p miniprotobuf implementation. Changes committed.

staheri14 · 2022-02-10T23:31:15Z

@s1fr0 I think your concern is that zint64 might have a different encoding than int64 in protobuf definition (so it does not map to int64 ), you may look at other scalar types in https://developers.google.com/protocol-buffers/docs/proto3, it may correspond to sint64? someone in the dev-helpdesk may also be able to help.

D4nte · 2022-02-11T00:18:36Z

content/docs/rfcs/16/README.md

@@ -578,7 +578,7 @@ This method is part of the `store` API and the specific resources to retrieve ar

 #### Request

-```curl -d '{"jsonrpc":"2.0","id":"id","method":"get_waku_v2_store_v1_messages", "params":[ "", [{"contentTopic":"/waku/2/default-content/proto"}],{"pageSize":2,"cursor":{"digest":"abcdef","receivedTime":1605887187.00},"forward":false}]}' --header "Content-Type: application/json" http://localhost:8545```
+```curl -d '{"jsonrpc":"2.0","id":"id","method":"get_waku_v2_store_v1_messages", "params":[ "", [{"contentTopic":"/waku/2/default-content/proto"}],{"pageSize":2,"cursor":{"digest":"abcdef","receivedTime":1605887187000000000},"forward":false}]}' --header "Content-Type: application/json" http://localhost:8545```


Would it be worth taking the opportunity to align the receivedTime field with the protobuf equivalent receiverTime?

Thanks! I'll open a new issue for this.

D4nte · 2022-02-11T00:19:23Z

content/docs/rfcs/21/README.md

+  + Timestamp start_time = 5;
+  + Timestamp end_time = 6;


Timestamp is a proto3 type: https://developers.google.com/protocol-buffers/docs/proto3 that is actually a timestamp in string format. Is that the type you wish to use?

Any reason why we are using different types for start_time, end_time and timestamp?

s1fr0 · 2022-02-11T10:57:18Z

@staheri14 indeed! Since I use libp2p miniprotobuf zint64 to write timestamps in buffers, these are zigzag encoded. According to proto3 it seems that such integers correspond to sint64.

D4nte · 2022-02-13T22:34:36Z

? wdyt @D4nte re ideal upgrade path for these types of changes.

I think there is no choice but for js-waku to support both protocols at a given time.
Hence, I'd like to propose for the following roadmap:

Store performance improvement to be released first, in a separate release than these changes so we don't put roadblock to improving/fixing the store performance issues Cc @jm-clius
Once the change are merged in RFC and finalized in nim-waku (up to you whether it's merged or just pending in a branch), we can work on integrating the changes in js-waku.
Once changes in js-waku are done and released, then new nim-waku release can be deploy as js-waku will be ready.

tl;dr: Give us enough time between change ready and change deployed for us to get js-waku ready.

jm-clius · 2022-02-14T08:41:59Z

tl;dr: Give us enough time between change ready and change deployed for us to get js-waku ready.

Right. This makes sense to me. @s1fr0 that would likely mean waiting for waku-org/nwaku#849 to be merged and then keeping your changes up to date with master until given a green light by js-waku and go-waku

jm-clius

Remember to change the protocol-id for 13/WAKU2-STORE to -beta4. The protocol-id is also listed in 10/WAKU2 and needs to upped there as well.

jm-clius · 2022-02-14T13:14:45Z

content/docs/rfcs/14/README.md

@@ -50,7 +49,7 @@ message WakuMessage {
  bytes payload = 1;
  string contentTopic = 2;
  uint32 version = 3;
-  double timestamp = 4;
+  int64 timestamp = 4;


Suggested change

int64 timestamp = 4;

sint64 timestamp = 4;

Thanks @jm-clius, addressed by 55829f0 (there where some other rfs affected).

content/docs/rfcs/21/README.md

…ed to beta4

We'll change to sint64 when we'll refactor chat2 to use Timestamp type for timestamps

D4nte · 2022-02-15T06:12:52Z

tl;dr: Give us enough time between change ready and change deployed for us to get js-waku ready.

Right. This makes sense to me. @s1fr0 that would likely mean waiting for status-im/nim-waku#849 to be merged and then keeping your changes up to date with master until given a green light by js-waku and go-waku

Thanks! just be sure to notify us when the change is ready :)

staheri14

The PR LGTM, is there any blocker for it to be merged? @s1fr0

content/docs/rfcs/13/README.md

content/docs/rfcs/21/README.md

content/docs/rfcs/14/README.md

s1fr0 · 2022-02-16T23:20:00Z

Hi @staheri14 , I'm waiting waku-org/nwaku#849 to be merged (see here).

oskarth · 2022-02-17T03:17:05Z

As a rule of thumb we don't want to block PRs from being merged into master, see https://trunkbaseddevelopment.com/ for methodology we are using.

If a hotfix is required this can be dealt with separately as a short-lived branch.

Since it seems we've already agreed to block until now we can keep doing so until waku-org/nwaku#849 gets merged, assuming we can do so this week. If that PR requires more spec changes, I suggest we merge this regardless.

D4nte · 2022-02-17T06:17:11Z

As a rule of thumb we don't want to block PRs from being merged into master, see trunkbaseddevelopment.com for methodology we are using.

If a hotfix is required this can be dealt with separately as a short-lived branch.

Since it seems we've already agreed to block until now we can keep doing so until status-im/nim-waku#849 gets merged, assuming we can do so this week. If that PR requires more spec changes, I suggest we merge this regardless.

IMHO, this PR can be merged as soon as it is ready. I don't see added value in blocking an RFC PR with a reference implementation PR. Just my 2 cents :)

jm-clius

Agree with comments above. I think you can go ahead and merge this, @s1fr0.

D4nte · 2022-02-18T03:47:46Z

One point I am confused about: Shouldn't the version of all protocols that use 14/WAKU-MESSAGE be upgraded?

Indeed, Relay, Filter and Lightpush use 14/WAKU-MESSAGE. Until this PR was merged, we expected the timestamp field time of Waku Messages sent and received via these protocols to be double. It is now expected to be sint64.

How does one know if a peer expect a double or sint64 timestamp?

It may also be relevant to add versioning to the WakuMessage protobuf using the package protobuf field. This would make it easier to handle such breaking changes.

D4nte · 2022-02-18T09:18:20Z

One point I am confused about: Shouldn't the version of all protocols that use 14/WAKU-MESSAGE be upgraded?

Indeed, Relay, Filter and Lightpush use 14/WAKU-MESSAGE. Until this PR was merged, we expected the timestamp field time of Waku Messages sent and received via these protocols to be double. It is now expected to be sint64.

How does one know if a peer expect a double or sint64 timestamp?

It may also be relevant to add versioning to the WakuMessage protobuf using the package protobuf field. This would make it easier to handle such breaking changes.

After more analysis, I think the best way forward is simply to use a new field on Waku Message. Making the current field deprecated. @s1fr0

vacp2p/rfc#483 vacp2p/rfc#491

* master: RFC16: add version call (#505) fix(noise): update RFC to implementation (#508) fixup: 37/WAKU2-NOISE fix images paths (#506) New RFC: 37/WAKU2-NOISE-SESSIONS (#504) 36/WAKU2-BINDINGS-API (#501) docs(16/WAKU2-RPC): add ENR to waku info (#502) Adding 35/WAKU2-NOISE to menu (#500) add RFC33 to index (#499) feat: 32/RLN raw spec New RFC: 35/WAKU2-NOISE (#496) Update on the rln registration figure to match the current spec (#497) 33/WAKU-DISCV5: Add first raw version (#487) Add pubsubTopic field to index (#492) Fix markdown links (#493) Categorize 22 & 31 (#490) Changed PB Timestamp index to 10 (#491) 13/14/16/21: Change in timestamp format (#483) add: RFC31 copyright statement (#489) 17/WAKU-RLN-RELAY: Revise spec for its draft version (#484)

oskarth requested review from staheri14 and jm-clius February 9, 2022 06:16

jm-clius suggested changes Feb 9, 2022

View reviewed changes

jm-clius mentioned this pull request Feb 9, 2022

Refactoring timestamps waku-org/nwaku#842

Merged

3 tasks

s1fr0 added 2 commits February 9, 2022 23:35

Timestamps refactored to int64 (nanoseconds resolution). Related to w…

1a314dc

…aku-org/nwaku#834

Updated times to Timestamp type

d988a6a

s1fr0 force-pushed the int64-timestamps branch from 48d4391 to d988a6a Compare February 9, 2022 22:35

s1fr0 changed the title ~~Timestamps refactored to int64 (nanoseconds resolution)~~ Timestamps refactored Feb 9, 2022

s1fr0 changed the title ~~Timestamps refactored~~ Refactoring timestamps Feb 9, 2022

Refactored timestamps

7cbd85f

s1fr0 marked this pull request as ready for review February 9, 2022 23:16

Made spec consistent with timestamp refactoring

29c89c6

oskarth reviewed Feb 10, 2022

View reviewed changes

oskarth changed the title ~~Refactoring timestamps~~ 13/14/16/21: Change in timestamp format Feb 10, 2022

jm-clius reviewed Feb 10, 2022

View reviewed changes

D4nte reviewed Feb 11, 2022

View reviewed changes

s1fr0 requested a review from jm-clius February 11, 2022 10:08

s1fr0 added 2 commits February 11, 2022 11:16

Fixed misunderstanding on proto3 types

2045af5

Fixed timestamps to sint64

6b2d91e

jm-clius suggested changes Feb 14, 2022

View reviewed changes

s1fr0 added 3 commits February 14, 2022 16:38

Fixed correct time to pb2 timestamps and Store protocol version chang…

55829f0

…ed to beta4

Revert changes (not needed for now)

1ad0cf8

Revert changes

7c9680e

We'll change to sint64 when we'll refactor chat2 to use Timestamp type for timestamps

staheri14 reviewed Feb 16, 2022

View reviewed changes

content/docs/rfcs/13/README.md Outdated Show resolved Hide resolved

content/docs/rfcs/21/README.md Outdated Show resolved Hide resolved

content/docs/rfcs/14/README.md Outdated Show resolved Hide resolved

oskarth mentioned this pull request Feb 17, 2022

Store performance improvements waku-org/nwaku#849

Merged

oskarth added the blocked This issue is blocked by some other work label Feb 17, 2022

jm-clius approved these changes Feb 17, 2022

View reviewed changes

s1fr0 merged commit d88dc12 into master Feb 17, 2022

s1fr0 deleted the int64-timestamps branch February 17, 2022 15:30

D4nte mentioned this pull request Feb 17, 2022

Implement Waku Store beta4 waku-org/js-waku#553

Closed

richard-ramos mentioned this pull request Feb 18, 2022

refactor: use int64 timestamps waku-org/go-waku#189

Merged

s1fr0 removed the blocked This issue is blocked by some other work label Feb 22, 2022

This was referenced Feb 22, 2022

Changed PB Timestamp index to 10 #491

Merged

Moved PB Timestamp index to 10 waku-org/nwaku#861

Merged

D4nte added a commit to waku-org/js-waku that referenced this pull request Feb 23, 2022

Add timestamp nanoseconds field

4d5d764

vacp2p/rfc#483 vacp2p/rfc#491

D4nte added a commit to waku-org/js-waku that referenced this pull request Feb 24, 2022

Add timestamp nanoseconds field

60168cc

vacp2p/rfc#483 vacp2p/rfc#491

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

13/14/16/21: Change in timestamp format #483

13/14/16/21: Change in timestamp format #483

s1fr0 commented Feb 4, 2022 •

edited

Loading

jm-clius left a comment •

edited

Loading

s1fr0 commented Feb 9, 2022 •

edited

Loading

staheri14 commented Feb 9, 2022 •

edited

Loading

staheri14 commented Feb 9, 2022

s1fr0 commented Feb 9, 2022

oskarth left a comment •

edited

Loading

oskarth commented Feb 10, 2022

jm-clius commented Feb 10, 2022

jm-clius Feb 10, 2022

s1fr0 Feb 10, 2022 •

edited

Loading

D4nte Feb 11, 2022 •

edited

Loading

s1fr0 Feb 11, 2022 •

edited

Loading

staheri14 commented Feb 10, 2022

D4nte Feb 11, 2022

s1fr0 Feb 11, 2022

D4nte Feb 11, 2022 •

edited

Loading

s1fr0 commented Feb 11, 2022

D4nte commented Feb 13, 2022

jm-clius commented Feb 14, 2022

jm-clius left a comment

jm-clius Feb 14, 2022

s1fr0 Feb 14, 2022

D4nte commented Feb 15, 2022

staheri14 left a comment

s1fr0 commented Feb 16, 2022

oskarth commented Feb 17, 2022

D4nte commented Feb 17, 2022

jm-clius left a comment

D4nte commented Feb 18, 2022

D4nte commented Feb 18, 2022 •

edited

Loading

13/14/16/21: Change in timestamp format #483

13/14/16/21: Change in timestamp format #483

Conversation

s1fr0 commented Feb 4, 2022 • edited Loading

jm-clius left a comment • edited Loading

Choose a reason for hiding this comment

s1fr0 commented Feb 9, 2022 • edited Loading

staheri14 commented Feb 9, 2022 • edited Loading

staheri14 commented Feb 9, 2022

s1fr0 commented Feb 9, 2022

oskarth left a comment • edited Loading

Choose a reason for hiding this comment

1) JSON RPC API

2) Protocol identifier in store protocol

oskarth commented Feb 10, 2022

jm-clius commented Feb 10, 2022

jm-clius Feb 10, 2022

Choose a reason for hiding this comment

s1fr0 Feb 10, 2022 • edited Loading

Choose a reason for hiding this comment

D4nte Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

s1fr0 Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

staheri14 commented Feb 10, 2022

D4nte Feb 11, 2022

Choose a reason for hiding this comment

s1fr0 Feb 11, 2022

Choose a reason for hiding this comment

D4nte Feb 11, 2022 • edited Loading

Choose a reason for hiding this comment

s1fr0 commented Feb 11, 2022

D4nte commented Feb 13, 2022

jm-clius commented Feb 14, 2022

jm-clius left a comment

Choose a reason for hiding this comment

jm-clius Feb 14, 2022

Choose a reason for hiding this comment

s1fr0 Feb 14, 2022

Choose a reason for hiding this comment

D4nte commented Feb 15, 2022

staheri14 left a comment

Choose a reason for hiding this comment

s1fr0 commented Feb 16, 2022

oskarth commented Feb 17, 2022

D4nte commented Feb 17, 2022

jm-clius left a comment

Choose a reason for hiding this comment

D4nte commented Feb 18, 2022

D4nte commented Feb 18, 2022 • edited Loading

s1fr0 commented Feb 4, 2022 •

edited

Loading

jm-clius left a comment •

edited

Loading

s1fr0 commented Feb 9, 2022 •

edited

Loading

staheri14 commented Feb 9, 2022 •

edited

Loading

oskarth left a comment •

edited

Loading

s1fr0 Feb 10, 2022 •

edited

Loading

D4nte Feb 11, 2022 •

edited

Loading

s1fr0 Feb 11, 2022 •

edited

Loading

D4nte Feb 11, 2022 •

edited

Loading

D4nte commented Feb 18, 2022 •

edited

Loading