-
Notifications
You must be signed in to change notification settings - Fork 101
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?
Changes from 4 commits
86ae459
2c8d1ef
37d2d56
e04cede
2642862
3d806ec
479acad
1b07b04
f11ed41
b0f92ab
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,143 @@ | ||
var utils = require('./utils') | ||
|
||
var maxTries = 1000000 | ||
|
||
function calculateEffectiveValues (utxos, feeRate) { | ||
return utxos.map(function (utxo) { | ||
if (isNaN(utils.uintOrNaN(utxo.value))) { | ||
return { | ||
utxo: utxo, | ||
effectiveValue: 0 | ||
} | ||
} | ||
|
||
var effectiveFee = utils.inputBytes(utxo) * feeRate | ||
var effectiveValue = utxo.value - effectiveFee | ||
return { | ||
utxo: utxo, | ||
effectiveValue: effectiveValue | ||
} | ||
}) | ||
} | ||
|
||
module.exports = function branchAndBound (utxos, outputs, feeRate, factor) { | ||
if (!isFinite(utils.uintOrNaN(feeRate))) return {} | ||
|
||
var costPerOutput = utils.outputBytes({}) * feeRate | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 costPerInput = utils.inputBytes({}) * feeRate | ||
var costOfChange = Math.floor((costPerInput + costPerOutput) * factor) | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. this doesn't take segwit into account; but neither does |
||
|
||
var outAccum = utils.sumOrNaN(outputs) + utils.transactionBytes([], outputs) * feeRate | ||
|
||
var effectiveUtxos = calculateEffectiveValues(utxos, feeRate).filter(function (x) { | ||
return x.effectiveValue > 0 | ||
}).sort(function (a, b) { | ||
return b.effectiveValue - a.effectiveValue | ||
}) | ||
|
||
var selected = search(effectiveUtxos, outAccum, costOfChange) | ||
if (selected != null) { | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I will leave this like this |
||
var inputs = [] | ||
|
||
for (var i = 0; i < effectiveUtxos.length; i++) { | ||
if (selected[i]) { | ||
inputs.push(effectiveUtxos[i].utxo) | ||
} | ||
} | ||
|
||
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 commentThe 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 commentThe 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 nvaluetoselect is just nvalue here (nFeeRet is 0 in the first pass, and BnB is used only in the first pass) Here the SelectCoins is called, which calls (through some layers) BnB with the same target There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 commentThe 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 commentThe 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 commentThe 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 commentThe reason will be displayed to describe this comment to others. Learn more. I can replicate the results with your code too - There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [for other readers: the discussion continued into the bitcoind pull request] |
||
return a + utils.inputBytes(x) | ||
}, 0) | ||
|
||
return { | ||
fee: fee | ||
} | ||
} | ||
} | ||
|
||
// Depth first search | ||
// Inclusion branch first (Largest First Exploration), then exclusion branch | ||
function search (effectiveUtxos, target, costOfChange) { | ||
if (effectiveUtxos.length === 0) { | ||
return | ||
} | ||
|
||
var tries = maxTries | ||
|
||
var selected = [] // true -> select the utxo at this index | ||
var selectedAccum = 0 // sum of effective values | ||
|
||
var traversingExclusion = [] // true -> traversing exclusion branch at this index | ||
|
||
var done = false | ||
var backtrack = false | ||
|
||
var remaining = effectiveUtxos.reduce(function (a, x) { | ||
return a + x.effectiveValue | ||
}, 0) | ||
|
||
var depth = 0 | ||
while (!done) { | ||
if (tries <= 0) { // Too many tries, exit | ||
return | ||
} else if (selectedAccum > target + costOfChange) { // Selected value is out of range, go back and try other branch | ||
backtrack = true | ||
} else if (selectedAccum >= target) { // Selected value is within range | ||
done = true | ||
} else if (depth >= effectiveUtxos.length) { // Reached a leaf node, no solution here | ||
backtrack = true | ||
} else if (selectedAccum + remaining < target) { // Cannot possibly reach target with amount remaining | ||
if (depth === 0) { // At the first utxo, no possible selections, so exit | ||
return | ||
} else { | ||
backtrack = true | ||
} | ||
} else { // Continue down this branch | ||
// Remove this utxo from the remaining utxo amount | ||
remaining -= effectiveUtxos[depth].effectiveValue | ||
// Inclusion branch first (Largest First Exploration) | ||
selected[depth] = true | ||
selectedAccum += effectiveUtxos[depth].effectiveValue | ||
depth++ | ||
} | ||
|
||
// Step back to the previous utxo and try the other branch | ||
if (backtrack) { | ||
backtrack = false // Reset | ||
depth-- | ||
|
||
// Walk backwards to find the first utxo which has not has its second branch traversed | ||
while (traversingExclusion[depth]) { | ||
// Reset this utxo's selection | ||
if (selected[depth]) { | ||
selectedAccum -= effectiveUtxos[depth].effectiveValue | ||
} | ||
selected[depth] = false | ||
traversingExclusion[depth] = false | ||
remaining += effectiveUtxos[depth].effectiveValue | ||
|
||
// Step back one | ||
depth-- | ||
|
||
if (depth < 0) { // We have walked back to the first utxo and no branch is untraversed. No solution, exit. | ||
return | ||
} | ||
} | ||
|
||
if (!done) { | ||
// Now traverse the second branch of the utxo we have arrived at. | ||
traversingExclusion[depth] = true | ||
|
||
// These were always included first, try excluding now | ||
selected[depth] = false | ||
selectedAccum -= effectiveUtxos[depth].effectiveValue | ||
depth++ | ||
} | ||
} | ||
tries-- | ||
} | ||
|
||
return selected | ||
} |
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.
nope, you have 1M here too :))
https://github.com/Xekyo/CoinSelectionSimulator/blob/master/src/main/scala/one/murch/bitcoin/coinselection/StackEfficientTailRecursiveBnB.scala#L9
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