[wallet] Restore ability to list incoming transactions by label

ryanofsky commented at 5:51 am on October 6, 2018: member

This change partially reverts #13075 and #14023.

fanquake added the label Wallet on Oct 6, 2018

MarcoFalke commented at 6:46 am on October 6, 2018: member

Is this for backport to 0.17.1? While not necessary, I believe it would simplify upgrade from 0.17.1+ to 0.18.0, because 0.17.1 could be used without -deprecatedrpc as workaround.

ryanofsky commented at 8:43 am on October 6, 2018: member

I think it’d be reasonable to backport this, since it’s a small change that restores a removed feature. But I think it won’t cherry-pick cleanly because #14023 was merged since the 0.17 branch. @Saicere originally reported the issue so they might be able to say whether there’s a benefit to backporting.

DrahtBot commented at 8:46 am on October 6, 2018: member

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#14726 (Use RPCHelpMan for all RPCs by MarcoFalke)

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.

Saicere commented at 4:45 pm on October 7, 2018: none

@Saicere originally reported the issue so they might be able to say whether there’s a benefit to backporting.

As far as I am concerned, it’s not really a big deal whether or not it’s backported to the 0.17 branch as long as it’s in before accounts are removed entirely. If you were using accounts in your application, you are probably running 0.17 with -deprecatedrpc=accounts anyway while RPC-facing code is updated.

kristapsk commented at 8:54 pm on October 7, 2018: contributor

ACK 8fcb76508493cd09d4895e7f3ad11cc5b945056e

in src/wallet/rpcwallet.cpp:1380 in 8fcb765084 outdated

1374@@ -1375,8 +1375,9 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
1375  * @param  fLong      Whether to include the JSON version of the transaction.
1376  * @param  ret        The UniValue into which the result is stored.
1377  * @param  filter     The "is mine" filter bool.
1378+ * @param  filter_label Optional label string to filter incoming transactions.
1379  */
1380-static void ListTransactions(CWallet* const pwallet, const CWalletTx& wtx, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter) EXCLUSIVE_LOCKS_REQUIRED(cs_main)
1381+static void ListTransactions(CWallet* const pwallet, const CWalletTx& wtx, int nMinDepth, bool fLong, UniValue& ret, const isminefilter& filter, const std::string* filter_label) EXCLUSIVE_LOCKS_REQUIRED(cs_main)

promag commented at 9:22 pm on October 7, 2018:

@param filter_label Optional label string to filter incoming transactions.

nit, filter_label = nullptr. Also, makes the diff smaller

ryanofsky commented at 2:17 am on October 10, 2018:

I think changing the signature is better because makes it easier to verify in the diff that there aren’t other calls that should be passing along label values. It may also make it more likely that new RPC methods added in the future will support filtering by label.

In general, I think default arguments in c++ can make code difficult to read, and should be used sparingly. If I see nullptr /* filter label */ I can see that the code actually intended to pass null instead of just forgetting to override the default. I also like being able to count from the end when functions take a lot of arguments.

jnewbery commented at 5:29 am on October 10, 2018:

I agree that default arguments should be used sparingly.

promag commented at 11:56 am on October 10, 2018:

:+1:

promag commented at 9:32 pm on October 7, 2018: member

Concept ACK.

jnewbery commented at 8:16 am on October 8, 2018: member

Concept ACK. For completeness, this functionality was hidden behind a deprecation switch in #12953 (based on #11497). The functionality was marked deprecated in #5575.

Apologies - it was my oversight that this functionality got dropped. I’m happy to backport this to V0.17 once this gets merged. I aim to review this week.

MarcoFalke added this to the milestone 0.17.1 on Oct 8, 2018

MarcoFalke added the label Needs backport on Oct 8, 2018

in test/functional/wallet_listtransactions.py:100 in 8fcb765084 outdated

 96@@ -97,9 +97,10 @@ def run_test(self):
 97         txid = self.nodes[1].sendtoaddress(multisig["address"], 0.1)
 98         self.nodes[1].generate(1)
 99         self.sync_all()
100-        assert not [tx for tx in self.nodes[0].listtransactions(dummy="*", count=100, skip=0, include_watchonly=False) if "label" in tx and tx["label"] == "watchonly"]
101-        txs = [tx for tx in self.nodes[0].listtransactions(dummy="*", count=100, skip=0, include_watchonly=True) if "label" in tx and tx['label'] == 'watchonly']
102-        assert_array_result(txs, {"category": "receive", "amount": Decimal("0.1")}, {"txid": txid})
103+        assert(len(self.nodes[0].listtransactions("watchonly", 100, 0, False)) == 0)

jnewbery commented at 3:40 am on October 9, 2018:

assert is a statement, not a function. Please remove the outer parentheses.
Is there a reason for removing the named args? It’s difficult to understand what the list of positional arguments mean.

ryanofsky commented at 2:23 am on October 10, 2018:

Fixed in 665d00fce4bc53a9ea9dc5a7d457d586af357bf3. The style changes just came from reverting to the previous code.

in test/functional/wallet_listtransactions.py:101 in 8fcb765084 outdated

 96@@ -97,9 +97,10 @@ def run_test(self):
 97         txid = self.nodes[1].sendtoaddress(multisig["address"], 0.1)
 98         self.nodes[1].generate(1)
 99         self.sync_all()
100-        assert not [tx for tx in self.nodes[0].listtransactions(dummy="*", count=100, skip=0, include_watchonly=False) if "label" in tx and tx["label"] == "watchonly"]
101-        txs = [tx for tx in self.nodes[0].listtransactions(dummy="*", count=100, skip=0, include_watchonly=True) if "label" in tx and tx['label'] == 'watchonly']
102-        assert_array_result(txs, {"category": "receive", "amount": Decimal("0.1")}, {"txid": txid})
103+        assert(len(self.nodes[0].listtransactions("watchonly", 100, 0, False)) == 0)
104+        assert_array_result(self.nodes[0].listtransactions("watchonly", 100, 0, True),

jnewbery commented at 3:40 am on October 9, 2018:

As above, please retain the named args.

ryanofsky commented at 2:24 am on October 10, 2018:

Fixed in 665d00fce4bc53a9ea9dc5a7d457d586af357bf3. The style changes just came from reverting to the previous code.

jnewbery commented at 4:19 am on October 9, 2018: member

Tested ACK 8fcb76508493cd09d4895e7f3ad11cc5b945056e with a couple of nits. @MarcoFalke - I’ll go ahead and open a PR for backporting. A couple of questions:

does the backport need release notes?
if we add release notes for the backport, do we also need release notes here?

MarcoFalke commented at 4:23 am on October 9, 2018: member

We assume the reader goes through the release notes for each release, so it should be sufficient to only mention on the 0.17. branch.

in src/wallet/rpcwallet.cpp:1467 in 8fcb765084 outdated

1464@@ -1461,9 +1465,11 @@ UniValue listtransactions(const JSONRPCRequest& request)
1465     if (request.fHelp || request.params.size() > 4)
1466         throw std::runtime_error(
1467             "listtransactions (dummy count skip include_watchonly)\n"

jnewbery commented at 5:24 am on October 9, 2018:

dummy needs to be changed to label here.

ryanofsky commented at 2:21 am on October 10, 2018:

Fixed in 665d00fce4bc53a9ea9dc5a7d457d586af357bf3

jnewbery commented at 8:05 am on October 9, 2018: member

Backport is #14441. PR numbering was totally intentional.

ryanofsky force-pushed on Oct 10, 2018

ryanofsky commented at 3:30 am on October 10, 2018: member

Added 1 commit 8fcb76508493cd09d4895e7f3ad11cc5b945056e -> 665d00fce4bc53a9ea9dc5a7d457d586af357bf3 (pr/list-label.1 -> pr/list-label.2, compare) with suggested fixes. Squashed 665d00fce4bc53a9ea9dc5a7d457d586af357bf3 -> fa7fae5c3612cf77c91b92a39c14e73478424e85 (pr/list-label.2 -> pr/list-label.3) Updated fa7fae5c3612cf77c91b92a39c14e73478424e85 -> 4deba4ced9fc4edd1a40837c083080b6ed4c8163 (pr/list-label.3 -> pr/list-label.4) with release notes changes to reflect the intent to backport this to 0.17.1

ryanofsky force-pushed on Oct 10, 2018

jnewbery commented at 7:23 am on October 10, 2018: member

ACK 4deba4ced9fc4edd1a40837c083080b6ed4c8163

jnewbery commented at 7:41 am on October 10, 2018: member

The release note should be removed from this PR (assuming #14441 is merged)

in src/wallet/rpcwallet.cpp:1467 in 4deba4ced9 outdated

1463@@ -1460,10 +1464,12 @@ UniValue listtransactions(const JSONRPCRequest& request)
1464 
1465     if (request.fHelp || request.params.size() > 4)
1466         throw std::runtime_error(
1467-            "listtransactions (dummy count skip include_watchonly)\n"
1468+            "listtransactions (label count skip include_watchonly)\n"

promag commented at 12:00 pm on October 10, 2018:

nit, fix consistency by adding spaces after ( and before )?

ryanofsky force-pushed on Oct 15, 2018

ryanofsky commented at 5:49 pm on October 15, 2018: member

Updated 4deba4ced9fc4edd1a40837c083080b6ed4c8163 -> 7cbe74f1524fc8621eb339d5eddd9530826c8461 (pr/list-label.4 -> pr/list-label.5) removing release notes and changing whitespace as suggested.

sipa commented at 9:58 pm on October 15, 2018: member

Very nitty comment; it feels like this functionality is closer to listreceivedbylabel than to listtransactions (as the latter is about all transactions which affect the balance of the wallet - and formerly account) rather than just incoming payments. Would it make sense to add it as an extra option to listreceivedbylabel (verbose or so), or a separate RPC, instead?

Saicere commented at 11:06 pm on October 15, 2018: none

Would it make sense to add it as an extra option to listreceivedbylabel (verbose or so), or a separate RPC, instead?

listreceivedbylabel doesn’t take a label argument or list transactions in any way, it just lists total received coins per label, as per the code.

jnewbery commented at 1:36 pm on October 16, 2018: member

ACK 7cbe74f1524fc8621eb339d5eddd9530826c8461. Only change since 4deba4c is removing release notes and changing whitespace.

ryanofsky commented at 4:57 pm on October 16, 2018: member

Would it make sense to add it as an extra option to listreceivedbylabel (verbose or so), or a separate RPC, instead?

It’d be good to improve listreceivedbylabel. But filtering by label in listtransactions used to work previously and I think was just removed by accident.

You could maybe say that listtransactions shouldn’t return labels or filter by label at all, since outgoing transactions won’t have labels. But I don’t think it makes sense to keep returning labels and only remove the ability to filter them.

in src/wallet/rpcwallet.cpp:1377 in 7cbe74f152 outdated

1374@@ -1375,8 +1375,9 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
1375  * @param  fLong      Whether to include the JSON version of the transaction.
1376  * @param  ret        The UniValue into which the result is stored.
1377  * @param  filter     The "is mine" filter bool.

meshcollider commented at 3:28 am on November 9, 2018:

IMO this should be renamed to avoid confusion, to something like filter_ismine

meshcollider commented at 3:35 am on November 9, 2018: contributor

utACK https://github.com/bitcoin/bitcoin/pull/14411/commits/7cbe74f1524fc8621eb339d5eddd9530826c8461

DrahtBot added the label Needs rebase on Nov 9, 2018

laanwj referenced this in commit 5150accdd2 on Nov 10, 2018

ryanofsky force-pushed on Nov 12, 2018

ryanofsky referenced this in commit 671d8ee370 on Nov 12, 2018

ryanofsky commented at 4:04 pm on November 12, 2018: member

Rebased 7cbe74f1524fc8621eb339d5eddd9530826c8461 -> 671d8ee370de4824f4ab30b904bf59514f5e52e5 (pr/list-label.5 -> pr/list-label.6) due to conflict with #14437. Also renamed filter variable as suggested by MeshCollider.

MarcoFalke removed the label Needs backport on Nov 12, 2018

MarcoFalke removed this from the milestone 0.17.1 on Nov 12, 2018

MarcoFalke added this to the milestone 0.18.0 on Nov 12, 2018

MarcoFalke added the label Needs backport on Nov 12, 2018

MarcoFalke removed this from the milestone 0.18.0 on Nov 12, 2018

MarcoFalke added this to the milestone 0.17.1 on Nov 12, 2018

MarcoFalke removed the label Needs backport on Nov 12, 2018

MarcoFalke removed this from the milestone 0.17.1 on Nov 12, 2018

MarcoFalke added this to the milestone 0.18.0 on Nov 12, 2018

MarcoFalke commented at 4:20 pm on November 12, 2018: member

Unless I am mistaken this has been “backported” in 5150accdd2a7c7f0edf964d56bd7d34b5f740cdc. So this must go into 0.18.0 instead. (assigned new milestone)

DrahtBot removed the label Needs rebase on Nov 12, 2018

in src/wallet/rpcwallet.cpp:1359 in 671d8ee370 outdated

1355@@ -1352,10 +1356,12 @@ UniValue listtransactions(const JSONRPCRequest& request)
1356 
1357     if (request.fHelp || request.params.size() > 4)
1358         throw std::runtime_error(
1359-            "listtransactions (dummy count skip include_watchonly)\n"
1360+            "listtransactions ( label count skip include_watchonly )\n"

MarcoFalke commented at 9:45 pm on November 13, 2018:

From the documentation below, label is a string, so should be surrounded by quotation marks in the oneline doc.

0            "listtransactions ( \"label\" count skip include_watchonly )\n"

ryanofsky commented at 9:56 pm on November 13, 2018:

re: #14411 (review)

From the documentation below, label is a string, so should be surrounded by quotation marks in the oneline doc.

Done in rebase.

[wallet] Restore ability to list incoming transactions by label

This change partially reverts #13075 and #14023.

Fixes #14382

65b740f92b

Rename ListTransactions filter variable

Suggested by MeshCollider <dobsonsa68@gmail.com> in
https://github.com/bitcoin/bitcoin/pull/14411#discussion_r232134086

da427dbd48

ryanofsky force-pushed on Nov 13, 2018

ryanofsky referenced this in commit ae813207ff on Nov 13, 2018

DrahtBot added the label Needs rebase on Nov 13, 2018

ryanofsky force-pushed on Nov 13, 2018

ryanofsky commented at 10:01 pm on November 13, 2018: member

Rebased 671d8ee370de4824f4ab30b904bf59514f5e52e5 -> da427dbd48ae8d12a2a79a7514a813e78064fe52 (pr/list-label.6 -> pr/list-label.7) due to conflict with #14720

DrahtBot removed the label Needs rebase on Nov 13, 2018

MarcoFalke commented at 4:57 pm on November 14, 2018: member

ACK da427dbd48ae8d12a2a79a7514a813e78064fe52 (Only checked that the rebase was done properly from the last time people reviewed this. Also checked that the tests are the same as in the backport 5150accdd2a7c7f0edf964d56bd7d34b5f740cdc, didn’t look at the code)

MarcoFalke merged this on Nov 14, 2018

MarcoFalke closed this on Nov 14, 2018

MarcoFalke referenced this in commit e74649e951 on Nov 14, 2018

jnewbery commented at 10:17 pm on November 21, 2018: member

utACK da427dbd48ae8d12a2a79a7514a813e78064fe52

JeremyRubin referenced this in commit efb550bbb3 on Nov 24, 2018

HashUnlimited referenced this in commit acf214b7f6 on Nov 26, 2018

deadalnix referenced this in commit 1d2c6a4bff on Mar 18, 2020

ftrader referenced this in commit 26601c5358 on May 19, 2020

xdustinface referenced this in commit de7c6514a3 on Dec 22, 2020

xdustinface referenced this in commit 96d306324f on Dec 22, 2020

UdjinM6 referenced this in commit b25d608471 on Aug 7, 2021

UdjinM6 referenced this in commit 7dc5565c7d on Sep 17, 2021

UdjinM6 referenced this in commit b6640644eb on Sep 24, 2021

kittywhiskers referenced this in commit 0cfa457848 on Oct 12, 2021

gades referenced this in commit e9ecdca335 on Mar 27, 2022

gades referenced this in commit 12c7e3b4c3 on Jun 1, 2022

DrahtBot locked this on Aug 16, 2022

[wallet] Restore ability to list incoming transactions by label #14411

Conflicts