refactor: Miniminer package linearization followups #28808

1. 
   kevkevinpal commented at 4:17 am on November 7, 2023: contributor

   Motivation

   In #28762 there were some post merge comments which are being addressed in this PR with the following commits

   8d4c46f Reorganizing MiniMinerMempoolEntry to match the order we have elsewhere

   • #28762 (review)

   7505ec2 Renaming cached_descendants to descendants for simpler variable naming

   • #28762 (review)

   b21f2f2 Code comment modifications to be more accurate to what is actually happening

   • #28762 (review) and
   • #28762 (review) and
   • #28762 (review)
2. 
   DrahtBot commented at 4:17 am on November 7, 2023: contributor

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

   Code Coverage

   For detailed information about the code coverage, see the test coverage report.

   Reviews

   See the guideline for information on the review process.

   Type	Reviewers
   ACK	murchandamus, theStack

   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:

   • #28335 (RFC: Remove boost usage from kernel api / headers by TheCharlatan)

   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.

3. DrahtBot added the label Refactoring on Nov 7, 2023
4. DrahtBot added the label CI failed on Nov 7, 2023
5. 
   glozow commented at 2:45 pm on November 7, 2023: member

   This diff appears to have added new lines with trailing whitespace. The following changes were suspected:

   diff –git a/src/test/miniminer_tests.cpp b/src/test/miniminer_tests.cpp @@ -668,7 +667,7 @@ BOOST_FIXTURE_TEST_CASE(manual_ctor, TestChain100Setup)

   •    miniminer_info.emplace_back(grandparent_zero_fee,          /*vsize_self=*/tx_vsize,/*vsize_ancestor=*/tx_vsize, /*fee_self=*/0,/*fee_ancestor=*/0);      
     

   @@ -696 +695 @@ BOOST_FIXTURE_TEST_CASE(manual_ctor, TestChain100Setup)

   •    // CPFP low + double low 

6. kevkevinpal force-pushed on Nov 7, 2023
7. [refactor] Change MiniMinerMempoolEntry order

   Changes MiniMinerMempoolEntry order to match the order of the params
   elsewhere in the codebase

   43423fd834
8. [refactor] Miniminer var cached_descendants to descendants

   Refactored a variable name to be less confusing

   83933eff00
9. kevkevinpal force-pushed on Nov 7, 2023
10. 
    kevkevinpal commented at 2:58 pm on November 7, 2023: contributor

    This diff appears to have added new lines with trailing whitespace. The following changes were suspected:

    diff –git a/src/test/miniminer_tests.cpp b/src/test/miniminer_tests.cpp @@ -668,7 +667,7 @@ BOOST_FIXTURE_TEST_CASE(manual_ctor, TestChain100Setup)

    * ```
         miniminer_info.emplace_back(grandparent_zero_fee,          /*vsize_self=*/tx_vsize,/*vsize_ancestor=*/tx_vsize, /*fee_self=*/0,/*fee_ancestor=*/0);      
      ```
    

    @@ -696 +695 @@ BOOST_FIXTURE_TEST_CASE(manual_ctor, TestChain100Setup)

    * ```
         // CPFP low + double low 
      ```
    

    Thanks! fixed in 43423fd and 89c1468

11. DrahtBot removed the label CI failed on Nov 7, 2023
12. glozow requested review from murchandamus on Nov 8, 2023
13. glozow requested review from theStack on Nov 8, 2023
14. in src/node/mini_miner.h:140 in 89c1468741 outdated

    137@@ -138,7 +138,7 @@ class MiniMiner
    138     MiniMiner(const CTxMemPool& mempool, const std::vector<COutPoint>& outpoints);
    139 
    140     /** Constructor in which the MiniMinerMempoolEntry entries have been constructed manually,
    

    ---

    ---

    

    murchandamus commented at 8:33 pm on November 8, 2023:

    Nit: First sentence ends in a comma now.

    ---

    

    kevkevinpal commented at 8:47 pm on November 8, 2023:

    fixed in b4b01d3
15. 
    murchandamus commented at 8:40 pm on November 8, 2023: contributor

    ACK 89c146874175f8481c59436813ccf52a177a8664

    This does address the comments I left on the prior PR #28762, and the conflicting PR appears to still be a draft. Looks cleaner and easier to understand to me, but also not particularly urgent.

16. [refactor] updating miniminer comments to be more accurate b4b01d3fb4
17. kevkevinpal force-pushed on Nov 8, 2023
18. 
    murchandamus commented at 8:51 pm on November 8, 2023: contributor

    Trivial fix of my nit and improved formatting of the corresponding comment.

    reACK b4b01d3fb42e7b688d97b75f57cfe18cfca6d943

19. theStack approved
20. 
    theStack commented at 0:01 am on November 9, 2023: contributor

    LGTM ACK b4b01d3fb42e7b688d97b75f57cfe18cfca6d943
21. glozow merged this on Nov 9, 2023
22. glozow closed this on Nov 9, 2023

    ---

23. 
    glozow commented at 9:31 am on November 9, 2023: member

    @kevkevinpal (nit) in the future, it’s best to delete the help text before you open the PR as it gets pulled into the merge commit
24. bitcoin locked this on Nov 8, 2024

Contributors
kevkevinpal DrahtBot glozow murchandamus theStack

Labels
Refactoring