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 query options to listunspent RPC call #8952

Closed
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
9 participants
@pedrobranco
Contributor

pedrobranco commented Oct 18, 2016

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 laanwj added the RPC/REST/ZMQ label Oct 18, 2016

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 19, 2016

Member

Concept ACK

Member

laanwj commented Oct 19, 2016

Concept ACK

@pedrobranco

This comment has been minimized.

Show comment
Hide comment
@pedrobranco

pedrobranco Oct 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) ?

Contributor

pedrobranco commented Oct 19, 2016

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Oct 19, 2016

Member

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

Member

laanwj commented Oct 19, 2016

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

@pedrobranco

This comment has been minimized.

Show comment
Hide comment
@pedrobranco

pedrobranco Oct 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.

Contributor

pedrobranco commented Oct 19, 2016

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.

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

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

@luke-jr

utACK code, one nit.

Show outdated Hide outdated src/wallet/rpcwallet.cpp Outdated
@pedrobranco

This comment has been minimized.

Show comment
Hide comment
@pedrobranco

pedrobranco Feb 7, 2017

Contributor

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

Contributor

pedrobranco commented Feb 7, 2017

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 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.

Member

laanwj commented Feb 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@pedrobranco

pedrobranco Feb 7, 2017

Contributor

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

Contributor

pedrobranco commented Feb 7, 2017

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

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Feb 7, 2017

Member

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

Member

laanwj commented Feb 7, 2017

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

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

This comment has been minimized.

@kallewoof

kallewoof Feb 28, 2017

Member

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

@kallewoof

kallewoof Feb 28, 2017

Member

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

This comment has been minimized.

@pedrobranco

pedrobranco Mar 2, 2017

Contributor

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.

@pedrobranco

pedrobranco Mar 2, 2017

Contributor

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.

This comment has been minimized.

@kallewoof

kallewoof Mar 2, 2017

Member

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

kallewoof Mar 2, 2017

Member

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.

This comment has been minimized.

@pedrobranco

pedrobranco Mar 7, 2017

Contributor

@kallewoof Any suggestions? nTargetSumAmount?

@pedrobranco

pedrobranco Mar 7, 2017

Contributor

@kallewoof Any suggestions? nTargetSumAmount?

This comment has been minimized.

@kallewoof

kallewoof Mar 10, 2017

Member

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

@kallewoof

kallewoof Mar 10, 2017

Member

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

Show outdated Hide outdated src/wallet/rpcwallet.cpp Outdated
@@ -2399,15 +2399,22 @@ UniValue listunspent(const JSONRPCRequest& request)
"\nArguments:\n"

This comment has been minimized.

@kallewoof

kallewoof Feb 28, 2017

Member

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

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

kallewoof Feb 28, 2017

Member

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

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

This comment has been minimized.

Show comment
Hide comment
@pedrobranco

pedrobranco Mar 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?

Contributor

pedrobranco commented Mar 7, 2017

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

This comment has been minimized.

Show comment
Hide comment
@fanquake

fanquake Apr 2, 2017

Member

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

Member

fanquake commented Apr 2, 2017

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

@pedrobranco

This comment has been minimized.

Show comment
Hide comment
@pedrobranco

pedrobranco Apr 12, 2017

Contributor

Ready.

Contributor

pedrobranco commented Apr 12, 2017

Ready.

@pedrobranco

This comment has been minimized.

Show comment
Hide comment
@pedrobranco

pedrobranco May 16, 2017

Contributor

Any thoughts if this is going to be merged? /cc @laanwj

Contributor

pedrobranco commented May 16, 2017

Any thoughts if this is going to be merged? /cc @laanwj

@@ -1954,12 +1954,15 @@ CAmount CWallet::GetImmatureWatchOnlyBalance() const
return nTotal;
}
void CWallet::AvailableCoins(std::vector<COutput>& vCoins, bool fOnlySafe, const CCoinControl *coinControl, bool fIncludeZeroValue) const
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

This comment has been minimized.

@laanwj

laanwj May 17, 2017

Member

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?

@laanwj

laanwj May 17, 2017

Member

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?

This comment has been minimized.

@promag

promag May 17, 2017

Member

@laanwj I agree with creating a structure and grow there.

@promag

promag May 17, 2017

Member

@laanwj I agree with creating a structure and grow there.

laanwj added a commit that referenced this pull request May 17, 2017

Merge #8952: Add query options to listunspent RPC call
bc63d0e Add query options to listunspent rpc call (Pedro Branco)

Tree-SHA512: 2d296eee8df4e7ac378206ac3003a300e6478502d4b814f1ed1a47614222b01cc35dba871345ced68629860c227aff2c9e4b7f0d4ed0aa7de8b04f26c983580f
@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 17, 2017

Member

Merged (and rebased) via 9390845

Member

laanwj commented May 17, 2017

Merged (and rebased) via 9390845

@laanwj laanwj referenced this pull request May 17, 2017

Closed

TODO for release notes 0.15.0 #9889

12 of 12 tasks complete
@pedrobranco

This comment has been minimized.

Show comment
Hide comment
@pedrobranco

pedrobranco May 17, 2017

Contributor

Thank you @laanwj 👍

Contributor

pedrobranco commented May 17, 2017

Thank you @laanwj 👍

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa May 18, 2017

Member

@laanwj I suppose you forgot to close after merging?

Member

sipa commented May 18, 2017

@laanwj I suppose you forgot to close after merging?

@sipa sipa closed this May 18, 2017

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj May 18, 2017

Member
Member

laanwj commented May 18, 2017

@pedrobranco pedrobranco deleted the uphold:enhancement/improve-rpc-listunspent branch Jun 5, 2017

@fedelcs

This comment has been minimized.

Show comment
Hide comment
@fedelcs

fedelcs Jul 12, 2018

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

fedelcs commented Jul 12, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@promag

promag Jul 13, 2018

Member

Looks like a bug.

Member

promag commented Jul 13, 2018

Looks like a bug.

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Jul 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.
Member

promag commented Jul 15, 2018

@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

This comment has been minimized.

Show comment
Hide comment
@RussianSwiss

RussianSwiss Jul 20, 2018

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.

RussianSwiss commented Jul 20, 2018

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.

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