refactor: Simplify bnb coin_selection test params #26111

yancyribbens commented at 3:02 PM on September 16, 2022: contributor

We can simplify the coin_selection_params by simply setting the only relevant param cost_of_change and explain the calculation used. The formula used to calculate the cost of change is from here.

yancyribbens force-pushed on Sep 16, 2022

DrahtBot added the label Refactoring on Sep 16, 2022

DrahtBot commented at 3:44 AM on September 17, 2022: contributor

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

Reviews

See the guideline for information on the review process. A summary of reviews will appear here.

Conflicts

Reviewers, this pull request conflicts with the following ones:

#26110 (test: Remove unused coin_selection param by yancyribbens)

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.

davidgumberg commented at 6:40 AM on January 18, 2023: contributor

I am not sure that I understand the motivation for this change.

yancyribbens commented at 11:47 AM on January 19, 2023: contributor

It's a trivial change, however the reason I made the commit is to simplify the tests. If you read the tests, do you know the value of?:

coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size) + coin_selection_params_bnb.m_change_fee;

Well it takes bit of tinkering, but it turns out the value is always CAmount(297); so why not just set that value to make the test easier to read.

in src/wallet/test/coinselector_tests.cpp:303 in c9ee2f2574 outdated

 297 | @@ -298,8 +298,11 @@ BOOST_AUTO_TEST_CASE(bnb_search_test)
 298 |          /*tx_noinputs_size=*/ 0,
 299 |          /*avoid_partial=*/ false,
 300 |      };
 301 | -    coin_selection_params_bnb.m_change_fee = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_output_size);
 302 | -    coin_selection_params_bnb.m_cost_of_change = coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size) + coin_selection_params_bnb.m_change_fee;
 303 | +
 304 | +    // Cost of change is the cost of creating the change output + cost of spending the change output in the future.
 305 | +    // cost of change = (change output size * effective feerate) + (size of spending change output * discard feerate)

kouloumos commented at 12:31 PM on January 19, 2023:

I do not agree with replacing the variables, but If you believe that as it is the code is not easily understandable, why not just add those comments?

yancyribbens commented at 1:18 PM on January 19, 2023:

Why not both? Just adding the comments would help though if you're really that interested in having the redundant computations run every time the test suit is run.

davidgumberg commented at 5:20 PM on January 19, 2023:

@yancyribbens I have a slightly different perception about whether the computations are redundant. As I see it, the advantage of unit tests is that we ensure that obvious cases have the obvious results that we expect them to have, every time.

yancyribbens commented at 5:33 PM on January 19, 2023:

@davidgumberg that's fair. I've updated this PR based on what @kouloumos said. I'm not sure if you agree that this is easier to reason about or not which was the motivation for creating this PR.

kouloumos commented at 12:32 PM on January 19, 2023: contributor

If you read the tests, do you know the value of?:

coin_selection_params_bnb.m_effective_feerate.GetFee(coin_selection_params_bnb.change_spend_size) + coin_selection_params_bnb.m_change_fee;

But why do you care of that value in the first place? Isn't more important to understand the logic behind the calculation than the actual result?

Then why not also change the next line to

// 204 = (68 * 3000) / 1000)
coin_selection_params_bnb.min_viable_change = CAmount(204)

Variables make maintenance easier and assist with code/test/functionality comprehension.

yancyribbens commented at 1:20 PM on January 19, 2023: contributor

Then why not also change the next line to:

 // 204 = (68 * 3000) / 1000)
coin_selection_params_bnb.min_viable_change = CAmount(204)

I think that's a good idea. It was a while ago when I put in this PR but I think I didn't look into that parameter at the time.

yancyribbens force-pushed on Jan 19, 2023

in src/wallet/test/coinselector_tests.cpp:314 in c086c64585 outdated

 312 | +
 313 | +    // cost_of_change = current fee to create the output + future fee to spend the output.
 314 | +    // cost of change = (change_spend_size * effective feerate) + (change_output_size * discard_feerate)
 315 | +    // 297 = ((68 * 3000) / 1000) + (31 * 3000) / 1000
 316 | +    coin_selection_params_bnb.m_cost_of_change = fee + coin_selection_params_bnb.m_change_fee;
 317 | +    BOOST_CHECK_EQUAL(CAmount(297), coin_selection_params_bnb.min_viable_change);

davidgumberg commented at 8:06 PM on January 19, 2023:

should be coin_selection_params_bnb.m_cost_of_change

yancyribbens commented at 1:26 AM on January 20, 2023:

@davidgumberg thanks. Interestingly the comment used to read: 297 = ((68 * 3000) / 1000) + (31 * 3000) / 1000 however the value of coin_selection_params_bnb.m_change_fee has changed since I originally opened this PR (I'm not sure why atm).

In this commit: https://github.com/bitcoin/bitcoin/commit/4fef5344288e454460b80db0316294e1ec1ad8ad the m_change_fee was 93.

yancyribbens force-pushed on Jan 20, 2023

refactor: Simplify bnb coin_selection test params fa0f8dfc67

achow101 commented at 3:54 PM on April 25, 2023: member

The feature request didn't seem to attract much attention in the past. Also, the issue seems not important enough right now to keep it sitting around idle in the list of open issues.

Closing due to lack of interest. Pull requests with improvements are always welcome.

achow101 closed this on Apr 25, 2023

bitcoin locked this on Apr 24, 2024