diff --git a/src/net_processing.cpp b/src/net_processing.cpp index bdbf077ab5d..34f108f9016 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -600,15 +600,6 @@ private: void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list& replaced_transactions) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main); - /** Handle the results of package validation: calls ProcessValidTx and ProcessInvalidTx for - * individual transactions, and caches rejection for the package as a group. - * @param[in] senders Must contain the nodeids of the peers that provided each transaction - * in package, in the same order. - * */ - void ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, const std::vector& senders) - EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main); - - /** A package to validate */ struct PackageToValidate { const Package m_txns; const std::vector m_senders; @@ -633,6 +624,12 @@ private: } }; + /** Handle the results of package validation: calls ProcessValidTx and ProcessInvalidTx for + * individual transactions, and caches rejection for the package as a group. + */ + void ProcessPackageResult(const PackageToValidate& package_to_validate, const PackageMempoolAcceptResult& package_result) + EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main); + /** Look for a child of this transaction in the orphanage to form a 1-parent-1-child package, * skipping any combinations that have already been tried. Return the resulting package along with * the senders of its respective transactions, or std::nullopt if no package is found. */ @@ -3252,22 +3249,21 @@ void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, c } } -void PeerManagerImpl::ProcessPackageResult(const Package& package, const PackageMempoolAcceptResult& package_result, const std::vector& senders) +void PeerManagerImpl::ProcessPackageResult(const PackageToValidate& package_to_validate, const PackageMempoolAcceptResult& package_result) { AssertLockNotHeld(m_peer_mutex); AssertLockHeld(g_msgproc_mutex); AssertLockHeld(cs_main); + const auto& package = package_to_validate.m_txns; + const auto& senders = package_to_validate.m_senders; + if (package_result.m_state.IsInvalid()) { m_recent_rejects_reconsiderable.insert(GetPackageHash(package)); } // We currently only expect to process 1-parent-1-child packages. Remove if this changes. if (!Assume(package.size() == 2)) return; - // No package results to look through for PCKG_POLICY or PCKG_MEMPOOL_ERROR - if (package_result.m_state.GetResult() == PackageValidationResult::PCKG_POLICY || - package_result.m_state.GetResult() == PackageValidationResult::PCKG_MEMPOOL_ERROR) return; - // Iterate backwards to erase in-package descendants from the orphanage before they become // relevant in AddChildrenToWorkSet. auto package_iter = package.rbegin(); @@ -3276,14 +3272,14 @@ void PeerManagerImpl::ProcessPackageResult(const Package& package, const Package const auto& tx = *package_iter; const NodeId nodeid = *senders_iter; const auto it_result{package_result.m_tx_results.find(tx->GetWitnessHash())}; - if (Assume(it_result != package_result.m_tx_results.end())) { + + // It is not guaranteed that a result exists for every transaction. + if (it_result != package_result.m_tx_results.end()) { const auto& tx_result = it_result->second; switch (tx_result.m_result_type) { case MempoolAcceptResult::ResultType::VALID: { - Assume(tx_result.m_replaced_transactions.has_value()); - std::list empty_replacement_list; - ProcessValidTx(nodeid, tx, tx_result.m_replaced_transactions.value_or(empty_replacement_list)); + ProcessValidTx(nodeid, tx, tx_result.m_replaced_transactions); break; } case MempoolAcceptResult::ResultType::INVALID: @@ -3378,9 +3374,7 @@ bool PeerManagerImpl::ProcessOrphanTx(Peer& peer) if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { LogPrint(BCLog::TXPACKAGES, " accepted orphan tx %s (wtxid=%s)\n", orphanHash.ToString(), orphan_wtxid.ToString()); - Assume(result.m_replaced_transactions.has_value()); - std::list empty_replacement_list; - ProcessValidTx(peer.m_id, porphanTx, result.m_replaced_transactions.value_or(empty_replacement_list)); + ProcessValidTx(peer.m_id, porphanTx, result.m_replaced_transactions); return true; } else if (state.GetResult() != TxValidationResult::TX_MISSING_INPUTS) { LogPrint(BCLog::TXPACKAGES, " invalid orphan tx %s (wtxid=%s) from peer=%d. %s\n", @@ -4553,7 +4547,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, const auto package_result{ProcessNewPackage(m_chainman.ActiveChainstate(), m_mempool, package_to_validate->m_txns, /*test_accept=*/false, /*client_maxfeerate=*/std::nullopt)}; LogDebug(BCLog::TXPACKAGES, "package evaluation for %s: %s\n", package_to_validate->ToString(), package_result.m_state.IsValid() ? "package accepted" : "package rejected"); - ProcessPackageResult(package_to_validate->m_txns, package_result, package_to_validate->m_senders); + ProcessPackageResult(package_to_validate.value(), package_result); } } // If a tx is detected by m_recent_rejects it is ignored. Because we haven't @@ -4578,9 +4572,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, const TxValidationState& state = result.m_state; if (result.m_result_type == MempoolAcceptResult::ResultType::VALID) { - Assume(result.m_replaced_transactions.has_value()); - std::list empty_replacement_list; - ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions.value_or(empty_replacement_list)); + ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions); pfrom.m_last_tx_time = GetTime(); } else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS) @@ -4670,7 +4662,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type, const auto package_result{ProcessNewPackage(m_chainman.ActiveChainstate(), m_mempool, package_to_validate->m_txns, /*test_accept=*/false, /*client_maxfeerate=*/std::nullopt)}; LogDebug(BCLog::TXPACKAGES, "package evaluation for %s: %s\n", package_to_validate->ToString(), package_result.m_state.IsValid() ? "package accepted" : "package rejected"); - ProcessPackageResult(package_to_validate->m_txns, package_result, package_to_validate->m_senders); + ProcessPackageResult(package_to_validate.value(), package_result); } } diff --git a/src/policy/packages.cpp b/src/policy/packages.cpp index 99d2a6d514e..693adcdfd0b 100644 --- a/src/policy/packages.cpp +++ b/src/policy/packages.cpp @@ -156,7 +156,10 @@ uint256 GetPackageHash(const std::vector& transactions) [](const auto& tx){ return tx->GetWitnessHash(); }); // Sort in ascending order - std::sort(wtxids_copy.begin(), wtxids_copy.end(), [](const auto& lhs, const auto& rhs) { return lhs.GetHex() < rhs.GetHex(); }); + std::sort(wtxids_copy.begin(), wtxids_copy.end(), [](const auto& lhs, const auto& rhs) { + return std::lexicographical_compare(std::make_reverse_iterator(lhs.end()), std::make_reverse_iterator(lhs.begin()), + std::make_reverse_iterator(rhs.end()), std::make_reverse_iterator(rhs.begin())); + }); // Get sha256 hash of the wtxids concatenated in this order HashWriter hashwriter; diff --git a/src/rpc/mempool.cpp b/src/rpc/mempool.cpp index 920bb9ea7f4..e6fe3c7ebd9 100644 --- a/src/rpc/mempool.cpp +++ b/src/rpc/mempool.cpp @@ -994,10 +994,8 @@ static RPCHelpMan submitpackage() fees.pushKV("effective-includes", effective_includes_res); } result_inner.pushKV("fees", fees); - if (it->second.m_replaced_transactions.has_value()) { - for (const auto& ptx : it->second.m_replaced_transactions.value()) { - replaced_txids.insert(ptx->GetHash()); - } + for (const auto& ptx : it->second.m_replaced_transactions) { + replaced_txids.insert(ptx->GetHash()); } break; } diff --git a/src/test/fuzz/tx_pool.cpp b/src/test/fuzz/tx_pool.cpp index 0b4019d5eb1..9f0aedf29b8 100644 --- a/src/test/fuzz/tx_pool.cpp +++ b/src/test/fuzz/tx_pool.cpp @@ -139,7 +139,6 @@ void CheckATMPInvariants(const MempoolAcceptResult& res, bool txid_in_mempool, b Assert(wtxid_in_mempool); Assert(res.m_state.IsValid()); Assert(!res.m_state.IsInvalid()); - Assert(res.m_replaced_transactions); Assert(res.m_vsize); Assert(res.m_base_fees); Assert(res.m_effective_feerate); @@ -154,7 +153,6 @@ void CheckATMPInvariants(const MempoolAcceptResult& res, bool txid_in_mempool, b Assert(res.m_state.IsInvalid()); const bool is_reconsiderable{res.m_state.GetResult() == TxValidationResult::TX_RECONSIDERABLE}; - Assert(!res.m_replaced_transactions); Assert(!res.m_vsize); Assert(!res.m_base_fees); // Fee information is provided if the failure is TX_RECONSIDERABLE. diff --git a/src/test/util/txmempool.cpp b/src/test/util/txmempool.cpp index 71cf8aca605..870cdd0b324 100644 --- a/src/test/util/txmempool.cpp +++ b/src/test/util/txmempool.cpp @@ -68,12 +68,6 @@ std::optional CheckPackageMempoolAcceptResult(const Package& txns, return strprintf("tx %s unexpectedly failed: %s", wtxid.ToString(), atmp_result.m_state.ToString()); } - //m_replaced_transactions should exist iff the result was VALID - if (atmp_result.m_replaced_transactions.has_value() != valid) { - return strprintf("tx %s result should %shave m_replaced_transactions", - wtxid.ToString(), valid ? "" : "not "); - } - // m_vsize and m_base_fees should exist iff the result was VALID or MEMPOOL_ENTRY const bool mempool_entry{atmp_result.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY}; if (atmp_result.m_base_fees.has_value() != (valid || mempool_entry)) { diff --git a/src/txorphanage.cpp b/src/txorphanage.cpp index 82206e56c32..92055ded722 100644 --- a/src/txorphanage.cpp +++ b/src/txorphanage.cpp @@ -278,7 +278,7 @@ std::vector TxOrphanage::GetChildrenFromSamePeer(const CTransac // Convert to a vector of CTransactionRef std::vector children_found; children_found.reserve(iters.size()); - for (const auto child_iter : iters) { + for (const auto& child_iter : iters) { children_found.emplace_back(child_iter->second.tx); } return children_found; @@ -310,7 +310,7 @@ std::vector> TxOrphanage::GetChildrenFromDiff // Convert iterators to pair std::vector> children_found; children_found.reserve(iters.size()); - for (const auto child_iter : iters) { + for (const auto& child_iter : iters) { children_found.emplace_back(child_iter->second.tx, child_iter->second.fromPeer); } return children_found; diff --git a/src/validation.h b/src/validation.h index e3b2a2d59b9..28b045fe80c 100644 --- a/src/validation.h +++ b/src/validation.h @@ -113,7 +113,6 @@ void PruneBlockFilesManual(Chainstate& active_chainstate, int nManualPruneHeight *| txid in mempool? | yes | no | no* | yes | yes | *| wtxid in mempool? | yes | no | no* | yes | no | *| m_state | yes, IsValid() | yes, IsInvalid() | yes, IsInvalid() | yes, IsValid() | yes, IsValid() | -*| m_replaced_transactions | yes | no | no | no | no | *| m_vsize | yes | no | no | yes | no | *| m_base_fees | yes | no | no | yes | no | *| m_effective_feerate | yes | yes | no | no | no | @@ -139,7 +138,7 @@ struct MempoolAcceptResult { const TxValidationState m_state; /** Mempool transactions replaced by the tx. */ - const std::optional> m_replaced_transactions; + const std::list m_replaced_transactions; /** Virtual size as used by the mempool, calculated using serialized size and sigops. */ const std::optional m_vsize; /** Raw base fees in satoshis. */