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

Remove default sizes #22

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

Remove default sizes #22

wants to merge 3 commits into from

Conversation

karelbilek
Copy link
Contributor

I didn't like the TX_INPUT_PUBKEYHASH and TX_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

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

@dcousens dcousens Aug 15, 2017

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.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍

@dcousens
Copy link
Contributor

It would be a breaking change too, as you are forcing script.length to be defined?

@karelbilek
Copy link
Contributor Author

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

@dcousens
Copy link
Contributor

ACK to breaking change.
Options object for the defaults though. 👍

@dcousens
Copy link
Contributor

dcousens commented Aug 15, 2017

I don't want users constantly guessing what their input lengths might be though.

Could we provide an options object that had nice defaults for unspecified input lengths maybe?

@dcousens
Copy link
Contributor

This is high priority.

@dcousens dcousens self-assigned this Sep 14, 2018
@dcousens dcousens removed their assignment Sep 23, 2019
@tiero
Copy link
Contributor

tiero commented Feb 24, 2020

Any bandwidth to get this through?

Copy link
Contributor

@tiero tiero left a 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
Copy link
Contributor

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 🤔

Copy link

@itsMikeLowrey itsMikeLowrey Mar 5, 2020

Choose a reason for hiding this comment

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

I think @dcousens has proposed that an option object with some defaults in case the user doesn't feed in input script and length.

@karelbilek
Copy link
Contributor Author

Comments should be addressed

You are free to overtake this PR, I have stopped any bitcoin-related development :)

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