From 868d041622e2f589ab4535c30ce683534b6d4f71 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 11 Nov 2014 16:41:44 -0800 Subject: [PATCH 1/5] Remove coinbase-dependant transactions during reorg. This still leaves transactions in mempool that are potentially invalid if the maturity period has been reorged out of, but at least they're not missing inputs entirely. --- src/main.cpp | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 70e3973e6..3a2c167e8 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1892,9 +1892,8 @@ bool static DisconnectTip(CValidationState &state) { // ignore validation errors in resurrected transactions list removed; CValidationState stateDummy; - if (!tx.IsCoinBase()) - if (!AcceptToMemoryPool(mempool, stateDummy, tx, false, NULL)) - mempool.remove(tx, removed, true); + if (tx.IsCoinBase() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, NULL)) + mempool.remove(tx, removed, true); } mempool.check(pcoinsTip); // Update chainActive and related variables. From 723d12c098456e7682e641076e76468a9fb0cec0 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 11 Nov 2014 20:57:54 -0800 Subject: [PATCH 2/5] Remove txn which are invalidated by coinbase maturity during reorg --- src/main.cpp | 1 + src/txmempool.cpp | 26 ++++++++++++++++++++++++++ src/txmempool.h | 1 + 3 files changed, 28 insertions(+) diff --git a/src/main.cpp b/src/main.cpp index 3a2c167e8..9e1c41ada 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1895,6 +1895,7 @@ bool static DisconnectTip(CValidationState &state) { if (tx.IsCoinBase() || !AcceptToMemoryPool(mempool, stateDummy, tx, false, NULL)) mempool.remove(tx, removed, true); } + mempool.removeCoinbaseSpends(pcoinsTip, pindexDelete->nHeight); mempool.check(pcoinsTip); // Update chainActive and related variables. UpdateTip(pindexDelete->pprev); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index e13f1cc35..ff04a8ad9 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -6,6 +6,7 @@ #include "txmempool.h" #include "clientversion.h" +#include "main.h" // for COINBASE_MATURITY #include "streams.h" #include "util.h" #include "utilmoneystr.h" @@ -453,6 +454,31 @@ void CTxMemPool::remove(const CTransaction &tx, std::list& removed } } +void CTxMemPool::removeCoinbaseSpends(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight) +{ + // Remove transactions spending a coinbase which are now immature + LOCK(cs); + list transactionsToRemove; + for (std::map::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { + const CTransaction& tx = it->second.GetTx(); + BOOST_FOREACH(const CTxIn& txin, tx.vin) { + std::map::const_iterator it2 = mapTx.find(txin.prevout.hash); + if (it2 != mapTx.end()) + continue; + const CCoins *coins = pcoins->AccessCoins(txin.prevout.hash); + if (fSanityCheck) assert(coins); + if (!coins || (coins->IsCoinBase() && nMemPoolHeight - coins->nHeight < COINBASE_MATURITY)) { + transactionsToRemove.push_back(tx); + break; + } + } + } + BOOST_FOREACH(const CTransaction& tx, transactionsToRemove) { + list removed; + remove(tx, removed, true); + } +} + void CTxMemPool::removeConflicts(const CTransaction &tx, std::list& removed) { // Remove transactions which depend on inputs of tx, recursively diff --git a/src/txmempool.h b/src/txmempool.h index d00bdd061..f671352b5 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -113,6 +113,7 @@ public: bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry); void remove(const CTransaction &tx, std::list& removed, bool fRecursive = false); + void removeCoinbaseSpends(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight); void removeConflicts(const CTransaction &tx, std::list& removed); void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight, std::list& conflicts); From b7b4318f3a65d6e55d44bff5da1091ec0b3a26d2 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 11 Nov 2014 21:06:15 -0800 Subject: [PATCH 3/5] Make CTxMemPool::check more thourough by using CheckInputs --- src/txmempool.cpp | 30 +++++++++++++++++++++++++++++- 1 file changed, 29 insertions(+), 1 deletion(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index ff04a8ad9..6ecd64636 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -6,7 +6,7 @@ #include "txmempool.h" #include "clientversion.h" -#include "main.h" // for COINBASE_MATURITY +#include "main.h" #include "streams.h" #include "util.h" #include "utilmoneystr.h" @@ -539,17 +539,22 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const uint64_t checkTotal = 0; + CCoinsViewCache mempoolDuplicate(const_cast(pcoins)); + LOCK(cs); + list waitingOnDependants; for (std::map::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { unsigned int i = 0; checkTotal += it->second.GetTxSize(); const CTransaction& tx = it->second.GetTx(); + bool fDependsWait = false; BOOST_FOREACH(const CTxIn &txin, tx.vin) { // Check that every mempool transaction's inputs refer to available coins, or other mempool tx's. std::map::const_iterator it2 = mapTx.find(txin.prevout.hash); if (it2 != mapTx.end()) { const CTransaction& tx2 = it2->second.GetTx(); assert(tx2.vout.size() > txin.prevout.n && !tx2.vout[txin.prevout.n].IsNull()); + fDependsWait = true; } else { const CCoins* coins = pcoins->AccessCoins(txin.prevout.hash); assert(coins && coins->IsAvailable(txin.prevout.n)); @@ -561,6 +566,29 @@ void CTxMemPool::check(const CCoinsViewCache *pcoins) const assert(it3->second.n == i); i++; } + if (fDependsWait) + waitingOnDependants.push_back(&it->second); + else { + CValidationState state; CTxUndo undo; + assert(CheckInputs(tx, state, mempoolDuplicate, false, 0, false, NULL)); + UpdateCoins(tx, state, mempoolDuplicate, undo, 1000000); + } + } + unsigned int stepsSinceLastRemove = 0; + while (!waitingOnDependants.empty()) { + const CTxMemPoolEntry* entry = waitingOnDependants.front(); + waitingOnDependants.pop_front(); + CValidationState state; + if (!mempoolDuplicate.HaveInputs(entry->GetTx())) { + waitingOnDependants.push_back(entry); + stepsSinceLastRemove++; + assert(stepsSinceLastRemove < waitingOnDependants.size()); + } else { + assert(CheckInputs(entry->GetTx(), state, mempoolDuplicate, false, 0, false, NULL)); + CTxUndo undo; + UpdateCoins(entry->GetTx(), state, mempoolDuplicate, undo, 1000000); + stepsSinceLastRemove = 0; + } } for (std::map::const_iterator it = mapNextTx.begin(); it != mapNextTx.end(); it++) { uint256 hash = it->second.ptx->GetHash(); From 7fd6219af7006fddc4b675236bcd43dd7a9eb553 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Tue, 18 Nov 2014 14:57:20 -0800 Subject: [PATCH 4/5] Make CTxMemPool::remove more effecient by avoiding recursion --- src/txmempool.cpp | 30 ++++++++++++++++++------------ 1 file changed, 18 insertions(+), 12 deletions(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 6ecd64636..840eb536b 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -427,26 +427,32 @@ bool CTxMemPool::addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry) } -void CTxMemPool::remove(const CTransaction &tx, std::list& removed, bool fRecursive) +void CTxMemPool::remove(const CTransaction &origTx, std::list& removed, bool fRecursive) { // Remove transaction from memory pool { LOCK(cs); - uint256 hash = tx.GetHash(); - if (fRecursive) { - for (unsigned int i = 0; i < tx.vout.size(); i++) { - std::map::iterator it = mapNextTx.find(COutPoint(hash, i)); - if (it == mapNextTx.end()) - continue; - remove(*it->second.ptx, removed, true); - } - } - if (mapTx.count(hash)) + std::deque txToRemove; + txToRemove.push_back(origTx.GetHash()); + while (!txToRemove.empty()) { - removed.push_front(tx); + uint256 hash = txToRemove.front(); + txToRemove.pop_front(); + if (!mapTx.count(hash)) + continue; + const CTransaction& tx = mapTx[hash].GetTx(); + if (fRecursive) { + for (unsigned int i = 0; i < tx.vout.size(); i++) { + std::map::iterator it = mapNextTx.find(COutPoint(hash, i)); + if (it == mapNextTx.end()) + continue; + txToRemove.push_back(it->second.ptx->GetHash()); + } + } BOOST_FOREACH(const CTxIn& txin, tx.vin) mapNextTx.erase(txin.prevout); + removed.push_back(tx); totalTxSize -= mapTx[hash].GetTxSize(); mapTx.erase(hash); nTransactionsUpdated++; From 34318d7fad7922ce56ff47908ff70e2c8a42ee56 Mon Sep 17 00:00:00 2001 From: Gavin Andresen Date: Mon, 24 Nov 2014 15:18:05 -0500 Subject: [PATCH 5/5] RPC-test based on invalidateblock for mempool coinbase spends --- qa/pull-tester/rpc-tests.sh | 1 + qa/rpc-tests/mempool_coinbase_spends.py | 94 +++++++++++++++++++++++++ 2 files changed, 95 insertions(+) create mode 100755 qa/rpc-tests/mempool_coinbase_spends.py diff --git a/qa/pull-tester/rpc-tests.sh b/qa/pull-tester/rpc-tests.sh index 071935759..d6ee00bb7 100755 --- a/qa/pull-tester/rpc-tests.sh +++ b/qa/pull-tester/rpc-tests.sh @@ -25,6 +25,7 @@ if [ "x${ENABLE_BITCOIND}${ENABLE_UTILS}${ENABLE_WALLET}" = "x111" ]; then ${BUILDDIR}/qa/rpc-tests/rest.py --srcdir "${BUILDDIR}/src" ${BUILDDIR}/qa/rpc-tests/mempool_spendcoinbase.py --srcdir "${BUILDDIR}/src" ${BUILDDIR}/qa/rpc-tests/httpbasics.py --srcdir "${BUILDDIR}/src" + ${BUILDDIR}/qa/rpc-tests/mempool_coinbase_spends.py --srcdir "${BUILDDIR}/src" #${BUILDDIR}/qa/rpc-tests/forknotify.py --srcdir "${BUILDDIR}/src" else echo "No rpc tests to run. Wallet, utils, and bitcoind must all be enabled" diff --git a/qa/rpc-tests/mempool_coinbase_spends.py b/qa/rpc-tests/mempool_coinbase_spends.py new file mode 100755 index 000000000..7b4371276 --- /dev/null +++ b/qa/rpc-tests/mempool_coinbase_spends.py @@ -0,0 +1,94 @@ +#!/usr/bin/env python2 +# Copyright (c) 2014 The Bitcoin Core developers +# Distributed under the MIT software license, see the accompanying +# file COPYING or http://www.opensource.org/licenses/mit-license.php. + +# +# Test re-org scenarios with a mempool that contains transactions +# that spend (directly or indirectly) coinbase transactions. +# + +from test_framework import BitcoinTestFramework +from bitcoinrpc.authproxy import AuthServiceProxy, JSONRPCException +from util import * +import os +import shutil + +# Create one-input, one-output, no-fee transaction: +class MempoolCoinbaseTest(BitcoinTestFramework): + + alert_filename = None # Set by setup_network + + def setup_network(self): + args = ["-checkmempool", "-debug=mempool"] + self.nodes = [] + self.nodes.append(start_node(0, self.options.tmpdir, args)) + self.nodes.append(start_node(1, self.options.tmpdir, args)) + connect_nodes(self.nodes[1], 0) + self.is_network_split = False + self.sync_all + + def create_tx(self, from_txid, to_address, amount): + inputs = [{ "txid" : from_txid, "vout" : 0}] + outputs = { to_address : amount } + rawtx = self.nodes[0].createrawtransaction(inputs, outputs) + signresult = self.nodes[0].signrawtransaction(rawtx) + assert_equal(signresult["complete"], True) + return signresult["hex"] + + def run_test(self): + start_count = self.nodes[0].getblockcount() + + # Mine three blocks. After this, nodes[0] blocks + # 101, 102, and 103 are spend-able. + new_blocks = self.nodes[1].setgenerate(True, 4) + self.sync_all() + + node0_address = self.nodes[0].getnewaddress() + node1_address = self.nodes[1].getnewaddress() + + # Three scenarios for re-orging coinbase spends in the memory pool: + # 1. Direct coinbase spend : spend_101 + # 2. Indirect (coinbase spend in chain, child in mempool) : spend_102 and spend_102_1 + # 3. Indirect (coinbase and child both in chain) : spend_103 and spend_103_1 + # Use invalidatblock to make all of the above coinbase spends invalid (immature coinbase), + # and make sure the mempool code behaves correctly. + b = [ self.nodes[0].getblockhash(n) for n in range(102, 105) ] + coinbase_txids = [ self.nodes[0].getblock(h)['tx'][0] for h in b ] + spend_101_raw = self.create_tx(coinbase_txids[0], node1_address, 50) + spend_102_raw = self.create_tx(coinbase_txids[1], node0_address, 50) + spend_103_raw = self.create_tx(coinbase_txids[2], node0_address, 50) + + # Broadcast and mine spend_102 and 103: + spend_102_id = self.nodes[0].sendrawtransaction(spend_102_raw) + spend_103_id = self.nodes[0].sendrawtransaction(spend_103_raw) + self.nodes[0].setgenerate(True, 1) + + # Create 102_1 and 103_1: + spend_102_1_raw = self.create_tx(spend_102_id, node1_address, 50) + spend_103_1_raw = self.create_tx(spend_103_id, node1_address, 50) + + # Broadcast and mine 103_1: + spend_103_1_id = self.nodes[0].sendrawtransaction(spend_103_1_raw) + self.nodes[0].setgenerate(True, 1) + + # ... now put spend_101 and spend_102_1 in memory pools: + spend_101_id = self.nodes[0].sendrawtransaction(spend_101_raw) + spend_102_1_id = self.nodes[0].sendrawtransaction(spend_102_1_raw) + + self.sync_all() + + assert_equal(set(self.nodes[0].getrawmempool()), set([ spend_101_id, spend_102_1_id ])) + + # Use invalidateblock to re-org back and make all those coinbase spends + # immature/invalid: + for node in self.nodes: + node.invalidateblock(new_blocks[0]) + + self.sync_all() + + # mempool should be empty. + assert_equal(set(self.nodes[0].getrawmempool()), set()) + +if __name__ == '__main__': + MempoolCoinbaseTest().main()