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

Merged
merged 1 commit into from Dec 1, 2015

Conversation

Projects
None yet
5 participants
@sipa
Member

sipa commented Nov 26, 2015

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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 27, 2015

Member

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

Member

jonasschnelli commented Nov 27, 2015

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 27, 2015

Member

Seems to have a deadlock or infinite loop.

Member

sipa commented Nov 27, 2015

Seems to have a deadlock or infinite loop.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 27, 2015

Member

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

Member

sipa commented Nov 27, 2015

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 27, 2015

Member

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

Member

sipa commented Nov 27, 2015

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

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 27, 2015

Member

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.

Member

gmaxwell commented Nov 27, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 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.

Member

sipa commented Nov 27, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 27, 2015

Member

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

Member

gmaxwell commented Nov 27, 2015

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 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.

Member

sipa commented Nov 27, 2015

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

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 27, 2015

Member

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

Member

gmaxwell commented Nov 27, 2015

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 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?

Member

sipa commented Nov 27, 2015

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 28, 2015

Member

Added release notes about this change.

Member

sipa commented Nov 28, 2015

Added release notes about this change.

@gmaxwell

This comment has been minimized.

Show comment
Hide comment
@gmaxwell

gmaxwell Nov 28, 2015

Member

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

Member

gmaxwell commented Nov 28, 2015

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 28, 2015

Member

Added a "trusted" field to listtransactions output for unconfirmed transactions.

Member

sipa commented Nov 28, 2015

Added a "trusted" field to listtransactions output for unconfirmed transactions.

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 29, 2015

Member

Rebased.

Member

sipa commented Nov 29, 2015

Rebased.

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

@sipa

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 30, 2015

Member

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

Member

sipa commented Nov 30, 2015

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

@laanwj laanwj added the Bug label Nov 30, 2015

@laanwj

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Nov 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.

Member

laanwj commented Nov 30, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 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.

Member

sipa commented Nov 30, 2015

@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

This comment has been minimized.

Show comment
Hide comment
@jonasschnelli

jonasschnelli Nov 30, 2015

Member

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).

bildschirmfoto 2015-11-30 um 16 51 07

bildschirmfoto 2015-11-30 um 16 45 54

Member

jonasschnelli commented Nov 30, 2015

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).

bildschirmfoto 2015-11-30 um 16 51 07

bildschirmfoto 2015-11-30 um 16 45 54

@dgenr8

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Nov 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.

Contributor

dgenr8 commented Nov 30, 2015

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

This comment has been minimized.

Show comment
Hide comment
@sipa

sipa Nov 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.

Member

sipa commented Nov 30, 2015

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

This comment has been minimized.

Show comment
Hide comment
@dgenr8

dgenr8 Nov 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.

Contributor

dgenr8 commented Nov 30, 2015

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

This comment has been minimized.

Show comment
Hide comment
@laanwj

laanwj Dec 1, 2015

Member

utACK

Member

laanwj commented Dec 1, 2015

utACK

@laanwj laanwj merged commit 9ac63d6 into bitcoin:master Dec 1, 2015

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details

laanwj added a commit that referenced this pull request Dec 1, 2015

Merge pull request #7105
9ac63d6 Keep track of explicit wallet conflicts instead of using mempool (Pieter Wuille)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment