Skip to content
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

Open
wants to merge 10 commits into
base: master
Choose a base branch
from
Open

Adding Branch and Bound algorithm #13

wants to merge 10 commits into from

Conversation

karelbilek
Copy link
Contributor

@karelbilek karelbilek commented Jul 2, 2017

#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.

@karelbilek
Copy link
Contributor Author

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.

@karelbilek
Copy link
Contributor Author

(both the scala algorithm and the C++ implementations are MIT licence, as is this code :) )

Copy link

@murchandamus murchandamus left a 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?


return utils.finalize(inputs, outputs, feeRate)
} else {
const fee = feeRate * utxos.reduce(function (a, x) {

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!

Copy link
Contributor Author

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)

Copy link
Contributor Author

@karelbilek karelbilek Jul 2, 2017

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

Copy link
Contributor Author

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

Copy link
Contributor Author

@karelbilek karelbilek Jul 2, 2017

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.

Copy link
Contributor Author

@karelbilek karelbilek Jul 2, 2017

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

Copy link

@murchandamus murchandamus Jul 3, 2017

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

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
Copy link

@murchandamus murchandamus Jul 2, 2017

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.

Copy link
Contributor Author

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor Author

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

Copy link
Contributor Author

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

@karelbilek
Copy link
Contributor Author

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 :)

@dcousens
Copy link
Contributor

dcousens commented Jul 6, 2017

Sorry for the delay here, reading now.

@karelbilek
Copy link
Contributor Author

karelbilek commented Jul 6, 2017 via email

})

var selected = search(effectiveUtxos, outAccum, costOfChange)
if (selected != null) {
Copy link
Contributor

@dcousens dcousens Jul 6, 2017

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

Copy link
Contributor Author

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;?

Copy link
Contributor Author

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

Copy link
Contributor Author

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

@dcousens
Copy link
Contributor

dcousens commented Jul 6, 2017

Looks good to me, at least for merge/ongoing research/testing :) - not for release until everything is tested though.

var costPerInput = utils.inputBytes({}) * feeRate
var costOfChange = costPerInput + costPerOutput

var outAccum = utils.sumOrNaN(outputs)
Copy link
Contributor

@dcousens dcousens Jul 6, 2017

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

Copy link
Contributor Author

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" :)

Copy link
Contributor

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

Copy link
Contributor

@dcousens dcousens Jul 12, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

NaNs 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.

@karelbilek
Copy link
Contributor Author

Hey, I was not really able to look at the code (holiday :)), will go through the notes etc

@karelbilek
Copy link
Contributor Author

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
Copy link
Contributor Author

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)
Copy link
Contributor Author

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

@karelbilek
Copy link
Contributor Author

Hm, I thought that for tests, I will copy blackjack tests, since they should be similar... but there are not blackjack tests 😭 😢 ok then

@karelbilek
Copy link
Contributor Author

karelbilek commented Jul 18, 2017

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.

@karelbilek
Copy link
Contributor Author

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.

@karelbilek
Copy link
Contributor Author

karelbilek commented Jul 18, 2017

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)

@dcousens
Copy link
Contributor

@Runn1ng if the fee has no meaningful result, maybe NaN?

@karelbilek
Copy link
Contributor Author

OK, bcash is finally solved (what a time sink), so I have time to return back to work on this :)

@karelbilek
Copy link
Contributor Author

I see bitcoin core have added a waste metric and bunch of other stuff to BnB

@mahnunchik
Copy link

Any news?

@karelbilek
Copy link
Contributor Author

lol. Not from me :) I stopped working on any bitcoin-related stuff, 0 mood to return

@mahnunchik
Copy link

@karelbilek what happened?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants