Merge bitcoin/bitcoin#30012: opportunistic 1p1c followups

7f6fb73c82 [refactor] use reference in for loop through iters (glozow)
6119f76ef7 Process every MempoolAcceptResult regardless of PackageValidationResult (glozow)
2b482dc1f3 [refactor] have ProcessPackageResult take a PackageToValidate (glozow)
c2ada05307 [doc] remove redundant PackageToValidate comment (glozow)
9a762efc7a [txpackages] use std::lexicographical_compare instead of sorting hex strings (glozow)
8496f69e1c [refactor] make MempoolAcceptResult::m_replaced_transactions non-optional (glozow)

Pull request description:

  Followups from #28970:
  - https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1568781077
  - https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1585554972
  - https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1581019326
  - https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1581036209
  - https://github.com/bitcoin/bitcoin/pull/28970#discussion_r1586258730

ACKs for top commit:
  instagibbs:
    reACK https://github.com/bitcoin/bitcoin/pull/30012/commits/7f6fb73c82fdff2afe5edefcc335ba6707d5627d

Tree-SHA512: 9d8393d5f2fedbc6ebce582ff2a8ed074a02dd8e7dbf562c14d48b439fdc1ee6c3203b3609366d3c883d44655cc1a5c83a75ca56e490d25c1a34d95a0622d458

src/net_processing.cpp

@@ -600,15 +600,6 @@ private:
     void ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, const std::list<CTransactionRef>& 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<NodeId>& 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<NodeId> 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<NodeId>& 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());
+                    ProcessValidTx(nodeid, tx, tx_result.m_replaced_transactions);
-                    std::list<CTransactionRef> empty_replacement_list;
-                    ProcessValidTx(nodeid, tx, tx_result.m_replaced_transactions.value_or(empty_replacement_list));
                     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());
+            ProcessValidTx(peer.m_id, porphanTx, result.m_replaced_transactions);
-            std::list<CTransactionRef> empty_replacement_list;
-            ProcessValidTx(peer.m_id, porphanTx, result.m_replaced_transactions.value_or(empty_replacement_list));
             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());
+            ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions);
-            std::list<CTransactionRef> empty_replacement_list;
-            ProcessValidTx(pfrom.GetId(), ptx, result.m_replaced_transactions.value_or(empty_replacement_list));
             pfrom.m_last_tx_time = GetTime<std::chrono::seconds>();
         }
         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);
             }
         }
 

src/policy/packages.cpp

@@ -156,7 +156,10 @@ uint256 GetPackageHash(const std::vector<CTransactionRef>& 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;

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) {
-                        for (const auto& ptx : it->second.m_replaced_transactions.value()) {
+                        replaced_txids.insert(ptx->GetHash());
-                            replaced_txids.insert(ptx->GetHash());
-                        }
                     }
                     break;
                 }

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.

src/test/util/txmempool.cpp

@@ -68,12 +68,6 @@ std::optional<std::string> 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)) {

src/txorphanage.cpp

@@ -278,7 +278,7 @@ std::vector<CTransactionRef> TxOrphanage::GetChildrenFromSamePeer(const CTransac
     // Convert to a vector of CTransactionRef
     std::vector<CTransactionRef> 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<std::pair<CTransactionRef, NodeId>> TxOrphanage::GetChildrenFromDiff
     // Convert iterators to pair<CTransactionRef, NodeId>
     std::vector<std::pair<CTransactionRef, NodeId>> 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;

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<std::list<CTransactionRef>> m_replaced_transactions;
+    const std::list<CTransactionRef> m_replaced_transactions;
     /** Virtual size as used by the mempool, calculated using serialized size and sigops. */
     const std::optional<int64_t> m_vsize;
     /** Raw base fees in satoshis. */