[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: contributor

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

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

    <!--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. DrahtBot cross-referenced this on Oct 6, 2018 from issue Refactor: separate wallet from node by ryanofsky
  7. DrahtBot cross-referenced this on Oct 6, 2018 from issue Multiprocess bitcoin by ryanofsky
  8. 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.

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

    ACK 8fcb76508493cd09d4895e7f3ad11cc5b945056e

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

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

    Concept ACK.

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

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

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

  17. DrahtBot cross-referenced this on Oct 9, 2018 from issue Refactor: Start to separate wallet from node by ryanofsky
  18. 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?
  19. 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.

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

  21. jnewbery cross-referenced this on Oct 9, 2018 from issue [wallet] Backport(0.17): Restore ability to list incoming transactions by label by jnewbery
  22. jnewbery commented at 8:05 AM on October 9, 2018: member

    Backport is #14441. PR numbering was totally intentional.

  23. ryanofsky force-pushed on Oct 10, 2018
  24. ryanofsky commented at 3:30 AM on October 10, 2018: contributor

    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

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

    ACK 4deba4ced9fc4edd1a40837c083080b6ed4c8163

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

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

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

  29. ryanofsky force-pushed on Oct 15, 2018
  30. ryanofsky commented at 5:49 PM on October 15, 2018: contributor

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

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

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

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

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

  34. ryanofsky commented at 4:57 PM on October 16, 2018: contributor

    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.

  35. DrahtBot cross-referenced this on Oct 20, 2018 from issue Rpc help helper class by karelbilek
  36. DrahtBot cross-referenced this on Oct 20, 2018 from issue docs: Consistent type names in RPC help descriptions by ch4ot1c
  37. kristapsk cross-referenced this on Oct 21, 2018 from issue Switch over to using labels instead of accounts by chris-belcher
  38. practicalswift cross-referenced this on Oct 22, 2018 from issue Enable flake8 rule E225 which checks for missing whitespace around op… by jbampton
  39. DrahtBot cross-referenced this on Oct 26, 2018 from issue Overhaul importmulti logic by sipa
  40. 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

  42. DrahtBot added the label Needs rebase on Nov 9, 2018
  43. laanwj referenced this in commit 5150accdd2 on Nov 10, 2018
  44. ryanofsky force-pushed on Nov 12, 2018
  45. ryanofsky referenced this in commit 671d8ee370 on Nov 12, 2018
  46. ryanofsky commented at 4:04 PM on November 12, 2018: contributor

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

  47. MarcoFalke removed the label Needs backport on Nov 12, 2018
  48. MarcoFalke removed this from the milestone 0.17.1 on Nov 12, 2018
  49. MarcoFalke added this to the milestone 0.18.0 on Nov 12, 2018
  50. MarcoFalke added the label Needs backport on Nov 12, 2018
  51. MarcoFalke removed this from the milestone 0.18.0 on Nov 12, 2018
  52. MarcoFalke added this to the milestone 0.17.1 on Nov 12, 2018
  53. MarcoFalke removed the label Needs backport on Nov 12, 2018
  54. MarcoFalke removed this from the milestone 0.17.1 on Nov 12, 2018
  55. MarcoFalke added this to the milestone 0.18.0 on Nov 12, 2018
  56. 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)

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

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

Fixes #14382
    65b740f92b
  60. Rename ListTransactions filter variable
    Suggested by MeshCollider <dobsonsa68@gmail.com> in
https://github.com/bitcoin/bitcoin/pull/14411#discussion_r232134086
    da427dbd48
  61. ryanofsky force-pushed on Nov 13, 2018
  62. ryanofsky referenced this in commit ae813207ff on Nov 13, 2018
  63. DrahtBot added the label Needs rebase on Nov 13, 2018
  64. ryanofsky force-pushed on Nov 13, 2018
  65. ryanofsky force-pushed on Nov 13, 2018
  66. ryanofsky force-pushed on Nov 13, 2018
  67. ryanofsky commented at 10:01 PM on November 13, 2018: contributor

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

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

  70. MarcoFalke merged this on Nov 14, 2018
  71. MarcoFalke closed this on Nov 14, 2018
  72. MarcoFalke referenced this in commit e74649e951 on Nov 14, 2018
  73. jnewbery commented at 10:17 PM on November 21, 2018: member

    utACK da427dbd48ae8d12a2a79a7514a813e78064fe52

  74. JeremyRubin referenced this in commit efb550bbb3 on Nov 24, 2018
  75. HashUnlimited referenced this in commit acf214b7f6 on Nov 26, 2018
  76. deadalnix referenced this in commit 1d2c6a4bff on Mar 18, 2020
  77. ftrader referenced this in commit 26601c5358 on May 19, 2020
  78. kristapsk cross-referenced this on Dec 12, 2020 from issue Drop support for pre-0.17 Bitcoin Core by kristapsk
  79. xdustinface referenced this in commit de7c6514a3 on Dec 22, 2020
  80. xdustinface referenced this in commit 96d306324f on Dec 22, 2020
  81. UdjinM6 cross-referenced this on Dec 28, 2020 from issue backport: Deprecate accounts by xdustinface
  82. UdjinM6 referenced this in commit b25d608471 on Aug 7, 2021
  83. UdjinM6 referenced this in commit 7dc5565c7d on Sep 17, 2021
  84. UdjinM6 referenced this in commit b6640644eb on Sep 24, 2021
  85. kwvg referenced this in commit 0cfa457848 on Oct 12, 2021
  86. gades referenced this in commit e9ecdca335 on Mar 27, 2022
  87. gades referenced this in commit 12c7e3b4c3 on Jun 1, 2022
  88. bitcoin locked this on Aug 16, 2022
Contributors
ryanofskyMarcoFalkeDrahtBotSaicerekristapskpromagjnewberysipameshcollider
Labels
Wallet

Milestone
0.18.0

Linked (view graph)
#5575 RPCWallet: Notate all account stuff as deprecated#11497 Hide accounts system behind deprecation switch#12953 Deprecate accounts#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#14534 Enable flake8 rule E225 which checks for missing whitespace around op…
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-05-20 06:54 UTC