test: Change coinselection parameter location to make tests independent #26020

pull yancyribbens wants to merge 1 commits into bitcoin:master from yancyribbens:fix-bnb-search-test-param changing 1 files +2 −1

yancyribbens commented at 12:01 PM on September 6, 2022: contributor

the subtract_fee_outputs param is expected to be true for all subsequent tests. It should be defined outside of a single test so that if it's removed or changed, all subsequent tests won't fail. Currently if you remove this test the following test fails. This change makes the tests independent.
test: Change coinselection parameter location to make tests independent b942c94d15
fanquake added the label Tests on Sep 6, 2022
aureleoules commented at 11:09 AM on September 7, 2022: member

ACK b942c94d153f83b77ef5d603211252d9abadde95. The first test does not use coin_selection_params_bnb.m_subtract_fee_outputs, thus can be removed there. Verified that coin_selection_params_bnb.m_subtract_fee_outputs = true is required in subsequent tests.
maflcko added the label Wallet on Sep 7, 2022
yancyribbens commented at 9:28 PM on September 9, 2022: contributor

@aureleoules thanks for pointing out that m_subtract_fee_outputs is not used in that test. After examining that test more closely, it doesn't seem to be testing wait it's supposed to be:

// Test fees subtracted from output

As you say, the param subtract_fee_outputs isn't being used. In the first assertion the value being selected is 1 sat while the second is using 1 * CENT which is why there's a difference in the first and second assertion. Also, there is no way to use a negative effective value like the comment says. There's a specific assert that precludes it here.

All in all, maybe the test should be removed in a future PR. Another possibility is to fix it to test this statement which maybe was the original intent.
DrahtBot commented at 4:38 PM 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.

Type Reviewers

ACK aureleoules, rajarshimaitra, theStack, achow101

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:
- #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.
rajarshimaitra approved
rajarshimaitra commented at 1:32 PM on September 19, 2022: contributor

tACK b942c94d153f83b77ef5d603211252d9abadde95

Verified that removing the test in question fails subsequent tests. Which signifies the expected value for m_subtract_fee_outputs is true for other cases..

One thought/question Should we also perform a test with m_subtract_fee_outputs = false in bnb?
maflcko requested review from achow101 on Sep 19, 2022
yancyribbens commented at 3:09 PM on September 19, 2022: contributor

@rajarshimaitra my opinion is that m_subtract_fee_outputs = false isn't important to test since this is equivalent to saying the cost of change is 0 and as far as I know shouldn't happen. Although others may have a better idea or feel different.
theStack approved
theStack commented at 1:10 AM on January 3, 2023: contributor

ACK b942c94d153f83b77ef5d603211252d9abadde95
achow101 commented at 5:35 PM on January 4, 2023: member

ACK b942c94d153f83b77ef5d603211252d9abadde95
achow101 merged this on Jan 4, 2023
achow101 closed this on Jan 4, 2023
sidhujag referenced this in commit 7dd64627f2 on Jan 4, 2023
bitcoin locked this on Jan 4, 2024