Skip to content

ZIP 225 & ZIP 244#375

Merged
str4d merged 26 commits into
zcash:masterfrom
nuttycom:feature/zip-225
Jun 8, 2021
Merged

ZIP 225 & ZIP 244#375
str4d merged 26 commits into
zcash:masterfrom
nuttycom:feature/zip-225

Conversation

@nuttycom
Copy link
Copy Markdown
Collaborator

@nuttycom nuttycom commented Apr 21, 2021

No description provided.

@r3ld3v r3ld3v added this to the Core Sprint 2021-16 milestone Apr 27, 2021
@nuttycom nuttycom force-pushed the feature/zip-225 branch from 4b8ea07 to da7f667 Compare May 3, 2021 20:43
@nuttycom nuttycom mentioned this pull request May 4, 2021
Closed
@nuttycom nuttycom force-pushed the feature/zip-225 branch from d054bc2 to a6940bf Compare May 4, 2021 18:07
@nuttycom nuttycom marked this pull request as ready for review May 4, 2021 18:31
@nuttycom
Copy link
Copy Markdown
Collaborator Author

nuttycom commented May 4, 2021

A note to reviewers: This PR is best viewed hiding whitespace changes.

@nuttycom nuttycom force-pushed the feature/zip-225 branch 3 times, most recently from 39770cf to aa060fa Compare May 6, 2021 15:42
@nuttycom nuttycom marked this pull request as draft May 7, 2021 20:14
conradoplg
conradoplg reviewed May 7, 2021
View reviewed changes
Copy link
Copy Markdown

@conradoplg conradoplg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Found a possible personalization field mismatch

Comment thread zcash_primitives/src/transaction/txid.rs Outdated
conradoplg
conradoplg reviewed May 7, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/txid.rs Outdated
@nuttycom nuttycom marked this pull request as ready for review May 10, 2021 21:47
@teor2345 teor2345 mentioned this pull request May 11, 2021
Merged
2 tasks
@nuttycom nuttycom marked this pull request as draft May 11, 2021 15:14
therealyingtong
therealyingtong reviewed May 13, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@therealyingtong therealyingtong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Flushing comments from partial review.

Comment thread zcash_primitives/src/transaction/mod.rs Outdated
Comment thread zcash_primitives/src/transaction/txid.rs Outdated
Copy link
Copy Markdown
Contributor

@therealyingtong therealyingtong May 7, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could to_txid() be a method on TransactionData?

Comment thread zcash_primitives/src/consensus.rs Outdated
Comment thread zcash_primitives/Cargo.toml Outdated
Comment thread zcash_primitives/Cargo.toml Outdated
Comment thread zcash_primitives/src/transaction/builder.rs Outdated
Comment thread zcash_primitives/src/transaction/mod.rs Outdated
Comment thread zcash_primitives/src/transaction/components/transparent/builder.rs Outdated
therealyingtong
therealyingtong reviewed May 13, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/txid.rs Outdated
therealyingtong
therealyingtong reviewed May 13, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/sighash_v5.rs Outdated
therealyingtong
therealyingtong reviewed May 13, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/txid.rs Outdated
@nuttycom nuttycom force-pushed the feature/zip-225 branch 2 times, most recently from 3450de4 to 239b125 Compare May 14, 2021 15:37
@nuttycom nuttycom marked this pull request as ready for review May 14, 2021 15:38
@nuttycom @str4d
Use Vector read/write operations where possible instead of Array
4f764c3 
Co-authored-by: str4d <jack@electriccoin.co>
@nuttycom nuttycom force-pushed the feature/zip-225 branch 2 times, most recently from ecc9b68 to 8a9eda7 Compare June 5, 2021 16:18
@nuttycom @str4d
Deserialize signatures directly into actions.
b93c503 
Co-authored-by: str4d <jack@electriccoin.co>
@nuttycom nuttycom force-pushed the feature/zip-225 branch 2 times, most recently from 54dce0e to 1d7c6ea Compare June 5, 2021 16:42
str4d
str4d approved these changes Jun 5, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 1d7c6ea9f9a60a564a7ead4cab9b9e6b55aef241

daira
daira reviewed Jun 6, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/builder.rs Outdated
daira
daira reviewed Jun 6, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/mod.rs Outdated
daira
daira reviewed Jun 6, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/sighash_v5.rs Outdated
Copy link
Copy Markdown
Contributor

@daira daira Jun 6, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not checked this file either.

daira
daira reviewed Jun 6, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Partial review. I commented which files I have not checked.

nuttycom and others added 3 commits June 7, 2021 09:27
@nuttycom
Be more explicit about empty sapling & orchard hashes.
0253442
@nuttycom @str4d
Minor error message improvement.
371f84d 
Co-authored-by: str4d <jack@electriccoin.co>
@nuttycom @str4d
Fix error in transaction auth digest.
f70285d 
Co-authored-by: str4d <jack@electriccoin.co>
daira
daira reviewed Jun 8, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/builder.rs
unauthed_tx,
let transparent_bundle = unauthed_tx.transparent_bundle.clone().map(|b| {
b.apply_signatures(
#[cfg(feature = "transparent-inputs")]
Copy link
Copy Markdown
Contributor

@daira daira Jun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this be a separate method for the case with transparent inputs, rather than a method with feature-conditional arguments?

Copy link
Copy Markdown
Contributor

@str4d str4d Jun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's going to happen in a subsequent PR, when the Orchard builder is integrated.

daira
daira reviewed Jun 8, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/mod.rs
if actions_without_auth.is_empty() {
Ok(None)
} else {
let flags = orchard_serialization::read_flags(&mut reader)?;
Copy link
Copy Markdown
Contributor

@daira daira Jun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The implementation of read_flags enforces that unused bits are zero. This is as intended but was ambiguous in the spec. The spec has been clarified in version 2021.2.4.

daira
daira reviewed Jun 8, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/mod.rs Outdated
daira
daira reviewed Jun 8, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/mod.rs
daira
daira reviewed Jun 8, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/mod.rs Outdated
daira
daira reviewed Jun 8, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/mod.rs
lock_time,
expiry_height: expiry_height.into(),
transparent_bundle,
sprout_bundle: None,
Copy link
Copy Markdown
Contributor

@daira daira Jun 8, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I know we don't support creating v4 transactions with a Sprout bundle, but we do support reading them, so it should probably be prop-tested. Please file a ticket. (Does not block this PR.)

daira
daira reviewed Jun 8, 2021
View reviewed changes
Comment thread zcash_primitives/src/transaction/txid.rs
const ZCASH_SAPLING_SIGS_HASH_PERSONALIZATION: &[u8; 16] = b"ZTxAuthSapliHash";
const ZCASH_ORCHARD_SIGS_HASH_PERSONALIZATION: &[u8; 16] = b"ZTxAuthOrchaHash";
#[cfg(feature = "zfuture")]
const ZCASH_TZE_WITNESSES_HASH_PERSONALIZATION: &[u8; 16] = b"ZTxAuthTZE__Hash";
Copy link
Copy Markdown
Contributor

@daira daira Jun 8, 2021 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I checked all of the non-TZE personalizations against the current version of ZIP 244.

daira
daira approved these changes Jun 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK with minor comments.

@nuttycom @daira
Apply suggestions from code review
eb3d01a 
Co-authored-by: Daira Hopwood <daira@jacaranda.org>
str4d
str4d approved these changes Jun 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@str4d str4d left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK eb3d01a. I confirmed it addressed @daira's comments.

@str4d str4d merged commit 0bfd1f7 into zcash:master Jun 8, 2021
daira
daira reviewed Jun 8, 2021
View reviewed changes
Copy link
Copy Markdown
Contributor

@daira daira left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Post-hoc ACK eb3d01a

@nuttycom nuttycom deleted the feature/zip-225 branch June 12, 2021 21:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@conradoplg conradoplg conradoplg left review comments

@daira daira daira approved these changes

@str4d str4d str4d approved these changes

+1 more reviewer

@therealyingtong therealyingtong therealyingtong left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

S-committed Status: Planned work in a sprint

Projects

None yet

Milestone

Core Sprint 2021-22

Development

Successfully merging this pull request may close these issues.

7 participants

@nuttycom @defuse @conradoplg @daira @str4d @therealyingtong @r3ld3v