From 0cf4c46fed552d7825e9b0b6e354b3b56c8a21b9 Mon Sep 17 00:00:00 2001 From: Greg Sanders Date: Fri, 29 Sep 2023 14:24:32 -0400 Subject: [PATCH] Remove dangles --- src/policy/ancestor_packages.cpp | 19 ++++++++----------- src/policy/ancestor_packages.h | 12 ++++-------- src/test/txpackage_tests.cpp | 10 ++++++++-- 3 files changed, 20 insertions(+), 21 deletions(-) diff --git a/src/policy/ancestor_packages.cpp b/src/policy/ancestor_packages.cpp index 082c6eea27400..35de154751080 100644 --- a/src/policy/ancestor_packages.cpp +++ b/src/policy/ancestor_packages.cpp @@ -95,7 +95,7 @@ Package AncestorPackage::FilteredTxns() const { Package result; for (const auto entryref : m_txns) { - if (!entryref.get().skip && !entryref.get().dangles) result.push_back(entryref.get().tx); + if (!entryref.get().skip) result.push_back(entryref.get().tx); } return result; } @@ -105,12 +105,11 @@ std::optional> AncestorPackage::FilteredAncestorSet const auto& entry_it = m_txid_to_entry.find(tx->GetHash()); if (entry_it == m_txid_to_entry.end()) return std::nullopt; const auto& entry = entry_it->second; - if (entry.dangles) return std::nullopt; + if (entry.skip) return std::nullopt; std::vector result; result.reserve(entry.ancestor_subset.size()); for (const auto entryref : m_txns) { if (!entryref.get().skip && entry.ancestor_subset.count(entryref.get().tx->GetHash()) > 0) { - Assume(!entryref.get().dangles); result.push_back(entryref.get().tx); } } @@ -131,13 +130,12 @@ std::optional> AncestorPackage::FilteredAncestorFeeA const auto& entry_it = m_txid_to_entry.find(tx->GetHash()); if (entry_it == m_txid_to_entry.end()) return std::nullopt; const auto& entry = entry_it->second; - if (entry.dangles || !entry.fee.has_value() || !entry.vsize.has_value()) return std::nullopt; + if (entry.skip || !entry.fee.has_value() || !entry.vsize.has_value()) return std::nullopt; CAmount total_fee{0}; int64_t total_vsize{0}; for (const auto& txid : entry.ancestor_subset) { const auto& anc_entry = m_txid_to_entry.at(txid); - Assume(!anc_entry.dangles); - if (!anc_entry.skip && !anc_entry.dangles) { + if (!anc_entry.skip) { // If tx has fee and vsize, then any of its non-skipped ancestors should too. if (anc_entry.fee.has_value() && anc_entry.vsize.has_value()) { total_fee += anc_entry.fee.value(); @@ -162,7 +160,6 @@ void AncestorPackage::SkipWithDescendants(const CTransactionRef& transaction) m_txid_to_entry.at(transaction->GetHash()).skip = true; for (const auto& descendant_txid : m_txid_to_entry.at(transaction->GetHash()).descendant_subset) { m_txid_to_entry.at(descendant_txid).skip = true; - m_txid_to_entry.at(descendant_txid).dangles = true; } } @@ -178,7 +175,7 @@ bool AncestorPackage::LinearizeWithFees() if (!m_ancestor_package_shaped) return false; // All fee and vsize information for non-skipped transactions must be available, otherwise linearization cannot be done. if (!std::all_of(m_txid_to_entry.cbegin(), m_txid_to_entry.cend(), - [](const auto& entry) { return entry.second.skip || entry.second.dangles || + [](const auto& entry) { return entry.second.skip || (entry.second.fee.has_value() && entry.second.vsize.has_value()); })) { return false; } @@ -199,12 +196,12 @@ bool AncestorPackage::LinearizeWithFees() int64_t{0}, [&](int64_t sum, const auto& anc) { return sum + *m_txid_to_entry.at(anc->GetHash()).vsize; }); miniminer_info.push_back(node::MiniMinerMempoolEntry{*entry.fee, ancestor_subset_fees, *entry.vsize, ancestor_subset_vsize, entry.tx}); - // Provide descendant cache, but filter for any transactions that dangle or skip + // Provide descendant cache, but filter for any transactions that skip std::set& descendant_cache_to_populate = descendant_caches.try_emplace(txid).first->second; for (const auto& txid : entry.descendant_subset) { if (!Assume(m_txid_to_entry.count(txid) > 0)) continue; const auto& entry = m_txid_to_entry.at(txid); - if (!entry.dangles && !entry.skip) { + if (!entry.skip) { descendant_cache_to_populate.insert(txid); } } @@ -220,7 +217,7 @@ bool AncestorPackage::LinearizeWithFees() std::vector> txns_copy(m_txns); std::sort(m_txns.begin(), m_txns.end(), CompareEntry()); Assume(std::all_of(m_txid_to_entry.cbegin(), m_txid_to_entry.cend(), [](const auto& entry) { - bool should_have_sequence = !entry.second.skip && !entry.second.dangles; + bool should_have_sequence = !entry.second.skip; return entry.second.mining_sequence.has_value() == should_have_sequence; })); if (!Assume(IsSorted(Txns()))) { diff --git a/src/policy/ancestor_packages.h b/src/policy/ancestor_packages.h index 0fc90dc5249c5..9b18013d775d4 100644 --- a/src/policy/ancestor_packages.h +++ b/src/policy/ancestor_packages.h @@ -33,13 +33,9 @@ class AncestorPackage struct PackageEntry { /** Whether this transaction should be skipped in FilteredAncestorSet() and linearization by - * fees, i.e. because it is already in the mempool. - * This value can be set to true by calling Skip(). */ + * fees, e.g., because it is already in the mempool, or has been rejected for un-retryable reasons. + * This value can be set to true by calling Skip() or SkipWithDescendants(). */ bool skip{false}; - /** Whether this transaction "dangles," i.e. we know nothing about it it because it is - * missing inputs or depends on another transaction that is missing inputs. - * This value can be set to true by calling SkipWithDescendants(). */ - bool dangles{false}; /** This value starts as std::nullopt when we don't have any fee information yet. It can be * updated by calling LinearizeWithFees() if this entry isn't being skipped. */ std::optional mining_sequence; @@ -119,11 +115,11 @@ class AncestorPackage Package FilteredTxns() const; /** Get the sorted, filtered ancestor subpackage for a tx. Includes the tx. Does not - * include skipped ancestors. If this transaction dangles, returns std::nullopt. */ + * include skipped ancestors. If this transaction itself is skipped, returns std::nullopt. */ std::optional> FilteredAncestorSet(const CTransactionRef& tx) const; /** Get the total fee and vsize of the ancestor subpackage for a tx. Includes the tx. Does not - * include skipped ancestors. If this transaction dangles or fee and vsize are + * include skipped ancestors. If this transaction is skipped or fee and vsize are * unavailable, returns std::nullopt. This result is always consistent with FilteredAncestorSet(). */ std::optional> FilteredAncestorFeeAndVsize(const CTransactionRef& tx) const; diff --git a/src/test/txpackage_tests.cpp b/src/test/txpackage_tests.cpp index 3750ff024f979..f8ac33d8886ab 100644 --- a/src/test/txpackage_tests.cpp +++ b/src/test/txpackage_tests.cpp @@ -229,8 +229,14 @@ BOOST_FIXTURE_TEST_CASE(ancestorpackage, TestChain100Setup) m_node.mempool->CalculateDescendants(m_node.mempool->GetIter(tx_20->GetHash()).value(), descendants_20); packageified.Skip(tx_20); for (const auto& desc_iter : descendants_20) { - BOOST_CHECK_EQUAL(packageified.FilteredAncestorSet(m_node.mempool->info(GenTxid::Txid(desc_iter->GetTx().GetHash())).tx)->size(), - desc_iter->GetCountWithAncestors() - 1); + const auto filtered_set = packageified.FilteredAncestorSet(m_node.mempool->info(GenTxid::Txid(desc_iter->GetTx().GetHash())).tx); + if (!filtered_set) { + // If it's itself, it returns nullopt because we marked it as skipped + BOOST_CHECK_EQUAL(desc_iter->GetTx().GetHash(), tx_20->GetHash()); + } else { + BOOST_CHECK_EQUAL(filtered_set->size(), + desc_iter->GetCountWithAncestors() - 1); + } } // SkipWithDescendants the 40th transaction. FilteredAncestorSet() for all of its descendants should return std::nullopt. const auto& tx_40{sorted_transactions[40]};