rpc: Speedup getaddressesbylabel #15463

pull promag wants to merge 1 commits into bitcoin:master from promag:2019-02-fix-15447 changing 1 files +12 −1
  1. promag commented at 4:11 PM on February 22, 2019: member

    Fixes #15447. Same approach of #14984, this change avoids duplicate key check when building the JSON response in memory.

  2. promag force-pushed on Feb 22, 2019
  3. in src/wallet/rpcwallet.cpp:3796 in a84d80dc6d outdated 
    3791 | @@ -3792,7 +3792,11 @@ static UniValue getaddressesbylabel(const JSONRPCRequest& request)
3792 |      UniValue ret(UniValue::VOBJ);
3793 |      for (const std::pair<const CTxDestination, CAddressBookData>& item : pwallet->mapAddressBook) {
3794 |          if (item.second.name == label) {
3795 | -            ret.pushKV(EncodeDestination(item.first), AddressBookDataToJSON(item.second, false));
3796 | +            UniValue address(UniValue::VOBJ);
3797 | +            address.pushKV(EncodeDestination(item.first), AddressBookDataToJSON(item.second, false));
    MarcoFalke commented at 4:44 PM on February 22, 2019:

    While item.first might be unique, EncodeDestination() of it might not be unique. You could work around that by creating a temporary map<string,type(item.second)>, fill it and then convert to univalue. It would still be cheaper, since insertion into a map is faster O(log(N)) than insertion into an univalue O(N)

    promag commented at 4:47 PM on February 22, 2019:

    Got it, and also would allow to reduce wallet lock scope.

    However what happens with duplicate results of EncodeDestination() ?

    MarcoFalke commented at 7:40 PM on February 22, 2019:

    in univalue they'd be replaced, in std::map::insert, they'd be skipped

    promag commented at 1:44 AM on February 25, 2019:

    Done.

    I could reverse the iteration to keep the behaviour, WDYT?

  4. laanwj added the label RPC/REST/ZMQ on Feb 22, 2019
  5. promag force-pushed on Feb 23, 2019
  6. instagibbs commented at 7:28 PM on February 28, 2019: member

    tag 0.18?

  7. MarcoFalke added this to the milestone 0.18.0 on Feb 28, 2019
  8. MarcoFalke removed this from the milestone 0.18.0 on Feb 28, 2019
  9. promag referenced this in commit af2609011c on Mar 3, 2019
  10. promag cross-referenced this on Mar 3, 2019 from issue 0.18: rpc: Speedup getaddressesbylabel by promag
  11. in src/wallet/rpcwallet.cpp:3797 in 643eba0aa2 outdated 
    3789 | @@ -3790,9 +3790,17 @@ static UniValue getaddressesbylabel(const JSONRPCRequest& request)
3790 |  
3791 |      // Find all addresses that have the given label
3792 |      UniValue ret(UniValue::VOBJ);
3793 | +    std::set<std::string> addresses;
3794 |      for (const std::pair<const CTxDestination, CAddressBookData>& item : pwallet->mapAddressBook) {
3795 |          if (item.second.name == label) {
3796 | -            ret.pushKV(EncodeDestination(item.first), AddressBookDataToJSON(item.second, false));
3797 | +            std::string address = EncodeDestination(item.first);
3798 | +            if (addresses.emplace(address).second) {
    ryanofsky commented at 2:29 PM on March 4, 2019:

    It would be good to have a comment like "Should always be true" if this is just a defensive check for a theoretical thing that should never happen. Or if there's an actual case where this could be false it would be good to note an example of what could cause this. Also if this can be false, it would be good to note the change in behavior in the commit message, since this code now keeps the first dup entry instead of the last dup entry.

    promag commented at 1:33 AM on March 13, 2019:

    Or if there's an actual case where this could be false it would be good to note an example of what could cause this.

    Not really sure, maybe @MeshCollider can help?

    since this code now keeps the first dup entry instead of the last dup entry.

    I've mentioned above #15463 (review) that the behavior could be preserved.

    ryanofsky commented at 3:11 PM on March 13, 2019:

    No need to go down a rabbit hole, but the code here is surprising, and adding a simple comment saying what the intention is (whatever it is) would make be an improvement, in my opinion. Would suggest: "mapAddressBook is not expected to contain duplicate address strings, but build a separate set as a precaution just in case it does." or "It is unclear whether mapAddressBook may contain duplicate address strings, so build a separate set as a precaution just in case it does."

    promag commented at 4:42 PM on March 13, 2019:

    I can add the comment, but I'd love to know if duplicate it could hit duplicate keys.

    meshcollider commented at 9:10 AM on March 18, 2019:

    I don't think its possible to have duplicates, I think it should always be true in theory

    promag commented at 12:08 AM on April 22, 2019:

    Comment added, thanks @ryanofsky.

    Also changed to assert(), after all duplicate addresses are unexpected.

  12. ryanofsky approved
  13. ryanofsky commented at 2:34 PM on March 4, 2019: contributor

    utACK 643eba0aa262ead636d5de18ced31b6f166ec033

  14. fanquake requested review from meshcollider on Mar 13, 2019
  16. DrahtBot commented at 7:22 PM on March 21, 2019: contributor

    <!--e57a25ab6845829454e8d69fc972939a-->

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

    <!--174a7506f384e20aa4161008e828411d-->

    Conflicts

    No conflicts as of last run.

  17. promag force-pushed on Apr 22, 2019
  18. promag force-pushed on Apr 22, 2019
  19. rpc: Speedup getaddressesbylabel 710a7136f9
  20. promag force-pushed on Apr 22, 2019
  21. ryanofsky approved
  22. ryanofsky commented at 2:54 PM on April 23, 2019: contributor

    utACK 710a7136f93133bf256d37dc8c8faf5a6b9ba89d. Just new comments and assert since last review.

  23. MarcoFalke commented at 2:58 PM on April 23, 2019: member

    utACK 710a7136f93133bf256d37dc8c8faf5a6b9ba89d

  24. MarcoFalke merged this on Apr 23, 2019
  25. MarcoFalke closed this on Apr 23, 2019
  26. MarcoFalke referenced this in commit cd14d210c4 on Apr 23, 2019
  27. promag cross-referenced this on May 14, 2019 from issue rpc: Speedup getrawmempool when verbose=true by promag
  28. deadalnix referenced this in commit 54c6eab4e0 on May 8, 2020
  29. ftrader referenced this in commit 26b780aa22 on Aug 17, 2020
  30. pravblockc referenced this in commit ef518a1303 on Sep 25, 2021
  31. pravblockc cross-referenced this on Sep 27, 2021 from issue Backports #15853 #15463 and #16073 by pravblockc
  32. pravblockc referenced this in commit 9577ca7b25 on Sep 29, 2021
  33. PastaPastaPasta referenced this in commit 2f845d8074 on Sep 30, 2021
  34. kwvg referenced this in commit 3ee77fcf6e on Oct 12, 2021
  35. bitcoin locked this on Dec 16, 2021
Contributors
promagMarcoFalkeinstagibbsryanofskymeshcolliderDrahtBot
Labels
RPC/REST/ZMQ
Linked (view graph)
#14984 rpc: Speedup getrawmempool when verbose=true#15447 getaddressesbylabel API spends much time#15517 0.18: rpc: Speedup getaddressesbylabel
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