wallet: Avoid unnecessary wtxvariant rewrites #35935

achow101 commented at 11:15 PM on August 7, 2026: member

wtxvariant records should never change, so it is unnecessary for us to be rewriting all wtxvariants in a CWalletTx every time the CWalletTx's state changes. Likewise, every time there is a new variant, CWalletTx states may not change so do not need to always be unconditionally written.

This PR adds a WalletBatch::WriteTxMetadata to write just the tx record (the former behavior of WriteTx) and renames WriteTx to WriteFullTx to indicate that it will write all relevant records for a CWalletTx. Most uses of WriteTx become WriteTxMetadata, except in migration and watch only export.

AddToWallet is changed to use WriteFullTx only for new transactions, and to do so immediately after the transaction is inserted to the wallet. CWalletTx::Update takes the WalletBatch now and will write the correct records according to what is actually being updated.

DrahtBot added the label Wallet on Aug 7, 2026

DrahtBot commented at 11:15 PM on August 7, 2026: contributor

The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

Code Coverage & Benchmarks

For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/35935.

Reviews

See the guideline and AI policy for information on the review process.

Type	Reviewers
Concept ACK	pablomartin4btc
Approach ACK	jeanpablojp

If your review is incorrectly listed, please copy-paste <code></code> into the comment that the bot should ignore.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#35998 (wallet: Handle or explicitly ignore WalletBatch write failures by achow101)
#35786 (wallet: drop spent parents redundant cache invalidation and notification by furszy)
#35716 (wallet: Replace mapWallet and wtxOrdered with a boost::multi_index by achow101)
#30343 (wallet, logging: Replace WalletLogPrintf() with LogInfo() by ryanofsky)
#27865 (wallet: Track no-longer-spendable TXOs separately by achow101)

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.

achow101 force-pushed on Aug 8, 2026

DrahtBot added the label CI failed on Aug 8, 2026

DrahtBot removed the label CI failed on Aug 8, 2026

in src/wallet/transaction.h:362 in b374e88038

 358 | @@ -357,7 +359,7 @@ class CWalletTx
 359 |      // If the given transaction has a different wtxid, the transaction is stored if it has not been seen before.
 360 |      // The canonical wtxid is also updated. The tx that is confirmed becomes canonical. For unconfirmed txs,
 361 |      // those with witnesses are preferred, followed by least weight.
 362 | -    bool Update(CTransactionRef tx, const TxState& arg_state);
 363 | +    bool Update(CTransactionRef tx, const TxState& arg_state, WalletBatch& batch);

pablomartin4btc commented at 2:18 AM on August 9, 2026:

nit: as per a41607f1acf039027d0ccf9c5070768c4d4ec243 in #35930...

    bool Update(CTransactionRef tx, const TxState& new_state, WalletBatch& batch);

achow101 commented at 6:04 PM on August 12, 2026:

Done when rebasing

in src/wallet/walletdb.h:238 in b374e88038 outdated

 234 | +    // Write a CWalletTx and all variant witness txs (single tx record and multiple wtxvariant records)
 235 | +    bool WriteFullTx(const CWalletTx& wtx);
 236 |      bool EraseTx(Txid hash);
 237 | +    // Write a single witness variant of CWalletTx (single wtxvariant record)
 238 |      bool WriteWtxVariant(const Txid& txid, const CTransactionRef& tx);
 239 | +    // Write only the canonical witness tx and all of the tx metadata (single tx record)

pablomartin4btc commented at 2:21 AM on August 9, 2026:

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.

achow101 commented at 6:05 PM on August 12, 2026:

The canonical witness tx is part of the tx record. The wtxid is not written, the full tx is.

in src/wallet/wallet.cpp:1111 in b374e88038 outdated

1107 | @@ -1099,7 +1108,7 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
1108 |              desc_tx->m_state = inactive_state;
1109 |              // Break caches since we have changed the state
1110 |              desc_tx->MarkDirty();
1111 | -            batch.WriteTx(*desc_tx);
1112 | +            batch.WriteTxMetadata(*desc_tx);

pablomartin4btc commented at 2:27 AM on August 9, 2026:

nit: return value discarded here — pre-existing from WriteTx, but since we're touching this area could be worth addressing (?).

achow101 commented at 6:07 PM on August 12, 2026:

I think that's out of scope for this PR. Handling errors here is more complicated since multiple transactions are supposed to be updated at the same time.

in src/wallet/wallet.cpp:4058 in b374e88038 outdated

4054 | @@ -4051,7 +4055,7 @@ util::Result<void> CWallet::ApplyMigrationData(WalletBatch& local_wallet_batch,
4055 |              return util::Error{strprintf(_("Error: Transaction %s in wallet cannot be identified to belong to migrated wallets"), wtx->GetHash().GetHex())};
4056 |          }
4057 |          // Rewrite the transaction so that anything that may have changed about it in memory also persists to disk
4058 | -        local_wallet_batch.WriteTx(*wtx);
4059 | +        local_wallet_batch.WriteTxMetadata(*wtx);

pablomartin4btc commented at 2:29 AM on August 9, 2026:

nit: same return value discarded here...

achow101 commented at 6:10 PM on August 12, 2026:

if I need to retouch.

in src/wallet/transaction.cpp:74 in b374e88038 outdated

  74 | -    if (inserted) {
  75 | -        ret = true;
  76 | +    const auto& [tx_pair, new_variant] = m_txs.emplace(new_tx->GetWitnessHash(), std::move(new_tx));
  77 | +    if (new_variant) {
  78 | +        if (!batch.WriteWtxVariant(GetHash(), tx_pair->second)) {
  79 | +            throw std::ios_base::failure("Unable to write wtxvariant record");

pablomartin4btc commented at 2:31 AM on August 9, 2026:

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.

achow101 commented at 6:09 PM on August 12, 2026:

Yes. bool doesn't work here because false means that no update occurred, not necessarily that a failure occurred. Using an exception here is to indicate that an actual failure occurred. There is only one caller of this function anyways.

pablomartin4btc commented at 3:01 AM on August 9, 2026: member

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...

DrahtBot added the label Needs rebase on Aug 11, 2026

wallet: Add WriteTxMetadata to write just the tx record

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.

1c557a3802

wallet: Write tx to database during CWalletTx::Update

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.

5db516591c

walletdb: Rename WriteTx to WriteFullTx

Clarify in the name that this function writes the entire metadata and
all of the witness tx variants.

c22005b205

achow101 force-pushed on Aug 11, 2026

DrahtBot removed the label Needs rebase on Aug 11, 2026

jeanpablojp commented at 1:29 AM on August 16, 2026: contributor

Approach ACK

I have a question about the new flow, when the tx already exists in the wallet, the CommitTransaction callback can change metadata (m_comment, m_replaces_txid...), but if Update doesn't detect a state change or a new variant, nothing is written, but before the PR, the callback's true guaranteed the write. The UpdateWalletTxFn comment still says that returning true saves to disk, is this intentional, or is it worth writing again with WriteTxMetadata in that case?