p2p: Allow 1P1C to fetch parent from compact block extra_txn

One relatively common pattern of 1P1C relay is receiving
a just-below-minfee parent, dropping it and marking it
as reconsiderable, then fetching it again from the peer
once the child is added to the orphanage.

A cache of dropped parents would be useful, and it turns
out we're already opportunistically storing transactions
like this for compact blocks as "extra transactions".
Use this size-limited cache to potentially fetch a
reconsiderable parent, and submit for validation.

src/net_processing.cpp

@@ -639,6 +639,9 @@ class PeerManagerImpl final : public PeerManager
     std::optional<PackageToValidate> Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid)
         EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
 
+    std::optional<PackageToValidate> Find1P1CPackageExtraTxn(const CTransactionRef& orphan_child, NodeId child_sender)
+        EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main);
+
     /**
      * Reconsider orphan transactions after a parent has been accepted to the mempool.
      *
@@ -1079,12 +1082,15 @@ class PeerManagerImpl final : public PeerManager
     /** Storage for orphan information */
     TxOrphanage m_orphanage;
 
-    void AddToCompactExtraTransactions(const CTransactionRef& tx) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
+    /** Opportunistically store rejected or replaced transactions for later evaluation in blocks or packages. */
+    void AddToCompactExtraTransactions(const CTransactionRef& tx, NodeId sender) EXCLUSIVE_LOCKS_REQUIRED(g_msgproc_mutex);
 
     /** Orphan/conflicted/etc transactions that are kept for compact block reconstruction.
      *  The last -blockreconstructionextratxn/DEFAULT_BLOCK_RECONSTRUCTION_EXTRA_TXN of
      *  these are kept in a ring buffer */
     std::vector<CTransactionRef> vExtraTxnForCompact GUARDED_BY(g_msgproc_mutex);
+    std::vector<NodeId> m_extra_txn_senders GUARDED_BY(g_msgproc_mutex);
+
     /** Offset into vExtraTxnForCompact to insert the next tx */
     size_t vExtraTxnForCompactIt GUARDED_BY(g_msgproc_mutex) = 0;
 
@@ -1883,16 +1889,35 @@ PeerManagerInfo PeerManagerImpl::GetInfo() const
     };
 }
 
-void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx)
+void PeerManagerImpl::AddToCompactExtraTransactions(const CTransactionRef& tx, NodeId sender)
 {
     if (m_opts.max_extra_txs <= 0)
         return;
-    if (!vExtraTxnForCompact.size())
+    if (!vExtraTxnForCompact.size()) {
         vExtraTxnForCompact.resize(m_opts.max_extra_txs);
+        m_extra_txn_senders = std::vector<NodeId>(m_opts.max_extra_txs, -1);
+    }
     vExtraTxnForCompact[vExtraTxnForCompactIt] = tx;
+    m_extra_txn_senders[vExtraTxnForCompactIt] = sender;
     vExtraTxnForCompactIt = (vExtraTxnForCompactIt + 1) % m_opts.max_extra_txs;
 }
 
+std::optional<PeerManagerImpl::PackageToValidate> PeerManagerImpl::Find1P1CPackageExtraTxn(const CTransactionRef& orphan_child,
+                                                                                           NodeId child_sender)
+{
+    for (size_t i = 0; i < vExtraTxnForCompact.size(); i++) {
+        const auto& maybe_parent = vExtraTxnForCompact[i];
+        // We stopped checking it earlier due to fee reasons, try again?
+        if (maybe_parent && m_recent_rejects_reconsiderable.contains(maybe_parent->GetWitnessHash().ToUint256())) {
+            Package maybe_cpfp_package{maybe_parent, orphan_child};
+            if (IsChildWithParents(maybe_cpfp_package) && !m_recent_rejects_reconsiderable.contains(GetPackageHash(maybe_cpfp_package))) {
+                return PeerManagerImpl::PackageToValidate{maybe_parent, orphan_child, /*parent_sender=*/m_extra_txn_senders[i], child_sender};
+            }
+        }
+    }
+    return std::nullopt;
+}
+
 void PeerManagerImpl::Misbehaving(Peer& peer, int howmuch, const std::string& message)
 {
     assert(howmuch > 0);
@@ -3211,7 +3236,7 @@ void PeerManagerImpl::ProcessInvalidTx(NodeId nodeid, const CTransactionRef& ptx
             m_txrequest.ForgetTxHash(ptx->GetHash());
         }
         if (maybe_add_extra_compact_tx && RecursiveDynamicUsage(*ptx) < 100000) {
-            AddToCompactExtraTransactions(ptx);
+            AddToCompactExtraTransactions(ptx, nodeid);
         }
     }
 
@@ -3248,7 +3273,8 @@ void PeerManagerImpl::ProcessValidTx(NodeId nodeid, const CTransactionRef& tx, c
     RelayTransaction(tx->GetHash(), tx->GetWitnessHash());
 
     for (const CTransactionRef& removedTx : replaced_transactions) {
-        AddToCompactExtraTransactions(removedTx);
+        // We don't track who sent replaced transactions; It was good enough to add to our mempool already.
+        AddToCompactExtraTransactions(removedTx, /*sender=*/-1);
     }
 }
 
@@ -4585,7 +4611,7 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
         }
         else if (state.GetResult() == TxValidationResult::TX_MISSING_INPUTS)
         {
-            bool fRejectedParents = false; // It may be the case that the orphans parents have all been rejected
+            bool fRejectedParents = false; // It may be the case that the an orphan parent has been rejected
 
             // Deduplicate parent txids, so that we don't have to loop over
             // the same parent txid more than once down below.
@@ -4632,8 +4658,19 @@ void PeerManagerImpl::ProcessMessage(CNode& pfrom, const std::string& msg_type,
                     if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) AddTxAnnouncement(pfrom, gtxid, current_time);
                 }
 
+                // Maybe we have its parent in extra_txn?
+                if (auto package_to_validate{Find1P1CPackageExtraTxn(/*maybe_parent=*/ptx, pfrom.GetId())}) {
+                    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 from extra_txns: %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);
+
+                    // Ended up succeeding
+                    if (package_result.m_state.IsValid()) return;
+                }
+
                 if (m_orphanage.AddTx(ptx, pfrom.GetId())) {
-                    AddToCompactExtraTransactions(ptx);
+                    AddToCompactExtraTransactions(ptx, pfrom.GetId());
                 }
 
                 // Once added to the orphan pool, a tx is considered AlreadyHave, and we shouldn't request it anymore.

test/functional/p2p_opportunistic_1p1c.py

@@ -56,10 +56,17 @@ class PackageRelayTest(BitcoinTestFramework):
     def set_test_params(self):
         self.setup_clean_chain = True
         self.num_nodes = 1
-        self.extra_args = [[
-            "-datacarriersize=100000",
-            "-maxmempool=5",
-        ]]
+        if self.options.noextratxn:
+            self.extra_args = [[
+                "-datacarriersize=100000",
+                "-maxmempool=5",
+                "-blockreconstructionextratxn=0", # require fetching parent even if cached
+            ]]
+        else:
+            self.extra_args = [[
+                "-datacarriersize=100000",
+                "-maxmempool=5",
+            ]]
         self.supports_cli = False
 
     def create_tx_below_mempoolminfee(self, wallet):
@@ -127,6 +134,13 @@ def test_basic_parent_then_child(self, wallet):
         peer_sender.wait_for_getdata([high_child_wtxid_int])
         peer_sender.send_and_ping(msg_tx(high_fee_child["tx"]))
 
+        if not self.options.noextratxn:
+            # 3. It's in extra transactions. Done!
+            node_mempool = node.getrawmempool()
+            assert low_fee_parent["txid"] in node_mempool
+            assert high_fee_child["txid"] in node_mempool
+            return
+
         # 3. Node requests the missing parent by txid.
         # It should do so even if it has previously rejected that parent for being too low feerate.
         parent_txid_int = int(low_fee_parent["txid"], 16)
@@ -171,12 +185,14 @@ def test_low_and_high_child(self, wallet):
         peer_sender.wait_for_getdata([med_child_wtxid_int])
         peer_sender.send_and_ping(msg_tx(med_fee_child["tx"]))
 
-        # 3. Node requests the orphan's missing parent.
-        parent_txid_int = int(low_fee_parent["txid"], 16)
-        peer_sender.wait_for_getdata([parent_txid_int])
+        # Extra transactions will allow it to go ahead and validate the package
+        if self.options.noextratxn:
+            # 3. Node requests the orphan's missing parent.
+            parent_txid_int = int(low_fee_parent["txid"], 16)
+            peer_sender.wait_for_getdata([parent_txid_int])
 
-        # 4. The low parent + low child are submitted as a package. They are not accepted due to low package feerate.
-        peer_sender.send_and_ping(msg_tx(low_fee_parent["tx"]))
+            # 4. The low parent + low child are submitted as a package. They are not accepted due to low package feerate.
+            peer_sender.send_and_ping(msg_tx(low_fee_parent["tx"]))
 
         assert low_fee_parent["txid"] not in node.getrawmempool()
         assert med_fee_child["txid"] not in node.getrawmempool()
@@ -197,13 +213,15 @@ def test_low_and_high_child(self, wallet):
         peer_sender.wait_for_getdata([high_child_wtxid_int])
         peer_sender.send_and_ping(msg_tx(high_fee_child["tx"]))
 
-        # 6. Node requests the orphan's parent, even though it has already been rejected, both by
-        # itself and with a child. This is necessary, otherwise high_fee_child can be censored.
-        parent_txid_int = int(low_fee_parent["txid"], 16)
-        peer_sender.wait_for_getdata([parent_txid_int])
+        # Extra transactions will allow it to go ahead and validate the package
+        if self.options.noextratxn:
+            # 6. Node requests the orphan's parent, even though it has already been rejected, both by
+            # itself and with a child. This is necessary, otherwise high_fee_child can be censored.
+            parent_txid_int = int(low_fee_parent["txid"], 16)
+            peer_sender.wait_for_getdata([parent_txid_int])
 
-        # 7. The low feerate parent + high feerate child are submitted as a package.
-        peer_sender.send_and_ping(msg_tx(low_fee_parent["tx"]))
+            # 7. The low feerate parent + high feerate child are submitted as a package.
+            peer_sender.send_and_ping(msg_tx(low_fee_parent["tx"]))
 
         # 8. Both transactions should now be in mempool
         node_mempool = node.getrawmempool()

test/functional/test_framework/test_framework.py

@@ -194,6 +194,8 @@ def parse_args(self):
                             help="use BIP324 v2 connections between all nodes by default")
         parser.add_argument("--v1transport", dest="v1transport", default=False, action="store_true",
                             help="Explicitly use v1 transport (can be used to overwrite global --v2transport option)")
+        parser.add_argument("--noextratxn", dest="noextratxn", default=False, action="store_true",
+                            help="No extra transactions for compact blocks")
 
         self.add_options(parser)
         # Running TestShell in a Jupyter notebook causes an additional -f argument

test/functional/test_runner.py

@@ -185,6 +185,7 @@
     'rpc_misc.py',
     'p2p_1p1c_network.py',
     'p2p_opportunistic_1p1c.py',
+    'p2p_opportunistic_1p1c.py --noextratxn',
     'interface_rest.py',
     'mempool_spend_coinbase.py',
     'wallet_avoid_mixing_output_types.py --descriptors',