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

TransactionBuilder for Sapling and transparent transactions #3417

Merged
merged 13 commits into from Aug 1, 2018

Conversation

@str4d
Copy link
Contributor

str4d commented Jul 25, 2018

This PR includes zcash/librustzcash#24, and will create a testnet chain split once merged into master (and once a Sapling transaction is mined).

str4d added some commits Jun 16, 2018

Alter SaplingNote::nullifier() to take a SaplingFullViewingKey
This means the API will work if you only have a SaplingExtendedSpendingKey,
as will be the case with ZIP 32.

@str4d str4d added the Sapling label Jul 25, 2018

@str4d str4d added this to the v2.0.0 milestone Jul 25, 2018

@str4d str4d added this to Review Backlog in Zcashd Team Jul 25, 2018

@str4d str4d moved this from Review Backlog to In Review in Zcashd Team Jul 25, 2018

@ebfull

ebfull approved these changes Jul 25, 2018

Copy link
Contributor

ebfull left a comment

ACK, but if it's not too much trouble I'd love to see a check that all of the anchors are the same (as a stop-gap for a protocol change to enforce this).

TransactionBuilder: Check that all anchors in a transaction are ident…
…ical

This reduces the amount of information that is leaked by the choice of anchor.
In future we will make a protocol change to enforce that all inputs use the
same anchor.
@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Jul 27, 2018

Addressed @ebfull's comment.

@str4d str4d force-pushed the str4d:sapling-tx-builder branch from f34ad0c to 25bb05d Jul 27, 2018

str4d added some commits Jul 27, 2018

@str4d str4d force-pushed the str4d:sapling-tx-builder branch from bac411d to a8dd4b0 Jul 30, 2018

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Jul 30, 2018

I'd like to see this merged together with a PR to use a version of sapling-crypto that includes zcash-hackworks/sapling-crypto#80 , so that we don't inadvertently end up with two chain splits. The rationale is that this rule change will create a split on the next Sapling transaction, whereas the RedDSA change will only do so if someone creates a nonstandard signature (that will never be created by an unmodified zcashd).

@daira

daira approved these changes Jul 30, 2018

Copy link
Contributor

daira left a comment

ut(ACK+cov)

auto fvk_from = sk_from.full_viewing_key();

auto sk = libzcash::SaplingSpendingKey::random();
auto xsk = sk.expanded_spending_key();

This comment has been minimized.

@daira

daira Jul 30, 2018

Contributor

Rename this to expsk.

#include <librustzcash.h>

SpendDescriptionInfo::SpendDescriptionInfo(
libzcash::SaplingExpandedSpendingKey xsk,

This comment has been minimized.

@daira

daira Jul 30, 2018

Contributor

Rename to expsk, also the member variable.

CTxOut out(value, scriptPubKey);
mtx.vout.push_back(out);
return true;
}

This comment has been minimized.

@daira

daira Jul 30, 2018

Contributor

Should there be a similar method that just takes a CTxOut, in case a caller wants to use a funky script?

This comment has been minimized.

@str4d

str4d Jul 30, 2018

Contributor

If someone needs that in future, they can add it.

change -= tOut.nValue;
}
if (change < 0) {
return boost::none;

This comment has been minimized.

@daira

daira Jul 30, 2018

Contributor

Maybe we should return a value that indicates why building the tx failed?

This comment has been minimized.

@str4d

str4d Jul 30, 2018

Contributor

Were this Rust, I'd be returning a Result<CTransaction, Reason>. The equivalent in C++ is very messy; probably the simplest is to return a std::pair<boost::optional<CTransaction>, Reason>, or we could add a GetFailureReason() method that exposes an internal variable we set before returning from Build().

This comment has been minimized.

@daira

daira Aug 1, 2018

Contributor

I would be inclined to implement a Result class.

Rename xsk to expsk
xsk will be used for ZIP 32 extended spending keys, so renaming here to
reduce confusion.
@ebfull

ebfull approved these changes Jul 30, 2018

@@ -170,6 +170,10 @@ friend class IncrementalMerkleTree<Depth, Hash>;
return tree.last();
}

uint64_t position() const {
return tree.size() - 1;

This comment has been minimized.

@ebfull

ebfull Jul 30, 2018

Contributor

NB: an IncrementalWitness cannot be constructed such that tree.size() is 0, so this will never underflow.

@Eirik0 Eirik0 self-requested a review Jul 30, 2018

@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Jul 31, 2018

Pushed some commits to adopt zcash/librustzcash#26 into this PR and fix a bug in the return value from SendChangeTo.

@ebfull

ebfull approved these changes Jul 31, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Jul 31, 2018

ACK on the two commits @ebfull pushed.

@str4d str4d changed the title TransactionBuilder with support for creating Sapling-only transactions TransactionBuilder for Sapling and transparent transactions Jul 31, 2018

@daira

daira approved these changes Jul 31, 2018

Copy link
Contributor

daira left a comment

ACK b7b088c and 04ed758 .

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Jul 31, 2018

Overall looks good.

One general comment that I have is that it would be nice if the methods in TransactionBuilder returned *this rather than bool or void. Because this is not yet used in production it is hard to tell if this makes TransactionBuilder easier to use. Returning a bool is useful if we are able to write code to recover from whatever has caused the failure. If we are not able to recover, we may as well throw some error rather than return bool as this can include more information. Similarly, are we able to recover if .Build() returns none? Would we be able to provide the end user more information if we throw an error in .Build() rather than returning none?

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Jul 31, 2018

Useful comments, but we can look into ergonomics after we've used this in z_sendmany (and thus have more feedback from actually using the API).

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 31, 2018

📌 Commit 04ed758 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Jul 31, 2018

⌛️ Testing commit 04ed758 with merge f87011a...

zkbot added a commit that referenced this pull request Jul 31, 2018

Auto merge of #3417 - str4d:sapling-tx-builder, r=str4d
TransactionBuilder for Sapling and transparent transactions

This PR includes zcash/librustzcash#24, and will create a testnet chain split once merged into master (and once a Sapling transaction is mined).
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 1, 2018

💔 Test failed - pr-merge

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Aug 1, 2018

Tests fail when running locally

./zcash-gtest --gtest_filter=TransactionBuilder.*
Note: Google Test filter = TransactionBuilder.*
[==========] Running 7 tests from 1 test case.
[----------] Global test environment set-up.
[----------] 7 tests from TransactionBuilder
[ RUN      ] TransactionBuilder.Invoke
gtest/test_transaction_builder.cpp:54: Failure
Value of: ContextualCheckTransaction(tx1, state, 2, 0)
  Actual: false
Expected: true
gtest/test_transaction_builder.cpp:55: Failure
      Expected: state.GetRejectReason()
      Which is: "bad-txns-sapling-output-description-invalid"
To be equal to: ""
gtest/test_transaction_builder.cpp:78: Failure
      Expected: static_cast<bool>(maybe_tx2)
      Which is: false
To be equal to: true
[  FAILED  ] TransactionBuilder.Invoke (1831 ms)
[ RUN      ] TransactionBuilder.ThrowsOnTransparentInputWithoutKeyStore
[       OK ] TransactionBuilder.ThrowsOnTransparentInputWithoutKeyStore (0 ms)
[ RUN      ] TransactionBuilder.RejectsInvalidTransparentOutput
[       OK ] TransactionBuilder.RejectsInvalidTransparentOutput (0 ms)
[ RUN      ] TransactionBuilder.RejectsInvalidTransparentChangeAddress
[       OK ] TransactionBuilder.RejectsInvalidTransparentChangeAddress (0 ms)
[ RUN      ] TransactionBuilder.FailsWithNegativeChange
gtest/test_transaction_builder.cpp:172: Failure
Value of: static_cast<bool>(builder.Build())
  Actual: false
Expected: true
[  FAILED  ] TransactionBuilder.FailsWithNegativeChange (1566 ms)
[ RUN      ] TransactionBuilder.ChangeOutput
gtest/test_transaction_builder.cpp:225: Failure
      Expected: static_cast<bool>(maybe_tx)
      Which is: false
To be equal to: true
[  FAILED  ] TransactionBuilder.ChangeOutput (1603 ms)
[ RUN      ] TransactionBuilder.SetFee
gtest/test_transaction_builder.cpp:303: Failure
      Expected: static_cast<bool>(maybe_tx)
      Which is: false
To be equal to: true
[  FAILED  ] TransactionBuilder.SetFee (1580 ms)
[----------] 7 tests from TransactionBuilder (6581 ms total)

[----------] Global test environment tear-down
[==========] 7 tests from 1 test case ran. (6581 ms total)
[  PASSED  ] 3 tests.
[  FAILED  ] 4 tests, listed below:
[  FAILED  ] TransactionBuilder.Invoke
[  FAILED  ] TransactionBuilder.FailsWithNegativeChange
[  FAILED  ] TransactionBuilder.ChangeOutput
[  FAILED  ] TransactionBuilder.SetFee

 4 FAILED TESTS
@ebfull

This comment has been minimized.

Copy link
Contributor

ebfull commented Aug 1, 2018

@bitcartel I haven't seen such failures.

I had to relocate ECC_Start() to avoid some kind of race condition in the test harness.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 1, 2018

📌 Commit a310f6c has been approved by ebfull

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 1, 2018

⌛️ Testing commit a310f6c with merge d84f14e...

zkbot added a commit that referenced this pull request Aug 1, 2018

Auto merge of #3417 - str4d:sapling-tx-builder, r=ebfull
TransactionBuilder for Sapling and transparent transactions

This PR includes zcash/librustzcash#24, and will create a testnet chain split once merged into master (and once a Sapling transaction is mined).
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Aug 1, 2018

☀️ Test successful - pr-merge
Approved by: ebfull
Pushing d84f14e to master...

@zkbot zkbot merged commit a310f6c into zcash:master Aug 1, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Review to Released (Merged in Master) Aug 1, 2018

@str4d str4d deleted the str4d:sapling-tx-builder branch Aug 1, 2018

@daira

This comment has been minimized.

Copy link
Contributor

daira commented Aug 1, 2018

a310f6c cannot be the right fix for that race condition, surely? It's dependent on the order in which tests are run.

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 1, 2018

@daira I think the problem was the presence of a second call. I thought I was fixing this once and for all in 3fd0a26, but I missed one. Removing the call from all individual tests, and leaving it in the initializer, should be sufficient.

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