Skip to content

Add versioned signatures for Sapling transactions#34

Merged
ConstanceBeguier merged 6 commits into
zsa1from
sapling_sighash_info
Oct 29, 2025
Merged

Add versioned signatures for Sapling transactions#34
ConstanceBeguier merged 6 commits into
zsa1from
sapling_sighash_info

Conversation

@ConstanceBeguier
Copy link
Copy Markdown

@ConstanceBeguier ConstanceBeguier commented Sep 9, 2025

Add SighashInfo (version and associated data) to

  • Sapling binding signatures, and
  • Sapling authorizing signatures

Update sapling_auth_digest by adding SighashInfo as defined in ZIP246

PaulLaux
PaulLaux previously approved these changes Sep 11, 2025
Copy link
Copy Markdown

@PaulLaux PaulLaux left a comment

Choose a reason for hiding this comment

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

approved with minor comments

Comment thread zcash_test_vectors/transaction.py Outdated
Comment thread zcash_test_vectors/zip_0244.py Outdated
Copy link
Copy Markdown

@PaulLaux PaulLaux left a comment

Choose a reason for hiding this comment

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

also,
Please remove #TODO comment in Transaction_v6.py:
assert have_orchard_zsa or not have_issuance #TODO: VA: Combine this with the above?

Comment thread zcash_test_vectors/transaction.py Outdated
Comment on lines +522 to +527
if hasSapling:
if hasSighashInfo:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

combine both if's into one

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

I cannot combine both into one because the last line is into hasSapling loop but not into hasSigashInfo loop

Comment thread zcash_test_vectors/transaction.py Outdated
for desc in self.vSpendsSapling: # vSpendProofsSapling
ret += bytes(desc.proof)
for desc in self.vSpendsSapling: # vSpendAuthSigsSapling
if hasSighashInfo:
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

let's remove this param hasSighashInfo completly and replace with a runtime check for the class:

if isinstance(self, (TransactionV6)):

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

all checks (x2)

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

or tx.version_bytes() == NU7_TX_VERSION_BYTES: to be unified with below

Copy link
Copy Markdown
Author

Choose a reason for hiding this comment

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

Done

Copy link
Copy Markdown

@PaulLaux PaulLaux left a comment

Choose a reason for hiding this comment

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

Are all test files regenerated? I don't see new files for Sapling

@ConstanceBeguier
Copy link
Copy Markdown
Author

Are all test files regenerated? I don't see new files for Sapling

Only transaction V6 is updated. So, only orchard_zsa_digests is updated.

@ConstanceBeguier ConstanceBeguier merged commit f80b610 into zsa1 Oct 29, 2025
3 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants