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

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 ee50c00

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 1406f1d - LGTM

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

src/wallet/spend.cpp

rajarshimaitra

ReACK 1424246

aureleoules

reACK 1424246

kouloumos

ACK 1424246, 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 3319eb9

aureleoules

reACK 3319eb9

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 3319eb9 - 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 Show resolved Hide resolved

test/functional/wallet_balance.py Show resolved Hide resolved

src/wallet/spend.h 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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

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

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

furszy commented Jul 28, 2022 •

edited

Loading

w0xlt left a comment

DrahtBot commented Jul 30, 2022 •

edited

Loading

jarolrod left a comment

furszy commented Aug 3, 2022

jarolrod left a comment

kouloumos left a comment

achow101 commented Oct 12, 2022

furszy commented Oct 13, 2022

vladimirfomene left a comment •

edited

Loading

aureleoules left a comment

rajarshimaitra left a comment

aureleoules left a comment

kouloumos left a comment

furszy commented Oct 14, 2022

kouloumos commented Oct 14, 2022

aureleoules left a comment

danielabrozzoni left a comment

danielabrozzoni left a comment

theStack commented Oct 25, 2022

furszy commented Oct 29, 2022

aureleoules left a comment

kouloumos left a comment

danielabrozzoni commented Nov 2, 2022

theStack left a comment

achow101 commented Nov 16, 2022

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

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

Conversation

furszy commented Jul 28, 2022 • edited Loading

w0xlt left a comment

Choose a reason for hiding this comment

DrahtBot commented Jul 30, 2022 • edited Loading

Conflicts

jarolrod left a comment

Choose a reason for hiding this comment

furszy commented Aug 3, 2022

jarolrod left a comment

Choose a reason for hiding this comment

kouloumos left a comment

Choose a reason for hiding this comment

achow101 commented Oct 12, 2022

furszy commented Oct 13, 2022

vladimirfomene left a comment • edited Loading

Choose a reason for hiding this comment

aureleoules left a comment

Choose a reason for hiding this comment

rajarshimaitra left a comment

Choose a reason for hiding this comment

aureleoules left a comment

Choose a reason for hiding this comment

kouloumos left a comment

Choose a reason for hiding this comment

furszy commented Oct 14, 2022

kouloumos commented Oct 14, 2022

aureleoules left a comment

Choose a reason for hiding this comment

danielabrozzoni left a comment

Choose a reason for hiding this comment

danielabrozzoni left a comment

Choose a reason for hiding this comment

theStack commented Oct 25, 2022

furszy commented Oct 29, 2022

aureleoules left a comment

Choose a reason for hiding this comment

kouloumos left a comment

Choose a reason for hiding this comment

danielabrozzoni commented Nov 2, 2022

theStack left a comment

Choose a reason for hiding this comment

achow101 commented Nov 16, 2022

furszy commented Jul 28, 2022 •

edited

Loading

DrahtBot commented Jul 30, 2022 •

edited

Loading

vladimirfomene left a comment •

edited

Loading