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

pull brunoerg wants to merge 4 commits into bitcoin:master from brunoerg:2022-08-add-label-listsinceblock changing 3 files +38 −9
  1. brunoerg commented at 9:16 PM on August 25, 2022: contributor

    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.

  2. brunoerg marked this as ready for review on Aug 26, 2022
  3. glozow added the label RPC/REST/ZMQ on Aug 26, 2022
  4. in src/wallet/rpc/transactions.cpp:555 in bfb8464416 outdated 
     551 | @@ -552,6 +552,8 @@ RPCHelpMan listsinceblock()
 552 |                      {"include_removed", RPCArg::Type::BOOL, RPCArg::Default{true}, "Show transactions that were removed due to a reorg in the \"removed\" array\n"
 553 |                                                                         "(not guaranteed to work on pruned nodes)"},
 554 |                      {"include_change", RPCArg::Type::BOOL, RPCArg::Default{false}, "Also add entries for change outputs.\n"},
 555 | +                    {"label|dummy", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, should be a valid label name to return only incoming transactions\n"
    luke-jr commented at 11:30 PM on August 26, 2022:

    Why dummy?

    brunoerg commented at 12:31 AM on August 27, 2022:

    Leftover from a copy/paste. Fixed.

  5. brunoerg force-pushed on Aug 27, 2022
  6. w0xlt approved
  7. w0xlt commented at 6:42 AM on August 27, 2022: contributor

    ACK https://github.com/bitcoin/bitcoin/pull/25934/commits/cf0048926205e7c830e7f7a73a3ce4850d9e2dba

    CI error is not related

  8. in src/wallet/rpc/transactions.cpp:556 in 876dacbb55 outdated 
     551 | @@ -552,6 +552,8 @@ RPCHelpMan listsinceblock()
 552 |                      {"include_removed", RPCArg::Type::BOOL, RPCArg::Default{true}, "Show transactions that were removed due to a reorg in the \"removed\" array\n"
 553 |                                                                         "(not guaranteed to work on pruned nodes)"},
 554 |                      {"include_change", RPCArg::Type::BOOL, RPCArg::Default{false}, "Also add entries for change outputs.\n"},
 555 | +                    {"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, should be a valid label name to return only incoming transactions\n"
 556 | +                          "with the specified label, or \"*\" to disable filtering and return all transactions."},
    furszy commented at 1:44 PM on September 3, 2022:

    destinations have a label, not transactions.

    unknown commented at 2:08 PM on September 3, 2022:

    Yes in Bitcoin Core afaik

    brunoerg commented at 6:04 PM on September 23, 2022:

    destinations have a label, not transactions.

    What does it mean (related to the change in this PR)? Sorry, couldn't understand...

    aureleoules commented at 9:00 AM on September 26, 2022:

    Labels are attached to destinations (addresses), not transactions. listsinceblock will list transactions that have destinations with such label.

                        {"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, should be a valid label name to return only incoming transactions\n"
                          "with destinations of the specified label, or \"*\" to disable filtering and return all transactions."},
    brunoerg commented at 1:07 PM on September 26, 2022:

    Yeah, semantic stuff. Thanks, @aureleoules, going to address this improvement in the doc.

  9. DrahtBot commented at 9:20 AM on September 23, 2022: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--021abf342d371248e50ceaed478a90ca-->

    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.

    <!--174a7506f384e20aa4161008e828411d-->

    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.

  10. in src/wallet/rpc/transactions.cpp:640 in cf00489262 outdated 
     635 | @@ -634,6 +636,14 @@ RPCHelpMan listsinceblock()
 636 |      bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());
 637 |      bool include_change = (!request.params[4].isNull() && request.params[4].get_bool());
 638 |  
 639 | +    const std::string* filter_label = nullptr;
 640 | +    if (!request.params[5].isNull() && request.params[5].get_str() != "*") {
    aureleoules commented at 8:41 AM on September 26, 2022:

    is the * needed? can't it just be omitted as the default behavior fetches all labels anyway?

    brunoerg commented at 7:11 PM on September 26, 2022:

    I tried to keep the same pattern from other RPCs (e.g. listtransactions), but since label is the last arg, maybe we don't know need * in fact.

  11. in src/wallet/rpc/transactions.cpp:642 in cf00489262 outdated 
     635 | @@ -634,6 +636,14 @@ RPCHelpMan listsinceblock()
 636 |      bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());
 637 |      bool include_change = (!request.params[4].isNull() && request.params[4].get_bool());
 638 |  
 639 | +    const std::string* filter_label = nullptr;
 640 | +    if (!request.params[5].isNull() && request.params[5].get_str() != "*") {
 641 | +        filter_label = &request.params[5].get_str();
 642 | +        if (filter_label->empty()) {
    aureleoules commented at 8:46 AM on September 26, 2022:

    labels can be empty

    ./src/bitcoin-cli -regtest -rpcwallet=test getaddressinfo bcrt1qj4g5z8s0pugx23w094a5llwnkd0jj7ng3zyw9q      
{
  "address": "bcrt1qj4g5z8s0pugx23w094a5llwnkd0jj7ng3zyw9q",
  "scriptPubKey": "00149551411e0f0f106545cf2d7b4ffdd3b35f297a68",
  "ismine": true,
  "solvable": true,
  "desc": "wpkh([dcedcf39/84'/1'/0'/0/7]0250c12454ba8a3cf8fa61ace4ece22a6fc556f5f9dac9c8018b35cc6a068b06b8)#tw7y6quc",
  "parent_desc": "wpkh([dcedcf39/84'/1'/0']tpubDDS9RJR5jQF6yWTKbvvL3aH3ggQMcxo8KAXR6iMvkNeNSKwSrV1tYpnSMQpvTaoEq8MrMMGNHN19fNCkd3Gq7beCMod7NHaiiqn7aqjXMAN/0/*)#llezhdte",
  "iswatchonly": false,
  "isscript": false,
  "iswitness": true,
  "witness_version": 0,
  "witness_program": "9551411e0f0f106545cf2d7b4ffdd3b35f297a68",
  "pubkey": "0250c12454ba8a3cf8fa61ace4ece22a6fc556f5f9dac9c8018b35cc6a068b06b8",
  "ischange": false,
  "timestamp": 1663246193,
  "hdkeypath": "m/84'/1'/0'/0/7",
  "hdseedid": "0000000000000000000000000000000000000000",
  "hdmasterfingerprint": "dcedcf39",
  "labels": [
    ""
  ]
}
  12. in test/functional/wallet_listsinceblock.py:475 in cf00489262 outdated 
     454 | +        block_hash = self.generate(self.nodes[2], 1)[0]
 455 | +
 456 | +        new_addr_transactions = self.nodes[1].listsinceblock(label="new_addr", blockhash=block_hash)["transactions"]
 457 | +        for transaction in new_addr_transactions:
 458 | +            assert_equal(transaction["label"], "new_addr")
 459 | +
    aureleoules commented at 8:48 AM on September 26, 2022:

    If the wildcard behavior is kept, because it is the same as listtransactions, maybe add a test that verifies that wildcard and the omitted label filter behave the same. Also test with empty label.

  13. aureleoules commented at 8:51 AM on September 26, 2022: member

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

  14. in src/wallet/rpc/transactions.cpp:639 in cf00489262 outdated 
     635 | @@ -634,6 +636,14 @@ RPCHelpMan listsinceblock()
 636 |      bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());
 637 |      bool include_change = (!request.params[4].isNull() && request.params[4].get_bool());
 638 |  
 639 | +    const std::string* filter_label = nullptr;
    aureleoules commented at 8:54 AM on September 26, 2022:

    use std::optional instead of nullptr

    brunoerg commented at 5:19 PM on September 26, 2022:

    Sounds good but is there a strong reason considering ListTransactions?

    aureleoules commented at 5:26 PM on September 26, 2022:

    from https://github.com/bitcoin/bitcoin/blob/master/doc/developer-notes.md#coding-style-c-functions-and-methods

    Generally, use std::optional to represent optional by-value inputs

    I think it's worth the refactor because pointers are not intended to be used as optional values.

    brunoerg commented at 7:43 PM on September 26, 2022:

    Makes sense, I am gonna refactor ListTransactions as well. Thanks

  15. brunoerg force-pushed on Sep 26, 2022
  16. brunoerg force-pushed on Sep 26, 2022
  17. brunoerg force-pushed on Sep 26, 2022
  18. in src/wallet/rpc/transactions.cpp:639 in cd444f4a90 outdated 
     635 | @@ -634,6 +636,11 @@ RPCHelpMan listsinceblock()
 636 |      bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());
 637 |      bool include_change = (!request.params[4].isNull() && request.params[4].get_bool());
 638 |  
 639 | +    std::optional<std::string> filter_label{""};
    aureleoules commented at 8:50 AM on September 27, 2022: 
        std::optional<std::string> filter_label;

    this should fix CI

  19. in test/functional/wallet_listsinceblock.py:463 in cd444f4a90 outdated 
     458 | +
 459 | +        new_addr_transactions = self.nodes[1].listsinceblock(label="new_addr", blockhash=block_hash)["transactions"]
 460 | +        for transaction in new_addr_transactions:
 461 | +            assert_equal(transaction["label"], "new_addr")
 462 | +
 463 | +        assert_equal(self.nodes[1].listsinceblock(label="", blockhash=block_hash), self.nodes[1].listsinceblock(blockhash=block_hash))
    aureleoules commented at 8:58 AM on September 27, 2022:

    I think the test is supposed to be like this:

            assert_equal(self.nodes[1].listsinceblock(label=None, blockhash=block_hash), self.nodes[1].listsinceblock(blockhash=block_hash)) # probably not needed because it is redundant

        empty_label_transactions = self.nodes[1].listsinceblock(label="", blockhash=block_hash)["transactions"]
        for transaction in empty_label_transactions:
            assert_equal(transaction["label"], "")
  20. in src/wallet/rpc/transactions.cpp:652 in ad2bc207e2 outdated 
     648 | @@ -642,7 +649,7 @@ RPCHelpMan listsinceblock()
 649 |          const CWalletTx& tx = pairWtx.second;
 650 |  
 651 |          if (depth == -1 || abs(wallet.GetTxDepthInMainChain(tx)) < depth) {
 652 | -            ListTransactions(wallet, tx, 0, true, transactions, filter, nullptr /* filter_label */, /*include_change=*/include_change);
 653 | +            ListTransactions(wallet, tx, 0, true, transactions, filter, /*filter_label=*/filter_label.has_value() ? &filter_label.value() : nullptr, /*include_change=*/include_change);
    aureleoules commented at 9:02 AM on September 27, 2022:

    Maybe rebase the refactor commit as the first commit so you don't have to dereference the std::optional value in ad2bc207e2fe0df85cb8f0f65a9067a106487719.

    brunoerg commented at 2:06 PM on September 27, 2022:

    Great

  21. brunoerg force-pushed on Sep 27, 2022
  22. in src/wallet/rpc/transactions.cpp:651 in 115a145c70 outdated 
     641 | @@ -642,7 +642,7 @@ RPCHelpMan listsinceblock()
 642 |          const CWalletTx& tx = pairWtx.second;
 643 |  
 644 |          if (depth == -1 || abs(wallet.GetTxDepthInMainChain(tx)) < depth) {
 645 | -            ListTransactions(wallet, tx, 0, true, transactions, filter, nullptr /* filter_label */, /*include_change=*/include_change);
 646 | +            ListTransactions(wallet, tx, 0, true, transactions, filter, /*filter_label=*/filter_label, /*include_change=*/include_change);
    aureleoules commented at 2:15 PM on September 27, 2022:

    115a145c70fc5f7209a41523e3c0fbec7e3732ce won't compile because filter_label is declared in the next commit. Use std::nullopt here.

    brunoerg commented at 2:51 PM on September 27, 2022:

    Nice, thanks!

  23. in test/functional/wallet_listsinceblock.py:461 in 1c373dd345 outdated 
     456 | +        self.nodes[2].sendtoaddress(new_addr, "0.001")
 457 | +        block_hash = self.generate(self.nodes[2], 1)[0]
 458 | +
 459 | +        labels = ["new_addr", ""]
 460 | +        for label in labels:
 461 | +            new_addr_transactions = self.nodes[1].listsinceblock(label=label, blockhash=block_hash)["transactions"]
    aureleoules commented at 2:20 PM on September 27, 2022:

    This list is always empty, not sure why, so the asserts below are never executed You could probably add an assert new_addr_transactions to make sure the list is non-empty once the test is fixed.

    brunoerg commented at 2:29 PM on September 27, 2022:

    the reason could be the blockhash, going to remove it.

  24. brunoerg force-pushed on Sep 27, 2022
  25. in test/functional/wallet_listsinceblock.py:477 in 2ddf967bd7 outdated 
     456 | +        self.nodes[2].sendtoaddress(new_addr, "0.001")
 457 | +        block_hash = self.generate(self.nodes[2], 1)[0]
 458 | +
 459 | +        labels = ["new_addr", ""]
 460 | +        for label in labels:
 461 | +            new_addr_transactions = self.nodes[1].listsinceblock(label=label)["transactions"]
    aureleoules commented at 3:07 PM on September 27, 2022:

    Since you have to retouch because of the linter, I would suggest adding assert new_addr_transactions for regression testing.

                new_addr_transactions = self.nodes[1].listsinceblock(label=label)["transactions"]
            assert new_addr_transactions

    Should be ready now, good job!

  26. brunoerg force-pushed on Sep 27, 2022
  27. in src/wallet/rpc/transactions.cpp:319 in ee284ff3a5 outdated 
     315 | @@ -316,7 +316,7 @@ static void MaybePushAddress(UniValue & entry, const CTxDestination &dest)
 316 |   */
 317 |  template <class Vec>
 318 |  static void ListTransactions(const CWallet& wallet, const CWalletTx& wtx, int nMinDepth, bool fLong,
 319 | -                             Vec& ret, const isminefilter& filter_ismine, const std::string* filter_label,
 320 | +                             Vec& ret, const isminefilter& filter_ismine, const std::optional<std::string> filter_label,
    aureleoules commented at 10:17 AM on September 28, 2022:

    nit

                                 Vec& ret, const isminefilter& filter_ismine, const std::optional<std::string>& filter_label,
  28. aureleoules approved
  29. aureleoules commented at 10:17 AM on September 28, 2022: member

    ACK ee284ff3a57d6785a780ecc2c8a7ec936dc761f3

  30. in src/wallet/rpc/transactions.cpp:641 in ee284ff3a5 outdated 
     635 | @@ -634,6 +636,11 @@ RPCHelpMan listsinceblock()
 636 |      bool include_removed = (request.params[3].isNull() || request.params[3].get_bool());
 637 |      bool include_change = (!request.params[4].isNull() && request.params[4].get_bool());
 638 |  
 639 | +    std::optional<std::string> filter_label;
 640 | +    if (!request.params[5].isNull()) {
 641 | +        filter_label = request.params[5].get_str();
 642 | +    }
    jonatack commented at 11:34 AM on September 28, 2022:

    suggestion if you retouch

    -    std::optional<std::string> filter_label;
-    if (!request.params[5].isNull()) {
-        filter_label = request.params[5].get_str();
-    }
+    const std::optional<std::string> filter_label{request.params[5].isNull() ? "" : request.params[5].get_str()};
    brunoerg commented at 8:11 PM on September 28, 2022:

    I think it's not right because "" means you want to filter the results with label: "" instead of getting all of them.

  31. in doc/release-notes-25934.md:8 in ee284ff3a5 outdated 
       0 | @@ -0,0 +1,8 @@
   1 | +Low-level changes
   2 | +=================
   3 | +
   4 | +RPC
   5 | +---
   6 | +
   7 | +- The `listsinceblock` command now accepts a `label` parameter
   8 | +  to fetch incoming transactions with the specified label.
    jonatack commented at 11:34 AM on September 28, 2022:

    maybe s/with/having|filtered by/

  32. in doc/release-notes-25934.md:7 in ee284ff3a5 outdated 
       0 | @@ -0,0 +1,8 @@
   1 | +Low-level changes
   2 | +=================
   3 | +
   4 | +RPC
   5 | +---
   6 | +
   7 | +- The `listsinceblock` command now accepts a `label` parameter
    jonatack commented at 11:35 AM on September 28, 2022: 
    - RPC `listsinceblock` now accepts an optional `label` argument
    brunoerg commented at 5:41 PM on September 28, 2022:

    Wouldn't be redundant to say "RPC listsinceblock" since this sentence is already into RPC section?

    jonatack commented at 7:29 PM on September 28, 2022:

    Seems to be the usual practice and is shorter and more precise than "command" (and it's actually a call), not a blocker

    brunoerg commented at 8:13 PM on September 28, 2022:

    Cool, gonna add it. Thanks

  33. in src/wallet/rpc/transactions.cpp:555 in ee284ff3a5 outdated 
     551 | @@ -552,6 +552,8 @@ RPCHelpMan listsinceblock()
 552 |                      {"include_removed", RPCArg::Type::BOOL, RPCArg::Default{true}, "Show transactions that were removed due to a reorg in the \"removed\" array\n"
 553 |                                                                         "(not guaranteed to work on pruned nodes)"},
 554 |                      {"include_change", RPCArg::Type::BOOL, RPCArg::Default{false}, "Also add entries for change outputs.\n"},
 555 | +                    {"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If set, should be a valid label name to return only incoming transactions\n"
    jonatack commented at 11:39 AM on September 28, 2022:

    Seems this can be shorter and on one line

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

    brunoerg commented at 5:41 PM on September 28, 2022:

    Perfect, you're right. Gonna update it.

    aureleoules commented at 6:10 PM on September 28, 2022:

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

    jonatack commented at 10:18 AM on September 30, 2022:

    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?

    aureleoules commented at 11:11 AM on September 30, 2022:

    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.

    brunoerg commented at 7:09 PM on December 6, 2022:

    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.

  34. in src/wallet/rpc/transactions.cpp:787 in ee284ff3a5 outdated 
     783 | @@ -777,7 +784,7 @@ RPCHelpMan gettransaction()
 784 |      WalletTxToJSON(*pwallet, wtx, entry);
 785 |  
 786 |      UniValue details(UniValue::VARR);
 787 | -    ListTransactions(*pwallet, wtx, 0, false, details, filter, nullptr /* filter_label */);
 788 | +    ListTransactions(*pwallet, wtx, 0, false, details, filter, std::nullopt /* filter_label */);
    jonatack commented at 11:41 AM on September 28, 2022: 
        ListTransactions(*pwallet, wtx, 0, false, details, filter, /*filter_label=*/std::nullopt);
  35. in test/functional/wallet_listsinceblock.py:460 in ee284ff3a5 outdated 
     455 | +
 456 | +        self.nodes[2].sendtoaddress(new_addr, "0.001")
 457 | +        self.generate(self.nodes[2], 1)
 458 | +
 459 | +        labels = ["new_addr", ""]
 460 | +        for label in labels:
    jonatack commented at 11:53 AM on September 28, 2022:

    nit, can drop labels, it's just as clear and simpler

    -        labels = ["new_addr", ""]
-        for label in labels:
+        for label in ["new_addr", ""]:
  36. jonatack commented at 12:08 PM on September 28, 2022: contributor

    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.

  37. brunoerg force-pushed on Sep 28, 2022
  38. aureleoules approved
  39. aureleoules commented at 9:44 AM on September 30, 2022: member

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

  40. in src/wallet/rpc/transactions.cpp:555 in 2a86921ba3 outdated 
     551 | @@ -552,6 +552,7 @@ RPCHelpMan listsinceblock()
 552 |                      {"include_removed", RPCArg::Type::BOOL, RPCArg::Default{true}, "Show transactions that were removed due to a reorg in the \"removed\" array\n"
 553 |                                                                         "(not guaranteed to work on pruned nodes)"},
 554 |                      {"include_change", RPCArg::Type::BOOL, RPCArg::Default{false}, "Also add entries for change outputs.\n"},
 555 | +                    {"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Return only incoming transactions\n"},
    jonatack commented at 10:02 AM on September 30, 2022:

    Looks like the second part of the sentence was truncated in the last push. Suggestion based on listtransactions label arg help:

                        {"label", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "Return only incoming transactions paying to addresses with the specified label.\n"},
  41. in doc/release-notes-25934.md:8 in 2a86921ba3 outdated 
       0 | @@ -0,0 +1,8 @@
   1 | +Low-level changes
   2 | +=================
   3 | +
   4 | +RPC
   5 | +---
   6 | +
   7 | +- RPC `listsinceblock` now accepts an optional `label` argument
   8 | +  to fetch incoming transactions having the specified label.
    jonatack commented at 10:04 AM on September 30, 2022: 
      to fetch incoming transactions having the specified label. (#25934)
  42. in src/wallet/rpc/transactions.cpp:651 in 2a86921ba3 outdated 
     647 | @@ -642,7 +648,7 @@ RPCHelpMan listsinceblock()
 648 |          const CWalletTx& tx = pairWtx.second;
 649 |  
 650 |          if (depth == -1 || abs(wallet.GetTxDepthInMainChain(tx)) < depth) {
 651 | -            ListTransactions(wallet, tx, 0, true, transactions, filter, nullptr /* filter_label */, /*include_change=*/include_change);
 652 | +            ListTransactions(wallet, tx, 0, true, transactions, filter, /*filter_label=*/filter_label, /*include_change=*/include_change);
    jonatack commented at 10:07 AM on September 30, 2022:

    Named args are helpful when they clarify a magic-looking value or unclear variable name, but in these cases they are redundant with the variable name.

                ListTransactions(wallet, tx, 0, true, transactions, filter, filter_label, include_change);
  43. in src/wallet/rpc/transactions.cpp:668 in 2a86921ba3 outdated 
     664 | @@ -659,7 +665,7 @@ RPCHelpMan listsinceblock()
 665 |              if (it != wallet.mapWallet.end()) {
 666 |                  // We want all transactions regardless of confirmation count to appear here,
 667 |                  // even negative confirmation ones, hence the big negative.
 668 | -                ListTransactions(wallet, it->second, -100000000, true, removed, filter, nullptr /* filter_label */, /*include_change=*/include_change);
 669 | +                ListTransactions(wallet, it->second, -100000000, true, removed, filter, /*filter_label=*/filter_label, /*include_change=*/include_change);
    jonatack commented at 10:08 AM on September 30, 2022: 
                    ListTransactions(wallet, it->second, -100000000, true, removed, filter, filter_label, include_change);
  44. jonatack commented at 10:19 AM on September 30, 2022: contributor

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

  45. brunoerg force-pushed on Oct 24, 2022
  46. brunoerg force-pushed on Oct 24, 2022
  47. brunoerg commented at 4:21 PM on October 24, 2022: contributor

    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.

  48. brunoerg commented at 4:21 PM on October 24, 2022: contributor

    Force-pushed addressing @jonatack's review.

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

  49. in test/functional/wallet_listsinceblock.py:456 in 0ff870fd03 outdated 
     448 | @@ -448,6 +449,19 @@ def test_send_to_self(self):
 449 |          assert any(c["address"] == addr for c in coins)
 450 |          assert all(self.nodes[2].getaddressinfo(c["address"])["ischange"] for c in coins)
 451 |  
 452 | +    def test_label(self):
 453 | +        self.log.info("Test label")
 454 | +        new_addr = self.nodes[1].getnewaddress("new_addr", "bech32")
 455 | +
 456 | +        self.nodes[2].sendtoaddress(new_addr, "0.001")
    jonatack commented at 3:27 PM on October 26, 2022:

    A few suggestions for the test.

         def test_label(self):
-        self.log.info("Test label")
-        new_addr = self.nodes[1].getnewaddress("new_addr", "bech32")
+        self.log.info('Test passing "label" argument fetches incoming transactions having the specified label')
+        new_addr = self.nodes[1].getnewaddress(label="new_addr", address_type="bech32")
 
-        self.nodes[2].sendtoaddress(new_addr, "0.001")
+        self.nodes[2].sendtoaddress(address=new_addr, amount="0.001")
         self.generate(self.nodes[2], 1)

    Some additional test ideas:

             self.nodes[2].sendtoaddress(new_addr, "0.001")
         self.generate(self.nodes[2], 1)
 
+        lsb = self.nodes[1].listsinceblock()
+        assert_greater_than(len(lsb["transactions"]), 2)
+
         for label in ["new_addr", ""]:
             new_addr_transactions = self.nodes[1].listsinceblock(label=label)["transactions"]
-            assert new_addr_transactions
-            for transaction in new_addr_transactions:
-                assert_equal(transaction["label"], label)
+            assert_equal(len(new_addr_transactions), 1)
+            assert_equal(new_addr_transactions[0]["label"], label)
+            if label == "new_addr":
+                assert_equal(new_addr_transactions[0]["address"], new_addr)
  50. jonatack commented at 5:28 PM on October 26, 2022: contributor

    ACK 0ff870fd03ade761a4f2c158af88ec8463d49a72

  51. aureleoules approved
  52. aureleoules commented at 6:34 PM on October 26, 2022: member

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

  53. DrahtBot added the label Needs rebase on Oct 27, 2022
  54. brunoerg force-pushed on Oct 27, 2022
  55. brunoerg force-pushed on Oct 27, 2022
  56. brunoerg commented at 8:26 PM on October 27, 2022: contributor

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

  57. DrahtBot removed the label Needs rebase on Oct 27, 2022
  58. jonatack commented at 11:13 PM on October 27, 2022: contributor

    ACK c9b60d335954ecad67a70072881f8094657c5a74

  59. jonatack commented at 11:16 PM on October 27, 2022: contributor

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

  60. aureleoules approved
  61. aureleoules commented at 10:11 AM on October 28, 2022: member

    reACK c9b60d335954ecad67a70072881f8094657c5a74

  62. DrahtBot added the label Needs rebase on Dec 6, 2022
  63. refactor, wallet: use optional for `label` in `ListTransactions` 852891ff98
  64. wallet, rpc: add `label` to `listsinceblock` 722e9a418d
  65. test: add coverage for `label` in `listsinceblock` fe488b4c4b
  66. doc: add release note for 25934 4e362c2b72
  67. brunoerg force-pushed on Dec 6, 2022
  68. brunoerg commented at 7:09 PM on December 6, 2022: contributor

    Force-pushed rebasing.

  69. DrahtBot removed the label Needs rebase on Dec 6, 2022
  70. w0xlt approved
  72. aureleoules approved
  73. aureleoules commented at 2:59 PM on December 7, 2022: member

    ACK 4e362c2b7269ae0426010850c605e5c1d0d58234

  74. achow101 commented at 11:32 PM on December 7, 2022: member

    ACK 4e362c2b7269ae0426010850c605e5c1d0d58234

  75. achow101 merged this on Dec 7, 2022
  76. achow101 closed this on Dec 7, 2022
  77. sidhujag referenced this in commit f777beea0a on Dec 8, 2022
  78. bitcoin locked this on Dec 7, 2023
Contributors
brunoergluke-jrw0xltfurszyaureleoulesDrahtBotjonatackachow101
Labels
RPC/REST/ZMQ
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:13 UTC
This site is hosted by @0xB10C
More mirrored repositories can be found on mirror.b10c.me