search

trezor / trezor-firmware

:lock: Trezor Firmware Monorepo
https://trezor.io
Other
1.36k stars 661 forks source link

Clean up bignum API #1524

Open andrewkozlik opened 3 years ago

andrewkozlik commented 3 years ago

I am not sure how feasible the following changes are given that the bn_* functions might already be being used downstream, but let's see what we can do. The bignum functions have a very inconsistent API, which makes them rather error-prone to use and difficult to read any code that uses them, because it's nearly impossible to tell what each function does without looking at the declaration every single time.

  1. There is no clear position for the return value. For example: bn_mult_k(bignum256 *x, uint8_t k, const bignum256 *prime) bn_multiply(const bignum256 *k, bignum256 *x, const bignum256 *prime) bn_power_mod(const bignum256 *x, const bignum256 *e, const bignum256 *prime, bignum256 *res) The first function uses the first argument to return the result, the second uses the middle argument and the last uses the last argument to return the result. I am unable to discern any pattern among the functions. It would help if they respected some convention, ideally if the first argument was the return value as is common in the standard C library functions like memcpy(dest, source, n).

  2. The function name bn_mod() is misleading, because what it does is actually bn_mod(k, n) = k < n ? k : k - n. Perhaps rename to bn_mod_finish(). Also the signature of the function void bn_mod(bignum256 *x, const bignum256 *prime) is misleading, because the second parameter can be any nonzero normalized bignum256.

  3. The interface is rather inconsistent in terms of how similarly named functions behave, for example: bn_add(bignum256 *x, const bignum256 *y) bn_subtract(const bignum256 *x, const bignum256 *y, bignum256 *res) The first is x += y and the second is res = x - y. I actually don't have a problem with this, because you can tell from the number of parameters, so this can stay. However, the following is a total mess: bn_addi(bignum256 *x, uint32_t y) bn_subi(bignum256 *x, uint32_t y, const bignum256 *prime) bn_mult_k(bignum256 *x, uint8_t k, const bignum256 *prime) bn_read_uint32(uint32_t in_number, bignum256 *out_number) The names of the second and third function should end with _mod and they should all be consistent in indicating that one of the operands is a small integer. For example bn_add_uint32, bn_sub_uint32_mod, bn_mult_uint8_mod and the last function should have the arguments swapped as noted in point 1. It's important that the width of the integer type be clearly indicated in the function name, since if an uint32_t is passed to bn_mult_k() as the second argument, then it will get truncated to uint8_t without any warning.

  4. There are a lot of functions which are declared, but do not seem to be defined anywhere like bn_fast_mod_old(), bn_inverse_fast_1(), bn_inverse_fast_2(), bn_inverse_fast_3() or bn_inverse_old().

The above are just examples. Someone would need to go through the functions one-by-one to unify the naming and argument order.

prusnak commented 3 years ago

1 - 3

Correct observations

  1. There are a lot of functions which are declared, but do not seem to be defined anywhere like bn_fast_mod_old, bn_inverse_fast_1(), bn_inverse_fast_2(), bn_inverse_fast_3() or bn_inverse_old().

These are probably just leftovers which can be deleted. We removed these functions, but forgot to remove declarations. PR in https://github.com/trezor/trezor-firmware/pull/1525