diff --git a/src/test/fuzz/txgraph.cpp b/src/test/fuzz/txgraph.cpp index cc20f9e3c4f..976839464b7 100644 --- a/src/test/fuzz/txgraph.cpp +++ b/src/test/fuzz/txgraph.cpp @@ -146,6 +146,30 @@ struct SimTxGraph if (oversized.has_value() && *oversized) oversized = std::nullopt; } + /** Destroy the transaction from the graph, including from the removed set. This will + * trigger TxGraph::Ref::~Ref. reset_oversize controls whether the cached oversized + * value is cleared (destroying does not clear oversizedness in TxGraph of the main + * graph while staging exists). */ + void DestroyTransaction(TxGraph::Ref* ref, bool reset_oversize) + { + auto pos = Find(ref); + if (pos == MISSING) { + // Wipe the ref, if it exists, from the removed vector. Use std::partition rather + // than std::erase because we don't care about the order of the entries that + // remain. + auto remove = std::partition(removed.begin(), removed.end(), [&](auto& arg) { return arg.get() != ref; }); + removed.erase(remove, removed.end()); + } else { + graph.RemoveTransactions(SetType::Singleton(pos)); + simrevmap.erase(simmap[pos].get()); + simmap[pos].reset(); + // This may invalidate our cached oversized value. + if (reset_oversize && oversized.has_value() && *oversized) { + oversized = std::nullopt; + } + } + } + /** Construct the set with all positions in this graph corresponding to the specified * TxGraph::Refs. All of them must occur in this graph and not be removed. */ SetType MakeSet(std::span arg) @@ -327,9 +351,9 @@ FUZZ_TARGET(txgraph) } break; } else if (sel_sim.removed.size() > 0 && command-- == 0) { - // ~Ref. Destroying a TxGraph::Ref has an observable effect on the TxGraph it - // refers to, so this simulation permits doing so separately from other actions on - // TxGraph. + // ~Ref (of an already-removed transaction). Destroying a TxGraph::Ref has an + // observable effect on the TxGraph it refers to, so this simulation permits doing + // so separately from other actions on TxGraph. // Pick a Ref of sel_sim.removed to destroy. Note that the same Ref may still occur // in the other graph, and thus not actually trigger ~Ref yet (which is exactly @@ -341,6 +365,28 @@ FUZZ_TARGET(txgraph) } sel_sim.removed.pop_back(); break; + } else if (command-- == 0) { + // ~Ref (of any transaction). + std::vector to_destroy; + to_destroy.push_back(pick_fn()); + while (true) { + // Keep adding either the ancestors or descendants the already picked + // transactions have in both graphs (main and staging) combined. Destroying + // will trigger deletions in both, so to have consistent TxGraph behavior, the + // set must be closed under ancestors, or descendants, in both graphs. + auto old_size = to_destroy.size(); + for (auto& sim : sims) sim.IncludeAncDesc(to_destroy, alt); + if (to_destroy.size() == old_size) break; + } + // The order in which these ancestors/descendants are destroyed should not matter; + // randomly shuffle them. + std::shuffle(to_destroy.begin(), to_destroy.end(), rng); + for (TxGraph::Ref* ptr : to_destroy) { + for (size_t level = 0; level < sims.size(); ++level) { + sims[level].DestroyTransaction(ptr, level == sims.size() - 1); + } + } + break; } else if (command-- == 0) { // SetTransactionFee. int64_t fee; @@ -457,6 +503,10 @@ FUZZ_TARGET(txgraph) // AbortStaging. real->AbortStaging(); sims.pop_back(); + // Reset the cached oversized value (if TxGraph::Ref destructions triggered + // removals of main transactions while staging was active, then aborting will + // cause it to be re-evaluated in TxGraph). + sims.back().oversized = std::nullopt; break; } } @@ -537,13 +587,4 @@ FUZZ_TARGET(txgraph) // Sanity check again (because invoking inspectors may modify internal unobservable state). real->SanityCheck(); - - // Remove all remaining transactions, because Refs cannot be destroyed otherwise (this will be - // addressed in a follow-up commit). - for (auto& sim : sims) { - for (auto i : sim.graph.Positions()) { - auto ref = sim.GetRef(i); - real->RemoveTransaction(*ref); - } - } } diff --git a/src/txgraph.cpp b/src/txgraph.cpp index e1ea1f9ea58..7312a402bf3 100644 --- a/src/txgraph.cpp +++ b/src/txgraph.cpp @@ -225,7 +225,8 @@ private: /** Information about the merges to be performed, if known. */ std::optional m_group_data = GroupData{}; /** Which entries were removed in this ClusterSet (so they can be wiped on abort). This - * includes all entries which have an (R) removed locator at this level (staging only). */ + * includes all entries which have an (R) removed locator at this level (staging only), + * plus optionally any transaction in m_unlinked. */ std::vector m_removed; /** Total number of transactions in this graph (sum of all transaction counts in all * Clusters, and for staging also those inherited from the main ClusterSet). */ @@ -368,8 +369,34 @@ public: auto& entry = m_entries[idx]; Assume(entry.m_ref != nullptr); entry.m_ref = nullptr; + // Mark the transaction as to be removed in all levels where it explicitly or implicitly + // exists. + bool exists_anywhere{false}; + bool exists{false}; + for (int level = 0; level <= GetTopLevel(); ++level) { + if (entry.m_locator[level].IsPresent()) { + exists_anywhere = true; + exists = true; + } else if (entry.m_locator[level].IsRemoved()) { + exists = false; + } + if (exists) { + auto& clusterset = GetClusterSet(level); + clusterset.m_to_remove.push_back(idx); + // Force recomputation of grouping data. + clusterset.m_group_data = std::nullopt; + // Do not wipe the oversized state of main if staging exists. The reason for this + // is that the alternative would mean that cluster merges may need to be applied to + // a formerly-oversized main graph while staging exists (to satisfy chunk feerate + // queries into main, for example), and such merges could conflict with pulls of + // some of their constituents into staging. + if (level == GetTopLevel() && clusterset.m_oversized == true) { + clusterset.m_oversized = std::nullopt; + } + } + } m_unlinked.push_back(idx); - Compact(); + if (!exists_anywhere) Compact(); } // Functions related to various normalization/application steps. @@ -382,12 +409,12 @@ public: * transactions from inherited (P,M) to explicit (P,P). */ Cluster* PullIn(Cluster* cluster) noexcept; /** Apply all removals queued up in m_to_remove to the relevant Clusters (which get a - * NEEDS_SPLIT* QualityLevel) in the specified level. */ - void ApplyRemovals(int level) noexcept; - /** Split an individual cluster (which must be in the top-level ClusterSet). */ + * NEEDS_SPLIT* QualityLevel) up to the specified level. */ + void ApplyRemovals(int up_to_level) noexcept; + /** Split an individual cluster. */ void Split(Cluster& cluster) noexcept; - /** Split all clusters that need splitting in the specified level. */ - void SplitAll(int level) noexcept; + /** Split all clusters that need splitting up to the specified level. */ + void SplitAll(int up_to_level) noexcept; /** Populate m_group_data based on m_deps_to_add in the specified level. */ void GroupClusters(int level) noexcept; /** Merge the specified clusters. */ @@ -451,6 +478,14 @@ void TxGraphImpl::ClearLocator(int level, GraphIndex idx) noexcept } // Update the transaction count. --clusterset.m_txcount; + // If clearing main, adjust the status of Locators of this transaction in staging, if it exists. + if (level == 0 && GetTopLevel() == 1) { + if (entry.m_locator[1].IsRemoved()) { + entry.m_locator[1].SetMissing(); + } else if (!entry.m_locator[1].IsPresent()) { + --m_staging_clusterset->m_txcount; + } + } } void Cluster::Updated(TxGraphImpl& graph) noexcept @@ -505,8 +540,9 @@ std::vector TxGraphImpl::GetConflicts() const noexcept // All main Clusters containing transactions in m_removed (so (P,R) ones) are conflicts. for (auto i : clusterset.m_removed) { auto& entry = m_entries[i]; - Assume(entry.m_locator[0].IsPresent()); - ret.push_back(entry.m_locator[0].cluster); + if (entry.m_locator[0].IsPresent()) { + ret.push_back(entry.m_locator[0].cluster); + } } // Then go over all Clusters at this level, and find their conflicts (the (P,P) ones). for (int quality = 0; quality < int(QualityLevel::NONE); ++quality) { @@ -873,39 +909,41 @@ Cluster* TxGraphImpl::PullIn(Cluster* cluster) noexcept return cluster; } -void TxGraphImpl::ApplyRemovals(int level) noexcept +void TxGraphImpl::ApplyRemovals(int up_to_level) noexcept { - auto& clusterset = GetClusterSet(level); - auto& to_remove = clusterset.m_to_remove; - // Skip if there is nothing to remove. - if (to_remove.empty()) return; - // There cannot be removals to be applied in main when staging exists (they should have been - // applied in StartStaging already, and none can be added to main while staging exists). - Assume(level == GetTopLevel()); - // Pull in all Clusters that are not in staging. - for (GraphIndex index : to_remove) { - auto cluster = FindCluster(index, level); - PullIn(cluster); - } - // Group the set of to-be-removed entries by Cluster*. - std::sort(to_remove.begin(), to_remove.end(), [&](GraphIndex a, GraphIndex b) noexcept { - return std::less{}(m_entries[a].m_locator[level].cluster, m_entries[b].m_locator[level].cluster); - }); - // Process per Cluster. - std::span to_remove_span{to_remove}; - while (!to_remove_span.empty()) { - Cluster* cluster = m_entries[to_remove_span.front()].m_locator[level].cluster; - if (cluster != nullptr) { - // If the first to_remove_span entry's Cluster exists, hand to_remove_span to it, so it - // can pop off whatever applies to it. - cluster->ApplyRemovals(*this, to_remove_span); - } else { - // Otherwise, skip this already-removed entry. This may happen when RemoveTransaction - // was called twice on the same Ref. - to_remove_span = to_remove_span.subspan(1); + Assume(up_to_level >= 0 && up_to_level <= GetTopLevel()); + for (int level = 0; level <= up_to_level; ++level) { + auto& clusterset = GetClusterSet(level); + auto& to_remove = clusterset.m_to_remove; + // Skip if there is nothing to remove in this level. + if (to_remove.empty()) continue; + // Pull in all Clusters that are not in staging. + if (level == 1) { + for (GraphIndex index : to_remove) { + auto cluster = FindCluster(index, level); + if (cluster != nullptr) PullIn(cluster); + } } + // Group the set of to-be-removed entries by Cluster*. + std::sort(to_remove.begin(), to_remove.end(), [&](GraphIndex a, GraphIndex b) noexcept { + return std::less{}(m_entries[a].m_locator[level].cluster, m_entries[b].m_locator[level].cluster); + }); + // Process per Cluster. + std::span to_remove_span{to_remove}; + while (!to_remove_span.empty()) { + Cluster* cluster = m_entries[to_remove_span.front()].m_locator[level].cluster; + if (cluster != nullptr) { + // If the first to_remove_span entry's Cluster exists, hand to_remove_span to it, so it + // can pop off whatever applies to it. + cluster->ApplyRemovals(*this, to_remove_span); + } else { + // Otherwise, skip this already-removed entry. This may happen when + // RemoveTransaction was called twice on the same Ref, for example. + to_remove_span = to_remove_span.subspan(1); + } + } + to_remove.clear(); } - to_remove.clear(); Compact(); } @@ -919,7 +957,7 @@ void TxGraphImpl::SwapIndexes(GraphIndex a, GraphIndex b) noexcept for (int i = 0; i < 2; ++i) { GraphIndex idx = i ? b : a; Entry& entry = m_entries[idx]; - // Update linked Ref. + // Update linked Ref, if any exists. if (entry.m_ref) GetRefIndex(*entry.m_ref) = idx; // Update the locators for both levels. The rest of the Entry information will not change, // so no need to invoke Cluster::Updated(). @@ -986,14 +1024,17 @@ void TxGraphImpl::Split(Cluster& cluster) noexcept } } -void TxGraphImpl::SplitAll(int level) noexcept +void TxGraphImpl::SplitAll(int up_to_level) noexcept { + Assume(up_to_level >= 0 && up_to_level <= GetTopLevel()); // Before splitting all Cluster, first make sure all removals are applied. - ApplyRemovals(level); - for (auto quality : {QualityLevel::NEEDS_SPLIT, QualityLevel::NEEDS_SPLIT_ACCEPTABLE}) { - auto& queue = GetClusterSet(level).m_clusters[int(quality)]; - while (!queue.empty()) { - Split(*queue.back().get()); + ApplyRemovals(up_to_level); + for (int level = 0; level <= up_to_level; ++level) { + for (auto quality : {QualityLevel::NEEDS_SPLIT, QualityLevel::NEEDS_SPLIT_ACCEPTABLE}) { + auto& queue = GetClusterSet(level).m_clusters[int(quality)]; + while (!queue.empty()) { + Split(*queue.back().get()); + } } } } @@ -1003,10 +1044,6 @@ void TxGraphImpl::GroupClusters(int level) noexcept auto& clusterset = GetClusterSet(level); // If the groupings have been computed already, nothing is left to be done. if (clusterset.m_group_data.has_value()) return; - // We should never need to compute main grouping while staging exists (it should have already - // been computing in StartStaging, and no modifications that invalidate it can be made while - // staging exists). - Assume(level == GetTopLevel()); // Before computing which Clusters need to be merged together, first apply all removals and // split the Clusters into connected components. If we would group first, we might end up @@ -1256,8 +1293,10 @@ void TxGraphImpl::ApplyDependencies(int level) noexcept auto cluster_span = std::span{clusterset.m_group_data->m_group_clusters} .subspan(group_data.m_cluster_offset, group_data.m_cluster_count); // Pull in all the Clusters that contain dependencies. - for (Cluster*& cluster : cluster_span) { - cluster = PullIn(cluster); + if (level == 1) { + for (Cluster*& cluster : cluster_span) { + cluster = PullIn(cluster); + } } // Invoke Merge() to merge them into a single Cluster. Merge(cluster_span); @@ -1398,6 +1437,7 @@ std::vector Cluster::GetAncestorRefs(const TxGraphImpl& graph, De // Translate all ancestors (in arbitrary order) to Refs (if they have any), and return them. for (auto idx : m_depgraph.Ancestors(idx)) { const auto& entry = graph.m_entries[m_mapping[idx]]; + Assume(entry.m_ref != nullptr); ret.push_back(entry.m_ref); } return ret; @@ -1410,6 +1450,7 @@ std::vector Cluster::GetDescendantRefs(const TxGraphImpl& graph, // Translate all descendants (in arbitrary order) to Refs (if they have any), and return them. for (auto idx : m_depgraph.Descendants(idx)) { const auto& entry = graph.m_entries[m_mapping[idx]]; + Assume(entry.m_ref != nullptr); ret.push_back(entry.m_ref); } return ret; @@ -1422,6 +1463,7 @@ std::vector Cluster::GetClusterRefs(const TxGraphImpl& graph) noe // Translate all transactions in the Cluster (in linearization order) to Refs. for (auto idx : m_linearization) { const auto& entry = graph.m_entries[m_mapping[idx]]; + Assume(entry.m_ref != nullptr); ret.push_back(entry.m_ref); } return ret; @@ -1513,7 +1555,8 @@ FeePerWeight TxGraphImpl::GetIndividualFeerate(const Ref& arg) noexcept // be identical if it occurs in both), and return the empty FeePerWeight if it isn't in any. Cluster* cluster{nullptr}; for (int level = 0; level <= GetTopLevel(); ++level) { - // Apply removals, so that we can correctly report FeePerWeight{} for non-existing transaction. + // Apply removals, so that we can correctly report FeePerWeight{} for non-existing + // transactions. ApplyRemovals(level); if (m_entries[GetRefIndex(arg)].m_locator[level].IsPresent()) { cluster = m_entries[GetRefIndex(arg)].m_locator[level].cluster; @@ -1600,6 +1643,11 @@ void TxGraphImpl::AbortStaging() noexcept // Destroy the staging ClusterSet. m_staging_clusterset.reset(); Compact(); + if (!m_main_clusterset.m_group_data.has_value()) { + // In case m_oversized in main was kept after a Ref destruction while staging exists, we + // need to re-evaluate m_oversized now. + m_main_clusterset.m_oversized = std::nullopt; + } } void TxGraphImpl::CommitStaging() noexcept @@ -1783,10 +1831,13 @@ void TxGraphImpl::SanityCheck() const } } - // Verify that all to-be-removed transactions have valid identifiers, and aren't removed yet. + // Verify that all to-be-removed transactions have valid identifiers. for (GraphIndex idx : clusterset.m_to_remove) { assert(idx < m_entries.size()); - assert(FindCluster(idx, level) != nullptr); + // We cannot assert that all m_to_remove transactions are still present: ~Ref on a + // (P,M) transaction (present in main, inherited in staging) will cause an m_to_remove + // addition in both main and staging, but a subsequence ApplyRemovals in main will + // cause it to disappear from staging too, leaving the m_to_remove in place. } // Verify that all to-be-added dependencies have valid identifiers. @@ -1801,6 +1852,15 @@ void TxGraphImpl::SanityCheck() const // Verify that the contents of m_removed matches what was expected based on the Entry vector. std::set actual_removed(clusterset.m_removed.begin(), clusterset.m_removed.end()); + for (auto i : expected_unlinked) { + // If a transaction exists in both main and staging, and is removed from staging (adding + // it to m_removed there), and consequently destroyed (wiping the locator completely), + // it can remain in m_removed despite not having an IsRemoved() locator. Exclude those + // transactions from the comparison here. + actual_removed.erase(i); + expected_removed[level].erase(i); + } + assert(actual_removed == expected_removed[level]); // If any GraphIndex entries remain in this ClusterSet, compact is not possible. @@ -1812,6 +1872,10 @@ void TxGraphImpl::SanityCheck() const if (clusterset.m_group_data.has_value()) { assert(clusterset.m_oversized == clusterset.m_group_data->m_group_oversized); } + + // For non-top levels, m_oversized must be known (as it cannot change until the level + // on top is gone). + if (level < GetTopLevel()) assert(clusterset.m_oversized.has_value()); } // Verify that the contents of m_unlinked matches what was expected based on the Entry vector. diff --git a/src/txgraph.h b/src/txgraph.h index a524e0468f7..8f9451921eb 100644 --- a/src/txgraph.h +++ b/src/txgraph.h @@ -50,7 +50,8 @@ public: /** Data type used to reference transactions within a TxGraph. * * Every transaction within a TxGraph has exactly one corresponding TxGraph::Ref, held by users - * of the class. Refs can only be destroyed after the transaction is removed from the graph. + * of the class. Destroying the TxGraph::Ref removes the corresponding transaction (in both the + * main and staging graphs). * * Users of the class can inherit from TxGraph::Ref. If all Refs are inherited this way, the * Ref* pointers returned by TxGraph functions can be cast to, and used as, this inherited type. @@ -104,7 +105,9 @@ public: /** Determine whether the graph is oversized (contains a connected component of more than the * configured maximum cluster count). If main_only is false and a staging graph exists, it is * queried; otherwise the main graph is queried. Some of the functions below are not available - * for oversized graphs. The mutators above are always available. */ + * for oversized graphs. The mutators above are always available. Removing a transaction by + * destroying its Ref while staging exists will not clear main's oversizedness until staging + * is aborted or committed. */ virtual bool IsOversized(bool main_only = false) noexcept = 0; /** Determine whether arg exists in the graph (i.e., was not removed). If main_only is false * and a staging graph exists, it is queried; otherwise the main graph is queried. This is @@ -167,8 +170,8 @@ public: /** Construct an empty Ref. Non-empty Refs can only be created using * TxGraph::AddTransaction. */ Ref() noexcept = default; - /** Destroy this Ref. This is only allowed when it is empty, or the transaction it refers - * to does not exist in the graph (in main nor staging). */ + /** Destroy this Ref. If it is not empty, the corresponding transaction is removed (in both + * main and staging, if it exists). */ virtual ~Ref(); // Support moving a Ref. Ref& operator=(Ref&& other) noexcept;