Add split array serialization functions for Transaction::V5 #2017
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
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.
teor2345
added a commit
to oxarbitrage/zebra
that referenced
this pull request
Apr 15, 2021
dconnolly
reviewed
Apr 15, 2021
dconnolly
reviewed
Apr 15, 2021
dconnolly
reviewed
Apr 15, 2021
dconnolly
reviewed
Apr 15, 2021
| // | ||
| // we specifically want to serialize `Vec`s here, rather than generic slices | ||
| #[allow(clippy::ptr_arg)] | ||
| pub fn zcash_serialize_external_count<W: io::Write, T: ZcashSerialize>( |
There was a problem hiding this comment.
On its surface, this implementation seems agnostic to anything about external_counts, it seems to just invoke zcash_serialize on each item in the vec where supported. Maybe zcash_serialize_vec ?
There was a problem hiding this comment.
This function is specifically for arrays that don't store a count before the data.
Almost all arrays in Zcash have a count before the data, except for a 4 split arrays in Transaction::V5, and some of the filter network messages.
dconnolly
reviewed
Apr 15, 2021
| // | ||
| // we specifically want to serialize `Vec`s here, rather than generic slices | ||
| #[allow(clippy::ptr_arg)] | ||
| pub fn zcash_serialize_bytes_external_count<W: io::Write>( |
There was a problem hiding this comment.
Related to the above comment, this seems generic enough to be zcash_serialize_vec_bytes or something agnostic of external_count?
There was a problem hiding this comment.
It isn't generic, it is designed for a very specific and rare use case.
#2017 (comment)
dconnolly
reviewed
Apr 15, 2021
teor2345
added a commit
that referenced
this pull request
Apr 15, 2021
…nullifier utility functions (#1996) * add sapling shielded data to transaction V5 * implement nullifiers * test v5 in shielded_data_roundtrip * Explicitly design serialization for Transaction V5 Implement serialization for V4 and V5 spends and outputs, to make sure that the design works. * Test serialization for v5 spends and outputs Also add a few missing v4 tests. * Delete a disabled proptest * Make v5 transactions a top-level heading And add a missing serialized type. * Fix a comment typo * v5 transaction RFC: split array serialization Based on #2017 * RFC: explicitly describe serialized field order And link to the spec * RFC: add the shared anchor serialization rule test Co-authored-by: teor <teor@riseup.net>
teor2345
commented
Apr 15, 2021
teor2345
commented
Apr 15, 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.
Motivation
Transaction::V5splits some data into multiple arrays, with a single count before the first array.But Zebra expects every array to have a compactsize prefix when serializing and deserializing.
Solution
Refactor out serialization functions for typed arrays with external lengths:
Also refactor out deserialization for raw byte arrays with external lengths:
(
Transaction::V5doesn't use raw byte arrays.)Use some of the new functions in zebra-network.
Move some tests into a consistent module structure.
The code in this pull request:
Review
@oxarbitrage can review before he uses these functions to implement #1829.
Related Issues
#1996 redesign transaction serialization
#1829 parse sapling data in transaction v5
Follow Up Work
Use these functions to handle
V5transactions