wallet: simplify and batch zap wallet txes process #28987
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. |
The function does not return DBErrors::NEED_REWRITE.
The wallet is unloaded at the beginning of the migration process, so no object is listening to the signals.
furszy
force-pushed
the
2023_wallet_zaptx
branch
from
ff21425
to
d26a285
Compare
src/wallet/wallet.cpp
Outdated
|
|
||
| // Apply db changes and remove transactions from the memory map | ||
| if (!batch.TxnCommit()) return DBErrors::NONCRITICAL_ERROR; |
There was a problem hiding this comment.
In d26a285 "wallet: batch ZapSelectTx db operations"
I'm not sure if NONCRITICAL_ERROR is really the right return code. It seems like something has gone critically wrong if TxnCommit were to fail.
There was a problem hiding this comment.
I'm not sure if
NONCRITICAL_ERRORis really the right return code. It seems like something has gone critically wrong ifTxnCommitwere to fail.
The returned error code doesn't really matter. Anything different from LOAD_OK is an error for all the function's callers. And they behave exactly the same for all codes.
Actually, the DBErrors return doesn't make sense anymore after this PR changes. The function is no longer traversing the entire db nor checking for extra, unneeded, stuff like the version inside anymore.
Based on this, I have reworked the PR cleaning up this aspect as well.
furszy
force-pushed
the
2023_wallet_zaptx
branch
3 times, most recently
from
875dbfb
to
e4f8a02
Compare
src/wallet/wallet.cpp
Outdated
|
|
||
| // Apply db changes and remove transactions from the memory map | ||
| if (!batch.TxnCommit()) return DBErrors::NONCRITICAL_ERROR; |
There was a problem hiding this comment.
I'm not sure if
NONCRITICAL_ERRORis really the right return code. It seems like something has gone critically wrong ifTxnCommitwere to fail.
The returned error code doesn't really matter. Anything different from LOAD_OK is an error for all the function's callers. And they behave exactly the same for all codes.
Actually, the DBErrors return doesn't make sense anymore after this PR changes. The function is no longer traversing the entire db nor checking for extra, unneeded, stuff like the version inside anymore.
Based on this, I have reworked the PR cleaning up this aspect as well.
The goal of the function is to erase the wallet transactions that match the inputted hashes. There is no need to traverse the database, reading record by record, to then perform single entry removals for each of them. To ensure consistency and improve performance, this change-set removes all tx records within a single atomic db batch operation, as well as it cleans up code, improves error handling and simplifies the transactions removal process entirely. This optimizes the removal of watch-only transactions during the wallet migration process and the 'removeprunedfunds' RPC command.
-BEGIN VERIFY SCRIPT- sed -i 's/ZapSelectTx/RemoveTxs/g' $(git grep -l 'ZapSelectTx' ./src/wallet) -END VERIFY SCRIPT-
furszy
force-pushed
the
2023_wallet_zaptx
branch
from
e4f8a02
to
9a3c5c8
Compare
|
ACK 9a3c5c8 The new code is much simpler. |
|
ACK 9a3c5c8 looks good, also |
| if (ZapSelectTx(txids_to_delete, deleted_txids) != DBErrors::LOAD_OK) { | ||
| error = _("Error: Could not delete watchonly transactions"); | ||
| if (auto res = RemoveTxs(txids_to_delete); !res) { | ||
| error = _("Error: Could not delete watchonly transactions. ") + util::ErrorString(res); |
There was a problem hiding this comment.
nit: The trailing space could be easily overlooked and dropped during translation.
There was a problem hiding this comment.
nit: The trailing space could be easily overlooked and dropped during translation.
@hebasto, what about creating a constant to represent the whitespace, or alternatively, implementing a function that concatenates strings with the whitespace inserted between them?
Work decoupled from #28574. Brother of #28894.
Includes two different, yet interconnected, performance and code improvements to the zap wallet transactions process.
As the goal of the
ZapSelectTxfunction is to erase tx records that match any of the inputted hashes. There is no need to traverse the whole database record by record. We could just check if the tx exist, and remove it directly by callingEraseTx().Instead of performing single write operations per removed tx record, this PR batches them all within a single atomic db txn.
Moreover, these changes will enable us to consolidate all individual write operations that take place during the wallet migration process into a single db txn in the future.