wallet: drop spent parents redundant cache invalidation and notification #35786

furszy commented at 7:28 PM on July 23, 2026: member

When a transaction spends outputs owned by the wallet, the wallet invalidates the cached amounts of the transactions those outputs came from, and CommitTransaction also notifies them. Neither is needed.

Both date back to pre-#27286, when a CWalletTx also cached the available amount. Now the balance is read live from m_txos and mapTxSpends, and CWalletTx cache only its own, tx specific, incoming and outgoing amounts, and whether the tx is from the wallet. None of this changes after one of its outputs is spent. So the cache invalidation recomputes the same values, and the notification makes the GUI refresh a status that has not changed.

test: check spend doesn't change parent tx cache

CWalletTx caches its own incoming and outgoing amounts, not the
wallet available balance. Which means spending one of its outputs
do not change the cache in any way, in the same way, abandoning
the spend does not change it either.

The cache can become stalled only when a descriptor is imported.

59da62d2e2

wallet: don't mark spent parents dirty on spend

Since #27286 the CWalletTx amounts cache does not contain the available
amount anymore, only the per-tx incoming and outgoing amounts, these are
frozen except when new descriptor is imported.

This means we no longer need to mark the parent txs of a spend as dirty
and recompute their values.

We can drop the MarkDirty loop in CommitTransaction along with the
MarkInputsDirty calls in AddToWallet, SyncTransaction and
RecursiveUpdateTxState, which do the same on the block, mempool,
abandon and conflict paths.

The single mentioned import case exception is already covered by
AddWalletDescriptor, which calls MarkDirty itself. So we can
safely delete MarkInputsDirty.

fb9c53c515

wallet: drop redundant notification in CommitTransaction

When the wallet creates a spend, CommitTransaction notifies the new tx
and every wallet tx it spends from. But no other spending path does
this: SyncTransaction, for a spend arriving from the mempool or a block,
notifies only the tx it was given, and RecursiveUpdateTxState, for
abandoning one, notifies only the txs whose own state changed. The ones
whose outputs become spendable again did not change state, so they are
not notified either.

On top of that, the GUI is the only consumer, and neither of its two
subscribers needs it. The transaction table takes the CT_UPDATED and
marks the notified tx row for a status refresh, but that status comes
from the tx's own depth, maturity and abandoned flag, and from whether
it is trusted. A spend changes none of that, and the amounts are not
part of the refresh (which are actually not changed during a spend).

6b6236ca46

DrahtBot added the label Wallet on Jul 23, 2026

DrahtBot commented at 7:28 PM on July 23, 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/35786.

Reviews

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

Type	Reviewers
Concept ACK	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:

#35716 (wallet: Replace mapWallet and wtxOrdered with a boost::multi_index by achow101)
#35501 (wallet: store all witness variants of a transaction by achow101)
#34872 (wallet: fix mixed-input transaction accounting in history RPCs by w0xlt)
#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#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.

in src/wallet/test/wallet_tests.cpp:831 in 6b6236ca46

 826 | +    BOOST_CHECK(wallet->IsSpent(coins[0]));
 827 | +    BOOST_CHECK(!wallet->IsSpent(coins[1]));
 828 | +    BOOST_CHECK_EQUAL(AvailableCoinsOf(*wallet, parent_tx).size(), 1U);
 829 | +    CheckValues(*wallet, parent_tx, before);
 830 | +
 831 | +    // Now check that abandoning the spending tx does not change the parent cached values neither.

pablomartin4btc commented at 5:27 PM on July 24, 2026:

typo nit (the body in 59da62d2e28d5986d9445e583663e63c9eed4911 is correct):

    // Now check that abandoning the spending tx does not change the parent cached values either.

in src/wallet/transaction.h:229 in fb9c53c515

 227 | +    // Represents the incoming and outgoing amounts of this specific tx.
 228 | +    // Not the final wallet balance. Only importing a descriptor could make values change.
 229 |      enum AmountType { DEBIT, CREDIT, AMOUNTTYPE_ENUM_ELEMENTS };
 230 |      mutable CachableAmount m_amounts[AMOUNTTYPE_ENUM_ELEMENTS];
 231 | +    // Cached value for whether the transaction spends any inputs known to the wallet
 232 | +    mutable std::optional<bool> m_cached_from_me{std::nullopt};

pablomartin4btc commented at 5:39 PM on July 24, 2026:

nit: in fb9c53c515cabbb89ff2429a2c7cd3f9f7da2823, the comment "Only importing a descriptor could make values change" applies to m_cached_from_me too? If so, perhaps could extend it to cover both.

Option A — extend the block comment to cover both fields, drop the separate one:

// Memory only
//
// Represents the incoming/outgoing amounts of this specific tx and whether it
// spends any wallet inputs. Not the final wallet balance.
// Only importing a descriptor could make these values change.
enum AmountType { DEBIT, CREDIT, AMOUNTTYPE_ENUM_ELEMENTS };
mutable CachableAmount m_amounts[AMOUNTTYPE_ENUM_ELEMENTS];
mutable std::optional<bool> m_cached_from_me{std::nullopt};

Option B — keep the separate comment but cross-reference:

  // Cached value for whether the transaction spends any inputs known to the wallet.
  // Same caching semantics as m_amounts: frozen except on descriptor import.
  mutable std::optional<bool> m_cached_from_me{std::nullopt};

Option A seems cleaner.

in src/wallet/wallet.cpp:1118 in fb9c53c515

1114 | @@ -1115,7 +1115,6 @@ CWalletTx* CWallet::AddToWallet(CTransactionRef tx, const TxState& state, const
1115 |              // Break caches since we have changed the state
1116 |              desc_tx->MarkDirty();
1117 |              batch.WriteTx(*desc_tx);
1118 | -            MarkInputsDirty(desc_tx->tx);

pablomartin4btc commented at 6:00 PM on July 24, 2026:

any parent tx that is wallet-relevant would be its own desc_tx and get MarkDirty() in its own iteration, is that the reasoning? (apart from AddWalletDescriptor calls CWallet::MarkDirty() at the end regardless)?

in src/wallet/test/wallet_tests.cpp:771 in 59da62d2e2

 766 | +            CachedTxGetDebit(wallet, wtx, /*avoid_reuse=*/false),
 767 | +            CachedTxGetChange(wallet, wtx),
 768 | +            CachedTxIsFromMe(wallet, wtx)};
 769 | +}
 770 | +
 771 | +static void CheckValues(const CWallet& wallet, const Txid& txid, const TxValues& before) EXCLUSIVE_LOCKS_REQUIRED(wallet.cs_wallet)

pablomartin4btc commented at 6:11 PM on July 24, 2026:

minor nit: perhaps CheckValuesUnchanged or AssertValuesUnchanged make the intent immediately clear at the call site...

in src/wallet/test/wallet_tests.cpp:759 in 59da62d2e2

 752 | @@ -753,5 +753,105 @@ BOOST_FIXTURE_TEST_CASE(RemoveTxs, TestChain100Setup)
 753 |      TestUnloadWallet(std::move(wallet));
 754 |  }
 755 |  
 756 | +struct TxValues
 757 | +{
 758 | +    CAmount credit, debit, change;
 759 | +    bool from_me;

pablomartin4btc commented at 6:13 PM on July 24, 2026:

minor nit: bool is_from_me; (I understand that perhaps it could be mirroring CachedTxIsFromMe in spend.cpp - bool tx_from_me)

in src/wallet/wallet.cpp:1307 in 6b6236ca46

1302 | @@ -1304,16 +1303,6 @@ void CWallet::UpdateTrucSiblingConflicts(const CWalletTx& parent_wtx, const Txid
1303 |      }
1304 |  }
1305 |  
1306 | -void CWallet::MarkInputsDirty(const CTransactionRef& tx)

pablomartin4btc commented at 6:21 PM on July 24, 2026:

MarkInputsDirty was actually a slightly misleading name (it marks the parent transactions dirty, not the inputs themselves), so removing it entirely is the best outcome.

pablomartin4btc commented at 7:36 PM on July 24, 2026: member

Concept ACK

Checked #27286.

After the above PR, the per-transaction caches (m_amounts, m_cached_from_me) reflect the transaction's own incoming/outgoing amounts — none of these change when a child tx spends one of the parent's outputs. So MarkInputsDirty is forcing a recompute that produces the same result, and the CommitTransaction notification loop is asking the GUI to repaint a row that looks identical. Both are dead work; removing them is correct.

Left some comments and nits (none blocking).