Orchard key components #40

str4d · 2021-03-05T23:37:22Z

Closes #29.

codecov-io · 2021-03-06T00:10:43Z

Codecov Report

Merging #40 (71542f7) into main (35da179) will decrease coverage by 25.95%.
The diff coverage is 0.00%.

@@             Coverage Diff             @@
##             main      #40       +/-   ##
===========================================
- Coverage   46.66%   20.71%   -25.96%     
===========================================
  Files           5        8        +3     
  Lines          75      169       +94     
===========================================
  Hits           35       35               
- Misses         40      134       +94

Impacted Files	Coverage Δ
src/address.rs	`0.00% <0.00%> (ø)`
src/keys.rs	`0.00% <0.00%> (ø)`
src/primitives/redpallas.rs	`0.00% <0.00%> (ø)`
src/primitives/sinsemilla.rs	`64.81% <0.00%> (-12.97%)`	⬇️
src/spec.rs	`0.00% <0.00%> (ø)`

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 35da179...71542f7. Read the comment docs.

dconnolly

Before getting into the details, any tests?

In zebra-chain we implemented proptest strategies for the root of the key derivation chain, and wrote tests to check derivation like this:
https://github.com/ZcashFoundation/zebra/blob/main/zebra-chain/src/sapling/keys/tests.rs

dconnolly · 2021-03-06T00:28:31Z

src/keys.rs

+di_from!(u8);
+di_from!(u16);
+di_from!(u32);
+di_from!(u64);
+di_from!(usize);


Are these all expected to be needed?

u8 and u16 are probably the least likely to be used. usize makes sense from a Rust perspective (given that this is an index), and u32, u64 are quite common to see as e.g. customer IDs in SQL tables.

Would it make sense to also support u128 and/or UUID (16 octets) as a source directly? If you want to support database identifiers, UUIDs are becoming pretty common.

These would require a TryFrom implementation (because they might be out-of-range), so I'm less inclined to support them directly; in that case the user probably needs to define an explicit mapping from the UUID to a diversifier (since the UUID is likely random, and thus almost guaranteed to be out-of-range).

src/keys.rs

dconnolly · 2021-03-06T00:31:21Z

src/address.rs

+impl Address {
+    pub(crate) fn from_parts(d: Diversifier, pk_d: pallas::Point) -> Self {
+        Address { d, pk_d }
+    }
 }


As mentioned in keys.rs, instead of a new method from_parts this could be accomplished with impl From<(Diversifier, DiversifiedTransmissionKey)> for Address

This is an internal API, for which I prefer to be explicit.

For Zebra usage this may not stay internal

If it does become necessary to expose it for Zebra, we can do so in a subsequent PR.

src/address.rs

str4d · 2021-03-06T00:33:34Z

Before getting into the details, any tests?

The Zebra tests you linked to fall into two categories:

Checking against test vectors.
Checking round-trip encodings of keys and addresses.

This PR does not implement encodings, so the latter do not apply (yet). For the former, we are writing a parallel implementation in Python (ideally by someone who is not involved in writing this implementation) for the purpose of generating test vectors that this implementation can be tested against. This is started in zcash/zcash-test-vectors#14

src/keys.rs

dconnolly · 2021-03-06T00:45:41Z

Before getting into the details, any tests?

The Zebra tests you linked to fall into two categories:

Checking against test vectors.

Checking round-trip encodings of keys and addresses.

This PR does not implement encodings, so the latter do not apply (yet). For the former, we are writing a parallel implementation in Python (ideally by someone who is not involved in writing this implementation) for the purpose of generating test vectors that this implementation can be tested against. This is started in zcash-hackworks/zcash-test-vectors#14

Fantastic!

Are there tests we can create to make sure the code in this PR is exercised somewhat, before those test vectors are available?

str4d · 2021-03-06T00:53:11Z

I don't see any meaningful unit tests given we currently only have a linear derivation tree (we could test e.g. FullViewingKey::default_address, but that's kind-of pointless for me to write at the same time; if I made a mistake, I'd likely duplicate it in the test). But we could definitely add documentation tests that checks the code runs.

Co-authored-by: Deirdre Connolly <[email protected]>

dconnolly · 2021-03-06T01:03:36Z

src/keys.rs

 /// A key that provides the capability to view incoming and outgoing transactions.
 ///
 /// This key is useful anywhere you need to maintain accurate balance, but do not want the
 /// ability to spend funds (such as a view-only wallet).


Suggested change

/// A key that provides the capability to view incoming and outgoing transactions.

///

/// This key is useful anywhere you need to maintain accurate balance, but do not want the

/// ability to spend funds (such as a view-only wallet).

/// An Orchard full viewing key, as defined in [Zcash Protocol Spec §4.2.3: Orchard Full Viewing Keys][orchardfullviewingkeyencoding]

///

/// Provides the capability to view incoming and outgoing transactions. This key is

/// useful anywhere you need to maintain accurate balance, but do not want the ability

/// to spend funds (such as a view-only wallet).

///

/// [orchardfullviewingkeyencoding]: https://zips.z.cash/protocol/nu5.pdf#orchardfullviewingkeyencoding

Is there a better section in the spec? Also the line wrap might be wonky

That section of the specification defines the string encoding of fvk, not fvk itself.

I'm not sure yet whether it makes sense to implement string encoding in this crate or the downstream users (zcash_primitives and zebra-chain), since doing so requires knowledge of the network type, which the downstream crates define but the Orchard protocol is agnostic over. It would be nice if the two downstream crates could agree on a network type, but that seems like it might require a larger refactor beyond Orchard.

That section of the specification defines the string encoding of fvk, not fvk itself.

I'm not sure yet whether it makes sense to implement string encoding in this crate or the downstream users (zcash_primitives and zebra-chain), since doing so requires knowledge of the network type, which the downstream crates define but the Orchard protocol is agnostic over. It would be nice if the two downstream crates could agree on a network type, but that seems like it might require a larger refactor beyond Orchard.

Oh very agree. I think there is no better layout/description of the Orchard Full Viewing Key than this section at present, the best other section i can see in version v2021.1.16-10-g928b3d is https://zips.z.cash/protocol/nu5.pdf#addressesandkeys, unless I'm missing something or there are incoming additions. I think this is a holdover from Sapling but.

oof that may mean we can't just impl FromStr and Display on this in zebra-chain, we'd have to wrap it or transform from this key: https://doc.zebra.zfnd.org/src/zebra_chain/sapling/keys.rs.html#684
That's ok, that's one of the few key types that has any external type impacting them like that for us

Use https://zips.z.cash/protocol/nu5.pdf#orchardkeycomponents for the spec link.

I started putting together a zcash_address crate which I think we could use as a common base for encodings. We'd impl FromAddress for orchard::PaymentAddress, for example (only defining the Orchard payment address conversion, for example), which would erase network-specific information where we don't need it, but the zebra-chain address type could retain it.

(The above crate is focused on addresses rather than keys, but we could maybe generalise it for keys in a similar fashion. I'm a bit on-the-fence about whether I want key encodings in an address-focused crate, but I also don't want to see everything become a mish-mash of tiny crates...)

src/spec.rs

src/keys.rs

dconnolly · 2021-03-06T01:25:16Z

@str4d a Q about DRYness vs, idk, explicitness in docstrings: there are several types that are similarly named between Orchard and Sapling, such as Incoming Viewing Keys, etc. In prose, not code, should we consistently distinguish them as 'Orchard Incoming Viewing Keys' etc in this library, just to be clear? Just so I'm not like, suggesting the same thing all over every doc comment. I don't have a particularly strong opinion on it.

src/keys.rs

src/constants.rs

src/keys.rs

src/spec.rs

This makes diversified address generation fallible (though with negligible probability). We expose this to users, so they can decide how to handle it (either just unwrapping, or incrementing the diversifier index). We alter spending key construction to reject spending keys that would not result in a default address (with diversifier index 0).

This is a more usable API, which we can use when we have the full viewing key and can obtain the DiversifierKey.

The protocol spec now returns \mathbb{P}_x instead of a bit sequence, matching what we do here.

str4d · 2021-03-16T21:17:03Z

src/spec.rs

+    if pk_d.is_identity().into() {
+        None


In my pairing with @ebfull, we discussed this, and it occurred to me that it's possible we actually never hit this: once we fix Pallas and the hash-to-curve domain separator, the diversifier space is fixed for all spending keys. Thus any diversifier that resulted in the identity would do so for all spending keys (though the spending key's diversifier key would alter which index that occured at, which could affect the default address).

We could in theory brute-force the 88-bit space to confirm that no diversifier will result in the identity. It's not practical to do so, but given that it's an 88-bit value being hashed into a 255-bit space, I think it's probably reasonable to just assert we never hit it. Thoughts @daira?

Another thing we could do is that if the result of the group hash is the zero point, we just replace it with another arbitrary point, such as another group hash output or (-1, 2). I would mildly prefer that.

The type of DiversifyHash should now be B -> P* (without the bottom).

Caught it in 4.1.1 but not in 5.4.1.6. That'll be in the next spec revision.

src/spec.rs

src/primitives/sinsemilla.rs

daira

utACK with comments.

ebfull · 2021-03-17T17:17:53Z

Spec is being modified: when DiversifyHash ordinarily samples the identity, we substitute it with the output of GroupHash("z.cash:Orchard-gd", "").

DiversifyHash is altered to replace the identity with another fixed point that is known to not be the identity.

src/spec.rs

src/primitives/sinsemilla.rs

src/spec.rs

daira

ACK with comments.

Co-authored-by: Daira Hopwood <[email protected]>

str4d added 2 commits March 5, 2021 23:28

Orchard key components

f077979

Implement ZIP 32 diversifier derivation

ceac39d

str4d added this to the Core Sprint 2021-08 milestone Mar 5, 2021

str4d requested review from daira and ebfull March 5, 2021 23:37

str4d added 2 commits March 5, 2021 23:46

Use reddsa to instantiate orchard::redpallas

eaa7158

Use orchard::redpallas types in orchard::keys implementation

2750170