Use a single wallet batch for UpgradeKeyMetadata #15433

1. 
   jonasschnelli commented at 3:52 am on February 18, 2019: contributor

   Opening wallets (the first time) after #14021 took on my end around 30 seconds due to the keymetadata migration (tested on regtest).

   Using a single wallet batch reduces the required time for the migration down to <1s on my system for a default 2k keypool wallet.

2. jonasschnelli added the label Wallet on Feb 18, 2019
3. Use a single wallet batch for UpgradeKeyMetadata 0bedcbafdf
4. jonasschnelli force-pushed on Feb 18, 2019
5. meshcollider added this to the milestone 0.18.0 on Feb 18, 2019
6. 
   meshcollider commented at 6:13 am on February 18, 2019: contributor

   Tagged for 0.18 because I’d consider this a bugfix
7. 
   Sjors commented at 8:16 am on February 18, 2019: member

   Concept ACK.

   We should also consider doing that for importmulti (at least per descriptor).

8. 
   laanwj commented at 10:43 am on February 18, 2019: member

   utACK and concept ACK 0bedcbafdfaa0e980ad91c0ba4b9039575017f0e
9. 
   meshcollider commented at 11:29 am on February 18, 2019: contributor

   utACK https://github.com/bitcoin/bitcoin/commit/0bedcbafdfaa0e980ad91c0ba4b9039575017f0e @achow101 want to take a look?
10. 
    promag commented at 4:33 pm on February 18, 2019: member

    Concept ACK.

    On line 387, before throwing, shouldn’t it commit the batch?

11. 
    MarcoFalke commented at 4:38 pm on February 18, 2019: member

    @promag Wouldn’t it doe that, as the unique ptr gets destroyed (and flush on exit was set to true by default)?
12. 
    promag commented at 4:46 pm on February 18, 2019: member

    @MarcoFalke yap, it wasn’t clear. Maybe worth a comment. @jonasschnelli is there a rationale for 1000?
13. 
    achow101 commented at 5:55 pm on February 18, 2019: member

    utACK 0bedcbafdfaa0e980ad91c0ba4b9039575017f0e
14. 
    jonasschnelli commented at 6:23 pm on February 18, 2019: contributor

    On line 387, before throwing, shouldn’t it commit the batch?

    As @MarcoFalke said, if the unique pointer gets out of the scope, it “commits” (flushes) to the database.

    @jonasschnelli is there a rationale for 1000?

    Not really. I thought 1000 is a good compromise between cache memory requirements and performance. A new constant looked after an overkill. Initially I wanted to tie it to the keypool size constant but somehow I thought then that this has nothing to do with memory consumption.

15. meshcollider merged this on Feb 18, 2019
16. meshcollider closed this on Feb 18, 2019

    ---

17. 
    MarcoFalke commented at 8:42 pm on February 18, 2019: member

    1000 seems like a good upper bound, since the speedup flattens out after this:

    (time in us)

    

18. deadalnix referenced this in commit da5b2def31 on Jun 5, 2020
19. DrahtBot locked this on Dec 16, 2021

Contributors
jonasschnelli meshcollider Sjors laanwj promag MarcoFalke achow101

Labels
Wallet

Milestone
0.18.0