[refactor] Merge getreceivedby tally into GetReceived function #17579

pull andrewtoth wants to merge 1 commits into bitcoin:master from andrewtoth:getreceivedby-refactor changing 1 files +48 −58

andrewtoth commented at 6:43 PM on November 24, 2019: contributor

This PR merges the tally code of getreceivedbyaddress and getreceivedbylabel into a single function GetReceived. This reduces repeated code and makes it similar to listreceivedbyaddress and listreceivedbylabel, which use the function ListReceived. It will also make the change in #14707 simpler and easier to review.
fanquake added the label RPC/REST/ZMQ on Nov 24, 2019
fanquake added the label Wallet on Nov 24, 2019
in src/wallet/rpcwallet.cpp:586 in 2377697e36 outdated
```
 582 | @@ -583,6 +583,57 @@ static UniValue signmessage(const JSONRPCRequest& request)
 583 |      return EncodeBase64(vchSig.data(), vchSig.size());
 584 |  }
 585 |  
 586 | +static UniValue GetReceived(interfaces::Chain::Lock& locked_chain, CWallet * const wallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet)
```
MarcoFalke commented at 10:38 PM on November 24, 2019:

style-nit: Instead of returning an UniValue, I'd prefer to return a CAmount, so that (if needed) the function can also be called from unit tests easily.

andrewtoth commented at 11:12 PM on November 25, 2019:

Done

andrewtoth commented at 12:21 AM on November 27, 2019:

@MarcoFalke can you expand on how returning CAmount makes it easier to test? Is it due to not having UniValue as a dependency? In that case I should remove UniValue as a parameter as well.
DrahtBot commented at 11:53 PM on November 24, 2019: 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:
- #18647 (rpc: remove g_rpc_node & g_rpc_chain by brakmic)
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 25, 2019

in src/wallet/rpcwallet.cpp:616 in 915a40aaec outdated

 623 | +        }
 624 | +
 625 | +        for (const CTxOut& txout : wtx.tx->vout) {
 626 | +            CTxDestination address;
 627 | +            if (ExtractDestination(txout.scriptPubKey, address) && wallet->IsMine(address) && addresses.count(address)) {
 628 | +                amount += txout.nValue;

andrewtoth commented at 8:56 PM on December 2, 2019:

This uses the multiple address case, used by getreceivedbylabel, and the singular case used by getreceivedbyaddress is no longer needed.

in src/wallet/rpcwallet.cpp:586 in 915a40aaec outdated

 582 | @@ -583,6 +583,57 @@ static UniValue signmessage(const JSONRPCRequest& request)
 583 |      return EncodeBase64(vchSig.data(), vchSig.size());
 584 |  }
 585 |  
 586 | +static CAmount GetReceived(interfaces::Chain::Lock& locked_chain, CWallet * const wallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(wallet->cs_wallet)

theStack commented at 11:08 PM on January 23, 2020:

Variable naming: s/wallet/pwallet (see e.g. ListReceived() and SendMoney() in the same file) to be more consistent

in src/wallet/rpcwallet.cpp:633 in 915a40aaec outdated

 628 | +                amount += txout.nValue;
 629 | +            }
 630 | +        }
 631 | +    }
 632 | +
 633 | +    return  amount;

theStack commented at 11:10 PM on January 23, 2020:

nit: Remove extra space after return

in src/wallet/rpcwallet.cpp:611 in 915a40aaec outdated

 618 | +        }
 619 | +
 620 | +        int depth = wtx.GetDepthInMainChain();
 621 | +        if (depth < min_depth) {
 622 | +            continue;
 623 | +        }

theStack commented at 11:15 PM on January 23, 2020:

More a matter of taste, but this could simply be put in the if condition above and without an extra variable (that is used only once anyway)? Like

if (wtx.IsCoinBase() || !locked_chain.checkFinalTx(*wtx.tx) || wtx.GetDepthInMainChain() < min_depth) {
    continue;
}

in src/wallet/rpcwallet.cpp:674 in 915a40aaec outdated

 699 | -                if (wtx.GetDepthInMainChain() >= nMinDepth)
 700 | -                    nAmount += txout.nValue;
 701 | -    }
 702 | -
 703 | -    return  ValueFromAmount(nAmount);
 704 | +    return ValueFromAmount(GetReceived(*locked_chain, pwallet, request.params, false));

theStack commented at 11:21 PM on January 23, 2020:

nit: put /* by_label */ before boolean parameter false to increase readability.

in src/wallet/rpcwallet.cpp:715 in 915a40aaec outdated

 738 | -            }
 739 | -        }
 740 | -    }
 741 | -
 742 | -    return ValueFromAmount(nAmount);
 743 | +    return ValueFromAmount(GetReceived(*locked_chain, pwallet, request.params, true));

theStack commented at 11:21 PM on January 23, 2020:

nit: put /* by_label */ before boolean parameter true to increase readability.

theStack commented at 11:23 PM on January 23, 2020: contributor

Concept ACK, left a view code review comments (mostly nits though)
andrewtoth force-pushed on Jan 25, 2020
andrewtoth commented at 1:27 AM on January 25, 2020: contributor

@theStack thank you for the review. I've addressed all comments.
andrewtoth force-pushed on Jan 25, 2020
andrewtoth force-pushed on Jan 26, 2020
theStack approved
theStack commented at 7:16 PM on January 26, 2020: contributor

ACK https://github.com/bitcoin/bitcoin/pull/17579/commits/4111967281a37cfe0d34802a80dba0715bfe3ffb
DrahtBot cross-referenced this on Feb 11, 2020 from issue [RPC] Include coinbase transactions in receivedby RPCs by andrewtoth
DrahtBot cross-referenced this on Feb 17, 2020 from issue Drop checkFinalTx and use Median Time Past to check finality of wallet transactions by ariard
DrahtBot cross-referenced this on Mar 25, 2020 from issue Use shared pointers only in validation interface by bvbfan
sipa commented at 3:27 AM on March 27, 2020: member

utACK 4111967281a37cfe0d34802a80dba0715bfe3ffb

Nice cleanup, and also nice to hoist the DepthInMainChain check out of the inner loop.

Optionally, since this is more than a pure code move, some of the variable names/comments could be improved as well (the "addresses" variable name, as well as the comment referring to pubkeys are outdated).
Merge getreceivedby tally into GetReceived function a1d5b12ec0

in src/wallet/rpcwallet.cpp:586 in 4111967281 outdated

 582 | @@ -583,6 +583,52 @@ static UniValue signmessage(const JSONRPCRequest& request)
 583 |      return EncodeBase64(vchSig.data(), vchSig.size());
 584 |  }
 585 |  
 586 | +static CAmount GetReceived(interfaces::Chain::Lock& locked_chain, CWallet * const pwallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(pwallet->cs_wallet)

MarcoFalke commented at 4:24 PM on March 27, 2020:

static CAmount GetReceived(interfaces::Chain::Lock& locked_chain, const CWallet& wallet, const UniValue& params, bool by_label) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)

andrewtoth force-pushed on Mar 27, 2020
theStack approved
theStack commented at 9:55 AM on March 29, 2020: contributor

re-ACK https://github.com/bitcoin/bitcoin/commit/a1d5b12ec07d2f7aa9fa955a6dd99e8a2be5cb25
DrahtBot cross-referenced this on Apr 5, 2020 from issue rpc: remove deprecated CRPCCommand constructor by MarcoFalke
DrahtBot cross-referenced this on Apr 15, 2020 from issue rpc: remove g_rpc_node & g_rpc_chain by brakmic
meshcollider commented at 2:43 AM on April 16, 2020: contributor

utACK a1d5b12ec07d2f7aa9fa955a6dd99e8a2be5cb25
DrahtBot cross-referenced this on Apr 17, 2020 from issue Reverse cs_main, cs_wallet lock order and reduce cs_main locking by ariard
MarcoFalke merged this on Apr 20, 2020
MarcoFalke closed this on Apr 20, 2020

in src/wallet/rpcwallet.cpp:603 in a1d5b12ec0

 598 | +    }
 599 | +
 600 | +    // Minimum confirmations
 601 | +    int min_depth = 1;
 602 | +    if (!params[1].isNull())
 603 | +        min_depth = params[1].get_int();

MarcoFalke commented at 2:15 PM on April 20, 2020:

    const int min_depth{params[1].isNull() ? 1 : params[1].get_int()};

can be written shorter

sidhujag referenced this in commit 3782e79671 on Apr 20, 2020
jasonbcox referenced this in commit fa57b76ebd on Oct 15, 2020
beegater cross-referenced this on Dec 1, 2021 from issue getreceivedby[address, label] rpc performance proposal by beegater
theStack cross-referenced this on Dec 3, 2021 from issue rpc: improve `getreceivedby{address,label}` performance by theStack
bitcoin locked this on Feb 15, 2022
andrewtoth deleted the branch on Aug 17, 2023

Contributors

Labels

Linked (view graph)

#14707 [RPC] Include coinbase transactions in receivedby RPCs #23645 getreceivedby[address, label] rpc performance proposal #23662 rpc: improve `getreceivedby{address,label}` performance

[refactor] Merge getreceivedby tally into GetReceived function #17579

Conflicts