`zcash_primitives::legacy::keys::AccountPrivKey::{to_bytes, from_bytes}` does not use the `xprv` encoding

[str4d]

When we defined a wallet-internal-only serialization for Unified Spending Keys, we wanted the transparent key part to use the same byte encoding as the xprv format from BIP 32 (i.e. without Base58 encoding, and without the leading version bytes that act like a Bech32 HRP). In the PR that made the change, I asked this question during review (https://github.com/zcash/librustzcash/pull/625#discussion_r964233132) and was told that hdwallet provided that encoding. As part of working on #1407, I have discovered this to be false.

The bytes used by the xprv encoding are:

1 byte: depth: 0x00 for master nodes, 0x01 for level-1 derived keys, ....
4 bytes: the fingerprint of the parent's key (0x00000000 if master key)
4 bytes: child number. This is ser32(i) for i in xi = xpar/i, with xi the key being serialized. (0x00000000 if master key)
32 bytes: the chain code
33 bytes: the public key or private key data (serP(K) for public keys, 0x00 || ser256(k) for private keys)

The hdwallet::ExtendedPrivKey::serialize encoding appears to be:

32 bytes: the private key data (ser256(k))
32 bytes: the chain code

This causes us a problem, because by omitting the extended metadata, we cannot parse an existing USK encoding with a transparent part into a bip32::ExtendedPrivateKey type (which requires the metadata).

[str4d]

Some of the missing information can be reconstructed from contextual information:

• ✅ depth is always 3, as AccountPrivKey is derived using the path m/44'/coin_type'/account'.
• ❌ We cannot determine the parent key fingerprint correctly without re-deriving the parent from the seed.
• ❓child number is account' (I believe including the hardening flag), which we "know" in the places we currently use the USK encoding (specifically for derived accounts in zcash_client_*), but don't necessarily have at the spot we are doing the decoding (in particular for imported UFVKs, support for which was recently added to zcash_client_*).

I also notice that AccountPubKey::{serialize, deserialize} is used in the UFVK encoding and has the same problem (for bip32 compatibility): it only encodes the chain code and pubkey. This uses the ordering chain code || pubkey and is therefore a suffix of the bytes used in the xpub encoding, rather than an entirely separate encoding. This encoding was intentional and is specified in ZIP 316; IIRC we decided that we didn't need any of this data and preferred the shorter UFVK encoding.

So in both cases we need to decide how to handle the unknown data. I think the "correct" way to handle this is to define AccountPrivKey and AccountPubKey as solely being wrappers around the chain code and pubkey / privkey, and then ensure there are no public APIs that expose the internal hdwallet (or soon-to-be bip32) types. Then when we migrate to bip32 we can just stick obviously-garbage data into the unknown fields.

The difference in encoding between AccountPrivKey (privkey || chaincode) and AccountPubKey (suffix of xpub) is annoying, and we may also want to fix this for AccountPrivKey.

[nuttycom]

I think it's fine to fix the USK encoding; the encoding methods are still under the unstable feature flag. Also, in order to stabilize this encoding, ZIP 316 would need to be updated to specify it, which hasn't yet been done.

The AccountPubKey encoding can stay as it is, I think; that choice was intentional.

[str4d]

I think it's fine to fix the USK encoding; the encoding methods are still under the unstable feature flag.

Agreed. The Android SDK already tells its users that they need to be prepared to regenerate the USK: https://github.com/Electric-Coin-Company/zcash-android-wallet-sdk/blob/159de09b2bf1cc1c752260c76d444c11dfabebc9/sdk-lib/src/main/java/cash/z/ecc/android/sdk/model/UnifiedSpendingKey.kt#L64-L74

The iOS SDK doesn't, but it also has a regeneration API like the Android SDK. And if we want this to be a joint we keep oiled, we need to exercise it.