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

pull ryanofsky wants to merge 2 commits into bitcoin:master from ryanofsky:pr/list-label changing 3 files +51 −25
  1. ryanofsky commented at 5:51 AM on October 6, 2018: member

    This change partially reverts #13075 and #14023.

    Fixes #14382

  2. fanquake added the label Wallet on Oct 6, 2018
  3. 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.

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

  5. DrahtBot commented at 8:46 AM on October 6, 2018: member

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    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.

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

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

    ACK 8fcb76508493cd09d4895e7f3ad11cc5b945056e

  8. 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:

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

    Concept ACK.

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

  11. MarcoFalke added this to the milestone 0.17.1 on Oct 8, 2018
  12. MarcoFalke added the label Needs backport on Oct 8, 2018
  13. 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.

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

  15. 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?
  16. 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.

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

  18. jnewbery commented at 8:05 AM on October 9, 2018: member

    Backport is #14441. PR numbering was totally intentional.

  19. ryanofsky force-pushed on Oct 10, 2018
  20. 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

  21. ryanofsky force-pushed on Oct 10, 2018
  22. jnewbery commented at 7:23 AM on October 10, 2018: member

    ACK 4deba4ced9fc4edd1a40837c083080b6ed4c8163

  23. jnewbery commented at 7:41 AM on October 10, 2018: member

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

  24. 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 )?

  25. ryanofsky force-pushed on Oct 15, 2018
  26. 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.

  27. 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?

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

  29. jnewbery commented at 1:36 PM on October 16, 2018: member

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

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

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

  33. DrahtBot added the label Needs rebase on Nov 9, 2018
  34. laanwj referenced this in commit 5150accdd2 on Nov 10, 2018
  35. ryanofsky force-pushed on Nov 12, 2018
  36. ryanofsky referenced this in commit 671d8ee370 on Nov 12, 2018
  37. 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.

  38. MarcoFalke removed the label Needs backport on Nov 12, 2018
  39. MarcoFalke removed this from the milestone 0.17.1 on Nov 12, 2018
  40. MarcoFalke added this to the milestone 0.18.0 on Nov 12, 2018
  41. MarcoFalke added the label Needs backport on Nov 12, 2018
  42. MarcoFalke removed this from the milestone 0.18.0 on Nov 12, 2018
  43. MarcoFalke added this to the milestone 0.17.1 on Nov 12, 2018
  44. MarcoFalke removed the label Needs backport on Nov 12, 2018
  45. MarcoFalke removed this from the milestone 0.17.1 on Nov 12, 2018
  46. MarcoFalke added this to the milestone 0.18.0 on Nov 12, 2018
  47. 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)

  48. DrahtBot removed the label Needs rebase on Nov 12, 2018
  49. 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.

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

  50. [wallet] Restore ability to list incoming transactions by label
    This change partially reverts #13075 and #14023.

Fixes #14382
    65b740f92b
  51. Rename ListTransactions filter variable
    Suggested by MeshCollider <dobsonsa68@gmail.com> in
https://github.com/bitcoin/bitcoin/pull/14411#discussion_r232134086
    da427dbd48
  52. ryanofsky force-pushed on Nov 13, 2018
  53. ryanofsky referenced this in commit ae813207ff on Nov 13, 2018
  54. DrahtBot added the label Needs rebase on Nov 13, 2018
  55. ryanofsky force-pushed on Nov 13, 2018
  56. ryanofsky force-pushed on Nov 13, 2018
  57. ryanofsky force-pushed on Nov 13, 2018
  58. 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

  59. DrahtBot removed the label Needs rebase on Nov 13, 2018
  60. 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)

  61. MarcoFalke merged this on Nov 14, 2018
  62. MarcoFalke closed this on Nov 14, 2018
  63. MarcoFalke referenced this in commit e74649e951 on Nov 14, 2018
  64. jnewbery commented at 10:17 PM on November 21, 2018: member

    utACK da427dbd48ae8d12a2a79a7514a813e78064fe52

  65. JeremyRubin referenced this in commit efb550bbb3 on Nov 24, 2018
  66. HashUnlimited referenced this in commit acf214b7f6 on Nov 26, 2018
  67. deadalnix referenced this in commit 1d2c6a4bff on Mar 18, 2020
  68. ftrader referenced this in commit 26601c5358 on May 19, 2020
  69. xdustinface referenced this in commit de7c6514a3 on Dec 22, 2020
  70. xdustinface referenced this in commit 96d306324f on Dec 22, 2020
  71. UdjinM6 referenced this in commit b25d608471 on Aug 7, 2021
  72. UdjinM6 referenced this in commit 7dc5565c7d on Sep 17, 2021
  73. UdjinM6 referenced this in commit b6640644eb on Sep 24, 2021
  74. kittywhiskers referenced this in commit 0cfa457848 on Oct 12, 2021
  75. gades referenced this in commit e9ecdca335 on Mar 27, 2022
  76. gades referenced this in commit 12c7e3b4c3 on Jun 1, 2022
  77. DrahtBot locked this on Aug 16, 2022
Contributors
ryanofskyMarcoFalkeDrahtBotSaicerekristapskpromagjnewberysipameshcollider
Labels
Wallet

Milestone
0.18.0

Linked (view graph)
#13075 [tests] Remove 'account' API from wallet functional tests#14023 Remove accounts rpcs#14382 Label support seems insufficient to replace accounts#14441 [wallet] Backport(0.17): Restore ability to list incoming transactions by label
github-metadata-mirror

This is a metadata mirror of the GitHub repository bitcoin/bitcoin. This site is not affiliated with GitHub. Content is generated from a GitHub metadata backup.
generated: 2026-04-13 15:15 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me