Reduce cs_main lock in listunspent #12639

1. 
   promag commented at 9:01 pm on March 7, 2018: member

   On my system, where the wallet has 10000 unspents, the cs_main lock duration changed from 191ms to 36ms. The loop that generates the response takes around 155ms. So, the lock duration is reduced to around 20%.
2. fanquake added the label RPC/REST/ZMQ on Mar 7, 2018
3. 
   MarcoFalke commented at 3:31 pm on March 8, 2018: member

   Needs rebase to fix travis (sorry)
4. promag force-pushed on Mar 11, 2018
5. 
   promag commented at 10:26 pm on March 11, 2018: member

   Rebased.
6. 
   conscott commented at 4:40 am on March 17, 2018: contributor

   Nice.

   Test ACK efbc070c5cb176b0f79801e9e6d5f5a52578edc1

7. 
   jonasschnelli commented at 3:05 am on March 19, 2018: contributor

   General Concept ACK. There is tiny risk that bitcoind spits out utxos that are already spent due the unlocking between AvailableCoins and the return output generation…. okay IMO.
8. 
   promag commented at 10:48 am on March 19, 2018: member

   @jonasschnelli yeah, the same can happen between the user receives the response and tries to use an unspent that was spent in between.
9. 
   sipa commented at 9:40 pm on March 22, 2018: member

   Concept ACK, needs rebase.
10. promag force-pushed on Mar 22, 2018
11. 
    promag commented at 11:35 pm on March 22, 2018: member

    Rebased and replaced 3 mapAddressBook lookups with 1 lookup.
12. in src/wallet/rpcwallet.cpp:3155 in 4c33ef6ffe outdated

    3012@@ -3013,9 +3013,11 @@ UniValue listunspent(const JSONRPCRequest& request)
    3013 
    3014     UniValue results(UniValue::VARR);
    3015     std::vector<COutput> vecOutputs;
    3016-    LOCK2(cs_main, pwallet->cs_wallet);
    3017+    {
    3018+        LOCK2(cs_main, pwallet->cs_wallet);
    3019+        pwallet->AvailableCoins(vecOutputs, !include_unsafe, nullptr, nMinimumAmount, nMaximumAmount, nMinimumSumAmount, nMaximumCount, nMinDepth, nMaxDepth);
    3020+    }
    

    ---

    ---

    

    promag commented at 11:37 pm on March 22, 2018:

    I thought taking a copy of the address book in the above block to avoid locking the wallet bellow. Thoughts?

    ---

    

    laanwj commented at 2:30 pm on April 17, 2018:

    As it contains an entry for every key, the address book can be quite a big data structure. I don’t think making a copy is a good trade-off.

    ---

    

    promag commented at 2:34 pm on April 17, 2018:

    Agree.
13. in src/wallet/rpcwallet.cpp:3037 in 4c33ef6ffe outdated

    3032@@ -3031,9 +3033,13 @@ UniValue listunspent(const JSONRPCRequest& request)
    3033         if (fValidAddress) {
    3034             entry.pushKV("address", EncodeDestination(address));
    3035 
    3036-            if (pwallet->mapAddressBook.count(address)) {
    3037-                entry.pushKV("label", pwallet->mapAddressBook[address].name);
    3038-                entry.pushKV("account", pwallet->mapAddressBook[address].name);
    3039+            {
    3040+                LOCK(pwallet->cs_wallet);
    

    ---

    ---

    

    promag commented at 2:35 pm on April 17, 2018:

    @laanwj how about locking cs_wallet (not cs_main) for the whole loop?

    ---

    

    laanwj commented at 2:39 pm on April 17, 2018:

    Depends on the relative overhead of locking/unlocking for every look-up here. If that is negligible, it’s preferable to lock/unlock with the small granularity like you have now.

    ---

    

    promag commented at 2:51 pm on April 17, 2018:

    This PR aims to reduce cs_main and that is already accomplished.

    Keeping the wallet locked for the whole loop is no worse than the previous implementation. Overhead aside, this implementation is more susceptible to an unspent being spent, so I would say to lock the whole loop to minimise that.

    ---

    

    promag commented at 11:52 am on May 3, 2018:

    Keeping the wallet locked for the whole loop @laanwj changed to this.

14. promag force-pushed on May 3, 2018
15. rpc: Reduce cs_main lock in listunspent d76962e056
16. refactor: Avoid extra lookups of mapAddressBook in listunspent RPC a59dac35ab
17. promag force-pushed on May 3, 2018
18. 
    promag commented at 11:53 am on May 3, 2018: member

    Rebased and split in 2 commits: the change in locks and the lookup refactor.
19. 
    laanwj commented at 1:36 pm on May 3, 2018: member

    utACK a59dac35abf64d5af4d499bc3397b3369eb76eda
20. laanwj merged this on May 3, 2018
21. laanwj closed this on May 3, 2018

    ---

22. laanwj referenced this in commit 7eb7076f70 on May 3, 2018
23. promag deleted the branch on May 3, 2018
24. jasonbcox referenced this in commit b03fb98a32 on Dec 6, 2019
25. jonspock referenced this in commit 4006008efd on Sep 29, 2020
26. jonspock referenced this in commit 6c3e00d159 on Sep 29, 2020
27. jonspock referenced this in commit 48cb79b339 on Oct 10, 2020
28. LarryRuane referenced this in commit 6554788057 on Mar 22, 2021
29. LarryRuane referenced this in commit 302231e998 on Mar 22, 2021
30. LarryRuane referenced this in commit e0115b06f8 on Apr 26, 2021
31. LarryRuane referenced this in commit f29a46de71 on Apr 26, 2021
32. UdjinM6 referenced this in commit 47cc03552f on May 21, 2021
33. UdjinM6 referenced this in commit dcd6c1df3c on May 25, 2021
34. LarryRuane referenced this in commit 15dca59349 on May 27, 2021
35. LarryRuane referenced this in commit 93a71c68a4 on May 27, 2021
36. TheArbitrator referenced this in commit 33c613de2d on Jun 7, 2021
37. TheArbitrator referenced this in commit 0d7bfd146b on Jun 7, 2021
38. MarcoFalke locked this on Sep 8, 2021

Contributors
promag MarcoFalke conscott jonasschnelli sipa laanwj

Labels
RPC/REST/ZMQ