Adding Branch and Bound algorithm #13

karelbilek · 2017-07-02T02:37:27Z

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 · 2017-07-02T02:38:46Z

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 · 2017-07-02T02:47:53Z

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

murchandamus

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?

murchandamus · 2017-07-02T04:42:35Z

branchandbound.js

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


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!

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)

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

I think it's actually a bug in the c++ code, I have added a comment to the PR, we'll see

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.

~~Oooh the issue is in the simulation, I think it allows negative fees. Which is what happens.~~ no I was wrong

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.

I can replicate the results with your code too -
murchandamus/CoinSelectionSimulator#5

[for other readers: the discussion continued into the bitcoind pull request]

murchandamus · 2017-07-02T04:51:30Z

branchandbound.js

@@ -0,0 +1,143 @@
+var utils = require('./utils')
+
+var maxTries = 1000000


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.

A typo I think! Thanks

nope, you have 1M here too :))

https://github.com/Xekyo/CoinSelectionSimulator/blob/master/src/main/scala/one/murch/bitcoin/coinselection/StackEfficientTailRecursiveBnB.scala#L9

but yeah 100k should be enough

I have left 1M here; in my experience it's very fast anyway, even in javascript on browser/in node

karelbilek · 2017-07-02T13:06:06Z

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 · 2017-07-06T06:46:04Z

Sorry for the delay here, reading now.

karelbilek · 2017-07-06T06:52:06Z

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

dcousens · 2017-07-06T06:55:16Z

branchandbound.js

+  })
+
+  var selected = search(effectiveUtxos, outAccum, costOfChange)
+  if (selected != null) {


!==, I'll add standard-js to the repo to avoid style nits

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

I originally thought about throwing Errors, but that's also kind of ugly

I will leave this like this

dcousens · 2017-07-06T06:57:30Z

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

dcousens · 2017-07-06T06:58:49Z

branchandbound.js

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


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

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

Handling them gracefully will be required for release/publish 👍 , but no issue for now

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 · 2017-07-11T16:35:07Z

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

karelbilek · 2017-07-17T16:02:13Z

I have now added factor for change of cost

karelbilek · 2017-07-17T16:09:02Z

branchandbound.js

+module.exports = function branchAndBound (utxos, outputs, feeRate) {
+  if (!isFinite(utils.uintOrNaN(feeRate))) return {}
+
+  var costPerOutput = utils.outputBytes({}) * feeRate


TODO: as written, this does not take segwit/different input/ouptut sizes into account

karelbilek · 2017-07-17T16:12:26Z

branchandbound.js

+
+  var costPerOutput = utils.outputBytes({}) * feeRate
+  var costPerInput = utils.inputBytes({}) * feeRate
+  var costOfChange = Math.floor((costPerInput + costPerOutput) * factor)


this doesn't take segwit into account; but neither does utils.finalize

karelbilek · 2017-07-18T15:00:27Z

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 · 2017-07-18T15:41:43Z

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.

see bitcoin/bitcoin#10637 (comment)

karelbilek · 2017-07-18T19:24:37Z

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 · 2017-07-18T19:28:33Z

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 · 2017-07-19T00:41:51Z

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

karelbilek · 2017-08-10T11:37:18Z

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

karelbilek · 2018-10-04T04:59:27Z

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

mahnunchik · 2022-10-21T20:38:14Z

Any news?

karelbilek · 2022-10-26T20:38:10Z

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

mahnunchik · 2022-10-27T08:47:00Z

@karelbilek what happened?

karelbilek requested a review from dcousens July 2, 2017 02:44

karelbilek mentioned this pull request Jul 2, 2017

Coin Selection with Murch's algorithm bitcoin/bitcoin#10637

Merged

murchandamus reviewed Jul 2, 2017

View reviewed changes

karelbilek mentioned this pull request Jul 3, 2017

Lower target gets better results murchandamus/CoinSelectionSimulator#5

Closed

dcousens reviewed Jul 6, 2017

View reviewed changes

karelbilek added 4 commits July 12, 2017 19:11

Adding Branch and Bound algorithm

86ae459

Adding BnB to strategy comparison

2c8d1ef

Adding factor to BnB

37d2d56

Note: has to be added to strategy list

Using bnb with factors

e04cede

karelbilek commented Jul 17, 2017

View reviewed changes

karelbilek added 4 commits July 18, 2017 21:10

Handling NaN edge case

2642862

Removing useless array

3d806ec

see bitcoin/bitcoin#10637 (comment)

Returning {fee: 0} on failure

479acad

Adding BnB testing

1b07b04

karelbilek added 2 commits July 18, 2017 22:03

Adding BnB fixtures

f11ed41

Removing useless test

b0f92ab

dcousens added the feature label Sep 14, 2018

		@@ -0,0 +1,143 @@
		var utils = require('./utils')

		var maxTries = 1000000

Adding Branch and Bound algorithm #13

Are you sure you want to change the base?

Adding Branch and Bound algorithm #13

Conversation

karelbilek commented Jul 2, 2017 • edited Loading

karelbilek commented Jul 2, 2017

karelbilek commented Jul 2, 2017

murchandamus left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelbilek Jul 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelbilek Jul 2, 2017 • edited Loading

Choose a reason for hiding this comment

karelbilek Jul 2, 2017 • edited Loading

Choose a reason for hiding this comment

murchandamus Jul 3, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

murchandamus Jul 2, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelbilek commented Jul 2, 2017

dcousens commented Jul 6, 2017

karelbilek commented Jul 6, 2017 via email

dcousens Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcousens commented Jul 6, 2017 • edited Loading

dcousens Jul 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dcousens Jul 12, 2017 • edited Loading

Choose a reason for hiding this comment

karelbilek commented Jul 11, 2017

karelbilek commented Jul 17, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

karelbilek commented Jul 18, 2017

karelbilek commented Jul 18, 2017 • edited Loading

karelbilek commented Jul 18, 2017

karelbilek commented Jul 18, 2017 • edited Loading

dcousens commented Jul 19, 2017

karelbilek commented Aug 10, 2017

karelbilek commented Oct 4, 2018

mahnunchik commented Oct 21, 2022

karelbilek commented Oct 26, 2022

mahnunchik commented Oct 27, 2022

karelbilek commented Jul 2, 2017 •

edited

Loading

murchandamus left a comment •

edited

Loading

karelbilek Jul 2, 2017 •

edited

Loading

karelbilek Jul 2, 2017 •

edited

Loading

karelbilek Jul 2, 2017 •

edited

Loading

murchandamus Jul 3, 2017 •

edited

Loading

murchandamus Jul 2, 2017 •

edited

Loading

dcousens Jul 6, 2017 •

edited

Loading

dcousens commented Jul 6, 2017 •

edited

Loading

dcousens Jul 6, 2017 •

edited

Loading

dcousens Jul 12, 2017 •

edited

Loading

karelbilek commented Jul 18, 2017 •

edited

Loading

karelbilek commented Jul 18, 2017 •

edited

Loading