Skip to content

[Wallet] improve detection of conflicted transactions #7067

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.

Sign up for GitHub

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

Jump to bottom
Closed
jonasschnelli wants to merge 5 commits into from
Closed

[Wallet] improve detection of conflicted transactions #7067

jonasschnelli wants to merge 5 commits into from

Conversation

@jonasschnelli
Copy link
Contributor

@jonasschnelli jonasschnelli commented Nov 20, 2015

If a wallet transaction gets evicted from the mempool (through maxmempool or similar ways of eviction like a bitcoind restart), the particular transaction was marked as conflicted (confirmations = -1) and the inputs can be spent again.

This PR enabled detection of conflicted transactions by actually checking the mempool and the utxo set. The "conflicted" flag is cached in memory and re-checked during a block tip update.

The check could be further improved:

[10:51:21]  <sipa>  or even cache the blockindex itself, and as long as the active one descends from it, don't reevaluate if true

@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Nov 20, 2015

Also includes a simple mempool limiting RPC test, PR passed this new test (confirmations = 0) as well as the txn_doublespend.py (confirmations = -1).

@jonasschnelli
Include 0 confirmation wtx in ReacceptWalletTransactions
4006e1e 
<0 (-1) stands for conflicted transactions. Broadcast everything that is not in the chain.
@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Nov 20, 2015

Added two commits:

  • 4006e1e reaccept/rebroadcast all wtx with a height of 0 (the current code only reaccepted wtx with a height of < 0 which is the indicator for conflicted wtx). IMO it shouldn't hurt if we try to AcceptToMemoryPool() an already existing wtx.
  • 88c1ed7f7a7e62032e629e701353031eeb977cab does prevent wtxs from being markt as conflicted if they already known in the utxo set

All tests are passing.

@instagibbs
Copy link
Member

instagibbs commented Nov 20, 2015

utACK

@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Nov 24, 2015

@gmaxwell: Thanks for testing!
Your absolutely right. The test order was wrong. Fixed, .. now lets see if travis can pass the walletbackup.py RPC test.

@gmaxwell
Copy link
Contributor

gmaxwell commented Nov 24, 2015

Reviewing, haven't tested. Will test when robo tests pass! :)

@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Nov 25, 2015

binaries if some likes to test without self-compiling: https://bitcoin.jonasschnelli.ch/pulls/7067/

sipa
sipa reviewed Nov 26, 2015
View reviewed changes
src/wallet/wallet.cpp
Copy link
Member

@sipa sipa Nov 26, 2015

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The conflicted check can move up here (no need to compute depth in that case).

@sipa sipa mentioned this pull request Nov 26, 2015
Merged
@sipa
Copy link
Member

sipa commented Nov 28, 2015

See alternative (but more invasive change) in #7105.

@jonasschnelli
Copy link
Contributor Author

jonasschnelli commented Nov 28, 2015

Closing in favor of #7105

@bitcoin bitcoin locked as resolved and limited conversation to collaborators Sep 8, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

No reviews

Assignees

No one assigned

Labels

Mempool Wallet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@jonasschnelli @instagibbs @gmaxwell @sipa @jtimon