search

bitcoinjs / coinselect

An unspent transaction output (UTXO) selection module for bitcoin.
MIT License
175 stars 101 forks source link

Consider Output Address Types #51

Closed itsMikeLowrey closed 4 years ago

itsMikeLowrey commented 4 years ago

How does coinselect(utxos, targets, feeRate) get a correct transaction size when coinselect does not know what type of change address will be used? The change address could be a really large script that could push the fee rate below the required fee rate or make change not worth keeping. Great library and thanks so much for helping me clarify my understandings.

junderw commented 4 years ago
  1. Currently such a script is non-standard, so it won't propagate across the network.
  2. That said, currently it assumes the change will be 25 byte script (P2PKH). The only other standard output script that is larger is P2WSH which is 35 byte script. (not P2SH embedded P2WSH, which is 23)

So yes, your estimate would be 10 bytes off if your change output was a P2WSH script. Worst case scenario is around 5% (if your tx is as small as possible)

I'm open to suggestions on how to handle this.

itsMikeLowrey commented 4 years ago

Thanks for the info. I am building a multisig wallet that uses P2WSH and I see two possible solutions:

  1. We could change the default change script to P2WSH and have some fees be larger than expected but never smaller.
  2. Leave the default change address type to P2PKH and take in an optional parameter that allows you to specify the change address type.

I have looked over the code and I could make a pr doing either of these solutions. Thanks again for all the help.

junderw commented 4 years ago

I would say both.

  1. We should make the most expensive default
  2. We should allow people to set it to their wallet type to save some fees

I would also perhaps add a flag for low R, default false, that if true will shave one byte off each signature estimate (the R value being low)

Bitcoin Core now uses low R and BitcoinJS ECPair and bip32 objects support low R (default is off though).

itsMikeLowrey commented 4 years ago

I think I might be to add a changeBytes() with a similar footprint to outputBytes(), but with a P2WSH default size. I would also add a changeThreshold() similar to dustThreshold() and use it in finalize instead of dustThreshold(). In order for changeBytes() to have the output.script parameter, I was considering adding an optional changeScript parameter to coinselect(). As for the r low flag, I am unsure how best to add that since it seems to concern other parts of the library.

itsMikeLowrey commented 4 years ago

This PR seems to address the bulk of these issues. I will try and get a grasp on that pr and help out.