wallet: Migrate wallets that are not in a wallet dir #26740
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. 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. |
There was a problem hiding this comment.
ACK 17ffb12 - LGTM
I cherry-picked the test on master and it indeed crashes.
There was a problem hiding this comment.
Code review ACK 17ffb12
The assertion error happens when you have the default legacy wallet created (no dir, standalone file), and try to migrate another wallet file that is placed in the same base directory.
As we are using the directory path inside the migration process to create the new database instead of the file path, the code auto-completes the path with the wallet.dat file name; which clashes with the default legacy wallet, which already exist, returning nullptr from the db creation process, crashing at the assertion point.
If there would be no default legacy wallet, the process should work "fine". The only problematic would be the migrated wallet file that would be named "wallet.dat" instead of using the wallet's real filename.
17ffb12 to
52634db
Compare
52634db to
0a0f293
Compare
Co-Authored-By: Ryan Ofsky <ryan@ofsky.org>
0a0f293 to
a1e6538
Compare
… dir a1e6538 test: Add test for migrating default wallet and plain file wallet (Andrew Chow) bdbe3fd wallet: Generated migrated wallet's path from walletdir and name (Andrew Chow) Pull request description: This PR fixes an assertion error that is hit during the setup of the new database during migration of a wallet that was not contained in a wallet dir. Also added a test for this case as well as one for migrating the default wallet. ACKs for top commit: ryanofsky: Code review ACK a1e6538 furszy: ACK a1e6538 Tree-SHA512: 96b218c0de8567d8650ec96e1bf58b0f8ca4c4726f5efc6362453979b56b9d569baea0bb09befb3a5aed8d16d29bf75ed5cd8ffc432bbd4cbcad3ac5574bc479
This PR fixes an assertion error that is hit during the setup of the new database during migration of a wallet that was not contained in a wallet dir. Also added a test for this case as well as one for migrating the default wallet.