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.

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

ZIP 243: Sapling SignatureHash #3233

Merged
merged 2 commits into from May 9, 2018

Conversation

4 participants
@str4d
Contributor

str4d commented May 1, 2018

Closes #3164.

@str4d str4d added this to the v1.1.1 milestone May 1, 2018

@str4d str4d requested review from gtank and ebfull May 1, 2018

@str4d str4d added this to In progress in Consensus Protocol Team May 1, 2018

@str4d str4d referenced this pull request May 2, 2018

Closed

Implement Sapling consensus rules #3065

25 of 25 tasks complete
@ebfull

ebfull approved these changes May 4, 2018

@str4d str4d moved this from In progress to In Review in Consensus Protocol Team May 4, 2018

uint256 GetShieldedSpendsHash(const CTransaction& txTo) {
CBLAKE2bWriter ss(SER_GETHASH, 0, ZCASH_SHIELDED_SPENDS_HASH_PERSONALIZATION);
for (unsigned int n = 0; n < txTo.vShieldedSpend.size(); n++) {
ss << txTo.vShieldedSpend[n];

This comment has been minimized.

@ebfull

ebfull May 8, 2018

Contributor

I'm confused... this seems to cover the signature inside the shielded spend. That isn't right, is it?

This comment has been minimized.

@str4d

str4d May 8, 2018

Contributor

Ooh, good catch!

str4d added some commits May 1, 2018

@str4d

This comment has been minimized.

Contributor

str4d commented May 8, 2018

Force-pushed to update the commits, fixing the bug @ebfull found, as well as a bug in the tests which meant that GetShieldedSpendsHash and GetShieldedOutputsHash weren't being exercised. Diff (excluding test data update):

diff --git a/src/script/interpreter.cpp b/src/script/interpreter.cpp
index 47ac62b..ec1ac52 100644
--- a/src/script/interpreter.cpp
+++ b/src/script/interpreter.cpp
@@ -1106,7 +1106,11 @@ uint256 GetJoinSplitsHash(const CTransaction& txTo) {
 uint256 GetShieldedSpendsHash(const CTransaction& txTo) {
     CBLAKE2bWriter ss(SER_GETHASH, 0, ZCASH_SHIELDED_SPENDS_HASH_PERSONALIZATION);
     for (unsigned int n = 0; n < txTo.vShieldedSpend.size(); n++) {
-        ss << txTo.vShieldedSpend[n];
+        ss << txTo.vShieldedSpend[n].cv;
+        ss << txTo.vShieldedSpend[n].anchor;
+        ss << txTo.vShieldedSpend[n].nullifier;
+        ss << txTo.vShieldedSpend[n].rk;
+        ss << txTo.vShieldedSpend[n].zkproof;
     }
     return ss.GetHash();
 }
diff --git a/src/test/sighash_tests.cpp b/src/test/sighash_tests.cpp
index 6cb2cf1..a73bd6f 100644
--- a/src/test/sighash_tests.cpp
+++ b/src/test/sighash_tests.cpp
@@ -145,7 +145,7 @@ void static RandomTransaction(CMutableTransaction &tx, bool fSingle, uint32_t co
         txout.nValue = insecure_rand() % 100000000;
         RandomScript(txout.scriptPubKey);
     }
-    if (tx.nVersionGroupId == SAPLING_TX_VERSION) {
+    if (tx.nVersionGroupId == SAPLING_VERSION_GROUP_ID) {
         tx.valueBalance = insecure_rand() % 100000000;
         for (int spend = 0; spend < shielded_spends; spend++) {
             SpendDescription sdesc;
@ebfull

ebfull approved these changes May 8, 2018

@gtank

gtank approved these changes May 8, 2018

A couple of nits or questions, but nothing blocking.

uint256 GetShieldedOutputsHash(const CTransaction& txTo) {
CBLAKE2bWriter ss(SER_GETHASH, 0, ZCASH_SHIELDED_OUTPUTS_HASH_PERSONALIZATION);
for (unsigned int n = 0; n < txTo.vShieldedOutput.size(); n++) {
ss << txTo.vShieldedOutput[n];

This comment has been minimized.

@gtank

gtank May 8, 2018

Contributor

Why are SpendDescriptions serialized fieldwise but OutputDescriptions are not?

This comment has been minimized.

@str4d

str4d May 8, 2018

Contributor

OutputDescriptions can be serialized as-is, whereas SpendDescriptions need to exclude the signature.

@@ -101,6 +101,7 @@ enum SigVersion
{
SIGVERSION_SPROUT = 0,
SIGVERSION_OVERWINTER = 1,
SIGVERSION_SAPLING = 2,

This comment has been minimized.

@gtank

gtank May 8, 2018

Contributor

This is not mentioned anywhere in ZIP-243.

This comment has been minimized.

@str4d

str4d May 8, 2018

Contributor

It's mostly an implementation detail, instead of having three separate functions. But the fact that we are switching on the version group ID might be worth mentioning (though that itself is also just an implementation detail of how we are targeting its use on v4 transactions).

@str4d

This comment has been minimized.

Contributor

str4d commented May 8, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Contributor

zkbot commented May 8, 2018

📌 Commit 5028498 has been approved by str4d

@zkbot

This comment has been minimized.

Contributor

zkbot commented May 8, 2018

⌛️ Testing commit 5028498 with merge 642a11d...

zkbot added a commit that referenced this pull request May 8, 2018

Auto merge of #3233 - str4d:3164-sapling-sighash, r=str4d
ZIP 243: Sapling SignatureHash

Closes #3164.
@zkbot

This comment has been minimized.

Contributor

zkbot commented May 8, 2018

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Contributor

str4d commented May 8, 2018

Transient failure.

@zkbot retry

@zkbot

This comment has been minimized.

Contributor

zkbot commented May 8, 2018

⌛️ Testing commit 5028498 with merge f0daf39...

zkbot added a commit that referenced this pull request May 8, 2018

Auto merge of #3233 - str4d:3164-sapling-sighash, r=str4d
ZIP 243: Sapling SignatureHash

Closes #3164.
@zkbot

This comment has been minimized.

Contributor

zkbot commented May 9, 2018

☀️ Test successful - pr-merge
Approved by: str4d
Pushing f0daf39 to master...

@zkbot zkbot merged commit 5028498 into zcash:master May 9, 2018

1 check passed

homu Test successful
Details

Consensus Protocol Team automation moved this from In Review to Complete PRs (Ignore) May 9, 2018

@str4d str4d deleted the str4d:3164-sapling-sighash branch May 16, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment