Skip to content

rpc: Speedup getaddressesbylabel #15463

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
maflcko merged 1 commit into from
Apr 23, 2019
Merged

rpc: Speedup getaddressesbylabel #15463

maflcko merged 1 commit into from
Apr 23, 2019

Conversation

@promag
Copy link
Contributor

@promag promag commented Feb 22, 2019 •
edited
Loading

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

maflcko
maflcko reviewed Feb 22, 2019
View reviewed changes
src/wallet/rpcwallet.cpp Outdated
Copy link
Member

@maflcko maflcko Feb 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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)

Copy link
Contributor Author

@promag promag Feb 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

However what happens with duplicate results of EncodeDestination() ?

Copy link
Member

@maflcko maflcko Feb 22, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@promag promag Feb 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done.

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

@instagibbs
Copy link
Member

instagibbs commented Feb 28, 2019

tag 0.18?

@maflcko maflcko modified the milestone: 0.18.0 Feb 28, 2019
promag added a commit to promag/bitcoin that referenced this pull request Mar 3, 2019
@promag
rpc: Speedup getaddressesbylabel
af26090 
Github-Pull: bitcoin#15463
Rebased-From: 643eba0
@promag promag mentioned this pull request Mar 3, 2019
Closed
ryanofsky
ryanofsky approved these changes Mar 4, 2019
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 643eba0aa262ead636d5de18ced31b6f166ec033

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@ryanofsky ryanofsky Mar 4, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@promag promag Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 (comment) that the behavior could be preserved.

Copy link
Contributor

@ryanofsky ryanofsky Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@promag promag Mar 13, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor

@meshcollider meshcollider Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@promag promag Apr 22, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comment added, thanks @ryanofsky.

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

@fanquake fanquake requested a review from meshcollider March 13, 2019 13:00
meshcollider
meshcollider reviewed Mar 18, 2019
View reviewed changes
Copy link
Contributor

@meshcollider meshcollider left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

utACK 643eba0

src/wallet/rpcwallet.cpp Outdated
Copy link
Contributor

@meshcollider meshcollider Mar 18, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Mar 21, 2019 •
edited
Loading

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

Conflicts

No conflicts as of last run.

@promag promag force-pushed the 2019-02-fix-15447 branch 2 times, most recently from 51d6fbe to 9454d06 Compare April 22, 2019 00:10
@promag promag force-pushed the 2019-02-fix-15447 branch from 9454d06 to 710a713 Compare April 22, 2019 09:00
ryanofsky
ryanofsky approved these changes Apr 23, 2019
View reviewed changes
Copy link
Contributor

@ryanofsky ryanofsky left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@maflcko
Copy link
Member

maflcko commented Apr 23, 2019

utACK 710a713

@maflcko maflcko merged commit 710a713 into bitcoin:master Apr 23, 2019
maflcko pushed a commit that referenced this pull request Apr 23, 2019
Merge #15463: rpc: Speedup getaddressesbylabel
cd14d21 
710a713 rpc: Speedup getaddressesbylabel (João Barbosa)

Pull request description:

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

ACKs for commit 710a71:
  MarcoFalke:
    utACK 710a713
  ryanofsky:
    utACK 710a713. Just new comments and assert since last review.

Tree-SHA512: 77c95df9ff3793e348619aa070e6fd36df9da1b461d708ab146652cb3699f1a472ef6eb38dafdb8374375cbc97daef07635fcb0501961f167a023309513742e2
@promag promag mentioned this pull request May 14, 2019
Merged
deadalnix pushed a commit to Bitcoin-ABC/bitcoin-abc that referenced this pull request May 8, 2020
@jasonbcox
Merge #15463: rpc: Speedup getaddressesbylabel
54c6eab 
Summary:
710a7136f9 rpc: Speedup getaddressesbylabel (João Barbosa)

Pull request description:

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

ACKs for commit 710a71:
  MarcoFalke:
    utACK 710a7136f93133bf256d37dc8c8faf5a6b9ba89d
  ryanofsky:
    utACK 710a7136f93133bf256d37dc8c8faf5a6b9ba89d. Just new comments and assert since last review.

Tree-SHA512: 77c95df9ff3793e348619aa070e6fd36df9da1b461d708ab146652cb3699f1a472ef6eb38dafdb8374375cbc97daef07635fcb0501961f167a023309513742e2

Backport of Core [[bitcoin/bitcoin#15463 | PR15463]]

Similar improvement to D6007

Test Plan:
```
ninja check check-functional
```

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6008
ftrader added a commit to bitcoin-cash-node/bitcoin-cash-node that referenced this pull request Aug 17, 2020
@ftrader @cculianu
Merge #15463: rpc: Speedup getaddressesbylabel
26b780a 
Summary:
710a7136f9 rpc: Speedup getaddressesbylabel (João Barbosa)

Pull request description:

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

ACKs for commit 710a71:
  MarcoFalke:
    utACK 710a7136f93133bf256d37dc8c8faf5a6b9ba89d
  ryanofsky:
    utACK 710a7136f93133bf256d37dc8c8faf5a6b9ba89d. Just new comments and assert since last review.

Tree-SHA512: 77c95df9ff3793e348619aa070e6fd36df9da1b461d708ab146652cb3699f1a472ef6eb38dafdb8374375cbc97daef07635fcb0501961f167a023309513742e2

Backport of Core [[bitcoin/bitcoin#15463 | PR15463]]

Similar improvement to D6007

Test Plan:
```
ninja check check-functional
```

Reviewers: #bitcoin_abc, deadalnix

Reviewed By: #bitcoin_abc, deadalnix

Differential Revision: https://reviews.bitcoinabc.org/D6008
@pravblockc pravblockc mentioned this pull request Sep 27, 2021
Merged
pravblockc pushed a commit to pravblockc/dash that referenced this pull request Sep 29, 2021
@pravblockc
Merge bitcoin#15463: rpc: Speedup getaddressesbylabel
9577ca7 
710a713 rpc: Speedup getaddressesbylabel (João Barbosa)

Pull request description:

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

ACKs for commit 710a71:
  MarcoFalke:
    utACK 710a713
  ryanofsky:
    utACK 710a713. Just new comments and assert since last review.

Tree-SHA512: 77c95df9ff3793e348619aa070e6fd36df9da1b461d708ab146652cb3699f1a472ef6eb38dafdb8374375cbc97daef07635fcb0501961f167a023309513742e2
PastaPastaPasta added a commit to dashpay/dash that referenced this pull request Sep 30, 2021
@PastaPastaPasta
Merge pull request #4469 from pravblockc/backports-v0.19-pr2
2f845d8 
Backports bitcoin#15853 bitcoin#15463 and bitcoin#16073
@bitcoin bitcoin locked as resolved and limited conversation to collaborators Dec 16, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@ryanofsky ryanofsky ryanofsky approved these changes

+2 more reviewers

@meshcollider meshcollider meshcollider left review comments

@maflcko maflcko maflcko left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

RPC/REST/ZMQ

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

getaddressesbylabel API spends much time

7 participants

@promag @instagibbs @DrahtBot @maflcko @meshcollider @ryanofsky @laanwj