python-bitcoinaddress icon indicating copy to clipboard operation
python-bitcoinaddress copied to clipboard

skip magic byte check

Open neonknight opened this issue 7 years ago • 3 comments

skip magic byte check by default. else valid btc addresses starting with 3 or bc1 are considered invalid. as magic byte values for these addresses is somewhat unintuitive I'd simply omit this check and leave it to the user to define a value for altcoins only.

neonknight avatar Oct 25 '18 09:10 neonknight

Thanks for your input! I understand your issue, and agree partially with your suggested code, but there's 2 points I would like to make:

  1. As far as I understand Bech32 addresses will not validate using this code, as they use a different encoding (not base58). So I don't think your patch solves this. I hope to add bc1 addresses at some point - I'll add it to my todo list (which is not very small, mind :) but it's good for our own website, too, so I'll probably get to it in one of the upcoming updates).

  2. I would like to suggest changing the default value not to None, but to something like (0, 5), the version bytes for non-testnet, non-bc1 bitcoin address types (0 for p2pkh and 5 for p2sh, which includes segwit addresses). I like supporting None as an option to allow any prefix, but by providing the suggested tuple as default value the code by default does not validate on other crypto addresses (such as LTC, though that shares the 5 unfortunately), so it's less easy to make mistakes (and I think the code is a bit more informative on top of that).

Please let me know if you agree, if so, I'll patch the lib accordingly.

johnnydebris avatar Oct 25 '18 15:10 johnnydebris

Hi, sorry for the late reply This sounds fine to me.

  1. so far Bech32 addresses don't seem to be popular (as not recommended), therefore validation for those addresses is probably not of urgent matter.
  2. a list of two items is still of manageable size, therefore this seems to be a working approach. It's just a bit non-intuitive, that 1 and 3 are matched by 0 and 5 - but a short comment should clarify the purpose/meaning of those numbers. Therefore: please patch as you suggested.

neonknight avatar Nov 01 '18 14:11 neonknight

Fixed in master: the default value is now (0, 5), I added support for None as value to skip magic byte validation entirely (any other value resolving to False also works, so you could pass an empty list or tuple to skip magic byte validation, too) and I changed the comment so it's more informative about the magic byte (oh, also renamed the argument to 'decimal_prefixes', since it's clearer, 'magic byte' is not used often (generally it's 'version byte') and 'decimal_prefixes' is more descriptive of the expected value).

Thanks a lot for the suggestions, if you have more ideas, don't hesitate to let us know!

(Oh, bech32 seems to indeed not be very pressing, but I will at some point add them I think, so if you're interested make sure to check the git commits every once in a while...)

johnnydebris avatar Nov 02 '18 09:11 johnnydebris