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

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

Pull request description:

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

  (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 bitcoin/bitcoin@77202f0
  darosior:
    ACK 77202f0
  LarryRuane:
    ACK 77202f0

Tree-SHA512: a428e791dfa59c359d3ccc67e8d3a4c1239815d2f6b29898e129700079271c00b3a45f091f70b65a6e54aa00a3d5b678b6da29d2a76b6cd6f946eaa7082ea696

Author: fanquake

doc/policy/packages.md

@@ -57,3 +57,18 @@ test accepts):
 
    - Warning: Batched fee-bumping may be unsafe for some use cases. Users and application developers
      should take caution if utilizing multi-parent packages.
+
+* Transactions in the package that have the same txid as another transaction already in the mempool
+  will be removed from the package prior to submission ("deduplication").
+
+   - *Rationale*: Node operators are free to set their mempool policies however they please, nodes
+     may receive transactions in different orders, and malicious counterparties may try to take
+     advantage of policy differences to pin or delay propagation of transactions. As such, it's
+     possible for some package transaction(s) to already be in the mempool, and there is no need to
+     repeat validation for those transactions or double-count them in fees.
+
+   - *Rationale*: We want to prevent potential censorship vectors. We should not reject entire
+     packages because we already have one of the transactions. Also, if an attacker first broadcasts
+     a competing package or transaction with a mutated witness, even though the two
+     same-txid-different-witness transactions are conflicting and cannot replace each other, the
+     honest package should still be considered for acceptance.

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

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))) {