[Wallet]Perform entire CWallet::TopUpKeyPool in a transaction. #7064

pull pstratem wants to merge 3 commits into bitcoin:master from pstratem:2015-11-19-wallet-topupkeypool changing 2 files +56 −38

pstratem commented at 2:29 AM on November 20, 2015: contributor

Previously after each call to GenerateNewKey the log would be flushed causing a massive performance penalty.
Rename pwalletdbEncryption to pwalletdb. 7ba1e839bc
Use pwalletdb in CWallet::AddKeyPubKey if available 7bc9a449fc

Perform entire CWallet::TopUpKeyPool in a transaction.

Previously after each call to GenerateNewKey the log would be flushed
causing a massive performance penalty.

a4471ca955

pstratem commented at 2:30 AM on November 20, 2015: contributor

Massive performance improvement.

Adding 10k keys went from 10 minutes to <10 seconds.
pstratem renamed this:
~~Perform entire CWallet::TopUpKeyPool in a transaction.~~
[Wallet]Perform entire CWallet::TopUpKeyPool in a transaction.
on Nov 20, 2015
jonasschnelli commented at 7:22 AM on November 20, 2015: contributor

Nice to see @pstratem work on the wallet!

Code-Review utACK.
jonasschnelli added the label Wallet on Nov 20, 2015
laanwj commented at 7:46 AM on November 20, 2015: member

That's a very nice speed improvement. Concept ACK.
instagibbs commented at 4:13 PM on November 20, 2015: member

My laptop goes from simply locking up in a few minutes and crashing(something IO related that I can't figure out) to ~2 seconds.

lightly tested ACK
sipa commented at 8:58 PM on November 26, 2015: member

Code review + concept ACK.

in src/wallet/wallet.cpp:None in a4471ca955

 120 | @@ -121,9 +121,15 @@ bool CWallet::AddKeyPubKey(const CKey& secret, const CPubKey &pubkey)
 121 |      if (!fFileBacked)
 122 |          return true;
 123 |      if (!IsCrypted()) {
 124 | -        return CWalletDB(strWalletFile).WriteKey(pubkey,
 125 | -                                                 secret.GetPrivKey(),
 126 | -                                                 mapKeyMetadata[pubkey.GetID()]);
 127 | +        if (pwalletdb) {

laanwj commented at 10:31 AM on November 27, 2015:

I don't like the code duplication here at every call site, as well as the manual deleting setting-to-NULL of pwalletdb. What about a inner class for context management like

/** Database reference. Can be nested, in which case the outer reference will own the handle.
  */
class DBRef
{
private:
    bool owning;
public:
    DBRef(CWallet * wallet): owning(false)
    {
        if (!wallet->pwalletdb) {
            wallet->pwalletdb = new CWalletDB(wallet->strWalletFile);
            owning = true;
       }
    }
    CWalletDB &get() { return *wallet->pwalletdb;  }
    ~DBRef()
    {
         if(owning) {
             delete wallet->pwalletdb;
             wallet->pwalletdb = NULL;
         }
    }
};

Then you can change this to

DBRef ref;
ref.get().WriteKey(pubkey, secret.GetPrivKey(), mapKeyMetadata[pubkey.GetID());

pstratem commented at 1:50 AM on December 8, 2015:

@laanwj yes i agree the if/else everywhere would get ugly very fast

pstratem closed this on Dec 13, 2015
MarcoFalke commented at 12:42 PM on December 13, 2015: member

Why is this closed now?
jgarzik commented at 3:33 AM on December 14, 2015: contributor

Certainly would like to see some version of this go in...
MarcoFalke locked this on Sep 8, 2021

Contributors

Labels