Minor wallet issue with conflicted txs

morcos commented at 4:45 am on January 8, 2016: member

In 0.12, a wallet tx thats not in the mempool does not free up its inputs. So if a tx spends the same inputs as a conflict tx included in a new block, its inputs are explicitly marked dirty so they will be recalculated as spendable again due to having no non-conflicted spends. However if the in-block conflict is reorged out, the wallet txs inputs should now be market spent again according to the 0.12 regime, but they are not marked dirty as the tx itself is not notified it is no longer conflicted.

This has the odd effect that the cached value is incorrect and so randomly they will be spendable or not depending on whether they have been recalculated.

This is easy enough to fix by marking all inputs of all txs in SyncTransaction dirty regardless of whether AddToWalletIfInvolvingMe returns true or false. But I’m not sure the performance impact this will have, so I’m not proposing that change now.

It’s a minor issue, but I just thought I’d document that that the cached credit available in wallet txs may not always be correct.

For reference, in 0.11, whenever a tx left the mempool it’s inputs were respendable. So if a tx was removed due to a conflict in a block, any remaining inputs would be spendable again. If the in-block conflict was later reorged out, the wallet txs inputs would remain spendable unless and until the wallet tx reentered the mempool.

jonasschnelli added the label Wallet on Jan 8, 2016

jb55 commented at 8:12 pm on September 10, 2018: member

Is this still an issue?

attilaaf referenced this in commit 66843371c2 on May 25, 2019

MarcoFalke added the label good first issue on Jul 29, 2019

MarcoFalke commented at 7:30 pm on July 29, 2019: member

Would be nice to have a test case for this to know that either

the bug still exists and should be fixed
the bug is fixed and can not be reintroduced in the future

ProofOfKeags commented at 6:36 pm on August 3, 2019: none

I would like to try and take this on. First off, I’m a new contributor, I’ve been working at the application layer of bitcoin for a couple years now, but this is my first dive into the implementation details of core.

As such, if you have any suggestions about specific files to look at, or a clarification of the following terminology, it would help me immensely in being able to answer A. whether it exists still, and B. a test that can ensure it is not reintroduced.

What is a “conflicted transaction” formally?
What is a “wallet tx” and how/when would it not appear in the mempool?
What does it mean to mark a transaction as “dirty”

I will do my best to find out the answers to these questions on my own but any references to which files this logic is contained in or definitions for the terms above would jumpstart my ability to figure this out.

meshcollider referenced this in commit 5e202382a9 on Sep 5, 2019

sidhujag referenced this in commit 4c9cc729fc on Sep 10, 2019

MarcoFalke removed the label good first issue on May 7, 2020

ryanofsky commented at 5:13 pm on January 7, 2021: member

Closing this issue. It still exists but behavior has changed in various ways. There are also different fixes possible other than the one suggested. Best current description of the problem and possible fixes is bug-reorg-spent.

Below is the original issue description annotated with details of what’s changed since this was reported.

re: #7315#issue-125542715

In 0.12, a wallet tx thats not in the mempool does not free up its inputs.

Specifically, this started in #7105 with GetDepthInMainChain no longer returning -1 for transactions not in the mempool.

So if a tx spends the same inputs as a conflict tx included in a new block, its inputs are explicitly marked dirty so they will be recalculated as spendable again due to having no non-conflicted spends. However if the in-block conflict is reorged out, the wallet txs inputs should now be market spent again according to the 0.12 regime, but they are not marked dirty as the tx itself is not notified it is no longer conflicted.

This is because there’s no code in the disconnect block notification handler to update the transaction state and clear the stale conflicting block pointer during the reorg. But there’s no reason this code couldn’t be written, and ideas for implementing it are suggested in bug-stale-conflicted.

Since this issue was first reported the problem was mitigated somewhat by https://github.com/bitcoin/bitcoin/pull/16624/commits/40ede992d97df38282919693dfe851c975c3b1d8 from #16624. There are still bugs immediately after the reorg, but they are fixed if the wallet is unloaded and reloaded.

This has the odd effect that the cached value is incorrect and so randomly they will be spendable or not depending on whether they have been recalculated.

At the time this issue was opened, the problem was limited to the stale cache issue described, because the GetDepthInMainChain implementation then would check that the conflicting block pointer was part of the active chain before trusting it. However in https://github.com/bitcoin/bitcoin/pull/15931/commits/0ff03871add000f8b4d8f82aeb168eed2fc9dc5f from #15931, this check was removed, so the problem now goes beyond cached balances and can affect non-cached balances and other wallet operations.

This is easy enough to fix by marking all inputs of all txs in SyncTransaction dirty regardless of whether AddToWalletIfInvolvingMe returns true or false. But I’m not sure the performance impact this will have, so I’m not proposing that change now.

Some other fixes are possible now, and probably preferable. See bug-reorg-spent.

It’s a minor issue, but I just thought I’d document that that the cached credit available in wallet txs may not always be correct.

For reference, in 0.11, whenever a tx left the mempool it’s inputs were respendable. So if a tx was removed due to a conflict in a block, any remaining inputs would be spendable again. If the in-block conflict was later reorged out, the wallet txs inputs would remain spendable unless and until the wallet tx reentered the mempool.

ryanofsky closed this on Jan 7, 2021

DrahtBot locked this on Aug 16, 2022

Minor wallet issue with conflicted txs #7315