-
Notifications
You must be signed in to change notification settings - Fork 36.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Issue#1643: Coinselection prunes extraneous inputs from ApproximateBestSubset #4906
Conversation
This is the first time I am trying to collaborate on an open-source project, please feel free to point out when I am doing something wrong. |
Automatic sanity-testing: PASSED, see http://jenkins.bluematt.me/pull-tester/p4906_3b92ce7ad950ceba14f1cc6ad637923f71ca61be/ for binaries and test log. |
I was made aware by @sipa that a candidate set that ended up matching the targetValue with the addition of a dust UTXO would still be picked immediately. That seems to be correct. However, unless one is trying to make a minuscule payment, it seems that the likelihood to add a bigger UTXO at some point that would exceed the target should be sufficiently larger, than the chance that you'd only add inputs that inch closer and closer to the target to match it finally without exceeding it. It should rather be unlikely to even happen in 1000 iterations (gut feeling), so the patch should work fine in the majority of situations. |
Have you simulated this, e.g. how a wallet progressed over time? I would expect this to result in grinding down the wallet into lots of dust change over time even worse than the current approach. Generally, so long as it doesn't result in bloating things up to the point where the transaction confirms slowly, we should generally prefer to make transactions bigger (under the rational that transaction fees will increase over time or at worst stay constant). E.g. If you already have a change output, a pass that looks at the scriptpubkeys you're spending and keeps adding more inputs assigned to the same pubkeys until the fee increase is some threshold or the change exceeds 2x the payment value (or something like that) would probably result in lower total transaction fees over time. (and better privacy) |
@gmaxwell I had not simulated it in advance. I will do so though, just haven't gotten around to it yet. Perhaps I'll get around to it this evening, otherwise I will probably get around to it tomorrow. |
@gmaxwell So, I have done some simulations. So far I have tried a few different scenarios, but I ended up being limited at around 10k utxo and 1000 operations, as my python implementation was not as fast as I had hoped. |
Please keep us updated on your progress. If you'd like some more eyes on your testing code, feel free to point it out. |
@gmaxwell The code for my simulation can be found here: https://github.com/Xekyo/CoinSelectionSimulator It is working now, and can be executed in bearable time, but the code could be a bit clearer. I am currently in my exam period and haven't had as much time to work on it, as I would like. I have tried a few different strategies to select coins, and have been experimenting with different distributions of incoming and outgoing payments. Different strategies:
Some results: Experiment 1: ... writing up my results I realize that I will want to create a .csv instead of the current text-form result, sorry gonna go back to studying now, but I'll probably be able to do it in the next break. Some questions I haven't been able to answer satisfyingly:
|
Hi, (When I created the .csv files with my output, I realized that my random models would sometimes generate spending instructions that asked for more than the wallet's current value. While those were were ignored as impossible, they still got into the statistics, so I wanted to fix that first.) I've been thinking about what one might want to be optimizing for. This is what I got so far:
|
Very interesting results! |
It's preferable to spend them: since it reduces the storage for a minimal full node (see the pruning patches in #4701)... subject to the restriction that you don't want someone to be able to gratitiously increase your transaction costs by sending you tiny utxo. |
Perhaps I can help. I have data from MoneyPot.com's hot wallet which you can use for simulation: https://gist.github.com/RHavar/7cd6f3fcf2bd3e485458 The positive amounts are amounts deposited, the negative amounts are sends. The data is sorted over time (oldest at the top, newest at the bottom) denominated in bitcoins. Things to note:
|
This PR has been open for a while, but garnered no ACKs. The author seems to have put a fair amount of time and thought into it. However, this definitely needs more review and testing. Ping, maintainers/testers/helpers? |
I've created another testcase using the MoneyPot.com data. On that one, I get different results yet again, this time the Regular wallet has the least UTXO in the end. Looking over the results, I noticed that all wallets created a change output in the single digit satoshis. Do I remember correctly that the wallet shouldn't create Dust Outputs? Shouldn't the change be at least 540 satoshis? If so, I should probably fix that behavior still and then have another look. However, if it ends up looking as promising as today, I would propose to just expire this Pull-Request. |
From what I understand ApproximateBestSubset is an approximate algorithm for the following:
The "approximate" part means that it may select a solution which is not the optimal one (e.g. sum(X)-nTargetValue is not really as small as possible). Your fix drops elements i from the result X for which sum(X)-i is still >= nTargetvalue, which if possible leads to a trivially better solution. From what I understand this can improve the result because vValue is sorted from low to high, and elements are added in that order, it can happen that an element is added to get the sum above nTargetValue, but makes earlier-added small elements redundant.
I've managed to reproduce this as well. For e.g. these input values:
ApproximateBestSubset sometimes selects Concept ACK - this cannot make things worse. Would be nice to have a unit test. |
Hi, Am 20.11.2015 11:11 schrieb "MarcoFalke" notifications@github.com:
I'd be interested to take another look, but I'm currently traveling. Please Xekyo |
@laanwj I've edited my fork to move the post-processing step to the end of ApproximateBestSubset. However, this patch may cause fewer dust outputs to be spent which contradicts Greg's assessment above. Are you sure about "cannot make things worse"? I feel my simulations have been somewhat inconclusive on that point. |
A further pass over the available inputs has been added to ApproximateBestSubset after a candidate set has been found. It will prune any extraneous inputs in the selected subset, in order to decrease the number of input and the resulting change.
23838db
to
123f22c
Compare
@MarcoFalke I realized my mistake and fixed it. After all my commits, I did the rebase as you suggested, and pushed to "Fix-#1643". I hope I did that right. :) |
I've added a simple test with
with an expected Sorry, I'm not sure how I would run the test on my machine right now, so I figured I'd just push it. I'll check back tomorrow. |
|
||
BOOST_FIXTURE_TEST_SUITE(coinselection_tests, BasicTestingSetup) | ||
|
||
BOOST_AUTO_TEST_CASE(sanity) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think you can just add this to src/wallet/test/wallet_tests.cpp
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah thanks, I had been wondering why there was no wallet tests in the regular test directory.
ACK |
86b99d4
to
78cc1af
Compare
I rebased and used fixup on the commits that only fixed previous mistakes. |
78cc1af
to
d4086a2
Compare
7877ccf
to
fc0f52d
Compare
@laanwj Travis had cleared my patch just when I realized that I had also messed up the indentation on the test. I've rebased it into two commits, one commit for the code, and one for the test. I expect this to turn green again momentarily. Sorry for keeping Travis so busy. :( Is there anything else that needs to be done about this PR? |
Wallet code itself looks good! Post-merge Tested ACK. @xekyo Thanks for sticking with this so long! There seems to be a small issue with the test code but I will create another PR for this... Note @laanwj: vValue is not sorted from low to high but from high to low but I think you meant it the right way. ;) |
This is a combination of 3 commits. - Coinselection prunes extraneous inputs from ApproximateBestSubset A further pass over the available inputs has been added to ApproximateBestSubset after a candidate set has been found. It will prune any extraneous inputs in the selected subset, in order to decrease the number of input and the resulting change. - Moved set reduction to the end of ApproximateBestSubset to reduce performance impact - Added a test for the pruning of extraneous inputs after ApproximateBestSet Github-Pull: #4906 Rebased-From: 5c03483 af9510e fc0f52d
This reverts PR bitcoin#4906, "Coinselection prunes extraneous inputs from ApproximateBestSubset". Apparently the previous behavior of slightly over-estimating the set of inputs was useful in cleaning up UTXOs. See also bitcoin#7664, bitcoin#7657, as well as 2016-07-01 discussion on #bitcoin-core-dev IRC.
This reverts PR bitcoin#4906, "Coinselection prunes extraneous inputs from ApproximateBestSubset". Apparently the previous behavior of slightly over-estimating the set of inputs was useful in cleaning up UTXOs. See also bitcoin#7664, bitcoin#7657, as well as 2016-07-01 discussion on #bitcoin-core-dev IRC.
Improvement for Issue#1643:
A further pass over the available inputs has been added to ApproximateBestSubset after a candidate set has been found. It will prune any extraneous inputs in the selected subset, in order to decrease the number of inputs and the resulting change.