wallet: Always rewrite tx records during migration #32985
Conversation
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. Code Coverage & BenchmarksFor details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32985. 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. |
Since loading a wallet may change some parts of tx records (e.g. adding nOrderPos), we should rewrite the records instead of copying them so that the automatic upgrade does not need to be performed again when the wallet is loaded.
4b06b56 to
af041c4
Compare
rkrux
left a comment
There was a problem hiding this comment.
lgtm ACK af041c4
This appears to be a preparatory commit for the latter PR #33034 where transaction is written in the SQLite transactions table as well during migration. This appears fine also because during wallet loading, the transaction is rewritten if it needs to be reordered based on an updated nOrderPos.
I don't suppose there is an easy way to test this in the wallet_migration functional test because the wallet is loaded right after migration is done and this subsequent load would also rewrite the transaction if needed. There is an inspect_sqlite_db function in the functional tests that can query the wallet data before the post-migration load but I don't know if it'd be trivial to generate a specific test that causes the transaction data to change after pre-migration load. Thus, not suggesting to add a functional test for this.
|
I'm assuming the PR description is referring to the changes |
Not for migration since legacy wallets are only opened with a |
Since loading a wallet may change some parts of tx records (e.g. adding nOrderPos), we should rewrite the records instead of copying them so that the automatic upgrade does not need to be performed again when the wallet is loaded.
This is useful for future PRs I'm working on where we need to be sure about what data exists in a tx record in descriptor wallets.