Re: [boost] [rfc] Preliminary Hash Library

Subject: Re: [boost] [rfc] Preliminary Hash Library
From: Scott McMurray (me22.ca+boost_at_[hidden])
Date: 2010-05-07 17:08:53

On 6 May 2010 07:54, Stewart, Robert <Robert.Stewart_at_[hidden]> wrote:
>
> Links from one section to the next (and the reverse) would be helpful.
>

Added.

>
> Not everyone considering your library knows about cryptographic hashes and the terminology relating to them. [...] Listing reasons for having created the library should be part of a Rationale section, or similar.
>

I've added an Introduction defining terms and suggesting usages, and
moved the history to a Rationale.

>
> In the bullet 2 table, the SHA-256 policy should be sha2<256>, right?
>

Yes.

>
> Why include CubeHash when it is merely a candidate and you recommend not using it? Â Including some of the others is useful merely because there are existing demands for their use. Â Is the same true of CubeHash?
>

I knew I needed to include MD5 and the SHAs as they seem to be the most used.

They are, however, constructed in the same way out of block cyphers
with similar implementations, which is part of the reason NIST is
currently holding its SHA-3 competition.  I included CubeHash to
ensure that different styles of hashes would still fit nicely in the
concepts.  (CubeHash is not based around a block cypher at all.)  I'd
like to add more second-round SHA-3 candidates as well (such as the
sponge-construction-based Keccak and the tweakable-block-cypher-based
Skein), but decided to get input on what I had before implementing
more models.

>
> To what bits does, "All the bits in the provided values are used," refer? Â Did you mean something like, "All bits in the input range values are used?"
>

Yes.  I've changed it to use your phrasing.

>
> Do you support Boost.Range/RangeEx?
>

Yes.  The documentation now has a "single-pass range of readable iterators".

>
> I don't understand to what the Portability Note applies. [...] The Alternative may make more sense when you clarify the "all bits" part I raised above.
>

I've rephrased both these sections; Let me know whether the changes
are any better.

>
> Is "HashPolicy" a good name? Â Is "Hash" not enough? Â That is, the Hash Concept? Â You don't have "MessageDigestPolicy" or "StreamHashPolicy," so why "HashPolicy?"
>

I put "Policy" in that name to emphasize that its usual usage would be
as a policy template argument, rather than providing any functions.
I've spelt that out explicitly now, and changed the concept name to
HashAlgorithm.  I'd prefer not to use just "Hash" since some people
refer to digests as hashes, so I think being more explicit would be
better.

>
> [...] link to that concept section [...]
>

Added.

>
> StreamHash Concept
>
> Notice that I removed the "<ValueBits>" part. Â I don't think that should be part of the concept name.
>

The thing is that a StreamHash that looks at the bottom 4 bits of a
value in update_one is semantically very different one one that looks
at all 8 bits, so I considered that important enough to be part of the
concept name.  It also simplifies the description of HashAlgorithm,
since it needs to say somehow that stream_hash<4>::type is different
from stream_hash<8>::type.

>
> Rather than spelling out the operations that must be supported, why not list the concepts that must be supported, when reasonable: DefaultConstructible, CopyConstructible, CopyAssignable?
>

Done.

>
> What does, "Provided models are accessible through HashPolicy models," mean?
>

I've changed that part to read, "Each HashAlgorithm  model provides
access to all its associated StreamHash models; Those StreamHash
models are generally not accessible in other ways."  It's not
essential though, and could be removed if it's more confusing than
helpful.

Thanks for the extensive feedback,
~ Scott McMurray