diff options
| -rw-r--r-- | src/node/miner.cpp | 52 | ||||
| -rw-r--r-- | src/node/miner.h | 8 | ||||
| -rw-r--r-- | src/rpc/rawtransaction.cpp | 105 | ||||
| -rw-r--r-- | src/test/fuzz/tx_pool.cpp | 8 | ||||
| -rw-r--r-- | src/txmempool.cpp | 56 | ||||
| -rw-r--r-- | src/txmempool.h | 11 | ||||
| -rw-r--r-- | src/validation.cpp | 82 | ||||
| -rw-r--r-- | src/validation.h | 24 | ||||
| -rw-r--r-- | src/wallet/rpcwallet.cpp | 2 | ||||
| -rwxr-xr-x | test/lint/lint-circular-dependencies.sh | 2 |
10 files changed, 170 insertions, 180 deletions
diff --git a/src/node/miner.cpp b/src/node/miner.cpp index 8778a79f8b..291a6e1d10 100644 --- a/src/node/miner.cpp +++ b/src/node/miner.cpp @@ -29,14 +29,16 @@ int64_t UpdateTime(CBlockHeader* pblock, const Consensus::Params& consensusParams, const CBlockIndex* pindexPrev) { int64_t nOldTime = pblock->nTime; - int64_t nNewTime = std::max(pindexPrev->GetMedianTimePast()+1, GetAdjustedTime()); + int64_t nNewTime = std::max(pindexPrev->GetMedianTimePast() + 1, GetAdjustedTime()); - if (nOldTime < nNewTime) + if (nOldTime < nNewTime) { pblock->nTime = nNewTime; + } // Updating time can change work required on testnet: - if (consensusParams.fPowAllowMinDifficultyBlocks) + if (consensusParams.fPowAllowMinDifficultyBlocks) { pblock->nBits = GetNextWorkRequired(pindexPrev, pblock, consensusParams); + } return nNewTime - nOldTime; } @@ -53,7 +55,8 @@ void RegenerateCommitments(CBlock& block, ChainstateManager& chainman) block.hashMerkleRoot = BlockMerkleRoot(block); } -BlockAssembler::Options::Options() { +BlockAssembler::Options::Options() +{ blockMinFeeRate = CFeeRate(DEFAULT_BLOCK_MIN_TX_FEE); nBlockMaxWeight = DEFAULT_BLOCK_MAX_WEIGHT; } @@ -108,8 +111,9 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc pblocktemplate.reset(new CBlockTemplate()); - if(!pblocktemplate.get()) + if (!pblocktemplate.get()) { return nullptr; + } CBlock* const pblock = &pblocktemplate->block; // pointer for convenience // Add dummy coinbase tx as first transaction @@ -125,15 +129,12 @@ std::unique_ptr<CBlockTemplate> BlockAssembler::CreateNewBlock(const CScript& sc pblock->nVersion = g_versionbitscache.ComputeBlockVersion(pindexPrev, chainparams.GetConsensus()); // -regtest only: allow overriding block.nVersion with // -blockversion=N to test forking scenarios - if (chainparams.MineBlocksOnDemand()) + if (chainparams.MineBlocksOnDemand()) { pblock->nVersion = gArgs.GetIntArg("-blockversion", pblock->nVersion); + } pblock->nTime = GetAdjustedTime(); - const int64_t nMedianTimePast = pindexPrev->GetMedianTimePast(); - - nLockTimeCutoff = (STANDARD_LOCKTIME_VERIFY_FLAGS & LOCKTIME_MEDIAN_TIME_PAST) - ? nMedianTimePast - : pblock->GetBlockTime(); + m_lock_time_cutoff = pindexPrev->GetMedianTimePast(); // Decide whether to include witness transactions // This is only needed in case the witness softfork activation is reverted @@ -193,8 +194,7 @@ void BlockAssembler::onlyUnconfirmed(CTxMemPool::setEntries& testSet) // Only test txs not already in the block if (inBlock.count(*iit)) { testSet.erase(iit++); - } - else { + } else { iit++; } } @@ -203,10 +203,12 @@ void BlockAssembler::onlyUnconfirmed(CTxMemPool::setEntries& testSet) bool BlockAssembler::TestPackage(uint64_t packageSize, int64_t packageSigOpsCost) const { // TODO: switch to weight-based accounting for packages instead of vsize-based accounting. - if (nBlockWeight + WITNESS_SCALE_FACTOR * packageSize >= nBlockMaxWeight) + if (nBlockWeight + WITNESS_SCALE_FACTOR * packageSize >= nBlockMaxWeight) { return false; - if (nBlockSigOpsCost + packageSigOpsCost >= MAX_BLOCK_SIGOPS_COST) + } + if (nBlockSigOpsCost + packageSigOpsCost >= MAX_BLOCK_SIGOPS_COST) { return false; + } return true; } @@ -217,10 +219,12 @@ bool BlockAssembler::TestPackage(uint64_t packageSize, int64_t packageSigOpsCost bool BlockAssembler::TestPackageTransactions(const CTxMemPool::setEntries& package) const { for (CTxMemPool::txiter it : package) { - if (!IsFinalTx(it->GetTx(), nHeight, nLockTimeCutoff)) + if (!IsFinalTx(it->GetTx(), nHeight, m_lock_time_cutoff)) { return false; - if (!fIncludeWitness && it->GetTx().HasWitness()) + } + if (!fIncludeWitness && it->GetTx().HasWitness()) { return false; + } } return true; } @@ -253,8 +257,9 @@ int BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& already m_mempool.CalculateDescendants(it, descendants); // Insert all descendants (not yet in block) into the modified set for (CTxMemPool::txiter desc : descendants) { - if (alreadyAdded.count(desc)) + if (alreadyAdded.count(desc)) { continue; + } ++nDescendantsUpdated; modtxiter mit = mapModifiedTx.find(desc); if (mit == mapModifiedTx.end()) { @@ -280,7 +285,7 @@ int BlockAssembler::UpdatePackagesForAdded(const CTxMemPool::setEntries& already // guaranteed to fail again, but as a belt-and-suspenders check we put it in // failedTx and avoid re-evaluation, since the re-evaluation would be using // cached size/sigops/fee values that are not actually correct. -bool BlockAssembler::SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set &mapModifiedTx, CTxMemPool::setEntries &failedTx) +bool BlockAssembler::SkipMapTxEntry(CTxMemPool::txiter it, indexed_modified_transaction_set& mapModifiedTx, CTxMemPool::setEntries& failedTx) { assert(it != m_mempool.mapTx.end()); return mapModifiedTx.count(it) || inBlock.count(it) || failedTx.count(it); @@ -307,7 +312,7 @@ void BlockAssembler::SortForBlock(const CTxMemPool::setEntries& package, std::ve // Each time through the loop, we compare the best transaction in // mapModifiedTxs with the next transaction in the mempool to decide what // transaction package to work on next. -void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpdated) +void BlockAssembler::addPackageTxs(int& nPackagesSelected, int& nDescendantsUpdated) { // mapModifiedTx will store sorted packages after they are modified // because some of their txs are already in the block @@ -423,7 +428,7 @@ void BlockAssembler::addPackageTxs(int &nPackagesSelected, int &nDescendantsUpda std::vector<CTxMemPool::txiter> sortedEntries; SortForBlock(ancestors, sortedEntries); - for (size_t i=0; i<sortedEntries.size(); ++i) { + for (size_t i = 0; i < sortedEntries.size(); ++i) { AddToBlock(sortedEntries[i]); // Erase from the modified set, if present mapModifiedTx.erase(sortedEntries[i]); @@ -440,13 +445,12 @@ void IncrementExtraNonce(CBlock* pblock, const CBlockIndex* pindexPrev, unsigned { // Update nExtraNonce static uint256 hashPrevBlock; - if (hashPrevBlock != pblock->hashPrevBlock) - { + if (hashPrevBlock != pblock->hashPrevBlock) { nExtraNonce = 0; hashPrevBlock = pblock->hashPrevBlock; } ++nExtraNonce; - unsigned int nHeight = pindexPrev->nHeight+1; // Height first in coinbase required for block.version=2 + unsigned int nHeight = pindexPrev->nHeight + 1; // Height first in coinbase required for block.version=2 CMutableTransaction txCoinbase(*pblock->vtx[0]); txCoinbase.vin[0].scriptSig = (CScript() << nHeight << CScriptNum(nExtraNonce)); assert(txCoinbase.vin[0].scriptSig.size() <= 100); diff --git a/src/node/miner.h b/src/node/miner.h index 0e8c02793a..e50db731b7 100644 --- a/src/node/miner.h +++ b/src/node/miner.h @@ -80,10 +80,11 @@ struct modifiedentry_iter { // This is sufficient to sort an ancestor package in an order that is valid // to appear in a block. struct CompareTxIterByAncestorCount { - bool operator()(const CTxMemPool::txiter &a, const CTxMemPool::txiter &b) const + bool operator()(const CTxMemPool::txiter& a, const CTxMemPool::txiter& b) const { - if (a->GetCountWithAncestors() != b->GetCountWithAncestors()) + if (a->GetCountWithAncestors() != b->GetCountWithAncestors()) { return a->GetCountWithAncestors() < b->GetCountWithAncestors(); + } return CompareIteratorByHash()(a, b); } }; @@ -143,7 +144,8 @@ private: // Chain context for the block int nHeight; - int64_t nLockTimeCutoff; + int64_t m_lock_time_cutoff; + const CChainParams& chainparams; const CTxMemPool& m_mempool; CChainState& m_chainstate; diff --git a/src/rpc/rawtransaction.cpp b/src/rpc/rawtransaction.cpp index 2dd121c6f6..7f3917b638 100644 --- a/src/rpc/rawtransaction.cpp +++ b/src/rpc/rawtransaction.cpp @@ -69,6 +69,43 @@ static void TxToJSON(const CTransaction& tx, const uint256 hashBlock, UniValue& } } +static std::vector<RPCArg> CreateTxDoc() +{ + return { + {"inputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The inputs", + { + {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", + { + {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"}, + {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output number"}, + {"sequence", RPCArg::Type::NUM, RPCArg::DefaultHint{"depends on the value of the 'replaceable' and 'locktime' arguments"}, "The sequence number"}, + }, + }, + }, + }, + {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The outputs (key-value pairs), where none of the keys are duplicated.\n" + "That is, each address can only appear once and there can only be one 'data' object.\n" + "For compatibility reasons, a dictionary, which holds the key-value pairs directly, is also\n" + " accepted as second parameter.", + { + {"", RPCArg::Type::OBJ_USER_KEYS, RPCArg::Optional::OMITTED, "", + { + {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in " + CURRENCY_UNIT}, + }, + }, + {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", + { + {"data", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "A key-value pair. The key must be \"data\", the value is hex-encoded data"}, + }, + }, + }, + }, + {"locktime", RPCArg::Type::NUM, RPCArg::Default{0}, "Raw locktime. Non-0 value also locktime-activates inputs"}, + {"replaceable", RPCArg::Type::BOOL, RPCArg::Default{false}, "Marks this transaction as BIP125-replaceable.\n" + "Allows this transaction to be replaced by a transaction with higher fees. If provided, it is an error if explicit sequence numbers are incompatible."}, + }; +} + static RPCHelpMan getrawtransaction() { return RPCHelpMan{ @@ -375,39 +412,7 @@ static RPCHelpMan createrawtransaction() "Returns hex-encoded raw transaction.\n" "Note that the transaction's inputs are not signed, and\n" "it is not stored in the wallet or transmitted to the network.\n", - { - {"inputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The inputs", - { - {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", - { - {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"}, - {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output number"}, - {"sequence", RPCArg::Type::NUM, RPCArg::DefaultHint{"depends on the value of the 'replaceable' and 'locktime' arguments"}, "The sequence number"}, - }, - }, - }, - }, - {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The outputs (key-value pairs), where none of the keys are duplicated.\n" - "That is, each address can only appear once and there can only be one 'data' object.\n" - "For compatibility reasons, a dictionary, which holds the key-value pairs directly, is also\n" - " accepted as second parameter.", - { - {"", RPCArg::Type::OBJ_USER_KEYS, RPCArg::Optional::OMITTED, "", - { - {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in " + CURRENCY_UNIT}, - }, - }, - {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", - { - {"data", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "A key-value pair. The key must be \"data\", the value is hex-encoded data"}, - }, - }, - }, - }, - {"locktime", RPCArg::Type::NUM, RPCArg::Default{0}, "Raw locktime. Non-0 value also locktime-activates inputs"}, - {"replaceable", RPCArg::Type::BOOL, RPCArg::Default{false}, "Marks this transaction as BIP125-replaceable.\n" - " Allows this transaction to be replaced by a transaction with higher fees. If provided, it is an error if explicit sequence numbers are incompatible."}, - }, + CreateTxDoc(), RPCResult{ RPCResult::Type::STR_HEX, "transaction", "hex string of the transaction" }, @@ -1435,39 +1440,7 @@ static RPCHelpMan createpsbt() return RPCHelpMan{"createpsbt", "\nCreates a transaction in the Partially Signed Transaction format.\n" "Implements the Creator role.\n", - { - {"inputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The json objects", - { - {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", - { - {"txid", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "The transaction id"}, - {"vout", RPCArg::Type::NUM, RPCArg::Optional::NO, "The output number"}, - {"sequence", RPCArg::Type::NUM, RPCArg::DefaultHint{"depends on the value of the 'replaceable' and 'locktime' arguments"}, "The sequence number"}, - }, - }, - }, - }, - {"outputs", RPCArg::Type::ARR, RPCArg::Optional::NO, "The outputs (key-value pairs), where none of the keys are duplicated.\n" - "That is, each address can only appear once and there can only be one 'data' object.\n" - "For compatibility reasons, a dictionary, which holds the key-value pairs directly, is also\n" - " accepted as second parameter.", - { - {"", RPCArg::Type::OBJ_USER_KEYS, RPCArg::Optional::OMITTED, "", - { - {"address", RPCArg::Type::AMOUNT, RPCArg::Optional::NO, "A key-value pair. The key (string) is the bitcoin address, the value (float or string) is the amount in " + CURRENCY_UNIT}, - }, - }, - {"", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED, "", - { - {"data", RPCArg::Type::STR_HEX, RPCArg::Optional::NO, "A key-value pair. The key must be \"data\", the value is hex-encoded data"}, - }, - }, - }, - }, - {"locktime", RPCArg::Type::NUM, RPCArg::Default{0}, "Raw locktime. Non-0 value also locktime-activates inputs"}, - {"replaceable", RPCArg::Type::BOOL, RPCArg::Default{false}, "Marks this transaction as BIP125 replaceable.\n" - " Allows this transaction to be replaced by a transaction with higher fees. If provided, it is an error if explicit sequence numbers are incompatible."}, - }, + CreateTxDoc(), RPCResult{ RPCResult::Type::STR, "", "The resulting raw transaction (base64-encoded string)" }, diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 702660f63e..c32c965ab0 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -86,7 +86,7 @@ void Finish(FuzzedDataProvider& fuzzed_data_provider, MockedTxPool& tx_pool, CCh BlockAssembler::Options options; options.nBlockMaxWeight = fuzzed_data_provider.ConsumeIntegralInRange(0U, MAX_BLOCK_WEIGHT); options.blockMinFeeRate = CFeeRate{ConsumeMoney(fuzzed_data_provider, /*max=*/COIN)}; - auto assembler = BlockAssembler{chainstate, *static_cast<CTxMemPool*>(&tx_pool), ::Params(), options}; + auto assembler = BlockAssembler{chainstate, *static_cast<CTxMemPool*>(&tx_pool), chainstate.m_params, options}; auto block_template = assembler.CreateNewBlock(CScript{} << OP_TRUE); Assert(block_template->block.vtx.size() >= 1); } @@ -224,13 +224,13 @@ FUZZ_TARGET_INIT(tx_pool_standard, initialize_tx_pool) // Make sure ProcessNewPackage on one transaction works and always fully validates the transaction. // The result is not guaranteed to be the same as what is returned by ATMP. const auto result_package = WITH_LOCK(::cs_main, - return ProcessNewPackage(node.chainman->ActiveChainstate(), tx_pool, {tx}, true)); + return ProcessNewPackage(chainstate, tx_pool, {tx}, true)); auto it = result_package.m_tx_results.find(tx->GetWitnessHash()); Assert(it != result_package.m_tx_results.end()); Assert(it->second.m_result_type == MempoolAcceptResult::ResultType::VALID || it->second.m_result_type == MempoolAcceptResult::ResultType::INVALID); - const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(chainstate, tx_pool, tx, bypass_limits)); + const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */ false)); const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID; SyncWithValidationInterfaceQueue(); UnregisterSharedValidationInterface(txr); @@ -330,7 +330,7 @@ FUZZ_TARGET_INIT(tx_pool, initialize_tx_pool) const auto tx = MakeTransactionRef(mut_tx); const bool bypass_limits = fuzzed_data_provider.ConsumeBool(); ::fRequireStandard = fuzzed_data_provider.ConsumeBool(); - const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(node.chainman->ActiveChainstate(), tx_pool, tx, bypass_limits)); + const auto res = WITH_LOCK(::cs_main, return AcceptToMemoryPool(tx_pool, chainstate, tx, GetTime(), bypass_limits, /* test_accept= */ false)); const bool accepted = res.m_result_type == MempoolAcceptResult::ResultType::VALID; if (accepted) { txids.push_back(tx->GetHash()); diff --git a/src/txmempool.cpp b/src/txmempool.cpp index 502a27dc6b..27fbb8acac 100644 --- a/src/txmempool.cpp +++ b/src/txmempool.cpp @@ -16,7 +16,6 @@ #include <util/moneystr.h> #include <util/system.h> #include <util/time.h> -#include <validation.h> #include <validationinterface.h> #include <cmath> @@ -74,6 +73,24 @@ private: const LockPoints& lp; }; +bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) +{ + AssertLockHeld(cs_main); + assert(lp); + // If there are relative lock times then the maxInputBlock will be set + // If there are no relative lock times, the LockPoints don't depend on the chain + if (lp->maxInputBlock) { + // Check whether active_chain is an extension of the block at which the LockPoints + // calculation was valid. If not LockPoints are no longer valid + if (!active_chain.Contains(lp->maxInputBlock)) { + return false; + } + } + + // LockPoints still valid + return true; +} + CTxMemPoolEntry::CTxMemPoolEntry(const CTransactionRef& tx, CAmount fee, int64_t time, unsigned int entry_height, bool spends_coinbase, int64_t sigops_cost, LockPoints lp) @@ -616,44 +633,27 @@ void CTxMemPool::removeRecursive(const CTransaction &origTx, MemPoolRemovalReaso RemoveStaged(setAllRemoves, false, reason); } -void CTxMemPool::removeForReorg(CChainState& active_chainstate, int flags) +void CTxMemPool::removeForReorg(CChain& chain, std::function<bool(txiter)> check_final_and_mature) { // Remove transactions spending a coinbase which are now immature and no-longer-final transactions AssertLockHeld(cs); + AssertLockHeld(::cs_main); + setEntries txToRemove; for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { - const CTransaction& tx = it->GetTx(); - LockPoints lp = it->GetLockPoints(); - bool validLP = TestLockPointValidity(active_chainstate.m_chain, &lp); - CCoinsViewMemPool view_mempool(&active_chainstate.CoinsTip(), *this); - if (!CheckFinalTx(active_chainstate.m_chain.Tip(), tx, flags) - || !CheckSequenceLocks(active_chainstate.m_chain.Tip(), view_mempool, tx, flags, &lp, validLP)) { - // Note if CheckSequenceLocks fails the LockPoints may still be invalid - // So it's critical that we remove the tx and not depend on the LockPoints. - txToRemove.insert(it); - } else if (it->GetSpendsCoinbase()) { - for (const CTxIn& txin : tx.vin) { - indexed_transaction_set::const_iterator it2 = mapTx.find(txin.prevout.hash); - if (it2 != mapTx.end()) - continue; - const Coin &coin = active_chainstate.CoinsTip().AccessCoin(txin.prevout); - if (m_check_ratio != 0) assert(!coin.IsSpent()); - unsigned int nMemPoolHeight = active_chainstate.m_chain.Tip()->nHeight + 1; - if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY)) { - txToRemove.insert(it); - break; - } - } - } - if (!validLP) { - mapTx.modify(it, update_lock_points(lp)); - } + if (check_final_and_mature(it)) txToRemove.insert(it); } setEntries setAllRemoves; for (txiter it : txToRemove) { CalculateDescendants(it, setAllRemoves); } RemoveStaged(setAllRemoves, false, MemPoolRemovalReason::REORG); + for (indexed_transaction_set::const_iterator it = mapTx.begin(); it != mapTx.end(); it++) { + LockPoints lp = it->GetLockPoints(); + if (!TestLockPointValidity(chain, &lp)) { + mapTx.modify(it, update_lock_points(lp)); + } + } } void CTxMemPool::removeConflicts(const CTransaction &tx) diff --git a/src/txmempool.h b/src/txmempool.h index 85417ac3fc..c6e08a3ca5 100644 --- a/src/txmempool.h +++ b/src/txmempool.h @@ -14,6 +14,7 @@ #include <utility> #include <vector> +#include <chain.h> #include <coins.h> #include <consensus/amount.h> #include <indirectmap.h> @@ -49,6 +50,11 @@ struct LockPoints { CBlockIndex* maxInputBlock{nullptr}; }; +/** + * Test whether the LockPoints height and time are still valid on the current chain + */ +bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) EXCLUSIVE_LOCKS_REQUIRED(cs_main); + struct CompareIteratorByHash { // SFINAE for T where T is either a pointer type (e.g., a txiter) or a reference_wrapper<T> // (e.g. a wrapped CTxMemPoolEntry&) @@ -583,7 +589,10 @@ public: void addUnchecked(const CTxMemPoolEntry& entry, setEntries& setAncestors, bool validFeeEstimate = true) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeRecursive(const CTransaction& tx, MemPoolRemovalReason reason) EXCLUSIVE_LOCKS_REQUIRED(cs); - void removeForReorg(CChainState& active_chainstate, int flags) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); + /** After reorg, check if mempool entries are now non-final, premature coinbase spends, or have + * invalid lockpoints. Update lockpoints and remove entries (and descendants of entries) that + * are no longer valid. */ + void removeForReorg(CChain& chain, std::function<bool(txiter)> check_final_and_mature) EXCLUSIVE_LOCKS_REQUIRED(cs, cs_main); void removeConflicts(const CTransaction& tx) EXCLUSIVE_LOCKS_REQUIRED(cs); void removeForBlock(const std::vector<CTransactionRef>& vtx, unsigned int nBlockHeight) EXCLUSIVE_LOCKS_REQUIRED(cs); diff --git a/src/validation.cpp b/src/validation.cpp index b0e6152b09..6ed5e7a548 100644 --- a/src/validation.cpp +++ b/src/validation.cpp @@ -212,24 +212,6 @@ bool CheckFinalTx(const CBlockIndex* active_chain_tip, const CTransaction &tx, i return IsFinalTx(tx, nBlockHeight, nBlockTime); } -bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) -{ - AssertLockHeld(cs_main); - assert(lp); - // If there are relative lock times then the maxInputBlock will be set - // If there are no relative lock times, the LockPoints don't depend on the chain - if (lp->maxInputBlock) { - // Check whether active_chain is an extension of the block at which the LockPoints - // calculation was valid. If not LockPoints are no longer valid - if (!active_chain.Contains(lp->maxInputBlock)) { - return false; - } - } - - // LockPoints still valid - return true; -} - bool CheckSequenceLocks(CBlockIndex* tip, const CCoinsView& coins_view, const CTransaction& tx, @@ -349,8 +331,8 @@ void CChainState::MaybeUpdateMempoolForReorg( while (it != disconnectpool.queuedTx.get<insertion_order>().rend()) { // ignore validation errors in resurrected transactions if (!fAddToMempool || (*it)->IsCoinBase() || - AcceptToMemoryPool( - *this, *m_mempool, *it, true /* bypass_limits */).m_result_type != + AcceptToMemoryPool(*m_mempool, *this, *it, GetTime(), + /* bypass_limits= */true, /* test_accept= */ false).m_result_type != MempoolAcceptResult::ResultType::VALID) { // If the transaction doesn't make it in to the mempool, remove any // transactions that depend on it (which would now be orphans). @@ -368,8 +350,39 @@ void CChainState::MaybeUpdateMempoolForReorg( // the disconnectpool that were added back and cleans up the mempool state. m_mempool->UpdateTransactionsFromBlock(vHashUpdate); + const auto check_final_and_mature = [this, flags=STANDARD_LOCKTIME_VERIFY_FLAGS](CTxMemPool::txiter it) + EXCLUSIVE_LOCKS_REQUIRED(m_mempool->cs, ::cs_main) { + bool should_remove = false; + AssertLockHeld(m_mempool->cs); + AssertLockHeld(::cs_main); + const CTransaction& tx = it->GetTx(); + LockPoints lp = it->GetLockPoints(); + bool validLP = TestLockPointValidity(m_chain, &lp); + CCoinsViewMemPool view_mempool(&CoinsTip(), *m_mempool); + if (!CheckFinalTx(m_chain.Tip(), tx, flags) + || !CheckSequenceLocks(m_chain.Tip(), view_mempool, tx, flags, &lp, validLP)) { + // Note if CheckSequenceLocks fails the LockPoints may still be invalid + // So it's critical that we remove the tx and not depend on the LockPoints. + should_remove = true; + } else if (it->GetSpendsCoinbase()) { + for (const CTxIn& txin : tx.vin) { + auto it2 = m_mempool->mapTx.find(txin.prevout.hash); + if (it2 != m_mempool->mapTx.end()) + continue; + const Coin &coin = CoinsTip().AccessCoin(txin.prevout); + assert(!coin.IsSpent()); + unsigned int nMemPoolHeight = m_chain.Tip()->nHeight + 1; + if (coin.IsSpent() || (coin.IsCoinBase() && ((signed long)nMemPoolHeight) - coin.nHeight < COINBASE_MATURITY)) { + should_remove = true; + break; + } + } + } + return should_remove; + }; + // We also need to remove any now-immature transactions - m_mempool->removeForReorg(*this, STANDARD_LOCKTIME_VERIFY_FLAGS); + m_mempool->removeForReorg(m_chain, check_final_and_mature); // Re-limit mempool size, in case we added any transactions LimitMempoolSize( *m_mempool, @@ -1078,15 +1091,13 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptMultipleTransactions(const std:: } // anon namespace -/** (try to) add transaction to memory pool with a specified acceptance time **/ -static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainparams, CTxMemPool& pool, - CChainState& active_chainstate, - const CTransactionRef &tx, int64_t nAcceptTime, - bool bypass_limits, bool test_accept) - EXCLUSIVE_LOCKS_REQUIRED(cs_main) +MempoolAcceptResult AcceptToMemoryPool(CTxMemPool& pool, CChainState& active_chainstate, const CTransactionRef& tx, + int64_t accept_time, bool bypass_limits, bool test_accept) + EXCLUSIVE_LOCKS_REQUIRED(cs_main) { + const CChainParams& chainparams{active_chainstate.m_params}; std::vector<COutPoint> coins_to_uncache; - auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, nAcceptTime, bypass_limits, coins_to_uncache, test_accept); + auto args = MemPoolAccept::ATMPArgs::SingleAccept(chainparams, accept_time, bypass_limits, coins_to_uncache, test_accept); const MempoolAcceptResult result = MemPoolAccept(pool, active_chainstate).AcceptSingleTransaction(tx, args); if (result.m_result_type != MempoolAcceptResult::ResultType::VALID) { // Remove coins that were not present in the coins cache before calling @@ -1103,12 +1114,6 @@ static MempoolAcceptResult AcceptToMemoryPoolWithTime(const CChainParams& chainp return result; } -MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx, - bool bypass_limits, bool test_accept) -{ - return AcceptToMemoryPoolWithTime(Params(), pool, active_chainstate, tx, GetTime(), bypass_limits, test_accept); -} - PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTxMemPool& pool, const Package& package, bool test_accept) { @@ -1161,8 +1166,8 @@ CChainState::CChainState( ChainstateManager& chainman, std::optional<uint256> from_snapshot_blockhash) : m_mempool(mempool), - m_params(::Params()), m_blockman(blockman), + m_params(::Params()), m_chainman(chainman), m_from_snapshot_blockhash(from_snapshot_blockhash) {} @@ -3500,7 +3505,7 @@ MempoolAcceptResult ChainstateManager::ProcessTransaction(const CTransactionRef& state.Invalid(TxValidationResult::TX_NO_MEMPOOL, "no-mempool"); return MempoolAcceptResult::Failure(state); } - auto result = AcceptToMemoryPool(active_chainstate, *active_chainstate.m_mempool, tx, /*bypass_limits=*/ false, test_accept); + auto result = AcceptToMemoryPool(*active_chainstate.m_mempool, active_chainstate, tx, GetTime(), /* bypass_limits= */ false, test_accept); active_chainstate.m_mempool->check(active_chainstate.CoinsTip(), active_chainstate.m_chain.Height() + 1); return result; } @@ -4530,7 +4535,6 @@ static const uint64_t MEMPOOL_DUMP_VERSION = 1; bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mockable_fopen_function) { - const CChainParams& chainparams = Params(); int64_t nExpiryTimeout = gArgs.GetIntArg("-mempoolexpiry", DEFAULT_MEMPOOL_EXPIRY) * 60 * 60; FILE* filestr{mockable_fopen_function(gArgs.GetDataDirNet() / "mempool.dat", "rb")}; CAutoFile file(filestr, SER_DISK, CLIENT_VERSION); @@ -4568,8 +4572,8 @@ bool LoadMempool(CTxMemPool& pool, CChainState& active_chainstate, FopenFn mocka } if (nTime > nNow - nExpiryTimeout) { LOCK(cs_main); - if (AcceptToMemoryPoolWithTime(chainparams, pool, active_chainstate, tx, nTime, false /* bypass_limits */, - false /* test_accept */).m_result_type == MempoolAcceptResult::ResultType::VALID) { + if (AcceptToMemoryPool(pool, active_chainstate, tx, nTime, /* bypass_limits= */ false, + /* test_accept= */ false).m_result_type == MempoolAcceptResult::ResultType::VALID) { ++count; } else { // mempool may contain the transaction already, e.g. from diff --git a/src/validation.h b/src/validation.h index 21cd389757..881438f37a 100644 --- a/src/validation.h +++ b/src/validation.h @@ -208,19 +208,23 @@ struct PackageMempoolAcceptResult }; /** - * Try to add a transaction to the mempool. This is an internal function and is - * exposed only for testing. Client code should use ChainstateManager::ProcessTransaction() + * Try to add a transaction to the mempool. This is an internal function and is exposed only for testing. + * Client code should use ChainstateManager::ProcessTransaction() * - * @param[in] active_chainstate Reference to the active chainstate. * @param[in] pool Reference to the node's mempool. + * @param[in] active_chainstate Reference to the active chainstate. * @param[in] tx The transaction to submit for mempool acceptance. + * @param[in] accept_time The timestamp for adding the transaction to the mempool. Usually + * the current system time, but may be different. + * It is also used to determine when the entry expires. * @param[in] bypass_limits When true, don't enforce mempool fee and capacity limits. * @param[in] test_accept When true, run validation checks but don't submit to mempool. * * @returns a MempoolAcceptResult indicating whether the transaction was accepted/rejected with reason. */ -MempoolAcceptResult AcceptToMemoryPool(CChainState& active_chainstate, CTxMemPool& pool, const CTransactionRef& tx, - bool bypass_limits, bool test_accept=false) EXCLUSIVE_LOCKS_REQUIRED(cs_main); +MempoolAcceptResult AcceptToMemoryPool(CTxMemPool& pool, CChainState& active_chainstate, const CTransactionRef& tx, + int64_t accept_time, bool bypass_limits, bool test_accept) + EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** * Atomically test acceptance of a package. If the package only contains one tx, package rules still @@ -250,11 +254,6 @@ PackageMempoolAcceptResult ProcessNewPackage(CChainState& active_chainstate, CTx bool CheckFinalTx(const CBlockIndex* active_chain_tip, const CTransaction &tx, int flags = -1) EXCLUSIVE_LOCKS_REQUIRED(cs_main); /** - * Test whether the LockPoints height and time are still valid on the current chain - */ -bool TestLockPointValidity(CChain& active_chain, const LockPoints* lp) EXCLUSIVE_LOCKS_REQUIRED(cs_main); - -/** * Check if transaction will be BIP68 final in the next block to be created on top of tip. * @param[in] tip Chain tip to check tx sequence locks against. For example, * the tip of the current active chain. @@ -581,8 +580,6 @@ protected: //! Only the active chainstate has a mempool. CTxMemPool* m_mempool; - const CChainParams& m_params; - //! Manages the UTXO set, which is a reflection of the contents of `m_chain`. std::unique_ptr<CoinsViews> m_coins_views; @@ -591,6 +588,9 @@ public: //! CChainState instances. BlockManager& m_blockman; + /** Chain parameters for this chainstate */ + const CChainParams& m_params; + //! The chainstate manager that owns this chainstate. The reference is //! necessary so that this instance can check whether it is the active //! chainstate within deeply nested method calls. diff --git a/src/wallet/rpcwallet.cpp b/src/wallet/rpcwallet.cpp index beda86e4b4..22ce271816 100644 --- a/src/wallet/rpcwallet.cpp +++ b/src/wallet/rpcwallet.cpp @@ -3162,7 +3162,7 @@ static std::vector<RPCArg> FundTxDoc() {"conf_target", RPCArg::Type::NUM, RPCArg::DefaultHint{"wallet -txconfirmtarget"}, "Confirmation target in blocks"}, {"estimate_mode", RPCArg::Type::STR, RPCArg::Default{"unset"}, std::string() + "The fee estimate mode, must be one of (case insensitive):\n" " \"" + FeeModes("\"\n\"") + "\""}, - {"replaceable", RPCArg::Type::BOOL, RPCArg::DefaultHint{"wallet default"}, "Marks this transaction as BIP125 replaceable.\n" + {"replaceable", RPCArg::Type::BOOL, RPCArg::DefaultHint{"wallet default"}, "Marks this transaction as BIP125-replaceable.\n" "Allows this transaction to be replaced by a transaction with higher fees"}, {"solving_data", RPCArg::Type::OBJ, RPCArg::Optional::OMITTED_NAMED_ARG, "Keys and scripts needed for producing a final transaction with a dummy signature.\n" "Used for fee estimation during coin selection.", diff --git a/test/lint/lint-circular-dependencies.sh b/test/lint/lint-circular-dependencies.sh index ab3866d23e..69185090d1 100755 --- a/test/lint/lint-circular-dependencies.sh +++ b/test/lint/lint-circular-dependencies.sh @@ -15,12 +15,10 @@ EXPECTED_CIRCULAR_DEPENDENCIES=( "index/base -> validation -> index/blockfilterindex -> index/base" "index/coinstatsindex -> node/coinstats -> index/coinstatsindex" "policy/fees -> txmempool -> policy/fees" - "policy/rbf -> txmempool -> validation -> policy/rbf" "qt/addresstablemodel -> qt/walletmodel -> qt/addresstablemodel" "qt/recentrequeststablemodel -> qt/walletmodel -> qt/recentrequeststablemodel" "qt/sendcoinsdialog -> qt/walletmodel -> qt/sendcoinsdialog" "qt/transactiontablemodel -> qt/walletmodel -> qt/transactiontablemodel" - "txmempool -> validation -> txmempool" "wallet/fees -> wallet/wallet -> wallet/fees" "wallet/wallet -> wallet/walletdb -> wallet/wallet" "node/coinstats -> validation -> node/coinstats" |