Incorrect parsing of high-precision timestamps #324

Closed boreq closed 1 year ago

A while ago I created this commit https://github.com/planetary-social/go-toolbelt/commit/0f7ad206c2b74c967534cc3352031d74f391e6e2 after forking this library to fix it just in Planetary as I didn't understand what implications my changes could have for other users of go-ssb. The original repo is here https://git.sr.ht/~cryptix/go-toolbelt.

This is an attempted fix to the way the timestamps are parsed in messages. The old code parses values differently if they contain a dot (e.g. 12.34). Right now I have no energy to once again figure out how the value is interpreted if it has a dot in it but if that is the case the value isn't interpreted as milliseconds but as something else. This causes some messages with high-precision timestamps in milliseconds to be interpreted as having a timestamp way in the future. Then they can be stuck at the top of the feed in a client.

The library being referenced seemingly provides a type called Millisecs that is supposed to be generic and not SSB specific so I believe that the naming of the type or documentation around is is incorrect. Perhaps we should just move this code to go-ssb and drop this dependency. I feel like this is such a tiny piece of code that it is easier to maintain it in the same library. Furthermore the library seems to be something that contains various unrelated Go programming language utility functions that seemingly shouldn't be grouped together as they have very different purposes.

For context according to protocol guide the timestamp should always be expressed in milliseconds. Manyverse also follows this standard. I understand that this behavior is based on some kind of a weird past behaviour of some clients. Perhaps it is time to move on and simplify this code?

My proposed action plan based on the info above would be:

copy Millisecs to this repo
fix it just like it was fixed in the fork (so I guess just copy the code from the fork and use that)

That being said I don't fully understand the historical context behind this quirk.

edit:

Actually this may be more complicated to fix than I thought as this specific type is used by:

Perhaps @staltz knows more if the JavaScript stack also contains some kind of a fix for this quirk. If not I think we should drop this code.

See the npm package monotonic-timestamp that Dominic created for this problem. It's used in ssb-validate

See the npm package monotonic-timestamp that Dominic created for this problem. It's used in ssb-validate

I am not entirely sure that this is the same problem. I am specifically referring to the fact that sometimes the value of the timestamp field is decoded differently. I understand that monotonic-timestamp just adds some extra stuff after the dot so the value itself is still in milliseconds.

That being said I don't understand why there is a requirement for the timestamp to always increase somewhere in the JS SSB codebase. If we measure in ms then obviously quick calls return the same results and there is nothing weird about this.

Also just to be clear "monotonic" means "non-decreasing" or "non-increasing" so getting the same value many times still means something is monotonic so again there is some confusion in the naming here (in the context of clocks in computer programs at least I believe).

Yes, I agree with you that the adherence to the definition of monotonic wasn't correct. Perhaps @arj03 can shed some light on why equal timestamps are a problem in SSB JS.

:+1: But I think the problem here is not specifically with this monotonic timestamp library but rather the fact that sometime in the past there was a client which didn't encode the time as milliseconds but rather for example nanoseconds or something like that and this code that I found tries to correct for that by guessing that if a timestamp contains a dot in it then it means that it is in ns and not ms. This seems wrong as the code you linked produces fractional timestamps in ms. Therefore we can't use the dot to guess if it was in ms or something else.

Yes, I that check seems wrong. In JS sometimes if you ask for time you'll get these fractional float values, that has always annoyed me. But they should all still be in the same ms format, and not in ns.

If I'm reading this right, then you're GTG @boreq with your proposal?

I'm down to review/merge/migrate/click buttons.

Upstream doesn't indicate how to contribute patches but I will try sending a patch to the email address that I got from the git logs.

Exchanged some emails with @cryptix and this course of action was suggested:

[x] move the type to ssb-refs https://github.com/ssbc/go-ssb-refs/pull/6
[x] tag a new go-ssb-refs version
[x] update https://github.com/ssbc/go-metafeed https://github.com/ssbc/go-metafeed/pull/30
[x] update https://github.com/ssbc/go-gabbygrove https://github.com/ssbc/go-gabbygrove/pull/3
[x] update https://github.com/ssbc/go-ssb https://github.com/ssbc/go-ssb/pull/325

Opinions?

welcome having this of my turf! i already :+1: the root PR and the rest is just renaming references so take this nod as going forward with the others once that is merged and tagged!

just while we are here anyhow: IIRC the increasing timestamps were intended to be used as a social "happened before" "proofs" but Dominic later realised that he could do that without taking the timestamp into account.

Nice! @boreq https://github.com/ssbc/go-ssb-refs/pull/6 is merged and is out https://github.com/ssbc/go-ssb-refs/releases/tag/v0.5.2. Wanna do the update labours on https://github.com/ssbc/go-ssb/pull/325 https://github.com/ssbc/go-gabbygrove/pull/3 and https://github.com/ssbc/go-metafeed/pull/30? Can also take a run if you like.

Ok all three remaining pull requests have been updated.

Nice, merged. Annnd https://github.com/ssbc/go-ssb/pull/326 is also in. All good to go here? Nice coordination folks!

I think we can close this unless we find more references to encodedTime in some packages? I am on my phone but I can check again in the evening.

Ah, I think https://github.com/ssbc/go-ssb-room/issues/372 needs to be done but we can follow up there.

I didn't see another reference in the go-ssb stack but feel free to re-open if you find one and we get to it!

~~Oh wait, there is more: https://github.com/search?q=org%3Assbc+encodedTime&type=code?~~

Oh wait, that's old. I can't find anything on third look, so yeh, gonna close this off again :upside_down_face:

ssbc / go-ssb

Incorrect parsing of high-precision timestamps #324