rpc: Speedup getaddressesbylabel

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.

promag force-pushed on Feb 22, 2019

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?

laanwj added the label RPC/REST/ZMQ on Feb 22, 2019

promag force-pushed on Feb 23, 2019

instagibbs commented at 7:28 pm on February 28, 2019: member

tag 0.18?

MarcoFalke added this to the milestone 0.18.0 on Feb 28, 2019

MarcoFalke removed this from the milestone 0.18.0 on Feb 28, 2019

promag referenced this in commit af2609011c on Mar 3, 2019

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 0:08 am on April 22, 2019:

Comment added, thanks @ryanofsky.

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

ryanofsky approved

ryanofsky commented at 2:34 pm on March 4, 2019: member

utACK 643eba0aa262ead636d5de18ced31b6f166ec033

fanquake requested review from meshcollider on Mar 13, 2019

meshcollider commented at 9:11 am on March 18, 2019: contributor

utACK https://github.com/bitcoin/bitcoin/pull/15463/commits/643eba0aa262ead636d5de18ced31b6f166ec033

DrahtBot commented at 7:22 pm on March 21, 2019: member

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

Conflicts

No conflicts as of last run.

promag force-pushed on Apr 22, 2019

rpc: Speedup getaddressesbylabel 710a7136f9

promag force-pushed on Apr 22, 2019

ryanofsky approved

ryanofsky commented at 2:54 pm on April 23, 2019: member

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

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

utACK 710a7136f93133bf256d37dc8c8faf5a6b9ba89d

MarcoFalke merged this on Apr 23, 2019

MarcoFalke closed this on Apr 23, 2019

MarcoFalke referenced this in commit cd14d210c4 on Apr 23, 2019

deadalnix referenced this in commit 54c6eab4e0 on May 8, 2020

ftrader referenced this in commit 26b780aa22 on Aug 17, 2020

pravblockc referenced this in commit ef518a1303 on Sep 25, 2021

pravblockc referenced this in commit 9577ca7b25 on Sep 29, 2021

PastaPastaPasta referenced this in commit 2f845d8074 on Sep 30, 2021

kittywhiskers referenced this in commit 3ee77fcf6e on Oct 12, 2021

DrahtBot locked this on Dec 16, 2021

rpc: Speedup getaddressesbylabel #15463

Conflicts