Transaction builder #92

str4d · 2019-07-16T08:03:04Z

Supports Sapling spends, Sapling outputs, and transparent (P2PKH and P2SH) outputs.

zcash_primitives/src/transaction/builder.rs

Eirik0 · 2019-07-24T22:17:46Z

When building I get a few warnings:

warning: variable does not need to be mutable
   --> sapling-crypto/src/jubjub/mod.rs:352:17
    |
352 |             for mut gen in tmp_params.pedersen_hash_generators.iter().cloned() {
    |                 ----^^^
    |                 |
    |                 help: remove this `mut`
    |
    = note: #[warn(unused_mut)] on by default

warning: variable does not need to be mutable
   --> sapling-crypto/src/jubjub/mod.rs:352:17
    |
352 |             for mut gen in tmp_params.pedersen_hash_generators.iter().cloned() {
    |                 ----^^^
    |                 |
    |                 help: remove this `mut`
    |
    = note: #[warn(unused_mut)] on by default

warning: variable does not need to be mutable
   --> sapling-crypto/src/circuit/uint32.rs:426:17
    |
426 |             let mut v = (0..32).map(|_| Boolean::constant(rng.gen())).collect::<Vec<_>>();
    |                 ----^
    |                 |
    |                 help: remove this `mut`

warning: variable does not need to be mutable
   --> sapling-crypto/src/circuit/uint32.rs:457:17
    |
457 |             let mut v = (0..32).map(|_| Boolean::constant(rng.gen())).collect::<Vec<_>>();
    |                 ----^
    |                 |
    |                 help: remove this `mut`

warning: variable does not need to be mutable
   --> sapling-crypto/src/circuit/pedersen_hash.rs:154:21
    |
154 |                 let mut input: Vec<bool> = (0..length).map(|_| rng.gen()).collect();
    |                     ----^^^^^
    |                     |
    |                     help: remove this `mut`

These may not be related to this PR, but are worth noting.

str4d · 2019-07-25T16:39:30Z

I think those warnings are due to NLL being activated on Rust 2015 edition in Rust 1.36.

Eirik0 · 2019-07-25T16:47:58Z

At least one of them shows up when building from master. I didn't spend too much time digging, but the NLL update makes sense. I don't mind if these are addressed as a separate issue.

str4d · 2019-07-25T21:40:03Z

I addressed @ebfull's comments by overhauling and extending the Amount struct. It is now used basically everywhere except inside Note, and enforces zatoshi range checks on instantiation.

ebfull

This looks great!

An implementation using local parameters is provided in the zcash_proofs crate.

Overrides the shift-left operator for pushing opcodes onto the Script, matching the notation used in zcashd.

This enables zcash_proofs to be compiled to WASM, which the directories crate doesn't support.

This helps to ensure type-safety of values that are required to satisfy zatoshi range bounds.

str4d · 2019-07-26T23:39:35Z

Rebased on master to fix merge conflicts caused by #91. I additionally had to modify various parts of this PR to account for the rand upgrade.

str4d · 2019-07-30T11:07:05Z

zcash_proofs/Cargo.toml

@@ -9,8 +9,13 @@ authors = [
 bellman = { path = "../bellman" }
 blake2b_simd = "0.5"
 byteorder = "1"
+directories = { version = "1", optional = true }


I added version 1 as a dependency instead of version 2, because version 1 has fewer transitive dependencies.

Is it still maintained?

Version 2 is actively maintained, but has a dependency on redox_users, which pulls in a bunch of extraneous crates we don't actually need. It's unclear to me whether version 1 is maintained.

str4d · 2019-07-30T11:10:28Z

zcash_primitives/Cargo.toml

@@ -15,6 +15,7 @@ fpe = "0.1"
 hex = "0.3"
 lazy_static = "1"
 pairing = { path = "../pairing" }
+rand = "0.7"


I added a dependency on rand separately from rand_core and rand_os because I wanted to access rand::seq::SliceRandom for shuffling the order of spends and outputs. I didn't bother replacing all the usages of rand_core and rand_os because it is unnecessary noise.

hdevalence · 2019-08-06T18:47:17Z

zcash_primitives/src/transaction/components/amount.rs

+pub const DEFAULT_FEE: Amount = Amount(10000);
+
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub struct Amount(i64);


It would be nice if this had a doc-comment explaining the purpose of the newtype and explicitly declaring any invariants it maintains. For instance in from_i64 below, the constructor enforces some requirements on the input -- are those invariants that the type maintains, or just a requirement for that constructor?

The main reason for the newtype is type safety. The requirements in the constructor are somewhat tricky however. MAX_MONEY is not actually the maximum amount of money in the consensus rules due to rounding errors (and in fact that will decrease slightly with the Blossom NU). It is a consensus-critical validity check on visible values (transparent outputs, vpub_old, vpub_new, and valueBalance).

Now, we could have Amount maintain {-MAX_MONEY..MAX_MONEY} as an invariant, but what would this imply for the API? I'm not keen on burdening the API with error checks for every addition, given that the network consensus rules mean that any transaction created leveraging out-of-range values will be unmineable. But is a panic!() a reasonable way to handle this case? Wrapping definitely isn't (and is partly the motivation for checking MAX_MONEY in the constructor, so we know that any values instantiated are far below the u64 upper end, and thus can assume there will be no overflow during transaction building).

I'll add a doc-comment in any case.

I think we should panic if addition or subtraction overflows the range -MAX_MONEY..MAX_MONEY.

hdevalence · 2019-08-06T19:00:11Z

zcash_primitives/src/transaction/components/amount.rs

+    /// Creates an Amount from an i64.
+    ///
+    /// Returns an error if the amount is out of range.
+    pub fn from_i64(amount: i64, allow_negative: bool) -> Result<Self, ()> {


This is a minor nit, but I think that having an allow_negative: bool parameter isn't the best API design. The reason is that when reading caller code, all that appears is

let amt = Amount::from_i64(val, true)?;

or similar, and the reader has to remember that the true means that negative values are allowed, rather than true meaning that the value is required to be positive (and therefore negative values are disallowed), which would be an alternate way to construct the API.

There are two alternatives that I think would improve readability in the long run:

(my preference) Split this code into two constructors whose names (strawman proposal) indicate their behaviour:

pub fn from_i64_nonnegative(amount: i64) -> Result<Self, ()>; pub fn from_i64_allow_negative(amount: i64) -> Result<Self, ()>;

Add an enum AmountValidation { NonNegative, NegativeAllowed } and replace the bool with an AmountValidation, so that caller code displays the validation criteria at the callsite:

let amt = Amount::from_i64(val, AmountValidation::NegativeAllowed)?;

This is an excellent point. I also prefer the explicit constructors.

hdevalence · 2019-08-06T19:01:50Z

zcash_primitives/src/transaction/components/amount.rs

+    type Output = Amount;
+
+    fn add(self, rhs: Amount) -> Amount {
+        Amount(self.0 + rhs.0)


Related to the invariant comment, what happens if I create two Amounts with MAX_MONEY - 1 and then add them? Is that a concern for this struct or is that the user's responsibility?

IMHO that's the user's responsibility, from the perspective of this struct. The Amounts are either being constructed from visible fields in a transaction (in which case the consensus rules will reject the result, because at least one field contain the sum that is above MAX_MONEY, or they are being constructed from/for Notes (in which case we are relying on the shielded value balance enforced by the Pedersen commitments, and the commitments themselves being enforced by the circuits). Per my reply to the invariant comment, we could panic!() here.

This is more intuitive than a boolean flag for handling non-negative Amounts stored in i64 values.

str4d · 2019-08-08T08:26:42Z

Pushed commits to address most of @hdevalence's comments. I'm still undecided on whether to enforce MAX_MONEY inside addition etc. with panic!().

Eirik0

I haven't finished going through this, but I had some suggestions regarding Error in builder.rs and wanted to get those posted.

Eirik0 · 2019-08-06T22:18:54Z

zcash_primitives/src/transaction/builder.rs

+    BindingSig,
+    ChangeIsNegative(i64),
+    InvalidAddress,
+    InvalidAmount,


Some of these errors could include additional information, for example InvalidAmount could include the amount. This can be added later.

Eirik0 · 2019-08-08T23:06:08Z

zcash_primitives/src/transaction/builder.rs

+};
+
+const DEFAULT_FEE: Amount = Amount(10000);
+const DEFAULT_TX_EXPIRY_DELTA: u32 = 20;


Note: This will need to be updated to account for Blossom.

zcash_primitives/src/transaction/builder.rs

Eirik0 · 2019-08-09T17:36:54Z

zcash_primitives/src/transaction/components/amount.rs

+    /// Creates an Amount from an i64.
+    ///
+    /// Returns an error if the amount is outside the range `{-MAX_MONEY..MAX_MONEY}`.
+    pub fn from_i64(amount: i64) -> Result<Self, ()> {


Would it make sense to change the return type here and for similar functions from Result<Self, ()> to Option<Self>?

I think I prefer this being an explicit (if unit) error. I know we are inconsistent about this in our API; we should open an issue for going through and refactoring everything to be internally consistent in how we decided to return errors vs None.

It would be great to go through and make these consistent, but I am happy leaving this for now.

Requires impl PartialEq for Transaction, which is implemented as a TxId comparison (relying on the invariant that Transaction is immutable).

Eirik0

utACK. In addition to reviewing this, I went through the cpp code and compared what I could. Everything looks great!

daira

ut(ACK+cov) modulo minor comments.

daira · 2019-08-13T21:12:08Z

librustzcash/src/rustzcash.rs

@@ -704,6 +705,11 @@ pub extern "system" fn librustzcash_sapling_final_check(
    binding_sig: *const [c_uchar; 64],
    sighash_value: *const [c_uchar; 32],
 ) -> bool {
+    let value_balance = match Amount::from_i64(value_balance) {
+        Ok(vb) => vb,
+        Err(()) => return false,


Please file a ticket about better error reporting.

Opened #101.

zcash_primitives/src/legacy.rs

daira · 2019-08-13T21:15:53Z

zcash_primitives/src/legacy.rs

+    }
+}
+
+impl Shl<OpCode> for Script {


sucks air through teeth at repeating this C++ antipattern

Why not just use a method?

In order to support concatenating different types with the same builder method (instead of a custom method per type), I would need to implement my own trait, which would result in more code than this. I also felt there was a benefit in having this legacy part be as close to the C++ code as possible for reviewability.

daira · 2019-08-13T21:37:47Z

zcash_primitives/src/transaction/builder.rs

+        to: PaymentAddress<Bls12>,
+        value: Amount,
+        memo: Option<Memo>,
+    ) -> Result<Self, Error> {


This does not seem to enforce (neither does PaymentAddress) that pk_d is not the zero point. zcash/zcash#3827

Opened #102.

daira · 2019-08-13T22:11:12Z

zcash_primitives/src/transaction/builder.rs

+                    let pk_d = {
+                        let dummy_ivk = Fs::random(&mut self.rng);
+                        g_d.mul(dummy_ivk, &JUBJUB)
+                    };


The protocol spec says: "As in Sprout, a dummy Sapling output note is constructed as normal but with zero value, and sent to a random shielded payment address." This looks correct.

daira · 2019-08-13T22:40:18Z

zcash_primitives/src/transaction/builder.rs

+        if let Some(anchor) = self.anchor {
+            let witness_root: Fr = witness.root().into();
+            if witness_root != anchor {
+                return Err(Error::AnchorMismatch);


This error condition is not tested.

daira · 2019-08-13T22:41:23Z

zcash_primitives/src/transaction/builder.rs

+    ) -> Result<Self, Error> {
+        let g_d = match to.g_d(&JUBJUB) {
+            Some(g_d) => g_d,
+            None => return Err(Error::InvalidAddress),


This error condition is not tested.

daira · 2019-08-13T22:42:24Z

zcash_primitives/src/transaction/builder.rs

+        } else {
+            self.anchor = Some(witness.root().into())
+        }
+        let witness = witness.path().ok_or(Error::InvalidWitness)?;


This error condition is not tested.

daira · 2019-08-13T22:46:39Z

zcash_primitives/src/transaction/builder.rs

+        self.mtx.binding_sig = Some(
+            prover
+                .binding_sig(&mut ctx, self.mtx.value_balance, &sighash)
+                .map_err(|()| Error::BindingSig)?,


I am satisfied that this cannot occur for the local TxProver. We do effectively test it for the mock TxProver (which does not generate a valid binding signature).

daira · 2019-08-13T23:02:05Z

zcash_primitives/src/transaction/components/amount.rs

+pub const DEFAULT_FEE: Amount = Amount(10000);
+
+#[derive(Clone, Copy, Debug, PartialEq)]
+pub struct Amount(i64);


I think we should panic if addition or subtraction overflows the range -MAX_MONEY..MAX_MONEY.

Co-Authored-By: Daira Hopwood <[email protected]>

str4d requested review from ebfull and Eirik0 July 16, 2019 08:03

str4d force-pushed the transaction-builder branch from 700326d to c5f92f8 Compare July 18, 2019 22:57

ebfull requested changes Jul 22, 2019

View reviewed changes

ebfull approved these changes Jul 26, 2019

View reviewed changes

str4d added 13 commits July 26, 2019 23:56

TxProver trait to abstract over the circuit parameters

0161803

An implementation using local parameters is provided in the zcash_proofs crate.

Sapling transaction builder

1862354

Script opcode and data support

dab3c00

Overrides the shift-left operator for pushing opcodes onto the Script, matching the notation used in zcashd.

TransparentAddress struct for P2PKH and P2SH

c26188a

transaction::Builder::add_transparent_output()

a1cd9df

Place zcash_proofs::prover::LocalTxProver behind a feature flag

54ef63b

This enables zcash_proofs to be compiled to WASM, which the directories crate doesn't support.

Pass tx builder RNG to SaplingNoteEncryption

4b61120

Allow transaction::Builder RNG to be configured

532299d

Pass tx builder RNG to spend_sig()

17f6bbc

Move Amount impl into a submodule

fa50d55

Amount::{zero, is_positive, is_negative}

0ea4408

impl operators for Amount

ab60b88

Make Amount opaque, and use it more

59ed258

This helps to ensure type-safety of values that are required to satisfy zatoshi range bounds.

str4d force-pushed the transaction-builder branch from ecb4279 to 59ed258 Compare July 26, 2019 23:37

ebfull approved these changes Jul 30, 2019

View reviewed changes

str4d requested a review from daira July 30, 2019 08:35

str4d commented Jul 30, 2019

View reviewed changes

ebfull mentioned this pull request Aug 6, 2019

Upgrade dependencies #94

Merged

hdevalence reviewed Aug 6, 2019

View reviewed changes

str4d added 2 commits August 8, 2019 00:55

Separate Amount::{from_i64, from_nonnegative_i64} APIs

7c07914

This is more intuitive than a boolean flag for handling non-negative Amounts stored in i64 values.

Improve documentation for Amount

e666321

Eirik0 suggested changes Aug 9, 2019

View reviewed changes

Eirik0 reviewed Aug 9, 2019

View reviewed changes

str4d added 2 commits August 13, 2019 15:10

Simplify structure of transaction builder errors

3a73f94

Simplify transaction builder tests

1760b27

Requires impl PartialEq for Transaction, which is implemented as a TxId comparison (relying on the invariant that Transaction is immutable).

Eirik0 approved these changes Aug 13, 2019

View reviewed changes

ebfull approved these changes Aug 13, 2019

View reviewed changes

daira mentioned this pull request Aug 13, 2019

[Sapling] If no change is given, this leaks that the output value is the same value as a previous output note zcash/zcash#3606

Open

daira approved these changes Aug 13, 2019

View reviewed changes

Panic if Amount addition or subtraction overflows

a28d94f

This was referenced Aug 13, 2019

Unify error handling strategy across the Rust APIs #100

Closed

Expose errors across the librustzcash FFI #101

Closed

Encapsulate PaymentAddress correctly #102

Closed

Test the full set of transaction builder error conditions #103

Open

Comment that we support a minimal set of script opcodes

4a6c9ec

Co-Authored-By: Daira Hopwood <[email protected]>

Eirik0 approved these changes Aug 13, 2019

View reviewed changes

str4d merged commit 52ea437 into zcash:master Aug 14, 2019

str4d deleted the transaction-builder branch August 14, 2019 00:11

str4d added this to the v0.1.0 milestone Aug 22, 2019

Transaction builder #92

Transaction builder #92

Conversation

str4d commented Jul 16, 2019

Eirik0 commented Jul 24, 2019

str4d commented Jul 25, 2019

Eirik0 commented Jul 25, 2019

str4d commented Jul 25, 2019

ebfull left a comment

Choose a reason for hiding this comment

str4d commented Jul 26, 2019

str4d Jul 30, 2019 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d commented Aug 8, 2019

Eirik0 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Eirik0 left a comment

Choose a reason for hiding this comment

daira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

str4d Jul 30, 2019 •

edited

Loading