From 1018ebd00237e873d572b9b9a297bae1f92ec5d5 Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 20 Dec 2022 08:04:30 +0000 Subject: [PATCH 1/6] test: Handle mining slow start inside `CreateNewBlock_validity` This partially reverts zcash/zcash@4f4a8c3c884adfc76ae34b2218b257b6a641e3ac and correctly handles the mining slow start without repeatedly calling CreateNewBlock. --- src/test/miner_tests.cpp | 13 +++++++------ 1 file changed, 7 insertions(+), 6 deletions(-) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index e223182ba..b80eb3964 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -167,14 +167,14 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) fCheckpointsEnabled = false; fCoinbaseEnforcedShieldingEnabled = false; + // Simple block creation, nothing special yet: + BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); + // We can't make transactions until we have inputs // Therefore, load 100 blocks :) std::vectortxFirst; for (unsigned int i = 0; i < sizeof(blockinfo)/sizeof(*blockinfo); ++i) { - // Simple block creation, nothing special yet: - BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); - CBlock *pblock = &pblocktemplate->block; // pointer for convenience pblock->nVersion = 4; // Fake the blocks taking at least nPowTargetSpacing to be mined. @@ -183,10 +183,13 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) // one spacing ahead of the tip. Within 11 blocks of genesis, the median // will be closer to the tip, and blocks will appear slower. pblock->nTime = chainActive.Tip()->GetMedianTimePast()+6*Params().GetConsensus().PoWTargetSpacing(i); + pblock->nBits = GetNextWorkRequired(chainActive.Tip(), pblock, chainparams.GetConsensus()); CMutableTransaction txCoinbase(pblock->vtx[0]); txCoinbase.nVersion = 1; txCoinbase.vin[0].scriptSig = CScript() << (chainActive.Height()+1) << OP_0; txCoinbase.vout[0].scriptPubKey = CScript(); + txCoinbase.vout[0].nValue = 50000 * (i + 1); + txCoinbase.vout[1].nValue = 12500 * (i + 1); pblock->vtx[0] = CTransaction(txCoinbase); if (txFirst.size() < 2) txFirst.push_back(new CTransaction(pblock->vtx[0])); @@ -280,10 +283,8 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) BOOST_CHECK(ProcessNewBlock(state, chainparams, NULL, pblock, true, NULL)); BOOST_CHECK_MESSAGE(state.IsValid(), state.GetRejectReason()); pblock->hashPrevBlock = pblock->GetHash(); - - // Need to recreate the template each round because of mining slow start - delete pblocktemplate; } + delete pblocktemplate; // Just to make sure we can still make simple blocks BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); From 38e319a88fbf51f6fb0a63148f18af3344f39aee Mon Sep 17 00:00:00 2001 From: Mark Friedenbach Date: Fri, 22 May 2015 14:49:50 -0700 Subject: [PATCH 2/6] Prevent block.nTime from decreasing Under some circumstances it is possible for there to be a significant, discontinuous jump in a node's clock value. On mining nodes, this can result in block templates which are no longer valid due to time-based nLockTime constraints. UpdateTime() is modified so that it will never decrease a block's nLockTime, thereby preventing such invalidations. (cherry picked from commit bitcoin/bitcoin@ef8dfe41d1eba0de6d6554e25e658169f97313b5) Zcash: Updated CreateNewBlock_validity test and wallet_1941 RPC test to ensure we satisfy the future timestamp soft fork rule. --- qa/rpc-tests/wallet_1941.py | 12 +++++------- src/miner.cpp | 17 ++++++++++++----- src/miner.h | 2 +- src/test/miner_tests.cpp | 7 +++++++ 4 files changed, 25 insertions(+), 13 deletions(-) diff --git a/qa/rpc-tests/wallet_1941.py b/qa/rpc-tests/wallet_1941.py index a725563ff..868f84acf 100755 --- a/qa/rpc-tests/wallet_1941.py +++ b/qa/rpc-tests/wallet_1941.py @@ -68,12 +68,10 @@ class Wallet1941RegressionTest (BitcoinTestFramework): self.nodes[0].generate(1) # Ensure the block times of the latest blocks exceed the variability - self.nodes[0].setmocktime(starttime + 3000) - self.nodes[0].generate(1) - self.nodes[0].setmocktime(starttime + 6000) - self.nodes[0].generate(1) - self.nodes[0].setmocktime(starttime + 9000) - self.nodes[0].generate(1) + # but fall inside the future timestamp soft fork rule. + for i in range(10): + self.nodes[0].setmocktime(starttime + (900 * (i + 1))) + self.nodes[0].generate(1) self.sync_all() # Confirm the balance on node 0. @@ -100,7 +98,7 @@ class Wallet1941RegressionTest (BitcoinTestFramework): assert_equal(Decimal(resp), 0) # Re-import the key on node 1, scanning from before the transaction. - self.nodes[1].z_importkey(key, 'yes', self.nodes[1].getblockchaininfo()['blocks'] - 110) + self.nodes[1].z_importkey(key, 'yes', self.nodes[1].getblockchaininfo()['blocks'] - 120) # Confirm that the balance on node 1 is valid now (node 1 must # have rescanned) diff --git a/src/miner.cpp b/src/miner.cpp index f3e6264d1..c5fa76e35 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -104,20 +104,25 @@ public: } }; -void UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev) +int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev) { + int64_t nOldTime = pblock->nTime; auto medianTimePast = pindexPrev->GetMedianTimePast(); - auto nTime = std::max(medianTimePast + 1, GetTime()); + int64_t nNewTime = std::max(medianTimePast + 1, GetTime()); // See the comment in ContextualCheckBlockHeader() for background. if (consensusParams.FutureTimestampSoftForkActive(pindexPrev->nHeight + 1)) { - nTime = std::min(nTime, medianTimePast + MAX_FUTURE_BLOCK_TIME_MTP); + nNewTime = std::min(nNewTime, medianTimePast + MAX_FUTURE_BLOCK_TIME_MTP); } - pblock->nTime = nTime; + + if (nOldTime < nNewTime) + pblock->nTime = nNewTime; // Updating time can change work required on testnet: if (consensusParams.nPowAllowMinDifficultyBlocksAfterHeight != std::nullopt) { pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, consensusParams); } + + return nNewTime - nOldTime; } bool IsShieldedMinerAddress(const MinerAddress& minerAddr) { @@ -1103,7 +1108,9 @@ void static BitcoinMiner(const CChainParams& chainparams) // Update nNonce and nTime pblock->nNonce = ArithToUint256(UintToArith256(pblock->nNonce) + 1); - UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev); + if (UpdateTime(pblock, chainparams.GetConsensus(), pindexPrev) < 0) + break; // Recreate the block if the clock has run backwards, + // so that we can use the correct time. if (chainparams.GetConsensus().nPowAllowMinDifficultyBlocksAfterHeight != std::nullopt) { // Changing pblock->nTime can change work required on testnet: diff --git a/src/miner.h b/src/miner.h index 04b7ce5c1..7942b05a7 100644 --- a/src/miner.h +++ b/src/miner.h @@ -110,6 +110,6 @@ void IncrementExtraNonce( void GenerateBitcoins(bool fGenerate, int nThreads, const CChainParams& chainparams); #endif -void UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev); +int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev); #endif // BITCOIN_MINER_H diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index b80eb3964..49b8fb981 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -284,6 +284,13 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) BOOST_CHECK_MESSAGE(state.IsValid(), state.GetRejectReason()); pblock->hashPrevBlock = pblock->GetHash(); } + + // Set the clock to be just ahead of the last "mined" block, to ensure we satisfy the + // future timestamp soft fork rule. + auto curTime = GetTime(); + OffsetClock::SetGlobal(); + OffsetClock::Instance()->Set(std::chrono::seconds(-curTime + pblocktemplate->block.nTime)); + delete pblocktemplate; // Just to make sure we can still make simple blocks From 0d9a5442eb2cb2a970fc0de898a5c58dbd34717d Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Mon, 26 Oct 2015 14:06:19 -0400 Subject: [PATCH 3/6] Make accessing mempool parents and children public (cherry picked from commit bitcoin/bitcoin@1f09287c667d3a7d10ab9c5c96037fe48899490d) --- src/txmempool.h | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/txmempool.h b/src/txmempool.h index b571207fe..dbaa81c3a 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -433,6 +433,8 @@ public: }; typedef std::set setEntries; + const setEntries & GetMemPoolParents(txiter entry) const; + const setEntries & GetMemPoolChildren(txiter entry) const; private: typedef std::map cacheMap; @@ -444,8 +446,6 @@ private: typedef std::map txlinksMap; txlinksMap mapLinks; - const setEntries & GetMemPoolParents(txiter entry) const; - const setEntries & GetMemPoolChildren(txiter entry) const; void UpdateParent(txiter entry, txiter parent, bool add); void UpdateChild(txiter entry, txiter child, bool add); From f18c96d7ae308c453bafe99f3cbd771ca80ee484 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Wed, 28 Oct 2015 14:56:28 -0400 Subject: [PATCH 4/6] Expose FormatStateMessage (cherry picked from commit bitcoin/bitcoin@5f122633020ce5d9f78c73394cda576a8657a3ac) --- src/main.cpp | 2 +- src/main.h | 2 ++ 2 files changed, 3 insertions(+), 1 deletion(-) diff --git a/src/main.cpp b/src/main.cpp index 151529689..284d6d6b7 100644 --- a/src/main.cpp +++ b/src/main.cpp @@ -1759,7 +1759,7 @@ CAmount GetMinRelayFee(const CTransaction& tx, const CTxMemPool& pool, unsigned } /** Convert CValidationState to a human-readable message for logging */ -static std::string FormatStateMessage(const CValidationState &state) +std::string FormatStateMessage(const CValidationState &state) { return strprintf("%s%s (code %i)", state.GetRejectReason(), diff --git a/src/main.h b/src/main.h index 863bbcbb2..0d027bce5 100644 --- a/src/main.h +++ b/src/main.h @@ -348,6 +348,8 @@ bool AcceptToMemoryPool( CTxMemPool& pool, CValidationState &state, const CTransaction &tx, bool fLimitFree, bool* pfMissingInputs, bool fRejectAbsurdFee=false); +/** Convert CValidationState to a human-readable message for logging */ +std::string FormatStateMessage(const CValidationState &state); struct CNodeStateStats { int nMisbehavior; From 45355e85d0af05704135f9be9f5e2a1e266dfb92 Mon Sep 17 00:00:00 2001 From: Alex Morcos Date: Tue, 3 Nov 2015 10:35:39 -0500 Subject: [PATCH 5/6] Rewrite CreateNewBlock Use the score index on the mempool to only add sorted txs in order. Remove much of the validation while building the block, relying on mempool to be consistent and only contain txs that can be mined. The mempool is assumed to be consistent as far as not containing txs which spend non-existent outputs or double spends, and scripts are valid. Finality of txs is still checked (except not coinbase maturity, assumed in mempool). Still TestBlockValidity in case mempool consistency breaks and return error state if an invalid block was created. Unit tests are modified to realize that invalid blocks can now be constructed if the mempool breaks its consistency assumptions and also updated to have the right fees, since the cached value is now used for block construction. Conflicts: src/miner.cpp (cherry picked from commit bitcoin/bitcoin@553cad94e29c33872b60d97b8574ed2773355f0b) Zcash: Merged in our changes. --- src/miner.cpp | 345 ++++++++++++++------------------------- src/test/miner_tests.cpp | 57 ++++--- 2 files changed, 162 insertions(+), 240 deletions(-) diff --git a/src/miner.cpp b/src/miner.cpp index c5fa76e35..621e0284a 100644 --- a/src/miner.cpp +++ b/src/miner.cpp @@ -45,6 +45,7 @@ #include #endif #include +#include using namespace std; @@ -58,49 +59,18 @@ using namespace std; // transactions in the memory pool. When we select transactions from the // pool, we select by highest priority or fee rate, so we might consider // transactions that depend on transactions that aren't yet in the block. -// The COrphan class keeps track of these 'temporary orphans' while -// CreateBlock is figuring out which transactions to include. -// -class COrphan -{ -public: - const CTransaction* ptx; - set setDependsOn; - CFeeRate feeRate; - CAmount feePaid; - double dPriority; - - COrphan(const CTransaction* ptxIn) : ptx(ptxIn), feeRate(0), feePaid(0), dPriority(0) - { - } -}; std::optional last_block_num_txs; std::optional last_block_size; -// We want to sort transactions by priority and fee rate, so: -typedef boost::tuple TxPriority; -class TxPriorityCompare +class ScoreCompare { - bool byFee; - public: - TxPriorityCompare(bool _byFee) : byFee(_byFee) { } + ScoreCompare() {} - bool operator()(const TxPriority& a, const TxPriority& b) + bool operator()(const CTxMemPool::txiter a, const CTxMemPool::txiter b) { - if (byFee) - { - if (a.get<1>() == b.get<1>()) - return a.get<0>() < b.get<0>(); - return a.get<1>() < b.get<1>(); - } - else - { - if (a.get<0>() == b.get<0>()) - return a.get<1>() < b.get<1>(); - return a.get<0>() < b.get<0>(); - } + return CompareTxMemPoolEntryByScore()(*b,*a); // Convert to less than } }; @@ -379,6 +349,10 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre pblocktemplate->vTxFees.push_back(-1); // updated at end pblocktemplate->vTxSigOps.push_back(-1); // updated at end + // If we're given a coinbase tx, it's been precomputed, its fees are zero, + // so we can't include any mempool transactions; this will be an empty block. + bool nonEmptyBlock = !next_cb_mtx; + // Largest block you're willing to create: unsigned int nBlockMaxSize = GetArg("-blockmaxsize", DEFAULT_BLOCK_MAX_SIZE); // Limit to between 1K and MAX_BLOCK_SIZE-1K for sanity: @@ -395,6 +369,22 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre nBlockMinSize = std::min(nBlockMaxSize, nBlockMinSize); // Collect memory pool transactions into the block + CTxMemPool::setEntries inBlock; + CTxMemPool::setEntries waitSet; + + // This vector will be sorted into a priority queue: + vector vecPriority; + TxCoinAgePriorityCompare pricomparer; + std::map waitPriMap; + typedef std::map::iterator waitPriIter; + double actualPriority = -1; + + std::priority_queue, ScoreCompare> clearedTxs; + bool fPrintPriority = GetBoolArg("-printpriority", DEFAULT_PRINTPRIORITY); + uint64_t nBlockSize = 1000; + uint64_t nBlockTx = 0; + unsigned int nBlockSigOps = 100; + int lastFewTxs = 0; CAmount nFees = 0; { @@ -409,108 +399,26 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre SaplingMerkleTree sapling_tree; assert(view.GetSaplingAnchorAt(view.GetBestAnchor(SAPLING), sapling_tree)); - // Priority order to process transactions - list vOrphan; // list memory doesn't move - map > mapDependers; - bool fPrintPriority = GetBoolArg("-printpriority", DEFAULT_PRINTPRIORITY); + int64_t nLockTimeCutoff = (STANDARD_LOCKTIME_VERIFY_FLAGS & LOCKTIME_MEDIAN_TIME_PAST) + ? nMedianTimePast + : pblock->GetBlockTime(); - // This vector will be sorted into a priority queue: - vector vecPriority; - vecPriority.reserve(mempool.mapTx.size()); - - // If we're given a coinbase tx, it's been precomputed, its fees are zero, - // so we can't include any mempool transactions; this will be an empty block. - if (!next_cb_mtx) { + bool fPriorityBlock = nBlockPrioritySize > 0; + if (nonEmptyBlock && fPriorityBlock) { + vecPriority.reserve(mempool.mapTx.size()); for (CTxMemPool::indexed_transaction_set::iterator mi = mempool.mapTx.begin(); - mi != mempool.mapTx.end(); ++mi) + mi != mempool.mapTx.end(); ++mi) { - const CTransaction& tx = mi->GetTx(); - - int64_t nLockTimeCutoff = (STANDARD_LOCKTIME_VERIFY_FLAGS & LOCKTIME_MEDIAN_TIME_PAST) - ? nMedianTimePast - : pblock->GetBlockTime(); - - if (tx.IsCoinBase() || !IsFinalTx(tx, nHeight, nLockTimeCutoff) || IsExpiredTx(tx, nHeight)) - continue; - - COrphan* porphan = NULL; - double dPriority = 0; - CAmount nTotalIn = 0; - bool fMissingInputs = false; - for (const CTxIn& txin : tx.vin) - { - // Read prev transaction - if (!view.HaveCoins(txin.prevout.hash)) - { - LogPrintf("INFO: missing coins for %s", txin.prevout.hash.GetHex()); - // This should never happen; all transactions in the memory - // pool should connect to either transactions in the chain - // or other transactions in the memory pool. - if (!mempool.mapTx.count(txin.prevout.hash)) - { - LogPrintf("ERROR: mempool transaction missing input\n"); - if (fDebug) assert("mempool transaction missing input" == 0); - fMissingInputs = true; - if (porphan) - vOrphan.pop_back(); - break; - } - - // Has to wait for dependencies - if (!porphan) - { - // Use list for automatic deletion - vOrphan.push_back(COrphan(&tx)); - porphan = &vOrphan.back(); - } - mapDependers[txin.prevout.hash].push_back(porphan); - porphan->setDependsOn.insert(txin.prevout.hash); - nTotalIn += mempool.mapTx.find(txin.prevout.hash)->GetTx().vout[txin.prevout.n].nValue; - continue; - } - const CCoins* coins = view.AccessCoins(txin.prevout.hash); - assert(coins); - - CAmount nValueIn = coins->vout[txin.prevout.n].nValue; - nTotalIn += nValueIn; - - int nConf = nHeight - coins->nHeight; - - dPriority += (double)nValueIn * nConf; - } - nTotalIn += tx.GetShieldedValueIn(); - - if (fMissingInputs) continue; - - // Priority is sum(valuein * age) / modified_txsize - unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION); - dPriority = tx.ComputePriority(dPriority, nTxSize); - - uint256 hash = tx.GetHash(); - mempool.ApplyDeltas(hash, dPriority, nTotalIn); - - CAmount feePaid = nTotalIn - tx.GetValueOut(); - CFeeRate feeRate(feePaid, nTxSize); - - if (porphan) - { - porphan->dPriority = dPriority; - porphan->feeRate = feeRate; - porphan->feePaid = feePaid; - } - else - vecPriority.push_back(TxPriority(dPriority, feeRate, feePaid, &(mi->GetTx()))); + double dPriority = mi->GetPriority(nHeight); + CAmount dummy; + mempool.ApplyDeltas(mi->GetTx().GetHash(), dPriority, dummy); + vecPriority.push_back(TxCoinAgePriority(dPriority, mi)); } + std::make_heap(vecPriority.begin(), vecPriority.end(), pricomparer); } - // Collect transactions into block - uint64_t nBlockSize = 1000; - uint64_t nBlockTx = 0; - int nBlockSigOps = 100; - bool fSortedByFee = (nBlockPrioritySize <= 0); - - TxPriorityCompare comparer(fSortedByFee); - std::make_heap(vecPriority.begin(), vecPriority.end(), comparer); + CTxMemPool::indexed_transaction_set::nth_index<2>::type::iterator mi = mempool.mapTx.get<2>().begin(); + CTxMemPool::txiter iter; // We want to track the value pool, but if the miner gets // invoked on an old block before the hardcoded fallback @@ -540,89 +448,84 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre } } - LogPrintf("%s: Evaluating %u transactions for inclusion in block.", __func__, vecPriority.size()); - while (!vecPriority.empty()) + while (nonEmptyBlock && (mi != mempool.mapTx.get<2>().end() || !clearedTxs.empty())) { - // Take highest priority transaction off the priority queue: - double dPriority = vecPriority.front().get<0>(); - CFeeRate feeRate = vecPriority.front().get<1>(); - CAmount feePaid = vecPriority.front().get<2>(); - const CTransaction& tx = *(vecPriority.front().get<3>()); + bool priorityTx = false; + if (fPriorityBlock && !vecPriority.empty()) { // add a tx from priority queue to fill the blockprioritysize + priorityTx = true; + iter = vecPriority.front().second; + actualPriority = vecPriority.front().first; + std::pop_heap(vecPriority.begin(), vecPriority.end(), pricomparer); + vecPriority.pop_back(); + } + else if (clearedTxs.empty()) { // add tx with next highest score + iter = mempool.mapTx.project<0>(mi); + mi++; + } + else { // try to add a previously postponed child tx + iter = clearedTxs.top(); + clearedTxs.pop(); + } + + if (inBlock.count(iter)) + continue; // could have been added to the priorityBlock + + const CTransaction& tx = iter->GetTx(); const uint256& hash = tx.GetHash(); - std::pop_heap(vecPriority.begin(), vecPriority.end(), comparer); - vecPriority.pop_back(); + bool fOrphan = false; + for (CTxMemPool::txiter parent : mempool.GetMemPoolParents(iter)) + { + if (!inBlock.count(parent)) { + fOrphan = true; + break; + } + } + if (fOrphan) { + if (priorityTx) + waitPriMap.insert(std::make_pair(iter,actualPriority)); + else + waitSet.insert(iter); + continue; + } - // Size limits - unsigned int nTxSize = ::GetSerializeSize(tx, SER_NETWORK, PROTOCOL_VERSION); + unsigned int nTxSize = iter->GetTxSize(); + if (fPriorityBlock && + (nBlockSize + nTxSize >= nBlockPrioritySize || !AllowFree(actualPriority))) { + fPriorityBlock = false; + waitPriMap.clear(); + } + if (!priorityTx && + (iter->GetModifiedFee() < ::minRelayTxFee.GetFee(nTxSize)) && + (iter->GetModifiedFee() < DEFAULT_FEE) && + (nBlockSize >= nBlockMinSize)) { + break; + } if (nBlockSize + nTxSize >= nBlockMaxSize) { + if (nBlockSize > nBlockMaxSize - 100 || lastFewTxs > 50) { + break; + } + // Once we're within 1000 bytes of a full block, only look at 50 more txs + // to try to fill the remaining space. + if (nBlockSize > nBlockMaxSize - 1000) { + lastFewTxs++; + } LogPrintf("%s: skipping tx %s: exceeded maximum block size %u.", __func__, hash.GetHex(), nBlockMaxSize); continue; } - // Legacy limits on sigOps: - unsigned int nTxSigOps = GetLegacySigOpCount(tx); + if (!IsFinalTx(tx, nHeight, nLockTimeCutoff) || IsExpiredTx(tx, nHeight)) + continue; + + unsigned int nTxSigOps = iter->GetSigOpCount(); if (nBlockSigOps + nTxSigOps >= MAX_BLOCK_SIGOPS) { + if (nBlockSigOps > MAX_BLOCK_SIGOPS - 2) { + break; + } LogPrintf("%s: skipping tx %s: exceeds legacy max sigops %u.", __func__, hash.GetHex(), MAX_BLOCK_SIGOPS); continue; } - // Skip free transactions if we're past the minimum block size: - double dPriorityDelta = 0; - CAmount nFeeDelta = 0; - mempool.ApplyDeltas(hash, dPriorityDelta, nFeeDelta); - if (fSortedByFee && - (dPriorityDelta <= 0) && - (nFeeDelta <= 0) && - (feeRate < ::minRelayTxFee) && - (feePaid < DEFAULT_FEE) && - (nBlockSize + nTxSize >= nBlockMinSize)) - { - LogPrintf( - "%s: skipping free tx %s (fee is %i; %s) with size %u, current block size is %u & already have minimum block size %u.", - __func__, hash.GetHex(), - feePaid, feeRate.ToString(), nTxSize, nBlockSize, nBlockMinSize); - continue; - } - - // Prioritise by fee once past the priority size or we run out of high-priority - // transactions: - if (!fSortedByFee && - ((nBlockSize + nTxSize >= nBlockPrioritySize) || !AllowFree(dPriority))) - { - fSortedByFee = true; - comparer = TxPriorityCompare(fSortedByFee); - std::make_heap(vecPriority.begin(), vecPriority.end(), comparer); - } - - if (!view.HaveInputs(tx)) { - LogPrintf("%s: not including tx %s; missing inputs.", __func__, hash.GetHex()); - continue; - } - - CAmount nTxFees = view.GetValueIn(tx)-tx.GetValueOut(); - - nTxSigOps += GetP2SHSigOpCount(tx, view); - if (nBlockSigOps + nTxSigOps >= MAX_BLOCK_SIGOPS) { - LogPrintf("%s: skipping tx %s: exceeds p2sh max sigops %u.", __func__, hash.GetHex(), MAX_BLOCK_SIGOPS); - continue; - } - - std::vector allPrevOutputs; - for (const auto& input : tx.vin) { - allPrevOutputs.push_back(view.GetOutputFor(input)); - } - - // Note that flags: we don't want to set mempool/IsStandard() - // policy here, but we still have to ensure that the block we - // create only contains transactions that are valid in new blocks. - CValidationState state; - PrecomputedTransactionData txdata(tx, allPrevOutputs); - if (!ContextualCheckInputs(tx, state, view, true, MANDATORY_SCRIPT_VERIFY_FLAGS, true, txdata, chainparams.GetConsensus(), consensusBranchId)) { - LogPrintf("%s: skipping tx %s: Failed contextual inputs check.", __func__, hash.GetHex()); - continue; - } - if (chainparams.ZIP209Enabled() && monitoring_pool_balances) { // Does this transaction lead to a turnstile violation? @@ -656,8 +559,7 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre orchardValue = orchardValueDummy; } - UpdateCoins(tx, view, nHeight); - + CAmount nTxFees = iter->GetFee(); // Added pblock->vtx.push_back(tx); pblocktemplate->vTxFees.push_back(nTxFees); @@ -669,31 +571,37 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre if (fPrintPriority) { + double dPriority = iter->GetPriority(nHeight); + CAmount dummy; + mempool.ApplyDeltas(tx.GetHash(), dPriority, dummy); LogPrintf("%s: priority %.1f fee %s txid %s\n", - __func__, dPriority, feeRate.ToString(), hash.GetHex()); + __func__, dPriority , CFeeRate(iter->GetModifiedFee(), nTxSize).ToString(), hash.GetHex()); } + inBlock.insert(iter); + // Add transactions that depend on this one to the priority queue - if (mapDependers.count(hash)) + for (CTxMemPool::txiter child : mempool.GetMemPoolChildren(iter)) { - for (COrphan* porphan : mapDependers[hash]) - { - if (!porphan->setDependsOn.empty()) - { - porphan->setDependsOn.erase(hash); - if (porphan->setDependsOn.empty()) - { - vecPriority.push_back(TxPriority(porphan->dPriority, porphan->feeRate, porphan->feePaid, porphan->ptx)); - std::push_heap(vecPriority.begin(), vecPriority.end(), comparer); - } + if (fPriorityBlock) { + waitPriIter wpiter = waitPriMap.find(child); + if (wpiter != waitPriMap.end()) { + vecPriority.push_back(TxCoinAgePriority(wpiter->second,child)); + std::push_heap(vecPriority.begin(), vecPriority.end(), pricomparer); + waitPriMap.erase(wpiter); + } + } + else { + if (waitSet.count(child)) { + clearedTxs.push(child); + waitSet.erase(child); } } } } - last_block_num_txs = nBlockTx; last_block_size = nBlockSize; - LogPrintf("%s: total tx: %u; total size: %u (excluding coinbase)", __func__, nBlockTx, nBlockSize); + LogPrintf("%s: total size %u (excluding coinbase) txs: %u fees: %ld sigops %d", __func__, nBlockSize, nBlockTx, nFees, nBlockSigOps); // Create coinbase tx if (next_cb_mtx) { @@ -763,8 +671,9 @@ CBlockTemplate* CreateNewBlock(const CChainParams& chainparams, const MinerAddre pblocktemplate->vTxSigOps[0] = GetLegacySigOpCount(pblock->vtx[0]); CValidationState state; - if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, true)) - throw std::runtime_error(std::string("CreateNewBlock(): TestBlockValidity failed: ") + state.GetRejectReason()); + if (!TestBlockValidity(state, chainparams, *pblock, pindexPrev, true)) { + throw std::runtime_error(strprintf("%s: TestBlockValidity failed: %s", __func__, FormatStateMessage(state))); + } } return pblocktemplate.release(); diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 49b8fb981..6c7a0854b 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -310,7 +310,22 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vout[0].nValue -= 10; hash = tx.GetHash(); bool spendsCoinbase = (i == 0) ? true : false; // only first tx spends coinbase - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); + // If we don't set the # of sig ops in the CTxMemPoolEntry, template creation fails + mempool.addUnchecked(hash, entry.Fee(10).Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); + tx.vin[0].prevout.hash = hash; + } + BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); + mempool.clear(); + + tx.vin[0].prevout.hash = txFirst[0]->GetHash(); + tx.vout[0].nValue = 50000LL; + for (unsigned int i = 0; i < 1001; ++i) + { + tx.vout[0].nValue -= 10; + hash = tx.GetHash(); + bool spendsCoinbase = (i == 0) ? true : false; // only first tx spends coinbase + // If we do set the # of sig ops in the CTxMemPoolEntry, template creation passes + mempool.addUnchecked(hash, entry.Fee(10).Time(GetTime()).SpendsCoinbase(spendsCoinbase).SigOps(20).FromTx(tx)); tx.vin[0].prevout.hash = hash; } BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); @@ -331,18 +346,17 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vout[0].nValue -= 350; hash = tx.GetHash(); bool spendsCoinbase = (i == 0) ? true : false; // only first tx spends coinbase - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(10).Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); tx.vin[0].prevout.hash = hash; } BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); delete pblocktemplate; mempool.clear(); - // orphan in mempool + // orphan in mempool, template creation fails hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).FromTx(tx)); - BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); - delete pblocktemplate; + mempool.addUnchecked(hash, entry.Fee(10).Time(GetTime()).FromTx(tx)); + BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); mempool.clear(); // child with higher priority than parent @@ -350,7 +364,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vin[0].prevout.hash = txFirst[1]->GetHash(); tx.vout[0].nValue = 39000LL; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(1000LL).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); tx.vin[0].prevout.hash = hash; tx.vin.resize(2); tx.vin[1].scriptSig = CScript() << OP_1; @@ -358,23 +372,23 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vin[1].prevout.n = 0; tx.vout[0].nValue = 49000LL; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(4000LL).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); delete pblocktemplate; mempool.clear(); - // coinbase in mempool + // coinbase in mempool, template creation fails tx.vin.resize(1); tx.vin[0].prevout.SetNull(); tx.vin[0].scriptSig = CScript() << OP_0 << OP_1; tx.vout[0].nValue = 0; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); - BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); - delete pblocktemplate; + // give it a fee so it'll get mined + mempool.addUnchecked(hash, entry.Fee(1).Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); + BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); mempool.clear(); - // invalid (pre-p2sh) txn in mempool + // P2SH txn in mempool (valid, Zcash always had P2SH enabled) tx.vin[0].prevout.hash = txFirst[0]->GetHash(); tx.vin[0].prevout.n = 0; tx.vin[0].scriptSig = CScript() << OP_1; @@ -382,28 +396,27 @@ 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()).SpendsCoinbase(true).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(100L).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); tx.vin[0].prevout.hash = hash; tx.vin[0].scriptSig = CScript() << std::vector(script.begin(), script.end()); tx.vout[0].nValue -= 10000; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(10).Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); delete pblocktemplate; mempool.clear(); - // double spend txn pair in mempool + // double spend txn pair in mempool, template creation fails tx.vin[0].prevout.hash = txFirst[0]->GetHash(); tx.vin[0].scriptSig = CScript() << OP_1; tx.vout[0].nValue = 49000LL; tx.vout[0].scriptPubKey = CScript() << OP_1; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(1000L).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); tx.vout[0].scriptPubKey = CScript() << OP_2; hash = tx.GetHash(); - mempool.addUnchecked(hash, entry.Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); - BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); - delete pblocktemplate; + mempool.addUnchecked(hash, entry.Fee(1000L).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); + BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); mempool.clear(); // subsidy changing @@ -430,7 +443,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()).SpendsCoinbase(true).FromTx(tx)); + mempool.addUnchecked(hash, entry.Fee(1000L).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); BOOST_CHECK(!CheckFinalTx(tx, LOCKTIME_MEDIAN_TIME_PAST)); // time locked @@ -444,7 +457,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()).SpendsCoinbase(true).FromTx(tx2)); + mempool.addUnchecked(hash, entry.Fee(1000L).Time(GetTime()).SpendsCoinbase(true).FromTx(tx2)); BOOST_CHECK(!CheckFinalTx(tx2, LOCKTIME_MEDIAN_TIME_PAST)); BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); From c16b1e8c246b79f7335d76b09c4fafa0d6569aba Mon Sep 17 00:00:00 2001 From: Jack Grigg Date: Tue, 20 Dec 2022 11:30:16 +0000 Subject: [PATCH 6/6] test: Improve CreateNewBlock_validity exception checks --- src/test/miner_tests.cpp | 16 ++++++++++++---- 1 file changed, 12 insertions(+), 4 deletions(-) diff --git a/src/test/miner_tests.cpp b/src/test/miner_tests.cpp index 6c7a0854b..334f487cf 100644 --- a/src/test/miner_tests.cpp +++ b/src/test/miner_tests.cpp @@ -297,6 +297,14 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) BOOST_CHECK(pblocktemplate = CreateNewBlock(chainparams, scriptPubKey)); delete pblocktemplate; + // Define a helper function to check the kind of failure from TestBlockValidity. + auto err_is = [](const std::string& msg) { + return [&](std::runtime_error const& e) { + BOOST_TEST_INFO(e.what()); + return std::string(e.what()).find(msg) != std::string::npos; + }; + }; + // block sigops > limit: 1000 CHECKMULTISIG + 1 tx.vin.resize(1); // NOTE: OP_NOP is used to force 20 SigOps for the CHECKMULTISIG @@ -314,7 +322,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) mempool.addUnchecked(hash, entry.Fee(10).Time(GetTime()).SpendsCoinbase(spendsCoinbase).FromTx(tx)); tx.vin[0].prevout.hash = hash; } - BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); + BOOST_CHECK_EXCEPTION(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error, err_is("bad-blk-sigops")); mempool.clear(); tx.vin[0].prevout.hash = txFirst[0]->GetHash(); @@ -356,7 +364,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) // orphan in mempool, template creation fails hash = tx.GetHash(); mempool.addUnchecked(hash, entry.Fee(10).Time(GetTime()).FromTx(tx)); - BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); + BOOST_CHECK_EXCEPTION(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error, err_is("bad-txns-inputs-missingorspent")); mempool.clear(); // child with higher priority than parent @@ -385,7 +393,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) hash = tx.GetHash(); // give it a fee so it'll get mined mempool.addUnchecked(hash, entry.Fee(1).Time(GetTime()).SpendsCoinbase(false).FromTx(tx)); - BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); + BOOST_CHECK_EXCEPTION(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error, err_is("bad-cb-multiple")); mempool.clear(); // P2SH txn in mempool (valid, Zcash always had P2SH enabled) @@ -416,7 +424,7 @@ BOOST_AUTO_TEST_CASE(CreateNewBlock_validity) tx.vout[0].scriptPubKey = CScript() << OP_2; hash = tx.GetHash(); mempool.addUnchecked(hash, entry.Fee(1000L).Time(GetTime()).SpendsCoinbase(true).FromTx(tx)); - BOOST_CHECK_THROW(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error); + BOOST_CHECK_EXCEPTION(CreateNewBlock(chainparams, scriptPubKey), std::runtime_error, err_is("bad-txns-inputs-missingorspent")); mempool.clear(); // subsidy changing