Refactor ZIP 32 code #1043
Refactor ZIP 32 code #1043
Conversation
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1043 +/- ##
==========================================
- Coverage 70.65% 70.54% -0.11%
==========================================
Files 140 140
Lines 13802 13792 -10
==========================================
- Hits 9752 9730 -22
- Misses 4050 4062 +12 ☔ View full report in Codecov by Sentry. |
|
In a pairing with @nuttycom, we decided to simplify this by just removing non-hardened derivation support from our Sapling implementation (which we have never used, and to our knowledge no one else widely does either). |
|
I thought I remembered a dependency of ZIP 316 on non-hardened derivation. Indeed there is, but only transparent non-hardened derivation, not Sapling. Still, we had better check that none of the ecosystem wallets in common use are using it with Sapling. |
str4d
force-pushed
the
zip32-refactor
branch
from
1f6c0a7
to
f40337a
Compare
|
Force-pushed to remove non-hardened derivation and simplify the resulting refactor. |
| @@ -1193,506 +1151,431 @@ mod tests { | |||
| }, | |||
| TestVector { | |||
| ask: Some([ | |||
| 0x28, 0x2b, 0xc1, 0x97, 0xa5, 0x16, 0x28, 0x7c, 0x8e, 0xa8, 0xf6, 0x8c, 0x42, | |||
There was a problem hiding this comment.
These updated test vectors come from zcash/zcash-test-vectors#94.
There was a problem hiding this comment.
I was just about to ask about that. I also noticed that the URL at the top of this block of test vectors should now change.
There was a problem hiding this comment.
Yeah, it would have changed automatically had the test vector repo been updated when the repo was moved.
| @@ -131,6 +139,9 @@ and this library adheres to Rust's notion of | |||
| - `fees::fixed::FeeRule::fixed_fee` | |||
| - `fees::zip317::FeeRule::marginal_fee` | |||
| - `sighash::TransparentAuthorizingContext::input_amounts` | |||
| - `zcash_primitives::zip32`: | |||
| - `ChildIndex` has been changed from an enum to an opaque struct, and no | |||
| longer supports non-hardened indices. | |||
There was a problem hiding this comment.
Do you want to add a note to ZIP 32 that discourages or deprecates use of non-hardened indices with Sapling?
I feel like this requires more community discussion.
There was a problem hiding this comment.
ZIP 32's existing language already somewhat discourages it, particularly the language that specifies Orchard. We only specify non-hardened derivation for an optional address_index path extension for Sapling with MAY, and then immediately note that zcashd did this path extension with hardened derivation instead (which made sense for its use case, and also means I still am unaware of anyone ever exercising this MAY).
I also note that while we do still use a non-hardened-like derivation process to produce Sapling internal keys, it is domain-separated from non-hardened derivation and thus distinct (so we don't use the functionality being removed here).
| ChildIndex::NonHardened(i) => i, | ||
| } | ||
| /// Returns the index as a 32-bit integer, including the hardened bit. | ||
| pub fn index(&self) -> u32 { |
There was a problem hiding this comment.
The naming here is confusing to me, it's the opposite of what I had thought you had been suggesting; from the perspective of an API user, I would expect "index" to mean the zero-based integer in either the non-hardened or the hardened range, and the "value" to mean the packed binary representation.
There was a problem hiding this comment.
ZIP 32 uses "index" to mean the thing that includes the hardened bit (i.e. index >= 2^31 means "hardened"). The closest thing it defines to what I call "value" here is "path level", but that's in the context of a full path, not a single standalone parent-to-child derivation.
There was a problem hiding this comment.
Hm. That's unfortunate naming in the ZIP then, but I suppose we're probably stuck with it and this should agree with the ZIP.
There was a problem hiding this comment.
The "index" terminology is inherited from BIP 32.
| /// Parses the given ZIP 32 child index. | ||
| /// | ||
| /// Returns `None` if the hardened bit is not set. | ||
| pub fn from_index(i: u32) -> Option<Self> { |
There was a problem hiding this comment.
I would almost prefer that this operation be called decode or something of the sort, pursuant to my previous comment.
str4d
force-pushed
the
zip32-refactor
branch
from
a86299c
to
b81b8ce
Compare
|
Rebased on |
str4d
force-pushed
the
zip32-refactor
branch
from
b81b8ce
to
7064efe
Compare
|
Force-pushed to make |
The Sapling-specific code moves to
zcash_primitives::sapling::zip32, to be part of thesapling-cryptoextraction.The protocol-independent types are refactored to support this refactor, and to enable themselves to be refactored into a protocol component crate that can be used by both
sapling-cryptoandorchard.Part of #738.