bitcoinjs / bitcoinjs-lib

A javascript Bitcoin library for node.js and browsers.
MIT License
5.68k stars 2.1k forks source link

Taproot address validation #1919

Closed DaniSchenk closed 5 months ago

DaniSchenk commented 1 year ago

In v6.0.1 I was able to validate taproot addresses like this:

import * as bitcoinjs from "bitcoinjs-lib";
const value = "bc1pxe5uh7cst3p3qzsuzng0r94uv0tn4wfzum2jflph0km6sjjjp5cqc4906u";

const isBitcoin = !!bitcoinjs.address.toOutputScript(value, bitcoinjs.networks.bitcoin);

I got a warning (Sending to a future segwit version address can lead to loss of funds. End users MUST be warned carefully...) but it validated the address.

In v6.1.0 I had to adjust to validation successfully. Otherwise, I got the error No ECC Library provided. You must call initEccLib() with a valid TinySecp256k1Interface instance if I did not initEccLib(). Based on this comment https://github.com/bitcoinjs/bitcoinjs-lib/issues/1889#issuecomment-1443792692 I installed a new lib to make it work again.

import * as bitcoinjs from "bitcoinjs-lib";
import * as ecc from "tiny-secp256k1";
const value = "bc1pxe5uh7cst3p3qzsuzng0r94uv0tn4wfzum2jflph0km6sjjjp5cqc4906u";

bitcoinjs.initEccLib(ecc);
const isBitcoin = !!bitcoinjs.address.toOutputScript(value, bitcoinjs.networks.bitcoin);

So a minor version change introduced a breaking change on my side. I'm curious if this is a fix, feature or bug 🤷‍♂️

junderw commented 1 year ago

I think the decision was made that anyone relying on the insecure warning-filled "validation" (it wasn't really validating anything other than the bech32 checksum, which is the whole reason for the warning) should have their app broken rather than including a backwards compatible way for them to continue not validating the p2tr address properly... or bumping major and any project who just never wants to bump major will never switch to proper validation.

TBH I personally was against including the future segwit version "(not-so-much) validation" logic, but there was a clarification in the BIP that stated that all future segwit versions "MUST NOT" hard fail... so our hands were semi-tied.

I understand that a strict interpretation of semver would state that this change needed to be a major bump, but in this specific case we decided against it.

We should probably document this decision in the warning itself so people who see that warning all the time know that if we add a new segwit version in the future and there's a more strict validation requirement, their app might break on minor bump.

DaniSchenk commented 1 year ago

Thank you so much for your explanation @junderw.

I would like to validate if a user provided string is a bitcoin address that can be used as a tx recipient in general. Since you seem unconvinced about the "validation", do you suggest another approach to test if a string is a bitcoin address?

junderw commented 1 year ago

That's the thing, a bech32m address with a future segwit version (v2~v16) IS a "valid" bitcoin address according to the BIP.

The only time you should see those warnings is when a new version of segwit is available but your bitcoinjs-lib's version didn't support that segwit version.

I say "valid" with quotes because it's a higher chance of resulting in an unspendable output, but technically the definition of a "valid" address doesn't match 1:1 to a "fully spendable output"

ie. 1BitcoinEaterAddressDontSendf59kuE is a "valid" address, but coins sent to it are virtually unspendable.

Perhaps the address API should return an object with a warnings array of strings that should be shown to a user.

tansanDOTeth commented 10 months ago

In v6.0.1 I was able to validate taproot addresses like this:

import * as bitcoinjs from "bitcoinjs-lib";
const value = "bc1pxe5uh7cst3p3qzsuzng0r94uv0tn4wfzum2jflph0km6sjjjp5cqc4906u";

const isBitcoin = !!bitcoinjs.address.toOutputScript(value, bitcoinjs.networks.bitcoin);

I got a warning (Sending to a future segwit version address can lead to loss of funds. End users MUST be warned carefully...) but it validated the address.

In v6.1.0 I had to adjust to validation successfully. Otherwise, I got the error No ECC Library provided. You must call initEccLib() with a valid TinySecp256k1Interface instance if I did not initEccLib(). Based on this comment #1889 (comment) I installed a new lib to make it work again.

import * as bitcoinjs from "bitcoinjs-lib";
import * as ecc from "tiny-secp256k1";
const value = "bc1pxe5uh7cst3p3qzsuzng0r94uv0tn4wfzum2jflph0km6sjjjp5cqc4906u";

bitcoinjs.initEccLib(ecc);
const isBitcoin = !!bitcoinjs.address.toOutputScript(value, bitcoinjs.networks.bitcoin);

So a minor version change introduced a breaking change on my side. I'm curious if this is a fix, feature or bug 🤷‍♂️

Thanks! I couldn't figure out what was happening, and this helped me progress. The fix is not obvious at all.