Batch flushing operations to the walletdb during top up and increase keypool size. #10831

1. 
   gmaxwell commented at 1:17 AM on July 15, 2017: contributor

   This carries the walletdb object from top-up through GenerateNewKey/DeriveNewChildKey/CWallet::AddKeyPubKey, which allows us to avoid the flush on destruction until the top up finishes instead of flushing the wallet for every key.

   This speeds up adding keys by well over 10x on my laptop (actually something like 17x), I wouldn't be surprised if it were an even bigger speedup on spinning rust.

   Then it increases the keypool size to 1000. I would have preferred to use 10,000 but in the case where the user creates a new wallet and then turns on encryption it seems kind of dumb to have >400KB of marked-used born unencrypted keys just laying around.

   (Thanks to Matt for cluesticking me on how to bypass the crypter spaghetti)

2. fanquake added the label Wallet on Jul 15, 2017
3. 
   gmaxwell commented at 4:02 AM on July 15, 2017: contributor

   @pstratem you might have some interest in reviewing this.

4. laanwj added this to the milestone 0.15.0 on Jul 15, 2017
5. 
   sipa commented at 6:06 PM on July 15, 2017: member

   utACK ff9a291b30d7464c1ff0990b1cab36d6119b2ae5

6. 
   instagibbs commented at 7:09 PM on July 15, 2017: member

   tACK https://github.com/bitcoin/bitcoin/pull/10831/commits/ff9a291b30d7464c1ff0990b1cab36d6119b2ae5

   with 1000 keypool size, topup went from 20 seconds to 1.6.

7. 
   TheBlueMatt commented at 11:41 PM on July 15, 2017: member

   utACK ff9a291b30d7464c1ff0990b1cab36d6119b2ae5 (needs rebase, though).

8. Pushdown walletdb object through GenerateNewKey/DeriveNewChildKey.

   This is needed but not sufficient for batching the wallet flushing
    when topping up the keypool.

   3a53f19718
9. 
   gmaxwell commented at 12:18 AM on July 16, 2017: contributor

   @TheBlueMatt rebased, please re-review

10. in src/wallet/wallet.cpp:209 in 3986271c0a outdated

     207 |      return true;
     208 |  }
     209 |  
     210 | +bool CWallet::AddKeyPubKey(const CKey& secret, const CPubKey &pubkey)
     211 | +{
     212 | +    CWalletDB walletdb(*dbw);
    

    ---

    ---

    

    TheBlueMatt commented at 11:05 PM on July 16, 2017:

    It seems strange that we're creating a db transaction, only to throw it out in a second. I think its safe, but it might be nicer to let AddKeyPubKeyWithDB create a CWalletDB if it needs one automagically (eg by passing the walletdb in as a pointer).

    ---

    

    gmaxwell commented at 2:57 AM on July 17, 2017:

    This isn't called when we don't need one; the case where we already have a dbwallet calls the WithDB version. (and changing the prototype of the function would mean changing the function it overrides, which a db handle doesn't even make sense for, and 29 callsites) Unless I'm misunderstanding you.

    ---

    

    TheBlueMatt commented at 3:17 PM on July 17, 2017:

    I was confused, sorry.

11. in src/wallet/wallet.cpp:200 in 3986271c0a outdated

     198 |          RemoveWatchOnly(script);
     199 | +    }
     200 |  
     201 |      if (!IsCrypted()) {
     202 | -        return CWalletDB(*dbw).WriteKey(pubkey,
     203 | +        return walletdb.WriteKey(pubkey,
    

    ---

    ---

    

    promag commented at 1:03 AM on July 17, 2017:

    Another approach is to use dbw as the storage of the transaction. For instance, CWalletDBWrapper could keep a CBD instance, referenced counted by CWalletDB. This could be thread safe too.

    ---

    

    promag commented at 1:06 AM on July 17, 2017:

    I'm happy to write such alternative if there is interest.

    ---

    

    gmaxwell commented at 2:58 AM on July 17, 2017:

    Seems like a reasonable thing to do, but I didn't even want to consider anything that complex-- I wanted to eliminate the complaint that having many keys is 'slow' for 0.15 because of the bad interactions with hd-wallet and rescan when the keypool isn't big.

12. 
    TheBlueMatt commented at 1:17 AM on July 17, 2017: member

    @promag sadly I think it's a bit late for that for 15, it might be a decent change later, however.

    On July 16, 2017 7:05:52 PM EDT, Matt Corallo notifications@github.com wrote:

    TheBlueMatt commented on this pull request.

    secret.GetPrivKey(), mapKeyMetadata[pubkey.GetID()]); } return true; }

    +bool CWallet::AddKeyPubKey(const CKey& secret, const CPubKey &pubkey) +{

    • CWalletDB walletdb(*dbw);

    It seems strange that we're creating a db transaction, only to throw it out in a second. I think its safe, but it might be nicer to let AddKeyPubKeyWithDB create a CWalletDB if it needs one automagically (eg by passing the walletdb in as a pointer).

13. 
    promag commented at 7:45 AM on July 17, 2017: member

    @TheBlueMatt another PR then.

    utACK 3986271.

14. 
    sipa commented at 8:03 AM on July 17, 2017: member

    reutACK 3986271c0accfe896c66a48d3ed884a7553b6413

15. in src/wallet/wallet.cpp:3212 in 3986271c0a outdated

    3205 | +            if (!walletdb.WritePool(nEnd, CKeyPool(GenerateNewKey(walletdb, internal), internal))) {
    3206 |                  throw std::runtime_error(std::string(__func__) + ": writing generated key failed");
    3207 | +            }
    3208 |  
    3209 |              if (internal) {
    3210 |                  setInternalKeyPool.insert(nEnd);
    

    ---

    ---

    

    laanwj commented at 8:04 AM on July 17, 2017:

    If we increase the default keypool size, the LogPrintf("keypool added key %d message below shouldn't be logged for every key. Or at least moved to a debug category. It's ugly to log that many messages at first run, and it is performance overhead too.

    ---

    

    gmaxwell commented at 2:00 PM on July 17, 2017:

    Aggregated to a summary line.

16. in src/wallet/wallet.cpp:109 in 4f811059fa outdated

     106 |          secret.MakeNewKey(fCompressed);
     107 |      }
     108 |  
     109 |      // Compressed public keys were introduced in version 0.6.0
     110 | -    if (fCompressed)
     111 | +    if (fCompressed) {
    

    ---

    ---

    

    promag commented at 9:16 AM on July 17, 2017:

    Unrelated, unless you want to fix all style issues?

    ---

    

    instagibbs commented at 1:01 PM on July 17, 2017:

    I don't think we should nit people doing partial style issue fixes unless it affects reviewability.

    ---

    

    laanwj commented at 1:10 PM on July 17, 2017:

    Yes, this seems to be a "damned if you do, damned if you don't" thing.

    • If you deliberately make style fixes to code you touch, you get comments that it's unrelated
    • If you deliberately don't make style fixes to code you touch, you get questions why you didn't update the code for the style guidelines

    I think both are valid, unless it makes review-ability in which case it could still be done in a separate commit in the same PR.

    ---

    

    gmaxwell commented at 2:01 PM on July 17, 2017:

    I have been trying to limit them to things that will show up in the same patch hunks, and only changes which I'm pretty sure won't break review. I'm happy to do what other people want, though I prefer to at least fix braces (because I dislike reviewing the risky looking unbraced code myself)-- but ultimately I just need clear guidance to avoid the damned if/don't.

17. 
    instagibbs commented at 1:05 PM on July 17, 2017: member

    reACK https://github.com/bitcoin/bitcoin/pull/10831/commits/4f811059fab9a9121befae09f66b9b12244051f0

18. 
    jonasschnelli commented at 1:20 PM on July 17, 2017: contributor

    Nice! utACK 4f811059fab9a9121befae09f66b9b12244051f0.

19. Pushdown walletdb though CWallet::AddKeyPubKey to avoid flushes.

    This prevents the wallet from being flushed between each and
     every key during top-up.  This results in a >10x speed-up
     for the top-up.

    30d8f3a18e
20. Increase wallet default keypool size to 1000. 41dc163587
21. Print one log message per keypool top-up, not one per key. b0e8e2de84
22. in src/wallet/wallet.cpp:186 in 4f811059fa outdated

     181 | +    }
     182 | +    if (!CCryptoKeyStore::AddKeyPubKey(secret, pubkey)) {
     183 | +        if (needsDB) pwalletdbEncryption = NULL;
     184 |          return false;
     185 | +    }
     186 | +    if (needsDB) pwalletdbEncryption = NULL;
    

    ---

    ---

    

    laanwj commented at 1:23 PM on July 17, 2017:

    I don't understand this code. Given that pwalletdbEncryption is not used in CCryptoKeyStore::AddKeyPubKey, could this code be moved before if (!CCryptoKeyStore::AddKeyPubKey? This would avoid some awkward duplication here. Or is it used indirectly? This looks really sneaky. Please add a comment if you need to keep it at least, so that no one else gets my bad idea.

    ---

    

    gmaxwell commented at 1:36 PM on July 17, 2017:

    It's used-- alas, the whole purpose of that pre-existing terrible variable is because CCryptoKeyStore::AddKeyPubKey calls AddCryptedKey which on actual wallets is overridden to the function below. CCryptoKeyStore rightfully has no concept of wallet databases, so the variable is used to tunnel through it, to get it to the AddCryptedKey override below. This infrastructure was preexisting because the encryption setup needs it. I can add a comment explaining it. Some people have suggested larger refactors to remove this knitted maze, but it's clearly out of scope for now.

    ---

    

    gmaxwell commented at 1:46 PM on July 17, 2017:

    Added a comment

23. 
    gmaxwell commented at 2:02 PM on July 17, 2017: contributor

    (new revisions are just due to adding a comment in the middle of the patch stack)

24. laanwj merged this on Jul 17, 2017
25. laanwj closed this on Jul 17, 2017

    ---

26. laanwj referenced this in commit 0b019357ff on Jul 17, 2017
27. 
    TheBlueMatt commented at 3:19 PM on July 17, 2017: member

    postumous utACK b0e8e2de8408cbaed9d70914c67b4c9f11397cb7 (lets clean up some of this stuff in 16).

28. PastaPastaPasta referenced this in commit e1c33b27ae on Aug 6, 2019
29. PastaPastaPasta referenced this in commit eb23923814 on Aug 6, 2019
30. PastaPastaPasta referenced this in commit 579daf962f on Aug 6, 2019
31. PastaPastaPasta referenced this in commit e682ac37e0 on Aug 6, 2019
32. PastaPastaPasta referenced this in commit 6d393030ee on Aug 6, 2019
33. UdjinM6 referenced this in commit 0599d3a030 on Aug 16, 2019
34. barrystyle referenced this in commit 70be863af9 on Jan 22, 2020
35. wqking referenced this in commit db0f560477 on Jan 29, 2020
36. tohsnoom referenced this in commit ed4ff4f92a on Jun 12, 2020
37. wqking referenced this in commit eeed6d9c8e on Aug 10, 2021
38. DrahtBot locked this on Sep 8, 2021

Contributors

gmaxwellsipainstagibbsTheBlueMattpromaglaanwjjonasschnelli

Milestone
0.15.0