Skip to content
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

Use a single wallet batch for UpgradeKeyMetadata #15433

Merged

Conversation

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Feb 18, 2019

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.

@jonasschnelli jonasschnelli force-pushed the 2019/02/wallet_key_upgrade_batch branch from 204ad5c to 0bedcba Feb 18, 2019
@meshcollider meshcollider added this to the 0.18.0 milestone Feb 18, 2019
@meshcollider
Copy link
Member

@meshcollider meshcollider commented Feb 18, 2019

Tagged for 0.18 because I'd consider this a bugfix

@Sjors
Copy link
Member

@Sjors Sjors commented Feb 18, 2019

Concept ACK.

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

@laanwj
Copy link
Member

@laanwj laanwj commented Feb 18, 2019

utACK and concept ACK 0bedcba

@meshcollider
Copy link
Member

@meshcollider meshcollider commented Feb 18, 2019

utACK 0bedcba

@achow101 want to take a look?

Copy link
Member

@promag promag left a comment

Concept ACK.

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

@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Feb 18, 2019

@promag Wouldn't it doe that, as the unique ptr gets destroyed (and flush on exit was set to true by default)?

@promag
Copy link
Member

@promag promag commented Feb 18, 2019

@MarcoFalke yap, it wasn't clear. Maybe worth a comment.

@jonasschnelli is there a rationale for 1000?

@achow101
Copy link
Member

@achow101 achow101 commented Feb 18, 2019

utACK 0bedcba

@jonasschnelli
Copy link
Contributor Author

@jonasschnelli jonasschnelli commented Feb 18, 2019

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.

meshcollider added a commit to meshcollider/bitcoin that referenced this issue Feb 18, 2019
0bedcba Use a single wallet batch for UpgradeKeyMetadata (Jonas Schnelli)

Pull request description:

  Opening wallets (the first time) after bitcoin#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.

Tree-SHA512: f68739e452d382f5294186f47511b94884a1a0868688dd3179034a7e091a67f93bc9dd45cdfc9fa6b1fe90362772b719278012f2f56b752b803c87db8597a7b0
@meshcollider meshcollider merged commit 0bedcba into bitcoin:master Feb 18, 2019
2 checks passed
@MarcoFalke
Copy link
Member

@MarcoFalke MarcoFalke commented Feb 18, 2019

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

(time in us)

upgradekeymetadata time

deadalnix added a commit to Bitcoin-ABC/bitcoin-abc that referenced this issue Jun 5, 2020
Summary: This is a backport of [[bitcoin/bitcoin#15433 | PR15433]]

Test Plan:
  ninja all check-all

Reviewers: #bitcoin_abc, majcosta

Reviewed By: #bitcoin_abc, majcosta

Differential Revision: https://reviews.bitcoinabc.org/D6383
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

7 participants