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.

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

Sapling transaction primitive data. #1814

Merged
merged 19 commits into from
Sep 22, 2020
Merged

Sapling transaction primitive data. #1814

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. —> 659cc55
  • Transaction: Implement BaseOutPoint class. —> f17b23c
  • 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 —> dbb618f
  • Fix sighash_tests —> e1e3316
  • Transaction GetShieldedValueIn & GetValueOut back port —> 0fbe023
  • BugFix in GetShieldedValueIn —> 57ee1bc
  • Missing SaplingOutPoint::ToString() method implemented —> e1fe5bf7
  • Transaction: add sapling data to toString —> 26ce38b
  • make SaplingTxData Optional in transaction primitive — PARTIALLY —> c3b1aed
  • Cleaning compiler warnings — PARTIALLY —> 644c381
  • transaction: fix sapling version to 2. —> 8fc595f
  • Transaction: GetDustThreshold method implemented. —> dadf199.
  • Transaction:hasSaplingData check is enough checking for vShieldedOutput and vShieldedSpend emptiness. —> 57d3236

Signature hash:

  • Introduce SignatureHash personalization updates —> 550217f
  • Enable sigversion for Sapling transactions. —> c04abe1
  • Sapling signature hash (Custom version of ZIP 243) —> eac206e
  • 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.

src/serialize.h Outdated Show resolved Hide resolved
src/sapling/proof.cpp Outdated Show resolved Hide resolved
src/serialize.h Show resolved Hide resolved
src/primitives/transaction.h Outdated Show resolved Hide resolved
src/test/sighash_tests.cpp Outdated Show resolved Hide resolved
src/script/sign.cpp Outdated Show resolved Hide resolved
src/script/interpreter.cpp Outdated Show resolved Hide resolved
src/script/interpreter.cpp Outdated Show resolved Hide resolved
src/script/interpreter.cpp Outdated Show resolved Hide resolved
@random-zebra random-zebra added this to the 5.0.0 milestone Sep 14, 2020
@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 078f700 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
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.

src/primitives/transaction.cpp Show resolved Hide resolved
src/script/interpreter.cpp Show resolved Hide resolved
src/script/interpreter.cpp Show resolved Hide resolved
@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

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

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

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

@Fuzzbawls Fuzzbawls Fuzzbawls approved these changes

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