remove keystore.c #1719
remove keystore.c #1719
Conversation
| let mut signer_commitment = [0u8; EC_PUBLIC_KEY_LEN]; | ||
| match unsafe { | ||
| bitbox02_sys::keystore_secp256k1_nonce_commit( | ||
| let mut signer_commitment = MaybeUninit::<bitbox02_sys::secp256k1_ecdsa_s2c_opening>::uninit(); |
There was a problem hiding this comment.
why do you prefer uninitialized bytes here instead of zeroed() (like earlier)?
There was a problem hiding this comment.
It's transferred from the C impl, where there was a = {0} in the C version for this one and not the other. I don't think it matters, should I change the other to uninit() as well?
There was a problem hiding this comment.
Changed the other to uninit()
src/keystore.h
Outdated
| #define KEYSTORE_U2F_SEED_LENGTH SHA256_LEN | ||
|
|
||
| // Max. length of an xpub string, including the null terminator. | ||
| #define XPUB_ENCODED_LEN 113 |
There was a problem hiding this comment.
XPUB_ENCODED_LEN is only used in rust, migrate as well?
There was a problem hiding this comment.
Indeed, can be removed, it's unused. Will do.
There was a problem hiding this comment.
Done, removed it and keystore.h
src/keystore.h
Outdated
|
|
||
| #include <secp256k1.h> | ||
|
|
||
| #define KEYSTORE_U2F_SEED_LENGTH SHA256_LEN |
There was a problem hiding this comment.
move KEYSTORE_U2F_SEED_LENGTH to rust or u2f.c?
There was a problem hiding this comment.
It's a bit confusing, SHA256_LEN is defined in u2f.c, so shouldn't be available here? 🤔
I'll delete it anyway, the only use is in SHA256 functions, so we can just use 32 (like we often do in hashing), or SHA256_LEN in u2f.c.
There was a problem hiding this comment.
Done, removed keystore.h
Fix undefined secp256k1_* symbols when linking firmware. These errors would happen if keystore.c stops referencing the secp256k1 symbols (see later commit). We link libsecp256k1 as a static archive and the linker resolves static libs left-to-right, only pulling in objects that satisfy currently-unresolved symbols. Since the Rust archive introduces the secp256k1_* references, it must appear before the secp256k1 library on the link line (otherwise the linker scans secp256k1 too early and doesn't pull the needed objects).
It has nothing to do with the keystore.
They are not related to the keystore. The keystore.c functions they use will be dealt with in the next commit.
The two remaning functions are unrelated to keystore, and inlined into the Rust C wrappers of secp256k1.
cedwies
left a comment
cedwies
left a comment
There was a problem hiding this comment.
Found no major issues. Nice refactoring. Please consider my comments.
| } | ||
|
|
||
| /// Length of a compressed secp256k1 pubkey. | ||
| const EC_PUBLIC_KEY_LEN: usize = 33; |
There was a problem hiding this comment.
why are you not using bitbox02::secp256k1::EC_PUBLIC_KEY_LEN here? Why did you choose the duplicate value?
| /// | ||
| /// # Returns | ||
| /// * `Ok(SignResult)` containing signature in compact format and recoverable id on success | ||
| /// * `Err(())` if the keystore is locked |
There was a problem hiding this comment.
This doc was moved, but I think this line is incorrect. Does this fn even look at "keystore locked"? Isn't this checked before this fn runs?
3 participants
All functions in it are unrelated to the keystore.