wallet, rpc: add `label` to `listsinceblock` #25934

brunoerg · 2022-08-25T21:16:59Z

This PR adds label parameter to listsinceblock to be able to fetch all incoming transactions having the specified label since a specific block.

It's possible to use it in listtransactions, however, it's only possible to set the number of transactions to return, not a specific block to fetch from. getreceivedbylabel only returns the total amount received, not the txs info. listreceivedbylabel doesn't list all the informations about the transactions and it's not possible to fetch since a block.

src/wallet/rpc/transactions.cpp

w0xlt

ACK cf00489

CI error is not related

src/wallet/rpc/transactions.cpp

DrahtBot · 2022-09-23T09:20:26Z

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

Reviews

See the guideline for information on the review process.

Type	Reviewers
ACK	w0xlt, aureleoules, achow101
Stale ACK	jonatack

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26238 (clang-tidy: fixup named argument comments by fanquake)
#25979 ([WIP] wallet: standardize change output detection process 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.

aureleoules

Concept ACK
I think this change can be useful for accountability.

src/wallet/rpc/transactions.cpp

test/functional/wallet_listsinceblock.py

src/wallet/rpc/transactions.cpp

test/functional/wallet_listsinceblock.py

src/wallet/rpc/transactions.cpp

test/functional/wallet_listsinceblock.py

aureleoules

ACK ee284ff

src/wallet/rpc/transactions.cpp

jonatack

Concept ACK. If I understand correctly, it looks like the PR title and description should describe this functionality as filtering the results by those having the label.

src/wallet/rpc/transactions.cpp

doc/release-notes-25934.md

jonatack · 2022-09-28T11:39:44Z

src/wallet/rpc/transactions.cpp

@@ -552,6 +552,8 @@ RPCHelpMan listsinceblock()
                    {"include_removed", RPCArg::Type::BOOL, RPCArg::Default{true}, "Show transactions that were removed due to a reorg in the \"removed\" array\n"
                                                                       "(not guaranteed to work on pruned nodes)"},
                    {"include_change", RPCArg::Type::BOOL, RPCArg::Default{false}, "Also add entries for change outputs.\n"},
+                    {"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, should be a valid label name to return only incoming transactions\n"


Seems this can be shorter and on one line

Suggested change

{"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, should be a valid label name to return only incoming transactions\n"

{"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Return only incoming transactions\n"

and s/with/having/

Unlike listtransactions, I wasn't able to get an invalid label error with listsinceblock. Maybe test for that like the one in test/functional/wallet_listtransactions.py::L282, if the label can be invalid in listsinceblock.

Perfect, you're right. Gonna update it.

I'm working on this in #26186.
I didn't think it would fit the scope of this PR.

That PR doesn't seem to touch/fix the invalidity check for listsinceblock. Should it be done here while adding the arg, or do you plan to update that pull after this one?

I've refactored LabelFromValue in #26186, and it's marked for the 24.0 milestone so maybe this PR should be rebased on top of #26186 and the check should be added here as well?
I also don't mind updating my pull if this one gets merged first.

Unlike listtransactions, I wasn't able to get an invalid label error with listsinceblock

I think in listtransactions label parameter is the first one, so you pass a value like "*" or a real name and if you try it with an empty label it throws an error. In this case, label is the last parameter, you can pass a label or leave it empty to fetch all, there is no need to adopt the same pattern as listtransactions.

src/wallet/rpc/transactions.cpp

test/functional/wallet_listsinceblock.py

aureleoules

re-ACK 2a86921 - minor changes and documentation improvements since my last review.

src/wallet/rpc/transactions.cpp

doc/release-notes-25934.md

src/wallet/rpc/transactions.cpp

jonatack · 2022-09-30T10:18:33Z

src/wallet/rpc/transactions.cpp

@@ -552,6 +552,8 @@ RPCHelpMan listsinceblock()
                    {"include_removed", RPCArg::Type::BOOL, RPCArg::Default{true}, "Show transactions that were removed due to a reorg in the \"removed\" array\n"
                                                                       "(not guaranteed to work on pruned nodes)"},
                    {"include_change", RPCArg::Type::BOOL, RPCArg::Default{false}, "Also add entries for change outputs.\n"},
+                    {"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, should be a valid label name to return only incoming transactions\n"


That PR doesn't seem to touch/fix the invalidity check for listsinceblock. Should it be done here while adding the arg, or do you plan to update that pull after this one?

jonatack

Not sure, but should the PR title and description describe this functionality as filtering the results by those having the label?

brunoerg · 2022-10-24T16:21:27Z

Not sure, but should the PR title and description describe this functionality as filtering the results by those having the label?

Just updated the description.

brunoerg · 2022-10-24T16:21:38Z

Force-pushed addressing @jonatack's review.

Obs: CI failed for Win64 native due an intermittent issue (See: #26330)

jonatack

ACK 0ff870f

test/functional/wallet_listsinceblock.py

aureleoules

reACK 0ff870f - minor changes and documentation improvements since my last review.

brunoerg · 2022-10-27T20:26:53Z

Force-pushed rebasing and addressing @jonatack's suggestion for the test.

jonatack

ACK c9b60d3

jonatack · 2022-10-27T23:16:09Z

@brunoerg it might be good to un-resolve the discussion from #25934 (comment) as it needs to be addressed (if I'm not confused).

aureleoules

reACK c9b60d3

brunoerg · 2022-12-06T19:09:57Z

Force-pushed rebasing.

w0xlt

ACK 4e362c2

aureleoules

ACK 4e362c2

achow101 · 2022-12-07T23:32:47Z

ACK 4e362c2

4e362c2 doc: add release note for 25934 (brunoerg) fe488b4 test: add coverage for `label` in `listsinceblock` (brunoerg) 722e9a4 wallet, rpc: add `label` to `listsinceblock` (brunoerg) 852891f refactor, wallet: use optional for `label` in `ListTransactions` (brunoerg) Pull request description: This PR adds `label` parameter to `listsinceblock` to be able to fetch all incoming transactions having the specified label since a specific block. It's possible to use it in `listtransactions`, however, it's only possible to set the number of transactions to return, not a specific block to fetch from. `getreceivedbylabel` only returns the total amount received, not the txs info. `listreceivedbylabel` doesn't list all the informations about the transactions and it's not possible to fetch since a block. ACKs for top commit: achow101: ACK 4e362c2 w0xlt: ACK bitcoin@4e362c2 aureleoules: ACK 4e362c2 Tree-SHA512: fbde5db8cebf7a27804154fa61997b5155ad512e978cebb78c17acab9efcb624ea5f39d649899d12e5e675f80d4d0064cae8132b864de0d93a8d1e6fbcb9a737

brunoerg marked this pull request as ready for review August 26, 2022 12:28

glozow added the RPC/REST/ZMQ label Aug 26, 2022

luke-jr reviewed Aug 26, 2022

View reviewed changes

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

brunoerg force-pushed the 2022-08-add-label-listsinceblock branch from bfb8464 to cf00489 Compare August 27, 2022 00:31

w0xlt approved these changes Aug 27, 2022

View reviewed changes

furszy reviewed Sep 3, 2022

View reviewed changes

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

aureleoules reviewed Sep 26, 2022

View reviewed changes

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

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

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

aureleoules reviewed Sep 26, 2022

View reviewed changes

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

brunoerg force-pushed the 2022-08-add-label-listsinceblock branch 3 times, most recently from 2097d20 to cd444f4 Compare September 26, 2022 22:46

aureleoules reviewed Sep 27, 2022

View reviewed changes

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

aureleoules reviewed Sep 27, 2022

View reviewed changes

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

aureleoules reviewed Sep 27, 2022

View reviewed changes

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

DrahtBot mentioned this pull request Sep 27, 2022

[WIP] wallet: standardize change output detection process #25979

Draft

3 tasks

brunoerg force-pushed the 2022-08-add-label-listsinceblock branch from cd444f4 to 1c373dd Compare September 27, 2022 14:10

aureleoules reviewed Sep 27, 2022

View reviewed changes

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

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

brunoerg force-pushed the 2022-08-add-label-listsinceblock branch from 1c373dd to 2ddf967 Compare September 27, 2022 14:51

aureleoules reviewed Sep 27, 2022

View reviewed changes

test/functional/wallet_listsinceblock.py Show resolved Hide resolved

brunoerg force-pushed the 2022-08-add-label-listsinceblock branch from 2ddf967 to ee284ff Compare September 27, 2022 16:16

aureleoules approved these changes Sep 28, 2022

View reviewed changes

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

jonatack reviewed Sep 28, 2022

View reviewed changes

brunoerg force-pushed the 2022-08-add-label-listsinceblock branch from ee284ff to 2a86921 Compare September 28, 2022 21:58

aureleoules approved these changes Sep 30, 2022

View reviewed changes

jonatack reviewed Sep 30, 2022

View reviewed changes

jonatack mentioned this pull request Sep 30, 2022

rpc: Sanitize label name in various RPCs with tests #26186

Merged

DrahtBot mentioned this pull request Oct 3, 2022

clang-tidy: fixup named argument comments #26238

Merged

DrahtBot mentioned this pull request Oct 20, 2022

rpc: make address field optional list{transactions, sinceblock} response #26349

Closed

brunoerg force-pushed the 2022-08-add-label-listsinceblock branch 2 times, most recently from c15c9b5 to 0ff870f Compare October 24, 2022 14:30

jonatack reviewed Oct 26, 2022

View reviewed changes

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

aureleoules approved these changes Oct 26, 2022

View reviewed changes

DrahtBot added the Needs rebase label Oct 27, 2022

brunoerg force-pushed the 2022-08-add-label-listsinceblock branch 2 times, most recently from 219b7f0 to c9b60d3 Compare October 27, 2022 20:22

DrahtBot removed the Needs rebase label Oct 27, 2022

jonatack reviewed Oct 27, 2022

View reviewed changes

aureleoules approved these changes Oct 28, 2022

View reviewed changes

DrahtBot added the Needs rebase label Dec 6, 2022

brunoerg added 4 commits December 6, 2022 15:27

refactor, wallet: use optional for label in ListTransactions

852891f

wallet, rpc: add label to listsinceblock

722e9a4

test: add coverage for label in listsinceblock

fe488b4

doc: add release note for 25934

4e362c2

brunoerg force-pushed the 2022-08-add-label-listsinceblock branch from c9b60d3 to 4e362c2 Compare December 6, 2022 19:05

DrahtBot removed the Needs rebase label Dec 6, 2022

w0xlt approved these changes Dec 7, 2022

View reviewed changes

aureleoules approved these changes Dec 7, 2022

View reviewed changes

achow101 merged commit a653f4b into bitcoin:master Dec 7, 2022

bitcoin locked and limited conversation to collaborators Dec 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

wallet, rpc: add `label` to `listsinceblock` #25934

wallet, rpc: add `label` to `listsinceblock` #25934

brunoerg commented Aug 25, 2022 •

edited

w0xlt left a comment

DrahtBot commented Sep 23, 2022 •

edited

aureleoules left a comment

aureleoules left a comment

jonatack left a comment

jonatack Sep 28, 2022 •

edited

brunoerg Sep 28, 2022

aureleoules Sep 28, 2022

jonatack Sep 30, 2022 •

edited

aureleoules Sep 30, 2022

brunoerg Dec 6, 2022

aureleoules left a comment

jonatack Sep 30, 2022 •

edited

jonatack left a comment

brunoerg commented Oct 24, 2022

brunoerg commented Oct 24, 2022

jonatack left a comment

aureleoules left a comment

brunoerg commented Oct 27, 2022

jonatack left a comment

jonatack commented Oct 27, 2022

aureleoules left a comment

brunoerg commented Dec 6, 2022

w0xlt left a comment

aureleoules left a comment

achow101 commented Dec 7, 2022

	{"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, should be a valid label name to return only incoming transactions\n"
	{"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Return only incoming transactions\n"

wallet, rpc: add label to listsinceblock #25934

wallet, rpc: add label to listsinceblock #25934

Conversation

brunoerg commented Aug 25, 2022 • edited

w0xlt left a comment

Choose a reason for hiding this comment

DrahtBot commented Sep 23, 2022 • edited

Reviews

Conflicts

aureleoules left a comment

Choose a reason for hiding this comment

aureleoules left a comment

Choose a reason for hiding this comment

jonatack left a comment

Choose a reason for hiding this comment

jonatack Sep 28, 2022 • edited

Choose a reason for hiding this comment

brunoerg Sep 28, 2022

Choose a reason for hiding this comment

aureleoules Sep 28, 2022

Choose a reason for hiding this comment

jonatack Sep 30, 2022 • edited

Choose a reason for hiding this comment

aureleoules Sep 30, 2022

Choose a reason for hiding this comment

brunoerg Dec 6, 2022

Choose a reason for hiding this comment

aureleoules left a comment

Choose a reason for hiding this comment

jonatack Sep 30, 2022 • edited

Choose a reason for hiding this comment

jonatack left a comment

Choose a reason for hiding this comment

brunoerg commented Oct 24, 2022

brunoerg commented Oct 24, 2022

jonatack left a comment

Choose a reason for hiding this comment

aureleoules left a comment

Choose a reason for hiding this comment

brunoerg commented Oct 27, 2022

jonatack left a comment

Choose a reason for hiding this comment

jonatack commented Oct 27, 2022

aureleoules left a comment

Choose a reason for hiding this comment

brunoerg commented Dec 6, 2022

w0xlt left a comment

Choose a reason for hiding this comment

aureleoules left a comment

Choose a reason for hiding this comment

achow101 commented Dec 7, 2022

wallet, rpc: add `label` to `listsinceblock` #25934

wallet, rpc: add `label` to `listsinceblock` #25934

brunoerg commented Aug 25, 2022 •

edited

DrahtBot commented Sep 23, 2022 •

edited

jonatack Sep 28, 2022 •

edited

jonatack Sep 30, 2022 •

edited

jonatack Sep 30, 2022 •

edited