PaymentAddress encapsulation

[str4d]

Closes #102.

[str4d]

Currently this PR encapsulates PaymentAddress when parsing from an encoded address, but does not prevent a user of the Rust API from creating an invalid PaymentAddress. The latter could go a couple of different directions:

• Provide a from_parts API that enforces pk_d being prime order (i.e. pk_d != 0), but allows invalid diversifiers. Make the internals accessible only via accessor methods.
• Same as the above, but also check for an invalid diversifier. Then we could maintain the invariant that diversifier is always valid, and functions like PaymentAddress::create_note could always succeed.

Thoughts? I could also postpone the above to a later PR, and merge the encoding changes as-is.

[str4d]

I took the first approach to encapsulation above (only enforcing pk_d validity as an invariant), as that was a smaller change to the overall codebase but requires almost all the same API changes to PaymentAddress. I figure we can enforce diversifier validity as an invariant in a subsequent PR, unless reviewers tell me otherwise 🙂

[daira]

utACK. Can we add a test for try_sapling_output_recovery that catches the case of zero pk_d that would previously have been (incorrectly) accepted?

[daira]

This can propagate the error by returning None, but that is correct and fixes a bug :-)

[daira]

Here I will add my regularly scheduled exhortation to rename the PrimeOrder marker to PrimeSubgroup (zcash/sapling-crypto#97). Here we actually have a point that is enforced to be prime order, and renaming the marker is still the right thing to do, because it's not the marker that is enforcing that.

Does not block.

[daira]

Similarly this should be renamed to as_prime_subgroup. Does not block.

[daira]

The spec indeed says that you should use a valid diversifier here (section 4.7.2 which refers to 4.2.2), so looping to find one is correct.

[str4d]

Added the test @daira requested. I checked that it fails if applied to current master.

[daira]

ACK 988b0d949dde359a4e9c013066df3d8280164122

[Eirik0]

utACK

[Eirik0]

What does this end up looking like on the cpp side? Previously we were passing in:

output.note.d.data(),
output.note.pk_d.begin(),

[str4d]

On the C++ side, those two fields are concatenated.

[str4d]

Rebased on master to fix two trivial merge conflicts with #110.

[codecov-io]

Codecov Report

Merging #109 into master will increase coverage by 0.04%.
The diff coverage is 77.27%.

@@            Coverage Diff             @@
##           master     #109      +/-   ##
==========================================
+ Coverage   56.17%   56.22%   +0.04%     
==========================================
  Files          43       43              
  Lines        6134     6120      -14     
==========================================
- Hits         3446     3441       -5     
+ Misses       2688     2679       -9

Impacted Files	Coverage Δ
librustzcash/src/rustzcash.rs	25.32% <0%> (+0.48%)	⬆️
zcash_proofs/src/sapling/prover.rs	0% <0%> (ø)	⬆️
librustzcash/src/tests/key_components.rs	100% <100%> (ø)	⬆️
librustzcash/src/tests/key_agreement.rs	100% <100%> (+3.84%)	⬆️
zcash_proofs/src/circuit/sapling.rs	96.01% <100%> (ø)	⬆️
zcash_client_backend/src/encoding.rs	81.48% <100%> (-3.27%)	⬇️

---

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 b19b40c...d6f6b50. Read the comment docs.