Keep track of explicit wallet conflicts instead of using mempool

sipa commented at 8:49 pm on November 26, 2015: member

This is an alternative approach for #7067. It stores the earlier block which conflicts (directly or indirectly) with transactions. The wallet relies on being told about conflicts, so this may need rescanning to discover conflicts with historical transactions (but those aren’t reliably detectable anyway, even with other approaches).

New semantics:

A negative confirmation count -N means that there is a block (with N confirms) that directly or indirectly (via other wallet transactions) conflicts with a given transaction.
Unconfirmed coins received from self are only considered spendable when they are in the mempool.

jonasschnelli added the label Wallet on Nov 27, 2015

jonasschnelli commented at 7:48 am on November 27, 2015: contributor

Concept ACK. I think this solution performs much better then #7067. Have plans to add tests for this soon.

sipa commented at 2:42 pm on November 27, 2015: member

Seems to have a deadlock or infinite loop.

sipa force-pushed on Nov 27, 2015

sipa commented at 4:28 pm on November 27, 2015: member

Fixed 3 bugs (infinite loop, marking the accepted rather than conflicting tx, and not rebroadcasting), and added some debug output.

sipa force-pushed on Nov 27, 2015

sipa commented at 7:38 pm on November 27, 2015: member

All rpc tests pass now, including the txn-doublespend.py one which tests negative confirmations.

gmaxwell commented at 8:01 pm on November 27, 2015: contributor

I tried this on an old wallet that has a number of conflicts in it… they show as ‘confirmations": 0,’. (On master, they show as -1). “So much for tests.”

Edit: Reading the code, I think this might be intentional. Bleh. I don’t think it’s acceptable to silently turn -1 confirmed count transactions to 0s in an upgrade, even if we release note it. I think we need to either find a way around that or force a rescan.

sipa commented at 8:06 pm on November 27, 2015: member

@gmaxwell You’ll need to rescan for them to be detected.

I don’t mind adding some “fall back” logic to guess historical conflicts, but it can’t be accurate, as we have no guaranteed way of knowing about them.

gmaxwell commented at 8:10 pm on November 27, 2015: contributor

@sipa: I’d be okay with it if it was too conservative, e.g. showing an evicted historical transaction as conflicted.

sipa commented at 8:12 pm on November 27, 2015: member

@gmaxwell It can’t work even for spends of outputs that were very recently spent, along with all other outputs of those transactions.

gmaxwell commented at 8:13 pm on November 27, 2015: contributor

As an aside Concept ACK; I like tracking the conflicts explicitly and knowing the conflict depths will be nice.

sipa commented at 8:13 pm on November 27, 2015: member

Maybe we just need a separate class like “limbo” which means unconfirmed, not known to be conflicted, but also not in the mempool?

sipa force-pushed on Nov 27, 2015

sipa force-pushed on Nov 28, 2015

sipa commented at 12:22 pm on November 28, 2015: member

Added release notes about this change.

gmaxwell commented at 12:50 pm on November 28, 2015: contributor

Limbo sounds useful. Damn I love this patch, but do not love the inaccurate data on a non-rescanned wallet.

sipa force-pushed on Nov 28, 2015

sipa commented at 1:35 pm on November 28, 2015: member

Added a “trusted” field to listtransactions output for unconfirmed transactions.

sipa force-pushed on Nov 28, 2015

Keep track of explicit wallet conflicts instead of using mempool 9ac63d6d30

sipa force-pushed on Nov 29, 2015

sipa commented at 12:24 pm on November 29, 2015: member

Rebased.

sipa added this to the milestone 0.12.0 on Nov 30, 2015

sipa commented at 11:52 am on November 30, 2015: member

Tagging as 0.12: this fixes the problem that evicted wallet transaction’s inputs are automatically considered respendable.

laanwj added the label Bug on Nov 30, 2015

laanwj commented at 12:22 pm on November 30, 2015: member

Yes, should definitely be in 0.12, should be mentioned that this a fix for a bug where the wallet silently re-spends old transactions because they’ve been evicted from the mempool, or if they’ve never been in the mempool (-walletbroadcast).

Edit: The UI regards everything with <0 confirms as ‘conflicted’, it does not distinguish between different negative values. This is good, it needs no change for this.

sipa commented at 12:43 pm on November 30, 2015: member

@laanwj It may make sense to distinguish in the UI between ‘unconfirmed’ (meaning in mempool) and something else like ’limbo’ (meaning not in mempool)? RPC exposes this via the “trusted” field.

jonasschnelli commented at 3:51 pm on November 30, 2015: contributor

Tested ACK.

Extended the PR with two commits: https://github.com/jonasschnelli/bitcoin/commits/2015/11/sipa_realconflicts (mempool limit / eviction RPC test + GUI transactions details improvement).

dgenr8 commented at 7:35 pm on November 30, 2015: contributor

The network is trying to advise wallet immediately of wtx conflicts. We should not stick our fingers in our ears and sing. These should be added to the wallet rather than showing up only when confirmed.

sipa commented at 7:50 pm on November 30, 2015: member

So what do you propose?

Pass every mempool rejected transaction to the wallet? That would trivially allow dust attacking your system.
Only pass rejected transactions that are not rejected because of validity or policy? Now it’s trivial to bypass.

Yes, more information about double spends is useful, but using the P2P network for it is only useful for cases where it wasn’t intented as an attack. And those case exist too.

Now, can we please talk about the bugs that this patch is intended to address? The incorrect assumption that whatever coins aren’t spent by the mempool can safely be considered available for other transactions. That too will reduce (accidental) double spends.

dgenr8 commented at 8:23 pm on November 30, 2015: contributor

I think this change is good and don’t mean to go off topic. It does move a little toward more historical info and less real-time info, by not marking unconfirmed conflicts as visibly. There is a lot going on in those few minutes. It’s good to see the wallet providing more information and hopefully soon, more control.

Since you asked, I think the first non-clone conflict of a mempool tx should be relayed, and added to the wallet if the wallet conflicts, with some protections.

laanwj commented at 8:21 am on December 1, 2015: member

utACK

laanwj merged this on Dec 1, 2015

laanwj closed this on Dec 1, 2015

laanwj referenced this in commit 30c2d8c635 on Dec 1, 2015

Warrows referenced this in commit 9c03c2f856 on Jul 19, 2019

Warrows referenced this in commit 60b7afdbe6 on Jul 19, 2019

Warrows referenced this in commit 758d254cad on Jul 20, 2019

Warrows referenced this in commit 10be1dbc68 on Jul 28, 2019

Warrows referenced this in commit 37b159e32f on Jul 28, 2019

Warrows referenced this in commit d109d4b5b5 on Aug 4, 2019

Warrows referenced this in commit f294246820 on Sep 11, 2019

Warrows referenced this in commit 5d059b540f on Sep 23, 2019

Warrows referenced this in commit 6a50e03a3e on Oct 9, 2019

random-zebra referenced this in commit 0f1764a3db on Oct 9, 2019

CaveSpectre11 referenced this in commit a877ae7017 on Dec 14, 2019

wqking referenced this in commit b9ee50b0bb on May 24, 2020

Kokary referenced this in commit a6818fc5cb on May 26, 2020

Kokary referenced this in commit 21daaa9b16 on Nov 13, 2020

Kokary referenced this in commit bb5535cce2 on Nov 17, 2020

Kokary referenced this in commit 40f3e9370d on Nov 17, 2020

KolbyML referenced this in commit dafbb75009 on Nov 21, 2020

Kokary referenced this in commit acb8424b57 on Nov 24, 2020

Cryptarchist referenced this in commit 9c3c8529c8 on Nov 30, 2020

lyricidal referenced this in commit a6d460d163 on Jul 24, 2021

MarcoFalke locked this on Sep 8, 2021

Keep track of explicit wallet conflicts instead of using mempool #7105