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

Thanks for this detailed review! Below the comments I did not address in the branch.

On 09.04.2011, at 02:04, Stefan Götz wrote:
> Hi René!
>
>> === modified file 'hipd/cookie.c'
>> --- hipd/cookie.c 2011-04-05 16:44:22 +0000
>> +++ hipd/cookie.c 2011-04-08 15:41:17 +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 int hip_cookie_difficulty = 0; /* a difficulty of i leads to approx. 2^(i-1) hash computations during BEX */
>
> [L] 'cookie' and 'puzzle' seem to refer to the same thing. It would be
> great if this ambiguity were removed from the code and only a single
> name was used.

Agreed, but that's not in the focus of this branch. I wouldn't want to change hip_cookie_difficulty to hip_puzzle_difficulty here, as the variable is located in cookie.c. Instead, we should probably rename the whole cookie functionality to puzzle.

>> === modified file 'lib/core/protodefs.h'
>> --- lib/core/protodefs.h 2011-02-04 14:44:37 +0000
>> +++ lib/core/protodefs.h 2011-04-08 15:41:17 +0000
>> @@ -737,13 +737,17 @@
>> uint64_t generation;
>> } __attribute__ ((packed));
>>
>> +
>> +/* puzzle and solutions are defined to have a length of 8 bytes */
>> +#define PUZZLE_LENGTH 8
>
> [M] 'const unsigned PUZZLE_LENGTH = 8;'

This won't work as I'm using PUZZLE_LENGTH for array definitions in structs, etc.

>> +static int hip_calculate_puzzle_solution(const struct puzzle_common *const compute_puzzle,
>> + unsigned char sha_digest[SHA_DIGEST_LENGTH],
>> + const int difficulty)
>> {
>
> [M] It seems that 'sha_digest' is accessed only within this function
> but never by any of its callers. Please declare it as a local variable
> instead of a function parameter.

It's called in a for loop by hip_solve_puzzle(). I don't want memory to be
allocated each time.

> [M] Since this function returns only two distinct values, please use
> 'bool' instead of 'int'

It returns -1, 0, and 1.

>> +int hip_verify_puzzle_solution(const uint8_t puzzle[PUZZLE_LENGTH],
>> + const uint8_t difficulty,
>> + const hip_hit_t initiator_hit,
>> + const hip_hit_t responder_hit,
>> + const uint8_t solution[PUZZLE_LENGTH])
>
> [M] This function only returns two distinct values. Please use 'bool'
> as the return type instead.

It's done like this everywhere in HIPL for notifying about errors. I don't want to introduce a "new way" of doing that in this branch.

> Btw. I like it that hip_hit_t's are passed around by value as opposed
> by error-prone reference.

Me too :)

>> === modified file 'lib/core/solve.h'
>> --- lib/core/solve.h 2010-10-15 15:29:14 +0000
>> +++ lib/core/solve.h 2011-04-08 15:41:17 +0000
>> @@ -30,10 +30,20 @@
>>
>> #include "protodefs.h"
>>
>> -#define HIP_PUZZLE_MAX_K 28
>> -
>> -uint64_t hip_solve_puzzle(const void *puzzle,
>> - const struct hip_common *hdr, int mode);
>> -int hip_solve_puzzle_m(struct hip_common *out, struct hip_common *in);
>> +/* right now we only support difficulties up to sizeof(int), as only ffs
>> + * is available on OpenWRT */
>
> [M] 'difficulties of up to sizeof(int)' is incorrect because in fact
> it is 'MIN(28, sizeof(int))'.

Nope, the difficulty can be max sizeof(int). It might be any lower value though.

>> +static const int max_puzzle_difficulty = sizeof(int) * 8 >= 28 ? 28 : sizeof(int) * 8;

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/