test: Remove redundant test #25966

pull yancyribbens wants to merge 1 commits into bitcoin:master from yancyribbens:remove-redundant-test changing 1 files +0 −11

yancyribbens commented at 1:37 PM on August 31, 2022: contributor

I can't think of any reason to keep this test case around labeled fix me. The test was originally added here however there was never an assertion about the coins that should be selected, only that a solution is found (which is a redundant solution to the test above). The comment was later added here to fix it, however it's unclear what exactly it's testing. A test was later added here where if the long term fee is less than the current fee, then select fewer UTXOs, which may have been the original intent.
test: Remove redundant test fb1c6c14c1
achow101 requested review from murchandamus on Aug 31, 2022
DrahtBot added the label Tests on Aug 31, 2022
S3RK commented at 6:41 AM on September 1, 2022: contributor

Another option would be to keep the test and check that even UTXOs with negative values are selected as expected.
yancyribbens commented at 9:09 AM on September 1, 2022: contributor

How can you have Utxos with negative values? There's a specific assertion here that prevents it.
yancyribbens commented at 9:57 AM on September 2, 2022: contributor

@Empact Since you added the fix me do you have any objection to removing it?
davidgumberg commented at 11:08 PM on September 2, 2022: contributor

It seems to me that the original intention of this test was to set a feerate high enough that a 1 cent input had an effective value that was zero or negative, and then ensure that it was excluded from selection, resulting in the selection of the 5, 3, 2 expected_result that we never actually checked for.

But, as @yancyribbens says, such a test could not even be performed because of the assert against effectively negative utxo's in SelectCoinsBnB.

ACK and tested; and CI complaint seems unrelated to this, related to #25717 .
yancyribbens commented at 8:38 AM on September 4, 2022: contributor

@davidgumberg, I'm still not sure about the original intent, although, I agree with you, that the test code be used to set a feerate such that a minimal amount of utxo's is selected. That test already exists here.
S3RK commented at 6:57 AM on September 5, 2022: contributor

How can you have Utxos with negative values? There's a specific assertion here that prevents it.

Ah.. this makes sense. We also filter out only positive effective value utxos in AttemptSelection

ACK fb1c6c14c11ccd4833c1a24f77c507f098d369ad
Zero-1729 commented at 8:14 AM on September 5, 2022: contributor

Concept ACK fb1c6c14c11ccd4833c1a24f77c507f098d369ad

No need to keep a redundant test around. However, it'd still be good to get more clarity from @Empact or anyone else familiar with the purpose of the redundant test's inclusion in the first place.
DrahtBot commented at 4:37 AM on September 23, 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 S3RK, achow101

Concept ACK davidgumberg, Zero-1729

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



Conflicts

No conflicts as of last run.
fanquake requested review from achow101 on Jan 12, 2023
achow101 commented at 10:24 PM on February 3, 2023: member

ACK fb1c6c14c11ccd4833c1a24f77c507f098d369ad
achow101 merged this on Feb 3, 2023
achow101 closed this on Feb 3, 2023
sidhujag referenced this in commit 4bec252ac7 on Feb 4, 2023
bitcoin locked this on Feb 3, 2024