Skip to content

Signmessage #8226

New issue

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

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

Already on GitHub? # to your account

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open

Signmessage #8226

wants to merge 3 commits into from

Conversation

Lagrang3
Copy link
Collaborator

@Lagrang3 Lagrang3 commented Apr 10, 2025

Add a new rpc called signmessagewithkey that can be used to sign messages using any key
from our wallet. You cannot directly select the key to use, but instead you provide a bitcoin address
and the wallet will figure out which of our keys corresponds to that bitcoin address.

Solves issue #8199

TODO:

  • add verification command (devtools/bip137-verifysignature)
  • add tests
  • add documentation

@Lagrang3
Copy link
Collaborator Author

Lagrang3 commented Apr 11, 2025

There is a variety of signature schemes in the market.
But it seems that Electrum's is somehow accepted as a standard and that seems to be
what Ocean accepts as well.

Therefore I implemented Electrum's signature scheme:

signature = base64(SigRec(SHA256(SHA256(
    "\x18Bitcoin Signed Message:\n" + var_int(len(message)) + message
))))

I checked against sparrow wallet and the verification succeeds.

$ l1-cli signmessagewithkey "some_string" "bcrt1q30zh8czhw4vzcv78t3ddv7zthtsw7w6wpgqz6z"
{
   "bech32": "bcrt1q30zh8czhw4vzcv78t3ddv7zthtsw7w6wpgqz6z",
   "keyidx": 1,
   "pubkey": "02fa14da5717ee05cd1eaf31ff871b8278f0b5dbe3a77ef3bf607c81b7391febe2",
   "signature": "204b7748da7641b218e0e3369f10c12bfd42fa39ed87aa27006499b8f5238b627b22f1d2c3f11de9b577a313760a9c5009fb91ed3d05480e782662559485a78fd7",
   "base64": "IEt3SNp2QbIY4OM2nxDBK/1C+jnth6onAGSZuPUji2J7IvHSw/Ed6bV3oxN2CpxQCfuR7T0FSA54JmJVlIWnj9c="
}

Screenshot from 2025-04-11 10-28-06

@Lagrang3
Copy link
Collaborator Author

In the future if it becomes necessary one could add a flag to select the signature scheme of choice.

Also for reviewers: I was tempted to make hsmd sign any message fromwire, adding more flexibility in
the signing format. But I didn't because I am not sure how much of a security vulnerability that could represent.
As a matter of fact in the comments of handle_sign_message in hsmd/libhsmd.c one can read
that prefixing the message save us from signing unwanted data.

@Lagrang3 Lagrang3 marked this pull request as ready for review April 11, 2025 09:46
@Lagrang3 Lagrang3 requested a review from cdecker as a code owner April 11, 2025 09:46
@Lagrang3 Lagrang3 force-pushed the signmessage branch 3 times, most recently from d291224 to 0a82edd Compare April 11, 2025 09:59
@vincenzopalazzo vincenzopalazzo self-requested a review April 11, 2025 10:33
@vincenzopalazzo vincenzopalazzo self-assigned this Apr 11, 2025
@luke-jr
Copy link

luke-jr commented Apr 11, 2025

Electrum-style is probably the worst of the options accepted by OCEAN fwiw

BIP 322 will be required if you need to support Taproot in the future.

BIP 137 would be probably trivial to add here (just a version byte change)

@Lagrang3 Lagrang3 force-pushed the signmessage branch 2 times, most recently from ec989c7 to 0024b60 Compare April 14, 2025 10:49
@ShahanaFarooqui ShahanaFarooqui added this to the v25.05 milestone Apr 15, 2025
@Lagrang3 Lagrang3 force-pushed the signmessage branch 3 times, most recently from a8d0e9b to 2f7c19d Compare April 15, 2025 09:44
with custom keys.

Changelog-Added: HSMD: add new wire api to sign messages with bitcoin wallet keys.

Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
To validate BIP137 signatures produced by core-lightning in tests.

Changelog-None.

Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
@Lagrang3 Lagrang3 force-pushed the signmessage branch 2 times, most recently from 3a49a38 to f134ed0 Compare April 23, 2025 08:38
@Lagrang3
Copy link
Collaborator Author

Tested on Ocean!

signmessagewithkey: allows to sign a message with a key associated with
one bitcoin address in our wallet.

Changelog-Added: add a new rpc command signmessagewithkey to #put messages with keys from our wallet.

Signed-off-by: Lagrang3 <lagrang3@protonmail.com>
@@ -356,6 +356,15 @@ msgdata,hsmd_sign_message,msg,u8,len
msgtype,hsmd_sign_message_reply,123
msgdata,hsmd_sign_message_reply,sig,secp256k1_ecdsa_recoverable_signature,

# sign a raw message with a derived key
msgtype,hsmd_sign_message_with_key,45
Copy link
Contributor

Choose a reason for hiding this comment

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

Can we call this bip137_sign_message? We're going to want to do 332 eventually, though it's a complex mess and I'm not sure I even understand it :(

Comment on lines +1110 to +1111
struct issued_address_type *listaddrtypes =
wallet_list_addresses(tmpctx, cmd->ld->wallet, 1, NULL);
Copy link
Contributor

Choose a reason for hiding this comment

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

the db doesn't contain types for old keys. You need to iterate from 1 to db_get_intvar(cmd->ld->wallet->db, "bip32_max_index", 0); to get all keys.

You could use wallet_get_addrtype() once you've found a match, and refuse if we never issued that keyidx as a p2wpkh?

u8 sig[65];
secp256k1_ecdsa_recoverable_signature rsig;

if (!fromwire_hsmd_sign_message_with_key_reply(msg, &rsig)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

We should use hsm_is_capable, and add the new msg type to capabilities. This means that an older VLS will not crash: we can tell the user it's not supported.

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants