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.
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.
For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/32985.
See the guideline for information on the review process.
| Type | Reviewers |
|---|---|
| ACK | rkrux |
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.
Reviewers, 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.
lgtm ACK af041c405756d3b8bb04cb2ebd8c32cf237ac2a9
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.
ReorderTransactions() makes during LoadWallet(). If this is the case, these changes are persisted during ReorderTransactions(), so copying the records to the SQLite DB when we do MigrateToSQLite should already transfer the changes to the new DB. Why do we need to rewrite the TX records? What in-memory changes are we trying to persist to the new DB?
