New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from Jul 17, 2017

Conversation

Projects
None yet
8 participants
@gmaxwell
Member

gmaxwell commented Jul 15, 2017

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)

@fanquake fanquake added the Wallet label Jul 15, 2017

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jul 15, 2017

Member

@pstratem you might have some interest in reviewing this.

Member

gmaxwell commented Jul 15, 2017

@pstratem you might have some interest in reviewing this.

@laanwj laanwj added this to the 0.15.0 milestone Jul 15, 2017

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 15, 2017

Member

utACK ff9a291

Member

sipa commented Jul 15, 2017

utACK ff9a291

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs

instagibbs Jul 15, 2017

Member

tACK ff9a291

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

Member

instagibbs commented Jul 15, 2017

tACK ff9a291

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

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 15, 2017

Contributor

utACK ff9a291 (needs rebase, though).

Contributor

TheBlueMatt commented Jul 15, 2017

utACK ff9a291 (needs rebase, though).

Pushdown walletdb object through GenerateNewKey/DeriveNewChildKey.
This is needed but not sufficient for batching the wallet flushing
 when topping up the keypool.
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jul 16, 2017

Member

@TheBlueMatt rebased, please re-review

Member

gmaxwell commented Jul 16, 2017

@TheBlueMatt rebased, please re-review

secret.GetPrivKey(),
mapKeyMetadata[pubkey.GetID()]);
}
return true;
}
bool CWallet::AddKeyPubKey(const CKey& secret, const CPubKey &pubkey)
{
CWalletDB walletdb(*dbw);

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 16, 2017

Contributor

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).

@TheBlueMatt

TheBlueMatt Jul 16, 2017

Contributor

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).

This comment has been minimized.

@gmaxwell

gmaxwell Jul 17, 2017

Member

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.

@gmaxwell

gmaxwell Jul 17, 2017

Member

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.

This comment has been minimized.

@TheBlueMatt

TheBlueMatt Jul 17, 2017

Contributor

I was confused, sorry.

@TheBlueMatt

TheBlueMatt Jul 17, 2017

Contributor

I was confused, sorry.

if (!IsCrypted()) {
return CWalletDB(*dbw).WriteKey(pubkey,
return walletdb.WriteKey(pubkey,

This comment has been minimized.

@promag

promag Jul 17, 2017

Member

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

promag Jul 17, 2017

Member

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.

This comment has been minimized.

@promag

promag Jul 17, 2017

Member

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

@promag

promag Jul 17, 2017

Member

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

This comment has been minimized.

@gmaxwell

gmaxwell Jul 17, 2017

Member

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.

@gmaxwell

gmaxwell Jul 17, 2017

Member

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.

@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 17, 2017

Contributor
Contributor

TheBlueMatt commented Jul 17, 2017

@promag

This comment has been minimized.

Show comment
Hide comment
@promag

promag Jul 17, 2017

Member

@TheBlueMatt another PR then.

utACK 3986271.

Member

promag commented Jul 17, 2017

@TheBlueMatt another PR then.

utACK 3986271.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Jul 17, 2017

Member

reutACK 3986271

Member

sipa commented Jul 17, 2017

reutACK 3986271

throw std::runtime_error(std::string(__func__) + ": writing generated key failed");
}
if (internal) {
setInternalKeyPool.insert(nEnd);

This comment has been minimized.

@laanwj

laanwj Jul 17, 2017

Member

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.

@laanwj

laanwj Jul 17, 2017

Member

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.

This comment has been minimized.

@gmaxwell

gmaxwell Jul 17, 2017

Member

Aggregated to a summary line.

@gmaxwell

gmaxwell Jul 17, 2017

Member

Aggregated to a summary line.

} else {
secret.MakeNewKey(fCompressed);
}
// Compressed public keys were introduced in version 0.6.0
if (fCompressed)
if (fCompressed) {

This comment has been minimized.

@promag

promag Jul 17, 2017

Member

Unrelated, unless you want to fix all style issues?

@promag

promag Jul 17, 2017

Member

Unrelated, unless you want to fix all style issues?

This comment has been minimized.

@instagibbs

instagibbs Jul 17, 2017

Member

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

@instagibbs

instagibbs Jul 17, 2017

Member

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

This comment has been minimized.

@laanwj

laanwj Jul 17, 2017

Member

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.

@laanwj

laanwj Jul 17, 2017

Member

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.

This comment has been minimized.

@gmaxwell

gmaxwell Jul 17, 2017

Member

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.

@gmaxwell

gmaxwell Jul 17, 2017

Member

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.

@instagibbs

This comment has been minimized.

Show comment
Hide comment
@instagibbs
Member

instagibbs commented Jul 17, 2017

reACK 4f81105

@jonasschnelli

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Jul 17, 2017

Member

Nice!
utACK 4f81105.

Member

jonasschnelli commented Jul 17, 2017

Nice!
utACK 4f81105.

return false;
}
if (needsDB) pwalletdbEncryption = NULL;

This comment has been minimized.

@laanwj

laanwj Jul 17, 2017

Member

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.

@laanwj

laanwj Jul 17, 2017

Member

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.

This comment has been minimized.

@gmaxwell

gmaxwell Jul 17, 2017

Member

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

gmaxwell Jul 17, 2017

Member

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.

This comment has been minimized.

@gmaxwell

gmaxwell Jul 17, 2017

Member

Added a comment

@gmaxwell

gmaxwell Jul 17, 2017

Member

Added a comment

gmaxwell added some commits Jul 15, 2017

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.
@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Jul 17, 2017

Member

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

Member

gmaxwell commented Jul 17, 2017

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

@laanwj laanwj merged commit b0e8e2d into bitcoin:master Jul 17, 2017

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Jul 17, 2017

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

b0e8e2d Print one log message per keypool top-up, not one per key. (Gregory Maxwell)
41dc163 Increase wallet default keypool size to 1000. (Gregory Maxwell)
30d8f3a Pushdown walletdb though CWallet::AddKeyPubKey to avoid flushes. (Gregory Maxwell)
3a53f19 Pushdown walletdb object through GenerateNewKey/DeriveNewChildKey. (Gregory Maxwell)

Pull request description:

  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)

Tree-SHA512: 868303de38fce4c3f67d7fe133f765f15435c94b39d252d7450b5fee5c607a3cc2f5e531861a69d8c8877bf130e0ff4c539f97500a6bc0ff6d67e4a42c9385c7
@TheBlueMatt

This comment has been minimized.

Show comment
Hide comment
@TheBlueMatt

TheBlueMatt Jul 17, 2017

Contributor

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

Contributor

TheBlueMatt commented Jul 17, 2017

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment