-
Notifications
You must be signed in to change notification settings - Fork 35.5k
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
wallet: optimize migration process, batch db transactions #28574
base: master
Are you sure you want to change the base?
Conversation
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code CoverageFor detailed information about the code coverage, see the test coverage report. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsReviewers, this pull request conflicts with the following ones:
If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first. |
1bc914b
to
3ea30fc
Compare
8476a92
to
6a2e6e4
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Concept ACK.
I'll also perform the benchmark as instructed clearly in the description and will review the code later.
Concept ACK |
… single db txn f053024 wallet: batch external signer descriptor import (Sjors Provoost) 1f65241 wallet: descriptors setup, batch db operations (furszy) 3eb769f wallet: batch legacy spkm TopUp (furszy) 075aa44 wallet: batch descriptor spkm TopUp (furszy) bb4554c bench: add benchmark for wallet creation procedure (furszy) Pull request description: Work decoupled from #28574. Instead of performing multiple single write operations per spkm setup call, this PR batches them all within a single atomic db txn. Speeding up the process and preventing the wallet from entering an inconsistent state if any of the intermediate transactions fail (which shouldn't happen but.. if it does, it is better to not store any spkm rather than storing them partially). To compare the changes, added benchmark in the first commit. ACKs for top commit: Sjors: re-utACK f053024 achow101: ACK f053024 BrandonOdiwuor: ACK f053024 theStack: Code-review ACK f053024 Tree-SHA512: aead8548473e17d4d53e8e7039bbaf5e8bf2fe83f33b33f81cdedefe8a31b7003ceb6d5379b1bad1ca2692e909492009a21284ec8338eede078df3d19046ab5a
rebased after #29130 merge. |
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
Turned into draft for now, due to failing CI. If this is no longer a WIP, you can move it out of draft. |
4f9d447
to
e898c85
Compare
🚧 At least one of the CI tasks failed. Make sure to run all tests locally, according to the Possibly this is due to a silent merge conflict (the changes in this pull request being Leave a comment here, if you need help tracking down a confusing failure. |
Grouping all db writes into a single atomic write operation. Speeding up the flow and preventing inconsistent states.
This will be useful in the following-up commit to batch the entire wallet migration process.
So it can be used within an external db txn context.
The wallet is isolated during migration and reloaded at the end of the process. There is no benefit on connecting the signals few lines before unloading the wallet.
Useful to ensure that the in-memory state is updated only after successfully committing the data to disk.
Preparing it to be used within a broader db txn procedure.
e898c85
to
4e72821
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Rebased due a hidden conflict with #26606.
Last step in a chain of PRs (#26836, #28894, #28987, #29403).
The initial benchmark conducted locally showed a ~65% processing time reduction, on a SSD.
Results, from the very first benchmark, can be found at the end of the description.
Detailed Description:
The current wallet migration process performs only individual db writes. Accessing disk to
delete all legacy records, clone and clean each address book entry for every created wallet,
create each new descriptor (with their corresponding master key, caches and key pool), and
also clone and delete each transaction that requires to be transferred to a different wallet.
This work consolidates all individual disk writes into two batch operations. One for the descriptors
creation from the legacy data and a second one for the execution of the migration process itself.
Efficiently dumping all the information to disk at once atomically at the end of each process.
This represent a speed up and also a consistency improvement. During migration, we either
want to succeed or fail. No other outcomes should be accepted. We should never leave a
partially migrated wallet on disk and request the user to manually restore the previous wallet from
a backup (at least not if we can avoid it).
Additionally, since db transactions relevant to the key pool generation were also batched, this work
also speeds up the regular wallet creation process.
Note for Testers:
The first commit introduces a benchmark for the migration process. This one can be
cherry-picked on top of master to compare results pre and post changes.
Please note that the benchmark setup may take some time (~70 seconds here) due to the absence
of a batching mechanism for the address generation process (
GetNewDestination()
calls). Which,I plan to address in a separate PR, just to not continue expanding this PR.
————————————————————————
Benchmark on master (debug mode)
WalletMigration
Benchmark on this branch (debug mode)
WalletMigration