[Wallet] improve detection of conflicted transactions

jonasschnelli commented at 12:27 pm on November 20, 2015: contributor

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:

0[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

add function to check if a transaction conflicts with the mempool or utxo set 0ed44a89ae

[Wallet] improve detection of conflicted transactions 9a221ed95e

jonasschnelli commented at 12:28 pm on November 20, 2015: contributor

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 added the label Wallet on Nov 20, 2015

jonasschnelli added the label Mempool on Nov 20, 2015

Include 0 confirmation wtx in ReacceptWalletTransactions

<0 (-1) stands for conflicted transactions. Broadcast everything that is not in the chain.

4006e1e69a

jonasschnelli commented at 1:57 pm on November 20, 2015: contributor

Added two commits:

4006e1e69a932544bae8cb704e10b2f8bf49494c 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 commented at 2:20 pm on November 20, 2015: member

utACK

Don't mark utxo-set-known transactions as conflicted dbaffe8f02

jonasschnelli force-pushed on Nov 20, 2015

CheckForConflicts(): Rearrange test order 76972400c5

jonasschnelli commented at 12:03 pm on November 24, 2015: contributor

@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 commented at 12:18 pm on November 24, 2015: contributor

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

jonasschnelli commented at 2:46 pm on November 25, 2015: contributor

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

in src/wallet/wallet.cpp: in 76972400c5

2869@@ -2849,8 +2870,8 @@ int CMerkleTx::GetDepthInMainChain(const CBlockIndex* &pindexRet) const
2870 {
2871     AssertLockHeld(cs_main);
2872     int nResult = GetDepthInMainChainINTERNAL(pindexRet);

sipa commented at 3:47 pm on November 26, 2015:

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

in src/wallet/wallet.cpp: in 76972400c5

305+    for (map<uint256, CWalletTx>::iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)
306+    {
307+        CWalletTx* wtx = &((*it).second);
308+
309+        // update the conflicted cache flag
310+        if (wtx->GetDepthInMainChain() <= 0)

sipa commented at 3:50 pm on November 26, 2015:

This is recursive logic. You’re computing the value of the conflicted flag based on the output of GetDepthInMainChain, while conflicted is used to determine the output value of it. I know it doesn’t matter here because you check for 0 or -1, but it’s be much cleaner to call the INTERNAL version here, I think?

jonasschnelli commented at 4:01 pm on November 26, 2015:

This PR changes GetDepthInMainChain() that it only calls GetDepthInMainChainINTERNAL, plus it evaluates the boolean cache flag if the transaction is conflicted. Should be okay IMO.

sipa commented at 4:17 pm on November 26, 2015:

I don’t disagree that it is okay. But it’s extremely ugly (and very easily turned into an inconsistent result if the logic ever gets changed).

in src/wallet/wallet.cpp: in 76972400c5

300+
301+void CWallet::UpdateConflicted()
302+{
303+    //check for conflicted transactions
304+    LOCK2(cs_main, cs_wallet);
305+    for (map<uint256, CWalletTx>::iterator it = mapWallet.begin(); it != mapWallet.end(); ++it)

sipa commented at 3:51 pm on November 26, 2015:

Iterating over all wallet transactions on every block update? That seems very inefficient…

jonasschnelli commented at 4:02 pm on November 26, 2015:

Right.. but this only gets when fInitialSync = false. But I agree and I could not found a better solution…

in src/main.cpp: in 76972400c5

799@@ -800,6 +800,37 @@ static std::string FormatStateMessage(const CValidationState &state)
800         state.GetRejectCode());
801 }
802 
803+bool CheckForConflicts(const CTransaction &tx)
804+{
805+    LOCK(mempool.cs);

sipa commented at 3:51 pm on November 26, 2015:

This accesses pcoinsTip, so it also needs to lock cs_main.

in src/main.cpp: in 76972400c5

805+    LOCK(mempool.cs);
806+
807+    CCoinsView dummy;
808+    CCoinsViewCache view(&dummy);
809+
810+    CCoinsViewMemPool viewMemPool(pcoinsTip, mempool);

jtimon commented at 4:09 pm on November 26, 2015:

You could have a const CTxMemPool& pool parameter instead of using the global mempool (like AcceptToMemoryPool). The same goes for pcoinsTip.

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

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

jonasschnelli commented at 2:42 pm on November 28, 2015: contributor

Closing in favor of #7105

jonasschnelli closed this on Nov 28, 2015

MarcoFalke locked this on Sep 8, 2021

[Wallet] improve detection of conflicted transactions #7067