-
Notifications
You must be signed in to change notification settings - Fork 102
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
Adding Branch and Bound algorithm #13
base: master
Are you sure you want to change the base?
Conversation
Code comments are welcome :) it's a big chunk of code. The comments in there are almost directly from the bitcoin-core PR I will also try to port the bitcoin-core tests in the PR here. |
(both the scala algorithm and the C++ implementations are MIT licence, as is this code :) ) |
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.
Untested, but LGTM, as I've remarked I haven't checked whether you're accounting for the fees of the outputs and transaction overhead elsewhere. Did I miss the tests, or are there none?
branchandbound.js
Outdated
|
||
return utils.finalize(inputs, outputs, feeRate) | ||
} else { | ||
const fee = feeRate * utxos.reduce(function (a, x) { |
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.
Just in case this isn't happening elsewhere, be sure that you don't forget accounting for the fees of the transaction overhead of 10 bytes and requested outputs to the target of the selection!
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.
Hm. Where is that hapenning in the achow101 PR code? I don't see it. (But maybe it happens before it goes into wallet.cpp CWallet::SelectCoinsMinConf
)
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.
Nope, the C++ doesn's seem to do that either.
It is not done in the coinselection.cpp - https://github.com/achow101/bitcoin/blob/19761499d5657b1f272a2fb04185dacf218fb104/src/wallet/coinselection.cpp
And it's not done in wallet.cpp - here the nvalue is just sum of the outputs
https://github.com/achow101/bitcoin/blob/19761499d5657b1f272a2fb04185dacf218fb104/src/wallet/wallet.cpp#L2437
nvaluetoselect is just nvalue here (nFeeRet is 0 in the first pass, and BnB is used only in the first pass)
https://github.com/achow101/bitcoin/blob/19761499d5657b1f272a2fb04185dacf218fb104/src/wallet/wallet.cpp#L2503
Here the SelectCoins is called, which calls (through some layers) BnB with the same target
https://github.com/achow101/bitcoin/blob/19761499d5657b1f272a2fb04185dacf218fb104/src/wallet/wallet.cpp#L2555
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 it's actually a bug in the c++ code, I have added a comment to the PR, we'll see
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.
It's a bug in the c++ code, now corrected. Anyway though, I noticed something weird.
When I corrected the error and add the overhead, the algorithm performs slightly worse. When I run, on the same randomly generated data, both the BnB with incorrect (lower) target and BnB with correct target, the BnB with incorrect targets always has better results. The difference is small, but consistent.
I have no idea why.
I will anyway add some tests finally, maybe there is some other subtle bug that I haven't found.
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.
Oooh the issue is in the simulation, I think it allows negative fees. Which is what happens. no I was wrong
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.
It could be that now that the transaction size is properly estimated, the allowance window for the exact match actually is too big. You might want to try with a "costOfChange" that is actually only the saved change output's cost (or just discount the cost of the saved input). That would be the lower bound on the saved cost, but may cause the algorithm to find fewer matches or require more inputs to find matches.
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 can replicate the results with your code too -
murchandamus/CoinSelectionSimulator#5
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.
[for other readers: the discussion continued into the bitcoind pull request]
@@ -0,0 +1,143 @@ | |||
var utils = require('./utils') | |||
|
|||
var maxTries = 1000000 |
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.
My gut feeling was 100k, you have 1M here. That might be very long on a huge wallet in an unfortunate UTXO pool and target combination. Might want to check what amount of time that allows, especially since JavaScript is probably slower than C++ in evaluation.
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.
A typo I think! Thanks
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.
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.
but yeah 100k should be enough
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 have left 1M here; in my experience it's very fast anyway, even in javascript on browser/in node
I haven't done the tests yet; as I said "I will also try to port the bitcoin-core tests in the PR here", which I will do today :) |
Sorry for the delay here, reading now. |
Np, lots of comments here :))
…On Jul 6, 2017 8:46 AM, "Daniel Cousens" ***@***.***> wrote:
Sorry for the delay here, reading now.
—
You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub
<#13 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAGZ8XRZA5BUk1_X3jtX53zJzV9zyXTvks5sLIKsgaJpZM4OLeIj>
.
|
}) | ||
|
||
var selected = search(effectiveUtxos, outAccum, costOfChange) | ||
if (selected != null) { |
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'll add standard-js
to the repo to avoid style nits
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.
standard-js
is already in there.
!= null
is fine for it, since it checks both undefined and null. Maybe it would be better to somehow return "empty" value from the "inner" function more explicitly (to avoid "null !== undefined" shenanigans), than just return;
?
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 originally thought about throwing Errors, but that's also kind of ugly
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 will leave this like this
Looks good to me, at least for merge/ongoing research/testing :) - not for release until everything is tested though. |
branchandbound.js
Outdated
var costPerInput = utils.inputBytes({}) * feeRate | ||
var costOfChange = costPerInput + costPerOutput | ||
|
||
var outAccum = utils.sumOrNaN(outputs) |
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.
outAccum
could be NaN
here, is that safe?
search
doesn't appear to handle it explicitly, though it appears it'd simply burn through tries
and exit
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 was not sure how would the NaNs come to the outputs and how to handle them exactly, frankly, so I just delegated it to "do it later" :)
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.
Handling them gracefully will be required for release/publish 👍 , but no issue for now
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.
NaN
s appear (intentionally, returned by sumOrNaN
) if any expected field is undefined
, or null
, or not an integer.
It proliferates NaN
throughout the math quickly such that an error can be caught, and is unusable as a Bitcoin value.
tl;dr, it can be useful too such as in split
or break
.
Hey, I was not really able to look at the code (holiday :)), will go through the notes etc |
Note: has to be added to strategy list
I have now added factor for change of cost |
module.exports = function branchAndBound (utxos, outputs, feeRate) { | ||
if (!isFinite(utils.uintOrNaN(feeRate))) return {} | ||
|
||
var costPerOutput = utils.outputBytes({}) * feeRate |
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.
TODO: as written, this does not take segwit/different input/ouptut sizes into account
|
||
var costPerOutput = utils.outputBytes({}) * feeRate | ||
var costPerInput = utils.inputBytes({}) * feeRate | ||
var costOfChange = Math.floor((costPerInput + costPerOutput) * factor) |
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.
this doesn't take segwit into account; but neither does utils.finalize
Hm, I thought that for tests, I will copy blackjack tests, since they should be similar... but there are not blackjack tests 😭 😢 ok then |
Hm, I now have no idea, what to return in BnB algorithm as fee, when the algorithm fails to find a match I will probably return just 0, since the strategy doesn't return anything meaningful for this. |
I have added some tests (mostly copying from accumulative.json) and fixed NaN issues. It should be working. However, the costOfChange/costPerInput/costPerOutput will need to have a revision with segwit. |
Hm. I don't know how to display the missed branches in the coverage... edit: oh, nyc's HTML export is useful. Correcting, comitting. Probably ready to merge now? Maybe the naming is confusing (bnb at some places, "branchandbound" at others) |
@Runn1ng if the fee has no meaningful result, maybe |
OK, bcash is finally solved (what a time sink), so I have time to return back to work on this :) |
I see bitcoin core have added a waste metric and bunch of other stuff to BnB |
Any news? |
lol. Not from me :) I stopped working on any bitcoin-related stuff, 0 mood to return |
@karelbilek what happened? |
#10
In the end, I used the C++ code from core PR as a reference ( bitcoin/bitcoin#10637 ), not the scala code.
It seems to be working, and performing rather well! Although it matches only in <10% of cases in the simulation, the reduction of the fees is significant.
The only question is what to take as a fallback. In the original paper and in the scala code that goes with it, random shuffle + accumulative is being used. In this module simulation, min+accumulative has the best results. (Even with the total costs from #11 )
However, when I tried to implement the "minimal" fallback into the scala code, the utxo set in the big example exploded, which caused larger total cost, plus the BnB search was significantly slower.
As I wrote in the linked pull request, I will try to port the big example into this code, what will happen.