Merge bitcoin/bitcoin#24310: docs / fixups from RBF and packages

77202f0554dcbbbb167d0ed3927cca0bf4609ce8 [doc] package deduplication (glozow)
d35a3cb3968d7584c7d5c42b121a80f34ea656bf [doc] clarify inaccurate comment about replacements paying higher feerate (glozow)
5ae187f8761f5f85a1ef41d24f75afb7eecf366f [validation] look up transaction by txid (glozow)

Pull request description:

  - Use txid, not wtxid, for `mempool.GetIter()`: https://github.com/bitcoin/bitcoin/pull/22674#discussion_r772934994
  - Fix a historically inaccurate comment about RBF during the refactors: https://github.com/bitcoin/bitcoin/pull/22855#discussion_r777130441
  - Add a section about package deduplication to policy/packages.md: https://github.com/bitcoin/bitcoin/pull/24152#discussion_r802955759 and https://github.com/bitcoin/bitcoin/pull/24152#discussion_r802723149

  (I'm intending for this to be in v23 since it's fixups for things that are already merged, which is why I split it from #24152)

ACKs for top commit:
  t-bast:
    LGTM, ACK https://github.com/bitcoin/bitcoin/pull/24310/commits/77202f0554dcbbbb167d0ed3927cca0bf4609ce8
  darosior:
    ACK 77202f0554dcbbbb167d0ed3927cca0bf4609ce8
  LarryRuane:
    ACK 77202f0554dcbbbb167d0ed3927cca0bf4609ce8

Tree-SHA512: a428e791dfa59c359d3ccc67e8d3a4c1239815d2f6b29898e129700079271c00b3a45f091f70b65a6e54aa00a3d5b678b6da29d2a76b6cd6f946eaa7082ea696

2 files changed, 21 insertions, 7 deletions

diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp
index 560efb6b42..6013080dc6 100644
--- a/src/test/txpackage_tests.cpp
+++ b/src/test/txpackage_tests.cpp
@@ -413,6 +413,17 @@ BOOST_FIXTURE_TEST_CASE(package_witness_swap_tests, TestChain100Setup)
 
         BOOST_CHECK(m_node.mempool->exists(GenTxid::Txid(ptx_child2->GetHash())));
         BOOST_CHECK(!m_node.mempool->exists(GenTxid::Wtxid(ptx_child2->GetWitnessHash())));
+
+        // Deduplication should work when wtxid != txid. Submit package with the already-in-mempool
+        // transactions again, which should not fail.
+        const auto submit_segwit_dedup = ProcessNewPackage(m_node.chainman->ActiveChainstate(), *m_node.mempool,
+                                                           {ptx_parent, ptx_child1}, /*test_accept=*/ false);
+        BOOST_CHECK_MESSAGE(submit_segwit_dedup.m_state.IsValid(),
+                            "Package validation unexpectedly failed: " << submit_segwit_dedup.m_state.GetRejectReason());
+        auto it_parent_dup = submit_segwit_dedup.m_tx_results.find(ptx_parent->GetWitnessHash());
+        auto it_child_dup = submit_segwit_dedup.m_tx_results.find(ptx_child1->GetWitnessHash());
+        BOOST_CHECK(it_parent_dup->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
+        BOOST_CHECK(it_child_dup->second.m_result_type == MempoolAcceptResult::ResultType::MEMPOOL_ENTRY);
     }
 
     // Try submitting Package1{child2, grandchild} where child2 is same-txid-different-witness as
diff --git a/src/validation.cpp b/src/validation.cpp
index 2813b62462..86d0022947 100644
--- a/src/validation.cpp
+++ b/src/validation.cpp
@@ -915,12 +915,15 @@ bool MemPoolAccept::ReplacementChecks(Workspace& ws)
     TxValidationState& state = ws.m_state;
 
     CFeeRate newFeeRate(ws.m_modified_fees, ws.m_vsize);
-    // It's possible that the replacement pays more fees than its direct conflicts but not more
-    // than all conflicts (i.e. the direct conflicts have high-fee descendants). However, if the
-    // replacement doesn't pay more fees than its direct conflicts, then we can be sure it's not
-    // more economically rational to mine. Before we go digging through the mempool for all
-    // transactions that would need to be removed (direct conflicts and all descendants), check
-    // that the replacement transaction pays more than its direct conflicts.
+    // The replacement transaction must have a higher feerate than its direct conflicts.
+    // - The motivation for this check is to ensure that the replacement transaction is preferable for
+    //   block-inclusion, compared to what would be removed from the mempool.
+    // - This logic predates ancestor feerate-based transaction selection, which is why it doesn't
+    //   consider feerates of descendants.
+    // - Note: Ancestor feerate-based transaction selection has made this comparison insufficient to
+    //   guarantee that this is incentive-compatible for miners, because it is possible for a
+    //   descendant transaction of a direct conflict to pay a higher feerate than the transaction that
+    //   might replace them, under these rules.
     if (const auto err_string{PaysMoreThanConflicts(ws.m_iters_conflicting, newFeeRate, hash)}) {
         return state.Invalid(TxValidationResult::TX_MEMPOOL_POLICY, "insufficient fee", *err_string);
     }
@@ -1318,7 +1321,7 @@ PackageMempoolAcceptResult MemPoolAccept::AcceptPackage(const Package& package,
         // we know is that the inputs aren't available.
         if (m_pool.exists(GenTxid::Wtxid(wtxid))) {
             // Exact transaction already exists in the mempool.
-            auto iter = m_pool.GetIter(wtxid);
+            auto iter = m_pool.GetIter(txid);
             assert(iter != std::nullopt);
             results.emplace(wtxid, MempoolAcceptResult::MempoolTx(iter.value()->GetTxSize(), iter.value()->GetFee()));
         } else if (m_pool.exists(GenTxid::Txid(txid))) {