phantom/bitcoin
1
0
Fork 0
mirror of https://github.com/bitcoin/bitcoin.git synced 2025-04-29 06:49:38 -04:00
Code Issues Projects Releases Packages Wiki Activity Actions 1

Merge bitcoin/bitcoin#32177: TxGraph: Increase fuzz coverage
Some checks are pending
CI / test each commit (push) Waiting to run
Details
CI / macOS 14 native, arm64, no depends, sqlite only, gui (push) Waiting to run
Details
CI / macOS 14 native, arm64, fuzz (push) Waiting to run
Details
CI / Windows native, VS 2022 (push) Waiting to run
Details
CI / Windows native, fuzz, VS 2022 (push) Waiting to run
Details
CI / Linux->Windows cross, no tests (push) Waiting to run
Details
CI / Windows, test cross-built (push) Blocked by required conditions
Details
CI / ASan + LSan + UBSan + integer, no depends, USDT (push) Waiting to run
Details

Browse source 
a40bd374aa Get*Union: disallow nulltpr Refs (Greg Sanders)
57433502e6 CountDistinctClusters: nullptrs disallowed (Greg Sanders)
8bca0d325a TxGraphImpl::Compact: m_main_clusterset.m_removed is always empty (Greg Sanders)
2c5cf987e9 TxGraphImpl::PullIn: only allowed when staging exists (Greg Sanders)

Pull request description:

  Was looking at my local coverage report, and noticed a few spots that will not or cannot be hit.

  CountDistinctClusters, GetAncestorsUnion, and GetDescendantsUnion accept nullptrs, but the test harness never employs them. Disallow them.

  We never call PullIn whenever there isn't staging, so just enforce that invariant via assertion.

  Remaining places that are not covered:

  1) Relinearize: Currently we seem to always start with a cold (not known to be optimal) cluster, and after one attempt at linearization result into something optimal. This means we never shortcircuit, nor run PostLinearization, nor store the quality as ACCEPTABLE. Reducing iterations causes these lines to be hit. sipa says he will take this on as varying the amount of iterations was meant to be done eventually anyways.
  2) We never do a move assignment operator when the lvalue already has a `m_graph` (so we never call UnlinkRef) 3358b1d105/src/txgraph.cpp (L2097)
  3) We never use the move constructor: 3358b1d105/src/txgraph.cpp (L2108)

ACKs for top commit:
  sipa:
    utACK a40bd374aa
  glozow:
    utACK a40bd374aa

Tree-SHA512: ca88297222e80e0d590889698899f892b9335cfa587a76a6c6ca62c8d846f208b6b0b9a9b1829bafabdb929a1a0c3a75f23edf7dd2b4f5e2dad0235e5bc68ba3
This commit is contained in:
merge-script 2025-04-03 09:51:28 +08:00
commit 99b9022844
No known key found for this signature in database
GPG key ID: 2EEB9F5CC09526C1
2 changed files with 11 additions and 9 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files

12
src/txgraph.cpp
View file

@ -411,8 +411,8 @@ public:
* values for remaining Entry objects, so this only does something when no to-be-applied
* operations or staged removals referring to GraphIndexes remain). */
void Compact() noexcept;
/** If cluster is not in staging, copy it there, and return a pointer to it. This has no
* effect if only a main graph exists, but if staging exists this modifies the locators of its
/** If cluster is not in staging, copy it there, and return a pointer to it.
* Staging must exist, and this modifies the locators of its
* 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
@ -928,7 +928,7 @@ Cluster* TxGraphImpl::FindCluster(GraphIndex idx, int level) const noexcept
Cluster* TxGraphImpl::PullIn(Cluster* cluster) noexcept
{
int to_level = GetTopLevel();
if (to_level == 0) return cluster;
Assume(to_level == 1);
int level = cluster->m_level;
Assume(level <= to_level);
// Copy the Cluster from main to staging, if it's not already there.
@ -1008,7 +1008,7 @@ void TxGraphImpl::Compact() noexcept
// to rewrite them. It is easier to delay the compaction until they have been applied.
if (!m_main_clusterset.m_deps_to_add.empty()) return;
if (!m_main_clusterset.m_to_remove.empty()) return;
if (!m_main_clusterset.m_removed.empty()) return;
Assume(m_main_clusterset.m_removed.empty()); // non-staging m_removed is always empty
if (m_staging_clusterset.has_value()) {
if (!m_staging_clusterset->m_deps_to_add.empty()) return;
if (!m_staging_clusterset->m_to_remove.empty()) return;
@ -1595,6 +1595,7 @@ std::vector<TxGraph::Ref*> TxGraphImpl::GetAncestorsUnion(std::span<const Ref* c
std::vector<std::pair<Cluster*, DepGraphIndex>> matches;
matches.reserve(args.size());
for (auto arg : args) {
Assume(arg);
// Skip empty Refs.
if (GetRefGraph(*arg) == nullptr) continue;
Assume(GetRefGraph(*arg) == this);
@ -1627,6 +1628,7 @@ std::vector<TxGraph::Ref*> TxGraphImpl::GetDescendantsUnion(std::span<const Ref*
std::vector<std::pair<Cluster*, DepGraphIndex>> matches;
matches.reserve(args.size());
for (auto arg : args) {
Assume(arg);
// Skip empty Refs.
if (GetRefGraph(*arg) == nullptr) continue;
Assume(GetRefGraph(*arg) == this);
@ -1880,7 +1882,7 @@ TxGraph::GraphIndex TxGraphImpl::CountDistinctClusters(std::span<const Ref* cons
std::vector<Cluster*> clusters;
clusters.reserve(refs.size());
for (const Ref* ref : refs) {
if (ref == nullptr) continue;
Assume(ref);
if (GetRefGraph(*ref) == nullptr) continue;
Assume(GetRefGraph(*ref) == this);
auto cluster = FindCluster(GetRefIndex(*ref), level);

8
src/txgraph.h
View file

@ -144,11 +144,11 @@ public:
virtual std::vector<Ref*> GetDescendants(const Ref& arg, bool main_only = false) noexcept = 0;
/** Like GetAncestors, but return the Refs for all transactions in the union of the provided
* arguments' ancestors (each transaction is only reported once). Refs that do not exist in
* the queried graph are ignored. */
* the queried graph are ignored. Null refs are not allowed. */
virtual std::vector<Ref*> GetAncestorsUnion(std::span<const Ref* const> args, bool main_only = false) noexcept = 0;
/** Like GetDescendants, but return the Refs for all transactions in the union of the provided
* arguments' descendants (each transaction is only reported once). Refs that do not exist in
* the queried graph are ignored. */
* the queried graph are ignored. Null refs are not allowed. */
virtual std::vector<Ref*> GetDescendantsUnion(std::span<const Ref* const> args, bool main_only = false) noexcept = 0;
/** Get the total number of transactions in the graph. If main_only is false and a staging
* graph exists, it is queried; otherwise the main graph is queried. This is available even
@ -159,8 +159,8 @@ public:
virtual std::strong_ordering CompareMainOrder(const Ref& a, const Ref& b) noexcept = 0;
/** Count the number of distinct clusters that the specified transactions belong to. If
* main_only is false and a staging graph exists, staging clusters are counted. Otherwise,
* main clusters are counted. Refs that do not exist in the queried graph are ignored. The
* queried graph must not be oversized. */
* main clusters are counted. Refs that do not exist in the queried graph are ignored. Refs
* can not be null. The queried graph must not be oversized. */
virtual GraphIndex CountDistinctClusters(std::span<const Ref* const>, bool main_only = false) noexcept = 0;
/** Perform an internal consistency check on this object. */