diff --git a/src/net_processing.cpp b/src/net_processing.cpp index 34f108f9016ef..55d4e35f28e90 100644 --- a/src/net_processing.cpp +++ b/src/net_processing.cpp @@ -636,6 +636,9 @@ class PeerManagerImpl final : public PeerManager std::optional Find1P1CPackage(const CTransactionRef& ptx, NodeId nodeid) EXCLUSIVE_LOCKS_REQUIRED(!m_peer_mutex, g_msgproc_mutex, cs_main); + std::optional 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. * @@ -1076,12 +1079,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 vExtraTxnForCompact GUARDED_BY(g_msgproc_mutex); + std::vector 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; @@ -1880,16 +1886,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(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::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); @@ -3208,7 +3233,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); } } @@ -3245,7 +3270,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); } } @@ -4577,7 +4603,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. @@ -4624,8 +4650,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(/*orphan_child=*/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.value(), package_result); + + // 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. diff --git a/test/functional/p2p_opportunistic_1p1c.py b/test/functional/p2p_opportunistic_1p1c.py index e07acd548136f..80f49f93d3e4e 100755 --- a/test/functional/p2p_opportunistic_1p1c.py +++ b/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() diff --git a/test/functional/test_framework/test_framework.py b/test/functional/test_framework/test_framework.py index a2f767cc9898c..7f6de5ac95d98 100755 --- a/test/functional/test_framework/test_framework.py +++ b/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 diff --git a/test/functional/test_runner.py b/test/functional/test_runner.py index 2b0b24ec058c4..0983539c5aa01 100755 --- a/test/functional/test_runner.py +++ b/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',