Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix signmessage to be Electrum-compatible #841

Merged
merged 1 commit into from
Apr 1, 2021
Merged

Conversation

AdamISZ
Copy link
Member

@AdamISZ AdamISZ commented Mar 26, 2021

Verification of message signatures against segwit addresses
is not currently functional/possible in Core, and additionally
the signing function used for messages in jmbitcoin, derived
from coincurve, was not compatible with Electrum either.
This commit uses the functionality in python-bitcointx's
signmessage module, and now the wallet method signmessage
creates signatures on messages against segwit addresses that
are verifiable in Electrum.

@AdamISZ
Copy link
Member Author

AdamISZ commented Mar 26, 2021

Some reference points: original commit that added signmessage, in Jun 2017, was working at that time: 28912ac#diff-326acdd19b880cb124ebd72bd49b94eb9b14167dfac0e1dad049059ec3c18e94R390-R393 - but note that that was shortly before segwit, so working with legacy addresses.

The functionality offered was specifically the ability to create signatures verifiable in Bitcoin Core; no verification method was added then (or since).
It seems that this was kind of "broken in at least two ways":

  • broken by segwit and the fact that Core decided it was better not to down the route of "guess the key by recovery" for either the scripthash or keyhash variants of segwit, see e.g. discussion here. Core intends to address this with a new signature standard, see here, but I am not sure the current status.
  • also the change to coincurve from secp256k1-py was "breaking" as far as my investigations indicate; specifically, see the change here - but at that time (Nov 2018), the difference was basically academic, since Joinmarket was already ~ segwit-only, anyway.

The later introduction of python-bitcointx replacing most (but not all unfortunately!) uses of coincurve didn't really change this, at the time, except in a trivial sense: an attempt to call the signmessage method of wallet-tool.py actually triggers a syntax error because of this line not having been updated to remove an argument in #536.

The fact that we haven't had complaints for years does rather suggest this is not an important feature today! But it could be useful, and as can be seen, the change needed to at least make it Electrum compatible, for now, is close to trivial.

A very important detail should be remembered: the Joinmarket communication protocol itself uses message signatures, and they are using the effectively now "legacy" coincurve-based jmbitcoin.ecdsa_sign function, which is as explained not compatible with this; but this cannot be changed until and unless we upgrade our protocol (current version 5).

Edit: just dropping a couple of additional related code links in case they're useful:

Core signing code
python-bitcointx examples using SignMessage/VerifyMessage

@AdamISZ
Copy link
Member Author

AdamISZ commented Mar 26, 2021

Tested (meaning: create signatures with Joinmarket address and wallet-tool signmessage, then attempt to verify in Electrum desktop) as working with both p2sh-p2wpkh and p2wpkh addresses. Note that only mainnet is supported (just to minimize the workload, not for a fundamental reason).

@kristapsk
Copy link
Member

Could we add some test(s) to test suite for this?

@AdamISZ
Copy link
Member Author

AdamISZ commented Mar 26, 2021

Could we add some test(s) to test suite for this?

Yep, good call, I'll add a few test cases.

@AdamISZ
Copy link
Member Author

AdamISZ commented Mar 27, 2021

Test suite passing, flake8 OK (we really need to re-set up CI ..), the signatures are from both wallet types and all verify against Electrum latest version. No real rush, but I'll squash and merge if I get a tACK.

Copy link
Member

@kristapsk kristapsk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ACK 53d41eb (tested against Electrum 4.0.9 with p2wpkh)

@AdamISZ
Copy link
Member Author

AdamISZ commented Apr 1, 2021

OK squashing and merging, thanks.

Verification of message signatures against segwit addresses
is not currently functional/possible in Core, and additionally
the signing function used for messages in jmbitcoin, derived
from coincurve, was not compatible with Electrum either.
This commit uses the functionality in python-bitcointx's
signmessage module, and now the wallet method `signmessage`
creates signatures on messages against segwit addresses that
are verifiable in Electrum.
Test cases of p2sh and native are added.
@AdamISZ AdamISZ merged commit 8c57852 into master Apr 1, 2021
@AdamISZ AdamISZ deleted the fix-signmessage branch April 12, 2021 19:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants