Add query options to listunspent RPC call #8952

pull pedrobranco wants to merge 1 commits into bitcoin:master from uphold:enhancement/improve-rpc-listunspent changing 4 files +81 −18
  1. pedrobranco commented at 11:34 am on October 18, 2016: contributor

    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).
    0> bitcoin-cli help listunspent
1...
25. query options    (json, optional) JSON with query options
3    {
4      "minimumAmount"    (numeric or string, default=0) Minimum value of each UTXO in BTC
5      "maximumAmount"    (numeric or string, default=21000000=unlimited) Maximum value of each UTXO in BTC
6      "maximumCount"     (numeric or string, default=0=unlimited) Maximum number of UTXOs
7      "minimumSumAmount" (numeric or string, default=21000000=unlimited) Minimum sum value all UTXOs in BTC
8    }
9...

    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:

    0> 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.

  2. pedrobranco force-pushed on Oct 18, 2016
  3. laanwj added the label RPC/REST/ZMQ on Oct 18, 2016
  4. pedrobranco force-pushed on Oct 18, 2016
  5. pedrobranco force-pushed on Oct 19, 2016
  6. pedrobranco force-pushed on Oct 19, 2016
  7. laanwj commented at 2:12 pm on October 19, 2016: member
    Concept ACK
  8. pedrobranco commented at 2:24 pm on October 19, 2016: contributor
    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) ?
  9. pedrobranco force-pushed on Oct 19, 2016
  10. laanwj commented at 3:39 pm on October 19, 2016: member
    Yes, would make sense. Passing the JSON “query object” is a good idea.
  11. pedrobranco commented at 5:01 pm on October 19, 2016: contributor

    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.

  12. pedrobranco force-pushed on Oct 19, 2016
  13. pedrobranco renamed this:
    Add selection options to listunspent RPC call
    Add query options to listunspent RPC call
    on Oct 19, 2016
  14. pedrobranco force-pushed on Oct 19, 2016
  15. luke-jr referenced this in commit ed5bc96ee9 on Dec 21, 2016
  16. in src/wallet/rpcwallet.cpp: in 98d0a6fb81 outdated 
    2361+            "4. query options    (json, optional) JSON with query options\n"
2362+            "    {\n"
2363+            "      \"minimumAmount\"    (numeric or string, default=0) Minimum value of each UTXO in " + CURRENCY_UNIT + "\n"
2364+            "      \"maximumAmount\"    (numeric or string, default=21000000=unlimited) Maximum value of each UTXO in " + CURRENCY_UNIT + "\n"
2365+            "      \"maximumCount\"     (numeric or string, default=0=unlimited) Maximum number of UTXOs\n"
2366+            "      \"minimumSumAmount\" (numeric or string, default=21000000=unlimited) Minimum sum value all UTXOs in " + CURRENCY_UNIT + "\n"
    luke-jr commented at 5:31 am on February 3, 2017:
    Instead of the dual meanings, just say default=unlimited (and make sure 0 doesn’t behave in such a manner).
    pedrobranco commented at 4:16 pm on February 3, 2017:
    👍
  17. luke-jr changes_requested
  18. luke-jr commented at 5:36 am on February 3, 2017: member
    utACK code, one nit.
  19. pedrobranco force-pushed on Feb 3, 2017
  20. pedrobranco commented at 2:11 pm on February 7, 2017: contributor
    @laanwj @luke-jr Is this PR mergeable as is?
  21. laanwj commented at 2:39 pm on February 7, 2017: member
    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.
  22. pedrobranco commented at 2:56 pm on February 7, 2017: contributor
    About tests: there aren’t any tests specifically for testing listunspent options. Should we now create them?
  23. laanwj commented at 3:12 pm on February 7, 2017: member
    Yes, tests for that would be great. They’d make it possible to automatically detect when something breaks this functionality.
  24. in src/wallet/wallet.cpp: in ae6a0c0e1d outdated 
    2096+                // Checks the sum amount of all UTXO's.
2097+                if (nMinimumSumAmount != MAX_MONEY) {
2098+                    nTotal += pcoin->tx->vout[i].nValue;
2099+
2100+                    if (nTotal >= nMinimumSumAmount) {
2101+                        return;
    kallewoof commented at 7:52 pm on February 28, 2017:
    Maybe I’m misreading, but this (nMinimumSumAmount) sounds like a maximum, not a minimum. Esp. since it defaults to MAX_MONEY rather than 0.
    pedrobranco commented at 0:20 am on March 2, 2017:
    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.
    kallewoof commented at 5:46 pm on March 2, 2017:

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

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

    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.

    pedrobranco commented at 6:09 pm on March 7, 2017:
    @kallewoof Any suggestions? nTargetSumAmount?
    kallewoof commented at 5:13 pm on March 10, 2017:
    @pedrobranco I say keep it as it is if no one else complains.
  25. in src/wallet/rpcwallet.cpp: in ae6a0c0e1d outdated 
    2413+            "5. query options    (json, optional) JSON with query options\n"
2414+            "    {\n"
2415+            "      \"minimumAmount\"    (numeric or string, default=0) Minimum value of each UTXO in " + CURRENCY_UNIT + "\n"
2416+            "      \"maximumAmount\"    (numeric or string, default=unlimited) Maximum value of each UTXO in " + CURRENCY_UNIT + "\n"
2417+            "      \"maximumCount\"     (numeric or string, default=unlimited) Maximum number of UTXOs\n"
2418+            "      \"minimumSumAmount\" (numeric or string, default=unlimited) Minimum sum value all UTXOs in " + CURRENCY_UNIT + "\n"
    kallewoof commented at 7:53 pm on February 28, 2017:
    Typo: [...]Minimum sum value of all UTXOs in " (missing of).
  26. in src/wallet/rpcwallet.cpp: in ae6a0c0e1d outdated 
    2398@@ -2399,15 +2399,22 @@ UniValue listunspent(const JSONRPCRequest& request)
2399             "\nArguments:\n"
    kallewoof commented at 7:55 pm on February 28, 2017:

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

    0"listunspent ( minconf maxconf  [\"addresses\",...] [include_unsafe] [options] )\n"
  27. pedrobranco commented at 6:01 pm on March 7, 2017: contributor

    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?

  28. pedrobranco force-pushed on Mar 7, 2017
  29. pedrobranco force-pushed on Mar 7, 2017
  30. pedrobranco force-pushed on Mar 8, 2017
  31. pedrobranco force-pushed on Mar 8, 2017
  32. fanquake commented at 7:06 am on April 2, 2017: member
    This needs a rebase. @pedrobranco Tests could be submitted in a follow-up PR.
  33. pedrobranco force-pushed on Apr 4, 2017
  34. Add query options to listunspent rpc call 11ee5ec98d
  35. pedrobranco force-pushed on Apr 5, 2017
  36. pedrobranco commented at 1:28 pm on April 12, 2017: contributor
    Ready.
  37. pedrobranco commented at 3:28 pm on May 16, 2017: contributor
    Any thoughts if this is going to be merged? /cc @laanwj
  38. in src/wallet/wallet.cpp:1957 in 11ee5ec98d 
    1953@@ -1954,12 +1954,15 @@ CAmount CWallet::GetImmatureWatchOnlyBalance() const
1954     return nTotal;
1955 }
1956 
1957-void CWallet::AvailableCoins(std::vector<COutput>& vCoins, bool fOnlySafe, const CCoinControl *coinControl, bool fIncludeZeroValue) const
1958+void CWallet::AvailableCoins(std::vector<COutput> &vCoins, bool fOnlySafe, const CCoinControl *coinControl, const CAmount &nMinimumAmount, const CAmount &nMaximumAmount, const CAmount &nMinimumSumAmount, const uint64_t &nMaximumCount, const int &nMinDepth, const int &nMaxDepth) const
    laanwj commented at 6:31 am on May 17, 2017:

    Not necessary in this pull, but if we keep extending these arguments I think it’d be better to group then into a CoinConstraints structure and pass that in.

    Speaking of which - isn’t CCoinControl pretty much that?

    promag commented at 2:01 pm on May 17, 2017:
    @laanwj I agree with creating a structure and grow there.
  39. laanwj referenced this in commit 9390845c53 on May 17, 2017
  40. laanwj commented at 7:12 am on May 17, 2017: member
    Merged (and rebased) via 9390845
  41. pedrobranco commented at 1:41 pm on May 17, 2017: contributor
    Thank you @laanwj 👍
  42. sipa commented at 0:38 am on May 18, 2017: member
    @laanwj I suppose you forgot to close after merging?
  43. sipa closed this on May 18, 2017
  44. laanwj commented at 8:31 am on May 18, 2017: member

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

    On May 18, 2017 2:38 AM, “Pieter Wuille” notifications@github.com 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 https://github.com/bitcoin/bitcoin/pull/8952#issuecomment-302268723, or mute the thread https://github.com/notifications/unsubscribe-auth/AAHutptwk3F7P5M0nGbOo-1CacVVlXtZks5r65L5gaJpZM4KZrh9 .

  45. pedrobranco deleted the branch on Jun 5, 2017
  46. fedelcs commented at 10:46 pm on July 12, 2018: none

    @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?

  47. promag commented at 6:06 am on July 13, 2018: member
    Looks like a bug.
  48. promag commented at 4:32 pm on July 15, 2018: member

    @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.
  49. RussianSwiss commented at 11:25 pm on July 20, 2018: none
    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.
  50. PastaPastaPasta referenced this in commit 7708efb186 on Jun 20, 2019
  51. PastaPastaPasta referenced this in commit 5393fa974d on Jun 22, 2019
  52. PastaPastaPasta referenced this in commit 49976954e9 on Jun 22, 2019
  53. PastaPastaPasta referenced this in commit e6f3dabb55 on Jun 22, 2019
  54. PastaPastaPasta referenced this in commit 77ff195632 on Jun 22, 2019
  55. PastaPastaPasta referenced this in commit fb5c67e683 on Jun 22, 2019
  56. PastaPastaPasta referenced this in commit 964f8795a9 on Jun 22, 2019
  57. PastaPastaPasta referenced this in commit b62425a780 on Jun 22, 2019
  58. PastaPastaPasta referenced this in commit bf46c0c98f on Jun 22, 2019
  59. PastaPastaPasta referenced this in commit c170523199 on Jun 22, 2019
  60. PastaPastaPasta referenced this in commit 7173974555 on Jun 22, 2019
  61. PastaPastaPasta referenced this in commit 885eb9d337 on Jun 22, 2019
  62. PastaPastaPasta referenced this in commit 30ad4b8e8c on Jun 26, 2019
  63. barrystyle referenced this in commit 0a8dacf7aa on Jan 22, 2020
  64. furszy referenced this in commit e3d0c2361e on May 17, 2021
  65. MarcoFalke locked this on Sep 8, 2021


pedrobranco laanwj luke-jr kallewoof fanquake promag sipa fedelcs RussianSwiss

Labels
RPC/REST/ZMQ

github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2024-12-22 12:12 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me