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.

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, w0xlt
Stale ACK	Bicaru20

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:

#35975 (wallet: Fix CWalletTx malleated transaction metadata sync by achow101)
#35935 (wallet: Avoid unnecessary wtxvariant rewrites by achow101)
#35716 (wallet: Replace mapWallet and wtxOrdered with a boost::multi_index by achow101)
#35569 (Encapsulation for CTransaction by purpleKarrot)
#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.

furszy commented at 3:24 PM on July 27, 2026:

done, pushed

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

 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.

furszy commented at 2:53 PM on July 27, 2026:

The change values also have the same behaviour. We could probably move the "Only importing a descriptor could make values change" at the top, just below "Memory only" or.. encapsulate all values in a struct. But still, seems a tiny nit we could leave it as is too.

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

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)?

furszy commented at 2:57 PM on July 27, 2026:

Not sure I understand the comment. Resetting a transaction's cache when no new descriptor has been imported results on the exact same values being re-computed again.

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

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

furszy commented at 3:22 PM on July 27, 2026:

I don't think this add much value, the intent should be clear enough as is now. But could change it if you are strong on it.

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

 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)

furszy commented at 3:23 PM on July 27, 2026:

Not sure. Everything is tx based, it is inside the TxValues struct.

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

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

furszy force-pushed on Jul 27, 2026

Bicaru20 commented at 7:27 PM on July 29, 2026: contributor

ACK 0ec1a08131.

I agree MarkInputsDirty is no longer needed. As explained in the description, since #27286 CWalletTx no longer saves on cache the amount aviable to spend since this was moved to m_txos and mapTxSpends. Now, the CWalletTx it only save the m_amounts which saves the fixed incoming and outgoing amounts, and the m_cached_from_me which saves whether the tx originated from the wallet.

In bdf7c84eda, the tests checks, through various paths, that the cached values are not changed. The first test checks the CommitTransaction and AddToWallet path by commiting a transaction that spends from a coin owned by the wallet, and the RecursiveUpdateTxState path by using AbandonTransaction on a spending transaction. The other test, chechs the SyncTransaction path by using transactionAddedToMempool with a spending transaction.

DrahtBot requested review from pablomartin4btc on Jul 29, 2026

w0xlt commented at 4:56 PM on August 3, 2026: contributor

Concept ACK

DrahtBot added the label Needs rebase on Aug 4, 2026

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.

e9fbb249ed

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.

6c17200992

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

9a075fca12

furszy force-pushed on Aug 13, 2026

DrahtBot added the label CI failed on Aug 13, 2026

DrahtBot removed the label Needs rebase on Aug 13, 2026