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

Add Sapling support to z_sendmany #3489

Merged
merged 10 commits into from Sep 3, 2018

Conversation

5 participants
@str4d
Copy link
Contributor

str4d commented Aug 27, 2018

Closes #3215.

str4d added some commits Aug 4, 2018

Move GetSpendingKeyForPaymentAddress visitor into wallet.h
Also fixes it to not use the global pwalletMain.

@str4d str4d added this to the v2.0.1 milestone Aug 27, 2018

@str4d str4d added this to In Review in Zcashd Team Aug 27, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Aug 28, 2018

@str4d Did you use this PR (as is, rebased on top of master) to send a Sapling tx on testnet?

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Aug 28, 2018

@daira
Copy link
Contributor

daira left a comment

Flushing comments so far.

throw JSONRPCError(RPC_INVALID_ADDRESS_OR_KEY, "From address does not belong to this node, zaddr spending key not found.");
}

// Remember whether this is a Sprout or Sapling address
// !fromTaddr && !fromSapling -> Sprout

This comment has been minimized.

@daira

daira Aug 29, 2018

Contributor

It might be clearer to add bool fromSprout = !fromTaddr && !fromSapling; rather than requiring the reader to remember this.

This comment has been minimized.

@Eirik0

Eirik0 Aug 29, 2018

Contributor

I find it nontrivial to follow the logic for fromTaddr/fromSapling/noSproutAddrs in this file and then isUsingBuilder_/isfromtaddr_/isfromzaddr_ in asyncoperation_sendmany.cpp. Could we change this to an enum which we pass through to AsyncRPCOperation_sendmany?

This comment has been minimized.

@str4d

str4d Aug 30, 2018

Contributor

Changing other logic in asyncoperation_sendmany.cpp is part of #2979; it's far too involved for this PR. I'll add fromSprout (and note that isfromtaddr_ vs isfromzaddr_ likely came from the same kind of request).

This comment has been minimized.

@bitcartel

bitcartel Sep 1, 2018

Contributor

@daira Does the latest commit address your concern?

// If we are sending from a shielded address, all recipient
// shielded addresses must be of the same type.
if (!fromTaddr) {
if (!fromSapling && toSapling) {

This comment has been minimized.

@daira

daira Aug 29, 2018

Contributor

With the suggestion above to add fromSprout, the if (!fromTaddr) is no longer necessary and this becomes if (fromSprout && toSapling), which seems clearer.

This comment has been minimized.

@bitcartel

bitcartel Sep 1, 2018

Contributor

@daira The latest commit addresses this point.

@Eirik0 Eirik0 self-requested a review Aug 29, 2018

const libzcash::InvalidEncoding& no) const
{
// Defaults to InvalidEncoding
return libzcash::SpendingKey();

This comment has been minimized.

@Eirik0

Eirik0 Aug 29, 2018

Contributor

We used to throw an exception here. In the sprout/sapling versions of this method we have changed throw exceptions to returning none. Why is that not the case here?

This comment has been minimized.

@str4d

str4d Aug 30, 2018

Contributor

Look elsewhere in the PR for where this visitor is applied. In the original location, we now throw an error if this visitor returns boost::none. In the new location, we check whether we have the key before we try to get it, so we know that we will never get boost::none.

// Contextual transaction we will build on
// (used if no Sapling addresses are involved)

This comment has been minimized.

@Eirik0

Eirik0 Aug 29, 2018

Contributor

I am a little confused by this. Is it the case that we are either using the TransactionBuilder or the CMutableTransaction, and never both?

This comment has been minimized.

@str4d

str4d Aug 30, 2018

Contributor

Correct.

This comment has been minimized.

@str4d

str4d Aug 30, 2018

Contributor

I'd like to change the function to take in a boost::variant<TransactionBuilder, CMutableTransaction>, but that will have to wait for #2979.

@@ -354,6 +365,141 @@ bool AsyncRPCOperation_sendmany::main_impl() {
LogPrint("zrpcunsafe", "%s: private output: %s\n", getId(), FormatMoney(z_outputs_total));
LogPrint("zrpc", "%s: fee: %s\n", getId(), FormatMoney(minersFee));


/**
* SCENARIO #0

This comment has been minimized.

@Eirik0

Eirik0 Aug 29, 2018

Contributor

Nit: This method is already so ginormous. It would be nice if these scenarios were extracted in to methods. Especially new ones that we are adding.

This comment has been minimized.

@str4d

str4d Aug 30, 2018

Contributor

Falls under #2979.

This comment has been minimized.

@daira

daira Sep 1, 2018

Contributor

Yes but this is almost unreviewable as it stands :-(

expsk = sk.expanded_spending_key();
from = expsk.full_viewing_key();
} else {
// TODO: Set "from" to something!

This comment has been minimized.

@Eirik0

Eirik0 Aug 29, 2018

Contributor

Should this be done as part of this PR?

This comment has been minimized.

@str4d

str4d Aug 30, 2018

Contributor

It needs to be done as part of 2.0.1, as otherwise it is just using the empty FVK's ovk, which IIRC is the all-zeroes byte string. @daira suggested that we use the FVK of one of the recipients, but that only works if one of the recipients is us, which is not guaranteed. I'm wondering whether we should define a way to derive a synthetic ovk from a transparent spending key, so that the semantics (from corresponds to sender's address) are maintained?

This comment has been minimized.

@bitcartel

bitcartel Aug 31, 2018

Contributor

Filed #3506

This comment has been minimized.

@daira

daira Sep 1, 2018

Contributor

Wait when did I suggest that? I don't remember doing so. I recommend using the FVK of the ZIP 32 account that sent the transfer.

This comment has been minimized.

@str4d

str4d Sep 3, 2018

Contributor

Oh, right - apologies. However, I still am unsure about this; if funds are coming from a t-address, there isn't any ZIP 32 account associated with it by definition (as ZIP 32 does not generate t-addrs). Note that in the case where the funds are coming from a Sapling address, from is set to the corresponding FVK, which will itself correspond to a ZIP 32 account once #3492 is merged (which itself will need to be rebased once this PR merges).

@@ -3639,6 +3638,9 @@ UniValue z_sendmany(const UniValue& params, bool fHelp)
// Keep track of addresses to spot duplicates
set<std::string> setAddress;

// Track whether we see any Sprout addresses
bool noSproutAddrs = fromTaddr || fromSapling;

This comment has been minimized.

@daira

daira Aug 30, 2018

Contributor

This would become:

bool noSproutAddrs = !fromSprout;
@bitcartel
Copy link
Contributor

bitcartel left a comment

Minor comments. Looks good!

@@ -251,6 +249,9 @@ bool AsyncRPCOperation_sendmany::main_impl() {
for (SendManyInputJSOP & t : z_inputs_) {
z_inputs_total += std::get<2>(t);
}
for (auto t : z_sapling_inputs_) {

This comment has been minimized.

@bitcartel

bitcartel Aug 31, 2018

Contributor

We don't allow sprout and sapling notes to be consumed in the same tx. So we could an assert here to check the condition that if z_inputs_ size is > 0, then z_sapling_inputs must be 0, and vice versa.

expsk = sk.expanded_spending_key();
from = expsk.full_viewing_key();
} else {
// TODO: Set "from" to something!

This comment has been minimized.

@bitcartel

bitcartel Aug 31, 2018

Contributor

Filed #3506

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 1, 2018

@daira @Eirik0 Does the latest commit and responses to review comments address your concerns?

@daira

daira approved these changes Sep 1, 2018

Copy link
Contributor

daira left a comment

utACK modulo comments.

@@ -244,6 +249,9 @@ bool AsyncRPCOperation_sendmany::main_impl() {
for (SendManyInputJSOP & t : z_inputs_) {
z_inputs_total += std::get<2>(t);
}
for (auto t : z_sapling_inputs_) {
z_inputs_total += t.note.value();
}

This comment has been minimized.

@daira

daira Sep 1, 2018

Contributor

Am I right in thinking that z_outputs_ can have either Sprout or Sapling recipients (but not a mix of both)? Add a comment explaining that.

This comment has been minimized.

@daira

daira Sep 1, 2018

Contributor

On line 280, change "protected" to "shielded" in the error message.

@@ -244,6 +249,9 @@ bool AsyncRPCOperation_sendmany::main_impl() {
for (SendManyInputJSOP & t : z_inputs_) {

This comment has been minimized.

@daira

daira Sep 1, 2018

Contributor

Please rename z_inputs_ to z_sprout_inputs_.

@@ -354,6 +365,141 @@ bool AsyncRPCOperation_sendmany::main_impl() {
LogPrint("zrpcunsafe", "%s: private output: %s\n", getId(), FormatMoney(z_outputs_total));
LogPrint("zrpc", "%s: fee: %s\n", getId(), FormatMoney(minersFee));


/**
* SCENARIO #0

This comment has been minimized.

@daira

daira Sep 1, 2018

Contributor

Yes but this is almost unreviewable as it stands :-(

if (!testmode) {
UniValue params = UniValue(UniValue::VARR);
params.push_back(signedtxn);
UniValue sendResultValue = sendrawtransaction(params, false);

This comment has been minimized.

@daira

daira Sep 1, 2018

Contributor

I ws temporarily confused because the non-Sapling scenarios use sign_send_raw_transaction. But I guess the builder has already signed it. In any case it's ugly that some code is duplicated between here and sign_send_raw_transaction.

return false;
}

// sort in descending order, so big notes appear first
std::sort(z_inputs_.begin(), z_inputs_.end(), [](SendManyInputJSOP i, SendManyInputJSOP j) -> bool {
return ( std::get<2>(i) > std::get<2>(j));
});
std::sort(z_sapling_inputs_.begin(), z_sapling_inputs_.end(),
[](SaplingNoteEntry i, SaplingNoteEntry j) -> bool {
return ( i.note.value() > j.note.value());

This comment has been minimized.

@daira

daira Sep 1, 2018

Contributor

Nit: odd whitespace usage here and on line 1085. The parens around the comparisons aren't helpful/needed IMHO.


// If we are sending from a shielded address, all recipient
// shielded addresses must be of the same type.
if (!fromTaddr) {

This comment has been minimized.

@daira

daira Sep 1, 2018

Contributor

The if (!fromTaddr) isn't necessary.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 1, 2018

@str4d Can you push a cleanup commit for latest comments from daira? We should be able to merge after that.

str4d added some commits Sep 3, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Sep 3, 2018

Pushed cleanup commits.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 3, 2018

📌 Commit 7c02acc has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 3, 2018

⌛️ Testing commit 7c02acc with merge 5824aca...

zkbot added a commit that referenced this pull request Sep 3, 2018

Auto merge of #3489 - str4d:3215-z_sendmany, r=str4d
Add Sapling support to z_sendmany

Closes #3215.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 3, 2018

💔 Test failed - pr-merge

str4d added some commits Sep 3, 2018

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Sep 3, 2018

Fixed file permissions of the new RPC test, and updated another RPC test after one of @daira's cleanup comments.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 3, 2018

📌 Commit af04224 has been approved by str4d

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 3, 2018

⌛️ Testing commit af04224 with merge edd3216...

zkbot added a commit that referenced this pull request Sep 3, 2018

Auto merge of #3489 - str4d:3215-z_sendmany, r=str4d
Add Sapling support to z_sendmany

Closes #3215.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 3, 2018

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

@zkbot zkbot merged commit af04224 into zcash:master Sep 3, 2018

1 check passed

homu Test successful
Details

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

@str4d str4d deleted the str4d:3215-z_sendmany branch Sep 3, 2018

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