test: Remove redundant test #25966
test: Remove redundant test #25966
Conversation
|
Another option would be to keep the test and check that even UTXOs with negative values are selected as expected. |
|
How can you have Utxos with negative values? There's a specific assertion here that prevents it. |
|
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 But, as @yancyribbens says, such a test could not even be performed because of the assert against effectively negative utxo's in ACK and tested; and CI complaint seems unrelated to this, related to #25717 . |
|
@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. |
|
The following sections might be updated with supplementary metadata relevant to reviewers and maintainers. ReviewsSee the guideline for information on the review process.
If your review is incorrectly listed, please react with 👎 to this comment and the bot will ignore it on the next update. ConflictsNo conflicts as of last run. |
|
ACK fb1c6c1 |
fb1c6c1 test: Remove redundant test (yancy) Pull request description: I can't think of any reason to keep this test case around labeled [fix me](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L242). The test was originally added [here](bitcoin@4566ab7) 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](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L222)). The comment was later added here to [fix](bitcoin@3842732) it, however it's unclear what exactly it's testing. A test was later added [here](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L366) where if the [long term fee](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L357) is less than the current [fee](https://github.com/bitcoin/bitcoin/blob/master/src/wallet/test/coinselector_tests.cpp#L356), then select fewer UTXOs, which may have been the original intent. ACKs for top commit: S3RK: ACK fb1c6c1 Zero-1729: Concept ACK fb1c6c1 achow101: ACK fb1c6c1 Tree-SHA512: bce2cdae669c144ffaa130237a1643e3b6728e13d603cebf5d9493c4c7c68b3635868e4d93d210783c2ded2a871f185ca09a2053168c05b26a1e056ff6edf68f
6 participants
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.