Handle "conflicted" transactions properly #3669

gavinandresen commented at 5:47 PM on February 13, 2014: contributor

This introduces the notion of a "conflicted" transaction-- a transaction created by the wallet that is not in either the blockchain or the memory pool, and that (therefore) is unlikely to ever be confirmed.

In the RPC interface, these transactions were previously reported as confirmations : 0 -- with this change, they are reported as confirmations : -1 and category : "conflicted".

So if a transaction is mutated or double-spent, and the mutated version ends up being mined, listtransactions will show both. Transactions can go from category conflicted to sent/received if a blockchain re-org happens.

This is not intended to be a complete solution to all the malleability issues, but is a necessary first step.

gmaxwell commented at 6:53 PM on February 13, 2014: contributor

ACK. This looks good to me, my initial concern was that the code might appear in a consensus path, but it looks like the consensus path tests directly. This works for me on my own wallet, correctly identifying an ordinary double spend I received as dead.

Maybe a slight concern that the word dead suggests a little too much finality, that and it's valid hex so /dead in my pager is not super useful. :P Think we could safely use "conflicted"? (E.g. is there a reason other than being conflicted that a wallet transaction would end up in this state— I can't think of one). I'm not too attached to such a shed painty change in any case.

gavinandresen commented at 7:01 PM on February 13, 2014: contributor

I'll change "dead" to "conflicted". I'm working on adding a conflicts [] array of txids to transaction info, so "conflicted" makes sense.

gmaxwell commented at 7:01 PM on February 13, 2014: contributor

zombie might be nice, sort of parallel with the unix usage. In any case, author's choice.

laanwj commented at 7:06 PM on February 13, 2014: member

@gavinandresen getOutputs and listCoins in walletmodel are used for coin control. In both cases the return value of GetDepthInMainChain() is passed through as nDepth argument to COutput. This information is used to compute a "priority" to be shown in the coin control dialog. It is never passed to the core. A negative value could have strange effects here.

See here for my proposed solution: https://github.com/laanwj/bitcoin/commit/2e9515e7f09093f664d802e8f42eadbe833fcd79

gavinandresen commented at 7:11 PM on February 13, 2014: contributor

Changed "dead" to "conflicted", and incorporated @laanwj 's fixes for the GUI.

Make qa/rpc-tests/ compatible with OSX

Reworked send.sh, so it works properly on my Mac (killall send.sh
doesn't work, because the process name is 'bash' not 'send.sh').
So now send.sh writes a .send.pid file, and invoking it as
send.sh -STOP (as the bitcoind -walletnotify) signals that PID.

f582eda4ed

murbard commented at 8:55 PM on February 13, 2014: none

Is there any reason not to spend zeroconf change now that this is fixed?

christophebiocca commented at 9:04 PM on February 13, 2014: none

@murbard zeroconf change can still get altered out from under you. That's going to remain true for as long as transactions are malleable.

gmaxwell commented at 9:10 PM on February 13, 2014: contributor

This just avoids being pathologically stupid once it has already been spent out from under you, e.g. spending the transaction you already know is doomed or displaying an incorrect balance.

murbard commented at 11:09 PM on February 13, 2014: none

Sure but the long run inclusion of a transaction is always probabilistic anyway (even when spending highly confirmed coins). Now that the hardwired assumption that change transactions succeed is gone, it's a matter of preference.

apetersson commented at 1:25 AM on February 14, 2014: none

if mutations are detected that would impact zeroconf change there are three things you can do: *) avoid using it *) if not possible, just spend both mutations, effectively double-spend yourself. *) ignore this advice and live with a slightly higher chance of non-confirming tx, because the parent was not confirmed.

in src/main.h:None in ab9b6d4287 outdated

 462 | @@ -461,9 +463,14 @@ class CMerkleTx : public CTransaction
 463 |  
 464 |  
 465 |      int SetMerkleBranch(const CBlock* pblock=NULL);
 466 | +
 467 | +    // Return depth of transaction in blockchain:
 468 | +    // -1 : not in blockchain, and not in memory pool (conflicted transaction)
 469 | +    // 0 : in memory pool, waiting to be included in a block
 470 | +    // >1 : this many blocks deep in the main chain

theuni commented at 5:48 AM on February 14, 2014:

docs nit for the sake of avoiding future copy/paste braino's...

s/>1/>0/ here, no?

laanwj commented at 8:40 AM on February 14, 2014:

Or >=1

apoelstra commented at 6:04 AM on February 14, 2014: contributor

Just tried this patch. It worked successfully. Thanks Gavin!

Originally I had sixteen 0-conf entries in listtransactions.

Ten of these were tx 92ad92abd8f267c29ed89107f3f7dbb6a299ed72cb5d6b73bee094b96781ca08, which conflicts with the confirmed transaction d72f8aca7159e8a21ae32d8b91ff12196eb9ea7d9f944d3b41b8d47c55d2351d. (This appeared eight times as a send and twice as a receive.) With the patch, it appears exactly eight times as 'conflicted', which is expected.

Four of them were tx 079b3a614125301114a30c0cae25709e6599e47375b6db57ac0fc86f0d684ed5, which conflicts with the confirmed transaction fa4c47a2c5bb03d82f7307bb011764a4bb471b7bdc0aa258c1c6dedcc22131dd. (This appeared twice as a send and twice as a receive.) With the patch, it appears exactly twice as 'conflicted', which is expected.

Edit Both of these conflicts were me manually double-spending to increase fees. Neither of them were malleated.

Edit2 Actually I think d72f8aca7159e8a21ae32d8b91ff12196eb9ea7d9f944d3b41b8d47c55d2351d is a malleated version of 92ad92abd8f267c29ed89107f3f7dbb6a299ed72cb5d6b73bee094b96781ca08 -- the reported output addresses and values match exactly. IIRC this was a coinjoin, so probably I signed and submitted it twice.

Edit3: As noted by Wladimir, getunconfirmedbalance includes the conflicted txes. end of edits

The other two were nonstandard transactions which conflict with each other but not any confirmed transactions. I was able to get these into my wallet by commenting out the fHave check in rpcsendrawtransaction, which I did to enable double spends. These two transactions no longer appear with the patch, which IMO is reasonable.

Every other transaction remains the same, and diff shows no change in their listtransactions entry. Again, as expected.

laanwj commented at 6:09 AM on February 14, 2014: member

Looks good, some nits

Change from dead transactions still counts toward the unconfirmed balance. Not sure what we want here, but this makes 'getunconfirmedbalance' (and the appropriate entry in UI) show high numbers in the presence of them. (I see #3671 now)
Dead transaction get the wrong icon in the UI. Will send a fix.

Edit: see https://github.com/laanwj/bitcoin/tree/2014_02_dead_tx_gui

Handle "conflicted" transactions properly

Extend CMerkleTx::GetDepthInMainChain with the concept of
a "conflicted" transaction-- a transaction generated by the wallet
that is not in the main chain or in the mempool, and, therefore,
will likely never be confirmed.

GetDepthInMainChain() now returns -1 for conflicted transactions
(0 for unconfirmed-but-in-the-mempool, and >1 for confirmed).

This makes getbalance, getbalance '*', and listunspent all agree when there are
mutated transactions in the wallet.

Before:
 listunspent: one 49BTC output
 getbalance: 96 BTC (change counted twice)
 getbalance '*': 46 BTC (spends counted twice)

After: all agree, 49 BTC available to spend.

2b72d46f42

gavinandresen commented at 4:11 PM on February 14, 2014: contributor

Fixed >=1 nit, added @laanwj 's GUI fixes as second commit.

laanwj commented at 4:30 PM on February 14, 2014: member

ACK

gavinandresen commented at 4:40 PM on February 14, 2014: contributor

@laanwj : pull-tester error is: RCC: Error in 'bitcoin.qrc': Cannot find file 'res/icons/transaction_conflicted.png'

jgarzik commented at 4:49 PM on February 14, 2014: contributor

ACK

laanwj commented at 4:49 PM on February 14, 2014: member

Huh that's strange; I do see src/qt/res/icons/transaction_conflicted.png in the list of files. (and I had no problems building and testing locally either, and there are no pngs in the 'untracked files' list in git status)

theuni commented at 4:53 PM on February 14, 2014: member

It needs to be listed in qt/Makefile.am, or it won't be added to the release tarball and 'make distcheck' will fail.

laanwj commented at 4:56 PM on February 14, 2014: member

@theuni ah I see. @gavinandresen:

diff --git a/src/qt/Makefile.am b/src/qt/Makefile.am
index 7de4729..030804d 100644
--- a/src/qt/Makefile.am
+++ b/src/qt/Makefile.am
@@ -243,6 +243,7 @@ RES_ICONS = \
   res/icons/toolbar_testnet.png \
   res/icons/transaction0.png \
   res/icons/transaction2.png \
+  res/icons/transaction_conflicted.png \
   res/icons/tx_inout.png \
   res/icons/tx_input.png \
   res/icons/tx_output.png \

qt: GUI for conflicted transactions

- Exclamation mark icon for conflicted transactions
- Show mouseover status for conflicted transactions as "conflicted"
- Don't show inactive transactions on overview page overview

9a3d936fc2

BitcoinPullTester commented at 6:52 PM on February 14, 2014: none

Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/9a3d936fc2e98b1e8234bf27e09cf7bc22811bee for binaries and test log. This test script verifies pulls every time they are updated. It, however, dies sometimes and fails to test properly. If you are waiting on a test, please check timestamps to verify that the test.log is moving at http://jenkins.bluematt.me/pull-tester/current/ Contact BlueMatt on freenode if something looks broken.

gavinandresen referenced this in commit 05d3ded072 on Feb 14, 2014

gavinandresen merged this on Feb 14, 2014

gavinandresen closed this on Feb 14, 2014

gavinandresen deleted the branch on Feb 14, 2014

boinggg commented at 9:43 AM on April 5, 2014: none

Please call them zombies

laanwj commented at 9:58 AM on April 5, 2014: member

Maybe renaming zapwallettx to shootzombietx in #3845 would make it more popular and get people to test it...

DrahtBot locked this on Sep 8, 2021