From 9ac63d6d3056600c1b784da0e6b98f679fa98b6e Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 26 Nov 2015 18:42:07 +0100 Subject: [PATCH] Keep track of explicit wallet conflicts instead of using mempool --- doc/release-notes.md | 18 ++++++ qa/rpc-tests/txn_clone.py | 2 +- qa/rpc-tests/txn_doublespend.py | 9 +-- src/wallet/rpcwallet.cpp | 6 +- src/wallet/wallet.cpp | 97 ++++++++++++++++++++++++++++----- src/wallet/wallet.h | 18 ++++-- 6 files changed, 124 insertions(+), 26 deletions(-) diff --git a/doc/release-notes.md b/doc/release-notes.md index f7958381b..96c830d17 100644 --- a/doc/release-notes.md +++ b/doc/release-notes.md @@ -215,6 +215,24 @@ of just announcing the hash. In a reorganization, all new headers are sent, instead of just the new tip. This can often prevent an extra roundtrip before the actual block is downloaded. +Negative confirmations and conflict detection +--------------------------------------------- + +The wallet will now report a negative number for confirmations that indicates +how deep in the block chain the conflict is found. For example, if a transaction +A has 5 confirmations and spends the same input as a wallet transaction B, B +will be reported as having -5 confirmations. If another wallet transaction C +spends an output from B, it will also be reported as having -5 confirmations. +To detect conflicts with historical transactions in the chain a one-time +`-rescan` may be needed. + +Unlike earlier versions, unconfirmed but non-conflicting transactions will never +get a negative confirmation count. They are not treated as spendable unless +they're coming from ourself (change) and accepted into our local mempool, +however. The new "trusted" field in the `listtransactions` RPC output +indicates whether outputs of an unconfirmed transaction are considered +spendable. + 0.12.0 Change log ================= diff --git a/qa/rpc-tests/txn_clone.py b/qa/rpc-tests/txn_clone.py index e8ced0e5b..b1f603a19 100755 --- a/qa/rpc-tests/txn_clone.py +++ b/qa/rpc-tests/txn_clone.py @@ -136,7 +136,7 @@ class TxnMallTest(BitcoinTestFramework): tx2 = self.nodes[0].gettransaction(txid2) # Verify expected confirmations - assert_equal(tx1["confirmations"], -1) + assert_equal(tx1["confirmations"], -2) assert_equal(tx1_clone["confirmations"], 2) assert_equal(tx2["confirmations"], 1) diff --git a/qa/rpc-tests/txn_doublespend.py b/qa/rpc-tests/txn_doublespend.py index 36081127b..d4665b3d4 100755 --- a/qa/rpc-tests/txn_doublespend.py +++ b/qa/rpc-tests/txn_doublespend.py @@ -99,7 +99,7 @@ class TxnMallTest(BitcoinTestFramework): # Now give doublespend and its parents to miner: self.nodes[2].sendrawtransaction(fund_foo_tx["hex"]) self.nodes[2].sendrawtransaction(fund_bar_tx["hex"]) - self.nodes[2].sendrawtransaction(doublespend["hex"]) + doublespend_txid = self.nodes[2].sendrawtransaction(doublespend["hex"]) # ... mine a block... self.nodes[2].generate(1) @@ -107,14 +107,15 @@ class TxnMallTest(BitcoinTestFramework): connect_nodes(self.nodes[1], 2) self.nodes[2].generate(1) # Mine another block to make sure we sync sync_blocks(self.nodes) + assert_equal(self.nodes[0].gettransaction(doublespend_txid)["confirmations"], 2) # Re-fetch transaction info: tx1 = self.nodes[0].gettransaction(txid1) tx2 = self.nodes[0].gettransaction(txid2) - + # Both transactions should be conflicted - assert_equal(tx1["confirmations"], -1) - assert_equal(tx2["confirmations"], -1) + assert_equal(tx1["confirmations"], -2) + assert_equal(tx2["confirmations"], -2) # Node0's total balance should be starting balance, plus 100BTC for # two more matured blocks, minus 1240 for the double-spend, plus fees (which are diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index b6eaca80b..a4ab2248a 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -65,6 +65,8 @@ void WalletTxToJSON(const CWalletTx& wtx, UniValue& entry) entry.push_back(Pair("blockhash", wtx.hashBlock.GetHex())); entry.push_back(Pair("blockindex", wtx.nIndex)); entry.push_back(Pair("blocktime", mapBlockIndex[wtx.hashBlock]->GetBlockTime())); + } else { + entry.push_back(Pair("trusted", wtx.IsTrusted())); } uint256 hash = wtx.GetHash(); entry.push_back(Pair("txid", hash.GetHex())); @@ -1421,7 +1423,9 @@ UniValue listtransactions(const UniValue& params, bool fHelp) " \"fee\": x.xxx, (numeric) The amount of the fee in " + CURRENCY_UNIT + ". This is negative and only available for the \n" " 'send' category of transactions.\n" " \"confirmations\": n, (numeric) The number of confirmations for the transaction. Available for 'send' and \n" - " 'receive' category of transactions.\n" + " 'receive' category of transactions. Negative confirmations indicate the\n" + " transation conflicts with the block chain\n" + " \"trusted\": xxx (bool) Whether we consider the outputs of this unconfirmed transaction safe to spend.\n" " \"blockhash\": \"hashvalue\", (string) The block hash containing the transaction. Available for 'send' and 'receive'\n" " category of transactions.\n" " \"blockindex\": n, (numeric) The block index containing the transaction. Available for 'send' and 'receive'\n" diff --git a/src/wallet/wallet.cpp b/src/wallet/wallet.cpp index 69b163ebc..57a41ec66 100644 --- a/src/wallet/wallet.cpp +++ b/src/wallet/wallet.cpp @@ -608,6 +608,14 @@ bool CWallet::AddToWallet(const CWalletTx& wtxIn, bool fFromLoadWallet, CWalletD wtx.BindWallet(this); wtxOrdered.insert(make_pair(wtx.nOrderPos, TxPair(&wtx, (CAccountingEntry*)0))); AddToSpends(hash); + BOOST_FOREACH(const CTxIn& txin, wtx.vin) { + if (mapWallet.count(txin.prevout.hash)) { + CWalletTx& prevtx = mapWallet[txin.prevout.hash]; + if (prevtx.nIndex == -1 && !prevtx.hashBlock.IsNull()) { + MarkConflicted(prevtx.hashBlock, wtx.GetHash()); + } + } + } } else { @@ -727,6 +735,20 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pbl { { AssertLockHeld(cs_wallet); + + if (pblock) { + BOOST_FOREACH(const CTxIn& txin, tx.vin) { + std::pair range = mapTxSpends.equal_range(txin.prevout); + while (range.first != range.second) { + if (range.first->second != tx.GetHash()) { + LogPrintf("Transaction %s (in block %s) conflicts with wallet transaction %s (both spend %s:%i)\n", tx.GetHash().ToString(), pblock->GetHash().ToString(), range.first->second.ToString(), range.first->first.hash.ToString(), range.first->first.n); + MarkConflicted(pblock->GetHash(), range.first->second); + } + range.first++; + } + } + } + bool fExisted = mapWallet.count(tx.GetHash()) != 0; if (fExisted && !fUpdate) return false; if (fExisted || IsMine(tx) || IsFromMe(tx)) @@ -747,9 +769,57 @@ bool CWallet::AddToWalletIfInvolvingMe(const CTransaction& tx, const CBlock* pbl return false; } +void CWallet::MarkConflicted(const uint256& hashBlock, const uint256& hashTx) +{ + LOCK2(cs_main, cs_wallet); + + CBlockIndex* pindex; + assert(mapBlockIndex.count(hashBlock)); + pindex = mapBlockIndex[hashBlock]; + int conflictconfirms = 0; + if (chainActive.Contains(pindex)) { + conflictconfirms = -(chainActive.Height() - pindex->nHeight + 1); + } + assert(conflictconfirms < 0); + + // Do not flush the wallet here for performance reasons + CWalletDB walletdb(strWalletFile, "r+", false); + + std::deque todo; + std::set done; + + todo.push_back(hashTx); + + while (!todo.empty()) { + uint256 now = todo.front(); + todo.pop_front(); + done.insert(now); + assert(mapWallet.count(now)); + CWalletTx& wtx = mapWallet[now]; + int currentconfirm = wtx.GetDepthInMainChain(); + if (conflictconfirms < currentconfirm) { + // Block is 'more conflicted' than current confirm; update. + // Mark transaction as conflicted with this block. + wtx.nIndex = -1; + wtx.hashBlock = hashBlock; + wtx.MarkDirty(); + wtx.WriteToDisk(&walletdb); + // Iterate over all its outputs, and mark transactions in the wallet that spend them conflicted too + TxSpends::const_iterator iter = mapTxSpends.lower_bound(COutPoint(now, 0)); + while (iter != mapTxSpends.end() && iter->first.hash == now) { + if (!done.count(iter->second)) { + todo.push_back(iter->second); + } + iter++; + } + } + } +} + void CWallet::SyncTransaction(const CTransaction& tx, const CBlock* pblock) { LOCK2(cs_main, cs_wallet); + if (!AddToWalletIfInvolvingMe(tx, pblock, true)) return; // Not one of ours @@ -1089,7 +1159,7 @@ void CWallet::ReacceptWalletTransactions() int nDepth = wtx.GetDepthInMainChain(); - if (!wtx.IsCoinBase() && nDepth < 0) { + if (!wtx.IsCoinBase() && nDepth == 0) { mapSorted.insert(std::make_pair(wtx.nOrderPos, &wtx)); } } @@ -1303,6 +1373,14 @@ bool CWalletTx::IsTrusted() const if (!bSpendZeroConfChange || !IsFromMe(ISMINE_ALL)) // using wtx's cached debit return false; + // Don't trust unconfirmed transactions from us unless they are in the mempool. + { + LOCK(mempool.cs); + if (!mempool.exists(GetHash())) { + return false; + } + } + // Trusted if all inputs are from us and are in the mempool: BOOST_FOREACH(const CTxIn& txin, vin) { @@ -1879,6 +1957,7 @@ bool CWallet::CreateTransaction(const vector& vecSend, CWalletTx& wt //a chance at a free transaction. //But mempool inputs might still be in the mempool, so their age stays 0 int age = pcoin.first->GetDepthInMainChain(); + assert(age >= 0); if (age != 0) age += 1; dPriority += (double)nCredit * age; @@ -2814,9 +2893,9 @@ int CMerkleTx::SetMerkleBranch(const CBlock& block) return chainActive.Height() - pindex->nHeight + 1; } -int CMerkleTx::GetDepthInMainChainINTERNAL(const CBlockIndex* &pindexRet) const +int CMerkleTx::GetDepthInMainChain(const CBlockIndex* &pindexRet) const { - if (hashBlock.IsNull() || nIndex == -1) + if (hashBlock.IsNull()) return 0; AssertLockHeld(cs_main); @@ -2829,17 +2908,7 @@ int CMerkleTx::GetDepthInMainChainINTERNAL(const CBlockIndex* &pindexRet) const return 0; pindexRet = pindex; - return chainActive.Height() - pindex->nHeight + 1; -} - -int CMerkleTx::GetDepthInMainChain(const CBlockIndex* &pindexRet) const -{ - AssertLockHeld(cs_main); - int nResult = GetDepthInMainChainINTERNAL(pindexRet); - if (nResult == 0 && !mempool.exists(GetHash())) - return -1; // Not in chain, not in mempool - - return nResult; + return ((nIndex == -1) ? (-1) : 1) * (chainActive.Height() - pindex->nHeight + 1); } int CMerkleTx::GetBlocksToMaturity() const diff --git a/src/wallet/wallet.h b/src/wallet/wallet.h index a4199488f..bc6e40ab9 100644 --- a/src/wallet/wallet.h +++ b/src/wallet/wallet.h @@ -156,11 +156,14 @@ struct COutputEntry /** A transaction with a merkle branch linking it to the block chain. */ class CMerkleTx : public CTransaction { -private: - int GetDepthInMainChainINTERNAL(const CBlockIndex* &pindexRet) const; - public: uint256 hashBlock; + + /* An nIndex == -1 means that hashBlock (in nonzero) refers to the earliest + * block in the chain we know this or any in-wallet dependency conflicts + * with. Older clients interpret nIndex == -1 as unconfirmed for backward + * compatibility. + */ int nIndex; CMerkleTx() @@ -193,16 +196,15 @@ public: int SetMerkleBranch(const CBlock& block); - /** * Return depth of transaction in blockchain: - * -1 : not in blockchain, and not in memory pool (conflicted transaction) + * <0 : conflicts with a transaction this deep in the blockchain * 0 : in memory pool, waiting to be included in a block * >=1 : this many blocks deep in the main chain */ int GetDepthInMainChain(const CBlockIndex* &pindexRet) const; int GetDepthInMainChain() const { const CBlockIndex *pindexRet; return GetDepthInMainChain(pindexRet); } - bool IsInMainChain() const { const CBlockIndex *pindexRet; return GetDepthInMainChainINTERNAL(pindexRet) > 0; } + bool IsInMainChain() const { const CBlockIndex *pindexRet; return GetDepthInMainChain(pindexRet) > 0; } int GetBlocksToMaturity() const; bool AcceptToMemoryPool(bool fLimitFree=true, bool fRejectAbsurdFee=true); }; @@ -481,6 +483,10 @@ private: void AddToSpends(const COutPoint& outpoint, const uint256& wtxid); void AddToSpends(const uint256& wtxid); + /* Mark a transaction (and its in-wallet descendants) as conflicting with a particular block. */ + void MarkConflicted(const uint256& hashBlock, const uint256& hashTx); + + void SyncMetaData(std::pair); public: