chore: add timestamp and ephemeral for opt-in dos validator #1713
Conversation
| let now = getNanosecondTime(getTime().toUnixFloat()) | ||
| let window = getNanosecondTime(MessageWindowInSec) | ||
|
|
||
| if now > ts: |
There was a problem hiding this comment.
open for other ways.
cant do abs(now-ts) since im working with uint64 and if I cast to signed, I will lose precision. So its the way I found.
There was a problem hiding this comment.
Maybe https://nim-lang.org/docs/times.html#abs%2CDuration from times can be used
There was a problem hiding this comment.
Sorry, mabe it is a completely stupid question, but would losing precision matter? Is the concern that someone could make the difference between TS and now overflow and thus the result of abs() be < window?
Can we check for overflow?
There was a problem hiding this comment.
ou between is indeed what I needed, but since in nwaku we generally use our own Timestamp (and not te Duration) I think this may be simpler. Less castings, etc.
Sorry, mabe it is a completely stupid question, but would losing precision matter? Is the concern that someone could make the difference between TS and now overflow and thus the result of abs() be < window?
mm havent done the maths if matters or not. Difference between ts and now will never overflow, since it will be always a smaller value. But since both values are uint64 I need to do ts-now or now-ts to ensure the result is not negative. Hence why i compare.
Mathematically is just abs(now-ts) but in practice thats not possible afaik without converting to int64 with a possible lose in precission.
There was a problem hiding this comment.
Wait, but Timestamp is an alias for int64 not uint64, unless I'm missing something?
There was a problem hiding this comment.
ouch indeed. Totally assumed it was uint64, since otherwise it would be a waste of bytes (assuming negative timestamps doesnt make sense). my bad.
fixed, way easier da7814a
some quick napkin math @vpavlin int64 overflow with 9223372036854775807, since its nano seconds thats 9223372036 . 854775807 and 9223372036 in unix timestamp its Fri Apr 11 2262 23:47:16 GMT+0000 so we are covered.
thanks all!
alrevuelta
requested review from
richard-ramos,
vpavlin,
jm-clius,
Ivansete-status and
kaiserd
There was a problem hiding this comment.
LGTM. We need overflow-proof arithmetic for waku Timestamps. In general, though, we want to stop people from directly having to choose between std/times and chronos/timer and just use waku_core/time which should eventually wrap what is needed from both the above. This is beyond the scope of this PR, of course.
| # Returns true if the message timestamp is: | ||
| # abs(now - msg.timestamp) < MessageWindowInSec | ||
| let ts = msg.timestamp | ||
| let now = getNanosecondTime(getTime().toUnixFloat()) |
There was a problem hiding this comment.
waku_core (exporting time) already provides getNowInNanosecondTime() so you don't have to import std/times
There was a problem hiding this comment.
yes right, i keep forgeting.
fixed 3092a51
| @@ -4,16 +4,20 @@ else: | |||
| {.push raises: [].} | |||
|
|
|||
| import | |||
| std/[math,times], | |||
There was a problem hiding this comment.
I think we should try to avoid importing std/times (especially when we use chronos.timer constructs in the same module). In this case we need it for absolute system time, but waku_core/time provides the wrappers for that.
|
Merging, failing experimental test is a known issue. |
fryorcraken
added
the
E:Opt-in message signing
timestampandephemeralof the message in the app hash calculation.