Parse shielded data in v5 transaction #90
Conversation
Bumps [bincode](https://github.com/servo/bincode) from 1.3.1 to 1.3.3. - [Release notes](https://github.com/servo/bincode/releases) - [Commits](bincode-org/bincode@v1.3.1...v1.3.3) Signed-off-by: dependabot[bot] <support@github.com>
Bumps [futures](https://github.com/rust-lang/futures-rs) from 0.3.13 to 0.3.14. - [Release notes](https://github.com/rust-lang/futures-rs/releases) - [Changelog](https://github.com/rust-lang/futures-rs/blob/master/CHANGELOG.md) - [Commits](rust-lang/futures-rs@0.3.13...0.3.14) Signed-off-by: dependabot[bot] <support@github.com>
Bumps [x25519-dalek](https://github.com/dalek-cryptography/x25519-dalek) from 1.1.0 to 1.1.1. - [Release notes](https://github.com/dalek-cryptography/x25519-dalek/releases) - [Changelog](https://github.com/dalek-cryptography/x25519-dalek/blob/main/CHANGELOG.md) - [Commits](dalek-cryptography/x25519-dalek@1.1.0...1.1.1) Signed-off-by: dependabot[bot] <support@github.com>
Bumps [color-eyre](https://github.com/yaahc/color-eyre) from 0.5.10 to 0.5.11. - [Release notes](https://github.com/yaahc/color-eyre/releases) - [Changelog](https://github.com/yaahc/color-eyre/blob/v0.5.11/CHANGELOG.md) - [Commits](eyre-rs/color-eyre@v0.5.10...v0.5.11) Signed-off-by: dependabot[bot] <support@github.com>
| // shielded data doesn't serialize by itself, so we have to stick it in | ||
| // a transaction | ||
|
|
||
| // stick `SharedAnchor` shielded data into a v4 transaction | ||
| let tx = Transaction::V5 { | ||
| inputs: Vec::new(), | ||
| outputs: Vec::new(), | ||
| lock_time: LockTime::min_lock_time(), | ||
| expiry_height: block::Height(0), | ||
| sapling_shielded_data: Some(shielded_v5), | ||
| rest: Vec::new(), | ||
| }; |
There was a problem hiding this comment.
v5 shielded data should have a serialize implementation, because all its data is continuous.
| // shielded data doesn't serialize by itself, so we have to stick it in | ||
| // a transaction | ||
| let tx1 = Transaction::V5 { | ||
| inputs: Vec::new(), | ||
| outputs: Vec::new(), | ||
| lock_time: LockTime::min_lock_time(), | ||
| expiry_height: block::Height(0), | ||
| sapling_shielded_data: Some(shielded1), | ||
| rest: Vec::new() | ||
| }; | ||
| let tx2 = Transaction::V5 { | ||
| inputs: Vec::new(), | ||
| outputs: Vec::new(), | ||
| lock_time: LockTime::min_lock_time(), | ||
| expiry_height: block::Height(0), | ||
| sapling_shielded_data: Some(shielded2), | ||
| rest: Vec::new() | ||
| }; |
There was a problem hiding this comment.
v5 shielded data should have a serialize implementation, because all its data is continuous.
There was a problem hiding this comment.
Looks like a good start, thanks for doing the tests.
Here's what we need to change to match the spec:
- serialize and deserialize the v5 sapling fields in the correct order
Here's what we need to change to match the design:
- implement serialize and deserialize on
Option<sapling::ShieldedData<SharedAnchor>>, and call that code fromTransaction::V5serialization
| // shielded data doesn't serialize by itself, so we have to stick it in | ||
| // a transaction | ||
| let tx1 = Transaction::V5 { | ||
| inputs: Vec::new(), | ||
| outputs: Vec::new(), | ||
| lock_time: LockTime::min_lock_time(), | ||
| expiry_height: block::Height(0), | ||
| sapling_shielded_data: Some(shielded1), | ||
| rest: Vec::new(), | ||
| }; | ||
| let tx2 = Transaction::V5 { | ||
| inputs: Vec::new(), | ||
| outputs: Vec::new(), | ||
| lock_time: LockTime::min_lock_time(), | ||
| expiry_height: block::Height(0), | ||
| sapling_shielded_data: Some(shielded2), | ||
| rest: Vec::new(), | ||
| }; |
There was a problem hiding this comment.
v5 shielded data should have a serialize implementation, because all its data is continuous.
| // shielded data doesn't serialize by itself, so we have to stick it in | ||
| // a transaction | ||
| let tx1 = Transaction::V5 { | ||
| inputs: Vec::new(), | ||
| outputs: Vec::new(), | ||
| lock_time: LockTime::min_lock_time(), | ||
| expiry_height: block::Height(0), | ||
| sapling_shielded_data: Some(shielded1), | ||
| rest: Vec::new(), | ||
| }; | ||
| let tx2 = Transaction::V5 { | ||
| inputs: Vec::new(), | ||
| outputs: Vec::new(), | ||
| lock_time: LockTime::min_lock_time(), | ||
| expiry_height: block::Height(0), | ||
| sapling_shielded_data: Some(shielded2), | ||
| rest: Vec::new(), | ||
| }; |
There was a problem hiding this comment.
v5 shielded data should have a serialize implementation, because all its data is continuous.
| @@ -187,8 +188,7 @@ impl ZcashSerialize for Transaction { | |||
| None => {} | |||
| } | |||
| } | |||
| // TODO: serialize sapling shielded data according to the V5 transaction spec | |||
| #[allow(unused_variables)] | |||
|
|
|||
| Transaction::V5 { | |||
There was a problem hiding this comment.
| Transaction::V5 { | |
| // Serialize Transaction v5 according to the spec. | |
| // See the transaction v5 table in: | |
| // https://zips.z.cash/protocol/nu5.pdf#txnencodingandconsensus | |
| Transaction::V5 { |
| @@ -200,13 +200,42 @@ impl ZcashSerialize for Transaction { | |||
| // Write version 5 and set the fOverwintered bit. | |||
There was a problem hiding this comment.
Let's add some comments to help people follow the spec
| // Write version 5 and set the fOverwintered bit. | |
| // header: write version 5 and set the fOverwintered bit. |
| lock_time.zcash_serialize(&mut writer)?; | ||
| writer.write_u32::<LittleEndian>(expiry_height.0)?; | ||
| inputs.zcash_serialize(&mut writer)?; | ||
| outputs.zcash_serialize(&mut writer)?; |
There was a problem hiding this comment.
| lock_time.zcash_serialize(&mut writer)?; | |
| writer.write_u32::<LittleEndian>(expiry_height.0)?; | |
| inputs.zcash_serialize(&mut writer)?; | |
| outputs.zcash_serialize(&mut writer)?; | |
| // when can this transaction be mined? | |
| lock_time.zcash_serialize(&mut writer)?; | |
| writer.write_u32::<LittleEndian>(expiry_height.0)?; | |
| // transparent | |
| inputs.zcash_serialize(&mut writer)?; | |
| outputs.zcash_serialize(&mut writer)?; |
| @@ -335,7 +364,67 @@ impl ZcashDeserialize for Transaction { | |||
| let inputs = Vec::zcash_deserialize(&mut reader)?; | |||
There was a problem hiding this comment.
Please add comments to v5 deserialization like the v5 serialization comments:
https://github.com/oxarbitrage/zebra/pull/90/files#diff-ea762b004cd64770c3f4900c77d53620d0723dac1ab023443542de4f55e85398R192-R207
| lock_time.zcash_serialize(&mut writer)?; | ||
| writer.write_u32::<LittleEndian>(expiry_height.0)?; | ||
| inputs.zcash_serialize(&mut writer)?; | ||
| outputs.zcash_serialize(&mut writer)?; | ||
|
|
||
| // TODO: serialize sapling shielded data according to the V5 transaction spec | ||
| match sapling_shielded_data { |
There was a problem hiding this comment.
Please follow the field order in the v5 transaction spec.
Spends have 1 count (compactsize), but they are split into 3 separate arrays in 2 different locations.
Outputs have 1 count, but they are split into 2 separate arrays in 2 different locations.
There was a problem hiding this comment.
Also, we should move the serialize and deserialize implementations to Option<sapling::ShieldedData<SharedAnchor>>, to match the design:
https://github.com/ZcashFoundation/zebra/pull/1996/files#diff-a377e69ec608cdeb4b2c5bf8159152daf73ad6584e960882525828bf79bfdc96R277
So v5 sapling serialization will look like the Sprout JoinSplit serialization, where the code is in a separate function from v5 transaction serialization:
https://github.com/ZcashFoundation/zebra/blob/main/zebra-chain/src/transaction/serialize.rs#L34
https://github.com/ZcashFoundation/zebra/blob/main/zebra-chain/src/transaction/serialize.rs#L124
https://github.com/ZcashFoundation/zebra/blob/main/zebra-chain/src/transaction/serialize.rs#L247
Let's also serialize and deserialize Option<sapling::ShieldedData<SharedAnchor>> for consistency.
| @@ -335,7 +364,67 @@ impl ZcashDeserialize for Transaction { | |||
| let inputs = Vec::zcash_deserialize(&mut reader)?; | |||
| let outputs = Vec::zcash_deserialize(&mut reader)?; | |||
|
|
|||
There was a problem hiding this comment.
Please follow the field order in the v5 transaction spec.
In general, tests shouldn't depend on debug formatting. But for now, removing the ' fixes the issue.
In Transaction::V5, Zcash splits some types into multiple arrays, with a single prefix count before the first array. Add utility functions for serializing and deserializing the subsequent arrays, with a paramater for the original array's length.
And fix the test module structure so it is consistent with the rest of zebra-chain.
External size vec serialize
Motivation
We need to parse V5 shielded data: ZcashFoundation#1829
Solution
Implement serialize, deserialize and tests.
The code in this pull request has:
Review
@teor2345
Related Issues
ZcashFoundation#1829
Follow Up Work