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
ACK	polespinasa
Concept ACK	rkrux
Stale ACK	jeanpablojp, pablomartin4btc

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)
#34909 (wallet, refactor: modularise wallet by extracting out legacy wallet migration by rkrux)
#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.

achow101 commented at 10:09 PM on August 21, 2026:

Leaving this for #35998

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.

achow101 commented at 10:09 PM on August 21, 2026:

Actually, leaving this for #35998

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

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?

achow101 commented at 10:17 PM on August 21, 2026: member

the CommitTransaction callback can change metadata (m_comment, m_replaces_txid...),

CommitTransaction is never called on transactions that already exist.

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?

Done to make it consistent, but we should never be using update_wtx for transactions that already exist.

achow101 force-pushed on Aug 21, 2026

jeanpablojp commented at 10:01 AM on August 25, 2026: contributor

ACK 97feae576ac35cfb7827e6761450c5b06573c07a

DrahtBot requested review from pablomartin4btc on Aug 25, 2026

pablomartin4btc commented at 3:17 AM on August 26, 2026: member

ACK 97feae576ac35cfb7827e6761450c5b06573c07a

nit: the UpdateWalletTxFn doc comment in wallet.h ("@return true if wtx is changed and needs to be saved to disk") is still accurate but could spell out that for pre-existing txs this specifically forces a WriteTxMetadata write via metadata_changed — and it would've also preempted the question above.

in src/wallet/transaction.cpp:79 in e8f3fc61a8

  79 | +            throw std::ios_base::failure("Unable to write wtxvariant record");
  80 | +        }
  81 |      }
  82 |      const auto& [wtxid, tx] = *tx_pair;
  83 |  
  84 | +    bool state_changed = metadata_changed;

polespinasa commented at 8:34 AM on August 26, 2026:

in e8f3fc6 wallet: Write tx to database during CWalletTx::Update

imo this variable name is a bit missleading, I would use record_changed or tx_record_chaned.

achow101 commented at 5:37 PM on August 26, 2026:

I changed this to use metadata_changed instead of initializing a new variable with it.

polespinasa commented at 10:13 AM on August 26, 2026: member

ACK 97feae576ac35cfb7827e6761450c5b06573c07a

rkrux commented at 11:00 AM on August 26, 2026: contributor

Concept ACK 97feae5

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.

9a0628d932

walletdb: Rename WriteTx to WriteFullTx

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

1548d15de6

achow101 force-pushed on Aug 26, 2026

polespinasa commented at 5:41 PM on August 26, 2026: member

re-ACK 1548d15de61886e0b8e67801146e9c168d9d4a66

Since last ACK just avoided an unecessary variable declaration.

DrahtBot requested review from rkrux on Aug 26, 2026

DrahtBot requested review from jeanpablojp on Aug 26, 2026

DrahtBot requested review from pablomartin4btc on Aug 26, 2026