Skip to content
This repository has been archived by the owner on Jun 14, 2024. It is now read-only.

14-WAKU2-MESSAGE: Data type and resolution of timestamp #448

Closed
3 tasks
staheri14 opened this issue Aug 6, 2021 · 10 comments
Closed
3 tasks

14-WAKU2-MESSAGE: Data type and resolution of timestamp #448

staheri14 opened this issue Aug 6, 2021 · 10 comments
Assignees

Comments

@staheri14
Copy link
Contributor

staheri14 commented Aug 6, 2021

Problem

The data type of timestamp in Waku2 (Wakumessage and in the store protocol) is set to float64, which represents seconds in the integer part and sub-seconds in the fractional part. However, this is not certain whether float64 is an appropriate type or int64 shall be used instead. Another question is also about the desired resolution i.e., seconds only or sub-seconds.

Opened the issue to address the following comment #406 (comment)
cc: @oskarth

Acceptance Criteria

  • To investigate the data type as well as the resolution used for the timestamp in other specs
  • To agree on the required resolution for the timestamp in Waku message
  • According to the result of the preceding items, then we can decide on whether to keep the current float64 type or to make an update
@staheri14 staheri14 self-assigned this Aug 6, 2021
@staheri14
Copy link
Contributor Author

staheri14 commented Aug 6, 2021

Update:
Based on what I have found so far in nim-eth and nimbus-eth1, the time is either:

  • saved as uint32/64 with seconds resolution e.g., in RLPx whisper Envelope (expiry field) and in discovery4 like in here
  • or as float64 with sub-seconds resolution e.g., in discovery5 PeerPool (lastLookupTime field) and KBucket(lastUpdated field)

There is also the use of the Time type which encapsulates two fields, one for seconds and the other for nanoseconds.

I still could not find a use of unit64 with sub-seconds resolution.

@staheri14
Copy link
Contributor Author

staheri14 commented Aug 6, 2021

Further Updates:

The Unix timestamp is commonly known and calculated as

the number of seconds since January 1, 1970, 00:00:00 UTC

The implementation of Twitter pagination and Facebook time-based pagination also adheres to the seconds resolution.

From my point of view and based on the research I have done so far, it is up to us to decide on our preferred time resolution and also be clear about that in the specs.

@staheri14
Copy link
Contributor Author

staheri14 commented Aug 6, 2021

@jm-clius @D4nte @richard-ramos thoughts/comments?

@richard-ramos
Copy link
Member

richard-ramos commented Aug 6, 2021

For what is worth, in Go I do have access to the unix epoch in seconds and nanoseconds, althought both of them are int64.

func (t Time) Unix() int64
func (t Time) UnixNano() int64

In order to support the float64 timestamps that WakuMessage has, I need to perform this conversion:

theTime := time.Now().UnixNano()
result: float64(theTime) / float64(time.Second)

@staheri14
Copy link
Contributor Author

For what is worth, in Go I do have access to the unix epoch in seconds and nanoseconds, althought both of them are int64.

func (t Time) Unix() int64
func (t Time) UnixNano() int64

In order to support the float64 timestamps that WakuMessage has, I need to perform this conversion:

theTime := time.Now().UnixNano()
result: float64(theTime) / float64(time.Second)

I see! Then the use of float64 for time is not conventional in Go.
I also agree that the data type of float64 is not very common to capture time.

@staheri14
Copy link
Contributor Author

staheri14 commented Aug 7, 2021

Some other inputs from nimbus dev-helpdesk:

usually OS do not store sub-seconds as floats... because its expensive even on modern CPUs, so conversion to floats usually artificial

Thus int64/uint64 is a more meaningful and common choice for time (but not float values). We are getting to a consensus re the data type; which should be uint or int.

Also, re being unsigned or signed, signed is a safer choice and avoids arithmetic bugs.

@oskarth
Copy link
Contributor

oskarth commented Aug 9, 2021

@kdeme thoughts from Nim POV? Was there a specific rationale for:

or as float64 with sub-seconds resolution e.g., in discovery5 PeerPool (lastLookupTime field) and KBucket(lastUpdated field)

@D4nte
Copy link
Contributor

D4nte commented Aug 11, 2021

FYI JS uses milliseconds since epoch (integer): https://www.w3schools.com/jsref/jsref_gettime.asp

Howevever, I don't think it should have an influence on the decision.

No other thoughts/comments from my side.

@kdeme
Copy link
Contributor

kdeme commented Aug 12, 2021

@kdeme thoughts from Nim POV? Was there a specific rationale for:

or as float64 with sub-seconds resolution e.g., in discovery5 PeerPool (lastLookupTime field) and KBucket(lastUpdated field)

The peer pool code linked is not discovery v5 code and much older and not the best example to look at nowadays.

In discoveryv5 (and probably in most more recent code in general) we use chronos Moment and the now proc, for example here: https://github.com/status-im/nim-eth/blob/a8d11dd30bf0b844e049702353addbbca803845c/eth/p2p/discoveryv5/protocol.nim#L131

I guess it depends on what the timestamp is used for, but for the cases linked in nim-eth code I believe it is the better and more simple way to store/transfer the "timestamp" (they are really used as a moment, to compare with another moment in time). It is stored as int64 so that sets you safe, and also gives nanosecond resolution if you need that (which is not needed in these cases but ok). If you do need a more humanly representable format, you can just do a conversion.

The discv4 and nim-eth p2p code should be altered to also use this.
The Whisper / Waku code uses uint32, as that is what the spec always stated since Whisper times.

@staheri14
Copy link
Contributor Author

Closing the issue since it has been addressed in waku-org/nwaku#834

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

No branches or pull requests

5 participants