nimble, uuid: generate UUIDs via std/sysrand, not pragmagic/uuids
#777
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Drop two nimble dependencies. Closes: 424
The `configlet uuid` command has a limit of 1000 UUIDs. Test more than that to assert that nothing weird happens at that number. We can't make this a much higher number because: - we compile the tests in debug mode - `configlet uuid` is intended for generating a small number of UUIDs. To generate a large number of UUIDs it should read more than 16 bytes at a time from the system CSPRNG. But I ran a test locally with n = 500_000_000 here.
It's better to avoid `add`, because we know the final length of the string, and the indices where we should copy the hex string representation of each UUID byte.
Arguable readability improvement: remove the need to write `.bytes`. If the object was exported, it's not obvious that at a callsite there's only one field. There's no performance difference. A Nim object has no bloat - it was still 16 bytes before. This could be a distinct array, but borrowing the `urandom` proc and slice operator is verbose.
Overoptimization.
Hash the UUID bytes, not the string representation. This should also be faster than using HashSet[array[36, char]].
I'm okay with this test taking on the order of 100 ms (but not many seconds).
This reverts the previous commit. I didn't immediately see a nice way to write this, because std/random doesn't have a `rand` proc that can operate directly on an openArray of bytes.
ee7
commented
Aug 7, 2023
ee7
changed the title
std/sysrand, not pkg/uuidsstd/sysrand, not pragmagic/uuids
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Replace two Nimble dependencies with our own code.
Before this commit, configlet would generate a UUID with the Nimble packages pragmagic/uuids and pragmagic/isaac (itself a port of a C implementation).
However, the
uuidspackage doesn't currently work on Windows with Nim 2.0 (due to Nim removing some functions for a deprecated Win32 API). I've created a PR for the package, but:std/sysrandhas been around for 2.5 years, but still warns that it hasn't been formally audited and doesn't guarantee safety for cryptographic purposes. But it's probably good enough for our purposes: there's more use, review, and maintenance of std/sysrand than pragmagic/isaac anyway (whose most recent commit was in 2017, and ISAAC is questionable as a CSPRNG).In the tests, add a test for distinctness, and test more UUIDs. The
configlet uuidcommand sets a limit of 1000 UUIDs, so let's assert that nothing strange happens above that number. Pick the number such that the genUuid tests take on the order of 100 ms.We can't make this a much higher number because we compile the tests in debug mode, and
configlet uuidis intended for generating a small number of UUIDs. But I tested locally that the new implementation could produce 500M distinct valid version 4 UUIDs.As a future optimization we could either read more than 16 bytes each time from the system CSPRNG, or once again use the system CSPRNG to seed a different PRNG.
Other benefits of this change:
configlet uuidwould only work on Windows and platforms with/dev/urandom.genUUIDtogenUuidfit our style elsewhere.Closes: #424
See also the individual commit messages.