Refactor Sapling data and use it in V4 #1946
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
oxarbitrage
commented
Mar 25, 2021
- move ShieldedData to sapling - add AnchorVariant - rename shielded_data to sapling_shielded data in V4 - move value_balance into ShieldedData - use temporal default PerSpendAnchor for ShieldedData - update prop tests for new structure
- manually implement PartialEq - use temporal default PerSpendAnchor for Spend - make anchor types available from sapling crate
- remove default anchor for ShieldedData - update serialize - update v5 prop test strategy
dconnolly
added
A-rust
oxarbitrage
commented
Mar 25, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
reviewed
Mar 26, 2021
teor2345
suggested changes
Mar 26, 2021
There was a problem hiding this comment.
I would like to review this change in two separate PRs:
- Changes to sapling_shielded_data in V4
- Adding sapling_shielded_data to V5
That way, we can get the V4 changes merged first.
Please also post screenshots of the spec when serialization or deserialization doesn't work. We need to base our code on the spec, not our tests.
oxarbitrage
changed the title
oxarbitrage
changed the title
teor2345
reviewed
Mar 31, 2021
There was a problem hiding this comment.
Here are the documentation and refactor changes I made:
- Rename anchor to per_spend_anchor
- Use nullifiers function directly in non-finalized state
- Use self.value_balance instead of passing it as an argument
- Derive Spend PartialEq rather than implementing it manually
- Derive Copy for tag types
- Add doc comments for ShieldedData refactor
Here are the bug fixes and tests I added:
- Add missing fields to ShieldedData PartialEq
- Add shielded data roundtrip and PartialEq proptests
Here are the features I added:
- Implement a per-spend anchor compatibility iterator
teor2345
approved these changes
Mar 31, 2021
dconnolly
reviewed
Apr 1, 2021
| if let Some(shielded_data) = shielded_data { | ||
| for sapling::Spend { nullifier, .. } in shielded_data.spends() { | ||
| for nullifier in shielded_data.nullifiers() { |
dconnolly
reviewed
Apr 1, 2021
Comment on lines
+497
to
+499
| } => match sapling_shielded_data { | ||
| Some(s) => s.value_balance, | ||
| None => Amount::try_from(0).unwrap(), |
dconnolly
reviewed
Apr 1, 2021
| @@ -439,10 +439,12 @@ impl<'a> SigHasher<'a> { | |||
| .personal(ZCASH_SHIELDED_SPENDS_HASH_PERSONALIZATION) | |||
| .to_state(); | |||
|
|
|||
| // TODO: make a generic wrapper in `spends.rs` that does this serialization | |||
dconnolly
reviewed
Apr 1, 2021
Comment on lines
+11
to
+31
| proptest! { | ||
| #[test] | ||
| fn shielded_data_roundtrip(shielded in any::<ShieldedData<PerSpendAnchor>>()) { | ||
| zebra_test::init(); | ||
|
|
||
| // shielded data doesn't serialize by itself, so we have to stick it in | ||
| // a transaction | ||
| let tx = Transaction::V4 { | ||
| inputs: Vec::new(), | ||
| outputs: Vec::new(), | ||
| lock_time: LockTime::min_lock_time(), | ||
| expiry_height: block::Height(0), | ||
| joinsplit_data: None, | ||
| sapling_shielded_data: Some(shielded), | ||
| }; | ||
|
|
||
| let data = tx.zcash_serialize_to_vec().expect("tx should serialize"); | ||
| let tx_parsed = data.zcash_deserialize_into().expect("randomized tx should deserialize"); | ||
|
|
||
| prop_assert_eq![tx, tx_parsed]; | ||
| } |
dconnolly
reviewed
Apr 1, 2021
Comment on lines
+33
to
+67
| /// Check that ShieldedData serialization is equal if `shielded1 == shielded2` | ||
| #[test] | ||
| fn shielded_data_serialize_eq(shielded1 in any::<ShieldedData<PerSpendAnchor>>(), shielded2 in any::<ShieldedData<PerSpendAnchor>>()) { | ||
| zebra_test::init(); | ||
|
|
||
| let shielded_eq = shielded1 == shielded2; | ||
|
|
||
| // shielded data doesn't serialize by itself, so we have to stick it in | ||
| // a transaction | ||
| let tx1 = Transaction::V4 { | ||
| inputs: Vec::new(), | ||
| outputs: Vec::new(), | ||
| lock_time: LockTime::min_lock_time(), | ||
| expiry_height: block::Height(0), | ||
| joinsplit_data: None, | ||
| sapling_shielded_data: Some(shielded1), | ||
| }; | ||
| let tx2 = Transaction::V4 { | ||
| inputs: Vec::new(), | ||
| outputs: Vec::new(), | ||
| lock_time: LockTime::min_lock_time(), | ||
| expiry_height: block::Height(0), | ||
| joinsplit_data: None, | ||
| sapling_shielded_data: Some(shielded2), | ||
| }; | ||
|
|
||
| let data1 = tx1.zcash_serialize_to_vec().expect("tx1 should serialize"); | ||
| let data2 = tx2.zcash_serialize_to_vec().expect("tx2 should serialize"); | ||
|
|
||
| if shielded_eq { | ||
| prop_assert_eq![data1, data2]; | ||
| } else { | ||
| prop_assert_ne![data1, data2]; | ||
| } | ||
| } |
dconnolly
reviewed
Apr 1, 2021
| self.binding_sig == other.binding_sig | ||
| // Now check that all the fields match | ||
| self.value_balance == other.value_balance | ||
| && self.shared_anchor == other.shared_anchor |
dconnolly
reviewed
Apr 1, 2021
Comment on lines
+121
to
+132
| /// Returns `Spend<PerSpendAnchor>` regardless of the underlying transaction | ||
| /// version, to allow generic verification over V4 and V5 transactions. | ||
| /// | ||
| /// # Correctness | ||
| /// | ||
| /// Do not use this function for serialization. | ||
| pub fn spends_per_anchor(&self) -> impl Iterator<Item = Spend<PerSpendAnchor>> + '_ { | ||
| self.spends() | ||
| .cloned() | ||
| .map(move |spend| Spend::<PerSpendAnchor>::from((spend, self.shared_anchor.clone()))) | ||
| } | ||
| } |
dconnolly
reviewed
Apr 1, 2021
| AnchorV: AnchorVariant + Clone, | ||
| { | ||
| /// The net value of Sapling spend transfers minus output transfers. | ||
| pub value_balance: Amount, |
dconnolly
reviewed
Apr 1, 2021
| /// The shared anchor for all `Spend`s in this transaction. | ||
| /// | ||
| /// A type of `()` means "not present in this transaction version". | ||
| pub shared_anchor: AnchorV::Shared, |
dconnolly
reviewed
Apr 1, 2021
| let joinsplit_data = OptV4Jsd::zcash_deserialize(&mut reader)?; | ||
|
|
||
| use futures::future::Either::*; | ||
| let shielded_data = if !shielded_spends.is_empty() { | ||
| Some(ShieldedData { | ||
| // Arbitraily use a spend for `first`, if both are present |
dconnolly
reviewed
Apr 1, 2021
Comment on lines
+294
to
+295
| // the spends are actually empty here, but we use the | ||
| // vec for consistency and readability |
This was referenced Apr 3, 2021
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
TODO
See the RFC for details: https://github.com/ZcashFoundation/zebra/blob/main/book/src/dev/rfcs/0010-v5-transaction.md#sapling-changes
Motivation
An important part of the transaction v5 design for Zebra is the support for sapling data in transactions V4 and V5.
I decided to split the design implementation as the Orchard parts depend on an open PR: #1885 while the refactoring needed to implement sapling data in both transaction as described by the design is independent.
Solution
Implement Sapling changes alone and use the new
AnchorVariantonly for V4 transactions.The code in this pull request has:
Review
This is a draft, still need a lot of work that i am aware of but i will like @teor2345 to take a look if possible before the end of their week.
@dconnolly can take a look as well if she haves the time.
Related Issues
Follow Up Work