Analyze overlap between `zebra_chain` and `zcash_primitives`

[hdevalence]

Now that the zcash_primitives crate has been published and we have mostly finished the networking code, we should analyze the overlap between zcash_primitives and zebra_chain, and decide:

1. What (intended) functionality is common between these two crates;
2. Whether we should re-export parts of zcash_primitives into zebra_chain;
3. Whether we should combine parts of zebra_chain into zcash_primitives and upstream the changes.

[hdevalence]

I started writing some first-pass notes mapping out the zcash_primitives crate. They are currently UNFINISHED, and I will keep editing this comment until they are done.

Notes on zcash_primitives, starting from lib.rs and working outwards. The order they appear here is the order in which I read through them, which is mostly arbitrary but attempts to be in internal module dependency order.

lib.rs

• all modules are public, except for the serialize and util modules, which are private.
• the crate does not use a reëxport façade, except that it does privately re-export jubjub::JubjubBls12 into the crate root.
• defines a lazy_static!'d JUBJUB instance.

serialize.rs

• defines an empty struct CompactSize with CompactSize encoding and decoding logic.
• CompactSize instances are never constructed, they're only used to name the encoding methods.
• decoding logic is more strict than ours, rejecting non-canonical encodings.
• we put this in an extension trait rather than creating a type, because there are no values of type CompactSize.
• similarly, defines an empty struct Vector and struct Optional, used for encoding arrays and Option<T>s, respectively.
• we have array encoding in an extension trait but do not do option decoding.
• does not contain a serialization trait – serialization is done by read and write methods outside of a trait.

util.rs

• contains a single function, hash_to_scalar<E: JubjubEngine>

block.rs

• struct BlockHash(pub [u8; 32])
• Display impl reverses the bytes of the hash before hex encoding -- is this to make the format line up with something else, and should we try to match?
• from_slice constructor
• struct BlockHeaderData
  • seems to contain fields exactly as laid out in the block header
  • no wrapper types for nonce, merkle roots,
• struct BlockHeader { hash: BlockHash, data: BlockHeaderData }
  • from_data constructor (not a From impl) computes a hash
  • read, write functions
  • impl Deref<Target=BlockHeaderData> for BlockHeader seems like an abuse of deref coercions to me
  • this appears to be so that the hash for the BlockHeaderData can be cached

block/equihash.rs

• Equihash verification

constants.rs

• contains various constants – do these all need to be public? is a meaningful distinction between public/private constants possible?

jubjub/mod.rs

• enum Unknown {}, enum PrimeOrder {}, enum FixedGenerators { ... }
  • these seem to be used as type parameter annotations
  • unclear why Unknown and PrimeOrder are enums rather than structs, which is how I've seen this before – maybe for consistency with FixedGenerators?
• ToUniform – generic hash-to-element interface?
• JubJubEngine - extension to the pairing Engine trait adding the embedded curve functionality

jubjub/edwards.rs

• contains a software (not circuit) implementation of JubJub in Edwards form
• subgroup information is maintained using PhantomData

jubjub/montgomery.rs

• contains a software (not circuit) implementation of JubJub in Montgomery form
• subgroup information is maintained using PhantomData

jubjub/fs.rs

• contains an implementation of the JubJub scalar field using the ff traits.

jubjub/tests.rs

• this doesn't seem to use Rust's testing tools in the normal way?
• there's test_suite<E: JubjubEngine> that calls each function in the test suite
• this is not invoked here, but in a #[test] in mod.rs, which doesn't seem to be cfg-gated?

group_hash.rs

• provides hash-to-group for JubJub
• is there a reason this isn't grouped with the rest of the jubjub code?

pedersen_hash.rs

• enum Personalization { NoteCommitment, MerkleTree(usize) }
• nit: pub fn Personalization::get_bits(&self) -> Vec<bool> could save an alloc by returning [bool; 6] and may not need to be pub?
• fn pedersen_hash performs the pedersen hash

primitives.rs

• unclear what this module means, since the whole crate is already primitives
• struct ValueCommitment represents the opening of a value commitment
• ValueCommitment::cm computes the commitment itself
• struct ProofGenerationKey
• struct ViewingKey
• ProofGenerationKey::to_viewing_key performs the conversion, but needs an &E::Params so it can't be a From impl -- is this avoidable?
• ViewingKey has functions rk, ivk, etc., which presumably correspond to the abbreviations in the spec, but there aren't doc comments if the abbreviation isn't already clear
• struct Diversifier
• struct PaymentAddress
• custom PartialEq implementation, but not Eq – could be replaced with derived impls?
• PaymentAddresses are constructed either from_parts or from_bytes
• a from_parts_unchecked function also exists for testing, but it's unclear why test code couldn't just unwrap or expect on from_parts
• struct Note
• same custom PartialEq implementation
• can compute note commitments, nullifiers
• possible opportunity to use bitvec here?

keys.rs

• pub fn prf_expand, pub fn prf_expand_vec seem to only ever be used here and in zip32.rs – do they need to be public?
• pub struct OutgoingViewingKey(pub [u8; 32])
• pub struct ExpandedSpendingKey has public fields ask, nsk, ovk
  • could these be encapsulated? looking at ripgrep results it's not clear.
• pub struct FullViewingKey has vk: ViewingKey and ovk: OutgoingViewingKey
  • is this naming also in the spec? it seems a bit strange that incoming viewing keys are vk but outgoing are ovk.
• ExpandedSpendingKey has from_spending_key(sk: &[u8]) but there doesn't appear to be a SpendingKey type -- is the byte slice just a serialized form?
• read and write methods for ExpandedSpendingKey serialize the whole esk, but all of the data is derivable from the sk above, so why serialize all the data?
• manual impl Clone for FullViewingKey that could probably be derived
• FullViewingKey has a from_expanded_spending_key method that takes an esk and a E::Params.
  • it's unfortunate that an explicit E::Params has to be passed around because it means that it's not possible to write all of these type conversions as From impls.
• it's not really clear what you can do with any of these key types, because they don't seem to have very many methods.

legacy.rs

• Comment says that it's for legacy transparent addresses and scripts.
• Defines a minimal subset of script opcodes required for t-addrs.
• pub struct Script(pub Vec<u8>) with read, write.
• impl Shl<OpCode> for Script is used to implement concatenation like a C++ iostream
  • std::ops::Shl is an arithmetic operation!
  • a better way would be impl FromIterator<Opcode> for Script
• pub enum TransparentAddress is either a PublicKey([u8; 20]) or a Script([u8; 20]).
• TransparentAddress::script() -> Script constructs a script for the addr.

merkle_tree.rs

• implementation of merkle tree for note commitments
• pub trait Hashable: Clone + Copy
  • read, write for serialization
  • combine returns the parent node of two trees
  • blank returns a blank leaf node
  • empty_root returns the empty root for the given depth
• no implementations of Hashable in this file
• only implementation in the crate is impl Hashable for Node in sapling.rs
• does this really need to be generic?
• struct PathFiller<Node: Hashable> is a wrapper for VecDeque<Node> that wraps pop_front with unwrap_or_else(|| Node::empty_root(depth))
  • empty instead of Default
• pub struct CommitmentTree<Node: Hashable> provides a Merkle tree of note commitments
  • nit: new() should be impl Default
  • read, write methods
  • is this encoding method used in consensus anywhere? per sean and str4d, no, it's only for internal use.
  • size computes the number of leaf nodes in the tree
  • append adds a Node by calling append_inner with SAPLING_COMMITMENT_TREE_DEPTH
  • root returns the tree root, calls root_inner with SAPLING_COMMITMENT_TREE_DEPTH and an empty PathFiller
• pub struct IncrementalWitness<Node: Hashable>
  • don't really understand what this does
  • used as a parameter to the transaction builder
• pub struct CommitmentTreeWitness<Node: Hashable>
  • "witness to a path from a position in a particular commitment tree to the root of that tree"
  • from_path constructor
  • from_slice constructor from its serialized form -- is this serialization consensus-critical? and where is the serialization defined?
  • looking for references to from_slice reveals its use in FFI, librustzcash_sapling_spend_proof.

note_encryption.rs

• defines constants for personalization and for data sizes
• pub struct Memo([u8; 512])
• impl Default for Memo constructs an empty memo field per ZIP302
• custom PartialEq implementation -- could be derived instead?
• Memo::to_utf8 parses UTF-8
• impl str::FromStr for Memo
• pub fn generate_esk(rng) -> Fs ?? shouldn't this be part of the ExtendedSpendingKey type, and why does it return a raw scalar?
• pub fn sapling_ka_agree, fn kdf_sapling do key agreement and kdf for note encryption (protocol §5.4.4.3–4)
• fn prf_ock does the sapling prf (protocol §5.4.2)
• pub struct SaplingNoteEncryption is supposed to provide a safe API for encrypting Sapling notes.
  • new constructor takes an OutgoingViewingKey, a Note<Bls12>, a PaymentAddress<Bls12>, a Memo, and an RNG.
  • seems unfortunate that the Bls12 parameter appears in Note and PaymentAddress...
  • esk and epk getters
  • encrypt_note_plaintext creates an encCiphertext as [u8; ENC_CIPHERTEXT_SIZE]
  • encrypt_outgoing_plaintext creates an outCiphertext as [u8; OUT_CIPHERTEXT_SIZE]
  • both of these take a &self
  • if the API is supposed to encapsulate note encryption, it might be nicer to have a builder API that constructs the note encryption task state and then a finalize method that consumes the intermediate state and produces the outputs.
• fn parse_note_plaintext_without_memo computes an Option<(Note<Bls12>, PaymentAddress<Bls12>)>
  • would be nice to have finer types on inputs
• pub fn try_sapling_note_decryption does "Trial decryption of the full note plaintext by the recipient. Attempts to decrypt and validate the given enc_ciphertext using the given ivk."
  • returns Option<(Note<Bls12>, PaymentAddress<Bls12>, Memo)>
  • corresponds to protocol §4.17.2
  • function takes four parameters, ivk, epk, cmu, enc_ciphertext
  • what do epk, cmu do?
  • can this decryption state be encapsulated so that the API looks like IncomingViewingKey::trial_decrypt(ciphertext)?
• pub fn try_sapling_compact_note_decryption seems similar but skips the memo field; corresponds to ZIP307
• pub fn try_sapling_output_recovery does protocol spec §4.17.3
  • returns Option<(Note<Bls12>, PaymentAddress<Bls12>, Memo)>
  • unclear from the types how this is different from try_sapling_note_decryption?
  • this also has many untyped fields, is it possible to encapsulate its input state somehow?

[dconnolly]

Look here re: #34

[dconnolly]

• We want to ID common areas in librustzcash, export those into crates, and have zebra and zcashd/librustzcash depend on them

[str4d]

Now that this issue has been closed, can I assume the review above is "complete"? If so, I'll go through it next week and open librustzcash issues for anything that we definitely want to address.

[hdevalence]

I'm not sure I'd say it was "complete" so much as "abandoned", in the sense that the parts that there are notes on are completed, but the parts that there aren't notes on have not been completed, and because of other priorities the uncompleted parts probably won't be finished in the near term.