Klemens Morgenstern wrote:

Another example that I would be interested in is how one would hash a `boost::json::value`.

Good suggestion, thanks. In theory, this is how one would do it: namespace boost { namespace json { template void tag_invoke( boost::hash2::hash_append_tag const&, Hash& h, Flavor const& f, boost::json::value const& v ) { boost::json::visit( [&](auto const& w){ boost::hash2::hash_append( h, f, w ); }, v ); } } // namespace json } // namespace boost Unfortunately though, this doesn't work as-is for subtle reasons, and I wonder whether I need to change the tag_invoke signature somewhat so that I can prevent these errors from happening. The problem here is that boost::json::value is implicitly constructible from many things, which makes the above tag_invoke overload also match those many things in addition to json::value. (We want it to only match json::value and nothing else.) The correct way to write the tag_invoke overload today is namespace boost { namespace json { template std::enable_if_t< std::is_same::value > tag_invoke( boost::hash2::hash_append_tag const&, Hash& h, Flavor const& f, V const& v ) { boost::json::visit( [&](auto const& w){ boost::hash2::hash_append( h, f, w ); }, v ); } } // namespace json } // namespace boost The other subtlety here is that boost::json::object should be considered an unordered range, rather than an ordinary range. It doesn't expose a `hasher` typedef, which is the heuristic is_unordered_range uses, but it already specializes is_unordered_range here: https://github.com/boostorg/json/blob/7f0bceb81280df758c7b4a7cacf8779351ebcc... so we're covered.

On Sat, Dec 7, 2024 at 8:16 AM Peter Dimov via Boost wrote:

The other subtlety here is that boost::json::object should be considered an unordered range, rather than an ordinary range.

Why?

It doesn't expose a `hasher` typedef, which is the heuristic is_unordered_range uses, but it already specializes is_unordered_range here:

Should we add a hasher typedef? Thanks

On Sat, Dec 7, 2024 at 7:27 AM Klemens Morgenstern via Boost < boost@lists.boost.org> wrote:

I wonder about the usefulness of pointers.

A use-case for pointers is: std::unordered_map< T*, std::weak_ptr<T> > Where the T is constructed via std::make_shared. Note: If any std::weak_ptr references the control block created by std::make_shared after the lifetime of all shared owners ended, the memory occupied by T persists until all weak owners get destroyed as well, which may be undesirable if sizeof(T) is large. Thanks

Ion Gaztañaga wrote:

Hi,

This is my review of this library

Thank you for the review. That's a lot of questions. :-)

- "HashAlgorithm::block_size" is an int, IMHO it should std::size_t.

Maybe. The type doesn't matter that much for integral constants, but I suppose size_t is more correct.

- No move constructor/assigment for algorithms. I don't know if those make any sense, but I imagine user implementations with those members are perfectly valid.

Hash algorithms should be allowed to provide a move constructor or a move assignment operator, yes.

- The semantics for copy constructor/assignment might not be clear. The library internally uses the copy constructor (e.g. when implementing the unordered range hash), so it certainly expects something more concrete than "equivalent to the original".

"Equivalent to the original" is the strongest possible requirement. I don't see how the library can possibly expect anything more than that. In particular,

I imagine a copy/assigned algorithm is required to lead to the same output when updated with the same data.

... this is implied by "equivalent to the original", because if the copy produced a different output, it wouldn't be equivalent.

- Should the HashAlgorithm support calls to "update" and "result" several times, interleaving both calls?

It should, yes. Any calls are allowed, in any order.

- I don't know if SHA-1/RIPEMD-128 should be in the library at all (insecure and not sure if useful in any real context).

SHA-1 is still widely used, e.g. for Git commit hashes. We added RIPEMD-128 in order to have a recommended replacement for MD5 in the case where the digest has to be exactly 128 bits.

- The original proposal mentions a "variadic version of hash_append", (https://www.open- std.org/jtc1/sc22/wg21/docs/papers/2014/n3980.html#variadic) maybe that would be a useful utility in the library.

It might be, yes.

- The original proposal uses a different syntax:

void operator()(void const* key, std::size_t len) noexcept { state_.Update(key, len); }

explicit operator result_type() noexcept { std::uint64_t h1, h2; state_.Final(&h1, &h2); return h1; }

Which are the advantages of the new approach (update/result) vs the original proposal?

It seems obvious to me that named functions are better than unnamed functions.

- The hash_append_unordered_range function is surprising for people like me that are not experts on hashing, but it's an important concept for users. The original proposal explains the problem (https://www.open- std.org/jtc1/sc22/wg21/docs/papers/2014/n3980.html#unordered), I feel the problem should be also explained in the documentation of Hash2, and how the library design solves it (using a copy of the HashAlgorithm, as proposed in n3980).

N3980 also says that "And there are various other schemes. They are all implementable. But they each have their advantages and disadvantages. Therefore this proposal proposes none of them. Should the future expose the ideal hash_append specification for unordered containers, it can always be added at that time."

Why does Hash2 offer a single alternative?

I don't know of a better one. Constructing a temporary std::set is not a serious alternative. In addition to being allocating, slow, and potentially throwing, it also relies on the existence of operator<, which an unordered container doesn't require.

-N3980 proposes a "type-erase hasher" (https://www.open- std.org/jtc1/sc22/wg21/docs/papers/2014/n3980.html#type_erased_hasher ), would it be interesting to include it in Hash2?

I had the type erased hasher in the "planned additions" list for a long time, partly because I thought I needed it for the hash2sum example. But it doesn't meet the hash algorithm requirements, because it doesn't have the correct constructors and because the copy constructor is allocating and potentially throwing (the latter is not, strictly speaking, a violation but it's still undesirable.)

Is tag_invoke superior because we can declare customization points for classes that we can't modify?

Yes.

But we need access to data members so we need priviledge access to that type...

Not always. E.g. std::complex or std::error_code provide accessors for the necessary members.

- The documentation shows (in the chapter "Use with Unordered Containers") several "hash" classes that are needed to be able to use Hash2 with std/boost unordered_xx classes (or boost::intrusive::unordered_xxx types). I think it would be a good idea to include such utilities in this Hash2 library (under names like "hasher", "seeded_hasher".

One of these, or maybe some different one, will eventually be included, once I decide on which.

- The design says "The size_type member type of the flavor affects how container and range sizes (typically of type size_t) are serialized. Since the size of size_t in bytes can vary, serializing the type directly results in different hash values when the code is compiled for 64 bit or for 32 bit. Using a fixed width type avoids this".

I don't agree with this, between 32 and 64 bit compilation you will have differences when hashing user-defined types that have pointers, stored std::size_t/ptrdiff_t members, etc..

There are still a lot of types for which the size type is the only source of variability, e.g. std::vector<int>.

IMHO xxx_flavours defined in the library should define size_type as "std::size_t". Those wanting portable hashes between 32 and 64 bit need to do a bigger effort than configuring Hash2, and probably defining size_type as std::uint64_t will reduce the performance of 32 bit users...

It doesn't. I initially had size_type defined as uint32_t for similar reasons, but it doesn't really matter, so I changed it to uint64_t.

- One thing I don't like is that the library depends on container_hash/describe and mp11. I would expect this library to be dependency-free (other than Config/Assert). Specially I dislike the "Describe" dependency. Is there a way to support Described classes without depending on that library?

No.

- There is an asymmetry between "writeNNle" and "writeNNbe" implementations (little vs big endian). In little endian implementations "detail::is_constant_evaluated() and endianess" is used to be able to use memcpy in some cases (https://github.com/pdimov/hash2/blob/develop/include/boost/hash2/detail /write.hpp#L26) where in the big endian implementation this is not performed (https://github.com/pdimov/hash2/blob/develop/include/boost/hash2/detail /write.hpp#L73).

Probably few people use big endian platforms (except those like me that still need to compile for powerpc targets...), but I would expect an equivalent implementation for big-endian functions.

The implementations are equivalent in practice. GCC and Clang recognize the pattern used and turn it into a single mov; the memcpy path is only necessary for MSVC, which doesn't support big endian architectures.

- class Str example: N is missing the type, missing <algorithm> include, operator== has the wrong type...

Sorry, why does operator== have the wrong type?

- All "Use with Unordered Containers" examples should be compilable, including the needed headers.

The examples are actually compilable, I just stripped the #includes when embedding them in the documentation to avoid the unnecessary repetition. Maybe I shouldn't have. :-)

Ion Gaztañaga wrote:

- I don't know if SHA-1/RIPEMD-128 should be in the library at all (insecure and not sure if useful in any real context).

Amusingly enough, I'm _right now_ looking at an API documentation which requires HMAC-SHA1. :-) (Proprietary and NDAed, of course.)

Matt, et al, I have read over the documentation for the Hash2 library, and what follows are my notes. First and foremost, *I appreciate the effort and detail that has gone into getting the documentation this far - no mean amount of work*. Also appreciate that there are plenty of links to external topics and many examples - all helpful, and perhaps some of the things I mention here are already planned. *General Points:* 1. Just a little confused as to the name "Hash2" when there is no "Hash1" ? Perhaps this was explained earlier and I missed it. 2. With all the doc in one single page I found the absence of an obvious Search function awkward, but perhaps this is forthcoming through another effort? I found I wanted to search for words such as "required", "optional", "hash_append" and so on. 3. I agree with other commenters that the all-in-one-page approach is daunting ("Grandmother's fruitcake"). Would have liked to see at least all the top level topics as separate pages, though ideally Reference material is better as one page per construct (class, method, property, structure) so a dev can link to that page when they are working hard on an implementation of the construct. *Contents:* 1. The "Supported Compilers" section should be moved up after the Overview, as part of a section "Getting Started with Hash2". 2. The Getting Started section should work through one example that a newcomer to hashing can understand: a. installing Hash2 b. linking/compiler requirements or options c. provides a short but meaningful example d. running and explaining the output 3. The Getting Started section could use a "Common Errors and Issues" section detailing the most likely errors, misconceptions or stumbling blocks and how to respond. And/Or a Troubleshooting section for the library as a whole. 4. Consider following this with a "Tutorial" section - repeating the steps of the Getting Started section, but with a meatier and potentially useful example. 5. Usage Examples, the code examples have almost no comments - would benefit from being extensively commented. - same for other code snippets throughout the doc - though "Examples" takes priority 6. The "Example" sections in the Reference would also benefit from a sentence/para explaining what the example does, in particular noting any gotchas or potential pain points. 7. In the doc: Described classes (using Boost.Describe). Boost.Describe should be a link to that libs docs. Same for mentions of other libraries. 8. In the block_size section for Hash Algorithm Requirements, there are the two paragraphs (my bold): Cryptographic hash functions provide a block_size value, which is their block size (e.g. 64 for MD5, 128 for SHA2-512) and is *required *in order to implement the corresponding HMAC. block_size is an *optional *requirement. These statements appear to be contradictory - and there is no such thing as an "optional requirement". There is such a thing as an "optional parameter/setting". Consider searching the doc for the phrase "optional requirement" and amending. Consider searching for other "required/optional" statements and verify there is no ambiguity. 9. Nit. Replace "e.g." with "for example, " - it is so much easier to read. Same for other contractions (i.e. = "that is" etc. = "and similar/so on"). 10. Should there be an "Acknowledgements" section near the end - design input, testers? I am the technical writer for the Cpp Alliance. Hope this helps. Best of luck with the project. - Peter Turcan On Mon, Dec 9, 2024 at 12:45 PM Matt Borland via Boost < boost@lists.boost.org> wrote:

...

I vote to ACCEPT this library.

...

Zach

Zach, thank you for your review and comments.

Matt _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

El 10/12/2024 a las 1:47, Peter Dimov via Boost escribió:

Peter Turcan wrote:

...

1. Just a little confused as to the name "Hash2" when there is no "Hash1" ? Perhaps this was explained earlier and I missed it.

"Hash1" is the library formerly known as Functional/Hash, and now named ContainerHash. It defines the function object boost::hash.

(Whereas Hash2 defines its stuff in namespace boost::hash2.)

Since Hash2 depends on ContainerHash for several traits and functions both have the same dependencies (Config, Assert, Describe, Mp11), and both are about hashing... Wouldn't make sense to merge both libraries into a single Hash library? Best, Ion

I have read over the documentation for the Hash2 library, and what follows are my notes.

Peter, thank you for the documentation review. I always find these beneficial. Matt

Christopher's Review... I vote to unconditionaly ACCEPT Hash2.

Chris, thank you for the review and portability testing. Matt

...

What I'd like to see short-term:   * Handle enhanced compiler warnings.

Would you please go into more detail here? We currently test with -Wall -Wextra.

Yes. I would like to. I appreciate and benefit from (at bare minimum in GCC): -Wall -Wextra -Wpedantic -Wconversion -Wsign-conversion And sometimes I even go for -Werror You would face a much larger onslaught of warnings with a commercial-grade CERT-C++ checker, so that set above is actually rather slack in my world. I can provide qan actual build-log with line numbers if needed. Or I can just PR the changes if you like. The whole warning-game is just like, hey, why the noise? 90% of the warnings are useless, but every once in a while you get a nugget that (might) prevent you a really big crash. So I just activate a bunch of warnings, handle them all and simultaneously weed out the good stuff. Here is my typical warning set "on the metal" real-time-cpp/ref_app/target/app/make/app_make.gmk at 8cc0ce3dd26c30eeba21a1826716f7604f5ef2e4 · ckormanyos/real-time-cpp | | | | | | | | | | | real-time-cpp/ref_app/target/app/make/app_make.gmk at 8cc0ce3dd26c30eeba... Source code for the book Real-Time C++, by Christopher Kormanyos - ckormanyos/real-time-cpp | | | Kind regards, Christopher On Tuesday, December 10, 2024 at 09:28:10 PM GMT+1, Peter Dimov via Boost wrote: Christopher Kormanyos wrote:

But here comes the fun part. When I studied the implementation it seemed highly portable. So I put it onto several embedded bare-metal systems. The code compiled and ran on 32-bit microcontrollers and even ran on an 8-bit controller. Rarely do we find such portability and quality in Boost-proposed libraries, or anywhere for that matter. Kudos on a job well done!

Thanks Christopher. :-)

What I'd like to see short-term:   * Handle enhanced compiler warnings.

Would you please go into more detail here? We currently test with -Wall -Wextra. _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

...

  * Include a subset of NIST testing.

I did think about a scenario where we would've committed the .rsp files to the repo and run them during CI or some such as part of a much more extensive test suite.

Thanks for responding, Christian. I think including and running the short test vectors might be a good compromise for your CI. They run rather quickly and I don't think that'll slow down CI too much.

...

  * Fuzzing run(s) on some hashes in CI.

Which hashes would you like to see fuzzed, assuming "all of them" is off the table? And how long should we fuzz as well?

I think one of them, your pick, like SHA2-512 maybe for 5 minutes in CI would add robustness. Maybe limit the fuzzed messages to 2 < len < 1024 bytes? This is not required in any way, although I think this would increase robustness. Christopher. On Wednesday, December 11, 2024 at 12:26:07 AM GMT+1, Christian Mazakas via Boost wrote: On Tue, Dec 10, 2024 at 12:07 PM Christopher Kormanyos via Boost < boost@lists.boost.org> wrote:

What I'd like to see short-term:   * Handle enhanced compiler warnings.   * Include a subset of NIST testing.   * Fuzzing run(s) on some hashes in CI.   * I think SHA-3 is worthy of inclusion.

I was wondering what kind of NIST testing you're alluding to. We do have some copy-pasted test vectors for myriad PDFs but for a good portion of the algorithms, we're using the test vectors outlined here: https://github.com/pdimov/hash2/blob/7a25f8518692b657e9272884519519fbaca2ec5... https://csrc.nist.gov/Projects/Cryptographic-Algorithm-Validation-Program/Se... Towards the bottom there under Test Vectors, one can download a .zip folder full of .rsp files which were used to verify the output of the applicable algorithms. I tried to avoid relying on those too much during testing because I wanted something quasi-human readable and understandable so if there was an applicable PDF, I preferred that. I did think about a scenario where we would've committed the .rsp files to the repo and run them during CI or some such as part of a much more extensive test suite. Which hashes would you like to see fuzzed, assuming "all of them" is off the table? And how long should we fuzz as well? I'm not sure if we can exhaustively fuzz the algorithms as part of a normal CI infrastructure. - Christian _______________________________________________ Unsubscribe & other changes: http://lists.boost.org/mailman/listinfo.cgi/boost

Zach Laine wrote:

One example is digest<>. The docs say, "digest<N> is a constexpr-friendly class template similar to std::array. It is used to store the resulting message digest of hash algorithms such as SHA2-256 or RIPEMD- 160.". So, exactly like C++17 std::array then? :)

Almost. Most of std::array does indeed become constexpr in C++17, except for comparisons. For those you have to wait until C++20. digest<> also has operator<< and to_string.

Zach Laine wrote:

Why define your own endian? You already have an external dependency it seems, on boost::container_hash, so why not use the logic and constants from Boost.Endian?

Endian would be an avoidable dependency, and people don't like those. ContainerHash is different because the reason to depend on it is to pick up user specializations of its traits. If you specialize ContainerHash's traits, it's exceedingly likely that you want the same specializations for Hash2, and it's friendlier to not make users specialize the same trait twice, as would have happened if I duplicated them.

Zach Laine wrote:

Also, even if you don't change any of the names above, please don't use the phrase "sized_range", since we now have a std::ranges concept with that name that means something substantially different

That thought has occurred to me, but I never came up with a replacement name that I was happy with.

(std::ranges::sized_range requires std::ranges::size(x), for instance, where yours does not).

If we add overloads for the _range functions that actually take ranges and not iterator pairs, the hash_append_sized_range function will however require and use r.size(), instead of taking the iterator distance.

Here is my review of the Boost.Hash2 library....I will have to vote REJECT.

Andrey, thank you for the extensive review and analysis. Matt

Andrey Semashev wrote:

Above, I have intentionally omitted the constructor from uint64_t as I find it confusing. In particular, it isn't clear what endianness it uses to seed the algorithm. It looks like it duplicates hash_append.

It doesn't use any endianness. It's just used as a seed from which to derive the initial state. Most, if not all, existing non-cryptographic algorithms take integral seeds.

The documentation should provide a more detailed description of the HashAlgorithm::result_type type. Specifically, what interface and operations it must support. Ideally, the library should provide a concept, similar to HashAlgorithm. Currently, the documentation only says it can be an unsigned integer type or a "std::array-like" type, where the latter is rather nebulous.

Hash2 started out as a C++03 library, and mandated use of boost::array. Then it became a C++11 library, and mandated use of std::array. But then it became a C++14 constexpr library, and std::array didn't work, so now it mandates the nebulous "std::array-like" type (which is not yet completely nailed down, but will crystallize with practice.)

Also on the topic of get_integral_result for "std::array-like" hash values, a few comments:

- It requires the static size of 8 regardless of the size of the requested integer, so even if uint32_t is requested it will still require at least std::array. This should not be necessary.

I'm not yet clear on the practical utility of using a result_type that is an array of fewer than 8 bytes. We don't have any such examples yet, so it's hard to tell whether this makes any sense and whether it should be explicitly blessed, and to what extent.

- It ignores the data beyond the initial 8 bytes. It might be ok for the purpose of slicing the hash value, but it is inconsistent with get_integral_result for integers, which attempts to mix bits through multiplications with a constant.

This is explicitly stated in the documentation of get_integral_result, in the Remarks. In practice it's a useful heuristic to assume that an array-like result implies a hash function of high quality (an avalanching one) so that picking any M bits would suffice. Not making this assumption would make things a bit hairy; it's not quite clear how to turn an arbitrary number of not-high-quality bits into 32. The library tries its best for the integral types, but there the possibilities are at least a finite amount and multiplications are built-in.

It might be possible to reuse one of the hash algorithms implemented in the library for this bit mixing.

Interesting "meta" approach, but not necessarily better in practice than the current one, under any practical criteria.

So I think, get_integral_result and HashAlgorithm::result_type should be better defined by the library.

It most likely will be, eventually, once we have near-100% clarity on how precisely it needs to be defined.

The hmac_* type aliases are not provided for siphash and xxhash. But I would actually prefer that those type aliases be removed for the rest of the hash algorithms, so that the algorithms don't bring the dependency on hmac.hpp, which is not always needed by the user. Typing hmac instead of hmac_sha2_256 is hardly any more tedious.

hmac.hpp is a very light header, so...

Thank you Peter for submitting the library and Matt for managing the review.

Thanks, Andrey, for your review.

Andrey Semashev wrote:

...

...

It might be possible to reuse one of the hash algorithms implemented in the library for this bit mixing.

Interesting "meta" approach, but not necessarily better in practice than the current one, under any practical criteria.

Why? Yes, applying, e.g. fnv1a_32, would be slower than a single multiply or slicing an array, but from the perspective of quality of the reduced digest this approach sounds good to me.

Because taking the low 32 bits from a high quality digest is better than taking the FNV-1a hash of all the bits.

On Tue, Dec 10, 2024 at 5:09 AM Andrey Semashev via Boost < boost@lists.boost.org> wrote:

...Vinnie has agreed to this change in the proposal

Well, no one needs my permission to deviate from the paper :)

Copyability is a rather strong requirement, which may not be supported by all implementations.

Could you please provide an example of a non-copyable HashAlgorithm? I'm struggling to see it. Your review was very well written btw, thanks. Regards

On 12/10/24 18:05, Andrey Semashev wrote:

On 12/10/24 17:50, Vinnie Falco wrote:

...

On Tue, Dec 10, 2024 at 5:09 AM Andrey Semashev via Boost mailto:boost@lists.boost.org> wrote:

Copyability is a rather strong requirement, which may not be supported by all implementations.

Could you please provide an example of a non-copyable HashAlgorithm? I'm struggling to see it.

The primary use case I had in mind is wrapping an external library, where the external library does not support creating a deep copy of the algorithm state. This is the case with libsodium (https://doc.libsodium.org/hashing/generic_hashing), for example.

I should clarify that libsodium uses structs for storing the state of the hashing algorithms. For example, for SHA2-256 the struct is crypto_hash_sha256_state: https://github.com/jedisct1/libsodium/blob/bfa6ee638673dc88c51331dc13e8f112f... As far as I could see, the documentation makes no guarantees about the contents of this struct and whether it is safe to memcpy it. So, even if the struct can be copied as bytes, I still don't consider copying as properly supported by the library, as one has to rely on implementation details.

Andrey Semashev wrote:

Here's the output on my machine:

sha1_160 (1024 bytes): 1220.093078 MiB/s sha1_160 (1048576 bytes): 1317.424120 MiB/s sha1_160 (16777216 bytes): 1318.296514 MiB/s sha2_256 (1024 bytes): 406.139530 MiB/s sha2_256 (1048576 bytes): 430.141013 MiB/s sha2_256 (16777216 bytes): 429.341694 MiB/s sha2_512 (1024 bytes): 629.593514 MiB/s sha2_512 (1048576 bytes): 712.100583 MiB/s sha2_512 (16777216 bytes): 711.549426 MiB/s openssl_sha1_160 (1024 bytes): 1531.142208 MiB/s openssl_sha1_160 (1048576 bytes): 2759.062140 MiB/s openssl_sha1_160 (16777216 bytes): 2738.579482 MiB/s openssl_sha2_256 (1024 bytes): 1541.291824 MiB/s openssl_sha2_256 (1048576 bytes): 2462.035414 MiB/s openssl_sha2_256 (16777216 bytes): 2449.770143 MiB/s openssl_sha2_512 (1024 bytes): 781.187505 MiB/s openssl_sha2_512 (1048576 bytes): 1086.239840 MiB/s openssl_sha2_512 (16777216 bytes): 1082.922782 MiB/s

So, depending on the input message size, OpenSSL is faster from 1.24x to 5.72x. Interestingly, despite what is said in Boost.Hash2 documentation (and confirmed by the test), OpenSSL's implementation of SHA2-256 is faster than SHA2-512. This could be because of the SHA ISA extensions in my CPU.

These are interesting timings. Originally I assumed that this is because OpenSSL uses SIMD. But you're right that it might actually use SHA-NI instead. https://en.wikipedia.org/wiki/Intel_SHA_extensions For some reason I thought that SHA-NI only supported SHA1, but it does support SHA2-256 as well. Is there an easy way to check what OpenSSL uses? Probably not because it selects the instruction set at runtime.

Ruben Perez wrote:

On Tue, 10 Dec 2024, 16:35 Peter Dimov via Boost, mailto:boost@lists.boost.org > wrote:

Andrey Semashev wrote:

...

Here's the output on my machine:

sha1_160 (1024 bytes): 1220.093078 MiB/s sha1_160 (1048576 bytes): 1317.424120 MiB/s sha1_160 (16777216 bytes): 1318.296514 MiB/s sha2_256 (1024 bytes): 406.139530 MiB/s sha2_256 (1048576 bytes): 430.141013 MiB/s sha2_256 (16777216 bytes): 429.341694 MiB/s sha2_512 (1024 bytes): 629.593514 MiB/s sha2_512 (1048576 bytes): 712.100583 MiB/s sha2_512 (16777216 bytes): 711.549426 MiB/s openssl_sha1_160 (1024 bytes): 1531.142208 MiB/s openssl_sha1_160 (1048576 bytes): 2759.062140 MiB/s openssl_sha1_160 (16777216 bytes): 2738.579482 MiB/s openssl_sha2_256 (1024 bytes): 1541.291824 MiB/s openssl_sha2_256 (1048576 bytes): 2462.035414 MiB/s openssl_sha2_256 (16777216 bytes): 2449.770143 MiB/s openssl_sha2_512 (1024 bytes): 781.187505 MiB/s openssl_sha2_512 (1048576 bytes): 1086.239840 MiB/s openssl_sha2_512 (16777216 bytes): 1082.922782 MiB/s

So, depending on the input message size, OpenSSL is faster from 1.24x to 5.72x. Interestingly, despite what is said in Boost.Hash2 documentation (and confirmed by the test), OpenSSL's implementation of SHA2-256 is faster than SHA2-512. This could be because of the SHA ISA extensions in my CPU.

These are interesting timings. Originally I assumed that this is because OpenSSL uses SIMD. But you're right that it might actually use SHA-NI instead.

https://en.wikipedia.org/wiki/Intel_SHA_extensions

For some reason I thought that SHA-NI only supported SHA1, but it does support SHA2-256 as well.

Is there an easy way to check what OpenSSL uses? Probably not because it selects the instruction set at runtime.

Is it expected to reach similar levels of performance in the future using Hash2? Or is this not in scope for the library?

I don't see why it wouldn't be in scope; performance optimizations are never ruled out. Taking advantage of SHA-NI when it's statically known to be available (under GCC/Clang -march=native, for instance) should be relatively easy to add. Dynamic dispatch is a bit more convoluted because of constexpr but should be possible.

Andrey Semashev wrote:

I think, HashAlgorithm should allow implementations to use the seed arguments to the hash algorithm constructors as salt, i.e. the constructor would implicitly call update().

No, this is a bad practice and should never be used. As I already said, the algorithm should at minimum include the size of the seed, and then pad to a multiple of the block size. This is not just something I made up, it's existing practice. See for instance how KMAC is defined in https://nvlpubs.nist.gov/nistpubs/SpecialPublications/NIST.SP.800-185.pdf If not that, you should at minimum include the hash of the seed in the initial prefix, instead of (or in addition to) the seed, like HMAC does. Don't forget to pad to a multiple of the block size.

Andrey Semashev wrote:

Currently, as I look in the hash algorithms provided by Boost.Hash2, most of them interpret the byte-range seed the way you describe, with the exception of fnv1a and siphash. fnv1a doesn't append the seed size (and doesn't pad as FNV-1a is not a block hash); I'm not sure if that's intentional.

I suppose it would be better for it to do so, yes.

siphash has special treatment of the seed if it is exactly 8 bytes.

That's because this specific handling of the key is part of its specification, as siphash is a keyed hash function. So if you use siphash as specified, using the key length that it mandates, you get its documented (in siphash.pdf) behavior and it passes the corresponding siphash test suite.

Given these differences, is the user able to have any sort of guarantee on consistent behavior of the constructors of various algorithms? If not, why provide the constructor at all?

The constructor does what it's specified to do in the hash algorithm requirements. Yes, the user can rely on that.

REJECT

Sincerely, Thomas Kent

Thank you Thomas for your review and concerns. Matt

On Wed, 11 Dec 2024, 02:55 Tom Kent via Boost, wrote:

There are several hashes given in this library that are used in security critical scenarios (e.g. SHA-2). Additionally, the explicit goal of the library is to give the user tools they need to build their own hashes.

I do not see any indication that any kind of cryptographic assessment was done on this library. There is not even a mention that the security of these hashes has been thought about nor in what scenarios they could be used. This is absolutely essential for any library providing cryptographic primitives. There are a *lot* of potential pitfalls in the implementation of a cryptographic primitive, even in cases where the "correctness" of the end result aren't in question.

For example, there have been timing attacks against SHA-2/HMAC where the difference in the amount of time processing takes can leak information about the secret key. https://dl.acm.org/doi/10.1007/978-3-030-89915-8_2

These side-channel type attacks can be extremely insidious, and are not things that people who are otherwise experts in C++/C/Assembly/etc would ever think about. For cryptography, we need a different type of expert than the type we typically grow in this community.

**If Boost wants to take on providing cryptographic primitives, we need to hold those libraries to a higher standard, including an evaluation by outside cryptopgophers before we release anything to the public.**

As a potential user mainly interested in the "hashing untyped byte sequences" use case (involving SHA2), do you think migrating from OpenSSL to Boost.Hash2 would be detrimental for security at this point? If the answer is yes, is there a way to remediate this (even after the library gets accepted)? Or is this just not the main use case of the library? The use case involves generating digests for a network protocol (MySQL). I'd like to know both Tom's and Peter's opinions. Thanks, Ruben.

On Thu, 12 Dec 2024 at 14:27, Peter Dimov wrote:

Ruben Perez wrote:

...

As a potential user mainly interested in the "hashing untyped byte sequences" use case (involving SHA2), do you think migrating from OpenSSL to Boost.Hash2 would be detrimental for security at this point? If the answer is yes, is there a way to remediate this (even after the library gets accepted)? Or is this just not the main use case of the library?

The use case involves generating digests for a network protocol (MySQL).

I'd like to know both Tom's and Peter's opinions.

Can you please point me to the source code portions in Boost.MySQL that implement SHA-2 authentication?

Current code (using OpenSSL): https://github.com/boostorg/mysql/blob/c438f26731e36c2db6457705ec5dbb9f7657d... Code using the proposed library: https://github.com/boostorg/mysql/pull/389/files#diff-1ce941e5f315c38f0eb53e... Protocol docs: https://dev.mysql.com/doc/dev/mysql-server/8.4.3/page_caching_sha2_authentic... It's somehow similar in spirit to SCRAM-SHA256, but built in-house by MySQL.

The actual SHA256 implementation in OpenSSL has been looked at much more than the one in Hash2 at this point, but the Hash2 code is easy to inspect and verify because it follows the reference implementation very closely at the moment (although this might change if we add SHA-NI optimizations in the future.)

OpenSSL's SHA implementations (SHS and SHA3) are also certified under FIPS 140-2 [1] if this is a relevant property for your users Reuben. Matt [1] https://csrc.nist.gov/projects/cryptographic-module-validation-program/certi...

On Thu, 12 Dec 2024, 21:17 Peter Dimov, wrote:

...

On Thu, 12 Dec 2024 at 14:27, Peter Dimov wrote:

...

Ruben Perez wrote:

...

As a potential user mainly interested in the "hashing untyped byte

sequences"

...

...

use case (involving SHA2), do you think migrating from OpenSSL to Boost.Hash2 would be detrimental for security at this point? If the answer is yes, is there a way to remediate this (even after the library gets accepted)? Or is this just not the main use case of the

Ruben Perez wrote: library?

...

...

...

The use case involves generating digests for a network protocol

(MySQL).

...

I'd like to know both Tom's and Peter's opinions.

Can you please point me to the source code portions in Boost.MySQL that implement SHA-2 authentication?

Current code (using OpenSSL):

https://github.com/boostorg/mysql/blob/c438f26731e36c2db6457705ec5dbb9f7657d...

...

Code using the proposed library:

https://github.com/boostorg/mysql/pull/389/files#diff-1ce941e5f315c38f0eb53e...

...

Protocol docs: https://dev.mysql.com/doc/dev/mysql-server/8.4.3/page_caching_sha2_authentic...

It's somehow similar in spirit to SCRAM-SHA256, but built in-house by MySQL.

In this specific case, if we assume that Hash2 is accepted into Boost, I'd say that using OpenSSL is much more susceptible to supply chain attacks.

The user acquires both Boost.MySQL and Boost.Hash2 through Boost, whereas the typical practice of acquiring OpenSSL under Windows until very recently was "web search and download random binaries from somewhere on the Internet."

Things are probably much better today because of vcpkg and conan, but the exact version of OpenSSL that the user will end up using is still an unknown variable.

The actual SHA256 implementation in OpenSSL has been looked at much more than the one in Hash2 at this point, but the Hash2 code is easy to inspect and verify because it follows the reference implementation very closely at the moment (although this might change if we add SHA-NI optimizations in the future.)

(I ignore here the C-style interface of OpenSSL's function, which has its own safety implications, but let's assume that the code in Boost.MySQL is 100% correct.)

Thanks for the clarifications, Peter.

On Thu, Dec 12, 2024 at 10:45 AM Matt Borland via Boost < boost@lists.boost.org> wrote:

I compile all the votes

Reviews are not votes. Thanks

Samuel Neves wrote:

A minor question I have regards the constants used in get_result_multiplier(). The documentation states that this is used to produce a uniform output on the target range, but it's unclear from the documentation or source code the rationale or criteria for the method or constants used here. This seems to get into integer hash territory, but for example the 4 -> 1 case consists of x -> (x*0x7f7f7f7f) >> 24, for which easy differentials exist, e.g., (x, x^0xc0800000) collide with probability ~1/4.

It's pretty hard to come up with "good" multipliers here because it's not possible to quantify the "good" part. There are basically two cases, one where the 32 bit input is uniformly distributed, and one in which it isn't. And when it isn't, that is, it comes from a "bad" hash, it's not really possible to optimize the multiplier without a known input distribution, and we don't have that. So I came up with some ad hoc criteria here https://github.com/pdimov/hash2/blob/develop/test/get_integral_result_4.cpp and tried to make the multipliers work for them.

One of the things that puzzles me about this library is its intended purpose. "Types don't know #" was clearly about hash types for use in hash tables, which explained why keying the hashes was mandatory. But it is unclear to me why MD5, SHA-1, and other cryptographic hashes are present here; [...] I believe the HashAlgorithm concept is too overloaded. It _requires_ that hashes be both keyable and extendable-output, [...] A user that wants to use this library with a hash that is not included here is going to be pushed to include functionality the hash does not provide and that they might not know how to achieve.

That is actually a good point: Why are those requirements on the HashAlgorithm concept at all? The library is (mostly) about allowing to hash types, i.e. the framework surrounding `hash_append`. I understand that a specific way (currently being discussed here on the list) of construction is required for implementing `hash_append` for unordered containers. But besides that, why does the library require specific constructors at all? The HashAlgorithm is provided by the user as an instance. How the users constructed that isn't relevant for the purpose of the library, is it? To my understanding having support for seeds could be highlighted as best practice for specific use cases, but not as a general requirement. I understand that the library provides ways to extract a hash out of an Algorithm. So including `result_type` and `result` makes sense, although given that this is C++11 the former seems a bit outdated as it can be determined by the latter. But besides that there are no requirements on HashAlgorithm relied upon by the library, are there? For user convenience I can see `SeedableHashAlgorithm` as a subset of `HashAlgorithm` as a useful, additional concept to include in the library. The library could provide traits to check a given type for a specific concept (when lacking C++20 concepts) This would give users and hash authors more freedom in which algorithm is used. Of course it could be stated, that all included algorithms conform to the `SeedableHashAlgorithm` concept, if they (meaningfully) are.

Samuel Neves wrote:

One of the things that puzzles me about this library is its intended purpose. "Types don't know #" was clearly about hash types for use in hash tables, which explained why keying the hashes was mandatory. But it is unclear to me why MD5, SHA-1, and other cryptographic hashes are present here; they are way too inefficient to be of much use in hash tables, so there is little reason to include them. If, on the other hand, this is meant as a more general hashing library, I have strong objections as detailed below.

There exist intermediate use cases between "key in a hash table" and "cryptographically secure hashing of a binary blob" that are also served by the library. Some of them are: - if you have a program with some program state S, represented by a C++ object, which the user can save to disk by means of e.g. Ctrl+S, you often want to know whether the current state has been saved or not (to display an indicator, or to decide whether to autosave on exit.) This is implemented by keeping a hash of the last save, and comparing the current hash to the last saved one. - if you have something like Boost.Compute, which needs to compile shaders, you can keep a cache of already compiled shader binaries, along with the hashes of the source used to create them, and then skip the compilation if the source matches a hash of an already compiled binary. https://github.com/boostorg/compute/blob/cf7907574d6159cd43e6cf687a7b656278c... - if you need to send complex C++ objects over the network, you can first ask the remote endpoint "do you already have the object with this hash?" and if so, skip sending the object. And there are many other cases isomorphic to the ones above. In all of these cases, you want a very good quality hash function, one for which you can ideally assume that collisions never happen, for some practical definition of never (less than cosmic rays or random RAM bit flips, for instance). And because of that, you want to be able to use a cryptographic, or ex-cryptographic, hash function, even when there is no external attacker, because non-cryptographic hash functions are generally optimized for speed and do not produce a sufficiently long digest size. (But if a non-cryptographic hash works for you, you also want to be able to use _that_, because more speed is always better than less speed, other things being equal.) In short, having a reliable framework for hashing arbitrary C++ objects with any user-selected hash algorithm opens up a lot of possibilities that one might not realize are there, and "how do I implement std::hash for my type" is only a small fraction of them.

...

I received the following review directly from Ivan Matek, because the review is written as a PDF that exceeds the allowable size of the ML. The document can be accessed here: https://drive.proton.me/urls/A6NCZVTK58#yuGnqqnzYlhA

Please post this at a minimum as an HTML email. Preferably reformat to be text-only.

Our future plan for the new boost.org is that we will collect and index reviews in the site database and cross-reference them from library pages. Reviews submitted as nicely formatted PDFs unfortunately will not be picked up by this. It would be best if all reviews were posted to the mailing list so they can be accessed by tooling.

Below is my attempt at converting this to text for posterity. Boost.Hash2 Review - Ivan Matek December 2024 About my Preferences/Experience/Expectations I am not a Boost developer. I am a longtime C++ developer. I know of issues with std::hash, not familiar with Boost.ContainerHash. I am pretty sure I used hash_combine years ago, but not sure. I prefer modern C++, and dislike features that are easy to use wrong. I do not like WG21 ABI/keyword(e.g. co_await) decisions as I feel they are ruining the language. In terms of safety vs performance my opinion is that it depends. In general I feel underlying code can use unsafe constructs as those components are much more used/tested than user facing API, when it comes to user code I prefer safer, harder to misuse constructs, unless we are talking low latency or HPC domains. When first hearing the name Hash2 I originally expected that the library would help many C++ developers that are frustrated with the fact C++23 std does not know how to hash std::pair or std::vector. That is still the main use case I am considering in this review, but during discussions on the mailing list and reading the documentation I have learned that it is good to also consider Hash2 as a general framework for decoupling type value representation and hashing algorithms. My knowledge of cryptography is basic. Bias I have not interacted in person with the primary author. But over years he has replied many times to my questions on the mailing list and I have noticed he is active on reddit where he often provides very good comments/answers, I often use the named argument trick I learned from his blog. Boost.Mp11 is one of my favorite Boost libraries. Beside that I know the author did a lot of work in Boost, e.g. fast hash maps and Boost.Describe. Due to all things above I consider him a world class C++ expert. With regards to the second author if I remember correctly we have never interacted. As for authors of the original old WG21 proposal I know that Howard is the author of std::chrono, provided many great answers on SO, and IIRC was author of original versions of libc++ so I also consider him a world class C++ expert. Limitations I did not follow any other discussion except the mailing list. Design I think the general C++ design of the library is great, so I will not spend much time on that, beside saying that I feel that like STL separates iterators and algorithms it separates hash algorithms and object representations. Beside that not hashing size when size of type is constant is a smart optimization. C++11 With regard to the decision to support C++11 I think new Boost libraries should not support C++11, but that is a topic that has been discussed at length, and in any case it is not a reason for library acceptance or rejection. It is possible that with C++17 or C++20 API could be nicer, but I can not judge that without actually seeing it, besides assuming that the concept for the algorithm could give nicer error messages. HashAlgorithm Concept Unfortunately I do not know enough about domain to judge it, and if it would be better that there are more than one concept. Describe With regards to dependency on Describe I feel Describe is a great library, although I have never used it in a production so this is a huge benefit, not a liability. I am very happy to see when Boost libraries are combined to produce nice code. I understand that people are worried for paying compilation of includes that they do not use, but I think benefits outweigh this unfortunate downside. Variant I do not understand why variant is not supported. There are three main areas where I think there is opportunity for improvements. Easy to use, hard to misuse wrappers for users More advanced avoidance of hashing size Cryptographic guarantees Easy to Use Hard to Misuse It is possible that I am misunderstanding the reasons for the signature of tag_invoke function, but it is not clear to me why current: template friend void tag_invoke(boost::hash2::hash_append_tag const&, Hash& h, Flavor const& f, X const& v) { boost::hash2::hash_append(h, f, v.a); boost::hash2::hash_append(h, f, v.b); } can not be template <class HashAppender> friend void tag_invoke(boost::hash2::hash_append_tag const&, HashAppender& ha, X const& v) { ha.hash_append(v.a); ha.hash_append(v.b); } (Flavor is encoded in type HashAppender). We remove from the user the ability to by mistake call update, result, pass wrong flavor(by passing {}). We also remove from them the ability to copy construct a local copy of h, but it is not clear to me if use cases for this would be common. Notice that this would remove access to update from most users. I mention this because there was a lot of discussion about span vs char*, len. In my opinion we can have the cake and eat most of it with wrapper helpers. Similar example is when it comes to mailing list suggestions to replace result() with finalize() and make further calls UB. I have no strong opinion here since I do not understand how realistic it is to repeatedly call result(), benefits it provides, if it is useful why (according to the mailing list) no other library supports that use so for sure here we are discussing more than just a naming and those design discussions are much more important than addition of simple wrapper… But I think this is another opportunity for a wrapper that uses result() internally, but only exposes finalize() to the user. Here wrapper may have small potential overhead compared to finalize() design since result() mutates the state while finalize() could rely on further calls being UB and not update state. It is possible that an optimizer might realize this even when wrapper is used, but I would guess it is too much to expect from the optimizer. Another case mentioned in the mailing list is hashing char* that is probably not what the user wants since it hashes pointer, not pointed to string. I know library must allow it, e.g. how would user cast char* in std::tuple in his code to integral value, sometimes raw pointer is key in a map… but it could be made explicit longer name function user must explicitly call, while common case hash_append does not allow pointer values. Another opportunity for wrappers is to have different wrappers when the user is concatenating bytes of a byte stream versus when the user is concatenating values. Here the helper makes the choice if to append size or not for types like std::span<char> or vector, i.e. ranges of bytes clearer since it is in the helper name. Simple examples: simple_hash_values h; std::string x{"ab"}; std::string y{"c"}; h.append(x); h.append(y); will produce different result than if we did simple_hash_values h; simple_hash_values h; std::string x{"a"}; std::string y{"bc"}; h.append(x); h.append(y); , but this two cases produce same result: simple_hash_bytes h; std::string x{"ab"}; std::string y{"c"}; char z[2] = {'d', 'e'}; h.append(x); h.append(y); h.append(z); simple_hash_bytes h; std::string_view x{"abcd"}; std::array y = {'e'}; h.append(x); h.append(y); It is not a requirement of a framework to produce wrappers, e.g. Boost.ASIO does not have to provide websocket code in it, it is fine for Beast to do it. But I wonder if in this case the potential for bugs is so large that it would be beneficial to add helpers to the “core” library. More Advanced Avoidance of Hashing Size Currently library smartly avoids hashing size for types whose representation size is fixed, e.g. int, std::array, etc… but it encodes size for variable representation size types, e.g. std::string_view, std::vector<float>. This is correct in general case since to know object ranges in serialization stream we can afford at most 1 object with variable size, but we do not know what objects future calls to append may hash, so the only safe general solution is to always hash size. But there is a specific case when we can avoid hashing the size of variable representation size object. When we know it is the only object of variable representation size we append between construction and destruction of the hasher. This for example happens when we hash the std::string key in unordered_map. We do not need to encode its size since it is the only object used to produce the hash. Here the library could provide a helper that understands this case, and as a bonus as in case of previous wrappers prevents wrong use(i.e. result() is called by wrapper, not user). This could be extended for more complex types, e.g. std::pair since here we only have 1 type with variable representation size, but I presume the most common case is when the type used is a simple type. Experiments with performance gains regarding this can be seen in the Performance section. In any case this is not a big issue because users can write this kind of wrappers for their use case relatively easily assuming they avoid some mistakes such as not appending anything in case of empty string. Cryptographic Guarantees This is the biggest concern I have about the library. As I am writing this there is currently a discussion on the mailing list suggesting hashing unordered ranges degrades cryptographic properties of used hash function. I know it is unlikely that this will matter for most users but as far as I understand currently that is not mentioned in documentation or in source code. I think it should be explicitly mentioned in both places. Just to make this point clear: the user does not even have to explicitly call this function, due to the dispatch rules of hash_append. This may sound too nitpicky, but I really believe this is a serious issue, for example of simple decisions that can have a big impact you can check this $160 million github issue. Performance I have tried to benchmark hashing performance in a few scenarios. All done with Clang 19.1 on Linux with libstdc++. Describe Overhead I was curious if the performance of the described class(class that uses Boost.Describe to achieve reflection) is the same as when I manually call hash_append for each member. This was initially not the case, but the author told me it is probably due to Clang not inlining, when I increased Clang inline “budget” performance difference disappeared, so this is great proof that in itself Describe is zero overhead. FNV1a for Short String I was curious if not encoding size would speed up hashing in cases when we hash just 1 value and then call result, e.g. what happens in insert/find of hash map. I focused on FNV1a variants since they were the fastest. In some cases hashing was so fast I had to check online with some tool that produced results that were correct since Clang unrolled loops so hard that performance was insanely fast. Not much to say here except that this confirms that for this case library abstractions do not use any techniques that might make Clang optimizer fail. But back to the general goal of determining if encoding size matters or not… I tried it with some collection of short strings and the performance difference exists, but it is a bit complicated. For std::string performance difference was almost 2x because clang gave up on inlining in case we were hashing size. For boost_static_string performance difference was noticeable, but tiny, obviously depending on length of string. For very short boost_static_strings hashing without size was 15% faster, but as size increased the difference remained the same in absolute time, so the relative performance difference was dropping. To keep this short I will not repeat the details here, if you want to know more you can search for "What is 7 bytes among friends" in the mailing list(to satisfy the need to hash something we always hash dummy byte if we do not hash size). While this is micro optimization I believe there is a design space to exploit it, discussed in detail in the Design part of the document. Comparing to SIMD SHA256 As I am not a Rust expert so I thought it would be nice exercise to check how fast/slow is a simple program written in Rust using what I quickly googled to be default SHA2 implementation(point was to simulate quick research and coding, instead of doing long evaluation of alternatives). As Hash2 does not use SIMD I expected Rust to be multiple times faster and it turned out to be the case. On my 13700H machine this program runs 6x faster than Boost.Hash2 equivalent. fn main() { const NUM_INTS: usize = 42_000_000; const VALUE: u32 = 42; let data: Vec<u32> = vec![VALUE; NUM_INTS]; let byte_data = unsafe { std::slice::from_raw_parts(data.as_ptr() as *const u8, data.len() * std::mem::size_of::<u32>()) }; let start = Instant::now(); let mut hasher = Sha256::new(); hasher.update(byte_data); let hash_result = hasher.finalize(); let duration = start.elapsed(); println!("{} microseconds", duration.as_millis()); println!("{:?}", hash_result.as_slice()); } I picked SHA256 since I know it benefits from SIMD instructions, did not check other algorithms. Documentation. I find documentation thorough and very well written, but hard to read for non experts, partly because the problem solved is non trivial, fundamental differences from std::hash and partly because the library does not expose easy to use wrappers. Few examples follow. What is the relationship of this library with Boost.ContainerHash? If I am C++ beginner how do I decide which one I should use for my usecase? I know in general it is not nice to claim one library is better than another, but the primary author of Hash2 is maintainer of Boost.ContainerHash and Hash2 library depends on it so I think it will not be a problem. Biggest pitfall that users need to be aware is that passing ranges that do not have constant size, will encode their size. This is obvious to me in retrospect, but I never had to think about this in the old world of std::hash because std::hash does not know how to hash more than one value. In particular I believe users need to be told explicitly about following: hash_append and hash_append_range are not just the same logic with different arguments, as for example std::ranges::find(v.begin(), v.end(), 74656) and std::ranges::find(v, 74656). They encode different information, then as an example explain that if you want to hash chunks of byte stream using std::span<char> you can not call hash_append because it will encode size of each chunk. You must call hash_append_range. Currently above is possible to understand if you read the documentation carefully, but I do not feel average C++ developer will be able to read hash_append dispatch rules(that make sense, but are very long) and figure out what is happening when he passes std::span, so I think we need few more // WRONG! code snippets in documentation and mention of this in the introduction. I do think it is great to link the proposal in the documentation, but I would move it from introduction since I believe for most users reading proposals is too difficult. Regarding unordered_map hash example I understand that hash maps are not the focus of the documentation, but I presume many users will be desperate to get a performant transparent comparator for stringy types, so I think it would be good to add an example with transparent hasher and comparator that handles std::string, std::string_view, char* in a performant way. For example of how difficult it is to do this in C++20 you can see reddit example(make sure to read about ABI in comments). Regarding other examples I feel that using nonRAII fopen/fclose and char* will scare away some users. I understand the example is about Hash2, not modern C++ or Boost.Scope, but I feel this is one more argument that wrapper helpers would be nice, e.g. we could wrap our char array and read size in span. An example with Mp11 is amazing because it replaces the switch needed if implemented in a simple way, but here again I wonder if most users will be confused and not understand it. Potential fix would be to use simple switch code in documentation and link to more advanced examples in the examples folder(if documentation has a way of detecting dead links so that it does not become a liability). Performance I believe it should be mentioned that algorithms are not implemented using SIMD instruction. I fully understand this is not the task of a hashing framework, but I still believe users will be unhappy if they find out after using the library for months that their sha256 is 5x slower than it can be. For the average user explanation that Hash2 is a hashing framework, not a collection of SIMD algorithms for different architectures will probably not mean much. They will just remember that Boost is slow, potentially sharing that opinion with coworkers and/or social media. So I think mentioning that no SIMD/AVX2 implementations are used in docs is very important. General Notes About Review Process I personally found the review process quite interesting, although it can be a huge time sink if you follow a few rabbit holes. I know it is not the primary purpose of the review but I can say I learned a lot in a few days. I noticed that very few comments about documentation were about how approachable it is to beginners(or even the average C++ developer). I would suggest that Boost considers adding this question to the list of questions during the review process. I have benefited tremendously in my evaluation from Samuel’s response. I have noticed that he did not post very often on the mailing list so I presume it was just luck or somebody pinged him as a domain expert. I wonder if this should be encouraged while announcing a review since Boost has plenty of C++ experts, but probably not so many domain experts for every field in which Boost is used. This would not mean Review Manager mailing random people he found on the internet, more of an encouragement for mailing list members to reach out to people in their circle they know might be knowledgeable about the domain and explain they do not need to review the entire library, e.g. mathematician can offer immensely valuable help reviewing some part of hypothetical graph library even if his knowledge of C++ is basic. I think we should update the policy page not mention modem :) tl;dr What is your evaluation of the design? I think the C++ design of the library is great, although the average C++ developer would benefit from an easier to use higher level interface. Can not judge cryptographic design. What is your evaluation of the implementation? Did not spent a lot of time looking into it, everything I saw looks fine from C++ perspective, can not judge wrt cryptographic correctness so I defensively assume it is not suitable for cryptographic purposes. This may sound unfair, because it is, but as I have no skills to assess the library in this regard I must assume the worst. What is your evaluation of the documentation? Detailed, well written, but not for beginners(too many hard concepts like type traits, too few examples of what not to do) What is your evaluation of the potential usefulness of the library? Do you already use it in industry? I think the library is currently useful for algorithms that do not benefit from SIMD implementations. But I feel that in most companies using it would require wrapping it in higher level wrappers that make misuse harder. I have not used it in production, I just built a few simple benchmarks and simple wrappers with it. I would never use or suggesting using library for something that interacts with the “outside world” since I am quite careful and I think any potential issues are much more likely than in something popular like OpenSSL, not because OpenSSL is so great, but because it is much more popular/scrutinized. Did you try to use the library? With which compiler(s)? Did you have any problems? I used Clang 19.1, C++23. Problems were just me wrongly assuming how spans of chars are “concatenated” when hashed, that doubles are trivially hashable, etc. no problems related to library itself. How much effort did you put into your evaluation? I spent a few days playing around with the library, but a lot of this time was spent reading up on the domain, figuring out how to enable clang-tidy warnings etc., not a classic design or code review domain expert would do. Are you knowledgeable about the problem domain? I know a fair bit about C++, the same can not be said about cryptography. My evaluation is CONDITIONAL ACCEPT with following criteria: More beginner friendly documentation, no need to turn it into a tutorial for C++, but few specific common pitfalls need to be addressed. Hashing “ab”, “c” and expecting it is same as hashing “a”, “bc” Hashing char* when user wanted to hash string Expecting hash_append and hash_append_range to produce same result Beginner friendly comparison with Container.Hash to help beginners navigate libraries. If comparing libraries is against Boost policy my apologies for this suggestion. Explicit mention in documentation that hash_append is not preserving cryptographic properties of used algorithm in certain conditions(this is assuming current implementation for hashing of unordered ranges) Explicit mention in documentation that currently algorithms are not implemented using SIMD.

Darryl Green wrote:

There has been a lot of discussion, in hash2 review, about non-idempotent result() / every hash2 hash algorithm is required to be an extensible hash function. I have questions:

Isn't any non-idempotent result function surprising?

I wouldn't say so. If you have a thing that exposes three operations, construct, update, result, your expectation should be that it supports the following sequence construct update update ... update result and any deviations put you into "read the documentation" land. Adding a second `result` call to the end can easily have one of the following effects: - return an unspecified value - return the same value as the previous call (idempotence) - fail (either by returning an error or throwing) - undefined behavior There's no a priori reason to expect idempotence here, unless the specification guarantees it, and if you look at the opinions expressed so far in this review, you'll find people who argue for each of the above being their preference, presumably the one who they wouldn't be surprised by. In addition, if you now add update update result to the end of that sequence, you have some further choices to make. I suspect that we'll also have a diversity of opinions here as to what the unsurprising effect of the above should be; my expectation would be for a person who considers idempotence the only unsurprising alternative to prefer the clean semantics of `result` always returning the hash of the partial message accumulated so far by the preceding sequence of update calls. But of course that's not at all undisputed, partly because it imposes a cost. As currently specified, you can obtain these semantics by doing the following: auto get_partial_result( Hash const& hash ) { return Hash( hash ).result(); } which is why I consider them derived semantics, and not a fundamental operation that the algorithm needs to provide. They are achievable from the outside, with no loss of functionality, and with the costs paid by the user who needs them.

As I said in my review, hash2 imposing the extensibility requirement on "raw" hash algorithms that are not XOFs (you can always do "something" to extend it but doing "something" does not always make a good extensible hash function) is surprising. Ideally hash2 should allow an XOF to be used but not by requiring every hash to pretend to be one. I did say some documentation would help but I feel that relying on docs to make it hard to "do the wrong thing" or even possible to do what should be a simple thing (use an existing hash algorithm/algorithm implementation) without that in itself being a silly mistake (NOT having result in some way stir the pot is a bug, now, unless one considers extension by repetition to be "good enough" - which absurd as it sounds, maybe it is - it's at least "honest").

In short, the argument here is that we shouldn't require from hash algorithm authors to provide result extension, because they might screw it up. That's a legitimate argument, in principle; if result extension was something uniquely hard to provide, or something particularly error-prone (measured against the baseline of implementing the rest of the hash algorithm), I would probably have agreed. But that's not the case. In fact, most of the time, result extension is free, in that if you just implement `result` in an entirely straightforward manner per the algorithm spec, you automatically get result extension. It's easier to implement it accidentally, than make a mistake. And it's not true that the rest of the hash algorithm is easier to implement, or less error prone. Seed construction and update are at least as tricky, if not more. In addition, the exact same argument applies against seeded construction; people can easily make mistakes when implementing it. But I consider this an acceptable cost to pay, because the functionality is necessary for the intended use of the hash algorithms, by the library and by the users. Why is result extension necessary? Well, consider the std::hash use case, where the wrapper object does this: std::size_t operator()( T const& v ) const { H h; boost::hash2::hash_append( h, {}, v ); return boost::hash2::get_integral_resultstd::size_t( h.result() ); } (ignoring seeding at the moment) If H::result_type is uint32_t, but std::size_t is 64 bit, get_integral_result needs to extend the uint32_t value into 64 bits. It currently does the equivalent of return h.result() * 0x7FFFFFFF'7FFFFFFF; but I'm planning to change it to take advantage of result extension and do this instead: return (uint64_t)h.result << 32 | h.result(); There is no scenario in which the former produces better 64 bit hash values than the latter, unless, of course, the implementation of result extension is majorly screwed up. For another example, consider hash_append_unordered_range, which currently does w += hash2::get_integral_resultstd::uint64_t( h2.result() ); As we already established, a 64 bit accumulator w isn't going to cut it; so I'm planning to change w from std::uint64_t w = 0; to std::uint64_t w[ 4 ] = {}; This would require the ability to obtain 256 bits of hash from the hash algorithm. Again, there is no way to do this from the outside, without support from the hash algorithm, and achieve a better result distribution. In the general case, when you need to do nontrivial things with the hash algorithm that aren't just a simple sequence of `update` calls, result extension is simply necessary. I suppose I can, in principle, implement the above use cases without result extension, by using an MGF-like scheme. E.g. instead of return (uint64_t)h.result << 32 | h.result(); do this H h1( h ); h1.update( '\x01', 1 ); auto r1 = h1.result(); H h2( h ); h2.update( '\x02', 1 ); auto r2 = h2.result(); return (uint64_t)r1 << 32 | r2; but this imposes completely unnecessary performance costs.

What is the specification for a "hash2 XOF"? Does the author really mean to take on the role of specifying the properties required? If FIPS says that a function is an XOF it probably is. If FIPS doesn't say that, but Peter says it is one (it must be to be a "hash2 hash algorithm")... Is that ok? Who wins?

If FIPS doesn't say that a hash function is an XOF, it isn't. Result extension doesn't automatically turn any hash function into an XOF, similarly to how byte sequence seeding doesn't turn any hash function into a MAC. You can use the function as XOF or MAC, but this doesn't make it meet the stricter cryptographic requirements for XOF or MAC. It's probably my own fault for not making that point in the documentation. I did make an explicit note in the constructor description that even though it makes all algorithms usable as MACs, an established MAC algorithm such as HMAC should be used unless the algorithm has been specifically designed to be used as a MAC (as SipHash was.) But that's not what I said when describing `result`, and I should have. To sum up with a general point, the hash algorithm requirements as specified reflect (my idea of) what a modern hash algorithm (one designed today) would provide as an API. If we look at SipHash, which is relatively recent, we'll see that it's a very good match for these requirements; the only slight issue is that it doesn't support seed sequences of arbitrary length (which is a regression in usability compared to HMAC.) In particular, it supports byte sequence seeding, doesn't expose its entire internal state as the digest, and trivially supports result extension. There is actually a SipHash variant that produces a 128 bit digest, instead of 64 bit one, and it's almost exactly equivalent to a result-extended hash2::siphash_64. The only difference is that some xor constants have been changed so that the low 64 bits of the 128 bit variant do not match the value produced by the 64 bit variant. Thanks to everyone who read this far. We should gamify the list and have some sort of achievement be unlocked by that.

Дмитрий Архипов wrote:

This ability to trivially change hashing algorithms is as far as I can tell the main objective of the library. That is the reason for the requirements of HashAlgorithm.

That is exactly correct. Thanks Dmitry.

Here's my review....

I recommend accepting the library.

Thank you Alan for the review. Matt