Open oxarbitrage opened 3 years ago
As far as importing secret keys, we decided as a team that implementations can handle this functionality on their own. We do allow for serialization/deserialization of key material, which allows them to/restore save key material to/from disk. We might want to document this for users in the library documentation, though.
The audit actions are of 2 types: Security and Others. This ticket is to keep track of the "Others". On each point we need a PR or an explication on why we consider no de change is needed.
Audit can be found in https://github.com/ZcashFoundation/redjubjub/blob/main/zcash-frost-audit-report-20210323.pdf
3.1 Potential API improvements
[x] Restrict the maximum number of signers and threshold parameters. At the moment these parameters are defined using
u32
type. Consider usingu8
or to put some hard-coded constant for the maximal intended numbers.Comments: We are actually using
u8
now but this might change.[ ] Document what the library does in relation to how to import secret keys. This can be required in certain use cases. The trusted dealer will have a way to instead of randomly picking the secret key, to take existing one. - #104
[x] Derive
Debug
,Eq
,PartialEq
for participants local public keys. This will allow implementers more freedom to work with public outputs of the key generation.[ ] Serialization and deserialization may be implemented for messages. It is hard to imagine how the code can be run over distributed network without such functionality.
Comments: Work in progress, the design is https://github.com/ZcashFoundation/redjubjub/pull/85 and an initial implementation PR is at https://github.com/ZcashFoundation/redjubjub/pull/95
Unused values
[x] The
KeyPackage
structure is never used, which leads to the warnings.Potential fix at https://github.com/ZcashFoundation/redjubjub/pull/99
3.2 Outdated dependencies
[x] The
byteorder
andrand_core
crates are outdated versions (presumably,rand_core
is on purpose pre-0.6). We suggest to update to the latest version after testing it, and potentially reviewing the changes.core_rand
updated at https://github.com/ZcashFoundation/redjubjub/pull/55 andbyteorder
in https://github.com/ZcashFoundation/redjubjub/pull/1003.3 Test coverage
3.4
unwrap()
risks[ ] We recommend to be careful with
unwrap()
usage, as it could lead to panics upon None/Err . Instead, use pattern matching to gracefully fail.There is a PR with some work in this subject at https://github.com/ZcashFoundation/redjubjub/pull/101
3.5 Null ID support
f(x)
is used to generate each party’s share of the secret. The actual secret isf(0)
and the party with ID i will be given a share with valuef(i)
. Since a DKG may be implemented in the future, we recommend that the ID 0 be declared invalid.3.6 Typo in a comment
[x] "langrange" instead of "lagrange" in https://github.com/ZcashFoundation/redjubjub/blob/2ebc08f91078617e5f7f34c37cd244beb850fe9e/src/frost.rs#L492
PR: https://github.com/ZcashFoundation/redjubjub/pull/97