Add query options to listunspent RPC call #8952

pedrobranco · 2016-10-18T11:34:25Z

This PR adds a new optional JSON options for listunspent RPC call:

Return unspents greater or equal than a specific amount in BTC: minimumAmount (default = 0).
Return unspents lower or equal than a specific amount in BTC: maximumAmount (default = MAX_MONEY = 21000000.0 BTC).
Return unspents with a total number lower or equal than a specific number: maximumCount (default=0=unlimited).
Return unspents which total is greater or equal than a specific amount in BTC: minimumSumAmount (default = MAX_MONEY = 21000000.0 BTC).

> bitcoin-cli help listunspent
...
5. query options    (json, optional) JSON with query options
    {
      "minimumAmount"    (numeric or string, default=0) Minimum value of each UTXO in BTC
      "maximumAmount"    (numeric or string, default=21000000=unlimited) Maximum value of each UTXO in BTC
      "maximumCount"     (numeric or string, default=0=unlimited) Maximum number of UTXOs
      "minimumSumAmount" (numeric or string, default=21000000=unlimited) Minimum sum value all UTXOs in BTC
    }
...

When selecting unspents on client side in a wallet with a large set of small unspents makes the call listunspent slow. Using with minimumAmount option could improve the performance of the RPC call, performance which depends on the size of wallet, the distribution of the unspents and the minimumAmount selected.

Example:

> bitcoin-cli listunspent 0 9999999 '[]' true '{ "minimumAmount": 10, "maximumAmount": 80, "minimumSumAmount": 50, "maximumCount": 3 }'

Note: this PR also changes/simplifies the code in AvailableCoins to make the longest calls later in execution time, which also could improve performance in some listunspent results.

laanwj · 2016-10-19T14:12:29Z

Concept ACK

pedrobranco · 2016-10-19T14:24:22Z

WDYT of adding new options like maximumAmount (which ables to define a range with minimumAmount) or maybe sumAmount (return unspents until the respective sum is bigger than sumAmount) ?

laanwj · 2016-10-19T15:39:23Z

Yes, would make sense. Passing the JSON "query object" is a good idea.

pedrobranco · 2016-10-19T17:01:52Z

~~I'm willing to implement~~ I've also implemented maximumAmount, minimumSumAmount and maximumCount options.

minimumAmount and maximumAmount are options to filter unspents, but maximumCount and minimumSumAmount are grouping options which returns unspents until satisfies one of these conditions.

Github-Pull: bitcoin#8952 Rebased-From: 98d0a6fb81b33a2cef429d3d6f9d56dd47508076

luke-jr

utACK code, one nit.

luke-jr · 2017-02-03T05:31:12Z

src/wallet/rpcwallet.cpp

Instead of the dual meanings, just say default=unlimited (and make sure 0 doesn't behave in such a manner).

pedrobranco · 2017-02-07T14:11:46Z

@laanwj @luke-jr Is this PR mergeable as is?

laanwj · 2017-02-07T14:39:49Z

Looks like it! but as we're past the feature freeze for 0.14 and this is a new feature, merging this will have to wait until 0.14 is branched off.

pedrobranco · 2017-02-07T14:56:22Z

About tests: there aren't any tests specifically for testing listunspent options. Should we now create them?

laanwj · 2017-02-07T15:12:11Z

Yes, tests for that would be great. They'd make it possible to automatically detect when something breaks this functionality.

kallewoof · 2017-02-28T19:52:30Z

src/wallet/wallet.cpp

+                    nTotal += pcoin->tx->vout[i].nValue;
+
+                    if (nTotal >= nMinimumSumAmount) {
+                        return;


Maybe I'm misreading, but this (nMinimumSumAmount) sounds like a maximum, not a minimum. Esp. since it defaults to MAX_MONEY rather than 0.

I see no issue here since is to be used like it was a minorant set of unspents which satisfies the passed value. If It it was called maximum it should not retrieve unspents which sum are greater than the passed value.

You are collecting the sum of the nValue and once the total exceeds the minimum you stop. Generally speaking,

int minimumSum = 5; int values[] = {1,2,3,2,3}; int sum = 0; for (int i = 0; i < 5; i++) { sum += values[i]; if (sum >= minimumSum) break; }

The above looks inverted to me, and will no doubt confuse the user. If you used the word minorant, it might help, but the minorant would be the vout set not the sum, I believe?

Anyway, if I'm the only one bothered by this feel free to keep as is.

@kallewoof Any suggestions? nTargetSumAmount?

@pedrobranco I say keep it as it is if no one else complains.

kallewoof · 2017-02-28T19:53:37Z

src/wallet/rpcwallet.cpp

+            "      \"minimumAmount\"    (numeric or string, default=0) Minimum value of each UTXO in " + CURRENCY_UNIT + "\n"
+            "      \"maximumAmount\"    (numeric or string, default=unlimited) Maximum value of each UTXO in " + CURRENCY_UNIT + "\n"
+            "      \"maximumCount\"     (numeric or string, default=unlimited) Maximum number of UTXOs\n"
+            "      \"minimumSumAmount\" (numeric or string, default=unlimited) Minimum sum value all UTXOs in " + CURRENCY_UNIT + "\n"


Typo: [...]Minimum sum value of all UTXOs in " (missing of).

kallewoof · 2017-02-28T19:55:31Z

src/wallet/rpcwallet.cpp

            "\nReturns array of unspent transaction outputs\n"
            "with between minconf and maxconf (inclusive) confirmations.\n"
            "Optionally filter to only include txouts paid to specified addresses.\n"
            "\nArguments:\n"


Need to add options to the first line, e.g.

"listunspent ( minconf maxconf [\"addresses\",...] [include_unsafe] [options] )\n"

pedrobranco · 2017-03-07T18:01:37Z

Yes, tests for that would be great. They'd make it possible to automatically detect when something breaks this functionality.

@laanwj I'm already working on it. Can I submit in another PR?

fanquake · 2017-04-02T07:06:29Z

This needs a rebase. @pedrobranco Tests could be submitted in a follow-up PR.

laanwj · 2017-05-17T07:12:59Z

Merged (and rebased) via 9390845

pedrobranco · 2017-05-17T13:41:34Z

Thank you @laanwj 👍

sipa · 2017-05-18T00:38:15Z

@laanwj I suppose you forgot to close after merging?

laanwj · 2017-05-18T08:31:13Z

Yes, did'nt automatically happen as I did a rebase locally. Thanks

…

On May 18, 2017 2:38 AM, "Pieter Wuille" ***@***.***> wrote: @laanwj <https://github.com/laanwj> I suppose you forgot to close after merging? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8952 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AAHutptwk3F7P5M0nGbOo-1CacVVlXtZks5r65L5gaJpZM4KZrh9> .

fedelcs · 2018-07-12T22:46:53Z

@pedrobranco I've been trying to use the minimumSumAmount along with the addresses parameter, and I keep getting fewer outputs than I should.

Looking at the code it seems that the minimumSumAmount is applied first, finding a correct set of outputs, but then when the addresses/destinations are checked, some of those are removed.

Is this the expected behavior? Shouldn't this be, first get all UTXO's that belong to addresses and then apply the filters?

promag · 2018-07-13T06:06:18Z

Looks like a bug.

promag · 2018-07-15T16:32:30Z

@fedelcs IIRC minimumSumAmount was added to allow early returns in the coin selection. Note that the problem only exists when both args are used.

To fix this we should either:

push down address filtering to CWallet::AvailableCoins
truncate the result once the minimumSumAmount is achieved.

RussianSwiss · 2018-07-20T23:25:21Z

Need to have a feature show all addresses' UTXO set with non-zero balance (not only own addresses like listunspent) in Bitcoin Core as a build-in feature.

bc63d0e Add query options to listunspent rpc call (Pedro Branco) Tree-SHA512: 2d296eee8df4e7ac378206ac3003a300e6478502d4b814f1ed1a47614222b01cc35dba871345ced68629860c227aff2c9e4b7f0d4ed0aa7de8b04f26c983580f Signed-off-by: Pasta <[email protected]> # Conflicts: # src/rpc/client.cpp # src/wallet/rpcwallet.cpp # src/wallet/wallet.cpp # src/wallet/wallet.h

063abcf [Doc] Add query_options listunspent parameter to release notes (random-zebra) 602f622 [QA] Excercise query_options parameter in listunspent (random-zebra) 93d4789 [RPC] Backport RPCTypeCheckArgument function and use it inside listunspent (furszy) c073ef2 [RPC] Add query options to listunspent rpc call. (furszy) Pull request description: Adaptation of bitcoin#8952 This PR adds a new optional JSON options for `listunspent` RPC call: * Return unspents greater or equal than a specific amount in PIV: minimumAmount. * Return unspents lower or equal than a specific amount in PIV: maximumAmount. * Return unspents with a total number lower or equal than a specific number: maximumCount. * Return unspents which total is greater or equal than a specific amount in PIV: minimumSumAmount. ``` > pivx-cli help listunspent ... 5. query options (json, optional) JSON with query options { "minimumAmount" (numeric or string, default=0) Minimum value of each UTXO in PIV "maximumAmount" (numeric or string, default=unlimited) Maximum value of each UTXO in PIV "maximumCount" (numeric or string, default=0=unlimited) Maximum number of UTXOs "minimumSumAmount" (numeric or string, default=unlimited) Minimum sum value all UTXOs in PIV } ... ``` ACKs for top commit: random-zebra: ACK 063abcf Fuzzbawls: ACK 063abcf Tree-SHA512: 0a54636657be5e366ddfff374cacb2cac9e7667af99e67d96dfefc3255a06d431dfb75539c90af8727d97697933042cd19ddf968894e655a5fd20a7b6eaeb5aa

pedrobranco force-pushed the enhancement/improve-rpc-listunspent branch from 57b9e76 to 3f89e80 Compare October 18, 2016 11:46

laanwj added the RPC/REST/ZMQ label Oct 18, 2016

pedrobranco force-pushed the enhancement/improve-rpc-listunspent branch 3 times, most recently from e1881d9 to 2e423ea Compare October 19, 2016 11:50

pedrobranco force-pushed the enhancement/improve-rpc-listunspent branch from 2e423ea to 5cf4057 Compare October 19, 2016 14:36

pedrobranco force-pushed the enhancement/improve-rpc-listunspent branch from 5cf4057 to 6660c22 Compare October 19, 2016 18:06

pedrobranco changed the title ~~Add selection options to listunspent RPC call~~ Add query options to listunspent RPC call Oct 19, 2016

pedrobranco force-pushed the enhancement/improve-rpc-listunspent branch from 6660c22 to 98d0a6f Compare October 19, 2016 18:12

luke-jr pushed a commit to bitcoinknots/bitcoin that referenced this pull request Dec 21, 2016

Add query options to listunspent rpc call

ed5bc96

Github-Pull: bitcoin#8952 Rebased-From: 98d0a6fb81b33a2cef429d3d6f9d56dd47508076

luke-jr suggested changes Feb 3, 2017

View reviewed changes

pedrobranco force-pushed the enhancement/improve-rpc-listunspent branch from 98d0a6f to ae6a0c0 Compare February 3, 2017 18:05

kallewoof reviewed Feb 28, 2017

View reviewed changes

pedrobranco force-pushed the enhancement/improve-rpc-listunspent branch 4 times, most recently from bba51b1 to c03488f Compare March 8, 2017 11:52

pedrobranco force-pushed the enhancement/improve-rpc-listunspent branch from c03488f to 4f6566d Compare April 4, 2017 19:06

Add query options to listunspent rpc call

11ee5ec

pedrobranco force-pushed the enhancement/improve-rpc-listunspent branch from 4f6566d to 11ee5ec Compare April 5, 2017 00:08

laanwj mentioned this pull request May 17, 2017

TODO for release notes 0.15.0 #9889

Closed

12 tasks

ryanofsky mentioned this pull request May 17, 2017

[qt] Move some WalletModel functions into CWallet #10295

Merged

sipa closed this May 18, 2017

jonasschnelli mentioned this pull request Jun 1, 2017

listunspent is really slow #10492

Closed

pedrobranco deleted the enhancement/improve-rpc-listunspent branch June 5, 2017 15:42

furszy mentioned this pull request Apr 15, 2021

[RPC] Add query options to listunspent rpc call. PIVX-Project/PIVX#2317

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Add query options to listunspent RPC call #8952

Add query options to listunspent RPC call #8952

Uh oh!

Conversation

pedrobranco commented Oct 18, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Oct 19, 2016

Uh oh!

pedrobranco commented Oct 19, 2016

Uh oh!

laanwj commented Oct 19, 2016

Uh oh!

pedrobranco commented Oct 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

luke-jr left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrobranco commented Feb 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

laanwj commented Feb 7, 2017

Uh oh!

pedrobranco commented Feb 7, 2017

Uh oh!

laanwj commented Feb 7, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pedrobranco commented Mar 7, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

fanquake commented Apr 2, 2017

Uh oh!

laanwj commented May 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pedrobranco commented May 17, 2017

Uh oh!

sipa commented May 18, 2017

Uh oh!

laanwj commented May 18, 2017 via email

Uh oh!

fedelcs commented Jul 12, 2018

Uh oh!

promag commented Jul 13, 2018

Uh oh!

promag commented Jul 15, 2018

Uh oh!

RussianSwiss commented Jul 20, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

9 participants

pedrobranco commented Oct 18, 2016 •

edited

Loading

pedrobranco commented Oct 19, 2016 •

edited

Loading

pedrobranco commented Feb 7, 2017 •

edited

Loading

pedrobranco commented Mar 7, 2017 •

edited

Loading

laanwj commented May 17, 2017 •

edited

Loading

RussianSwiss commented Jul 20, 2018 •

edited

Loading