Skip to content

Conversation

@kevkevinpal
Copy link
Contributor

@kevkevinpal kevkevinpal commented Nov 7, 2023

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

7505ec2 Renaming cached_descendants to descendants for simpler variable naming

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

@DrahtBot
Copy link
Contributor

DrahtBot commented Nov 7, 2023

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.

@glozow
Copy link
Member

glozow commented Nov 7, 2023

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 
    

@kevkevinpal kevkevinpal force-pushed the miniminerLinearizationFollowups branch from b21f2f2 to cec3cfc Compare November 7, 2023 14:52
Changes MiniMinerMempoolEntry order to match the order of the params
elsewhere in the codebase
Refactored a variable name to be less confusing
@kevkevinpal kevkevinpal force-pushed the miniminerLinearizationFollowups branch from cec3cfc to 89c1468 Compare November 7, 2023 14:57
@kevkevinpal
Copy link
Contributor Author

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

Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

@kevkevinpal kevkevinpal force-pushed the miniminerLinearizationFollowups branch from 89c1468 to b4b01d3 Compare November 8, 2023 20:46
Copy link
Contributor

@murchandamus murchandamus left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

reACK b4b01d3

Copy link
Contributor

@theStack theStack left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM ACK b4b01d3

@glozow glozow merged commit d60ebea into bitcoin:master Nov 9, 2023
@glozow
Copy link
Member

glozow commented Nov 9, 2023

@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

@glozow glozow mentioned this pull request Nov 9, 2023
57 tasks
@bitcoin bitcoin locked and limited conversation to collaborators Nov 8, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants