bitcoinjs / bitcoinjs-lib

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

BIP143 rejects uncompressed public keys in P2WPKH or P2WSH #981

Closed mandelmonkey closed 6 years ago

mandelmonkey commented 6 years ago

Not sure if I am misunderstanding something here but does this not only check if the output is p2wpkh and not p2wsh as well?

i.e. is correct?

if (kpPubKey.length !== 33 &&
      (input.signType === scriptTypes.P2WPKH || input.signType === scriptTypes.P2WSH)) throw new Error('BIP143 rejects uncompressed public keys in P2WPKH or P2WSH')

https://github.com/bitcoinjs/bitcoinjs-lib/blob/86cd4a44a1524686d993f35b02a1ed331aaa551f/src/transaction_builder.js#L709

dabura667 commented 6 years ago

This covers all templates handled by TransactionBuilder, so multisig is also checked. That’s why input.pubkeys and input.signatures are arrays

dcousens commented 6 years ago

@mandelmonkey could you elaborate on your question?

dabura667 commented 6 years ago

I think what he means is:

Segwit enforces compressed public keys for ALL instances of pubkeys in the witness stack. Multisig and single sig.

His question is: It looks like this one line is only checking for single sig. allowing multisig segwit with uncompressed pubkeys to pass and make an invalid transaction. Is he right in his assessment that this is a bug, or are we checking somewhere else and he doesn’t notice it?

mandelmonkey commented 6 years ago

Hi guys its probably my misunderstanding! but the following line in my mind checks if the pubkey is uncompressed and if it is it checks if the script type is p2wpkh, if so it throws an error because uncompressed public keys keys are not supported with segwot.

if (kpPubKey.length !== 33 && input.signType === scriptTypes.P2WPKH) throw new Error('BIP143 rejects uncompressed public keys in P2WPKH or P2WSH')

however the comment says "in P2WPKH or P2WSH" but the if statement is only checking if the script type is p2wpkh and not p2wsh as well.

thats where I got confused

dcousens commented 6 years ago

Will add a test case

dcousens commented 6 years ago

Closing in favour of https://github.com/bitcoinjs/bitcoinjs-lib/pull/987