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

furszy · 2022-07-28T13:34:21Z

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

w0xlt

Tested ACK 8621fbf

DrahtBot · 2022-07-30T14:25:26Z

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

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

test/functional/wallet_balance.py

furszy · 2022-08-03T13:23:43Z

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

jarolrod

ACK ee50c00a1b3f09234a43f0fca140fcdd5f875ebd

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

doc/release-notes.md

src/wallet/rpc/coins.cpp

kouloumos

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?

achow101 · 2022-10-12T20:35:11Z

Reopened by request

furszy · 2022-10-13T02:44:17Z

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.

vladimirfomene

Review ACK. Just Nits

src/wallet/rpc/coins.cpp

aureleoules

ACK 1406f1d0f1db5cd68c2459953ba913e845cb36e2 - LGTM

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

src/wallet/spend.cpp

rajarshimaitra

ReACK 14242469930701462e39e34525fbd1a32a16daa7

aureleoules

reACK 14242469930701462e39e34525fbd1a32a16daa7

kouloumos

ACK 14242469930701462e39e34525fbd1a32a16daa7, with a small nit about naming.

src/wallet/rpc/spend.cpp

furszy · 2022-10-14T14:57:10Z

Updated per @kouloumos feedback.

tiny diff, only renamed AvailableCoinsParams to CoinFilterParams.

kouloumos · 2022-10-14T15:22:19Z

reACK 3319eb9d3238f6fc02d51694ca5f215c58b7fc0b

aureleoules

reACK 3319eb9d3238f6fc02d51694ca5f215c58b7fc0b

danielabrozzoni

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!

src/wallet/spend.cpp

test/functional/wallet_balance.py

src/wallet/spend.h

danielabrozzoni

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 · 2022-10-25T14:26:32Z

Concept ACK

so we can return the immature coinbase UTXOs as well.

Plus clean callers that use the params default values

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

furszy · 2022-10-29T11:59:45Z

Rebased, conflicts solved.
Ready to go.

aureleoules

reACK fa84df1

kouloumos

reACK fa84df1

danielabrozzoni · 2022-11-02T13:24:17Z

reACK fa84df1

theStack

Code-review ACK fa84df1

achow101 · 2022-11-16T00:40:16Z

ACK fa84df1

…" flag fa84df1 scripted-diff: wallet: rename AvailableCoinsParams members to snake_case (furszy) 61c2265 wallet: group AvailableCoins filtering parameters in a single struct (furszy) f0f6a35 RPC: listunspent, add "include immature coinbase" flag (furszy) Pull request description: Simple PR; adds a "include_immature_coinbase" flag to `listunspent` to include the immature coinbase UTXOs on the response. Requested by bitcoin#25728. ACKs for top commit: danielabrozzoni: reACK fa84df1 achow101: ACK fa84df1 aureleoules: reACK fa84df1 kouloumos: reACK fa84df1 theStack: Code-review ACK fa84df1 Tree-SHA512: 0f3544cb8cfd0378a5c74594480f78e9e919c6cfb73a83e0f3112f8a0132a9147cf846f999eab522cea9ef5bd3ffd60690ea2ca367dde457b0554d7f38aec792

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

furszy mentioned this pull request Jul 28, 2022

rpc: listunspent doesn't return immature coinbase outputs #25728

Closed

furszy force-pushed the 2022_RPC_listunspent_include_immature_coinbase branch 3 times, most recently from 77b92ea to 8621fbf Compare July 28, 2022 15:39

w0xlt approved these changes Jul 29, 2022

View reviewed changes

DrahtBot mentioned this pull request Jul 30, 2022

rpc: Filter inputs by type during CoinSelection #25183

Closed

jarolrod reviewed Aug 2, 2022

View reviewed changes

test/functional/wallet_balance.py Outdated Show resolved Hide resolved

test/functional/wallet_balance.py Outdated Show resolved Hide resolved

furszy force-pushed the 2022_RPC_listunspent_include_immature_coinbase branch from 8621fbf to 785f78b Compare August 3, 2022 13:15

furszy force-pushed the 2022_RPC_listunspent_include_immature_coinbase branch from 785f78b to ee50c00 Compare August 3, 2022 13:26

jarolrod reviewed Aug 3, 2022

View reviewed changes

doc/release-notes.md Outdated Show resolved Hide resolved

furszy force-pushed the 2022_RPC_listunspent_include_immature_coinbase branch from ee50c00 to 36c2676 Compare August 3, 2022 20:44

luke-jr suggested changes Aug 9, 2022

View reviewed changes

src/wallet/rpc/coins.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2022_RPC_listunspent_include_immature_coinbase branch from 36c2676 to c070ea2 Compare August 12, 2022 16:50

kouloumos reviewed Sep 7, 2022

View reviewed changes

DrahtBot mentioned this pull request Sep 14, 2022

refactor: Set RPCArg options with designated initializers #26074

Merged

DrahtBot added the Needs rebase label Sep 16, 2022

achow101 closed this Oct 12, 2022

achow101 reopened this Oct 12, 2022

furszy force-pushed the 2022_RPC_listunspent_include_immature_coinbase branch from c070ea2 to cdb4365 Compare October 12, 2022 21:01

DrahtBot removed the Needs rebase label Oct 12, 2022

furszy force-pushed the 2022_RPC_listunspent_include_immature_coinbase branch from cdb4365 to 47be4ac Compare October 13, 2022 00:10

vladimirfomene reviewed Oct 13, 2022

View reviewed changes

src/wallet/rpc/coins.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2022_RPC_listunspent_include_immature_coinbase branch from 9f57166 to 427548d Compare October 13, 2022 14:28

aureleoules approved these changes Oct 13, 2022

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2022_RPC_listunspent_include_immature_coinbase branch from 1022c18 to 1424246 Compare October 13, 2022 20:54

rajarshimaitra approved these changes Oct 14, 2022

View reviewed changes

aureleoules approved these changes Oct 14, 2022

View reviewed changes

kouloumos reviewed Oct 14, 2022

View reviewed changes

src/wallet/rpc/spend.cpp Outdated Show resolved Hide resolved

furszy force-pushed the 2022_RPC_listunspent_include_immature_coinbase branch from 1424246 to 3319eb9 Compare October 14, 2022 14:53

aureleoules approved these changes Oct 14, 2022

View reviewed changes

danielabrozzoni reviewed Oct 14, 2022

View reviewed changes

src/wallet/spend.cpp Outdated Show resolved Hide resolved

test/functional/wallet_balance.py Outdated Show resolved Hide resolved

src/wallet/spend.h Outdated Show resolved Hide resolved

danielabrozzoni approved these changes Oct 15, 2022

View reviewed changes

This was referenced Oct 17, 2022

test: clean and extend availablecoins_tests coverage #25789

Merged

wallet: Faster transaction creation by removing pre-set-inputs fetching responsibility from Coin Selection #25685

Merged

DrahtBot added the Needs rebase label Oct 27, 2022

furszy added 3 commits October 29, 2022 08:45

RPC: listunspent, add "include immature coinbase" flag

f0f6a35

so we can return the immature coinbase UTXOs as well.

wallet: group AvailableCoins filtering parameters in a single struct

61c2265

Plus clean callers that use the params default values

furszy force-pushed the 2022_RPC_listunspent_include_immature_coinbase branch from 3319eb9 to fa84df1 Compare October 29, 2022 11:54

DrahtBot removed the Needs rebase label Oct 29, 2022

aureleoules approved these changes Oct 31, 2022

View reviewed changes

kouloumos reviewed Oct 31, 2022

View reviewed changes

theStack approved these changes Nov 8, 2022

View reviewed changes

maflcko assigned achow101 Nov 9, 2022

achow101 merged commit f0c646f into bitcoin:master Nov 16, 2022

furszy deleted the 2022_RPC_listunspent_include_immature_coinbase branch May 27, 2023 01:48

bitcoin locked and limited conversation to collaborators May 26, 2024

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

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

Uh oh!

Conversation

furszy commented Jul 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

w0xlt left a comment

Choose a reason for hiding this comment

Uh oh!

DrahtBot commented Jul 30, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Conflicts

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

furszy commented Aug 3, 2022

Uh oh!

jarolrod left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

kouloumos left a comment

Choose a reason for hiding this comment

Uh oh!

achow101 commented Oct 12, 2022

Uh oh!

furszy commented Oct 13, 2022

Uh oh!

vladimirfomene left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rajarshimaitra left a comment

Choose a reason for hiding this comment

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

kouloumos left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

furszy commented Oct 14, 2022

Uh oh!

kouloumos commented Oct 14, 2022

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

danielabrozzoni left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

danielabrozzoni left a comment

Choose a reason for hiding this comment

Uh oh!

theStack commented Oct 25, 2022

Uh oh!

furszy commented Oct 29, 2022

Uh oh!

aureleoules left a comment

Choose a reason for hiding this comment

Uh oh!

kouloumos left a comment

Choose a reason for hiding this comment

Uh oh!

danielabrozzoni commented Nov 2, 2022

Uh oh!

furszy commented Jul 28, 2022 •

edited

Loading

DrahtBot commented Jul 30, 2022 •

edited

Loading

vladimirfomene left a comment •

edited

Loading