Code review comment for lp:~rene-hummen/hipl/puzzle-fixes

On 14.04.2011, at 08:21, Stefan Götz wrote:
> Hi René!
>
> review:needs_fixing
>
>> === modified file 'hipd/cookie.c'
>> --- hipd/cookie.c 2011-04-05 16:44:22 +0000
>> +++ hipd/cookie.c 2011-04-12 17:04:30 +0000
>> @@ -56,7 +56,7 @@
>>
>> #define HIP_R1TABLESIZE 3 /* precreate only this many R1s */
>>
>> -static int hip_cookie_difficulty = 1ULL; /* a difficulty of i leads to approx. 2^(i-1) hash computations during BEX */
>> +static uint8_t hip_cookie_difficulty = 0; /* a difficulty of i leads to approx. 2^(i-1) hash computations during BEX */
>
> [M] The change from int to uint8_t is a step backwards in my opinion
> for two reasons:
>
> 1) built-in types should be used in preference to defined types,
> unless specifically required to, e.g., achieve a guaranteed data
> layout. In this case this means that 'unsigned short' is preferable to
> 'uint8_t'.

I don't mind the data type so much as long as it is unsigned. However, the difficulty will never be higher than 256 (see below).

> 2) I assume that uint8_t was chosen to limit the range of supported
> values to 256 instead of 2^32, correct? Is 256 indeed a useful
> limitation? I thought it should not be greater than 32 due to the
> limitations of OpenWRT, right? So if 'hip_cookie_difficulty' is 33
> it's just as bad as if it was 257, right? If that is so, it seems to
> me that the change to 'uint8_t' does not have an advantage.
> It has a disadvantage though: 'uint8_t' is less efficient to handle
> than 'int' or 'unsigned int'.
> Thus, I'd recommend to change the type to 'unsigned int' or leave it
> at 'int'. The same goes for all related 'uint80_t' variables and
> function arguments.

uint8_t is simply the length of the header field on the wire. That seems like a natural limitation for me. The fact that we only support difficulties up to 32 is just an implementation issue of one function and might change in the future.

>> +static int hip_single_puzzle_computation(const struct puzzle_hash_input *const puzzle_input,
>> + unsigned char sha_digest[SHA_DIGEST_LENGTH],
>> + const uint8_t difficulty)
>
> [M] I guess the fact that 'sha_digest' is still a function parameter
> instead of a local variable is due to our bad review/fix timing
> yesterday?

Hmm, that's a bit strange. The merge proposal on launchpad already show my change here.

René

--
Dipl.-Inform. Rene Hummen, Ph.D. Student
Chair of Communication and Distributed Systems
RWTH Aachen University, Germany
tel: +49 241 80 20772
web: http://www.comsys.rwth-aachen.de/team/rene-hummen/