-
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
Remove default sizes #22
base: master
Are you sure you want to change the base?
Conversation
@@ -7,15 +7,15 @@ function utxoScore (x, feeRate) { | |||
return x.value - (feeRate * utils.inputBytes(x)) | |||
} | |||
|
|||
module.exports = function coinSelect (utxos, outputs, feeRate) { | |||
module.exports = function coinSelect (utxos, outputs, feeRate, changeInputLengthEstimate, changeOutputLength) { |
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.
If we did this, it would have to be an options
object with defaults.
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 would be a breaking change too, as you are forcing |
Yes. I thought it would be better, since the caller can't accidentally forget it, at which point it fallbacks to default - and maybe wrong, if it's P2SH/native segwit/.... It is an API breaking change |
ACK to breaking change. |
I don't want users constantly guessing what their input lengths might be though. Could we provide an |
This is high priority. |
Any bandwidth to get this through? |
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.
Comments should be addressed
|
||
function inputBytes (input) { | ||
return TX_INPUT_BASE + (input.script ? input.script.length : TX_INPUT_PUBKEYHASH) | ||
return TX_INPUT_BASE + input.script.length |
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.
not clear what happens if the input provided has no script 🤔
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.
You are free to overtake this PR, I have stopped any bitcoin-related development :) |
I didn't like the
TX_INPUT_PUBKEYHASH
andTX_OUTPUT_PUBKEYHASH
in utils.js, because it might cause problems later (with some refactoring, or segwit lengths, etc).It turned out to be bigger code change than I thought....
it's not that burning, and it's a big change in the end, so maybe it can wait after the other PRs