RPC: listunspent, add "include immature coinbase" flag #25730

furszy commented at 1:34 PM on July 28, 2022: member

Simple PR; adds a "include_immature_coinbase" flag to listunspent to include the immature coinbase UTXOs on the response. Requested by #25728.

fanquake added the label RPC/REST/ZMQ on Jul 28, 2022

furszy force-pushed on Jul 28, 2022

w0xlt approved

w0xlt commented at 1:52 AM on July 29, 2022: contributor

Tested ACK https://github.com/bitcoin/bitcoin/pull/25730/commits/8621fbf672157df5475fc5cbaa6524b4d0f105c7

DrahtBot commented at 2:25 PM on July 30, 2022: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#25789 (test: clean and extend availablecoins_tests coverage by furszy)

If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

jarolrod commented at 5:15 PM on August 2, 2022: member

tACK 8621fbf672157df5475fc5cbaa6524b4d0f105c7

Generated some blocks on regtest and checked the output of listunspent {args} ... include_immature_coinbase=true, to check that various immature coinbases were infact displayed

in test/functional/wallet_balance.py:78 in 8621fbf672 outdated

  73 | @@ -74,8 +74,18 @@ def run_test(self):
  74 |          self.log.info("Mining blocks ...")
  75 |          self.generate(self.nodes[0], 1)
  76 |          self.generate(self.nodes[1], 1)
  77 | +
  78 | +        # Verify listunspent returning immature coinbase if 'include_immature_coinbase' is set

jarolrod commented at 5:16 PM on August 2, 2022:

nit

        # Verify listunspent returns immature coinbase if 'include_immature_coinbase' is set

in test/functional/wallet_balance.py:84 in 8621fbf672 outdated

  79 | +        assert_equal(len(self.nodes[0].listunspent(include_immature_coinbase=True)), 1)
  80 | +        assert_equal(len(self.nodes[0].listunspent(include_immature_coinbase=False)), 0)
  81 | +
  82 |          self.generatetoaddress(self.nodes[1], COINBASE_MATURITY + 1, ADDRESS_WATCHONLY)
  83 |  
  84 | +        # Verify listunspent returning all immature coinbases if 'include_immature_coinbase' is set

jarolrod commented at 5:16 PM on August 2, 2022:

nit

        # Verify listunspent returns all immature coinbases if 'include_immature_coinbase' is set

furszy force-pushed on Aug 3, 2022

furszy commented at 1:23 PM on August 3, 2022: member

thanks for the feedback. Tackled @jarolrod nits and add release-notes.

furszy force-pushed on Aug 3, 2022

in doc/release-notes.md:103 in ee50c00a1b outdated

  98 | @@ -99,6 +99,10 @@ Wallet
  99 |  - RPC `getreceivedbylabel` now returns an error, "Label not found
 100 |    in wallet" (-4), if the label is not in the address book. (#25122)
 101 |  
 102 | +- RPC `listunspent` now has a new argument `include_immature_coinbase`
 103 | +  to include coinbase UTXOs that don't met the minimum spendability

jarolrod commented at 3:24 PM on August 3, 2022:

nit

  to include coinbase UTXOs that don't meet the minimum spendability

furszy commented at 8:45 PM on August 3, 2022:

should hire you as translator, pushed 👍🏼.

jarolrod commented at 3:24 PM on August 3, 2022: member

ACK ee50c00a1b3f09234a43f0fca140fcdd5f875ebd

Only change is small comment change and addition of release notes since my last review.

furszy force-pushed on Aug 3, 2022

in src/wallet/rpc/coins.cpp:525 in 36c2676526 outdated

 520 | @@ -521,6 +521,9 @@ RPCHelpMan listunspent()
 521 |                              {"minimumSumAmount", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"unlimited"}, "Minimum sum value of all UTXOs in " + CURRENCY_UNIT + ""},
 522 |                          },
 523 |                          "query_options"},
 524 | +                    {
 525 | +                        "include_immature_coinbase", RPCArg::Type::BOOL, RPCArg::Default{false}, "Include coinbase UTXOs that haven't met the minimum spendability depth requirement\n"

luke-jr commented at 11:11 PM on August 9, 2022:

Please don't add booleans as positional arguments. This should probably be part of query_options above.

furszy commented at 4:26 PM on August 12, 2022:

sure thanks, that is better.

luke-jr changes_requested

furszy force-pushed on Aug 12, 2022

kouloumos commented at 3:28 PM on September 7, 2022: contributor

Concept ACK,

Although this flag is already part of the *receivedby* RPCs since #14707, the logic there is different. Therefore this PR changes AvailableCoins (mainly used by CreateTransactionInternal to get the available coins prior to coin selection) in order to allow immature coinbase txns to make it to the available coins list. To accomplish that, it introduces the include_immature_coinbase option as part of coin control.

Is the inclusion of immature coinbase txns in scope for Coin Control features? Is there a create-tx scenario that we will want to choose those txns?

DrahtBot added the label Needs rebase on Sep 16, 2022

achow101 closed this on Oct 12, 2022

achow101 reopened this on Oct 12, 2022

achow101 commented at 8:35 PM on October 12, 2022: member

Reopened by request

furszy force-pushed on Oct 12, 2022

DrahtBot removed the label Needs rebase on Oct 12, 2022

furszy force-pushed on Oct 13, 2022

furszy commented at 2:44 AM on October 13, 2022: member

Is the inclusion of immature coinbase txns in scope for Coin Control features? Is there a create-tx scenario that we will want to choose those txns?

Good point @kouloumos (sorry for the delay). It shouldn't be there. Just moved it to be another AvailableCoins argument.

Plus, to make AvailableCoins more "friendly" to use with so many arguments, have grouped all the filtering parameters inside a struct 9f57166.

in src/wallet/rpc/coins.cpp:518 in 9f571660bc outdated

 514 | @@ -515,6 +515,7 @@ RPCHelpMan listunspent()
 515 |                              {"maximumAmount", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"unlimited"}, "Maximum value of each UTXO in " + CURRENCY_UNIT + ""},
 516 |                              {"maximumCount", RPCArg::Type::NUM, RPCArg::DefaultHint{"unlimited"}, "Maximum number of UTXOs"},
 517 |                              {"minimumSumAmount", RPCArg::Type::AMOUNT, RPCArg::DefaultHint{"unlimited"}, "Minimum sum value of all UTXOs in " + CURRENCY_UNIT + ""},
 518 | +                            {"include_immature_coinbase", RPCArg::Type::BOOL, RPCArg::Default{false}, "Include immature coinbase transactions"}

vladimirfomene commented at 11:39 AM on October 13, 2022:

I believe you meant to say, Include immature coinbase UTXOs. instead of Include immature coinbase transactions

furszy commented at 2:32 PM on October 13, 2022:

yep, same concept. If the tx is immature, all its outputs are immature. But let me push this.

kouloumos commented at 2:48 PM on October 13, 2022:

It's not that a big of an issue, but "transactions" is how it is used on all other RPCs that have the same flag, therefore this change creates a small inconsistency.

furszy commented at 3:04 PM on October 13, 2022:

yeah @kouloumos, I thought about it as well. Ended up changing it because in this RPC command listunspent, we only care about returning outputs. So, kinda make sense to use the UTXO term here.

Either way is fine for me.

kouloumos commented at 3:08 PM on October 13, 2022:

Agree, I didn't think it through.

vladimirfomene commented at 12:22 PM on October 13, 2022: none

Review ACK. Just Nits

furszy force-pushed on Oct 13, 2022

in src/wallet/spend.cpp:297 in 427548d8df outdated

 295 | @@ -301,19 +296,13 @@ CoinsResult AvailableCoins(const CWallet& wallet,
 296 |  
 297 |  CoinsResult AvailableCoinsListUnspent(const CWallet& wallet, const CCoinControl* coinControl, const CAmount& nMinimumAmount, const CAmount& nMaximumAmount, const CAmount& nMinimumSumAmount, const uint64_t nMaximumCount, bool include_immature_coinbase)

aureleoules commented at 2:42 PM on October 13, 2022:

427548d8df0b2786ee360f443bb07adc264773bc maybe you could wrap these args as well?

furszy commented at 3:38 PM on October 13, 2022:

I didn't do it because of the only_spendable arg that must be false for this method (the flag is set internally, not configurable by the method callers). But.. could also pass the struct and force set the only_spendable flag internally too.

kouloumos commented at 5:43 PM on October 13, 2022:

@furszy if you do decide to do that, maybe by using the struct here as well https://github.com/bitcoin/bitcoin/blob/0bac04b7585017508425b21d402fd3c52e6a13d5/src/wallet/rpc/coins.cpp#L593-L596 it creates a good opportunity (as you'll now touch all the existing code that uses them) to rename the variables to snake_case?

furszy commented at 7:38 PM on October 13, 2022:

Pushed both changes.

Squashed the AvailableCoinsListUnspent changes inside 56c7d7d and added an scripted-diff commit 1022c18 at the end for the snake_case rename to minimize the review burden.

aureleoules approved

aureleoules commented at 2:53 PM on October 13, 2022: member

ACK 1406f1d0f1db5cd68c2459953ba913e845cb36e2 - LGTM

427548d8df0b2786ee360f443bb07adc264773bc: I verified that the calls with hard-coded values match the defaults of AvailableCoinsParams.

rajarshimaitra approved

rajarshimaitra commented at 4:46 PM on October 13, 2022: contributor

Concept + tACK 427548d8df0b2786ee360f443bb07adc264773bc

Made some regtest blocks and manually created coinbase transactions and verified that they are reported in list unspent. This is in general useful for any wallet library that wants to sync with core using listunspent.

ACK on grouping the AvialbleCoins args, looks much more cleaner..

furszy force-pushed on Oct 13, 2022

furszy commented at 7:40 PM on October 13, 2022: member

updated per feedback

furszy force-pushed on Oct 13, 2022

rajarshimaitra approved

rajarshimaitra commented at 7:46 AM on October 14, 2022: contributor

ReACK 14242469930701462e39e34525fbd1a32a16daa7

aureleoules approved

aureleoules commented at 8:22 AM on October 14, 2022: member

reACK 14242469930701462e39e34525fbd1a32a16daa7

in src/wallet/rpc/spend.cpp:1388 in 1424246993 outdated

1384 | @@ -1385,7 +1385,9 @@ RPCHelpMan sendall()
1385 |                      total_input_value += tx->tx->vout[input.prevout.n].nValue;
1386 |                  }
1387 |              } else {
1388 | -                for (const COutput& output : AvailableCoins(*pwallet, &coin_control, fee_rate, /*nMinimumAmount=*/0).All()) {
1389 | +                AvailableCoinsParams coins_params;

kouloumos commented at 9:18 AM on October 14, 2022:

nit: Regarding consistency for variable names, I think the filter_coins name that you use for the same variable in wallet/rpc/coins.cpp is better, or maybe a mixture of your current names: coins_filter_params.

Talking about naming, although AvailableCoinsParams is only used in the AvailableCoins function (hence the naming I guess), I think the name could do a better job describing the content. May I suggest CoinFilter(ing)Params, which also follows the naming scheme used in CoinSelectionParams. https://github.com/bitcoin/bitcoin/blob/3f1f5f6f1ec49d0fb2acfddec4021b3582ba0553/src/wallet/coinselection.h#L116

furszy commented at 2:51 PM on October 14, 2022:

thought about it too. I tried to use a distinctive name for this struct because we do have some "coins filtering" args inside CoinControl as well.

But yeah, probably the best is to use the CoinFilterParams naming for this new struct and, in a future PR, instead of passing CoinControl to AvailableCoins, only pass CoinFilterParams (which will need to include what is inside CoinControl).

kouloumos commented at 2:59 PM on October 14, 2022:

I have to admit that I haven't look deep into the usage of CoinControl yet, but I I don't see such an issue on the distinction that CoinFilterParams are all the filters that are not part of CoinControl. Cause CoinControl is also a user-facing concept.

furszy commented at 3:12 PM on October 14, 2022:

What I meant is that, ideally, we should only provide CoinFilterParams to AvailableCoins and not two structs that contain filtering args. But that can be done later (I'm removing the preset inputs skip from AvailableCoins inside #25685).

kouloumos commented at 9:52 AM on October 14, 2022: contributor

ACK 14242469930701462e39e34525fbd1a32a16daa7, with a small nit about naming.

furszy force-pushed on Oct 14, 2022

furszy commented at 2:57 PM on October 14, 2022: member

Updated per @kouloumos feedback.

tiny diff, only renamed AvailableCoinsParams to CoinFilterParams.

kouloumos commented at 3:22 PM on October 14, 2022: contributor

reACK 3319eb9d3238f6fc02d51694ca5f215c58b7fc0b

aureleoules approved

aureleoules commented at 3:37 PM on October 14, 2022: member

reACK 3319eb9d3238f6fc02d51694ca5f215c58b7fc0b

in src/wallet/spend.cpp:352 in fc503c7ca9 outdated

 310 | -            /*nMinimumAmount=*/ 1,
 311 | -            /*nMaximumAmount=*/ MAX_MONEY,
 312 | -            /*nMinimumSumAmount=*/ MAX_MONEY,
 313 | -            /*nMaximumCount=*/ 0
 314 | -    ).total_amount;
 315 | +    return AvailableCoins(wallet, coinControl).total_amount;

danielabrozzoni commented at 4:41 PM on October 14, 2022:

Is there a specific reason for removing the explicit /*feerate=*/ std::nullopt here? (I don't have anything against it, I'm just wondering)

furszy commented at 5:59 PM on October 14, 2022:

because feerate default value is std::nullopt (see AvailableCoins declaration). So, no need to explicitly set it.

in test/functional/wallet_balance.py:90 in 3319eb9d32 outdated

  85 |          self.generatetoaddress(self.nodes[1], COINBASE_MATURITY + 1, ADDRESS_WATCHONLY)
  86 |  
  87 | +        # Verify listunspent returns all immature coinbases if 'include_immature_coinbase' is set
  88 | +        # For now, only the legacy wallet will see the coinbases going to the imported 'ADDRESS_WATCHONLY'
  89 | +        assert_equal(len(self.nodes[0].listunspent(query_options={'include_immature_coinbase': False})), 1 if self.options.descriptors else 2)
  90 | +        assert_equal(len(self.nodes[0].listunspent(query_options={'include_immature_coinbase': True})), 1 if self.options.descriptors else COINBASE_MATURITY + 2)

danielabrozzoni commented at 5:05 PM on October 14, 2022:

tiny tiny nit: the COINBASE_MATURITY + 2 left me wondering for a sec, this might be easier to read if we had a variable storing how many blocks were generated:

n_blocks_generate = COINBASE_MATURITY + 2  # feel free to use a better name :)
self.generatetoaddress(self.nodes[1],  n_blocks_generate, ADDRESS_WATCHONLY)
...
assert_equal(len(self.nodes[0].listunspent(query_options={'include_immature_coinbase': True})), 1 if self.options.descriptors else n_blocks_generate + 1)

furszy commented at 6:14 PM on October 14, 2022:

k, as it is a tiny tiny nit, will only do it if have to retouch the code so we don't loose the ACKs.

in src/wallet/spend.h:82 in 3319eb9d32 outdated

  83 | -                           bool only_spendable = true) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
  84 | +                           const CoinFilterParams& params = {}) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet);
  85 |  
  86 |  /**
  87 | - * Wrapper function for AvailableCoins which skips the `feerate` parameter. Use this function
  88 | + * Wrapper function for AvailableCoins which skips the `feerate` and `CoinFilterParams::only_spendable` parameters. Use this function

danielabrozzoni commented at 5:09 PM on October 14, 2022:

Sorry, I'm not sure I follow here 😅

Why are you saying that this function "skips" CoinFilterParams::only_spendable? To me, it seems that it's skipping only the feerate parameter (i.e., AvailableCoins takes a feerate param, and AvailableCoinsListUnspent doesn't)

furszy commented at 6:13 PM on October 14, 2022:

Internally, AvailableCoinsListUnspent sets only_spendable to false (this isn't a new behavior introduced on this PR, AvailableCoinsListUnspent callers expect to receive all the coins). Check one of my previous comments #25730 (review)

So, what the comment basically says is "do not customize CoinFilterParams::only_spendable, it will not be taken into account".

danielabrozzoni commented at 2:31 PM on October 15, 2022:

Ah, now it makes sense, thanks!

danielabrozzoni commented at 5:10 PM on October 14, 2022: contributor

Concept ACK, I have a couple of questions (beginner here, sorry :)) and I still haven't tested the code, but overalls it looks good to me!

danielabrozzoni approved

danielabrozzoni commented at 3:51 PM on October 15, 2022: contributor

tACK 3319eb9d3238f6fc02d51694ca5f215c58b7fc0b - the code looks good to me (but my C++ experience is limited), I tested locally and it works as expected. As I pointed out in #25728, this makes it way easier to track immature coins - at the moment in BDK we have to use listunspent combined with listtransactions to make sure to track correctly immature utxos.

Thanks for taking care of this! :)

theStack commented at 2:26 PM on October 25, 2022: contributor

Concept ACK

DrahtBot added the label Needs rebase on Oct 27, 2022

RPC: listunspent, add "include immature coinbase" flag

so we can return the immature coinbase UTXOs as well.

f0f6a3577b

wallet: group AvailableCoins filtering parameters in a single struct

Plus clean callers that use the params default values

61c2265629

scripted-diff: wallet: rename AvailableCoinsParams members to snake_case

-BEGIN VERIFY SCRIPT-

sed -i 's/nMinimumAmount/min_amount/g' $(git grep -l nMinimumAmount)
sed -i 's/nMaximumAmount/max_amount/g' $(git grep -l nMaximumAmount)
sed -i 's/nMinimumSumAmount/min_sum_amount/g' $(git grep -l nMinimumSumAmount)
sed -i 's/nMaximumCount/max_count/g' $(git grep -l nMaximumCount)

-END VERIFY SCRIPT-

fa84df1f03

furszy force-pushed on Oct 29, 2022

furszy commented at 11:59 AM on October 29, 2022: member

Rebased, conflicts solved. Ready to go.

DrahtBot removed the label Needs rebase on Oct 29, 2022

aureleoules approved

aureleoules commented at 4:45 PM on October 31, 2022: member

reACK fa84df1f033a5d1a8342ea941eca0b5ef73d78e7

kouloumos commented at 5:24 PM on October 31, 2022: contributor

reACK fa84df1f033a5d1a8342ea941eca0b5ef73d78e7

danielabrozzoni commented at 1:24 PM on November 2, 2022: contributor

reACK fa84df1f033a5d1a8342ea941eca0b5ef73d78e7

theStack approved

theStack commented at 5:01 PM on November 8, 2022: contributor

Code-review ACK fa84df1f033a5d1a8342ea941eca0b5ef73d78e7

maflcko assigned achow101 on Nov 9, 2022

achow101 commented at 12:40 AM on November 16, 2022: member

ACK fa84df1f033a5d1a8342ea941eca0b5ef73d78e7

achow101 merged this on Nov 16, 2022

achow101 closed this on Nov 16, 2022

sidhujag referenced this in commit 7387848c0a on Nov 16, 2022

furszy deleted the branch on May 27, 2023

bitcoin locked this on May 26, 2024