Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Feature: Blake2xb and LtHash #40

Open
wants to merge 81 commits into
base: develop
Choose a base branch
from
Open

Feature: Blake2xb and LtHash #40

wants to merge 81 commits into from

Conversation

liss-h
Copy link
Contributor

@liss-h liss-h commented Apr 26, 2023

TODOs: (some of the stuff is probably more something for you, alex)

  • fix cmake conan integration so that sodium is actually fetched
  • maybe figure out how to disable AVX2 or SSE extensions for sodium to get accurate benchmark results
  • Since this is based on folly: License stuff? (not copy pasted but idk how this works)

More general:

  • figure out if and how we want to integrate them with the rest of the library -> for now we don't want to integrate
  • implement allocator support
  • update readme
  • add examples
  • implement blake2b wrapper
  • documentation
  • fix cmake and conan @bigerl

@liss-h liss-h requested a review from bigerl April 26, 2023 10:31
@bigerl
Copy link
Member

bigerl commented May 2, 2023

with gcc-12 I get:

dice-hash/include/dice/hash/blake2xb/Blake2xb.hpp:143:45: error: ‘constexpr’ static data member ‘private_tag’ must have an initializer
  143 |                 static constexpr PrivateTag private_tag;

@liss-h
Copy link
Contributor Author

liss-h commented May 3, 2023

Thx for getting the basics set up. I managed to fix the test and example runs, but have no idea why the two failing actions are failing so would be nice if you could have a look at those two.

Btw: needed to update to gcc11 and clang11 because gcc10 and clang10 were not able to build the project anymore.

@liss-h
Copy link
Contributor Author

liss-h commented May 3, 2023

And update on sodium: The license is ISC so copying it over would be fine.
I think just copying over this folder should do it.

@liss-h
Copy link
Contributor Author

liss-h commented May 4, 2023

So, since integrating libsodium here directly turned out to be a little more problematic than anticipated, I instead added a flag to conditionally enable libsodium support (and therefore Blake2b, Blake2Xb and LtHash).

The flag is called WITH_SODIUM, as in cmake -DWITH_SODIUM=OFF ....
The default is currently ON but I can change that of course if you prefer @bigerl.

@mcb5637 mcb5637 marked this pull request as ready for review September 2, 2024 09:58
@mcb5637
Copy link
Contributor

mcb5637 commented Sep 2, 2024

POBR diff fails, because i fixed the files to be checked by it

@liss-h liss-h self-assigned this Sep 2, 2024
Copy link
Contributor Author

@liss-h liss-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

First review, mostly buildsystem related

conanfile.py Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
cmake/conan_cmake.cmake Outdated Show resolved Hide resolved
cmake/lib-config.cmake.in Show resolved Hide resolved
examples/policyUsage.cpp Outdated Show resolved Hide resolved
Copy link
Contributor Author

@liss-h liss-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

highway stuff looks good to me 👍

src/dice/hash/blake/internal/blake3/.gitignore Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
Copy link
Contributor Author

@liss-h liss-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Some stuff left

test_package/conanfile.py Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
Copy link
Contributor Author

@liss-h liss-h left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think this is good to go now.

Just to be extra sure: could you try if it works as a dependency in another library?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants