remove keystore.c #1719
remove keystore.c #1719
Conversation
benma
force-pushed
the
keystore-c
branch
2 times, most recently
from
472a888 to
939adf1
Compare
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?
There was a problem hiding this comment.
👍 Removed the constant completely, see added commit.
| /// | ||
| /// # 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?
There was a problem hiding this comment.
Fixed, see added commit. This used to be true but stopped being true after some refactor in the past.
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.
It was a leftover from the time we used libwally-core. The same constant is available in bitcoin::secp256k1::constants::PUBLIC_KEY_SIZE.
keystore locked error used to be the case in the past when this function took the private key from the keystore, but that changed.
cedwies
left a comment
cedwies
left a comment
There was a problem hiding this comment.
Thanks for addressing the comments, code review tACK :)
3 participants
All functions in it are unrelated to the keystore.