Skip to content

Sapling transaction primitive data. #1814

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.

Sign up for GitHub

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

Jump to bottom
Merged
random-zebra merged 19 commits into from
Sep 22, 2020
Merged

Sapling transaction primitive data. #1814

random-zebra merged 19 commits into from
Sep 22, 2020

Conversation

@furszy
Copy link

@furszy furszy commented Aug 20, 2020 •
edited
Loading

Work coming from #1798. Decoupled commits related to the Sapling transaction primitive data and the new signature hash to make the review a little bit easier there. Plus, squashed some of them.

List of commits taken from #1798:

Primitive data:

  • Add serialization for primitive boost::optional. —> 659cc558
  • Transaction: Implement BaseOutPoint class. —> f17b23c7
  • Backport zkSNARK compression. —> 5dbc3c9
  • Basic Sapling transaction primitive data --> c49594f
  • Add serialization for std:list objects. —> 7cc57f7
  • sapling_transaction.h migrated to the new serialization —> dbb618f6
  • Fix sighash_tests —> e1e33163
  • Transaction GetShieldedValueIn & GetValueOut back port —> 0fbe0236
  • BugFix in GetShieldedValueIn —> 57ee1bc6
  • Missing SaplingOutPoint::ToString() method implemented —> e1fe5bf7
  • Transaction: add sapling data to toString —> 26ce38b5
  • make SaplingTxData Optional in transaction primitive — PARTIALLY —> c3b1aedc
  • Cleaning compiler warnings — PARTIALLY —> 644c381f
  • transaction: fix sapling version to 2. —> 8fc595f5
  • Transaction: GetDustThreshold method implemented. —> dadf199c.
  • Transaction:hasSaplingData check is enough checking for vShieldedOutput and vShieldedSpend emptiness. —> 57d3236f

Signature hash:

  • Introduce SignatureHash personalization updates —> 550217f7
  • Enable sigversion for Sapling transactions. —> c04abe1d
  • Sapling signature hash (Custom version of ZIP 243) —> eac206e9
  • Not try to serialize empty fields in Sapling signature hash ---> ce5a467

@furszy furszy self-assigned this Aug 20, 2020
random-zebra
random-zebra reviewed Sep 14, 2020
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Looking pretty good. Code review ACK, modulo some notes and nits.
There is room for some cleanup due to the removal of the sigversion parameter from the SignatureHash, but it can be addressed in a followup PR.

@random-zebra random-zebra added this to the 5.0.0 milestone Sep 14, 2020
@furszy furszy force-pushed the 2020_sapling_tx_data branch from dbbad19 to b4c8c8e Compare September 15, 2020 00:58
@furszy
Copy link
Author

furszy commented Sep 15, 2020

Great feedback!, PR updated.

@furszy furszy force-pushed the 2020_sapling_tx_data branch 2 times, most recently from e2c2119 to 8cddca5 Compare September 17, 2020 04:08
@furszy
Copy link
Author

furszy commented Sep 17, 2020 •
edited
Loading

Updated removing the following commit:
Enable sigversion for Sapling transactions. —> c04abe1

Instead of defining the SigVersion inside SignatureHash, it's better to pass it as an argument, moving the protocol upgrade enforcement check/decision to the caller side.
TODO: This change affects 078f7005 which will need to input SIGVERSION_SAPLING.

@furszy furszy force-pushed the 2020_sapling_tx_data branch 2 times, most recently from ada8e3d to 0b77cfb Compare September 18, 2020 06:59
@furszy furszy requested a review from random-zebra September 18, 2020 16:15
random-zebra
random-zebra reviewed Sep 21, 2020
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

The global flag to guard v2 txes looks good now.

But have found more instances where we try to access sapData without first checking if it's initialized.

In GetShieldedSpendsHash and GetShieldedOutputsHash we should probably also check first the version of txTo, and return early (maybe just UINT256_ZERO, instead of the hash of an empty stream) if either IsSapling() or hasSaplingData() is false.

One more thing.
The hack in the unit test (a3eaeb0) can be removed now that the version serialization is properly guarded.

@furszy furszy force-pushed the 2020_sapling_tx_data branch from 0b77cfb to 3219140 Compare September 21, 2020 21:53
@furszy
Copy link
Author

furszy commented Sep 21, 2020

Done, updated based on the feedback.
GetShieldedSpendsHash and GetShieldedOutputsHash are both protected on the caller side.

@random-zebra
Copy link

random-zebra commented Sep 21, 2020

GetShieldedSpendsHash and GetShieldedOutputsHash are both protected on the caller side.

Looking good.
Maybe it's worth to assert(txTo.sapData) (or return an error if it's null) inside them, just to prevent possible future hard-to-discover segfaults if someone introduces code that calls these functions without checking txTo first?

@furszy
Copy link
Author

furszy commented Sep 21, 2020

done, assertion preventing possible future hard-to-discover segfaults implemented.

random-zebra
random-zebra approved these changes Sep 21, 2020
View reviewed changes
Copy link

@random-zebra random-zebra left a comment

Choose a reason for hiding this comment

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

Nice. 🍻 ACK 2fff9ec

@furszy furszy requested a review from Fuzzbawls September 22, 2020 02:23
Fuzzbawls
Fuzzbawls approved these changes Sep 22, 2020
View reviewed changes
Copy link
Collaborator

@Fuzzbawls Fuzzbawls left a comment

Choose a reason for hiding this comment

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

utACK 2fff9ec

@random-zebra random-zebra merged commit 2018fca into PIVX-Project:master Sep 22, 2020
@furszy furszy mentioned this pull request Sep 24, 2020
Merged
furszy added a commit that referenced this pull request Oct 1, 2020
@furszy
Merge #1867: BugFix: g_IsSaplingActive flag is not initialized at sta…
c65d036 
…rtup.

520d889 BugFix: g_IsSaplingActive flag wasn't being updated at startup. (furszy)

Pull request description:

  Small bug introduced in #1814, the flag is set on every new tip but not initialized at startup.
  Discovered rebasing #1798 on top of master and running any sapling functional test. As `g_IsSaplingActive` is false at startup, the transaction hash is different, making the merkle tree hash different, failing in `VerifyDB`.

ACKs for top commit:
  random-zebra:
    utACK 520d889
  Fuzzbawls:
    utACK 520d889

Tree-SHA512: 609d0257a524aa85a12296ab827b0cd9dc640e60455801203d52709672e79cd9837f606743b976e5cc64d9f0659d81bd19b5bb6ec6b339fd01eebf44af58677e
@furszy furszy deleted the 2020_sapling_tx_data branch November 29, 2022 14:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

+1 more reviewer

@random-zebra random-zebra random-zebra approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@furszy furszy

Labels

Sapling Upstream ZCash Wallet

Projects

None yet

Milestone

5.0.0

Development

Successfully merging this pull request may close these issues.

3 participants

@furszy @random-zebra @Fuzzbawls