Handle "conflicted" transactions properly #3669

gavinandresen · 2014-02-13T17:47:45Z

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 · 2014-02-13T18:53:48Z

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 · 2014-02-13T19:01:33Z

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

gmaxwell · 2014-02-13T19:01:39Z

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

laanwj · 2014-02-13T19:06:22Z

@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: laanwj@2e9515e

gavinandresen · 2014-02-13T19:11:18Z

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

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.

murbard · 2014-02-13T20:55:39Z

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

christophebiocca · 2014-02-13T21:04:13Z

@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 · 2014-02-13T21:10:37Z

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 · 2014-02-13T23:09:19Z

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 · 2014-02-14T01:25:41Z

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.

theuni · 2014-02-14T05:48:48Z

src/main.h

+    // Return depth of transaction in blockchain:
+    // -1 : not in blockchain, and not in memory pool (conflicted transaction)
+    // 0 : in memory pool, waiting to be included in a block
+    // >1 : this many blocks deep in the main chain


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

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

apoelstra · 2014-02-14T06:04:55Z

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 · 2014-02-14T06:09:25Z

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 Report transaction conflicts, and tentative account balance fix #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

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.

gavinandresen · 2014-02-14T16:11:57Z

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

laanwj · 2014-02-14T16:30:57Z

ACK

gavinandresen · 2014-02-14T16:40:34Z

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

jgarzik · 2014-02-14T16:49:17Z

ACK

laanwj · 2014-02-14T16:49:57Z

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 · 2014-02-14T16:53:42Z

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 · 2014-02-14T16:56:09Z

@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 \

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

BitcoinPullTester · 2014-02-14T18:52:05Z

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.

Handle "conflicted" transactions properly

boinggg · 2014-04-05T09:43:14Z

Please call them zombies

laanwj · 2014-04-05T09:58:59Z

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

gavinandresen mentioned this pull request Feb 14, 2014

Report transaction conflicts, and tentative account balance fix #3671

Merged

theuni reviewed Feb 14, 2014
View reviewed changes

laanwj mentioned this pull request Feb 14, 2014

Make qa/rpc-tests/ compatible with OSX #3667

Merged

qt: GUI for conflicted transactions

9a3d936

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

gavinandresen added a commit that referenced this pull request Feb 14, 2014

Merge pull request #3669 from gavinandresen/dead_txns

05d3ded

Handle "conflicted" transactions properly

gavinandresen merged commit 05d3ded into bitcoin:master Feb 14, 2014

gavinandresen deleted the dead_txns branch February 14, 2014 19:53

zathras-crypto mentioned this pull request Dec 13, 2014

0.0.9 UI mastercoin-MSC/mastercore#243

Merged

bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle "conflicted" transactions properly #3669

Handle "conflicted" transactions properly #3669

gavinandresen commented Feb 13, 2014

gmaxwell commented Feb 13, 2014

gavinandresen commented Feb 13, 2014

gmaxwell commented Feb 13, 2014

laanwj commented Feb 13, 2014

gavinandresen commented Feb 13, 2014

murbard commented Feb 13, 2014

christophebiocca commented Feb 13, 2014

gmaxwell commented Feb 13, 2014

murbard commented Feb 13, 2014

apetersson commented Feb 14, 2014

theuni Feb 14, 2014

laanwj Feb 14, 2014

apoelstra commented Feb 14, 2014

laanwj commented Feb 14, 2014

gavinandresen commented Feb 14, 2014

laanwj commented Feb 14, 2014

gavinandresen commented Feb 14, 2014

jgarzik commented Feb 14, 2014

laanwj commented Feb 14, 2014

theuni commented Feb 14, 2014

laanwj commented Feb 14, 2014

BitcoinPullTester commented Feb 14, 2014

boinggg commented Apr 5, 2014

laanwj commented Apr 5, 2014

Handle "conflicted" transactions properly #3669

Handle "conflicted" transactions properly #3669

Conversation

gavinandresen commented Feb 13, 2014

gmaxwell commented Feb 13, 2014

gavinandresen commented Feb 13, 2014

gmaxwell commented Feb 13, 2014

laanwj commented Feb 13, 2014

gavinandresen commented Feb 13, 2014

murbard commented Feb 13, 2014

christophebiocca commented Feb 13, 2014

gmaxwell commented Feb 13, 2014

murbard commented Feb 13, 2014

apetersson commented Feb 14, 2014

theuni Feb 14, 2014

Choose a reason for hiding this comment

laanwj Feb 14, 2014

Choose a reason for hiding this comment

apoelstra commented Feb 14, 2014

laanwj commented Feb 14, 2014

gavinandresen commented Feb 14, 2014

laanwj commented Feb 14, 2014

gavinandresen commented Feb 14, 2014

jgarzik commented Feb 14, 2014

laanwj commented Feb 14, 2014

theuni commented Feb 14, 2014

laanwj commented Feb 14, 2014

BitcoinPullTester commented Feb 14, 2014

boinggg commented Apr 5, 2014

laanwj commented Apr 5, 2014