getreceivedby[address, label] rpc performance proposal #23645

1. 
   beegater commented at 7:33 PM on December 1, 2021: none

   Hi,

   Problem

   The refactoring done in #17579 introduced some performance degradation for the rpc commands getreceivedbyaddress and getreceivedbylabel, which prevented an upgrade to versions >=0.21.0 for us without custom patches that revert this PR. The requests take notably longer to complete. This is especially painful when using big wallet files or issuing a large number of requests in a short period of time. I know that the foremost goal of Bitcoin Core may not be performance, but given that these rpc commands are some of the most prominent ones needed when interacting with wallets via rpc, I propose to do some changes here.

   Analysis

   Currently, a set of search address is generated from the scriptpubkeys and then these addresses are compared against all addresses in the wallet, which means for each transaction in the wallet, the time consuming conversion from scriptpubkey to address needs to happen (in ExtractDestination(...)).

   Solution Proposal

   Given that the majority of use cases comprises searching for a small set of addresses in a large set of addresses (the wallet), it makes more sense to rather create a set of scriptpubkeys from the search addresses and then compare each scriptpubkey in the wallet against this set. This is similar to how it was done before #17579, except that we now use a set, which means a search for multiple addresses (corresponding to a single label) stays possible and the refactoring can be more or less kept as is.

   I may not have time soon to implement this properly, but what do you think (conceptually)?

2. beegater added the label Bug on Dec 1, 2021
3. 
   theStack commented at 12:58 PM on December 3, 2021: member

   Concept ACK

   Thanks for reporting! I think we definitely don't want users to avoid updating to new versions due to RPC calls performing significantly worse, i.e. I'd agree that this needs to be fixed (unless there is a really good reason not to). I vaguely remember having reviewed this PR at that time (more than two years ago), obviously neither me nor other reviewers were aware that this could affect the performance -- in a strict sense this PR should not have been labelled as "refactoring". Will take a deeper look within the next days.

   To reproduce the issue easier, would you mind sharing what is a "big wallet" in your use-case, in terms of order of magnitude of number of transactions/addresses in the wallet? Also do you have concrete numbers on taking "notably longer"?

4. 
   beegater commented at 11:46 PM on December 4, 2021: none

   To reproduce the issue easier, would you mind sharing what is a "big wallet" in your use-case, in terms of order of magnitude of number of transactions/addresses in the wallet? Also do you have concrete numbers on taking "notably longer"?

   We are using some test files that contain ~ 10k - 100k transactions of different types. They are part of a highly integrated testing environment with deeply predefined hardware where we issue a large number of requests at a high frequency via rpc. I can't give you exact numbers on "notably longer" since our setup operates on a pass/fail basis, but it was for sure more than factor of 5, probably even 10.

   Since we have been running through a lot of trouble due changes like this, we are generally quite slow with following updates of Bitcoin Core, unfortunately. The problem is that once these requests clog up, Bitcoin Core becomes unresponsive altogether, which would be catastrophic in our prod env. Of course, we also have other measures to avoid this, but we want to ensure this not to be a bottleneck in any case.

   I am not an expert on how feasible such a testing setup is for a project like Bitcoin Core, but it would be helpful if the most common rpc requests had automatic test cases in terms of performance. This could help spot and discuss potential severe degradations already at the PR-merge stage and would save some downstream effort. Not talking about corner cases here, but things like getting the amount of received coins for an address should be as responsive as possible.

5. 
   MarcoFalke commented at 9:13 AM on December 6, 2021: member

   There are benchmarks (https://github.com/bitcoin/bitcoin/blob/master/src/bench/wallet_balance.cpp), but they currently don't fail CI when the performance drops.

   Improvements are welcome.

6. 
   antonilol commented at 3:59 PM on December 15, 2021: none

   listunspent took a very long when i had 2000 UTXOs (on testnet, i was filling blocks there :)) is it possible to speed up listunspent as well?

7. 
   iprok commented at 10:25 AM on February 22, 2022: none

   I confirm the issue. I'm trying time bitcoin-cli getreceivedbyaddress SOMEADDRESS 7 and results are 0m0.016s - bitcoin core 0.20.1 0m0.786s - bitcoin core 0.21.2 0m1.934s - bitcoin core 22.0 - absolutely impossible timing for us

   We have 1758 transactions and 1715 addresses.

   I'm using binary version x86_64-linux from https://bitcoincore.org/bin/bitcoin-core-22.0/ on host with Debian Stretch

8. 
   iprok commented at 7:34 AM on April 27, 2022: none

   @theStack hello, excuse me for bothering you, but are any news or plans here?

9. 
   theStack commented at 10:31 AM on April 27, 2022: member

   @theStack hello, excuse me for bothering you, but are any news or plans here?

   There is a PR tackling this problem (https://github.com/bitcoin/bitcoin/pull/23662) that should be ready for merge soon, i.e. the next release will very likely include the fix.

10. MarcoFalke added this to the milestone 24.0 on Apr 27, 2022
11. achow101 closed this on May 16, 2022

    ---

12. sidhujag referenced this in commit c217be178c on May 28, 2022
13. DrahtBot locked this on May 16, 2023

Milestone
24.0