wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet

achow101 commented at 10:28 pm on August 28, 2025: member

One of the ways that the wallet would determine if a transaction belonged to the wallet was by checking if the total amount being spent by a transaction from outputs known to the wallet was greater than 0. This has worked fine until recently since there was no reason for 0-value outputs to be created. However, with ephemeral dust and P2A, it is possible to create standard 0-value outputs, and the wallet was not correctly identifying the spends of such outputs. This PR updates IsFromMe to only check whether the wallet knows any of the inputs, rather than checking the debit amount of a transaction.

Additionally, a new functional test is added to test for this case, as well as a few other anchor output related scenarios. This also revealed a bug in sendall which would cause an assertion error when trying to spend all of the outputs in a wallet that has anchor outputs.

Fixes #33265

DrahtBot added the label Wallet on Aug 28, 2025

DrahtBot commented at 10:28 pm on August 28, 2025: 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/33268.

Reviews

See the guideline for information on the review process.

Type	Reviewers
Stale ACK	jsarenik

If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#33034 (wallet: Store transactions in a separate sqlite table by achow101)
#32763 (wallet: Replace CWalletTx::mapValue and vOrderForm with explicit class members by achow101)
#31615 (validation: ensure assumevalid is always used during reindex by Eunovo)
#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 28, 2025

DrahtBot added the label CI failed on Aug 28, 2025

DrahtBot commented at 10:52 pm on August 28, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/49140364057 LLM reason (✨ experimental): Lint failure (py_lint) due to an unused import in test_wallet_anchor.py (CTransaction) flagged by ruff.

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Aug 29, 2025

jsarenik commented at 8:00 am on August 29, 2025: none

Tested ACK 7d1c3ce7f0f0. I have compiled it and ran the script from issue #33265 with no output other than the current UTC date. Works for me.

Thanks!

in src/wallet/wallet.cpp:1643 in e3a367156d outdated

1633@@ -1634,7 +1634,11 @@ bool CWallet::IsMine(const COutPoint& outpoint) const
1634 
1635 bool CWallet::IsFromMe(const CTransaction& tx) const
1636 {
1637-    return (GetDebit(tx) > 0);
1638+    LOCK(cs_wallet);

instagibbs commented at 11:46 am on August 29, 2025:

which is necessary for marking anchor outputs as spent.

To be pedantic, it’s not about PayToAnchor(P2A), which can be any value, but any 0-value outputs, even if they weren’t ephemeral (say a miner mined it to dust your wallet).

There are other legitimate 0-value output use-cases such as connector outputs, so let’s just call it 0-value dust?

achow101 commented at 7:55 pm on August 29, 2025:

Updated the commit message.

jsarenik commented at 12:09 pm on August 29, 2025: none

After it gets merged into master branch, I would recommend backporting it to current stable release branch which already contains the ephemeral outputs support (29.x).

in test/functional/wallet_anchor.py:33 in 7d1c3ce7f0 outdated

28+        self.num_nodes = 1
29+
30+    def skip_test_if_missing_module(self):
31+        self.skip_if_no_wallet()
32+
33+    def test_anchor_listunspent(self):

instagibbs commented at 1:03 pm on August 29, 2025:

this subtest is covering 0-value outputs, not anchors per se. I’d rather keep the tests logically separated to not confused the concepts more than they already are :)

achow101 commented at 7:55 pm on August 29, 2025:

Clarified the test name

instagibbs commented at 1:04 pm on August 29, 2025: member

@jsarenik technically this issue could happen if miners mine 0-value outputs of any kind, but given that it’s now relay-possible, seems like a good idea to backport

fanquake added the label Needs backport (29.x) on Aug 29, 2025

achow101 force-pushed on Aug 29, 2025

jsarenik commented at 6:53 pm on August 30, 2025: none

Tested ACK de98ffb

furszy commented at 5:42 pm on August 31, 2025: member

q: is IsFromMe() method still relevant? It seems we could directly call IsMine() on the inputs, which is also cached now and should be slightly safer for migration.

achow101 commented at 6:31 pm on August 31, 2025: member

is IsFromMe() method still relevant?

Yes, transactions that we make are still treated specially.

furszy commented at 6:59 pm on August 31, 2025: member

is IsFromMe() method still relevant?

Yes, transactions that we make are still treated specially.

Where?. IsFromMe() seems to be only ever called in AddToWalletIfInvolvingMe() and ApplyMigrationData() next to the IsMine() call. In both places we effectively treat it as is_mine = IsMine(outputs) || IsFromMe(inputs).

achow101 commented at 7:30 pm on August 31, 2025: member

Where?

Hmm, I forgot that CachedTxIsFromMe does not use IsFromMe. Might need to change that in this PR as well.

achow101 force-pushed on Sep 1, 2025

achow101 commented at 10:50 pm on September 1, 2025: member

Hmm, I forgot that CachedTxIsFromMe does not use IsFromMe. Might need to change that in this PR as well.

I’ve pulled in 2 commits from #27865 which drop CachedTxIsFromMe and replace it with an IsFromMe check which is stored on disk.

In both places we effectively treat it as is_mine = IsMine(outputs) || IsFromMe(inputs).

While that’s true, moving IsFromMe into IsMine does change IsMine’s semantics in other places, so I’m not sure that that would necessarily be correct. I think it would be better to explore that in a different PR.

test: Test wallet 'from me' status change

If something is imported into the wallet, it can change the 'from me'
status of a transaction. This status is only visible through
gettransaction's "fee" field which is only shown for transactions that
are 'from me'.

009b273df8

wallet: Determine IsFromMe by checking for TXOs of inputs

Instead of checking whether the total amount of inputs known by the
wallet is greater than 0, we should be checking for whether the input is
known by the wallet. This enables us to determine whether a transaction
spends an of output with an amount of 0, which is necessary for marking
0-value dust outputs as spent.

4430e5c25f

wallet: Throw an error in sendall if the tx size cannot be calculated 101c9cb6e5

test: Add a test for anchor outputs in the wallet 1cd57c67b1

wallet: Add m_from_me to cache "from me" status

m_from_me is used to track whether a transaction is "from me", i.e. has
any inputs which belong to the wallet.

6d926f150a

wallet: Replace CachedTxIsFromMe with direct m_from_me lookup

Instead of looking at the cached amounts or searching every input of a
transaction each time we want to determine whether it is "from me", use
m_from_me  which stores this value for us.

e201a1e5a4

achow101 force-pushed on Sep 1, 2025

DrahtBot added the label CI failed on Sep 1, 2025

DrahtBot commented at 11:06 pm on September 1, 2025: contributor

🚧 At least one of the CI tasks failed. Task lint: https://github.com/bitcoin/bitcoin/runs/49358785571 LLM reason (✨ experimental): Lint Python code failure: unused variable fee in test_wallet_listtransactions.py (F841).

Try to run the tests locally, according to the documentation. However, a CI failure may still happen due to a number of reasons, for example:

Possibly due to a silent merge conflict (the changes in this pull request being incompatible with the current code in the target branch). If so, make sure to rebase on the latest commit of the target branch.
A sanitizer issue, which can only be found by compiling with the sanitizer and running the affected test.
An intermittent issue.

Leave a comment here, if you need help tracking down a confusing failure.

DrahtBot removed the label CI failed on Sep 2, 2025

wallet: Identify transactions spending 0-value outputs, and add tests for anchor outputs in a wallet #33268

Code Coverage & Benchmarks

Reviews

Conflicts