Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Handle "conflicted" transactions properly #3669

Merged
merged 3 commits into from Feb 14, 2014

Conversation

@gavinandresen
Copy link
Contributor

gavinandresen commented Feb 13, 2014

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

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Feb 13, 2014

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

This comment has been minimized.

Copy link
Contributor Author

gavinandresen commented Feb 13, 2014

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

@gmaxwell

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Feb 13, 2014

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Feb 13, 2014

@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

This comment has been minimized.

Copy link
Contributor Author

gavinandresen commented Feb 13, 2014

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

This comment has been minimized.

Copy link

murbard commented Feb 13, 2014

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

@christophebiocca

This comment has been minimized.

Copy link

christophebiocca commented Feb 13, 2014

@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

This comment has been minimized.

Copy link
Contributor

gmaxwell commented Feb 13, 2014

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

This comment has been minimized.

Copy link

murbard commented Feb 13, 2014

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

This comment has been minimized.

Copy link

apetersson commented Feb 14, 2014

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.

src/main.h Outdated
// 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

This comment has been minimized.

Copy link
@theuni

theuni Feb 14, 2014

Member

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

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

This comment has been minimized.

Copy link
@laanwj

laanwj Feb 14, 2014

Member

Or >=1

@apoelstra

This comment has been minimized.

Copy link
Contributor

apoelstra commented Feb 14, 2014

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

This comment has been minimized.

Copy link
Member

laanwj commented Feb 14, 2014

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

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

This comment has been minimized.

Copy link
Contributor Author

gavinandresen commented Feb 14, 2014

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

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Feb 14, 2014

ACK

@gavinandresen

This comment has been minimized.

Copy link
Contributor Author

gavinandresen commented Feb 14, 2014

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

@jgarzik

This comment has been minimized.

Copy link
Contributor

jgarzik commented Feb 14, 2014

ACK

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Feb 14, 2014

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

This comment has been minimized.

Copy link
Member

theuni commented Feb 14, 2014

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

This comment has been minimized.

Copy link
Member

laanwj commented Feb 14, 2014

@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

This comment has been minimized.

Copy link

BitcoinPullTester commented Feb 14, 2014

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 added a commit that referenced this pull request Feb 14, 2014
Handle "conflicted" transactions properly
@gavinandresen gavinandresen merged commit 05d3ded into bitcoin:master Feb 14, 2014
@gavinandresen gavinandresen deleted the gavinandresen:dead_txns branch Feb 14, 2014
@boinggg

This comment has been minimized.

Copy link

boinggg commented Apr 5, 2014

Please call them zombies

@laanwj

This comment has been minimized.

Copy link
Member

laanwj commented Apr 5, 2014

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

@zathras-crypto zathras-crypto mentioned this pull request Dec 13, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

You can’t perform that action at this time.