[Draft] BIP322 Implementation #240

rajarshimaitra · 2021-03-15T10:05:34Z

This is a draft implementation of the BIP322 generic message signer using Bitcoin scripts.

There are a lot of rough edges to iron out. Looking for generic approach comments. The approach might be completely wrong, as it only reflects my own understanding after reading the BIP.

So far the signer can only validate message and message_challenge given the signature (of type Legacy/Simple/Full).

A simple test case for these three types of signature validation included.

Looking for comments to figure out the rest of the required details.

This PR implements a BIP322 generic message signer with Bitcoin Scripts.

sanket1729

Did not review the test. I think we should first focus on the BIP322Validtor struct which should only have msg, sig, and scriipt_pubkey.

Then, we can use the interpreter to automatically infer the descriptor. Look at from_txdata function.

The last step is correct where we interpret the descriptor.

Let us try to fix the above in this iteration.

Next, we can focus on extending to height and age and using satisfy API for signing logic.

src/bip322.rs

sanket1729 · 2021-03-15T23:26:18Z

src/bip322.rs

+//! `https://github.com/bitcoin/bips/blob/master/bip-0322.mediawiki`
+//!
+
+use crate::{Descriptor, DescriptorTrait, MiniscriptKey, ToPublicKey};


We cannot use crate:: because that will cause rustc 1.29 to fail.

Using super:: instead.

sanket1729 · 2021-03-15T23:35:13Z

src/bip322.rs

+use std::convert::From;
+
+// BIP322 message tag = sha256("BIP0322-signed-message")
+static MIDSTATE: [u8; 32] = [


I think we should use const instead of static.

It would be great if we have a test similar to https://github.com/rust-bitcoin/rust-bitcoin/pull/259/files to verify the value is computed correctly

sanket1729 · 2021-03-15T23:55:32Z

src/bip322.rs

+    MessageTag,
+    MIDSTATE,
+    64,
+    doc = "test hash",


A more expressive doc comment

sanket1729 · 2021-03-15T23:56:20Z

src/bip322.rs

+);
+
+/// BIP322 Error types
+#[derive(Debug)]


Would be great to derive all possible things that we can derive.

There's not much to drive here as InterpreterError only derives debug.

rust-miniscript/src/interpreter/error.rs

Lines 20 to 21 in 86a63d8

#[derive(Debug)]

pub enum Error {

sanket1729 · 2021-03-16T00:59:15Z

src/bip322.rs

+    /// Create a new BIP322 validator
+    pub fn new(msg: &[u8], sig: Option<Bip322Signature>, addr: Descriptor<T>) -> Self {
+        Bip322 {
+            message: msg.to_vec(),


I think we should consume the message instead of the new allocation.

sanket1729 · 2021-03-16T01:00:03Z

src/bip322.rs

+
+impl<T: MiniscriptKey + ToPublicKey> Bip322<T> {
+    /// Create a new BIP322 validator
+    pub fn new(msg: &[u8], sig: Option<Bip322Signature>, addr: Descriptor<T>) -> Self {


I don't like that the addr variable for the descriptor. See detailed comment later for the suggestion of BIP322Validator struct

renaming addr to mesage_challenge.

Message challenge can be two things. Either descriptor or a Script structure. Throughout the logic currently, we are simply using descriptor.script_pubkey() to derive the message challenge script. We can as well use a script_pubkey directly here as no descriptor specific functionality is used anywhere. But I think it's better to keep the descriptor here to make the API easier for complex challenge scripts. Otherwise, lib users need to construct the script. Also, we can use the descriptor in the future if such functionality is required.

Will update if that's not acceptable for other reasons.

sanket1729 · 2021-03-16T01:02:47Z

src/bip322.rs

+
+        // create and return final transaction
+        Transaction {
+            version: 0,


I have made a comment to BIP322 to make this always 2 and it was positively received. I think we should keep it always 2.

sanket1729 · 2021-03-16T01:08:30Z

src/bip322.rs

+    /// Create to_sign transaction
+    /// This will create a transaction structure with empty signature and witness field
+    /// its up to the user of the library to fill the Tx with appropriate signature and witness  
+    pub fn to_sign(&self) -> Transaction {


This should take a satisfier as an argument and should call satisfy to fill the witness and script sig.

This should also take in the age and height as arguments instead of hard coded zeeros

Changing to_sign to empty_to_sign.

The purpose of this function is to create an empty to_sign transaction template, which then can be filled with the correct signature depending upon the BIP322Signature types. Which is currently happening inside the validate() function here.

rust-miniscript/src/bip322.rs

Lines 180 to 211 in c740598

Bip322Signature::Full(to_sign) => self.tx_validation(to_sign),

// If Simple Signature is provided, the resulting `to_sign` Tx will be computed

Bip322Signature::Simple(witness) => {

// create empty to_sign transaction

let mut to_sign = self.to_sign();

to_sign.input[0].witness = witness.to_owned();

self.tx_validation(&to_sign)

}

// Legacy Signature can only be used to validate against P2PKH message_challenge

Bip322Signature::Legacy(sig, pubkey) => {

if !self.message_challenge.script_pubkey().is_p2pkh() {

return Err(BIP322Error::InternalError("Legacy style signature is only applicable for P2PKH message_challenge".to_string()));

} else {

let mut sig_ser = sig.serialize_der()[..].to_vec();

// By default SigHashType is ALL

sig_ser.push(SigHashType::All as u8);

let script_sig = Builder::new()

.push_slice(&sig_ser[..])

.push_key(&pubkey)

.into_script();

let mut to_sign = self.to_sign();

to_sign.input[0].script_sig = script_sig;

self.tx_validation(&to_sign)

Here we are mutating the empty to_sign transaction with corresponding signatures. Which can be done with a satisfier too. But because a Bip322Signature::Full type signature will contain the entire to_sign transaction itself (which I think will be most of the cases, the other two cases are rather simplistic and more specific so can be hardcoded easily), using a separate satisfier to create the same to_sign feels like more roundabout than necessary.

Bip322Signer probably needs to use a satisfier in some way, but we can flesh that detail out later.

For age and height, I think it's better to provide these values in the validator structure itself. Though I am not sure if I have applied these values correctly in the empty_to_spend creation, please check.

sanket1729 · 2021-03-16T01:15:46Z

src/bip322.rs

+
+    /// Validate a BIP322 Signature against the message and challenge script
+    /// This will require a BIP322Signature inside the structure
+    pub fn validate(&self) -> Result<bool, BIP322Error> {


We should reproduce the errors states from the BIP. This should return Result<(T, S), BIP322Error> where BIP322Error should either be Inconclusive(..) or Invalid(..)

Is it possible to somehow derive these error types from the IterpreterErrors?

Also if I understand it right, neither to_spend nor to_sign transactions are supposed to be mined. They are not validated against the chain. So what the purpose of T and S here and how the validator is supposed to validate Time and Age of an unmined transaction? Also, what does it mean to have a signature against a message only valid "after x time"? These are not transactions, but simple generic messages.

rajarshimaitra · 2021-03-16T18:40:40Z

Updated with changes. This is ready for the next review.

BIP322 Implementation

c740598

This PR implements a BIP322 generic message signer with Bitcoin Scripts.

sanket1729 reviewed Mar 16, 2021

View reviewed changes

Review comment updates 1

46b17e5

rajarshimaitra force-pushed the bip322 branch from f2de66a to 46b17e5 Compare March 16, 2021 18:40

rajarshimaitra mentioned this pull request Jun 7, 2021

Implementing proof of reserves bitcoindevkit/bdk#356

Closed

6 tasks

dr-orlovsky mentioned this pull request Jan 14, 2022

More complete MessageSignature.is_signed_by_address rust-bitcoin/rust-bitcoin#684

Closed

SarcasticNastik mentioned this pull request Jul 7, 2022

BIP322 message-signing #444

Draft

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[Draft] BIP322 Implementation #240

[Draft] BIP322 Implementation #240

rajarshimaitra commented Mar 15, 2021

sanket1729 left a comment

sanket1729 Mar 15, 2021

rajarshimaitra Mar 16, 2021

sanket1729 Mar 15, 2021

sanket1729 Mar 15, 2021

rajarshimaitra Mar 16, 2021

sanket1729 Mar 15, 2021

rajarshimaitra Mar 16, 2021

sanket1729 Mar 15, 2021

rajarshimaitra Mar 16, 2021

sanket1729 Mar 16, 2021

rajarshimaitra Mar 16, 2021

sanket1729 Mar 16, 2021

rajarshimaitra Mar 16, 2021 •

edited

sanket1729 Mar 16, 2021

rajarshimaitra Mar 16, 2021

sanket1729 Mar 16, 2021

sanket1729 Mar 16, 2021

rajarshimaitra Mar 16, 2021

sanket1729 Mar 16, 2021

rajarshimaitra Mar 16, 2021

rajarshimaitra commented Mar 16, 2021

	Bip322Signature::Full(to_sign) => self.tx_validation(to_sign),

	// If Simple Signature is provided, the resulting `to_sign` Tx will be computed
	Bip322Signature::Simple(witness) => {
	// create empty to_sign transaction
	let mut to_sign = self.to_sign();

	to_sign.input[0].witness = witness.to_owned();

	self.tx_validation(&to_sign)
	}

	// Legacy Signature can only be used to validate against P2PKH message_challenge
	Bip322Signature::Legacy(sig, pubkey) => {
	if !self.message_challenge.script_pubkey().is_p2pkh() {
	return Err(BIP322Error::InternalError("Legacy style signature is only applicable for P2PKH message_challenge".to_string()));
	} else {
	let mut sig_ser = sig.serialize_der()[..].to_vec();

	// By default SigHashType is ALL
	sig_ser.push(SigHashType::All as u8);

	let script_sig = Builder::new()
	.push_slice(&sig_ser[..])
	.push_key(&pubkey)
	.into_script();

	let mut to_sign = self.to_sign();

	to_sign.input[0].script_sig = script_sig;

	self.tx_validation(&to_sign)

[Draft] BIP322 Implementation #240

Are you sure you want to change the base?

[Draft] BIP322 Implementation #240

Conversation

rajarshimaitra commented Mar 15, 2021

sanket1729 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajarshimaitra Mar 16, 2021 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

rajarshimaitra commented Mar 16, 2021

rajarshimaitra Mar 16, 2021 •

edited