test: coinselection edge cases #24580
Conversation
S3RK
force-pushed
the
coinselection_tests
branch
from
dc6c808
to
1607529
Compare
| assert_equal(len(tx['vin']), 1) | ||
| assert_equal(len(tx['vout']), 2) | ||
|
|
||
| # if small coins > MIN_CHANGE than we just add one small coin to the big one and create change |
There was a problem hiding this comment.
This comment confuses me. After paying the fees, the big coin doesn't have enough funds to pay fees and create the output here, so we would always need both coins.
There was a problem hiding this comment.
Oops. The idea was that you have more than 1 small coin. If small coins > MIN_CHANGE that the tests in test_one_big_and_many_small_coins doesn't work. Any idea how to improve that? For now, added more than one small coin in this test case
S3RK
force-pushed
the
coinselection_tests
branch
from
1607529
to
e378883
Compare
|
Retouched comments to address Xekyo's feedback. |
|
Looks good. What is the next step you plan for this PR? How can one help? |
|
I thought this PR can be merged roughly as it is (addressing all the concerns of course), as I believe the test is valuable in itself. As a follow up I'd like to fix some of the inefficiencies of coin selection by increasing BnB upper limit. But for that we need to improve how we drop change for fees. I tried one approach in #23367 but it doesn't work that well. So I'm going splitting #23367 into parts that could be merged independently (the first one -> #24649). Let's talk on the next wallet meeting. |
|
There is probably a conflict with #24494. |
|
While I appreciate more tests, these tests seem to be extremely specific to the way that coin selection currently works and look like they would start failing as soon as we change something, such as removing knapsack. So I'm not sure if we actually want to have these tests if they are going to be removed and/or require a ton of recalculation for future coin selection changes. If we do want these tests, then I don't think it should use legacy addresses so much as hopefully we won't make those by default soon. |
Agree, these are not your normal tests, primarily because it's hard to define the "right" or "expected" outcome. I created these tests to expose some shortcomings of current coin selection and justify increasing BnB limit (#24752). It'd be nice to have them around in some form to avoid regressions, maybe we can incorporate them somehow into simulations. wdyt?
|
|
To throw my relatively unproductive opinion into the mix, this is a staggeringly large number of magic numbers. Ideally all numbers in tests are "obvious" or programmatic. It's a nightmare to maintain otherwise. Didn't read or think enough about overall thrust of the tests. |
|
Thanks for the feedback. Maybe a better way to verify coin selection would to integrate some simulation/benchmark into this repo. |
This PR adds tests for some edge cases in coin selection.
There are 4 sections by the type of utxo pool: