fuzz: txgraph: fix real_is_optimal flag propagation in CommitStaging #33132

1. 
   theStack commented at 0:45 am on August 4, 2025: contributor

   In the txgraph fuzz test, the CommitStaging step updates the SimTxGraph levels simply by erasing the front (=main) one in the sims vector, i.e. the staging level instance takes the place of the main level instance:

   https://github.com/bitcoin/bitcoin/blob/83a2216f5278c1123ba740f1c8a055652e033ddd/src/test/fuzz/txgraph.cpp#L668-L672

   This also includes the real_is_optimal flag (reflecting whether the corresponding real graph is known to be optimally linearized), without taking into account that this flag should only be set if both levels before the commiting are optimal.

   E.g. in case of #33097, at this point the main level is not optimally linearized, while the staging level is, and due to the incorrect propagation of the latter the simulation incorrectly assumes that the main level is optimal after, leading to the assertion fail in the additional checks that are ran in this case[1]. Fix this by setting the flag in the resulting main level explicitly. This is done in a generic way, in case there will ever be more than two levels (not sure what is planned in this direction), a simpler alternative would be e.g. main_optimal = sim[0].real_is_optimal && sim[1].real_is_optimal.

   Fixes #33097.

   [1] see https://github.com/theStack/bitcoin/commit/0aedf09ccc0ad4fff5956bdc012778534612c017 for the printf-debug-session-clutter, if that is useful/interesting for anyone (most of the output turned out to be irrelevant to the actual cause of #33097, but it was an entertaining way to discover the interface and get a first glimpse of TxGraph internals as a cluster-mempool newbie).

2. fuzz: txgraph: fix `real_is_optimal` flag propagation in `CommitStaging`

   In the `txgraph` fuzz test, the `CommitStaging` step updates the
   `SimTxGraph` levels simply by erasing the front (=main) one in the
   `sims` vector, i.e. the staging level instance takes the place of the
   main level instance. This also includes the `real_is_optimal` flag
   (reflecting whether the corresponding real graph is known to be
   optimally linearized), without taking into account that this flag
   should only be set if _both_ levels before the commiting are optimal.
   
   E.g. in case of #33097, the main level is not optimally linearized,
   while the staging level is, and due to the incorrect propagation of the
   latter to the simulation incorrectly assumes that the main level is
   optimal, leading to the assertion fail. Fix this by setting the flag
   in the resulting main level explicitly.
   
   Resolves the fuzzing assertion fail in issue #33097.

   444dcb2f99
3. maflcko added the label Tests on Aug 4, 2025
4. 
   glozow commented at 1:37 pm on August 4, 2025: member

   ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c

   The fix works and makes sense to me. I suppose a concrete scenario (edited for more clarity):

   1. we have main and staging, neither optimally linearized. there are some non-optimal clusters that haven’t been pulled from main to staging (this is the case in the real TxGraph, though the sim has a full copy)
   2. we create an observer for the main level (alternatively, it could be oversized)
   3. we call DoWork() which returns true after optimally linearizing the staging level and skipping the main level
   4. we delete the main level observer
   5. we call CommitStaging. sims staging gets copied over.
   6. in the real TxGraph, while some clusters are optimal (the ones that changed in staging), others aren’t
5. 
   DrahtBot commented at 1:37 pm on August 4, 2025: contributor

   The following sections might be updated with supplementary metadata relevant to reviewers and maintainers.

   Code Coverage & Benchmarks

   For details see: https://corecheck.dev/bitcoin/bitcoin/pulls/33132.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	glozow, sipa

   If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update.

6. fanquake requested review from marcofleon on Aug 4, 2025
7. in src/test/fuzz/txgraph.cpp:674 in 444dcb2f99

   667@@ -668,7 +668,10 @@ FUZZ_TARGET(txgraph)
   668             } else if (block_builders.empty() && sims.size() > 1 && command-- == 0) {
   669                 // CommitStaging.
   670                 real->CommitStaging();
   671+                // Resulting main level is only guaranteed to be optimal if all levels are
   672+                const bool main_optimal = std::all_of(sims.cbegin(), sims.cend(), [](const auto &sim) { return sim.real_is_optimal; });
   673                 sims.erase(sims.begin());
   674+                sims.front().real_is_optimal = main_optimal;
   

   ---

   ---

   

   sipa commented at 2:03 pm on August 4, 2025:

   Nit: if this is actually aiming to be forward compatible with a future more-than-two-levels extension (which we may in fact need at some point), I think this should be sims.back(). ... = .... Doesn’t matter now of course as it’s all equivalent to level 0.
8. 
   sipa commented at 2:03 pm on August 4, 2025: member

   ACK 444dcb2f9944ad5208bf00c9f197da7b2c98063c
9. glozow merged this on Aug 4, 2025
10. glozow closed this on Aug 4, 2025

    ---

11. theStack deleted the branch on Aug 4, 2025

Contributors
theStack glozow DrahtBot sipa

Review Requested
marcofleon

Labels
Tests