Implementation for ZIP-222 Transparent Zcash Extensions #286

nuttycom · 2020-09-01T19:51:22Z

This implements the core data structures and transaction builder function for TZEs.

codecov · 2020-09-01T20:11:01Z

Codecov Report

Merging #286 into master will decrease coverage by 1.27%.
The diff coverage is 53.28%.

@@            Coverage Diff             @@
##           master     #286      +/-   ##
==========================================
- Coverage   68.17%   66.89%   -1.28%     
==========================================
  Files          59       62       +3     
  Lines        5963     6435     +472     
==========================================
+ Hits         4065     4305     +240     
- Misses       1898     2130     +232

Impacted Files	Coverage Δ
zcash_extensions/src/consensus/transparent.rs	`0.00% <0.00%> (ø)`
zcash_primitives/src/consensus.rs	`17.96% <0.00%> (-1.39%)`	⬇️
zcash_primitives/src/legacy.rs	`67.53% <ø> (+1.29%)`	⬆️
zcash_primitives/src/prover.rs	`33.33% <ø> (+2.56%)`	⬆️
zcash_primitives/src/redjubjub.rs	`87.12% <ø> (+0.25%)`	⬆️
...sh_primitives/src/transaction/components/amount.rs	`42.55% <ø> (ø)`
zcash_primitives/src/extensions/transparent.rs	`33.33% <33.33%> (ø)`
zcash_primitives/src/transaction/builder.rs	`57.93% <42.10%> (-1.24%)`	⬇️
zcash_primitives/src/transaction/mod.rs	`44.33% <54.76%> (-3.93%)`	⬇️
zcash_primitives/src/transaction/sighash.rs	`75.51% <56.09%> (-14.20%)`	⬇️
... and 36 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update d077872...eeb0c2b. Read the comment docs.

str4d

Looking good overall!

Cargo.toml

zcash_extensions/Cargo.toml

zcash_extensions/src/consensus/transparent.rs

zcash_extensions/src/transparent/demo.rs

Co-authored-by: Kris Nuttycombe <kris.nuttycombe@gmail.com>

…able. Fix demo test compilation errors.

This adds a `Future` variant for both BranchId and NetworkUpgrade.

Co-authored-by: str4d <thestr4d@gmail.com>

str4d

utACK

nuttycom · 2020-09-24T21:15:37Z

zcash_primitives/src/extensions/transparent.rs

+}
+
+#[derive(Debug, PartialEq)]
+pub enum Error<E> {


It may be possible now to move this to zcash_extensions.

daira

utACK with comments.

zcash_extensions/Cargo.toml

zcash_primitives/src/extensions/transparent.rs

zcash_extensions/src/transparent/demo.rs

zcash_primitives/src/transaction/builder.rs

daira · 2020-10-08T18:01:38Z

zcash_primitives/src/transaction/builder.rs

+            // The witness is expected to commit to the precommitment internally?
+            // (Or make it part of the sighash?)
+            // - TODO: Check whether transparent inputs committing to script_pubkey was
+            //   only so that hardware wallets "knew" what address was being spent from.


I believe so. @str4d?

(In any case this TODO should be resolved.)

@str4d what are your thoughts here? It seems like the information that a hardware wallet would need to display to the user in order for that user to be able to confidently sign a TZE transaction would vary by TZE; is there actually anything "general" that we can do here?

I think the "general" thing we do is what we are currently doing: require the entire TZE precondition to be part of the signed transaction digest. Any future address format linked to TZEs would be inherently linked to a particular precondition, and thus derivable from it.

The "Need to enable witness to commit to the amount" is already "solved", because each TZE has access to the prevout before calling Builder::add_tze_input, so they can cache the needed properties there. Alternatively, we could explicitly pass the prevout back to them when calling tze_in.builder.

I think expecting the TZE to cache what it needs is the proper thing to do here. That makes it so that there's a single path by which the implementers can achieve their goal, which is a good thing.

zcash_primitives/src/transaction/components.rs

zcash_extensions/Cargo.toml

Co-authored-by: Daira Hopwood <daira@jacaranda.org>

zcash_primitives/src/transaction/builder.rs

nuttycom added this to the Core Sprint 2020-35 milestone Sep 1, 2020

nuttycom self-assigned this Sep 1, 2020

nuttycom requested review from str4d, therealyingtong, daira and LarryRuane September 1, 2020 19:51

nuttycom mentioned this pull request Sep 1, 2020

Make version group constants agree with master. str4d/librustzcash#13

Closed

str4d requested changes Sep 4, 2020

View reviewed changes

r3ld3v modified the milestones: Core Sprint 2020-35, Core Sprint 2020-37 Sep 10, 2020

nuttycom requested a review from str4d September 10, 2020 17:57

str4d and others added 18 commits September 10, 2020 11:58

Core TZE traits and structs

90a17b3

Co-authored-by: Kris Nuttycombe <kris.nuttycombe@gmail.com>

Add TZEs to transaction format

b3332db

Demo TZE parsing and consensus logic

ce9a695

TZE consensus context and program sets

bf7f95b

Co-authored-by: Kris Nuttycombe <kris.nuttycombe@gmail.com>

Add transaction-builder suport for TZE-bearing transactions.

961d251

Make error a type member of FromPayload trait rather than a type vari…

659c34a

…able. Fix demo test compilation errors.

Add validation to demo transaction builder.

ca8e24e

Add tests for transaction builder support.

4a954c7

Check transparent input for correctness before modifying vin.

fe6bea1

Keep transaction details private.

4edbd26

Change extension_id and mode types to u32

294aa86

Add signature hash over TZE data.

dc9b007

zcash_extensions: Only run demo builder test if we have parameters

93ffd94

Add test for tze txn parsing.

8299f33

Update to non-yanked version of subtle crate.

4bf6309

Formatter fix.

81b6f8a

Use NetworkUpgrade for TZE epoch determination.

aed2759

This adds a `Future` variant for both BranchId and NetworkUpgrade.

Demo TZE fixes in response to PR comments.

af931d5

nuttycom and others added 2 commits September 23, 2020 13:44

Use more idiomatic construction of fixed-length arrays from slices.

2e02803

Co-authored-by: str4d <thestr4d@gmail.com>

Documentation fixes from code review.

995d063

Co-authored-by: str4d <thestr4d@gmail.com>

nuttycom force-pushed the zip-tzes branch from b6353e6 to 995d063 Compare September 23, 2020 20:16

nuttycom and others added 2 commits September 23, 2020 14:37

Use CompactSize & Vector::write for tze sighash construction.

9963fb9

Generate plausible scripts for transaction proptests.

b6cd096

Co-authored-by: str4d <thestr4d@gmail.com>

nuttycom requested a review from str4d September 23, 2020 20:49

Formatting fix.

03b4e56

nuttycom force-pushed the zip-tzes branch from 5363ce9 to 03b4e56 Compare September 23, 2020 23:14

str4d approved these changes Sep 24, 2020

View reviewed changes

nuttycom commented Sep 24, 2020

View reviewed changes

Zcashd integration is based on branch id.

e4e159b

r3ld3v modified the milestones: Core Sprint 2020-37, Core Sprint 2020-39 Sep 28, 2020

nuttycom added 2 commits September 28, 2020 14:06

Merge remote-tracking branch 'upstream/master' into zip-tzes

6d0287c

Replace domain separator with a 1-byte tag.

26aa239

daira approved these changes Oct 9, 2020

View reviewed changes

Add personalization to demo hashes.

9188a9d

nuttycom commented Oct 9, 2020

View reviewed changes

zcash_extensions/Cargo.toml Outdated Show resolved Hide resolved

nuttycom and others added 2 commits October 9, 2020 16:19

Apply suggestions from code review

39ab6e5

Co-authored-by: Daira Hopwood <daira@jacaranda.org>

Document TzeIn serialization.

cc121ed

r3ld3v modified the milestones: Core Sprint 2020-39, Core Sprint 2020-41 Oct 12, 2020

str4d added the S-committed Status: Planned work in a sprint label Oct 12, 2020

Fix formatting.

536c77b

nuttycom commented Oct 13, 2020

View reviewed changes

zcash_primitives/src/transaction/builder.rs Outdated Show resolved Hide resolved

Update comment in zcash_primitives/src/transaction/builder.rs

eeb0c2b

nuttycom merged commit 72b6de3 into zcash:master Oct 13, 2020

nuttycom deleted the zip-tzes branch October 14, 2020 23:13

daira mentioned this pull request Oct 19, 2020

Update ZIP 307 (light client protocol) with ZIP 212 changes zcash/zips#380

Closed

str4d mentioned this pull request Aug 1, 2024

zcash_protocol: Set the ZFuture consensus branch ID to agree with zcashd #1480

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Implementation for ZIP-222 Transparent Zcash Extensions #286

Implementation for ZIP-222 Transparent Zcash Extensions #286

nuttycom commented Sep 1, 2020

codecov bot commented Sep 1, 2020 •

edited

Loading

str4d left a comment

str4d left a comment

nuttycom Sep 24, 2020

daira left a comment •

edited

Loading

daira Oct 8, 2020

nuttycom Oct 9, 2020

str4d Oct 13, 2020

nuttycom Oct 13, 2020

Implementation for ZIP-222 Transparent Zcash Extensions #286

Implementation for ZIP-222 Transparent Zcash Extensions #286

Conversation

nuttycom commented Sep 1, 2020

codecov bot commented Sep 1, 2020 • edited Loading

Codecov Report

str4d left a comment

Choose a reason for hiding this comment

str4d left a comment

Choose a reason for hiding this comment

nuttycom Sep 24, 2020

Choose a reason for hiding this comment

daira left a comment • edited Loading

Choose a reason for hiding this comment

daira Oct 8, 2020

Choose a reason for hiding this comment

nuttycom Oct 9, 2020

Choose a reason for hiding this comment

str4d Oct 13, 2020

Choose a reason for hiding this comment

nuttycom Oct 13, 2020

Choose a reason for hiding this comment

codecov bot commented Sep 1, 2020 •

edited

Loading

daira left a comment •

edited

Loading