[RPC] Include coinbase transactions in receivedby RPCs #14707

andrewtoth commented at 2:20 AM on November 12, 2018: contributor

The current *receivedby* RPCs filter out coinbase transactions. This doesn't seem correct since an output to your address in a coinbase transaction is receiving those coins.

This PR corrects this behaviour. Also, a new option include_immature_coinbase is added (default=false) that includes immature coinbase transactions when set to true.

However, since this is potentially a breaking change this PR introduces a hidden configuration option -deprecatedrpc=exclude_coinbase. This can be set to revert to previous behaviour. If no reports of broken workflow are received, then this option can be removed in a future release.

Fixes #14654.

meshcollider added the label RPC/REST/ZMQ on Nov 12, 2018

sipa commented at 2:55 AM on November 12, 2018: member

Concept ACK, but you can't break compatibility by reordering another argument to listreceivedbyaddress. It's also unnecessary I think; you can specify "null" to get the default.

andrewtoth commented at 4:11 AM on November 12, 2018: contributor

@sipa Not sure how to pass "null" as address_filter parameter without breaking #14417.

in src/wallet/rpcwallet.cpp:1056 in f3d8ca1f98 outdated

1053 |      bool has_filtered_address = false;
1054 |      CTxDestination filtered_address = CNoDestination();
1055 | -    if (!by_label && params.size() > 3) {
1056 | -        if (!IsValidDestinationString(params[3].get_str())) {
1057 | +    if (!by_label && params.size() > 4) {
1058 | +        if (!IsValidDestinationString(params[4].get_str())) {

meshcollider commented at 5:41 AM on November 12, 2018:

Can't you just check if address_filter is an empty string?

in src/wallet/rpcwallet.cpp:1197 in f3d8ca1f98 outdated

1195 |              "\nArguments:\n"
1196 |              "1. minconf           (numeric, optional, default=1) The minimum number of confirmations before payments are included.\n"
1197 |              "2. include_empty     (bool, optional, default=false) Whether to include addresses that haven't received any payments.\n"
1198 |              "3. include_watchonly (bool, optional, default=false) Whether to include watch-only addresses (see 'importaddress').\n"
1199 | -            "4. address_filter    (string, optional) If present, only return information on this address.\n"
1200 | +            "4. include_coinbase  (bool, optional, default=false) Whether to include coinbase transactions.\n"

promag commented at 11:49 AM on November 12, 2018:

Breaking change here, don't change argument order, description above is correct.

in src/rpc/client.cpp:50 in f3d8ca1f98 outdated

  45 |      { "getreceivedbylabel", 1, "minconf" },
  46 | +    { "getreceivedbylabel", 2, "include_coinbase" },
  47 |      { "listreceivedbyaddress", 0, "minconf" },
  48 |      { "listreceivedbyaddress", 1, "include_empty" },
  49 |      { "listreceivedbyaddress", 2, "include_watchonly" },
  50 | +    { "listreceivedbyaddress", 3, "include_coinbase" },

promag commented at 11:55 AM on November 12, 2018:

Should be 4.

in src/wallet/rpcwallet.cpp:1244 in f3d8ca1f98 outdated

1238 | @@ -1215,14 +1239,15 @@ static UniValue listreceivedbylabel(const JSONRPCRequest& request)
1239 |          return NullUniValue;
1240 |      }
1241 |  
1242 | -    if (request.fHelp || request.params.size() > 3)
1243 | +    if (request.fHelp || request.params.size() > 4)
1244 |          throw std::runtime_error(
1245 |              "listreceivedbylabel ( minconf include_empty include_watchonly)\n"

promag commented at 11:56 AM on November 12, 2018:

Missing new argument.

in src/wallet/rpcwallet.cpp:637 in f3d8ca1f98 outdated

 631 | @@ -624,13 +632,14 @@ static UniValue getreceivedbylabel(const JSONRPCRequest& request)
 632 |          return NullUniValue;
 633 |      }
 634 |  
 635 | -    if (request.fHelp || request.params.size() < 1 || request.params.size() > 2)
 636 | +    if (request.fHelp || request.params.size() < 1 || request.params.size() > 3)
 637 |          throw std::runtime_error(
 638 |              "getreceivedbylabel \"label\" ( minconf )\n"

promag commented at 11:56 AM on November 12, 2018:

Missing new argument.

promag commented at 11:57 AM on November 12, 2018: member

Concept ACK

@sipa Not sure how to pass "null" as address_filter parameter without breaking #14417.

You can use named arguments instead.

Could have release note of the changed RPC's and the new argument.

gmaxwell commented at 8:21 PM on November 12, 2018: contributor

Do we know a usecase where anyone would want this behaviour? It seems transparently broken to me.

If not, I think I would prefer we fix it and have a LOUD release note about the change, and if we're feeling especially conservative have a hidden config option to restore the old behaviour which we can drop after a couple releases if no one reports needing it.

Having an option presents a perpetual usage complexity increase, and defaulting the option to the old behaviour risks users being messed up by continuing to miss these payments.

One thing to consider, however, is that coinbase payments have different deconfirmation risks then general transactions... and cannot be spent for 100 blocks. E.g. you don't want service that accept a 1 conf deposit and withdraw being exploited as a orphan risk eliminator. Nor do we want to undermine parties cashflow handling by causing them to think they can spend more now than they can. There is justification for treating coinbase payments differently (e.g. only showing them once they're spendable), just not for hiding them completely. Having a "include coinbase payments yes/no" doesn't solve these concerns. Having a "delay them by 100 blocks, yes/no" probably would.

So maybe I would suggest having an argument to hide them until maturity, defaulting on, a hidden option to get the old behaviour, defaulting off, to be removed once it's confirmed no one needs it, and a loud release note explaining the change.

promag commented at 8:34 PM on November 12, 2018: member

@gmaxwell it is however useful to know if a payment was made regardless if it takes 100 blocks to be spendable?

gmaxwell commented at 11:18 PM on November 12, 2018: contributor

It can be, but listreceivedbyaddress doesn't normally show unconfirmed payments and the output IIRC doesn't even tell you if its an immature coinbase payment (and if it did, many people who didn't even realize coinbase payments were possible would not know to check for it)

promag commented at 11:39 PM on November 12, 2018: member

@gmaxwell what if we consider minconf the depth added to the minimum spendable depth? Then minconf=1 for coinbase transactions would be 101?

andrewtoth commented at 12:07 AM on November 13, 2018: contributor

@gmaxwell I agree. I think it is an error that should be corrected. I'm not sure why anyone would want coinbase txs excluded. I haven't seen a good explanation for why they are filtered out, or a usecase.

DrahtBot commented at 3:20 PM on November 13, 2018: contributor

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

Conflicts

Reviewers, this pull request conflicts with the following ones:

#20892 (tests: Run both descriptor and legacy tests within a single test invocation by achow101)

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.

andrewtoth force-pushed on Nov 15, 2018

andrewtoth commented at 4:56 AM on November 15, 2018: contributor

Commits and PR description have been updated.

andrewtoth force-pushed on Nov 16, 2018

andrewtoth commented at 6:52 PM on November 16, 2018: contributor

I refactored the getreceivedby calls to use a common function GetReceived. This mirrors the listreceivedby calls and doesn't repeat as much code.

andrewtoth force-pushed on Nov 16, 2018

andrewtoth renamed this:
~~[RPC] Add include_coinbase option to receiveby RPCs~~
[RPC] Include coinbase transactions in receiveby RPCs
on Nov 17, 2018

andrewtoth renamed this:
~~[RPC] Include coinbase transactions in receiveby RPCs~~
[RPC] Include coinbase transactions in receivedby RPCs
on Nov 17, 2018

andrewtoth force-pushed on Nov 17, 2018

andrewtoth force-pushed on Nov 20, 2018

andrewtoth commented at 2:43 AM on November 20, 2018: contributor

@gmaxwell Updated this PR with your suggestions.

andrewtoth force-pushed on Nov 23, 2018

DrahtBot added the label Needs rebase on Nov 23, 2018

andrewtoth force-pushed on Nov 23, 2018

DrahtBot removed the label Needs rebase on Nov 23, 2018

andrewtoth force-pushed on Nov 24, 2018

andrewtoth force-pushed on Dec 1, 2018

DrahtBot added the label Needs rebase on Dec 5, 2018

andrewtoth force-pushed on Dec 6, 2018

DrahtBot removed the label Needs rebase on Dec 6, 2018

DrahtBot added the label Needs rebase on Dec 10, 2018

andrewtoth force-pushed on Dec 10, 2018

DrahtBot removed the label Needs rebase on Dec 10, 2018

DrahtBot added the label Needs rebase on Jan 29, 2019

andrewtoth force-pushed on Feb 1, 2019

DrahtBot removed the label Needs rebase on Feb 1, 2019

maflcko added the label Needs rebase on Feb 12, 2019

in src/wallet/rpcwallet.cpp:653 in 7b5db83754 outdated

 649 |              RPCHelpMan{"getreceivedbyaddress",
 650 |                  "\nReturns the total amount received by the given address in transactions with at least minconf confirmations.\n",
 651 |                  {
 652 |                      {"address", RPCArg::Type::STR, /* opt */ false, /* default_val */ "", "The bitcoin address for transactions."},
 653 |                      {"minconf", RPCArg::Type::NUM, /* opt */ true, /* default_val */ "1", "Only include transactions confirmed at least this many times."},
 654 | +                    {"include_immature", RPCArg::Type::BOOL, /* opt */ true, /* default_val */ "false", "Include immature coinbase transactions."},

maflcko commented at 11:52 PM on February 12, 2019:

                    {"include_immature", RPCArg::Type::BOOL, /* default */ "false", "Include immature coinbase transactions."},

andrewtoth commented at 4:12 AM on February 14, 2019:

Done

andrewtoth force-pushed on Feb 14, 2019

DrahtBot removed the label Needs rebase on Feb 14, 2019

andrewtoth force-pushed on Feb 16, 2019

DrahtBot added the label Needs rebase on Mar 4, 2019

andrewtoth force-pushed on Mar 10, 2019

DrahtBot removed the label Needs rebase on Mar 10, 2019

practicalswift commented at 4:14 PM on May 7, 2019: contributor

This PR doesn't compile when rebased on master. Are you still working on this @andrewtoth? :-)

andrewtoth commented at 11:17 PM on May 7, 2019: contributor

@practicalswift Hmm I rebased and it compiled fine. I'll push the rebase anyways.

andrewtoth force-pushed on May 7, 2019

DrahtBot added the label Needs rebase on Jul 9, 2019

andrewtoth force-pushed on Jul 11, 2019

DrahtBot removed the label Needs rebase on Jul 12, 2019

andrewtoth force-pushed on Jul 28, 2019

andrewtoth force-pushed on Jul 29, 2019

DrahtBot added the label Needs rebase on Aug 16, 2019

andrewtoth force-pushed on Aug 18, 2019

DrahtBot removed the label Needs rebase on Aug 18, 2019

DrahtBot added the label Needs rebase on Sep 2, 2019

andrewtoth force-pushed on Oct 1, 2019

DrahtBot removed the label Needs rebase on Oct 1, 2019

DrahtBot added the label Needs rebase on Oct 29, 2019

andrewtoth force-pushed on Nov 16, 2019

DrahtBot removed the label Needs rebase on Nov 16, 2019

DrahtBot added the label Needs rebase on Mar 11, 2020

maflcko referenced this in commit 3be119c0f6 on Apr 20, 2020

andrewtoth force-pushed on Apr 20, 2020

DrahtBot removed the label Needs rebase on Apr 20, 2020

sidhujag referenced this in commit 3782e79671 on Apr 20, 2020

andrewtoth commented at 1:48 PM on April 30, 2020: contributor

I've update this after #17579 so it should be much easier to review. One thing I'm not sure of is how to make the final argument hidden with RPCHelpMan.

maflcko commented at 2:08 PM on April 30, 2020: member

Can you explain why you want to hide an argument, especially since you suggest users set the argument to false if they run into problems. In other words you are asking users to blindly pass an undocumented value into an RPC.

andrewtoth commented at 2:14 PM on April 30, 2020: contributor

Seems I misread one of the comments:

if we're feeling especially conservative have a hidden config option to restore the old behaviour which we can drop after a couple releases if no one reports needing it.

So, it should be a hidden config option instead of a hidden argument. I can update to include the config option, but do you think it's necessary?

maflcko commented at 2:18 PM on April 30, 2020: member

Oh, I didn't recall that comment. I see two options:

Show the argument in the RPC help, but explain that it is not supposed to be toggled unless needed and that it is going away in a few releases.
Or, add a debug config option that says the same.

DrahtBot added the label Needs rebase on May 1, 2020

andrewtoth force-pushed on May 2, 2020

andrewtoth commented at 7:40 PM on May 2, 2020: contributor

@MarcoFalke thanks I've updated with a hidden config option.

DrahtBot removed the label Needs rebase on May 2, 2020

DrahtBot added the label Needs rebase on Sep 23, 2020

adamjonas commented at 5:13 PM on December 17, 2020: member

@andrewtoth ping for a rebase.

andrewtoth force-pushed on Dec 17, 2020

DrahtBot removed the label Needs rebase on Dec 17, 2020

andrewtoth force-pushed on Dec 18, 2020

in src/wallet/rpcwallet.cpp:674 in f84ff8f984 outdated

 670 | @@ -671,11 +671,22 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b
 671 |      if (!params[1].isNull())
 672 |          min_depth = params[1].get_int();
 673 |  
 674 | +    bool include_immature{params[2].isNull() ? false : params[2].get_bool()};

LarryRuane commented at 9:15 PM on December 30, 2020:

nit, if you have a chance to retouch

    bool include_immature{!params[2].isNull() && params[2].get_bool()};

in src/wallet/rpcwallet.cpp:1091 in f84ff8f984 outdated

1087 |      }
1088 |  
1089 | +    // getreceivedbyaddress has address filter at 3rd index, so next params index is 4
1090 | +    // getreceivedbylabel has no filter parameter, so next params index is 3
1091 | +    int next_index{by_label ? 3 : 4};
1092 | +    bool include_immature{params[next_index].isNull() ? false : params[next_index].get_bool()};

LarryRuane commented at 12:35 AM on December 31, 2020:

    bool include_immature{!params[next_index].isNull() && params[next_index].get_bool()};

in src/wallet/rpcwallet.cpp:967 in f84ff8f984 outdated

1110 |  
1111 | +        // Coinbase with less than 1 confirmation is an orphan
1112 | +        if ((wtx.IsCoinBase() && (nDepth < 1 || filter_coinbase))
1113 | +            || (!include_immature && wtx.IsImmatureCoinBase())
1114 | +            || !pwallet->chain().checkFinalTx(*wtx.tx))
1115 | +            continue;

LarryRuane commented at 7:14 AM on December 31, 2020:

Consider enclosing the continue; within braces? I'm aware they weren't there previously, but when the condition becomes multi-line, perhaps they help with readability.

andrewtoth commented at 9:12 PM on February 12, 2021:

Thanks for the review! I've updated with your suggestions along with the latest rebase.

LarryRuane commented at 7:17 AM on December 31, 2020: contributor

ACK f84ff8f98494eff2bcfac970c7eef1d1de5f4c51 Ran tests, reviewed code and tests in detail, looks good. My suggestions are nonblocking.

in src/wallet/rpcwallet.cpp:4614 in f84ff8f984 outdated

4609 | @@ -4578,8 +4610,8 @@ static const CRPCCommand commands[] =
4610 |      { "wallet",             "listaddressgroupings",             &listaddressgroupings,          {} },
4611 |      { "wallet",             "listlabels",                       &listlabels,                    {"purpose"} },
4612 |      { "wallet",             "listlockunspent",                  &listlockunspent,               {} },
4613 | -    { "wallet",             "listreceivedbyaddress",            &listreceivedbyaddress,         {"minconf","include_empty","include_watchonly","address_filter"} },
4614 | -    { "wallet",             "listreceivedbylabel",              &listreceivedbylabel,           {"minconf","include_empty","include_watchonly"} },
4615 | +    { "wallet",             "listreceivedbyaddress",            &listreceivedbyaddress,         {"minconf","include_empty","include_watchonly","address_filter","include_immature"} },
4616 | +    { "wallet",             "listreceivedbylabel",              &listreceivedbylabel,           {"minconf","include_empty","include_watchonly","include_immature"} },

maflcko commented at 6:45 PM on January 28, 2021:

can remove the diff in commands, and then rebase

andrewtoth commented at 9:13 PM on February 12, 2021:

Done, thanks!

DrahtBot added the label Needs rebase on Jan 28, 2021

andrewtoth force-pushed on Feb 12, 2021

DrahtBot removed the label Needs rebase on Feb 12, 2021

DrahtBot added the label Needs rebase on Mar 10, 2021

bitcoin deleted a comment on Mar 13, 2021

andrewtoth force-pushed on Apr 6, 2021

DrahtBot removed the label Needs rebase on Apr 6, 2021

in src/wallet/rpcwallet.cpp:1089 in c7c662bf72 outdated

1085 | +    int next_index{by_label ? 3 : 4};
1086 | +    bool include_immature{!params[next_index].isNull() && params[next_index].get_bool()};
1087 | +
1088 | +    // Filtering out coinbase outputs is deprecated
1089 | +    // It can be enabled by setting deprecatedrpc=filter_coinbase
1090 | +    bool filter_coinbase{pwallet->chain().rpcEnableDeprecated("filter_coinbase")};

adamjonas commented at 9:43 PM on April 6, 2021:

getting a compile error after the latest rebase:

wallet/rpcwallet.cpp:1089:26: error: use of undeclared identifier 'pwallet'
    bool filter_coinbase{pwallet->chain().rpcEnableDeprecated("filter_coinbase")};

    bool filter_coinbase{wallet.chain().rpcEnableDeprecated("filter_coinbase")};

andrewtoth commented at 12:26 AM on April 7, 2021:

Oof forgot to include that with the rebase. Thanks for checking!

andrewtoth force-pushed on Apr 7, 2021

in src/wallet/rpcwallet.cpp:1083 in 77da82934c outdated

1079 |          filtered_address = DecodeDestination(params[3].get_str());
1080 |          has_filtered_address = true;
1081 |      }
1082 |  
1083 | +    // getreceivedbyaddress has address filter at 3rd index, so next params index is 4
1084 | +    // getreceivedbylabel has no filter parameter, so next params index is 3

LarryRuane commented at 5:35 AM on April 10, 2021:

In these two comments, should "getreceivedby" be "listreceivedby"?

andrewtoth commented at 7:04 PM on April 19, 2021:

These lines were removed in the latest push.

in src/wallet/rpcwallet.cpp:1223 in 77da82934c outdated

1218 | @@ -1189,6 +1219,7 @@ static RPCHelpMan listreceivedbyaddress()
1219 |                      {"include_empty", RPCArg::Type::BOOL, /* default */ "false", "Whether to include addresses that haven't received any payments."},
1220 |                      {"include_watchonly", RPCArg::Type::BOOL, /* default */ "true for watch-only wallets, otherwise false", "Whether to include watch-only addresses (see 'importaddress')"},
1221 |                      {"address_filter", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If present, only return information on this address."},

LarryRuane commented at 5:49 AM on April 10, 2021:

                    {"address_filter", RPCArg::Type::STR, RPCArg::Optional::OMITTED_NAMED_ARG, "If present and nonempty, only return information on this address."},

This matches the code and explains how to specify include_immature without having to specify an address filter.

bitcoin deleted a comment on Apr 10, 2021

in src/wallet/rpcwallet.cpp:674 in 77da82934c outdated

 670 | @@ -671,11 +671,22 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b
 671 |      if (!params[1].isNull())
 672 |          min_depth = params[1].get_int();
 673 |  
 674 | +    bool include_immature{!params[2].isNull() && params[2].get_bool()};

jnewbery commented at 10:26 AM on April 14, 2021:

All these new variables (both here and in ListReceived()) can be const.

in src/wallet/rpcwallet.cpp:688 in 77da82934c outdated

 684 | -        if (wtx.IsCoinBase() || !wallet.chain().checkFinalTx(*wtx.tx) || wtx.GetDepthInMainChain() < min_depth) {
 685 | +        int depth{wtx.GetDepthInMainChain()};
 686 | +        if (depth < min_depth
 687 | +            // Coinbase with less than 1 confirmation is an orphan
 688 | +            || (wtx.IsCoinBase() && (depth < 1 || filter_coinbase))
 689 | +            || (!include_immature && wtx.IsImmatureCoinBase())

jnewbery commented at 10:31 AM on April 14, 2021:

Consistency helps readability. I think you can make this more consistent and readable as:

            || (wtx.IsCoinBase() && (depth < 1 || !include_coinbase))
            || (wtx.IsImmatureCoinBase() && !include_immature_coinbase)

(where include_coinbase = !(wallet.chain().rpcEnableDeprecated("filter_coinbase")))

LarryRuane commented at 3:45 PM on April 14, 2021:

Another idea, this logic is close to that added to ListReceived() further down in this file; have you considered refactoring this logic into a separate small function? This may make the logic unit-testable, but may not be worth it.

andrewtoth commented at 7:19 PM on April 19, 2021:

@jnewbery Great suggestion, I agree this makes it much more readable. @LarryRuane Since there are only two instances of this duplication, should we defer to the rule of three)?

in src/wallet/rpcwallet.cpp:587 in 77da82934c outdated

 720 | @@ -709,8 +721,11 @@ static RPCHelpMan getreceivedbyaddress()
 721 |              + HelpExampleCli("getreceivedbyaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 0") +
 722 |              "\nThe amount with at least 6 confirmations\n"
 723 |              + HelpExampleCli("getreceivedbyaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 6") +
 724 | +            "\nThe amount with at least 6 confirmations including immature coinbase outputs\n"
 725 | +            + HelpExampleCli("getreceivedbyaddress", "\"" + EXAMPLE_ADDRESS[0] + "\" 6 true") +
 726 |              "\nAs a JSON-RPC call\n"

jnewbery commented at 10:32 AM on April 14, 2021:

It's odd that "As a JSON-RPC call" no longer refers to the example immediately above. I think only one JSON-RPC example is needed.

in src/wallet/rpcwallet.cpp:1085 in 77da82934c outdated

1081 |      }
1082 |  
1083 | +    // getreceivedbyaddress has address filter at 3rd index, so next params index is 4
1084 | +    // getreceivedbylabel has no filter parameter, so next params index is 3
1085 | +    int next_index{by_label ? 3 : 4};
1086 | +    bool include_immature{!params[next_index].isNull() && params[next_index].get_bool()};

jnewbery commented at 10:36 AM on April 14, 2021:

C++ is a strongly typed language - we should use types in the function signatures to catch logic errors at compilation rather than pass through opaque structures.

Can you update the function signature of ListReceived() to explicitly take the argument types?

in src/wallet/rpcwallet.cpp:1089 in 77da82934c outdated

1085 | +    int next_index{by_label ? 3 : 4};
1086 | +    bool include_immature{!params[next_index].isNull() && params[next_index].get_bool()};
1087 | +
1088 | +    // Filtering out coinbase outputs is deprecated
1089 | +    // It can be enabled by setting deprecatedrpc=filter_coinbase
1090 | +    bool filter_coinbase{wallet.chain().rpcEnableDeprecated("filter_coinbase")};

jnewbery commented at 10:37 AM on April 14, 2021:

Again, I think you should be consistent and either have include_coinbase and include_immature_coinbase or exclude_coinbase and exclude_immature_coinbase as variables.

in src/wallet/rpcwallet.cpp:686 in 77da82934c outdated

 682 |      for (const std::pair<const uint256, CWalletTx>& wtx_pair : wallet.mapWallet) {
 683 |          const CWalletTx& wtx = wtx_pair.second;
 684 | -        if (wtx.IsCoinBase() || !wallet.chain().checkFinalTx(*wtx.tx) || wtx.GetDepthInMainChain() < min_depth) {
 685 | +        int depth{wtx.GetDepthInMainChain()};
 686 | +        if (depth < min_depth
 687 | +            // Coinbase with less than 1 confirmation is an orphan

jnewbery commented at 10:39 AM on April 14, 2021:

"orphan" is a very overloaded word, and I don't think it's correct here. I think you're trying to say that the coinbase transaction has been conflicted out of the main chain?

in src/wallet/rpcwallet.cpp:677 in 77da82934c outdated

 670 | @@ -671,11 +671,22 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b
 671 |      if (!params[1].isNull())
 672 |          min_depth = params[1].get_int();
 673 |  
 674 | +    bool include_immature{!params[2].isNull() && params[2].get_bool()};
 675 | +
 676 | +    // Filtering out coinbase outputs is deprecated
 677 | +    // It can be enabled by setting deprecatedrpc=filter_coinbase

jnewbery commented at 10:42 AM on April 14, 2021:

perhaps s/filter_coinbase/exclude_coinbase/ is more explicit.

in test/functional/wallet_listreceivedby.py:172 in 77da82934c outdated

 168 | @@ -167,5 +169,117 @@ def run_test(self):
 169 |          balance = self.nodes[1].getreceivedbylabel("mynewlabel")
 170 |          assert_equal(balance, Decimal("0.0"))
 171 |  
 172 | +        self.run_include_coinbase_test()

jnewbery commented at 10:45 AM on April 14, 2021:

I don't see much benefit in splitting this out into a separate function, unless you also restructure the run_test() function to be:

    def run_test(self):
        self.run_listbyreceived_test()
        self.run_include_coinbase_test()

in test/functional/wallet_listreceivedby.py:240 in 77da82934c outdated

 235 | +        self.log.info("listreceivedbylabel includes label with defaults")
 236 | +        assert_array_result(self.nodes[0].listreceivedbylabel(),
 237 | +                            {"label": label},
 238 | +                            {"label": label, "amount": reward})
 239 | +
 240 | +        self.log.info("Orphan block that paid to address")

jnewbery commented at 10:46 AM on April 14, 2021:

"Orphan" is wrong here. Perhaps "Invalidate".

(also update references to 'orphan' below)

in test/functional/wallet_listreceivedby.py:263 in 77da82934c outdated

 258 | +                            {"label": label},
 259 | +                            {}, True)
 260 | +
 261 | +
 262 | +        self.log.info("Generate 101 blocks to node with deprecated filter_coinbase")
 263 | +        address = self.nodes[1].getnewaddress(label)

jnewbery commented at 10:48 AM on April 14, 2021:

maybe use a new variable name address2 here?

in doc/release-notes-14707.md:4 in 77da82934c outdated

   0 | @@ -0,0 +1,17 @@
   1 | +Wallet `receivedby` RPCs now include coinbase transactions
   2 | +-------------
   3 | +
   4 | +Previously, the following wallet RPCs incorrectly filtered out coinbase transactions:

jnewbery commented at 10:49 AM on April 14, 2021:

Previously, the following wallet RPCs excluded coinbase transactions:

LarryRuane commented at 3:40 PM on April 14, 2021:

I agree that "filter" isn't the best term, because it's slightly ambiguous. Maybe it's just me, but while it probably means "exclude", it could possibly mean "include".

in doc/release-notes-14707.md:14 in 77da82934c outdated

   9 | +
  10 | +`listreceivedbyaddress`
  11 | +
  12 | +`listreceivedbylabel`
  13 | +
  14 | +This release corrects this behaviour and returns results accounting for received coins from coinbase outputs.

jnewbery commented at 10:49 AM on April 14, 2021:

This release changes this behaviour and returns results accounting for received coins from coinbase outputs.

in doc/release-notes-14707.md:17 in 77da82934c outdated

  12 | +`listreceivedbylabel`
  13 | +
  14 | +This release corrects this behaviour and returns results accounting for received coins from coinbase outputs.
  15 | +
  16 | +A new option, `include_immature` (default=`false`), determines whether to account for immature coinbase transactions.
  17 | +Immature coinbase transactions are coinbase transactions that have 100 or fewer confirmations, and are not spendable.

jnewbery commented at 10:50 AM on April 14, 2021:

I think this should also document the deprecatedrpc=filter_coinbase (or deprecatedrpc=exclude_coinbase if you rename it) config option, including that the intention is to remove it after some time.

in src/wallet/rpcwallet.cpp:712 in 77da82934c outdated

 708 | @@ -698,6 +709,7 @@ static RPCHelpMan getreceivedbyaddress()
 709 |                  {
 710 |                      {"address", RPCArg::Type::STR, RPCArg::Optional::NO, "The bitcoin address for transactions."},
 711 |                      {"minconf", RPCArg::Type::NUM, /* default */ "1", "Only include transactions confirmed at least this many times."},
 712 | +                    {"include_immature", RPCArg::Type::BOOL, /* default */ "false", "Include immature coinbase transactions."},

jnewbery commented at 10:51 AM on April 14, 2021:

Should this and the other updated RPCs throw if a user supplies include_immature=true and deprecatedrpc=filter_coinbase is set. That would be inconsistent.

in test/functional/wallet_listreceivedby.py:277 in 77da82934c outdated

 278 | +
 279 | +        self.log.info("listreceivedbylabel does not include label when filtering coinbase")
 280 | +        assert_array_result(self.nodes[1].listreceivedbylabel(),
 281 | +                            {"label": label},
 282 | +                            {}, True)
 283 | +

jnewbery commented at 10:52 AM on April 14, 2021:

See the above comment about include_immature and "-deprecatedrpc=filter_coinbase". Perhaps add a test for that combination?

in test/functional/wallet_listreceivedby.py:2 in 77da82934c outdated

   0 | @@ -1,5 +1,5 @@
   1 |  #!/usr/bin/env python3
   2 | -# Copyright (c) 2014-2019 The Bitcoin Core developers
   3 | +# Copyright (c) 2014-2021 The Bitcoin Core developers

jnewbery commented at 10:52 AM on April 14, 2021:

No need to update this. They get updated automatically every year.

in test/functional/wallet_listreceivedby.py:5 in 77da82934c outdated

   0 | @@ -1,5 +1,5 @@
   1 |  #!/usr/bin/env python3
   2 | -# Copyright (c) 2014-2019 The Bitcoin Core developers
   3 | +# Copyright (c) 2014-2021 The Bitcoin Core developers
   4 |  # Distributed under the MIT software license, see the accompanying
   5 |  # file COPYING or http://www.opensource.org/licenses/mit-license.php.
   6 |  """Test the listreceivedbyaddress RPC."""

glozow commented at 1:46 PM on April 14, 2021:

May want to update this comment to reflect the test expansion

in src/wallet/rpcwallet.cpp:550 in 77da82934c outdated

 685 | +        int depth{wtx.GetDepthInMainChain()};
 686 | +        if (depth < min_depth
 687 | +            // Coinbase with less than 1 confirmation is an orphan
 688 | +            || (wtx.IsCoinBase() && (depth < 1 || filter_coinbase))
 689 | +            || (!include_immature && wtx.IsImmatureCoinBase())
 690 | +            || !wallet.chain().checkFinalTx(*wtx.tx)) {

glozow commented at 2:22 PM on April 14, 2021:

I expect this refers to outputs that we can't spend yet because a timelock hasn't been met. If a user wants to include immature coinbases, might they want to include nonfinal ones too? I understand that include_immature is supposed to be only for immature coinbases but I think a user might find it more helpful to be able to configure spendability in general rather than just for coinbases

andrewtoth commented at 7:07 PM on April 19, 2021:

I thought this was an interesting idea. However, while implementing and trying to write a test, I was unable to broadcast a non-final tx (rejected with non-final). Do you know any way that a wallet would be able to have a non-final tx? If not, then it's possible this check is unnecessary and can be removed.

glozow commented at 7:54 PM on April 19, 2021:

Oh hm, I guess the finality wouldn't really be reflected in the received transaction's header, but in the redeem script for it. I'm not too sure how the wallet keeps track of timelocked outputs 🤔 I don't think this check should be removed though

in test/functional/wallet_listreceivedby.py:244 in 77da82934c outdated

 239 | +
 240 | +        self.log.info("Orphan block that paid to address")
 241 | +        self.nodes[0].invalidateblock(hash)
 242 | +
 243 | +        self.log.info("getreceivedbyaddress does not include orphan when minconf is 0 when including immature")
 244 | +        balance = self.nodes[0].getreceivedbyaddress(address=address, minconf=0, include_immature=True)

glozow commented at 2:28 PM on April 14, 2021:

Question: What happens if a user puts -spendzeroconfchange=0 (it's True by default, right?) and then calls getreceivedbyaddress(minconf=0)?

andrewtoth commented at 7:14 PM on April 19, 2021:

Hmm I'm not sure there is any effect in this case. The getreceivedby* RPCs only return a balance, and setting minconf=0 would return the unconfirmed balance using the tally code in GetReceived. I don't believe it would be affected by spendzeroconfchange, which from what I can see is only used in CWallet::SelectCoins.

glozow commented at 3:57 PM on April 14, 2021: member

Concept ACK to not filtering out coinbases (especially if they're mature, I can't imagine why someone would want to filter those out). I have a few questions.

DrahtBot added the label Needs rebase on Apr 19, 2021

andrewtoth force-pushed on Apr 19, 2021

andrewtoth commented at 7:15 PM on April 19, 2021: contributor

@jnewbery @glozow @LarryRuane thank you all for your review! I've updated with all of your suggestions.

in src/rpc/client.cpp:59 in 80ad7d7e05 outdated

  54 |      { "listreceivedbyaddress", 2, "include_watchonly" },
  55 | +    { "listreceivedbyaddress", 4, "include_not_yet_spendable" },
  56 |      { "listreceivedbylabel", 0, "minconf" },
  57 |      { "listreceivedbylabel", 1, "include_empty" },
  58 |      { "listreceivedbylabel", 2, "include_watchonly" },
  59 | +    { "listreceivedbylabel", 3, "include_not_yet_spendable" },

maflcko commented at 7:39 PM on April 19, 2021:

AssertionError: RPC client conversion table and RPC server named arguments mismatch! {('getreceivedbyaddress', 2, 'include_not_yet_spendable'), ('listreceivedbylabel', 3, 'include_immature_coinbase'), ('listreceivedbylabel', 3, 'include_not_yet_spendable'), ('listreceivedbyaddress', 4, 'include_not_yet_spendable'), ('listreceivedbyaddress', 4, 'include_immature_coinbase'), ('getreceivedbylabel', 2, 'include_immature_coinbase'), ('getreceivedbyaddress', 2, 'include_immature_coinbase'), ('getreceivedbylabel', 2, 'include_not_yet_spendable')}

andrewtoth commented at 7:44 PM on April 19, 2021:

Fixed. Thanks!

andrewtoth force-pushed on Apr 19, 2021

DrahtBot removed the label Needs rebase on Apr 19, 2021

in doc/release-notes-14707.md:16 in 2b310631a7 outdated

  11 | +
  12 | +`listreceivedbylabel`
  13 | +
  14 | +This release changes this behaviour and returns results accounting for received coins from coinbase outputs.
  15 | +
  16 | +A new option, `include_immature_coinbase` (default=`false`), determines whether to account for immature coinbase transactions or non-final transactions.

jnewbery commented at 9:13 AM on June 14, 2021:

As far as I can see, include_immature_coinbase has no impact on whether non-final transactions are included.

A new option, `include_immature_coinbase` (default=`false`), determines whether to account for immature coinbase transactions.

in test/functional/wallet_listreceivedby.py:281 in 2b310631a7 outdated

 281 | +                            {}, True)
 282 | +
 283 | +        self.log.info("getreceivedbyaddress throws when setting include_immature_coinbase with deprecated exclude_coinbase")
 284 | +        assert_raises_rpc_error(-8, 'include_immature_coinbase is incompatible with deprecated exclude_coinbase', self.nodes[1].getreceivedbyaddress, address2, 1, True)
 285 | +
 286 | +

jnewbery commented at 9:13 AM on June 14, 2021:

nit: remove extra linebreak if you retouch this branch.

jnewbery commented at 12:39 PM on November 4, 2021:

Feel free to remove this extra newline if you need to touch this branch again.

jnewbery commented at 9:13 AM on June 14, 2021: contributor

Code review ACK 5e13dac43f257cc0869150c8f8b400b353885997 modulo release note comment.

andrewtoth force-pushed on Jun 16, 2021

andrewtoth commented at 8:26 PM on June 16, 2021: contributor

@jnewbery not sure what commit your ACK is referencing. Looks like github does not recognize it either.

jnewbery commented at 10:12 AM on June 17, 2021: contributor

@jnewbery not sure what commit your ACK is referencing. Looks like github does not recognize it either.

Oops, sorry @andrewtoth - I rebased your branch on master before building/reviewing (building a branch that's based on a very old master commit results in a very slow build since lots of header files have changed). I then accidentally left my ACK for my new head commit hash instead of the one in this PR.

ACK 164e26d42e

DrahtBot added the label Needs rebase on Sep 3, 2021

andrewtoth force-pushed on Sep 14, 2021

DrahtBot removed the label Needs rebase on Sep 14, 2021

jnewbery commented at 12:39 PM on November 4, 2021: contributor

reACK 1f084ec0ef @andrewtoth: github doesn't generate a notification for reviewers if you just force-push a rebase. If you want people to re-review after a rebase you should leave a comment. That'll notify everyone who has previously reviewed the PR.

in src/wallet/rpcwallet.cpp:688 in 513759861c outdated

 684 | @@ -685,11 +685,26 @@ static CAmount GetReceived(const CWallet& wallet, const UniValue& params, bool b
 685 |      if (!params[1].isNull())
 686 |          min_depth = params[1].get_int();
 687 |  
 688 | +    const bool include_immature_coinbase{!params[2].isNull() && params[2].get_bool()};

LarryRuane commented at 12:37 AM on December 6, 2021:

nit: I had to think about this logic before concluding that the default is false (the ! and isNull forming a sort of double-negative didn't help), so consider prefacing this with a comment stating that the default is false.

maflcko commented at 8:51 AM on December 6, 2021:

an alternative would be to write {null? default : get_bool()}

andrewtoth commented at 5:19 PM on December 6, 2021:

Hi @LarryRuane - this logic was yours #14707 (review) :). I will revert to what @MarcoFalke suggested.

LarryRuane commented at 4:30 AM on December 6, 2021: contributor

This PR has hidden conflicts in the functional test; the generate() and generatetoaddress() signatures have changed.

maflcko commented at 9:20 AM on December 6, 2021: member

Yeah, needs rebase again.

                                   TypeError: generatetoaddress() missing 1 required keyword-only argument: 'invalid_call'

Include coinbase transactions in receivedby wallet rpcs bce20c34d6

andrewtoth force-pushed on Dec 6, 2021

andrewtoth commented at 5:33 PM on December 6, 2021: contributor

Rebased.

maflcko commented at 8:00 AM on December 7, 2021: member

Looks like there are test failures with descriptor wallets?

Test including coinbase transactions in receivedby wallet rpcs b5696750a9

Coinbase receivedby rpcs release notes 1dcba996d3

andrewtoth commented at 3:49 PM on December 7, 2021: contributor

@MarcoFalke fixed.

andrewtoth force-pushed on Dec 7, 2021

jnewbery commented at 5:04 PM on December 7, 2021: contributor

reACK 1dcba996d30d83aebe8c73f42f5d4056d6472166

in doc/release-notes-14707.md:17 in 1dcba996d3

  12 | +`listreceivedbylabel`
  13 | +
  14 | +This release changes this behaviour and returns results accounting for received coins from coinbase outputs.
  15 | +
  16 | +A new option, `include_immature_coinbase` (default=`false`), determines whether to account for immature coinbase transactions.
  17 | +Immature coinbase transactions are coinbase transactions that have 100 or fewer confirmations, and are not spendable.

maflcko commented at 7:30 PM on December 7, 2021:

Is this still up-to-date? The default seems to be true. Also, instead of using the release notes to explain what immature txs are, what about moving this to the rpc help? Either this is relevant for future releases as well, or not at all.

andrewtoth commented at 7:41 PM on December 7, 2021:

The option is defined here and here, made clear to be false when no option is specified by your suggestion here.

I can move the immature txs explanation to the rpc help if I have to rebase again. Do you think it's a blocker? The release notes will have to be manually updated anyways, right?

maflcko commented at 7:46 PM on December 7, 2021:

No, not a blocker.

Sorry for the brain fart. I keep messing up true and false for boolean values lately.

maflcko commented at 7:49 PM on December 7, 2021:

Maybe for clarity (in the future) the second section can be switched with the third section, so that the behavior changing stuff is bundled together and the new rpc arg option is in a separate bundle?

maflcko commented at 7:53 PM on December 7, 2021:

If someone decides to address the nits, the notes snippet can also be moved to the main release notes file in the same pull request, to avoid having to touch it again.

maflcko commented at 7:31 PM on December 7, 2021: member

Sorry for the incremental review

maflcko merged this on Dec 7, 2021

maflcko closed this on Dec 7, 2021

sidhujag referenced this in commit 224b8599fa on Dec 7, 2021

RandyMcMillan referenced this in commit 778314924e on Dec 23, 2021

maflcko referenced this in commit a7e3afb221 on May 20, 2022

sidhujag referenced this in commit 1b86b89488 on May 28, 2022

andrewtoth deleted the branch on Nov 16, 2022

jamesdorfman referenced this in commit 05a62b2d91 on Jun 3, 2023

delta1 referenced this in commit 91f2b1114e on Jun 5, 2023

knst referenced this in commit 16319f00f4 on Aug 17, 2023

knst referenced this in commit fcb4620d97 on Aug 18, 2023

knst referenced this in commit 38a100dd82 on Aug 18, 2023

knst referenced this in commit 5bd115dc38 on Aug 18, 2023

knst referenced this in commit 40ad50f460 on Aug 20, 2023

knst referenced this in commit 7246cf2856 on Aug 24, 2023

PastaPastaPasta referenced this in commit b12b323e60 on Aug 30, 2023

bitcoin locked this on Nov 16, 2023