From ee57e93099f243cf9fbf9c10265057a53f06e062 Mon Sep 17 00:00:00 2001 From: Pieter Wuille Date: Thu, 14 Nov 2024 22:45:46 -0500 Subject: [PATCH] txgraph: Add internal sanity check function (tests) To make testing more powerful, expose a function to perform an internal sanity check on the state of a TxGraph. This is especially important as TxGraphImpl contains many redundantly represented pieces of information: * graph contains clusters, which refer to entries, but the entries refer back * graph maintains pointers to Ref objects, which point back to the graph. This lets us make sure they are always in sync. --- src/test/fuzz/txgraph.cpp | 8 +++ src/txgraph.cpp | 123 ++++++++++++++++++++++++++++++++++++++ src/txgraph.h | 3 + 3 files changed, 134 insertions(+) diff --git a/src/test/fuzz/txgraph.cpp b/src/test/fuzz/txgraph.cpp index 1d7fc8345a0..367ded4b146 100644 --- a/src/test/fuzz/txgraph.cpp +++ b/src/test/fuzz/txgraph.cpp @@ -363,6 +363,11 @@ FUZZ_TARGET(txgraph) } } } + + // After running all modifications, perform an internal sanity check (before invoking + // inspectors that may modify the internal state). + real->SanityCheck(); + // Compare simple properties of the graph with the simulation. assert(real->GetTransactionCount() == sim.GetTransactionCount()); @@ -411,6 +416,9 @@ 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 i : sim.graph.Positions()) { diff --git a/src/txgraph.cpp b/src/txgraph.cpp index 81802e2ddf4..c31e7a087d2 100644 --- a/src/txgraph.cpp +++ b/src/txgraph.cpp @@ -12,6 +12,7 @@ #include #include +#include #include #include @@ -94,6 +95,8 @@ public: } /** Get the number of transactions in this Cluster. */ LinearizationIndex GetTxCount() const noexcept { return m_linearization.size(); } + /** Given a DepGraphIndex into this Cluster, find the corresponding GraphIndex. */ + GraphIndex GetClusterEntry(DepGraphIndex index) const noexcept { return m_mapping[index]; } /** Only called by Graph::SwapIndexes. */ void UpdateMapping(DepGraphIndex cluster_idx, GraphIndex graph_idx) noexcept { m_mapping[cluster_idx] = graph_idx; } /** Push changes to Cluster and its linearization to the TxGraphImpl Entry objects. */ @@ -126,6 +129,10 @@ public: FeePerWeight GetIndividualFeerate(DepGraphIndex idx) noexcept; /** Modify the fee of a Cluster element. */ void SetFee(TxGraphImpl& graph, DepGraphIndex idx, int64_t fee) noexcept; + + // Debugging functions. + + void SanityCheck(const TxGraphImpl& graph) const; }; /** The transaction graph. @@ -189,6 +196,8 @@ private: void SetMissing() noexcept { cluster = nullptr; index = 0; } /** Mark this Locator as present, in the specified Cluster. */ void SetPresent(Cluster* c, DepGraphIndex i) noexcept { cluster = c; index = i; } + /** Check if this Locator is missing. */ + bool IsMissing() const noexcept { return cluster == nullptr && index == 0; } /** Check if this Locator is present (in some Cluster). */ bool IsPresent() const noexcept { return cluster != nullptr; } }; @@ -285,6 +294,8 @@ public: std::vector GetAncestors(const Ref& arg) noexcept final; std::vector GetDescendants(const Ref& arg) noexcept final; GraphIndex GetTransactionCount() noexcept final; + + void SanityCheck() const final; }; void Cluster::Updated(TxGraphImpl& graph) noexcept @@ -1098,6 +1109,118 @@ void TxGraphImpl::SetTransactionFee(const Ref& ref, int64_t fee) noexcept } } +void Cluster::SanityCheck(const TxGraphImpl& graph) const +{ + // There must be an m_mapping for each m_depgraph position (including holes). + assert(m_depgraph.PositionRange() == m_mapping.size()); + // The linearization for this Cluster must contain every transaction once. + assert(m_depgraph.TxCount() == m_linearization.size()); + // m_quality and m_setindex are checked in TxGraphImpl::SanityCheck. + + // Compute the chunking of m_linearization. + LinearizationChunking linchunking(m_depgraph, m_linearization); + + // Verify m_linearization. + SetType m_done; + assert(m_depgraph.IsAcyclic()); + for (auto lin_pos : m_linearization) { + assert(lin_pos < m_mapping.size()); + const auto& entry = graph.m_entries[m_mapping[lin_pos]]; + // Check that the linearization is topological. + m_done.Set(lin_pos); + assert(m_done.IsSupersetOf(m_depgraph.Ancestors(lin_pos))); + // Check that the Entry has a locator pointing back to this Cluster & position within it. + assert(entry.m_locator.cluster == this); + assert(entry.m_locator.index == lin_pos); + // Check linearization position. + if (!linchunking.GetChunk(0).transactions[lin_pos]) { + linchunking.MarkDone(linchunking.GetChunk(0).transactions); + } + // If this Cluster has an acceptable quality level, its chunks must be connected. + if (IsAcceptable()) { + assert(m_depgraph.IsConnected(linchunking.GetChunk(0).transactions)); + } + } + // Verify that each element of m_depgraph occured in m_linearization. + assert(m_done == m_depgraph.Positions()); +} + +void TxGraphImpl::SanityCheck() const +{ + /** Which GraphIndexes ought to occur in m_unlinked, based on m_entries. */ + std::set expected_unlinked; + /** Which Clusters ought to occur in m_clusters, based on m_entries. */ + std::set expected_clusters; + + // Go over all Entry objects in m_entries. + for (GraphIndex idx = 0; idx < m_entries.size(); ++idx) { + const auto& entry = m_entries[idx]; + if (entry.m_ref == nullptr) { + // Unlinked Entry must have indexes appear in m_unlinked. + expected_unlinked.insert(idx); + } else { + // Every non-unlinked Entry must have a Ref that points back to it. + assert(GetRefGraph(*entry.m_ref) == this); + assert(GetRefIndex(*entry.m_ref) == idx); + } + const auto& locator = entry.m_locator; + // Every Locator must be in exactly one of these 2 states. + assert(locator.IsMissing() + locator.IsPresent() == 1); + if (locator.IsPresent()) { + // Verify that the Cluster agrees with where the Locator claims the transaction is. + assert(locator.cluster->GetClusterEntry(locator.index) == idx); + // Remember that we expect said Cluster to appear in the m_clusters. + expected_clusters.insert(locator.cluster); + } + + } + + std::set actual_clusters; + // For all quality levels... + for (int qual = 0; qual < int(QualityLevel::NONE); ++qual) { + QualityLevel quality{qual}; + const auto& quality_clusters = m_clusters[qual]; + // ... for all clusters in them ... + for (ClusterSetIndex setindex = 0; setindex < quality_clusters.size(); ++setindex) { + const auto& cluster = *quality_clusters[setindex]; + // Remember we saw this Cluster (only if it is non-empty; empty Clusters aren't + // expected to be referenced by the Entry vector). + if (cluster.GetTxCount() != 0) { + actual_clusters.insert(&cluster); + } + // Sanity check the cluster, according to the Cluster's internal rules. + cluster.SanityCheck(*this); + // Check that the cluster's quality and setindex matches its position in the quality list. + assert(cluster.m_quality == quality); + assert(cluster.m_setindex == setindex); + } + } + + // Verify that all to-be-removed transactions have valid identifiers, and aren't removed yet. + for (GraphIndex idx : m_to_remove) { + assert(idx < m_entries.size()); + assert(m_entries[idx].m_locator.IsPresent()); + } + + // Verify that all to-be-added dependencies have valid identifiers. + for (auto [par_idx, chl_idx] : m_deps_to_add) { + assert(par_idx != chl_idx); + assert(par_idx < m_entries.size()); + assert(chl_idx < m_entries.size()); + } + + // Verify that the actually encountered clusters match the ones occurring in Entry vector. + assert(actual_clusters == expected_clusters); + + // Verify that the contents of m_unlinked matches what was expected based on the Entry vector. + std::set actual_unlinked(m_unlinked.begin(), m_unlinked.end()); + assert(actual_unlinked == expected_unlinked); + + // If no to-be-removed transactions, or to-be-added dependencies remain, m_unlinked must be + // empty (to prevent memory leaks due to an ever-growing m_entries vector). + if (m_to_remove.empty() && m_deps_to_add.empty()) assert(actual_unlinked.empty()); +} + } // namespace TxGraph::Ref::~Ref() diff --git a/src/txgraph.h b/src/txgraph.h index 0a20ad232d2..709b4ba7c2b 100644 --- a/src/txgraph.h +++ b/src/txgraph.h @@ -94,6 +94,9 @@ public: /** Get the total number of transactions in the graph. */ virtual GraphIndex GetTransactionCount() noexcept = 0; + /** Perform an internal consistency check on this object. */ + virtual void SanityCheck() const = 0; + protected: // Allow TxGraph::Ref to call UpdateRef and UnlinkRef. friend class TxGraph::Ref;