Serialize/Deserialize sapling data in V5 #1860

oxarbitrage · 2021-03-06T20:35:23Z

Motivation

Parse sapling data in transaction v5: #1829

Solution

It is very similar to V4 but the order of the fields is different in the 2 versions. This complicates a bit the de-duplication efforts.

For the serialize a new type SigShieldedData was added and the ZcashSerialize implementation is done against this new type that haves the shielded data and a flag to control if the serializing of the binding_sig field will be done here or not. V4 needs to serialize the binding_sig later so the flag for it is false while in V5 everything can be done in the same place so this flag is true.

For deserialization the idea is similar but we cant implement a type as zcash_deserialize cant access the instance(no self is available). So by now i implemented this as a function.

The rest is pretty straightforward however i am not sure if this is the best approach, open to options.

Review

Related Issues

If merged this will close #1829

dconnolly · 2021-03-07T00:56:18Z

zebra-chain/src/transaction/serialize.rs

+struct SigShieldedData<'a> {
+    sig: bool,
+    shielded_data: &'a Option<ShieldedData>,
+}


transaction::ShieldedData includes binding_sig, for the Sapling data I'm not sure why this type is needed.

It is needed for the order of the fields inside a transaction. In V4 transactions we have the spends and outputs for sapling then the joinsplits and then the binding sig. This is explained in https://github.com/ZcashFoundation/zebra/blob/main/zebra-chain/src/transaction/serialize.rs#L147-L153 and can be verified by checking the fields of the V4 transaction in the spec.

In the other hand in the V5 transactions the binding sig is intermediately after the sapling spends and outputs so it can be serialized right there as there is nothing in the middle.

Please add a doc comment for this type, that explains why it's needed, and how it should be used.

Actually, it looks like we can't use this type, because ShieldedData is different in V4 and V5 transactions.

dconnolly · 2021-03-07T01:03:51Z

zebra-chain/src/transaction/serialize.rs

-                } else {
-                    None
-                };
+                let shielded_data =


Transaction::v4 isn't changing so I don't think we need to change this.

The idea is to use the same code in V4 and V5 as they are doing almost the same for sapling stuff(except for the order of the fields).

Even though they have the same names, the underlying types of these fields are different, so it might be best to just duplicate some code.

dconnolly · 2021-03-07T01:05:11Z

zebra-chain/src/transaction.rs

@@ -222,21 +226,26 @@ impl Transaction {
        // This function returns a boxed iterator because the different
        // transaction variants end up having different iterator types
        match self {
-            // JoinSplits with Groth Proofs
+            // Transactions with ShieldedData


teor2345 · 2021-03-07T22:40:33Z

zebra-chain/src/transaction.rs

+        /// The shielded data for this transaction, if any.
+        shielded_data: Option<ShieldedData>,
+        /// The net value of Sapling spend transfers minus output transfers.
+        value_balance: Amount,


v5 transactions also include the Orchard value balance.

So we'll need to rename shielded_data, ShieldedData, and value_balance to include sapling. We should make this change across V4 and V5 transactions, for consistency.

Actually, we'll need to make this change in this PR, because the v5 transaction format changes the fields inside ShieldedData. So we'll need to:

rename ShieldedData to SaplingV4ShieldedData

add SaplingV5ShieldedData

in a later PR, add OrchardActionData

@dconnolly what's a good name for the new SaplingV5ShieldedData type?
It doesn't have any proofs or signatures, just value commitments, nullifiers, keys, and ciphertext.

What about having ShieldedData::V4 and ShieldedData::V5 instead ?

What about having ShieldedData::V4 and ShieldedData::V5 instead ?

That's still easily confused with Orchard shielded data. (And a bit confusing with Sprout shielded data as well.)

teor2345 · 2021-03-07T22:44:39Z

zebra-chain/src/transaction/serialize.rs

+        let shielded_data = self.shielded_data;
+        match shielded_data {
+            None => {
+                // Signal no shielded spends and no shielded outputs.


Suggested change

// Signal no shielded spends and no shielded outputs.

// There are no shielded spends and no shielded outputs.

// Since all other fields are omitted, the serialization is the same in V4 and V5 transactions.

teor2345

The contents of the vSpendsSapling and vOutputsSapling fields has changed in V5 transactions: they contain much less data. So we won't be able to parse them using the existing ShieldedData type.

This isn't quite clear from the spec, because the field sizes are wrong. I've made some comments on the draft spec PR.

oxarbitrage · 2021-03-09T14:42:08Z

Blocked on #1863

teor2345 · 2021-03-17T01:09:23Z

zebra-chain/src/transaction/serialize.rs

+                let shielded_spends = Vec::zcash_deserialize(&mut reader)?;
+                let shielded_outputs = Vec::zcash_deserialize(&mut reader)?;
+                let shielded_data =
+                    deserialize_shielded_data(&mut reader, shielded_spends, shielded_outputs)?;
+                let sapling_value_balance = (&mut reader).zcash_deserialize_into()?;


If inputs and outputs are None, these fields do not exist in V5:
#1829 (comment)

I will add a test for this bug to the design in #1863.

After #1863 is completed, please revise all the code in this PR, based on the latest spec.

oxarbitrage · 2021-03-28T15:29:29Z

This PR was replaced by the already approved and merged design for V5 (#1886), an initial refactor based on the design and implementation in V4 (#1946) and a PR that will be submitted for V5.

parse sapling data in transaction v5

adba8d9

zfnd-bot bot assigned oxarbitrage Mar 6, 2021

add shielded data for V5 to sapling_nullifiers()

696affe

dconnolly reviewed Mar 7, 2021

View reviewed changes

teor2345 reviewed Mar 7, 2021

View reviewed changes

teor2345 suggested changes Mar 7, 2021

View reviewed changes

change value_balance to sapling_value_balance everywhere

a42cf36

oxarbitrage added NU-5 Network Upgrade: NU5 specific tasks S-blocked Status: Blocked on other tasks labels Mar 9, 2021

teor2345 reviewed Mar 17, 2021

View reviewed changes

teor2345 mentioned this pull request Mar 17, 2021

Check Transaction cardinality at parse time, to limit memory usage #1917

Closed

2 tasks

oxarbitrage closed this Mar 28, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Serialize/Deserialize sapling data in V5 #1860

Serialize/Deserialize sapling data in V5 #1860

oxarbitrage commented Mar 6, 2021

dconnolly Mar 7, 2021

oxarbitrage Mar 7, 2021

teor2345 Mar 7, 2021

teor2345 Mar 7, 2021

dconnolly Mar 7, 2021

oxarbitrage Mar 7, 2021

teor2345 Mar 7, 2021

dconnolly Mar 7, 2021

teor2345 Mar 7, 2021 •

edited

Loading

teor2345 Mar 7, 2021

oxarbitrage Mar 8, 2021

teor2345 Mar 8, 2021

teor2345 Mar 7, 2021 •

edited

Loading

teor2345 left a comment

oxarbitrage commented Mar 9, 2021

teor2345 Mar 17, 2021

oxarbitrage commented Mar 28, 2021

	// Signal no shielded spends and no shielded outputs.
	// There are no shielded spends and no shielded outputs.
	// Since all other fields are omitted, the serialization is the same in V4 and V5 transactions.

Serialize/Deserialize sapling data in V5 #1860

Serialize/Deserialize sapling data in V5 #1860

Conversation

oxarbitrage commented Mar 6, 2021

Motivation

Solution

Review

Related Issues

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

teor2345 Mar 7, 2021 • 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

teor2345 Mar 7, 2021 • edited Loading

Choose a reason for hiding this comment

teor2345 left a comment

Choose a reason for hiding this comment

oxarbitrage commented Mar 9, 2021

Choose a reason for hiding this comment

oxarbitrage commented Mar 28, 2021

teor2345 Mar 7, 2021 •

edited

Loading

teor2345 Mar 7, 2021 •

edited

Loading