Add message signer & message signature verifier by q-src · Pull Request #601 · bitcoindevkit/bdk

q-src · 2022-05-06T09:43:00Z

Description

Ths PR adds the possibility to

sign an arbitrary message
verify a given message signature

using ECDSA.

Checklists

All Submissions:

I've signed all my commits
I followed the contribution guidelines
I ran cargo fmt and cargo clippy before committing

New Features:

I've added tests for the new feature
I've added docs for the new feature
I've updated CHANGELOG.md

afilini

Overall this is a good start but it feels a bit disconnected from everything else in BDK.

There are a few ways to "connect" this to the Wallet, the easiest one is probably to iterate over the signers of a wallet and taking their key (if any) with descriptor_secret_key().

You could have an API exposed on the wallet that takes a message and a derivation index, and internally iterates on the signers, derives the keys to the right index and then signs the message.

You should also figure out a good way to handle this when you have multiple keys in your descriptor. You could return a list of signatures, or a map with key -> sig entries, I don't know.. There are a few ways to do it and I haven't thought about them in detail yet, I'd like to see your proposal since you've given this more thoughts than myself!

afilini · 2022-05-10T18:05:47Z

src/keys/msgsig.rs

+use bitcoin::{Address, Network, PrivateKey, PublicKey};
+
+/// Trait for message signers
+pub trait MessageSigner<S> {


Are there any other signature types you'd plan to support on top of RecoverableSignature? I don't really understand why you need the generic here and on the verifier

Yes, exactly. The idea behind this is to add other signature types.

As far as I am aware there are no plans to switch to schnorr signatures for Bitcoin signed messages, so I don't think there's really too much room for upgrades here.

I was asking if you had at least one specific use case in memory (like a BIP which uses other sig algorithms) but if you don't have any examples I would just say it's better to drop the generic and stick to the classic ecdsa algorithm.

No, I had no other use cases in mind than multi-sig and possibly schonrr. But since the latter is not planned and multi-sig can be done with a map as suggested by you, I agree it's better to just add a sign method to the wallet.

afilini · 2022-05-10T18:07:04Z

src/keys/msgsig.rs

+
+/// A message signature verifier using ECDSA
+pub struct EcdsaMessageSignatureVerifier {
+    secp: Secp256k1<All>,


I would just take a reference here to avoid cloning the context. If you don't want to add a lifetime to the struct you could just change the api of verify() to take a secp argument

I mean, change the member declaration to be a reference and then take one in from_pub and from_address

I agree, secp should at least be changed to a reference. However, regarding a secp argument at verify(), this would force BDK to have new methods on the wallet struct for every signing / verification algorithm.

I don't understand what do you mean here, can you elaborate a bit more?

My interpretation is thatverify() doesn't have to be a method on the wallet, because you don't need anything from the wallet to verify a message (while for signing you need the private keys)

I intially wanted to have similar interfaces for signing and verification. Therfore, I thought if signing is moved to the wallet, so should also verification. Combined with my assumption of having schnorr for message signatures at some point, BDK would end up with multiple verify methods.

However, this is not the case since schnorr is aparently not planned. Moreover, you are right that verify does not have to be on the wallet - except you want verification to happen with the pubkey of a different pk(...) wallet. But this feels a little odd.

Therefore, I suggest that we change the PR to have

a method on the wallet for message signing which uses all private keys of the wallet

message signature verification which is decoupled from the wallet

secp as a reference: 707f6c3

afilini · 2022-05-10T18:08:43Z

src/keys/msgsig.rs

+    fn sign(&self, message: &str) -> RecoverableSignature {
+        let msg_hash = signed_msg_hash(message);
+        self.secp.sign_recoverable(
+            &Message::from_slice(&msg_hash.into_inner()[..]).unwrap(),


Either replace the unwrap with an expect (for reasonably unlikey errors) or change this function to return a Result

fb772ed ✔️

afilini · 2022-05-10T18:08:56Z

src/keys/msgsig.rs

+
+/// A message signer using ECDSA
+pub struct EcdsaMessageSigner {
+    secp: Secp256k1<All>,


Same thing about secp as for the verifier

secp as a reference: 707f6c3

q-src · 2022-05-11T16:55:54Z

Thanks for the review. The additions in this PR are intentionally disconnected from the rest of BDK for single responsibility reasons but also because I was not sure if / how you want it to be connected - especially regarding multi-sig wallets and other signing algorithms.

So I chose to add the traits MessageSigner and MessageSignatureVerifier. With this I wanted to prevent that the wallet struct has to be extended with another method for each signing scheme / algorithm which is added (e.g. sign_msg_ecdsa, sign_msg_schnorr,…). Maybe this is just me having too much of a non-rust way of thinking?
Nevertheless, this is a design decision which I think should be made by the maintainers of BDK.

I intended to join your dev call yesterday to discuss this. However, I didn’t manage to attend due to a change in my schedule…

LLFourn · 2022-05-16T06:09:50Z

Hey @q-src. I noticed that you didn't implement BIP-137 or BIP-322 are they not applicable to what you want to do?

It's not clear this functionality needs a trait around it in BDK. How did you imagine using these traits?

q-src · 2022-08-17T14:10:18Z

@LLFourn I was not aware of BIP-322 and therefore did not implement it. However, it looks very much like what I want to achieve.

Apparently, BIP-322 is being added to rust-miniscript (rust-bitcoin/rust-miniscript#444). Shall we wait until this is done and then use the miniscript implementation?

LLFourn · 2022-08-17T14:37:56Z

@q-src TBH miniscript doesn't sound like the right place either. It might make sense to have a BIP322 implementation in bdk if it is supported somehow by the existing components of the library. I'm not sure if it is though.

q-src mentioned this pull request May 6, 2022

Add sign / verify for arbitrary messages bitcoindevkit/bdk-ffi#139

Closed

afilini requested changes May 10, 2022

View reviewed changes

thunderbiscuit mentioned this pull request Jun 28, 2022

W26 BDK Language Bindings Dev Call bitcoindevkit/.github#11

Closed

q-src and others added 6 commits June 30, 2022 14:09

Add message signer & message signature verifier

0326235

Run cargo fmt

44e63e5

Simplify return statement

a3283c4

Fix message signing

fb772ed

Store Secp256k1 as a reference to avoid copying

707f6c3

Remove generic aspects; There's only one sig. algo

f3fcefd

notmandatory assigned q-src Jul 4, 2022

notmandatory added the new feature New feature or request label Jul 4, 2022

thunderbiscuit mentioned this pull request Aug 19, 2022

Add message signing bitcoindevkit/bdk-ffi#229

Closed

getlipa closed this by deleting the head repository Mar 14, 2023

thunderbiscuit mentioned this pull request May 29, 2023

Feature Proposal: BIP322 message signing bitcoindevkit/bdk_wallet#150

Open

Conversation

q-src commented May 6, 2022

Description

Checklists

All Submissions:

New Features:

Uh oh!

afilini left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

q-src commented May 11, 2022

Uh oh!

LLFourn commented May 16, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

q-src commented Aug 17, 2022

Uh oh!

LLFourn commented Aug 17, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

LLFourn commented May 16, 2022 •

edited

Loading