Remove dangles

instagibbs · Sep 29, 2023 · 0cf4c46 · 0cf4c46
1 parent 5a647bb
commit 0cf4c46
Show file tree

Hide file tree

Showing 3 changed files with 20 additions and 21 deletions.
diff --git 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<std::vector<CTransactionRef>> 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<CTransactionRef> 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<std::pair<CAmount, int64_t>> 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<uint256>& 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<std::reference_wrapper<PackageEntry>> 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
@@ -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<uint32_t> 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<std::vector<CTransactionRef>> 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<std::pair<CAmount, int64_t>> FilteredAncestorFeeAndVsize(const CTransactionRef& tx) const;
 

diff --git 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]};