hirosystems / stacks.js

JavaScript libraries for identity, auth, storage and transactions on the Stacks blockchain.
https://stacks.js.org
MIT License
952 stars 310 forks source link

Encoding/Decoding P2SH Bitcoin addresses results in incorrect address version #969

Closed 2snEM6 closed 3 years ago

2snEM6 commented 3 years ago

Describe the bug

P2SH Bitcoin addresses are not being properly decoded into the correct address version.

What version of Blockstack.js are you using?

Latest version of stacks.js

To Reproduce

Let's say we have a valid Bitcoin P2SH address: 3MKGnaKrBZH4ruKCi2tqVveNPuAzfPdLTy (Which is actually one I picked from the list of stackers of the current cycle)

If we use the methods decodeBtcAddress and then getBTCAddress from https://github.com/blockstack/stacks.js/blob/a6b7da0586caa893c7cdf2ef4a3011de3248afe8/packages/stacking/src/utils.ts#L31

and https://github.com/blockstack/stacks.js/blob/a6b7da0586caa893c7cdf2ef4a3011de3248afe8/packages/stacking/src/utils.ts#L48

respectively as follows:

const { hashmode, data } = decodeBtcAddress('3MKGnaKrBZH4ruKCi2tqVveNPuAzfPdLTy');
const btcAddress = getBTCAddress(hashmode, data);

btcAddress results in a base58 string representation: jxrr98hLqRZbAkrcMZZZRZDst9DomPUZi which is not equal to jxrr98hLqRZbAkrcMZZZRZDst9DomPUZi

If instead we choose the correct version for the address, which is 5 and perform:

const btcAddress = getBTCAddress(5, data);

We get that btcAddress === 3MKGnaKrBZH4ruKCi2tqVveNPuAzfPdLTy

Expected behavior

Decoding and then decoding a P2SH Bitcoin address should result in the same base58 string representation of the original input.

Additional context I'm not sure if I am missing something or I am using the wrong functions. What if we use decodeBTCAddress to use that format on the Pox contract where poxAddresses are required, and then we want to encode that format back into a base58 representation. That would result in a mistmatch.

Please, let me know if I am doing something wrong and this is not an issue at all.

Cheers.

reedrosenbluth commented 3 years ago

Neither of these functions are currently publicly exposed via this package.

That being said, decodeBtcAddress and getBTCAddress aren't inverse functions. The hashMode returned by decodeBtcAddress is actually a Stacks address hash mode, not a bitcoin address version byte.

The stacks address hash modes are specified in SIP 005 as such:

0x00: A single public key is used. Hash it like a Bitcoin P2PKH output.
0x01: One or more public keys are used. Hash them as a Bitcoin multisig P2SH redeem script.
0x02: A single public key is used. Hash it like a Bitcoin P2WPKH-P2SH output.
0x03: One or more public keys are used. Hash them as a Bitcoin P2WSH-P2SH output.

TBH I'm not really sure why the getBTCAddress function is still in this library, as it isn't being used anywhere.

reedrosenbluth commented 3 years ago

@limiaspasdaniel can you provide some more context on what you would like to do with these functions? Perhaps they should be public and documented a bit better...

2snEM6 commented 3 years ago

Neither of these functions are currently publicly exposed via this package.

That being said, decodeBtcAddress and getBTCAddress aren't inverse functions. The hashMode returned by decodeBtcAddress is actually a Stacks address hash mode, not a bitcoin address version byte.

The stacks address hash modes are specified in SIP 005 as such:

0x00: A single public key is used. Hash it like a Bitcoin P2PKH output.
0x01: One or more public keys are used. Hash them as a Bitcoin multisig P2SH redeem script.
0x02: A single public key is used. Hash it like a Bitcoin P2WPKH-P2SH output.
0x03: One or more public keys are used. Hash them as a Bitcoin P2WSH-P2SH output.

TBH I'm not really sure why the getBTCAddress function is still in this library, as it isn't behind used anywhere.

Thanks for the explaination. I was using the combination of the two functions wrongly then.

I am dealing with the POX contract, and some of the read only functions return a poxAddr in the format of two buffer, one for the version and one for the rest of the bytes. Eg:

{
  version: 0x01
  buffer: 0xFFFFFFFFF
}

I want to convert that format into a base58check encoded string representation (a btc address).

I noticed that the version buffer of some delegator pools that use P2SH addresses doesn't match the appropiate address versions defined by the Bitcoin protocol.

Perhaps I am not understanding how to encode that format into a readable b58check string representation

friedger commented 3 years ago

@limiaspasdaniel There is a check in pox.clar for the version:

(define-private (check-pox-addr-version (version (buff 1)))
    (or (is-eq version ADDRESS_VERSION_P2PKH) 
        (is-eq version ADDRESS_VERSION_P2SH)
        (is-eq version ADDRESS_VERSION_P2WPKH)
        (is-eq version ADDRESS_VERSION_P2WSH)))

This is called in stacks-stx and delegate-stx when the pox address is provided. The values for address_version is 0,1,2,3 in mainnet and testnet.

On the other hand we see a version value of 0x14 in the explorer, e.g. https://explorer.stacks.co/txid/0xf2c6f6d001ca33ed7cf47e56756eb756f7caa6fe33532927e41d8f797128edc9?chain=mainnet

reedrosenbluth commented 3 years ago

@limiaspasdaniel closing this issue since this isn't a bug. If you are still confused about these encoding formats, please start a discussion in the discussions section of this repo.

zone117x commented 3 years ago

Note PR https://github.com/blockstack/stacks.js/pull/1020 introduced poxAddressToBtcAddress which performs the correct encoding that getBTCAddress can be mistaken as performing.