wallet: Avoid unnecessary wtxvariant rewrites - #35935
Conversation
When writing a tx to the wallet, we don't always need to rewrite all of the wtx variants. Most writes can write the single tx record since they are only updating metadata stored in that record.
|
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/35935. ReviewsSee the guideline and AI policy 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. |
When a CWalletTx is updated, we should immediately write the changes to the database. The update may be writing a new wtx variant, updating the metadata, or both. There is no need to rewrite all wtx variants or update the metadata if it did not change.
Clarify in the name that this function writes the entire metadata and all of the witness tx variants.
30e17f9 to
b374e88
Compare
pablomartin4btc
left a comment
pablomartin4btc
left a comment
There was a problem hiding this comment.
Concept ACK
WriteFullTx (previously WriteTx) was a single all-or-nothing write regardless of what actually changed. This PR correctly separates the two independent concerns: variant records (write-once, immutable) and metadata (mutable, state-driven), and writes only what the specific operation requires.
Left a few comments...
| // The canonical wtxid is also updated. The tx that is confirmed becomes canonical. For unconfirmed txs, | ||
| // those with witnesses are preferred, followed by least weight. | ||
| bool Update(CTransactionRef tx, const TxState& arg_state); | ||
| bool Update(CTransactionRef tx, const TxState& arg_state, WalletBatch& batch); |
| bool EraseTx(Txid hash); | ||
| // Write a single witness variant of CWalletTx (single wtxvariant record) | ||
| bool WriteWtxVariant(const Txid& txid, const CTransactionRef& tx); | ||
| // Write only the canonical witness tx and all of the tx metadata (single tx record) |
There was a problem hiding this comment.
nit: this is a bit confusing I think — it seems to say "canonical witness tx" and "all metadata" but this function only writes ONE record (the DBKeys::TX record). Perhaps something like "// Write only the tx record (state, timestamps, canonical wtxid) — no wtxvariant records" would be clearer.
| // Break caches since we have changed the state | ||
| desc_tx->MarkDirty(); | ||
| batch.WriteTx(*desc_tx); | ||
| batch.WriteTxMetadata(*desc_tx); |
There was a problem hiding this comment.
nit: return value discarded here — pre-existing from WriteTx, but since we're touching this area could be worth addressing (?).
| } | ||
| // Rewrite the transaction so that anything that may have changed about it in memory also persists to disk | ||
| local_wallet_batch.WriteTx(*wtx); | ||
| local_wallet_batch.WriteTxMetadata(*wtx); |
There was a problem hiding this comment.
nit: same return value discarded here...
| const auto& [tx_pair, new_variant] = m_txs.emplace(new_tx->GetWitnessHash(), std::move(new_tx)); | ||
| if (new_variant) { | ||
| if (!batch.WriteWtxVariant(GetHash(), tx_pair->second)) { | ||
| throw std::ios_base::failure("Unable to write wtxvariant record"); |
There was a problem hiding this comment.
All other write methods in this area return bool on failure. Update() throwing std::ios_base::failure works, but it means callers need an unexpected try-catch.
3 participants
wtxvariantrecords should never change, so it is unnecessary for us to be rewriting allwtxvariantsin aCWalletTxevery time theCWalletTx's state changes. Likewise, every time there is a new variant,CWalletTxstates may not change so do not need to always be unconditionally written.This PR adds a
WalletBatch::WriteTxMetadatato write just thetxrecord (the former behavior ofWriteTx) and renamesWriteTxtoWriteFullTxto indicate that it will write all relevant records for aCWalletTx. Most uses ofWriteTxbecomeWriteTxMetadata, except in migration and watch only export.AddToWalletis changed to useWriteFullTxonly for new transactions, and to do so immediately after the transaction is inserted to the wallet.CWalletTx::Updatetakes theWalletBatchnow and will write the correct records according to what is actually being updated.