wallet: Always rewrite tx records during migration#32985
wallet: Always rewrite tx records during migration#32985sedited merged 1 commit intobitcoin:masterfrom
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 copy-paste 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.
achow101
force-pushed
the
migratewallet-rewrite-txs
branch
from
4b06b56 to
af041c4
Compare
rkrux
left a comment
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 |
| // Rewrite the transaction so that anything that may have changed about it in memory also persists to disk | ||
| local_wallet_batch.WriteTx(*wtx); | ||
| } | ||
|
|
||
| // Do the removes | ||
| if (txids_to_delete.size() > 0) { | ||
| if (auto res = RemoveTxs(local_wallet_batch, txids_to_delete); !res) { | ||
| return util::Error{_("Error: Could not delete watchonly transactions. ") + util::ErrorString(res)}; | ||
| } | ||
| } |
There was a problem hiding this comment.
I fail to see how this fixes a bad nOrderPos as the field is not updated after removal and only during wallet load.
Wouldn't be better to call ReorderTransactions after the txs removal? Which also calls WriteTx internally which is what you want to do here anyway.
There was a problem hiding this comment.
nOrderPos is updated in memory on loading, but not written. By calling WriteTx here, we're able to persist that to the new database.
While there will be gaps in the ordering, that doesn't really matter.
There was a problem hiding this comment.
nOrderPos is updated in memory on loading, but not written. By calling WriteTx here, we're able to persist that to the new database.
Note for others: this is because we load the BDB wallet in read-only mode during migration.
6 participants
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.