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

On 07.04.2011, at 21:59, Christof Mroz wrote:
> Review: Needs Fixing
> Overall, this is definately cleaner than the old code.
>
> There is one piece of code that does not match its corresponding comment; as soon as this is clarified (or fixed), I'll approve.
> Would be nice if you could address the other points too (mainly missing documentation for newly added functions).
>
>> +static int hip_calculate_puzzle_solution(const struct puzzle_common *const compute_puzzle,
>> + unsigned char sha_digest[SHA_DIGEST_LENGTH],
>> + const int difficulty)
>> {
>> - uint64_t mask = 0;
>> - uint64_t randval = 0;
>> - uint64_t maxtries = 0;
>> - uint64_t digest = 0;
>> - uint8_t cookie[48];
>> - int err = 0;
>> - const union {
>> - struct hip_puzzle pz;
>> - struct hip_solution sl;
>> - } *u;
>> -
>> - HIP_HEXDUMP("puzzle", puzzle_or_solution,
>> - (mode == HIP_VERIFY_PUZZLE ? sizeof(struct hip_solution) :
>> - sizeof(struct hip_puzzle)));
>> -
>> - /* pre-create cookie */
>> - u = puzzle_or_solution;
>> -
>> - HIP_IFEL(u->pz.K > HIP_PUZZLE_MAX_K, 0,
>> - "Cookie K %u is higher than we are willing to calculate"
>> - " (current max K=%d)\n", u->pz.K, HIP_PUZZLE_MAX_K);
>> -
>> - mask = hton64((1ULL << u->pz.K) - 1);
>> - memcpy(cookie, &u->pz.I, sizeof(uint64_t));
>> -
>> - HIP_DEBUG("(u->pz.I: 0x%llx\n", u->pz.I);
>> -
>> - if (mode == HIP_VERIFY_PUZZLE) {
>> - ipv6_addr_copy((hip_hit_t *) (cookie + 8), &hdr->hits);
>> - ipv6_addr_copy((hip_hit_t *) (cookie + 24), &hdr->hitr);
>> - randval = u->sl.J;
>> - maxtries = 1;
>> - } else if (mode == HIP_SOLVE_PUZZLE) {
>> - ipv6_addr_copy((hip_hit_t *) (cookie + 8), &hdr->hitr);
>> - ipv6_addr_copy((hip_hit_t *) (cookie + 24), &hdr->hits);
>> - maxtries = 1ULL << (u->pz.K + 3);
>> - get_random_bytes(&randval, sizeof(uint64_t));
>> + uint32_t truncated_digest = 0;
>> + int err = -1;
>> +
>> + HIP_IFEL(difficulty > max_puzzle_difficulty,
>> + 1, "difficulty exceeds max. configured difficulty\n");
>> +
>> + HIP_IFEL(hip_build_digest(HIP_DIGEST_SHA1,
>> + compute_puzzle,
>> + sizeof(struct puzzle_common),
>> + sha_digest),
>> + -1, "failed to compute hash digest\n");
>
> You could return early with a (successful) solution of zero if difficulty == 0.
> If the caller is responsible of taking that shortcut, I'd add an assertion here to catch future callers who forget about this.

Done. I don't see your point in adding an assertion here.

>> + /* we are interested in the 8 least significant bytes */
>> + truncated_digest = *(uint32_t *) &sha_digest[SHA_DIGEST_LENGTH - sizeof(uint32_t)];
>
> The 8 least significant bytes have index 0..7. Also, sizeof(uint32_t) is 4, not 8.
>
>> +int hip_solve_puzzle(const uint64_t puzzle,
>> + const int difficulty,
>> + const hip_hit_t initiator_hit,
>> + const hip_hit_t responder_hit,
>> + uint64_t *solution)
>> +{
>> + struct puzzle_common compute_puzzle;
>> + unsigned char sha_digest[SHA_DIGEST_LENGTH];
>> + int err = -1;
>> + uint64_t i = 0;
>> +
>> + HIP_IFEL(!solution, -1, "invalid parameter passed (NULL)\n");
>> +
>> + HIP_IFEL(difficulty > max_puzzle_difficulty, -1,
>> + "Cookie (K = %i) is higher than we are willing to calculate"
>> + " (current max K = %i)\n", difficulty, max_puzzle_difficulty);
>
> Again, we can exit early for difficulty = 0.

Done.

>> +}
>> +
>> +int hip_verify_puzzle_solution(const uint64_t puzzle,
>> + const uint8_t difficulty,
>> + const hip_hit_t initiator_hit,
>> + const hip_hit_t responder_hit,
>> + const uint64_t solution)
>
> Missing documentation (there are other instances too).

Done. Also for the other instances.

>> +/* right now we only support difficulties up to sizeof(int), as only ffs
>> + * is available on OpenWRT */
>
> OpenSSL bignums (which are used in HIPL anyway) could be an alternative to ffs(), but a poor WRT is probably not gonna handle such a big puzzle anyway... so I'd say your Solution is OK.

Well, I don't think we will have puzzles of difficulty 32 any time soon.

>> +// For unknown reasons, this file does not compile with the following,
>> +// seemingly useless forward declaration
>> +Suite *lib_core_solve(void);
>
> What's the error message?

This was previously fixed in trunk. Changed accordingly.

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/