wallet: group independent db writes on single batched db transaction #25297
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. |
|
|
||
| if (auto* conf = std::get_if<TxStateConfirmed>(&state)) { | ||
| for (const CTxIn& txin : tx.vin) { | ||
| std::pair<TxSpends::const_iterator, TxSpends::const_iterator> range = mapTxSpends.equal_range(txin.prevout); | ||
| while (range.first != range.second) { | ||
| if (range.first->second != tx.GetHash()) { | ||
| WalletLogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), conf->confirmed_block_hash.ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n); | ||
| MarkConflicted(conf->confirmed_block_hash, conf->confirmed_block_height, range.first->second); | ||
| MarkConflicted(batch, conf->confirmed_block_hash, conf->confirmed_block_height, range.first->second); |
There was a problem hiding this comment.
nit:
| MarkConflicted(batch, conf->confirmed_block_hash, conf->confirmed_block_height, range.first->second); | |
| MarkConflicted(batch, /*hashBlock=*/conf->confirmed_block_hash, /*conflicting_height=*/conf->confirmed_block_height, /*hashTx=*/range.first->second); |
There was a problem hiding this comment.
Probably better to do a const auto& txid = range.first->second; above the if block (the same value is accessed multiple times).
Will do it if have to retouch the area.
|
Thanks for the review w0xlt! About the nits: |
|
Flush on close is probably not quite the performance improvement that you think it is. While it is an improvement for BDB, with SQLite, the argument is ignored. SQLite flushes after each write operation regardless. It would be better to use transactions with Otherwise, I think this is a good idea to be reusing the batches. |
|
Thanks for the feedback 👌🏼
Almost there, been working on it :).
Have another branch with more of them (chained to the batched operations work). I'll probably end up decoupling some of them into another PR (otherwise this will end up being pretty big). |
a26b1ea to
e64d639
Compare
|
Update: Pushed the first step toward the implementation of db write batched operations for entire processes (single atomic writes). Basically, is the utilization of the same db handler across all the methods, whom internally write to db, that are called during a specific process workflow (only the parent method who starts the process will construct the In order to provide the Note: |
8c3c8e0 to
4be5200
Compare
…h' is created c318211 walletdb: fix last client version update (furszy) bda8ebe wallet: don't read db every time that a new WalletBatch is created (furszy) Pull request description: Found it while was working on #25297. We are performing a db read operation every time that a new `WalletBatch` is created, inside the constructor, just to check if the client version field is inside the db or not. As the client version field does not change in the entire db lifecycle, this operation can be done only once: The first time that the db is accessed/opened and the client version value can be cached. ACKs for top commit: achow101: ACK c318211 w0xlt: reACK c318211 Tree-SHA512: 7fb780c656e169e8eb21e7212242494a647f6506d6da2cca828703713d440d29c82bec9e7d2c410f37b49361226ccd80846d3eeb8168383d0c2a11d85d73bee2
…letBatch' is created c318211 walletdb: fix last client version update (furszy) bda8ebe wallet: don't read db every time that a new WalletBatch is created (furszy) Pull request description: Found it while was working on bitcoin#25297. We are performing a db read operation every time that a new `WalletBatch` is created, inside the constructor, just to check if the client version field is inside the db or not. As the client version field does not change in the entire db lifecycle, this operation can be done only once: The first time that the db is accessed/opened and the client version value can be cached. ACKs for top commit: achow101: ACK c318211 w0xlt: reACK bitcoin@c318211 Tree-SHA512: 7fb780c656e169e8eb21e7212242494a647f6506d6da2cca828703713d440d29c82bec9e7d2c410f37b49361226ccd80846d3eeb8168383d0c2a11d85d73bee2
27d1315 to
21a914e
Compare
Add `initialize` flag to 'WalletBatch' constructor. If "initialize=false" the handler will not open nor initialize the db on the constructor. Users can create the handler object beforehand and pass it across different functions as ref. Allowing an entire process to use the same handler, perform batched operations and avoid db access acquisition if no write occurs. The db access will, only, be initialized before the first write/erase operation.
So it can be re-used in time-long scenarios like whole chain scan/rescan and walk-through a block vtx for connection/disconnection where we are creating a new 'WalletBatch' on every single operation. Example: new tx write, addressbook record storage, addressbook "mark used" update, mark tx conflicted, update spent output etc.
…KeyPool, KeepDestination
…call Instead of creating one internally.
…riteDescriptor Instead of creating one internally.
… DeactivateScriptPubKeyMan And remove unimplemented DescriptorScriptPubKeyMan::SetupDescriptor method.
35f3a6e to
c4b366f
Compare
|
🐙 This pull request conflicts with the target branch and needs rebase. |
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
1 similar comment
|
There hasn't been much activity lately and the patch still needs rebase. What is the status here?
|
|
Still relevant and quite useful but closing it until #28574 and its derivatives are merged. |
The block connection process, same as many other wallet processes, contains plenty individual db writes.
This PR batches all the db transaction that occurs along each wallet workflow. Dumping all the information to disk at once atomically at the end of the process.
Then, for BDB, fixed places where we are flushing to db directly on individual writes. e.g we do it in the in the chain sync/scan process, when an output that belongs to the wallet is found if the address is not inside the address book.
Plus, in several places across the wallet flows, we create new
WalletBatchobjects. Which, internally, mean: Increasing the db references number, try to open the db and, for SQLite, setup and bind the statements.This PR avoids all this overhead by sharing the same db handler instance through each entire workflow.