Closes #2910. Add z_listunspent RPC call. #2913

bitcartel · 2018-01-27T07:03:08Z

Have tested on network. Opening up for review.

bitcartel · 2018-01-27T07:04:03Z

This PR is helpful in debugging the merging of notes in PR #2797. At the time of writing, you can cherry-pick / rebase ths PR on top of #2797 without any conflict.

Output looks like this:

./zcash-cli z_listunspent
[
  {
    "txid": "abcd...",
    "js_index": 0,
    "output_index": 1,
    "confirmations": 33,
    "address": "zcABCD...",
    "amount": 0.0000xxxx,
    "memo": "beefbeefbeefbeef000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000000"
  }

bitcartel · 2018-02-22T23:47:40Z

I'd really like to get this into 1.0.15 (time-permitting) because it complements z_mergetoaddress (for example, a user doesn't know which zaddr has many notes and is a good candidate for consolidation) and users have requested both features. Can you review @arcalinea and @arielg? The PR is pretty much self-contained, adding a new RPC call, and should not change existing behaviour.

arcalinea

ACK, pending 1 minor change. I tried the cli interface out and it looks good. Also ran the tests.

arcalinea · 2018-03-13T21:00:32Z

src/test/rpc_wallet_tests.cpp

+    // minconf must be >= 0
+    BOOST_CHECK_THROW(CallRPC("z_listunspent -1"), runtime_error);
+
+    // maxconf must be < minconf


maxconf must be > minconf

Pushed update to fix this.

bitcartel · 2018-03-14T22:43:47Z

Self-reminder:

Check behaviour with viewing keys

bitcartel · 2018-03-16T04:50:39Z

@arcalinea I added support for viewing keys in a second commit which needs review. Thx.

bitcartel · 2018-03-16T06:07:34Z

Added @braddmiller as reviewer, related to locking notes, as we may need a z_listlockunspent RPC call for notes to mirror listlockunspent for utxos.

braddmiller

Tested ACK. A few non-blocking comments and questions

braddmiller · 2018-03-21T16:13:03Z

src/wallet/rpcwallet.cpp

+        throw JSONRPCError(RPC_INVALID_PARAMETER, "Maximum number of confirmations must be greater or equal to the minimum number of confirmations");
+    }
+
+    std::set<libzcash::PaymentAddress> zaddrs = {};


already using namespace std, just a non-blocking consistency comment. Future instances of set don't specify the namespace.

braddmiller · 2018-03-21T17:48:19Z

src/test/rpc_wallet_tests.cpp

+    BOOST_CHECK_NO_THROW(CallRPC("z_listunspent 1 999 true [\"ztjiDe569DPNbyTE6TSdJTaSDhoXEHLGvYoUnBU1wfVNU52TEyT6berYtySkd21njAeEoh8fFJUT42kua9r8EnhBaEKqCpP\"]"));
+
+    // wrong network, mainnet instead of testnet
+    BOOST_CHECK_THROW(CallRPC("z_listunspent 1 999 true [\"zcMuhvq8sEkHALuSU2i4NbNQxshSAYrpCExec45ZjtivYPbuiFPwk6WHy4SvsbeZ4siy1WheuRGjtaJmoD1J8bFqNXhsG6U\"]"), runtime_error);


I'm not clear on the purpose of this test.

This is just testing that you can't pass in a valid address for a different network. The test case is running on testnet, so we throw it a real mainnet address.

braddmiller · 2018-03-21T18:05:26Z

qa/rpc-tests/wallet_protectcoinbase.py

+        assert(len(results) == 1)
+        assert_equal(results[0]["address"], myzaddr)
+        assert_equal(results[0]["amount"], shieldvalue)
+        assert_equal(results[0]["confirmations"], 0)


Should we check assert_equal(results[0]["spendable"], True) here with zero confirmations?

Sure, I've added this, with a comment to clarify the note is unspent (which means its spendable) but it does need 1 confirmation before it can actually be spent.

Why should "spendable" be true if the note has zero confirmations?

Oh, because "spendable" is a misnomer; this field is really designed to distinguish whether the wallet holds a spending key for the address. Let's make sure this is clearly documented in the RPC help.

arcalinea · 2018-03-22T20:20:36Z

wallet_protectcoinbase.py fails on line 396 wait_and_assert_operationid_status for me, the status is not being returned. Still trying to figure out why...

leto · 2018-03-23T07:08:56Z

src/wallet/rpcwallet.cpp

+            "    \"js_index\" : n          (numeric) the joinsplit index\n"
+            "    \"output_index\" : n      (numeric) the output index of the joinsplit\n"
+            "    \"confirmations\" : n     (numeric) the number of confirmations\n"
+            "    \"spendable\" : true|false  (boolean) true if note is spendable, fale if watchonly\n"


s/fale/false/

Document that when minconf=0, unspent notes with zero confirmations are included even though they aren't immediately spendable. Or possibly change the field name — either to "have_spending_key", or to "watchonly" reversing the sense.

leto · 2018-03-23T07:12:57Z

src/wallet/rpcwallet.cpp

+            "\nExamples\n"
+            + HelpExampleCli("z_listunspent", "")
+            + HelpExampleCli("z_listunspent", "6 9999999 \"[\\\"ztbx5DLDxa5ZLFTchHhoPNkKs57QzSyib6UqXpEdy76T1aUdFxJt1w9318Z8DJ73XzbnWHKEZP9Yjg712N5kMmP4QzS9iC9\\\",\\\"ztfaW34Gj9FrnGUEf833ywDVL62NWXBM81u6EQnM6VR45eYnXhwztecW1SjxA7JrmAXKJhxhj3vDNEpVCQoSvVoSpmbhtjf\\\"]\"")
+            + HelpExampleRpc("z_listunspent", "6, 9999999 \"[\\\"ztbx5DLDxa5ZLFTchHhoPNkKs57QzSyib6UqXpEdy76T1aUdFxJt1w9318Z8DJ73XzbnWHKEZP9Yjg712N5kMmP4QzS9iC9\\\",\\\"ztfaW34Gj9FrnGUEf833ywDVL62NWXBM81u6EQnM6VR45eYnXhwztecW1SjxA7JrmAXKJhxhj3vDNEpVCQoSvVoSpmbhtjf\\\"]\"")


Above 2 examples are missing a false for watchonly flag:

+ HelpExampleCli("z_listunspent", "6 9999999 false \"[\\\"ztbx5DLDxa5ZLFTchHhoPNkKs57QzSyib6UqXpEdy76T1aUdFxJt1w9318Z8DJ73XzbnWHKEZP9Yjg712N5kMmP4QzS9iC9\\\",\\\"ztfaW34Gj9FrnGUEf833ywDVL62NWXBM81u6EQnM6VR45eYnXhwztecW1SjxA7JrmAXKJhxhj3vDNEpVCQoSvVoSpmbhtjf\\\"]\"") + HelpExampleRpc("z_listunspent", "6, 9999999 false \"[\\\"ztbx5DLDxa5ZLFTchHhoPNkKs57QzSyib6UqXpEdy76T1aUdFxJt1w9318Z8DJ73XzbnWHKEZP9Yjg712N5kMmP4QzS9iC9\\\",\\\"ztfaW34Gj9FrnGUEf833ywDVL62NWXBM81u6EQnM6VR45eYnXhwztecW1SjxA7JrmAXKJhxhj3vDNEpVCQoSvVoSpmbhtjf\\\"]\"")

leto · 2018-03-23T07:54:54Z

This has been successfully ported to Hush: MyHush/hush#121 💯

One question I have is about amount=0 notes, which HushList uses. Should they be in the output of z_listunspent ?

I look forward to using z_listunspent and it's improved ease + performance over current methods to get at various data.

daira · 2018-03-25T12:15:07Z

Reviewing now.

daira

See comments.

daira · 2018-03-25T12:18:04Z

qa/rpc-tests/wallet_protectcoinbase.py

        # This send will succeed.  We send two coinbase utxos totalling 20.0 less a fee of 0.00010000, with no change.
        shieldvalue = Decimal('20.0') - Decimal('0.0001')
        recipients = []
        recipients.append({"address":myzaddr, "amount": shieldvalue})
        myopid = self.nodes[0].z_sendmany(mytaddr, recipients)
        mytxid = wait_and_assert_operationid_status(self.nodes[0], myopid)
        self.sync_all()
+
+        # Verify that z_listunspent can return a note which has zero confirmations


"a note that has zero confirmations"

(having zero confirmations is a restrictive qualifier on the note)

daira · 2018-03-25T12:22:28Z

qa/rpc-tests/wallet_protectcoinbase.py

+        assert(len(results) == 1)
+        assert_equal(results[0]["address"], myzaddr)
+        assert_equal(results[0]["amount"], shieldvalue)
+        assert_equal(results[0]["confirmations"], 0)


Why should "spendable" be true if the note has zero confirmations?

Oh, because "spendable" is a misnomer; this field is really designed to distinguish whether the wallet holds a spending key for the address. Let's make sure this is clearly documented in the RPC help.

daira · 2018-03-25T12:25:19Z

qa/rpc-tests/wallet_protectcoinbase.py

+        notes = self.nodes[2].z_listunspent()
+        sum_of_notes = 0
+        for note in notes:
+            sum_of_notes += note["amount"]


This can be written as

sum_of_notes = sum([note["amount"] for note in notes])

(also below).

daira · 2018-03-25T12:31:40Z

src/wallet/rpcwallet.cpp

+        throw runtime_error(
+            "z_listunspent ( minconf maxconf includeWatchonly [\"zaddr\",...] )\n"
+            "\nReturns array of unspent shielded notes with between minconf and maxconf (inclusive) confirmations.\n"
+            "Optionally filter to only include notes sent to specified addresses.\n"


Document that addresses may not be duplicated (or remove that restriction).

It would make more sense to me to just implicitly deduplicate, unless that's inconsistent with other RPC methods.

Other RPC methods return a duplicated address error message so following that.

daira · 2018-03-25T12:40:28Z

src/wallet/rpcwallet.cpp

+            "    \"js_index\" : n          (numeric) the joinsplit index\n"
+            "    \"output_index\" : n      (numeric) the output index of the joinsplit\n"
+            "    \"confirmations\" : n     (numeric) the number of confirmations\n"
+            "    \"spendable\" : true|false  (boolean) true if note is spendable, fale if watchonly\n"


Document that when minconf=0, unspent notes with zero confirmations are included even though they aren't immediately spendable. Or possibly change the field name — either to "have_spending_key", or to "watchonly" reversing the sense.

daira · 2018-03-25T15:09:03Z

src/wallet/wallet.cpp

+    std::set<PaymentAddress>& filterAddresses,
+    int minDepth,
+    int maxDepth,
+    bool ignoreUnspendable)


Rename this parameter to requireSpendingKey or similar.

daira · 2018-03-25T15:09:48Z

src/wallet/wallet.cpp

+        CWalletTx wtx = p.second;
+
+        // Filter the transactions before checking for notes
+        if (!CheckFinalTx(wtx) || wtx.GetBlocksToMaturity() > 0 || wtx.GetDepthInMainChain() < minDepth || wtx.GetDepthInMainChain() > maxDepth ) {


Nitpick: remove space before closing paren.

daira · 2018-03-25T15:14:17Z

src/wallet/rpcwallet.cpp

+        }
+    }
+    else {
+        // User did not provide zaddrs, so use default i.e. all addresses


GetUnspentFilteredNotes supports the unfiltered case without having to enumerate all addresses; use that support.

daira · 2018-03-25T15:31:24Z

src/wallet/wallet.cpp

+            PaymentAddress pa = nd.address;
+
+            // skip notes which belong to a different payment address in the wallet
+            if (!(filterAddresses.empty() || filterAddresses.count(pa))) {


It seems like a questionable API to have the empty set of addresses mean "don't filter by address"; I would expect that not to be treated as a special case and to just return no results. const std::set<PaymentAddress> * with nullptr meaning "don't filter", or boost::optional<const std::set<PaymentAddress>&> (but note portability issues), seem like more appropriate types.

This approach has been used in other related methods; so beyond scope to change in this PR, but have filed ticket: #3118

daira · 2018-03-25T15:35:14Z

src/wallet/wallet.cpp

+            } catch (const note_decryption_failed &err) {
+                // Couldn't decrypt with this spending key
+                throw std::runtime_error(strprintf("Could not decrypt note for payment address %s", CZCPaymentAddress(pa).ToString()));
+            } catch (const std::exception &exc) {


Can this be narrowed? (I'm concerned about losing stack trace information by rethrowing.)

Not sure. Won't change for now since its the same for GetFilteredNotes

daira · 2018-03-25T22:51:47Z

@leto wrote:

One question I have is about amount=0 notes, which HushList uses. Should they be in the output of z_listunspent ?

Maybe an optional parameter to filter the minimum value of the notes returned?

leto · 2018-03-26T02:19:54Z

@daira I would definitely find it useful to have an optional parameter that filters out amount=0 notes, which are technically "can be spent" but which most wallet software would like to filter out. In particular, Hush uses many amount=0 notes and this proposed parameter would prevent our frontend from having to process and essentially ignore a lot of data

bitcartel · 2018-03-26T22:31:01Z

Filed #3119 for optional parameter to filter based on note value.

bitcartel · 2018-03-26T22:39:47Z

@daira Force pushed to address review comments. Squashed the two commits into one.

daira

utACK

bitcartel · 2018-03-27T22:12:07Z

Two ACKs and ported to Hush by @leto (might want to backport some of the latest tweaks and fixed)

@zkbot r+

zkbot · 2018-03-27T22:12:08Z

📌 Commit f083d15 has been approved by bitcartel

zkbot · 2018-03-27T22:12:15Z

⌛ Testing commit f083d1510f5cf26996e950dcf3bf3cffbabc013d with merge 262ff6c7e8c2afeffcc4e911759b2bbf23062e1f...

zkbot · 2018-03-27T23:41:25Z

💔 Test failed - pr-merge

str4d · 2018-03-28T12:33:01Z

PyFlakes warnings 😄

bitcartel · 2018-03-28T17:40:47Z

Pushed pyflakes fix. @zkbot r+

zkbot · 2018-03-28T17:40:47Z

📌 Commit d72c19a has been approved by bitcartel

zkbot · 2018-03-28T17:40:52Z

⌛ Testing commit d72c19a with merge 5bdce01...

Closes #2910. Add z_listunspent RPC call. Have tested on network. Opening up for review.

zkbot · 2018-03-28T19:13:14Z

☀️ Test successful - pr-merge
Approved by: bitcartel
Pushing 5bdce01 to master...

bitcartel added A-wallet Area: Wallet A-rpc-interface Area: RPC interface labels Jan 27, 2018

bitcartel added this to the 1.0.15 milestone Jan 27, 2018

bitcartel self-assigned this Jan 27, 2018

bitcartel requested review from str4d and arcalinea January 27, 2018 07:03

bitcartel changed the title ~~Closes #2910. Add z_listunspent RPC call.~~ [WIP] Closes #2910. Add z_listunspent RPC call. Jan 27, 2018

bitcartel changed the title ~~[WIP] Closes #2910. Add z_listunspent RPC call.~~ Closes #2910. Add z_listunspent RPC call. Jan 27, 2018

bitcartel mentioned this pull request Jan 27, 2018

Implement z_mergetoaddress for combining UTXOs and notes #2797

Merged

bitcartel force-pushed the 2910_z_listunspent branch 2 times, most recently from c95d3a0 to bbfa26f Compare January 29, 2018 20:52

str4d modified the milestones: v1.0.15, v1.1.0 Feb 21, 2018

bitcartel force-pushed the 2910_z_listunspent branch from bbfa26f to 09f0f95 Compare February 23, 2018 07:29

bitcartel requested a review from arielgabizon February 23, 2018 07:30

arcalinea reviewed Mar 13, 2018

View reviewed changes

bitcartel force-pushed the 2910_z_listunspent branch from 09f0f95 to 35c4f2d Compare March 14, 2018 18:54

bitcartel requested review from braddmiller and removed request for arielgabizon March 14, 2018 18:56

bitcartel force-pushed the 2910_z_listunspent branch from 35c4f2d to 140f85f Compare March 14, 2018 23:52

braddmiller approved these changes Mar 21, 2018

View reviewed changes

bitcartel force-pushed the 2910_z_listunspent branch from 3603748 to 9ac72ae Compare March 21, 2018 22:20

leto reviewed Mar 23, 2018

View reviewed changes

daira requested changes Mar 25, 2018

View reviewed changes

This was referenced Mar 26, 2018

Consider changing API of GetFilteredNotes in wallet.cpp #3118

Open

Add optional parameter to filter result of z_listunspent by minimum value of note #3119

Open

bitcartel force-pushed the 2910_z_listunspent branch from 9ac72ae to f083d15 Compare March 26, 2018 22:39

daira approved these changes Mar 26, 2018

View reviewed changes

bitcartel removed the request for review from str4d March 27, 2018 05:37

Closes zcash#2910. Add z_listunspent RPC call.

d72c19a

bitcartel force-pushed the 2910_z_listunspent branch from f083d15 to d72c19a Compare March 28, 2018 17:39

zkbot added a commit that referenced this pull request Mar 28, 2018

Auto merge of #2913 - bitcartel:2910_z_listunspent, r=bitcartel

5bdce01

Closes #2910. Add z_listunspent RPC call. Have tested on network. Opening up for review.

zkbot merged commit d72c19a into zcash:master Mar 28, 2018

zkbot mentioned this pull request Mar 28, 2018

Bitcoin Core refactoring and cleanups 1 #2912

Merged

daira added this to 1.1.0: Wallet & DB in Release planning Mar 31, 2018

oxarbitrage mentioned this pull request Feb 9, 2020

GetFilteredNotes argument change to pointer #4336

Closed

cronicc mentioned this pull request Apr 4, 2022

Backport upstream improvements HorizenOfficial/zen#446

Merged

Closes #2910. Add z_listunspent RPC call. #2913

Closes #2910. Add z_listunspent RPC call. #2913

Conversation

bitcartel commented Jan 27, 2018 • edited

bitcartel commented Jan 27, 2018

bitcartel commented Feb 22, 2018 • edited

arcalinea left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bitcartel commented Mar 14, 2018 • edited

bitcartel commented Mar 16, 2018

bitcartel commented Mar 16, 2018

braddmiller left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

arcalinea commented Mar 22, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

leto commented Mar 23, 2018

daira commented Mar 25, 2018

daira left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

daira commented Mar 25, 2018

leto commented Mar 26, 2018

bitcartel commented Mar 26, 2018

bitcartel commented Mar 26, 2018

daira left a comment

Choose a reason for hiding this comment

bitcartel commented Mar 27, 2018

zkbot commented Mar 27, 2018

zkbot commented Mar 27, 2018

zkbot commented Mar 27, 2018

str4d commented Mar 28, 2018

bitcartel commented Mar 28, 2018

zkbot commented Mar 28, 2018

zkbot commented Mar 28, 2018

zkbot commented Mar 28, 2018

bitcartel commented Jan 27, 2018 •

edited

bitcartel commented Feb 22, 2018 •

edited

bitcartel commented Mar 14, 2018 •

edited