Migrate from hdwallet to bip32
#1421
Conversation
| - `zcash_primitives::legacy::keys`: | ||
| - `AccountPrivKey::{from_bytes, to_bytes}` now use the byte encoding from the | ||
| inside of a `xprv` Base58 string encoding from BIP 32, excluding the prefix | ||
| bytes (i.e. starting with `depth`). | ||
| - `AccountPrivKey::from_extended_privkey` now takes | ||
| `bip32::ExtendedPrivateKey<secp256k1::SecretKey>`. | ||
| - The following methods now return `Result<_, bip32::Error>`: | ||
| - `AccountPrivKey::from_seed` | ||
| - `AccountPrivKey::derive_secret_key` | ||
| - `AccountPrivKey::derive_external_secret_key` | ||
| - `AccountPrivKey::derive_internal_secret_key` | ||
| - `AccountPubKey::derive_external_ivk` | ||
| - `AccountPubKey::derive_internal_ivk` |
There was a problem hiding this comment.
Would this be a good time to move the transparent keys to zcash_keys? I think that if we did so, zcash_keys would no longer need to depend upon zcash_primitives.
There was a problem hiding this comment.
Hmm, I kinda see it, but also the other protocol-specific key types exist in their own crates, so zcash-keys may not be where we want this code.
There was a problem hiding this comment.
It may instead make more sense to plan a move to some zcash_transparent crate that involves not just the key material, but the transparent builder logic etc. as well.
There was a problem hiding this comment.
In some sense bip32 is that protocol-specific key derivation crate; we just have to add some more layers for Zcash, so I think this going in zcash_keys would be reasonable.
The advantage to moving it to zcash_keys is that then the bip32 dependency can go behind the transparent-inputs feature flag, and we get some nice additional pruning of dependencies.
There was a problem hiding this comment.
Moving these as-is may mean that zcash_keys no longer depends on zcash_primitives, but it would mean that zcash_primitives now needs to depends on zcash_keys, because the rest of the transparent logic is still there and needs these key types. So the move would not decouple the crates, it would just invert their relationship.
As part of this, we migrate to `secp256k1 0.27`. This version does not bump `secp256k1-sys`, so remains compatible with the `libsecp256k1` revision used in `zcashd`. The `zcash_primitives::legacy::keys::AccountPrivKey` encoding also changes to preserve the transparent extended key metadata. Previously the type was documented as such, but only encoded the private key and chain code; the new encoding now matches the documentation. As a side effect, the unstable encoding of `zcash_keys::keys::UnifiedSpendingKey` also changes. Closes #1407. Closes #1408.
Codecov ReportAttention: Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #1421 +/- ##
==========================================
+ Coverage 63.14% 63.16% +0.01%
==========================================
Files 126 126
Lines 14719 14729 +10
==========================================
+ Hits 9294 9303 +9
- Misses 5425 5426 +1 ☔ View full report in Codecov by Sentry. |
| @@ -8,6 +8,9 @@ and this library adheres to Rust's notion of | |||
| ## [Unreleased] | |||
| ### Changed | |||
| - MSRV is now 1.70.0. | |||
| - `zcash_client_sqlite::error::SqliteClientError` has changed variants: | |||
| - Removed `HdwalletError`. | |||
| - Added `TransparentDerivation`. | |||
There was a problem hiding this comment.
Note that we also currently do an .unwrap() after calling derive_secret_key in hdwallet from zcash_client_backend here:
librustzcash/zcash_client_backend/src/data_api/wallet.rs
Lines 826 to 829 in 03fc64c
It would be good to be able to report a useful error there, but zcash_client_backend has no suitable error type to do so.
librustzcash [changed their USK encoding](zcash/librustzcash#1421) recently.
As part of this, we migrate to
secp256k1 0.27. This version does not bumpsecp256k1-sys, so remains compatible with thelibsecp256k1revision used inzcashd.The
zcash_primitives::legacy::keys::AccountPrivKeyencoding also changes to preserve the transparent extended key metadata. Previously the type was documented as such, but only encoded the private key and chain code; the new encoding now matches the documentation. As a side effect, the unstable encoding ofzcash_keys::keys::UnifiedSpendingKeyalso changes.Closes #1407.
Closes #1408.