Fix potential parse error in UpdateManager

[Szpoti]

Fixes #12048

This small change ensures that, in the event the sanity checks fail, we convert the correct version string into a Version object without encountering an exception.

[kiminuo]

Why do we check if there is a new version every 5 seconds? https://github.com/zkSNACKs/WalletWasabi/blame/master/WalletWasabi.Daemon/Global.cs#L80 ?

[kiminuo]

So if I understand correctly, then this is a workaround basically.

If people who release the software know that they are not suppose to create tags with, eg, letters, then there is no issue and this fix is not necessary.

If people who release the software make a mistake and release v2.0.7abc, then this patch makes sure that we download 2.0.7 ... which might not even exist...?

[Szpoti]

Why do we check if there is a new version every 5 seconds?

No particular reason to do it every 5 seconds, just to know when there is a new update. We can increase this timespan, decreasing resource usage.

If people who release the software make a mistake and release v2.0.7abc, then this patch makes sure that we download 2.0.7 ... which might not even exist...?

Let's say the client is on 2.0.6, and we release 2.0.7, with the tagname "v2.0.7b" because for whatever reason.
"api/software/versions" gives back that there is indeed, a 2.0.7 ClientVersion, which is newer, so an update is requested.
UpdateManager gets the data from https://api.github.com/repos/zkSNACKs/WalletWasabi/releases/latest, which will contain the latest release, in this case the major release 2.0.7 with the tagname "v2.0.7b".
This PR makes sure that we don't run into exception when parsing the software version from the tagname.

Because we only get data from our api software/versions and github's api releases/latest, we can't download anything that doesn't exist. As long as we do a newer major release than the client, increasing the 3rd number, UpdateManager gets it.

Now if we are already on a 2.0.7 version, and release 2.0.7abc, that changes nothing as we will not update, no newer major release was found.

[kiminuo]

Why do we check if there is a new version every 5 seconds?

No particular reason to do it every 5 seconds, just to know when there is a new update. We can increase this timespan, decreasing resource usage.

Well, are you aware that Github can ban you for this, right? And funnily my IP got banned but we keep ignoring this error so it's hidden and does not surface to user in any way. So if I'm right about this, then you attempt to download every 5s, get banned for some short period of time and then got unbanned, so then it effectively works as if you were checking every 1 hour or so.

But how could 5s checking be approved is mind boggling to me. I mean is this an oversight or was this deliberate decision?

[kiminuo]

If people who release the software make a mistake and release v2.0.7abc, then this patch makes sure that we download 2.0.7 ... which might not even exist...?

Let's say the client is on 2.0.6, and we release 2.0.7, with the tagname "v2.0.7b" because for whatever reason. "api/software/versions" gives back that there is indeed, a 2.0.7 ClientVersion, which is newer, so an update is requested. UpdateManager gets the data from https://api.github.com/repos/zkSNACKs/WalletWasabi/releases/latest, which will contain the latest release, in this case the major release 2.0.7 with the tagname "v2.0.7b". This PR makes sure that we don't run into exception when parsing the software version from the tagname.

Because we only get data from our api software/versions and github's api releases/latest, we can't download anything that doesn't exist. As long as we do a newer major release than the client, increasing the 3rd number, UpdateManager gets it.

Now if we are already on a 2.0.7 version, and release 2.0.7abc, that changes nothing as we will not update, no newer major release was found.

Sounds good.

So do we actually want to support releasing 2.0.7a and 2.0.7b, for example? Or do we plan to release such versions (from time to time)? It seems like with your PR it should work well in the sense that one gets the latest version which seems reasonable.

[Szpoti]

I mean is this an oversight or was this deliberate decision?

Oh no, those fast requests only goes towards our backend.
We only send requests to github api max 1-3 times once we got back from our backend that there is a new version on github that we need to download.

[kiminuo]

Oh no, those fast requests only goes towards our backend.

Ok, still it's a lot of requests. 17280 requests per day by a single user.

[Szpoti]

It seems like with your PR it should work well in the sense that one gets the latest version which seems reasonable.

Not true sadly/not sadly.
As 2.0.7a and 2.0.7b are both the same major version 7, if you are already at 2.0.7a, we won't download 2.0.7b, because the backend will give back "clientVersion":"2.0.7", which will be equal to your local client's version.
What can happen is we release 2.0.7a, and later 2.0.7b. If the user didn't auto-update before that to 2.0.7a, he will download 2.0.7b.

[Szpoti]

So do we actually want to support releasing 2.0.7a and 2.0.7b, for example? Or do we plan to release such versions (from time to time)?

I'm not aware of that, no. This PR just makes sure if it does happen - by accident or for a reason - we can still auto-update.

Ok, still it's a lot of requests. 17280 requests per day by a single user.

You're right with that, I'll create a PR to increase. Does 3 minutes sounds reasonable?
Also with Lucas's websocket PR, we could in theory send out a notification about a new release in the future.

[kiminuo]

Ok, still it's a lot of requests. 17280 requests per day by a single user.

You're right with that, I'll create a PR to increase. Does 3 minutes sounds reasonable?

Opinions will differ on this one but I think that 1 hour sounds like a reasonable minimum and more than a day is too much.

I would just create a PR and David will comment on that.

[kristapsk]

Ok, still it's a lot of requests. 17280 requests per day by a single user.

You're right with that, I'll create a PR to increase. Does 3 minutes sounds reasonable?

Opinions will differ on this one but I think that 1 hour sounds like a reasonable minimum and more than a day is too much.

I would just create a PR and David will comment on that.

Although our backend can handle this with ease, agree that there is no need to do this more often than once per hour. With websockets in future we could push this info from server to client, instead of polling.

[kiminuo]

It seems like with your PR it should work well in the sense that one gets the latest version which seems reasonable.

Not true sadly/not sadly. As 2.0.7a and 2.0.7b are both the same major version 7, if you are already at 2.0.7a, we won't download 2.0.7b, because the backend will give back "clientVersion":"2.0.7", which will be equal to your local client's version. What can happen is we release 2.0.7a, and later 2.0.7b. If the user didn't auto-update before that to 2.0.7a, he will download 2.0.7b.

I declare it to be a minefield then :)

I would say that it would be great to modify a release document and mention these things there ("Do not release versions that differ only by suffix").

[Szpoti]

I would say that it would be great to modify a release document and mention these things there ("Do not release versions that differ only by suffix").

I would just create a PR and David will comment on that.

Will do both. Nevertheless these are out of scope for the current PR. I still think this PR makes sense and should be merged, do you agree?

[lontivero]

Ok, still it's a lot of requests. 17280 requests per day by a single user.

You're right with that, I'll create a PR to increase. Does 3 minutes sounds reasonable?

Opinions will differ on this one but I think that 1 hour sounds like a reasonable minimum and more than a day is too much.
I would just create a PR and David will comment on that.

Although our backend can handle this with ease, agree that there is no need to do this more often than once per hour. With websockets in future we could push this info from server to client, instead of polling.

It doesn't need to be done with any frequency, it should be part of the websocket handshake, that's enough.