Redesign Sapling data model for V5 shared anchor and spends #2021

teor2345 · 2021-04-16T07:29:44Z

Motivation

In V5 transactions, the shared anchor is only present if there are any spends.
This requires an extra TransferData type to model correctly.

(This change was made after the NU5 spec audit.)

Solution

Update the design and tests to include:

a TransferData type which makes sure that the shared anchor is only present when there are spends
an AtLeastOne vector wrapper, which makes sure its inner vector always contains at least one element

As part of this change, delete the manual PartialEq impl and its tests, because we can derive PartialEq now.

The code in this pull request has:

Documentation Comments
Unit Tests and Property Tests

Review

Both @oxarbitrage and @dconnolly should have a look at the new design, these changes are pretty major.

Related Issues

#1829 implement V5 transaction for sapling
#2020 PR for V5 serialization for sapling

Follow Up Work

#2028 use AtLeastOne for other types, like JoinSplitData, Block.transaction, and some network messages

zebra-chain/src/sapling/shielded_data.rs

book/src/dev/rfcs/0010-v5-transaction.md

oxarbitrage · 2021-04-17T14:25:55Z

I think we should go ahead with this redesign. It is more clear than what we have now and matches better what we need to implement.

The shared anchor is only present if there are any spends. As part of this change, delete the manual PartialEq impl and its tests, because we can derive PartialEq now.

Interactive rename using the following commands: ```sh fastmod Spends SpendsAndMaybeOutputs fastmod NoSpends JustOutputs ```

This vector wrapper ensures that it always contains at least one element.

Also update the RFC to use AtLeastOne for Orchard.

teor2345 · 2021-04-19T02:43:59Z

I think we should go ahead with this redesign. It is more clear than what we have now and matches better what we need to implement.

What do you think of the AtLeastOne change?
https://github.com/ZcashFoundation/zebra/pull/2021/files#diff-a377e69ec608cdeb4b2c5bf8159152daf73ad6584e960882525828bf79bfdc96R174

dconnolly · 2021-04-20T03:10:34Z

book/src/dev/rfcs/0010-v5-transaction.md

@@ -128,11 +128,11 @@ struct FieldNotPresent;

 impl AnchorVariant for PerSpendAnchor {
    type Shared = FieldNotPresent;
-    type PerSpend = tree::Root;
+    type PerSpend = sapling::tree::Root;


dconnolly · 2021-04-20T03:10:41Z

book/src/dev/rfcs/0010-v5-transaction.md

 }

 impl AnchorVariant for SharedAnchor {
-    type Shared = tree::Root;
+    type Shared = sapling::tree::Root;


teor2345 · 2021-04-20T06:22:11Z

Everyone has read through this, so I'm going to merge it, so we can get on with the work that it's blocking.

dconnolly · 2021-04-20T13:31:35Z

book/src/dev/rfcs/0010-v5-transaction.md

+If there are no spends and no outputs:
+* in v4, the value_balance is fixed to zero
+* in v5, the value balance field is not present
+* in both versions, the binding_sig field is not present


dconnolly · 2021-04-20T13:44:18Z

book/src/dev/rfcs/0010-v5-transaction.md

+    /// If there are no spends, there must not be a shared
+    /// anchor.


dconnolly · 2021-04-20T13:44:27Z

book/src/dev/rfcs/0010-v5-transaction.md

+    /// In Transaction::V5, if there are any spends,
+    /// there must also be a shared spend anchor.


dconnolly · 2021-04-20T13:50:48Z

zebra-chain/src/sapling/shielded_data.rs

-    pub rest_outputs: Vec<Output>,
+    /// In V5 transactions, also contains a shared anchor, if there are any
+    /// spends.
+    pub transfers: TransferData<AnchorV>,


dconnolly · 2021-04-20T13:51:35Z

zebra-chain/src/sapling/shielded_data.rs

+        /// The anchor is the root of the Sapling note commitment tree in a previous
+        /// block. This root should be in the best chain for a transaction to be
+        /// mined, and it must be in the relevant chain for a transaction to be
+        /// valid.


dconnolly · 2021-04-20T13:52:43Z

zebra-chain/src/sapling/shielded_data.rs

+
+        /// Maybe some outputs (can be empty).
+        ///
+        /// Use the [`ShieldedData::outputs`] method to get an iterator over the


dconnolly · 2021-04-20T13:52:47Z

zebra-chain/src/sapling/shielded_data.rs

+
+        /// At least one spend.
+        ///
+        /// Use the [`ShieldedData::spends`] method to get an iterator over the


dconnolly · 2021-04-20T13:53:58Z

zebra-chain/src/sapling/shielded_data.rs

-        }
-        .into_iter()
-        .chain(self.rest_spends.iter())
+        self.transfers.spends()


dconnolly · 2021-04-20T13:54:05Z

zebra-chain/src/sapling/shielded_data.rs

-        }
-        .into_iter()
-        .chain(self.rest_outputs.iter())
+        self.transfers.outputs()


dconnolly · 2021-04-20T13:54:31Z

zebra-chain/src/sapling/shielded_data.rs

+    /// Provide the shared anchor for this transaction, if present.
+    ///
+    /// The shared anchor is only present if:
+    /// * there is at least one spend, and
+    /// * this is a `V5` transaction.
+    pub fn shared_anchor(&self) -> Option<AnchorV::Shared> {
+        self.transfers.shared_anchor()


dconnolly · 2021-04-20T13:56:30Z

zebra-chain/src/sapling/shielded_data.rs

 /// The Sapling `value_balance` field is optional in `Transaction::V5`, but
 /// required in `Transaction::V4`. In both cases, if there is no `ShieldedData`,
-/// then the field value must be zero. Therefore, only need to store
+/// then the field value must be zero. Therefore, we only need to store
 /// `value_balance` when there is some Sapling `ShieldedData`.
 ///
 /// In `Transaction::V4`, each `Spend` has its own anchor. In `Transaction::V5`,
-/// there is a single `shared_anchor` for the entire transaction. This
-/// structural difference is modeled using the `AnchorVariant` type trait.
-#[derive(Clone, Debug, Serialize, Deserialize)]
+/// there is a single `shared_anchor` for the entire transaction, which is only
+/// present when there is at least one spend. These structural differences are
+/// modeled using the `AnchorVariant` type trait and `TransferData` enum.


dconnolly · 2021-04-20T14:06:35Z

zebra-chain/src/sapling/shielded_data.rs

+        // this awkward construction avoids returning a newtype struct or
+        // type-erased boxed iterator


dconnolly · 2021-04-20T14:07:04Z

zebra-chain/src/sapling/shielded_data.rs

+
+    /// Iterate over the [`Output`]s for this transaction.
+    pub fn outputs(&self) -> impl Iterator<Item = &Output> {
+        use TransferData::*;


If we're importing this inside several methods, should we just import it at the top of the module?

So this is a bit of a weird one, because TransferData is an enum.

Sometimes you want the fully qualified name, if it's not obvious from the context.

dconnolly · 2021-04-20T14:07:25Z

zebra-chain/src/sapling/shielded_data.rs

+        match self {
+            SpendsAndMaybeOutputs { maybe_outputs, .. } => maybe_outputs,
+            JustOutputs { outputs, .. } => outputs.as_vec(),
        }
+        .iter()


Yeah it's much nicer to have an inner checked Vec<T> impl rather than (T, Vec<T>).

dconnolly · 2021-04-20T14:07:34Z

zebra-chain/src/sapling/shielded_data.rs

+        match self {
+            SpendsAndMaybeOutputs { shared_anchor, .. } => Some(shared_anchor.clone()),
+            JustOutputs { .. } => None,
+        }


dconnolly · 2021-04-20T14:08:00Z

zebra-chain/src/serialization/constraint.rs

+// CORRECTNESS
+//
+// All conversions to `AtLeastOne<T>` must go through `TryFrom<Vec<T>>`,
+// so that the type constraint is satisfied.


dconnolly · 2021-04-20T14:08:29Z

zebra-chain/src/serialization/zcash_deserialize.rs

+/// Deserialize an `AtLeastOne` vector, where the number of items is set by a
+/// compactsize prefix in the data. This is the most common format in Zcash.
+impl<T: ZcashDeserialize + TrustedPreallocate> ZcashDeserialize for AtLeastOne<T> {
+    fn zcash_deserialize<R: io::Read>(mut reader: R) -> Result<Self, SerializationError> {
+        let v: Vec<T> = (&mut reader).zcash_deserialize_into()?;
+        v.try_into()
+    }
+}


dconnolly · 2021-04-20T14:08:52Z

zebra-chain/src/serialization/zcash_serialize.rs

+/// Serialize an `AtLeastOne` vector as a compactsize number of items, then the
+/// items. This is the most common format in Zcash.
+impl<T: ZcashSerialize> ZcashSerialize for AtLeastOne<T> {
+    fn zcash_serialize<W: io::Write>(&self, mut writer: W) -> Result<(), io::Error> {
+        self.as_vec().zcash_serialize(&mut writer)
+    }
+}


dconnolly · 2021-04-20T14:10:22Z

zebra-chain/src/transaction/arbitrary.rs

+    type Parameters = ();
+
+    fn arbitrary_with(_args: Self::Parameters) -> Self::Strategy {
+        // TODO: add an extra spend or output using Either, and stop using filter_map


dconnolly · 2021-04-20T14:11:29Z

zebra-chain/src/transaction/joinsplit.rs

@@ -48,4 +48,10 @@ impl<P: ZkSnarkProof> JoinSplitData<P> {
    pub fn joinsplits(&self) -> impl Iterator<Item = &JoinSplit<P>> {
        std::iter::once(&self.first).chain(self.rest.iter())
    }
+
+    /// Iterate over the [`Nullifier`]s in `self`.
+    pub fn nullifiers(&self) -> impl Iterator<Item = &Nullifier> {


dconnolly · 2021-04-20T14:13:52Z

zebra-chain/src/transaction/serialize.rs

+        // `TransferData` ensures this field is only present when there is at
+        // least one spend.


dconnolly · 2021-04-20T14:14:12Z

zebra-chain/src/transaction/serialize.rs

+        let shared_anchor = if spends_count > 0 {
+            Some(reader.read_32_bytes()?.into())
+        } else {
+            None
+        };


dconnolly

✅

teor2345 added C-design Category: Software design work A-consensus Area: Consensus rule updates A-rust Area: Updates to Rust code C-enhancement Category: This is an improvement C-cleanup Category: This is a cleanup NU-5 Network Upgrade: NU5 specific tasks P-Medium labels Apr 16, 2021

teor2345 added this to the 2021 Sprint 7 milestone Apr 16, 2021

teor2345 requested review from dconnolly and oxarbitrage April 16, 2021 07:29

teor2345 self-assigned this Apr 16, 2021

teor2345 mentioned this pull request Apr 16, 2021

Implement Sapling serialization in Transaction V5 #2020

Merged

2 tasks

oxarbitrage mentioned this pull request Apr 16, 2021

fake_v5_round_trip test fails on blocks but not in transactions #2023

Closed

teor2345 commented Apr 17, 2021

View reviewed changes

zebra-chain/src/sapling/shielded_data.rs Outdated Show resolved Hide resolved

teor2345 commented Apr 17, 2021

View reviewed changes

book/src/dev/rfcs/0010-v5-transaction.md Outdated Show resolved Hide resolved

This was referenced Apr 18, 2021

Use the AtLeastOne generic type to enforce field counts #2028

Closed

Parse Sapling data in Transaction Version 5 #1829

Closed

teor2345 changed the base branch from issue1829_3 to main April 19, 2021 02:00

teor2345 added 5 commits April 19, 2021 12:02

Redesign Sapling data model for V5 shared anchor and spends

8f3360b

The shared anchor is only present if there are any spends. As part of this change, delete the manual PartialEq impl and its tests, because we can derive PartialEq now.

Stop creating a temporary Vec for the spend and output iterators

34314f7

Rename TransferData variants

36a1079

Interactive rename using the following commands: ```sh fastmod Spends SpendsAndMaybeOutputs fastmod NoSpends JustOutputs ```

Refactor out common sprout nullifier code

f037224

Implement the AtLeastOne constrained vector type

673b95d

This vector wrapper ensures that it always contains at least one element.

teor2345 force-pushed the redesign-shared-anchor-spend branch from dd6b0a3 to 673b95d Compare April 19, 2021 02:03

Simplify Sapling TransferData using AtLeastOne

cc8292f

Also update the RFC to use AtLeastOne for Orchard.

teor2345 force-pushed the redesign-shared-anchor-spend branch from 5536af6 to cc8292f Compare April 19, 2021 02:35

teor2345 changed the title ~~Redesign Sapling data model for V5 shared anchor and spends (do not merge)~~ Redesign Sapling data model for V5 shared anchor and spends Apr 19, 2021

teor2345 marked this pull request as ready for review April 19, 2021 02:42

dconnolly reviewed Apr 20, 2021

View reviewed changes

teor2345 merged commit 53779d2 into main Apr 20, 2021

teor2345 deleted the redesign-shared-anchor-spend branch April 20, 2021 06:22

dconnolly reviewed Apr 20, 2021

View reviewed changes

zebra-chain/src/sapling/shielded_data.rs

}

.into_iter()

.chain(self.rest_spends.iter())

self.transfers.spends()

Copy link

Contributor

dconnolly Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

dconnolly reviewed Apr 20, 2021

View reviewed changes

zebra-chain/src/sapling/shielded_data.rs

}

.into_iter()

.chain(self.rest_outputs.iter())

self.transfers.outputs()

Copy link

Contributor

dconnolly Apr 20, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍

dconnolly reviewed Apr 20, 2021

View reviewed changes

This was referenced Apr 20, 2021

zebra-chain: Port array-wrapper types to const generics #2042

Closed

Cleanup: move spends_per_anchor to TransferData #2046

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Redesign Sapling data model for V5 shared anchor and spends #2021

Redesign Sapling data model for V5 shared anchor and spends #2021

teor2345 commented Apr 16, 2021 •

edited

Loading

oxarbitrage commented Apr 17, 2021

teor2345 commented Apr 19, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

teor2345 commented Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

teor2345 Apr 20, 2021

dconnolly Apr 20, 2021

teor2345 Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly Apr 20, 2021

dconnolly left a comment

		/// If there are no spends, there must not be a shared
		/// anchor.

		/// In Transaction::V5, if there are any spends,
		/// there must also be a shared spend anchor.

		// this awkward construction avoids returning a newtype struct or
		// type-erased boxed iterator

		// `TransferData` ensures this field is only present when there is at
		// least one spend.

Redesign Sapling data model for V5 shared anchor and spends #2021

Redesign Sapling data model for V5 shared anchor and spends #2021

Conversation

teor2345 commented Apr 16, 2021 • edited Loading

Motivation

Solution

Review

Related Issues

Follow Up Work

oxarbitrage commented Apr 17, 2021

teor2345 commented Apr 19, 2021

Choose a reason for hiding this comment

Choose a reason for hiding this comment

teor2345 commented Apr 20, 2021

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

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

dconnolly left a comment

Choose a reason for hiding this comment

teor2345 commented Apr 16, 2021 •

edited

Loading