Repair UUIDs duplicates (storage 3.1 upgrade)

[Athozus]

This is a proposal for the most important hotfix of this release. What does it do ?

• It first checks, when giving an UUID, if it is not already used in the database ;
• At the startup, in after migrating in migrate.lua, it checks for non-duplicates, and if there are, it gives new ID to all messages that are not the same (mail.are_message_sames()). Of course, because it iterates through all messages, after fixing message1, it checks message2, so if there is a message3 also concerned by the duplicate, it is fixed in the message2 loop.

I tested it with 10 000 messages, it runs quite good (no performance loss). If you want to test it, you can use the /mail_spam command written in #103.

Of course, performance can still be improved, but that's not really a mess here.

I ping @Bastrabun because he was the first who noticed the issue on Your Land.

Fix #122, #126.

[BuckarooBanzay]

did i read this correctly: the "repair" code runs on every startup, right?

and: are you sure this is needed, uuid collisions should be borderline impossible :/

[S-S-X]

Could you @Athozus or @Bastrabun reveal what is the issue with current RNG or implementation? And if there's issues with RNG on Your Land could it be that RNG is compromised by one of the mods?

It is true that it is easy to trash math.random, if that's the issue then better solution would be to use Minetest's own RNG API like PcgRandom which you could, if you feel paranoid, initialize with SecureRandom.

[Athozus]

The issue was fixed in #137. However, « contaminated » databases were already infected.

So it is repairing that duplicates caused by the bug fixed in #137.

Maybe, it should not be run at every startup, but like when passed into a command ? Like /mail_repair, only available for admins ?

[S-S-X]

Maybe, it should not be run at every startup, but like when passed into a command ?

If it is to fix duplicates caused by now fixed bug then you could just automatically run it once and then increment database version to v3.1 or v4 depending on which number you like most / if you want to use some versioning scheme that differentiates breaking changes from bug fixes and feature additions.

[Athozus]

v3.1 is a good way to do it, indeed.

[Athozus]

Applied your suggestions 👍

[Athozus]

docker-compose command-not-found in the checks. Without that error, they all have succeed. I don't understand how to fix this.

[S-S-X]

docker-compose command-not-found in the checks. Without that error, they all have succeed. I don't understand how to fix this.

I guess it is issue with action runner, not mail mod. @BuckarooBanzay might have idea.

[BuckarooBanzay]

docker-compose command-not-found in the checks

had that somewhere else yesterday too, seems like a github issue, just ignore it :)

@Bastrabun could please elaborate on the uuid/rng-issue, i'm curious how something like this would happen 🤔

[Athozus]

docker-compose command-not-found in the checks

had that somewhere else yesterday too, seems like a github issue, just ignore it :)

I ran it, it was fixed, thanks !

[Athozus]

Applied you suggestions in latest commit c052e59. Sounds ready to merge (I requested a review anyway).

So now it focuses only on migrate.lua, it no longer checks for a chance over millions (if not billions) of duplicate.

The risk was of 1 over 16³⁰.12 = 1,6.10³⁷. Minetest servers can be very popular, but 😁

[Athozus]

@Bastrabun could please elaborate on the uuid/rng-issue, i'm curious how something like this would happen 🤔

Btw, it was not an error from RNG, it was due to the bug fixed in #137, which use the selected message id into compose.lua, so for a new message.

[BuckarooBanzay]

code looks sane-ish, didn't have time to test though :/

[S-S-X]

Minor suggestions for migrate.lua, don't really care if these get applied or not.

util/uuid.lua however should be just reverted to master branch as it has not been changed other than whitespace.

Drop uuid.lua from changeset and fine for me then.

[S-S-X]

Seems fine, didn't test however.

[Athozus]

Note that it is still upgrading to 3.1, I didn't changed that. It is an important PR so I prefer to remind you, as it was discussed a lot on our servers.