Support Segwit (including P2SH wrapped) signatures created by Electrum

[lf94]

Should resolve the following issues:

• https://github.com/bitcoinjs/bitcoinjs-message/issues/28
• https://github.com/bitcoinjs/bitcoinjs-message/issues/20
• https://github.com/coreyphillips/moonshine/issues/24

Big thanks to @coreyphillips who I saw found the root cause. While I knew it was some offset happening, I was not sure why.

Sponsored by Bity and Relai.

[junderw]

Judging from #6 and #12 it seems that Electrum treats P2SH-P2WPKH as the exact same offset as P2PKH-COMPRESSED.

Since it is not a standard BIP, choosing one will break the other, unless Trezor decided to switch to a new format?

Since this Segwit support was modeled after Trezor, I'm going to ask for a review from

Trezor: @prusnak

and

Electrum: @ecdsa @SomberNight @lukechilds

[junderw]

Pasting the original issue from bitcoinjs-lib by @prusnak

For TREZOR we implemented a way how to Sign and Verify SegWit P2SH addresses. This is achieved by introducing extra v values used during signing.

Currently the following are used:

• 27-30 uncompressed pubkey (base58)
• 31-34 compressed pubkey (base58)

We added the following:

• 35-38 p2sh segwit pubkey (base58)
• 39-42 segwit pubkey (bech32)

Unit tests for reference

Reference https://github.com/bitcoinjs/bitcoinjs-lib/issues/880

[junderw]

So by doing -4 for p2sh you are turning 35-38 into 31-34 which means that it is the same as P2PKH compressed.

So I think what happened was:

1. Core didn't support segwit or wrapped segwit
2. Trezor tried getting them to support it (?) but Core being Core, was taking forever
3. Trezor created their own method without a BIP
4. Core added support but did not change the flag byte
5. Electrum followed suit

Does this sound right?

[lf94]

@junderw your interpretation of the situation is absolutely correct. It only makes sense to continue with this implementation until a standard BIP arrives. If bitcoinjs-message continues to perpetuate this variant, it only harms the ecosystem further.

I would suggest a major version change if following semver.

[junderw]

Can someone link the PR where Bitcoin Core added message signing support for p2wpkh and p2sh-p2wpkh?

BitcoinJS should be following Core in absence of any BIP.

[lf94]

Can someone link the PR where Bitcoin Core added message signing support for p2wpkh and p2sh-p2wpkh?

BitcoinJS should be following Core in absence of any BIP.

It appears Bitcoin Core still does not support signing messages with P2SH or P2WPKH addresses.

[junderw]

Ah ok, so this is two non-core implementations battling it out... I see...

hmm...

I personally like Trezor's method better...

sigh

Maybe I can add the two following in a backwards compatible way.

1. Allow verify to succeed for either method.
2. Add (yet another) 2 segwit options for p2sh(p2wpkh)-electrum p2wpkh-electrum

ugh...

I would much rather just pick one or the other. In which case I'd pick Trezor since they were first and they actually removed ambiguity. (In the electrum method, you can't tell the script type from the flag byte, so you just have to try all the address types.)

[lf94]

I 100% agree the situation (and the fact it's still a situation) is disappointing to say the least.

My vote's on adding an Electrum method then. When there's a standard BIP, maybe do a new major release?

[SomberNight]

I had collected references/links here at one point: https://github.com/spesmilo/electrum/issues/3861

[junderw]

@lf94 I made some changes.

Could you read the README and try out signing and verifying using the methods described?

[junderw]

I have verified this pull request works as per the new instructions in the README against the latest version of Electrum.

[prusnak]

It only makes sense to continue with this implementation until a standard BIP arrives.

Standard BIP for this will never come.

BIP folks descided to come up with a completely new different standard which signs the whole scripts, not just the addresses - https://github.com/bitcoin/bips/blob/master/bip-0322.mediawiki

[ademcan]

Thank you very much for the PR @lf94 it solves my issue (#28 )! And thank you all for the great discussion

[lf94]

@junderw looks good to me. Thank you for your time! I guess we will add support for verifying both Electrum and Trezor generated signatures as you've done here.