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

gmaxwell · 2017-07-15T01:17:47Z

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)

gmaxwell · 2017-07-15T04:02:06Z

@pstratem you might have some interest in reviewing this.

sipa · 2017-07-15T18:06:54Z

utACK ff9a291b30d7464c1ff0990b1cab36d6119b2ae5

instagibbs · 2017-07-15T19:09:04Z

tACK ff9a291

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

TheBlueMatt · 2017-07-15T23:41:39Z

utACK ff9a291b30d7464c1ff0990b1cab36d6119b2ae5 (needs rebase, though).

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

gmaxwell · 2017-07-16T00:18:24Z

@TheBlueMatt rebased, please re-review

TheBlueMatt · 2017-07-16T23:05:48Z

src/wallet/wallet.cpp

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

I was confused, sorry.

promag · 2017-07-17T01:03:21Z

src/wallet/wallet.cpp

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.

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

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 · 2017-07-17T01:17:44Z

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

promag · 2017-07-17T07:45:51Z

@TheBlueMatt another PR then.

utACK 3986271.

sipa · 2017-07-17T08:03:52Z

reutACK 3986271c0accfe896c66a48d3ed884a7553b6413

laanwj · 2017-07-17T08:04:17Z

src/wallet/wallet.cpp

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.

Aggregated to a summary line.

promag · 2017-07-17T09:16:51Z

src/wallet/wallet.cpp

Unrelated, unless you want to fix all style issues?

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

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.

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 · 2017-07-17T13:05:21Z

reACK 4f81105

jonasschnelli · 2017-07-17T13:20:25Z

Nice!
utACK 4f811059fab9a9121befae09f66b9b12244051f0.

laanwj · 2017-07-17T13:23:28Z

src/wallet/wallet.cpp

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.

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.

Added a comment

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 · 2017-07-17T14:02:24Z

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

… 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 · 2017-07-17T15:19:39Z

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

Signed-off-by: Pasta <pasta@dashboost.org>

…ing top up and increase keypool size (#3045) * backport bitcoin#10831 Signed-off-by: Pasta <pasta@dashboost.org> * Apply the same logic for SetHDChain/SetCryptedHDChain

Reference: bitcoin#10831

Reference: bitcoin/bitcoin#10831

fanquake added the Wallet label Jul 15, 2017

laanwj added this to the 0.15.0 milestone Jul 15, 2017

Pushdown walletdb object through GenerateNewKey/DeriveNewChildKey.

3a53f19

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

TheBlueMatt reviewed Jul 16, 2017

View reviewed changes

promag reviewed Jul 17, 2017

View reviewed changes

laanwj reviewed Jul 17, 2017

View reviewed changes

promag reviewed Jul 17, 2017

View reviewed changes

laanwj reviewed Jul 17, 2017

View reviewed changes

gmaxwell added 3 commits July 17, 2017 13:46

Pushdown walletdb though CWallet::AddKeyPubKey to avoid flushes.

30d8f3a

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.

Increase wallet default keypool size to 1000.

41dc163

Print one log message per keypool top-up, not one per key.

b0e8e2d

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

PastaPastaPasta added a commit to PastaPastaPasta/dash that referenced this pull request Aug 6, 2019

backport bitcoin#10831

6d39303

Signed-off-by: Pasta <pasta@dashboost.org>

wqking added a commit to wqking-misc/Phore that referenced this pull request Jan 29, 2020

Batch flushing operations to the walletdb

db0f560

Reference: bitcoin#10831

tohsnoom pushed a commit to phoreproject/Phore that referenced this pull request Jun 12, 2020

Batch flushing operations to the walletdb

ed4ff4f

Reference: bitcoin#10831

wqking added a commit to wqking-misc/SocialSend that referenced this pull request Aug 10, 2021

Batch flushing operations to the walletdb

eeed6d9

Reference: bitcoin/bitcoin#10831

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

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

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

Uh oh!

Conversation

gmaxwell commented Jul 15, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

gmaxwell commented Jul 15, 2017

Uh oh!

sipa commented Jul 15, 2017

Uh oh!

instagibbs commented Jul 15, 2017

Uh oh!

TheBlueMatt commented Jul 15, 2017

Uh oh!

gmaxwell commented Jul 16, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

TheBlueMatt commented Jul 17, 2017 via email

Uh oh!

promag commented Jul 17, 2017

Uh oh!

sipa commented Jul 17, 2017

Uh oh!

laanwj Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

laanwj Jul 17, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

instagibbs commented Jul 17, 2017

Uh oh!

jonasschnelli commented Jul 17, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

gmaxwell commented Jul 17, 2017

Uh oh!

TheBlueMatt commented Jul 17, 2017

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

8 participants

gmaxwell commented Jul 15, 2017 •

edited

Loading

laanwj Jul 17, 2017 •

edited

Loading

laanwj Jul 17, 2017 •

edited

Loading