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

Sapling support for z_listunspent #3510

Merged
merged 9 commits into from Oct 4, 2018

Conversation

@arcalinea
Copy link
Contributor

arcalinea commented Sep 6, 2018

Closes #3378.

@arcalinea arcalinea self-assigned this Sep 6, 2018

@arcalinea arcalinea requested a review from LarryRuane Sep 6, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 11, 2018

@LarryRuane What needs to be added to the rpc test? Any blockers? PR might need a rebase too.

@bitcartel bitcartel added this to the v2.0.1 milestone Sep 11, 2018

@bitcartel bitcartel added this to In Progress in Zcashd Team Sep 11, 2018

@LarryRuane

This comment has been minimized.

Copy link
Contributor

LarryRuane commented Sep 11, 2018

@LarryRuane What needs to be added to the rpc test? Any blockers? PR might need a rebase too.

@bitcartel @arcalinea, I rebased to latest master (but not pushed to github yet), so the test can do the (necessary) z_sendmany.

I can't test setting the includeWatchonly flag to true (default is false) post-sapling activation, because we're missing sapling support for z_exportviewingkey (and possibly z_importviewingkey as well). But I would not call this a blocker; we can add testing for that as part of the PR that implements Sapling support for those.

So I think the test is good, but it may be finding an actual pre-Sapling bug relating to z_exportviewingkey or z_importviewingkey (or both; they're used in combination). I need to make sure the test itself isn't incorrect, but it seems that z_listunspent on a node that has imported the viewing key is returning a transaction that has already been spent! (The exporting node correctly does not report that transaction as unspent.)

So I've written a minimal test to demonstrate this bug (if it is a bug). Interestingly, z_gettotalbalance shows a private balance that's definitely too high, but on both nodes (the node that owns the spending keys as well as the node that imported the viewing key).

But I probably should go ahead right now and push (to Jay's branch) the test as it is (cleaned up), which does test Sapling z_listunspent but without testing includeWatchonly. Then it can be reviewed. Does that sound okay?

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 11, 2018

@LarryRuane Go for it and please continue investigation. I'm testing and fiddling with the other PR right now, z_listreceivedbyaddress, and pushed some commits there.

@LarryRuane

This comment has been minimized.

Copy link
Contributor

LarryRuane commented Sep 11, 2018

@LarryRuane Go for it and please continue investigation. I'm testing and fiddling with the other PR right now, z_listreceivedbyaddress, and pushed some commits there.

@bitcartel I just pushed (not forced) my updates to the RPC test (to this PR).

libzcash::SproutPaymentAddress addr = boost::get<libzcash::SproutPaymentAddress>(zaddr);
if (!fIncludeWatchonly && !pwalletMain->HaveSproutSpendingKey(addr)) {
// TODO: Add visitor to handle support for Sprout and Sapling addrs.
if (boost::get<libzcash::SproutPaymentAddress>(&zaddr) != nullptr){

This comment has been minimized.

@LarryRuane

LarryRuane Sep 11, 2018

Contributor

should there be 4-space indenting here (and in other places)?

@bitcartel
Copy link
Contributor

bitcartel left a comment

I can list sapling notes, but a few tweaks needed:

  1. Rebase onto master (I did this locally, you just need some minor fixes)
  2. Change "jsoutindex" to "outindex" for Sapling shielded outputs
  3. When calling z_listunspent 0, the output note has not been confirmed yet, but it still shows as being spendable, even though it cannot be spent until it has been confirmed (I can't recall if this same issue impacts Sprout shielded notes or not)
    "confirmations": 0,
    "spendable": true,
  1. The change flag is always false
@arcalinea

This comment has been minimized.

Copy link
Contributor

arcalinea commented Sep 13, 2018

As for comment #3, the same issue should have affected Sprout txs before as well, because spendable is based on whether the user has the spending key (the alternative being a watch-only address), not whether the tx has been confirmed. Should we modify this behavior so it only displays spendable when the user has the spending key and the tx has >0 confirmations?

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 19, 2018

@arcalinea Thanks, sound like its fine as is if spendable means the user has the spending key.
One outstanding issue is that during a sprint review dicussion of z_listreceivedbyaddress, there was talk of following the output style of z_validateaddress for outindex etc. as it would provide more info about sprout / sapling.

@str4d str4d moved this from In Progress to In Review in Zcashd Team Sep 19, 2018

@arcalinea

This comment has been minimized.

Copy link
Contributor

arcalinea commented Sep 20, 2018

So by adding type is sprout or sapling?

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 21, 2018

@arcalinea I don't think it's essential since it should be obvious from looking at the address. Could be something we add later in a future release.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 21, 2018

I tried testing but ran into errors importing an extended spending key. I think this PR needs a rebase.

@LarryRuane LarryRuane force-pushed the arcalinea:3378_z_listunspent branch from bd603fd to e10ec0f Sep 24, 2018

@LarryRuane

This comment has been minimized.

Copy link
Contributor

LarryRuane commented Sep 24, 2018

@bitcartel I just force-pushed a rebase with zcash master. The new rpc test added as part of this PR, wallet_listnotes.py, passes but I didn't try the full test suite. I don't have try permissions so maybe you can give it a ... er, try.

Once #3499 is merged, we may want to combine their (currently separate) rpc unit tests, or at least make them more consistent.

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Sep 24, 2018

@zkbot try

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 24, 2018

⌛️ Trying commit e10ec0f with merge 3bfadae...

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

Auto merge of #3510 - arcalinea:3378_z_listunspent, r=<try>
Sapling support for z_listunspent

Needs to be rebased on master, after Larry adds rest of the RPC test

Closes #3378.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Sep 25, 2018

☀️ Test successful - pr-try
State: approved= try=True

@@ -2473,7 +2473,8 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
" {\n"
" \"txid\" : \"txid\", (string) the transaction id \n"
" \"jsindex\" : n (numeric) the joinsplit index\n"
" \"jsoutindex\" : n (numeric) the output index of the joinsplit\n"
" \"jsoutindex\" (sprout) : n (numeric) the output index of the joinsplit\n"
" \"outindex\" (sapling) : n (numeric) the output index\n"

This comment has been minimized.

@Eirik0

Eirik0 Sep 26, 2018

Contributor

Do we need to say something to indicate that you will get either one or the other of these two fields?

This comment has been minimized.

@bitcartel

bitcartel Sep 29, 2018

Contributor

Should be fine as is (for now).

@@ -2463,7 +2463,7 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
"1. minconf (numeric, optional, default=1) The minimum confirmations to filter\n"
"2. maxconf (numeric, optional, default=9999999) The maximum confirmations to filter\n"
"3. includeWatchonly (bool, optional, default=false) Also include watchonly addresses (see 'z_importviewingkey')\n"
"4. \"addresses\" (string) A json array of zaddrs to filter on. Duplicate addresses not allowed.\n"
"4. \"addresses\" (string) A json array of zaddrs (both Sprout and Sapling) to filter on. Duplicate addresses not allowed.\n"

This comment has been minimized.

@Eirik0

Eirik0 Sep 26, 2018

Contributor

IMO the default should be both Sprout and Sapling and we should only say something if this is not the case.

assert(boost::get<libzcash::SproutPaymentAddress>(&zaddr) != nullptr);
libzcash::SproutPaymentAddress addr = boost::get<libzcash::SproutPaymentAddress>(zaddr);
if (!fIncludeWatchonly && !pwalletMain->HaveSproutSpendingKey(addr)) {
// TODO: Add visitor to handle support for Sprout and Sapling addrs.

This comment has been minimized.

@Eirik0

Eirik0 Sep 26, 2018

Contributor

In this particular case, this has already been done see class HaveSpendingKeyForPaymentAddress : public boost::static_visitor<bool> in wallet.h It would be good to reuse that below as this could replace this whole if .. else if .. else block.

@@ -2583,7 +2603,27 @@ UniValue z_listunspent(const UniValue& params, bool fHelp)
}
results.push_back(obj);
}

// TODO: Sapling

This comment has been minimized.

@Eirik0

Eirik0 Sep 26, 2018

Contributor

This comment can be removed.

@LarryRuane

This comment has been minimized.

Copy link
Contributor

LarryRuane commented Oct 1, 2018

Can your cleanup commit be backed out and put into a different pull-request?

Yes, I will back it out, give me a minute; I didn't realize use of that visitor had been added. UPDATE: it's now backed out.

@LarryRuane LarryRuane force-pushed the arcalinea:3378_z_listunspent branch from 9448204 to 5f57bab Oct 1, 2018

assert_equal(1, len(unspent_cb))
assert_equal(False, unspent_cb[0]['change'])
assert_equal(txid_1, unspent_cb[0]['txid'])
assert_equal(True, unspent_cb[0]['spendable'])

This comment has been minimized.

@Eirik0

Eirik0 Oct 1, 2018

Contributor

Just to note: There is assert_true and assert_false in util.py

This comment has been minimized.

@LarryRuane

LarryRuane Oct 1, 2018

Contributor

Thanks, didn't know about that. I think in this case it's more consistent to keep it as-is because then all the assertions in this group have the same format: compare an expected value with an actual value. Less thinking for the reader.

obj.push_back(Pair("spendable", hasSaplingSpendingKey));
obj.push_back(Pair("address", EncodePaymentAddress(entry.address)));
obj.push_back(Pair("amount", ValueFromAmount(CAmount(entry.note.value())))); // note.value() is equivalent to plaintext.value()
obj.push_back(Pair("memo", HexStr(entry.memo)));

This comment has been minimized.

@Eirik0

Eirik0 Oct 1, 2018

Contributor

for sprout we do:

std::string data(entry.plaintext.memo().begin(), entry.plaintext.memo().end());
obj.push_back(Pair("memo", HexStr(data)));

Sapling note entry memo is defined as std::array<unsigned char, ZC_MEMO_SIZE> memo;
Sprout note entry plain plaintext memo is defined as std::array<unsigned char, ZC_MEMO_SIZE> memo_;

In sapling we just pass in the memo without making a string of it. Is this correct?

This comment has been minimized.

@bitcartel

bitcartel Oct 2, 2018

Contributor

Yes, should be correct with the end result being the same. The Sprout code path could be simplified with HexStr(entry.plaintext.memo()):

inline std::string HexStr(const T& vch, bool fSpaces=false)
which iterates over the array
return HexStr(vch.begin(), vch.end(), fSpaces);

# but this requires Sapling support for those RPCs

if __name__ == '__main__':
WalletListNotes().main()

This comment has been minimized.

@Eirik0

Eirik0 Oct 1, 2018

Contributor

My comment on this test is that we seem to be testing a lot of the sprout logic (Which I would expect to be tested elsewhere). We do not test the sapling note change logic (but I think this is tested as part of the update to z_listreceivedbyarrdess), we do not test that we use outindex rather than jsindex/jsoutindex, and we do not test the memo field. These are the differences between sprout/sapling, and therefore, I was expecting them to be tested.

That being said, in the cpp file these look good. Please look at the comment regarding the memo and once that is verified this should be ackable.

This comment has been minimized.

@bitcartel

bitcartel Oct 4, 2018

Contributor

Filed #3562

@Eirik0

Eirik0 approved these changes Oct 2, 2018

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Oct 2, 2018

utACK

@mdr0id mdr0id self-requested a review Oct 3, 2018

@mdr0id
Copy link
Contributor

mdr0id left a comment

NACK per this comment:

@arcalinea & @LarryRuane Has it been ensured that other tests exercise this new Sapling flow, if it is needed?

Specifically:

BOOST_AUTO_TEST_CASE(rpc_z_listunspent_parameters)
wallet_protectcoinbase.py

@Eirik0

This comment has been minimized.

Copy link
Contributor

Eirik0 commented Oct 3, 2018

In regards to BOOST_AUTO_TEST_CASE(rpc_z_listunspent_parameters), the parameters have not changed. We could add some lines to get a new sapling address and then test that we do not throw when we pass that in, but that should also be covered by the python rpc test and indirectly by other tests which will end up relying on IsValidPaymentAddress(...). I am not sure that we should update wallet_protectcoinbase.py because we can test the listunspent logic independently of whatever that test is doing.

@mdr0id

mdr0id approved these changes Oct 4, 2018

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 4, 2018

@Eirik0 Filed #3563

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 4, 2018

@LarryRuane Did you put your improvements into a new PR so we don't forget?

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 4, 2018

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 4, 2018

📌 Commit 5f57bab has been approved by bitcartel

@bitcartel bitcartel moved this from In Review to Merge Queue in Zcashd Team Oct 4, 2018

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 4, 2018

⌛️ Testing commit 5f57bab with merge 38446a4...

zkbot added a commit that referenced this pull request Oct 4, 2018

Auto merge of #3510 - arcalinea:3378_z_listunspent, r=bitcartel
Sapling support for z_listunspent

Needs to be rebased on master, after Larry adds rest of the RPC test

Closes #3378.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 4, 2018

💔 Test failed - pr-merge

@str4d

This comment has been minimized.

Copy link
Contributor

str4d commented Oct 4, 2018

Two test failures:

test/rpc_wallet_tests.cpp(1445): error: in "rpc_wallet_tests/rpc_z_listunspent_parameters": exception runtime_error expected but not raised
=== Running testscript wallet_protectcoinbase.py ===
  File "/home/admin/latent/debian8-kitchensink/build/qa/rpc-tests/test_framework/test_framework.py", line 121, in main
    self.run_test()
  File "/home/admin/latent/debian8-kitchensink/build/qa/rpc-tests/wallet_protectcoinbase.py", line 150, in run_test
    assert_equal("Invalid parameter, spending key for address does not belong to wallet" in errorString, True)
  File "/home/admin/latent/debian8-kitchensink/build/qa/rpc-tests/test_framework/util.py", line 362, in assert_equal
    raise AssertionError("%sexpected: <%s> but was: <%s>" % (message, str(expected), str(actual)))
Initializing test directory /tmp/test2K2L8j
Mining blocks...
waiting for async operation opid-1d9df2b1-319c-427c-a8b5-3044fa836d7f
waiting for async operation opid-58ea0638-448c-4b30-b686-5910e3e441e5
waiting for async operation opid-fc6ec97a-a83a-4d57-ae84-ee6a31431aa0
Assertion failed: expected: <False> but was: <True>
Stopping nodes
Cleaning up
Failed
!!! FAIL: wallet_protectcoinbase.py !!!

@str4d str4d moved this from Merge Queue to In Progress in Zcashd Team Oct 4, 2018

@LarryRuane

This comment has been minimized.

Copy link
Contributor

LarryRuane commented Oct 4, 2018

I'm taking a look at those failures

@bitcartel

This comment has been minimized.

Copy link
Contributor

bitcartel commented Oct 4, 2018

ACK on the fix commit. Also @LarryRuane has locally verified that the qa tests pass with the fix above.

@zkbot r+

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 4, 2018

📌 Commit 27a6a99 has been approved by bitcartel

@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 4, 2018

⌛️ Testing commit 27a6a99 with merge 4ba3555...

zkbot added a commit that referenced this pull request Oct 4, 2018

Auto merge of #3510 - arcalinea:3378_z_listunspent, r=bitcartel
Sapling support for z_listunspent

Closes #3378.
@zkbot

This comment has been minimized.

Copy link
Collaborator

zkbot commented Oct 4, 2018

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 4ba3555 to master...

@zkbot zkbot merged commit 27a6a99 into zcash:master Oct 4, 2018

1 check passed

homu Test successful
Details

Zcashd Team automation moved this from In Progress to Released (Merged in Master) Oct 4, 2018

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