From 0c9959a3081328f1a8f4d9a5d27d1559b6ede561 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 26 Aug 2015 18:15:04 -0700 Subject: [PATCH 1/8] Add failing test checking timelocked-txn removal during reorg --- qa/pull-tester/rpc-tests.py | 2 +- ...ol_coinbase_spends.py => mempool_reorg.py} | 26 ++++++++++++++----- 2 files changed, 21 insertions(+), 7 deletions(-) rename qa/rpc-tests/{mempool_coinbase_spends.py => mempool_reorg.py} (73%) diff --git a/qa/pull-tester/rpc-tests.py b/qa/pull-tester/rpc-tests.py index 5004b09c1..095572417 100755 --- a/qa/pull-tester/rpc-tests.py +++ b/qa/pull-tester/rpc-tests.py @@ -77,7 +77,7 @@ testScripts = [ 'rawtransactions.py', 'rest.py', 'mempool_spendcoinbase.py', - 'mempool_coinbase_spends.py', + 'mempool_reorg.py', 'httpbasics.py', 'zapwallettxes.py', 'proxy_test.py', diff --git a/qa/rpc-tests/mempool_coinbase_spends.py b/qa/rpc-tests/mempool_reorg.py similarity index 73% rename from qa/rpc-tests/mempool_coinbase_spends.py rename to qa/rpc-tests/mempool_reorg.py index c64a15b9f..fdbaf689a 100755 --- a/qa/rpc-tests/mempool_coinbase_spends.py +++ b/qa/rpc-tests/mempool_reorg.py @@ -52,16 +52,25 @@ class MempoolCoinbaseTest(BitcoinTestFramework): # 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) ] + b = [ self.nodes[0].getblockhash(n) for n in range(101, 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) + spend_101_raw = self.create_tx(coinbase_txids[1], node1_address, 50) + spend_102_raw = self.create_tx(coinbase_txids[2], node0_address, 50) + spend_103_raw = self.create_tx(coinbase_txids[3], node0_address, 50) + + # Create a block-height-locked transaction which will be invalid after reorg + timelock_tx = self.nodes[0].createrawtransaction([{"txid": coinbase_txids[0], "vout": 0}], {node0_address: 50}) + # Set the time lock + timelock_tx = timelock_tx.replace("ffffffff", "11111111", 1) + timelock_tx = timelock_tx[:-8] + hex(self.nodes[0].getblockcount() + 2)[2:] + "000000" + timelock_tx = self.nodes[0].signrawtransaction(timelock_tx)["hex"] + assert_raises(JSONRPCException, self.nodes[0].sendrawtransaction, timelock_tx) # 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].generate(1) + assert_raises(JSONRPCException, self.nodes[0].sendrawtransaction, timelock_tx) # Create 102_1 and 103_1: spend_102_1_raw = self.create_tx(spend_102_id, node1_address, 50) @@ -69,7 +78,8 @@ class MempoolCoinbaseTest(BitcoinTestFramework): # Broadcast and mine 103_1: spend_103_1_id = self.nodes[0].sendrawtransaction(spend_103_1_raw) - self.nodes[0].generate(1) + last_block = self.nodes[0].generate(1) + timelock_tx_id = self.nodes[0].sendrawtransaction(timelock_tx) # ... now put spend_101 and spend_102_1 in memory pools: spend_101_id = self.nodes[0].sendrawtransaction(spend_101_raw) @@ -77,7 +87,11 @@ class MempoolCoinbaseTest(BitcoinTestFramework): self.sync_all() - assert_equal(set(self.nodes[0].getrawmempool()), set([ spend_101_id, spend_102_1_id ])) + assert_equal(set(self.nodes[0].getrawmempool()), set([ spend_101_id, spend_102_1_id, timelock_tx_id ])) + + for node in self.nodes: + node.invalidateblock(last_block[0]) + assert_equal(set(self.nodes[0].getrawmempool()), set([ spend_101_id, spend_102_1_id, spend_103_1_id ])) # Use invalidateblock to re-org back and make all those coinbase spends # immature/invalid: From 9b060e5cfb0d185b553b21ae19d390f81e83bd4d Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 26 Aug 2015 18:58:17 -0700 Subject: [PATCH 2/8] Fix removal of time-locked transactions during reorg --- src/main.cpp | 2 +- src/txmempool.cpp | 25 +++++++++++++++---------- src/txmempool.h | 2 +- 3 files changed, 17 insertions(+), 12 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 55b051734..3422c56cf 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2350,7 +2350,7 @@ bool static DisconnectTip(CValidationState& state, const Consensus::Params& cons // UpdateTransactionsFromBlock finds descendants of any transactions in this // block that were added back and cleans up the mempool state. mempool.UpdateTransactionsFromBlock(vHashUpdate); - mempool.removeCoinbaseSpends(pcoinsTip, pindexDelete->nHeight); + mempool.removeForReorg(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 6d1df0b3d..1c38e3260 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -11,6 +11,7 @@ #include "main.h" #include "policy/fees.h" #include "streams.h" +#include "timedata.h" #include "util.h" #include "utilmoneystr.h" #include "utiltime.h" @@ -478,22 +479,26 @@ void CTxMemPool::remove(const CTransaction &origTx, std::list& rem } } -void CTxMemPool::removeCoinbaseSpends(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight) +void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight) { // Remove transactions spending a coinbase which are now immature LOCK(cs); list transactionsToRemove; for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { const CTransaction& tx = it->GetTx(); - BOOST_FOREACH(const CTxIn& txin, tx.vin) { - indexed_transaction_set::const_iterator it2 = mapTx.find(txin.prevout.hash); - if (it2 != mapTx.end()) - continue; - const CCoins *coins = pcoins->AccessCoins(txin.prevout.hash); - if (nCheckFrequency != 0) assert(coins); - if (!coins || (coins->IsCoinBase() && ((signed long)nMemPoolHeight) - coins->nHeight < COINBASE_MATURITY)) { - transactionsToRemove.push_back(tx); - break; + if (!IsFinalTx(tx, nMemPoolHeight, GetAdjustedTime())) { + transactionsToRemove.push_back(tx); + } else { + BOOST_FOREACH(const CTxIn& txin, tx.vin) { + indexed_transaction_set::const_iterator it2 = mapTx.find(txin.prevout.hash); + if (it2 != mapTx.end()) + continue; + const CCoins *coins = pcoins->AccessCoins(txin.prevout.hash); + if (nCheckFrequency != 0) assert(coins); + if (!coins || (coins->IsCoinBase() && ((signed long)nMemPoolHeight) - coins->nHeight < COINBASE_MATURITY)) { + transactionsToRemove.push_back(tx); + break; + } } } } diff --git a/src/txmempool.h b/src/txmempool.h index c470bbe28..f45d5a208 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -376,7 +376,7 @@ public: bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate = true); void remove(const CTransaction &tx, std::list& removed, bool fRecursive = false); - void removeCoinbaseSpends(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight); + void removeForReorg(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, bool fCurrentEstimate = true); From b0a064c4b825c15dee87739348bab23f13541bdd Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Sat, 5 Sep 2015 21:40:21 -0700 Subject: [PATCH 3/8] Fix comment in removeForReorg --- src/txmempool.cpp | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 1c38e3260..a2a53b018 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -481,7 +481,7 @@ void CTxMemPool::remove(const CTransaction &origTx, std::list& rem void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight) { - // Remove transactions spending a coinbase which are now immature + // Remove transactions spending a coinbase which are now immature and no-longer-final transactions LOCK(cs); list transactionsToRemove; for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { From 474b84a7413f124524cccf097dd36c7a24d406b8 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 9 Sep 2015 14:54:11 -0700 Subject: [PATCH 4/8] Make indentation in ActivateBestChainStep readable --- src/main.cpp | 66 ++++++++++++++++++++++++++-------------------------- 1 file changed, 33 insertions(+), 33 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 3422c56cf..e05237da2 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2525,43 +2525,43 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c bool fContinue = true; int nHeight = pindexFork ? pindexFork->nHeight : -1; while (fContinue && nHeight != pindexMostWork->nHeight) { - // Don't iterate the entire list of potential improvements toward the best tip, as we likely only need - // a few blocks along the way. - int nTargetHeight = std::min(nHeight + 32, pindexMostWork->nHeight); - vpindexToConnect.clear(); - vpindexToConnect.reserve(nTargetHeight - nHeight); - CBlockIndex *pindexIter = pindexMostWork->GetAncestor(nTargetHeight); - while (pindexIter && pindexIter->nHeight != nHeight) { - vpindexToConnect.push_back(pindexIter); - pindexIter = pindexIter->pprev; - } - nHeight = nTargetHeight; - - // Connect new blocks. - BOOST_REVERSE_FOREACH(CBlockIndex *pindexConnect, vpindexToConnect) { - if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : NULL)) { - if (state.IsInvalid()) { - // The block violates a consensus rule. - if (!state.CorruptionPossible()) - InvalidChainFound(vpindexToConnect.back()); - state = CValidationState(); - fInvalidFound = true; - fContinue = false; - break; + // Don't iterate the entire list of potential improvements toward the best tip, as we likely only need + // a few blocks along the way. + int nTargetHeight = std::min(nHeight + 32, pindexMostWork->nHeight); + vpindexToConnect.clear(); + vpindexToConnect.reserve(nTargetHeight - nHeight); + CBlockIndex *pindexIter = pindexMostWork->GetAncestor(nTargetHeight); + while (pindexIter && pindexIter->nHeight != nHeight) { + vpindexToConnect.push_back(pindexIter); + pindexIter = pindexIter->pprev; + } + nHeight = nTargetHeight; + + // Connect new blocks. + BOOST_REVERSE_FOREACH(CBlockIndex *pindexConnect, vpindexToConnect) { + if (!ConnectTip(state, chainparams, pindexConnect, pindexConnect == pindexMostWork ? pblock : NULL)) { + if (state.IsInvalid()) { + // The block violates a consensus rule. + if (!state.CorruptionPossible()) + InvalidChainFound(vpindexToConnect.back()); + state = CValidationState(); + fInvalidFound = true; + fContinue = false; + break; + } else { + // A system error occurred (disk space, database error, ...). + return false; + } } else { - // A system error occurred (disk space, database error, ...). - return false; - } - } else { - PruneBlockIndexCandidates(); - if (!pindexOldTip || chainActive.Tip()->nChainWork > pindexOldTip->nChainWork) { - // We're in a better position than we were. Return temporarily to release the lock. - fContinue = false; - break; + PruneBlockIndexCandidates(); + if (!pindexOldTip || chainActive.Tip()->nChainWork > pindexOldTip->nChainWork) { + // We're in a better position than we were. Return temporarily to release the lock. + fContinue = false; + break; + } } } } - } if (fBlocksDisconnected) mempool.TrimToSize(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000); From bb8ea1f6304d7ed3f5fe0a01c060ac9f94629349 Mon Sep 17 00:00:00 2001 From: Matt Corallo Date: Wed, 9 Sep 2015 16:31:20 -0700 Subject: [PATCH 5/8] removeForReorg calls once-per-disconnect-> once-per-reorg --- src/main.cpp | 22 ++++++++++++++-------- src/main.h | 2 +- 2 files changed, 15 insertions(+), 9 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index e05237da2..8f67c0351 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2310,12 +2310,11 @@ void static UpdateTip(CBlockIndex *pindexNew) { } } -/** Disconnect chainActive's tip. You want to manually re-limit mempool size after this */ +/** Disconnect chainActive's tip. You probably want to call mempool.removeForReorg and manually re-limit mempool size after this, with cs_main held. */ bool static DisconnectTip(CValidationState& state, const Consensus::Params& consensusParams) { CBlockIndex *pindexDelete = chainActive.Tip(); assert(pindexDelete); - mempool.check(pcoinsTip); // Read block from disk. CBlock block; if (!ReadBlockFromDisk(block, pindexDelete, consensusParams)) @@ -2350,8 +2349,6 @@ bool static DisconnectTip(CValidationState& state, const Consensus::Params& cons // UpdateTransactionsFromBlock finds descendants of any transactions in this // block that were added back and cleans up the mempool state. mempool.UpdateTransactionsFromBlock(vHashUpdate); - mempool.removeForReorg(pcoinsTip, pindexDelete->nHeight); - mempool.check(pcoinsTip); // Update chainActive and related variables. UpdateTip(pindexDelete->pprev); // Let wallets know transactions went from 1-confirmed to @@ -2375,7 +2372,6 @@ static int64_t nTimePostConnect = 0; bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, CBlockIndex* pindexNew, const CBlock* pblock) { assert(pindexNew->pprev == chainActive.Tip()); - mempool.check(pcoinsTip); // Read block from disk. int64_t nTime1 = GetTimeMicros(); CBlock block; @@ -2412,7 +2408,6 @@ bool static ConnectTip(CValidationState& state, const CChainParams& chainparams, // Remove conflicting transactions from the mempool. list txConflicted; mempool.removeForBlock(pblock->vtx, pindexNew->nHeight, txConflicted, !IsInitialBlockDownload()); - mempool.check(pcoinsTip); // Update chainActive & related variables. UpdateTip(pindexNew); // Tell wallet about transactions that went from mempool @@ -2515,8 +2510,11 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c // Disconnect active blocks which are no longer in the best chain. bool fBlocksDisconnected = false; while (chainActive.Tip() && chainActive.Tip() != pindexFork) { - if (!DisconnectTip(state, chainparams.GetConsensus())) + if (!DisconnectTip(state, chainparams.GetConsensus())) { + // Probably an AbortNode() error, but try to keep mempool consistent anyway + mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1); return false; + } fBlocksDisconnected = true; } @@ -2550,6 +2548,9 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c break; } else { // A system error occurred (disk space, database error, ...). + // Probably gonna shut down ASAP, but try to keep mempool consistent anyway + if (fBlocksDisconnected) + mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1); return false; } } else { @@ -2563,8 +2564,11 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c } } - if (fBlocksDisconnected) + if (fBlocksDisconnected) { + mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1); mempool.TrimToSize(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000); + } + mempool.check(pcoinsTip); // Callbacks/notifications for a new best chain. if (fInvalidFound) @@ -2672,6 +2676,7 @@ bool InvalidateBlock(CValidationState& state, const Consensus::Params& consensus // ActivateBestChain considers blocks already in chainActive // unconditionally valid already, so force disconnect away from it. if (!DisconnectTip(state, consensusParams)) { + mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1); return false; } } @@ -2689,6 +2694,7 @@ bool InvalidateBlock(CValidationState& state, const Consensus::Params& consensus } InvalidChainFound(pindex); + mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1); return true; } diff --git a/src/main.h b/src/main.h index bdbfa3826..2996fdcb5 100644 --- a/src/main.h +++ b/src/main.h @@ -467,7 +467,7 @@ bool InvalidateBlock(CValidationState& state, const Consensus::Params& consensus /** Remove invalidity status from a block and its descendants. */ bool ReconsiderBlock(CValidationState& state, CBlockIndex *pindex); -/** The currently-connected chain of blocks. */ +/** The currently-connected chain of blocks (protected by cs_main). */ extern CChain chainActive; /** Global variable that points to the active CCoinsView (protected by cs_main) */ From 7e49f5f8b4e237d7212d027a7bea4bbd52c9e7b6 Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 29 Oct 2015 14:06:13 -0400 Subject: [PATCH 6/8] Track coinbase spends in CTxMemPoolEntry This allows us to optimize CTxMemPool::removeForReorg. --- src/main.cpp | 13 ++++++++++++- src/test/miner_tests.cpp | 24 +++++++++++++----------- src/test/test_bitcoin.cpp | 2 +- src/test/test_bitcoin.h | 4 +++- src/txmempool.cpp | 8 +++++--- src/txmempool.h | 5 ++++- 6 files changed, 38 insertions(+), 18 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index 8f67c0351..ad8819eb3 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -953,7 +953,18 @@ bool AcceptToMemoryPool(CTxMemPool& pool, CValidationState &state, const CTransa CAmount inChainInputValue; double dPriority = view.GetPriority(tx, chainActive.Height(), inChainInputValue); - CTxMemPoolEntry entry(tx, nFees, GetTime(), dPriority, chainActive.Height(), pool.HasNoInputsOf(tx), inChainInputValue); + // Keep track of transactions that spend a coinbase, which we re-scan + // during reorgs to ensure COINBASE_MATURITY is still met. + bool fSpendsCoinbase = false; + BOOST_FOREACH(const CTxIn &txin, tx.vin) { + const CCoins *coins = view.AccessCoins(txin.prevout.hash); + if (coins->IsCoinBase()) { + fSpendsCoinbase = true; + break; + } + } + + CTxMemPoolEntry entry(tx, nFees, GetTime(), dPriority, chainActive.Height(), pool.HasNoInputsOf(tx), inChainInputValue, fSpendsCoinbase); unsigned int nSize = entry.GetTxSize(); // Don't accept it if it can't get into a block diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 1d7c9f65c..ab7357641 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -119,7 +119,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) { tx.vout[0].nValue -= 1000000; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx)); + bool spendsCoinbase = (i == 0) ? true : false; // only first tx spends coinbase + mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); tx.vin[0].prevout.hash = hash; } BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); @@ -139,7 +140,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) { tx.vout[0].nValue -= 10000000; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx)); + bool spendsCoinbase = (i == 0) ? true : false; // only first tx spends coinbase + mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); tx.vin[0].prevout.hash = hash; } BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); @@ -158,7 +160,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vin[0].prevout.hash = txFirst[1]->GetHash(); tx.vout[0].nValue = 4900000000LL; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx)); + mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); tx.vin[0].prevout.hash = hash; tx.vin.resize(2); tx.vin[1].scriptSig = CScript() << OP_1; @@ -166,7 +168,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vin[1].prevout.n = 0; tx.vout[0].nValue = 5900000000LL; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx)); + mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); delete pblocktemplate; mempool.clear(); @@ -177,7 +179,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vin[0].scriptSig = CScript() << OP_0 << OP_1; tx.vout[0].nValue = 0; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx)); + mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); delete pblocktemplate; mempool.clear(); @@ -190,12 +192,12 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) script = CScript() << OP_0; tx.vout[0].scriptPubKey = GetScriptForDestination(CScriptID(script)); hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx)); + mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); tx.vin[0].prevout.hash = hash; tx.vin[0].scriptSig = CScript() << (std::vector)script; tx.vout[0].nValue -= 1000000; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx)); + mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); delete pblocktemplate; mempool.clear(); @@ -206,10 +208,10 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vout[0].nValue = 4900000000LL; tx.vout[0].scriptPubKey = CScript() << OP_1; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx)); + mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); tx.vout[0].scriptPubKey = CScript() << OP_2; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx)); + mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); delete pblocktemplate; mempool.clear(); @@ -235,7 +237,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vout[0].scriptPubKey = CScript() << OP_1; tx.nLockTime = chainActive.Tip()->nHeight+1; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx)); + mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); BOOST_CHECK(!CheckFinalTx(tx, LOCKTIME_MEDIAN_TIME_PAST)); // time locked @@ -249,7 +251,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx2.vout[0].scriptPubKey = CScript() << OP_1; tx2.nLockTime = chainActive.Tip()->GetMedianTimePast()+1; hash = tx2.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx2)); + mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx2)); BOOST_CHECK(!CheckFinalTx(tx2, LOCKTIME_MEDIAN_TIME_PAST)); BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); diff --git a/src/test/test_bitcoin.cpp b/src/test/test_bitcoin.cpp index 351870014..9645c7c94 100644 --- a/src/test/test_bitcoin.cpp +++ b/src/test/test_bitcoin.cpp @@ -150,7 +150,7 @@ CTxMemPoolEntry TestMemPoolEntryHelper::FromTx(CMutableTransaction &tx, CTxMemPo CAmount inChainValue = hasNoDependencies ? txn.GetValueOut() : 0; return CTxMemPoolEntry(txn, nFee, nTime, dPriority, nHeight, - hasNoDependencies, inChainValue); + hasNoDependencies, inChainValue, spendsCoinbase); } void Shutdown(void* parg) diff --git a/src/test/test_bitcoin.h b/src/test/test_bitcoin.h index 815b22741..343c27673 100644 --- a/src/test/test_bitcoin.h +++ b/src/test/test_bitcoin.h @@ -65,10 +65,11 @@ struct TestMemPoolEntryHelper double dPriority; unsigned int nHeight; bool hadNoDependencies; + bool spendsCoinbase; TestMemPoolEntryHelper() : nFee(0), nTime(0), dPriority(0.0), nHeight(1), - hadNoDependencies(false) { } + hadNoDependencies(false), spendsCoinbase(false) { } CTxMemPoolEntry FromTx(CMutableTransaction &tx, CTxMemPool *pool = NULL); @@ -78,5 +79,6 @@ struct TestMemPoolEntryHelper TestMemPoolEntryHelper &Priority(double _priority) { dPriority = _priority; return *this; } TestMemPoolEntryHelper &Height(unsigned int _height) { nHeight = _height; return *this; } TestMemPoolEntryHelper &HadNoDependencies(bool _hnd) { hadNoDependencies = _hnd; return *this; } + TestMemPoolEntryHelper &SpendsCoinbase(bool _flag) { spendsCoinbase = _flag; return *this; } }; #endif diff --git a/src/txmempool.cpp b/src/txmempool.cpp index a2a53b018..5a3062291 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -21,9 +21,11 @@ using namespace std; CTxMemPoolEntry::CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee, int64_t _nTime, double _entryPriority, unsigned int _entryHeight, - bool poolHasNoInputsOf, CAmount _inChainInputValue): + bool poolHasNoInputsOf, CAmount _inChainInputValue, + bool _spendsCoinbase): tx(_tx), nFee(_nFee), nTime(_nTime), entryPriority(_entryPriority), entryHeight(_entryHeight), - hadNoDependencies(poolHasNoInputsOf), inChainInputValue(_inChainInputValue) + hadNoDependencies(poolHasNoInputsOf), inChainInputValue(_inChainInputValue), + spendsCoinbase(_spendsCoinbase) { nTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION); nModSize = tx.CalculateModifiedSize(nTxSize); @@ -488,7 +490,7 @@ void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMem const CTransaction& tx = it->GetTx(); if (!IsFinalTx(tx, nMemPoolHeight, GetAdjustedTime())) { transactionsToRemove.push_back(tx); - } else { + } else if (it->GetSpendsCoinbase()) { BOOST_FOREACH(const CTxIn& txin, tx.vin) { indexed_transaction_set::const_iterator it2 = mapTx.find(txin.prevout.hash); if (it2 != mapTx.end()) diff --git a/src/txmempool.h b/src/txmempool.h index f45d5a208..4c35f4ca0 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -67,6 +67,7 @@ private: unsigned int entryHeight; //! Chain height when entering the mempool bool hadNoDependencies; //! Not dependent on any other txs when it entered the mempool CAmount inChainInputValue; //! Sum of all txin values that are already in blockchain + bool spendsCoinbase; //! keep track of transactions that spend a coinbase // Information about descendants of this transaction that are in the // mempool; if we remove this transaction we must remove all of these @@ -80,7 +81,7 @@ private: public: CTxMemPoolEntry(const CTransaction& _tx, const CAmount& _nFee, int64_t _nTime, double _entryPriority, unsigned int _entryHeight, - bool poolHasNoInputsOf, CAmount _inChainInputValue); + bool poolHasNoInputsOf, CAmount _inChainInputValue, bool spendsCoinbase); CTxMemPoolEntry(const CTxMemPoolEntry& other); const CTransaction& GetTx() const { return this->tx; } @@ -109,6 +110,8 @@ public: uint64_t GetCountWithDescendants() const { return nCountWithDescendants; } uint64_t GetSizeWithDescendants() const { return nSizeWithDescendants; } CAmount GetFeesWithDescendants() const { return nFeesWithDescendants; } + + bool GetSpendsCoinbase() const { return spendsCoinbase; } }; // Helpers for modifying CTxMemPool::mapTx, which is a boost multi_index. From b7fa4aa3876b56694b27af0beef367be9e0733fd Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Thu, 12 Nov 2015 15:54:17 -0500 Subject: [PATCH 7/8] Don't call removeForReorg if DisconnectTip fails --- src/main.cpp | 8 +------- 1 file changed, 1 insertion(+), 7 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index ad8819eb3..feb526e09 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2521,11 +2521,8 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c // Disconnect active blocks which are no longer in the best chain. bool fBlocksDisconnected = false; while (chainActive.Tip() && chainActive.Tip() != pindexFork) { - if (!DisconnectTip(state, chainparams.GetConsensus())) { - // Probably an AbortNode() error, but try to keep mempool consistent anyway - mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1); + if (!DisconnectTip(state, chainparams.GetConsensus())) return false; - } fBlocksDisconnected = true; } @@ -2559,9 +2556,6 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c break; } else { // A system error occurred (disk space, database error, ...). - // Probably gonna shut down ASAP, but try to keep mempool consistent anyway - if (fBlocksDisconnected) - mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1); return false; } } else { From 2d8860e820e2ca73000f558eb9686206bec2652a Mon Sep 17 00:00:00 2001 From: Suhas Daftuar Date: Mon, 23 Nov 2015 16:06:12 -0500 Subject: [PATCH 8/8] Fix removeForReorg to use MedianTimePast --- src/main.cpp | 6 +++--- src/txmempool.cpp | 4 ++-- src/txmempool.h | 2 +- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/src/main.cpp b/src/main.cpp index feb526e09..0b758f391 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -2570,7 +2570,7 @@ static bool ActivateBestChainStep(CValidationState& state, const CChainParams& c } if (fBlocksDisconnected) { - mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1); + mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1, STANDARD_LOCKTIME_VERIFY_FLAGS); mempool.TrimToSize(GetArg("-maxmempool", DEFAULT_MAX_MEMPOOL_SIZE) * 1000000); } mempool.check(pcoinsTip); @@ -2681,7 +2681,7 @@ bool InvalidateBlock(CValidationState& state, const Consensus::Params& consensus // ActivateBestChain considers blocks already in chainActive // unconditionally valid already, so force disconnect away from it. if (!DisconnectTip(state, consensusParams)) { - mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1); + mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1, STANDARD_LOCKTIME_VERIFY_FLAGS); return false; } } @@ -2699,7 +2699,7 @@ bool InvalidateBlock(CValidationState& state, const Consensus::Params& consensus } InvalidChainFound(pindex); - mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1); + mempool.removeForReorg(pcoinsTip, chainActive.Tip()->nHeight + 1, STANDARD_LOCKTIME_VERIFY_FLAGS); return true; } diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 5a3062291..9d2513948 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -481,14 +481,14 @@ void CTxMemPool::remove(const CTransaction &origTx, std::list& rem } } -void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight) +void CTxMemPool::removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags) { // Remove transactions spending a coinbase which are now immature and no-longer-final transactions LOCK(cs); list transactionsToRemove; for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { const CTransaction& tx = it->GetTx(); - if (!IsFinalTx(tx, nMemPoolHeight, GetAdjustedTime())) { + if (!CheckFinalTx(tx, flags)) { transactionsToRemove.push_back(tx); } else if (it->GetSpendsCoinbase()) { BOOST_FOREACH(const CTxIn& txin, tx.vin) { diff --git a/src/txmempool.h b/src/txmempool.h index 4c35f4ca0..c4ea51557 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -379,7 +379,7 @@ public: bool addUnchecked(const uint256& hash, const CTxMemPoolEntry &entry, setEntries &setAncestors, bool fCurrentEstimate = true); void remove(const CTransaction &tx, std::list& removed, bool fRecursive = false); - void removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight); + void removeForReorg(const CCoinsViewCache *pcoins, unsigned int nMemPoolHeight, int flags); void removeConflicts(const CTransaction &tx, std::list& removed); void removeForBlock(const std::vector& vtx, unsigned int nBlockHeight, std::list& conflicts, bool fCurrentEstimate = true);