Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Update Transaction definition. #105

Merged
merged 16 commits into from Dec 5, 2019
Merged

Update Transaction definition. #105

merged 16 commits into from Dec 5, 2019

Conversation

@hdevalence
Copy link
Member

hdevalence commented Nov 18, 2019

Progress towards #13.

This creates a Transaction enum over all transaction versions, with the goal of making (structurally) invalid Zcash transactions unrepresentable. Of course, it's still possible to make one which is semantically invalid, with e.g., invalid proofs, or contextually invalid, e.g., double-spending a note, but it should not be possible to construct a Transaction which does not have a valid encoding (e.g., a version 1 transaction with Sapling proofs in it, or some other structurally invalid combination. In addition to being a better design, this is also important for us, as we want to be able to construct transaction hashes by an infallible From impl.

To do this, the Transaction struct has one enum variant for each transaction encoding version, and the data is modeled (at times slightly differently from the description in the spec) so that each variant has only the appropriate fields.

Previously this comment said it closes #13; it doesn't, and there is a bunch of work left to do, but that work could be split out into separate, further tickets:

  1. Many of the types in the structures that make up a Transaction need to be refined to assign them semantic meaning. The complete list can be found by running rg 'XXX refine' -A 2 in zebra/zebra-chain/src. Fleshing out many of these types will require further work – for instance, the Zcash-flavored Ed25519 pubkey in the JoinSplit data requires #109. (This item is now #123).

  2. The actual transaction encoding and decoding is not implemented, although there is a stub module in which it will live (transaction/serialization.rs). Implementing ZcashSerialize and ZcashDeserialize is a further chunk of work, but because all of the unrefined types are in place, it can be done independently of (1) by, e.g., parsing something as a [u8; 32] instead of some refined PublicKey type. (This item is now #124).

  3. The as-yet-unimplemented encoding and decoding is not tested. In addition to the transaction test vectors, we could define a proptest strategy to generate random transaction data to check that the encodings round-trip. (This item is now #124).

zebra-chain/src/transaction.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction.rs Outdated Show resolved Hide resolved
zebra-chain/src/transaction.rs Outdated Show resolved Hide resolved
@dconnolly

This comment has been minimized.

Copy link
Member

dconnolly commented Nov 21, 2019

The build job is having a hard time
image

@dconnolly dconnolly added this to In progress in 🦓 via automation Nov 21, 2019
dconnolly and others added 8 commits Nov 14, 2019
This could alternately use bytes::Bytes to save some allocations
but I don't think this is important to get perfectly now.  In the future, we
will want to have all of the script handling code in the zebra-script crate,
but we will need to have a container type for an encoded script in zebra-chain,
because otherwise zebra-chain would depend on zebra-script and not the other
way around.
These are only *transparent* inputs and outputs, so by putting Transparent in
the name (instead of Transaction) it's more clear that a transaction's inputs
or outputs can also be shielded.
This attempts to map the versioning and field presence rules into an ADT, so
that structurally invalid transactions (e.g., a BCTV14 proof in a Sapling
transaction) are unrepresentable.
Co-Authored-By: Daira Hopwood <daira@jacaranda.org>
hdevalence added 4 commits Nov 27, 2019
This has a lot of duplication and should really use generics to abstract over
Sprout-on-BCTV14 or Sprout-on-Groth16.
This also adds a trait to abstract over them.
@hdevalence hdevalence marked this pull request as ready for review Nov 28, 2019
@hdevalence

This comment has been minimized.

Copy link
Member Author

hdevalence commented Nov 28, 2019

Rewrote the description above.

@hdevalence hdevalence mentioned this pull request Dec 5, 2019
hdevalence added 3 commits Dec 5, 2019
This moves the TransactionHash changes along with the moved TransactionHash code.
🦓 automation moved this from In progress to Reviewer approved Dec 5, 2019
Copy link
Member

dconnolly left a comment

LGTM!

@dconnolly dconnolly merged commit c013895 into main Dec 5, 2019
1 check passed
1 check passed
Google Cloud Build Google Cloud Build
Details
🦓 automation moved this from Reviewer approved to Done Dec 5, 2019
@dconnolly dconnolly deleted the tx branch Dec 5, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
🦓
  
Done
3 participants
You can’t perform that action at this time.