From c6893b0f0b7b205c8da4b9d281a55c9eb843b582 Mon Sep 17 00:00:00 2001 From: glozow Date: Thu, 28 Nov 2024 06:43:08 -0500 Subject: [PATCH] [txdownload] remove unique_parents that we already have This means we no longer return parents we already have in the m_unique_parents result from MempoolRejectedTx. We need to separate the loop that checks AlreadyHave parents from the loop that adds parents as announcements, because we may do the latter loop multiple times for different peers. --- src/node/txdownloadman_impl.cpp | 12 +++++++----- src/test/txdownload_tests.cpp | 9 +++++++-- 2 files changed, 14 insertions(+), 7 deletions(-) diff --git a/src/node/txdownloadman_impl.cpp b/src/node/txdownloadman_impl.cpp index e9fe958f5d..804c00494f 100644 --- a/src/node/txdownloadman_impl.cpp +++ b/src/node/txdownloadman_impl.cpp @@ -348,6 +348,12 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction } } if (!fRejectedParents) { + // Filter parents that we already have. + // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been + // previously rejected for being too low feerate. This orphan might CPFP it. + std::erase_if(unique_parents, [&](const auto& txid){ + return AlreadyHaveTx(GenTxid::Txid(txid), /*include_reconsiderable=*/false); + }); const auto current_time{GetTime()}; for (const uint256& parent_txid : unique_parents) { @@ -357,11 +363,7 @@ node::RejectedTxTodo TxDownloadManagerImpl::MempoolRejectedTx(const CTransaction // Eventually we should replace this with an improved // protocol for getting all unconfirmed parents. const auto gtxid{GenTxid::Txid(parent_txid)}; - // Exclude m_lazy_recent_rejects_reconsiderable: the missing parent may have been - // previously rejected for being too low feerate. This orphan might CPFP it. - if (!AlreadyHaveTx(gtxid, /*include_reconsiderable=*/false)) { - AddTxAnnouncement(nodeid, gtxid, current_time, /*p2p_inv=*/false); - } + AddTxAnnouncement(nodeid, gtxid, current_time, /*p2p_inv=*/false); } // Potentially flip add_extra_compact_tx to false if AddTx returns false because the tx was already there diff --git a/src/test/txdownload_tests.cpp b/src/test/txdownload_tests.cpp index 6d277b5629..88064b5a5f 100644 --- a/src/test/txdownload_tests.cpp +++ b/src/test/txdownload_tests.cpp @@ -231,10 +231,14 @@ BOOST_FIXTURE_TEST_CASE(handle_missing_inputs, TestChain100Setup) // it's in RecentRejectsFilter. Specifically, the parent is allowed to be in // RecentRejectsReconsiderableFilter, but it cannot be in RecentRejectsFilter. const bool expect_keep_orphan = !parent_recent_rej; + const unsigned int expected_parents = parent_recent_rej || parent_recent_conf || parent_in_mempool ? 0 : 1; + // If we don't expect to keep the orphan then expected_parents is 0. + // !expect_keep_orphan => (expected_parents == 0) + BOOST_CHECK(expect_keep_orphan || expected_parents == 0); const auto ret_1p1c = txdownload_impl.MempoolRejectedTx(orphan, state_orphan, nodeid, /*first_time_failure=*/true); std::string err_msg; const bool ok = CheckOrphanBehavior(txdownload_impl, orphan, ret_1p1c, err_msg, - /*expect_orphan=*/expect_keep_orphan, /*expect_keep=*/true, /*expected_parents=*/expect_keep_orphan ? 1 : 0); + /*expect_orphan=*/expect_keep_orphan, /*expect_keep=*/true, /*expected_parents=*/expected_parents); BOOST_CHECK_MESSAGE(ok, err_msg); } @@ -278,11 +282,12 @@ BOOST_FIXTURE_TEST_CASE(handle_missing_inputs, TestChain100Setup) for (int32_t i = 1; i < num_parents; ++i) { txdownload_impl.RecentConfirmedTransactionsFilter().insert(parents[i]->GetHash().ToUint256()); } + const unsigned int expected_parents = 1; const auto ret_1recon_conf = txdownload_impl.MempoolRejectedTx(orphan, state_orphan, nodeid, /*first_time_failure=*/true); std::string err_msg; const bool ok = CheckOrphanBehavior(txdownload_impl, orphan, ret_1recon_conf, err_msg, - /*expect_orphan=*/true, /*expect_keep=*/true, /*expected_parents=*/num_parents); + /*expect_orphan=*/true, /*expect_keep=*/true, /*expected_parents=*/expected_parents); BOOST_CHECK_MESSAGE(ok, err_msg); }