txgraph: use enum Level instead of bool main_only #33354

1. 
   sipa commented at 7:57 pm on September 9, 2025: member

   Part of #30289. Inspired by #28676 (review).

   Since there has been more than one case in the development of #28676 of calling a TxGraph function without correctly setting the bool main_only argument that many of its interface functions have, make these mandatory and explicit, using an enum class Level:

   0enum class Level {
   1    TOP, //!< Refers to staging if it exists, main otherwise.
   2    MAIN //!< Always refers to the main graph, whether staging is present or not.
   3};
   

2. 
   DrahtBot commented at 7:57 pm on September 9, 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/33354.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	vasild, instagibbs, glozow

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

   Conflicts

   Reviewers, this pull request conflicts with the following ones:

   • #33157 (cluster mempool: control/optimize TxGraph memory usage by sipa)

   If you consider this pull request important, please also help to review the conflicting pull requests. Ideally, start with the one that should be merged first.

   LLM Linter (✨ experimental)

   Possible typos and grammar issues:

   • “(which may be indicative of the transaction having been removed already.” -> “(which may be indicative of the transaction having been removed already).” [Missing closing parenthesis makes the parenthetical unclosed and the sentence punctuation inconsistent.]

   drahtbot_id_5_m

3. sipa force-pushed on Sep 9, 2025
4. in src/test/fuzz/txgraph.cpp:1173 in b9af83e72c outdated

   1172-    for (int main_only = 0; main_only < 2; ++main_only) {
   1173-        auto& sim = main_only ? sims[0] : sims.back();
   1174+    // Try to run a full comparison, for both TxGraph::Level::MAIN and TxGraph::Level::TOP in
   1175+    // TxGraph inspector functions that support both.
   1176+    for (int level_select_int = 0; level_select_int < 2; ++level_select_int) {
   1177+        TxGraph::Level level = level_select_int == 0 ? TxGraph::Level::TOP : TxGraph::Level::MAIN;
   

   ---

   ---

   

   vasild commented at 8:24 am on September 10, 2025:

   Can be written as:

   0for (auto level : { TxGraph::Level::TOP, TxGraph::Level::MAIN }) {
   

   ---

   

   sipa commented at 12:03 pm on September 10, 2025:

   Indeed, done!
5. vasild approved
6. 
   vasild commented at 9:14 am on September 10, 2025: contributor

   ACK b9af83e72c60c1ee71251cf9f3dbae0ffccbe305

   Passing enum values with meaningful names seems better and more readable than passing true / false or omitting the argument.

7. txgraph: use enum Level instead of bool main_only d45f3717d2
8. sipa force-pushed on Sep 10, 2025
9. vasild approved
10. 
    vasild commented at 12:12 pm on September 10, 2025: contributor

    ACK d45f3717d2c65d1a6012a4bc2f47ff75004fd171
11. 
    instagibbs commented at 12:32 pm on September 10, 2025: member

    concept ACK, having the two values was never confusing, it just took mental load to figure out what the implied argument meant in each context (and lead to bugs too)
12. 
    instagibbs commented at 1:08 pm on September 10, 2025: member

    ACK d45f3717d2c65d1a6012a4bc2f47ff75004fd171

    significantly easier to read usages, thank you

13. glozow added the label Refactoring on Sep 11, 2025
14. 
    glozow commented at 2:59 pm on September 11, 2025: member

    code review ACK d45f3717d2c65d1a6012a4bc2f47ff75004fd171
15. glozow merged this on Sep 11, 2025
16. glozow closed this on Sep 11, 2025

    ---

Contributors
sipa DrahtBot vasild instagibbs glozow

Labels
Refactoring