Add `Witness::p2wpkh` constructor #2084

tcharding · 2023-09-20T05:08:28Z

In order to create the witness to spend p2wpkh output one must create a Witness that includes the signature and the pubkey, we should have a function for this.

Notes

The PR originally added a push_p2wphk method, this is now instead a constrcutor Witness:p2wpkh (after review discussion below).

Patch 1 changes push_bitcoin_signature to take an ecdsa::Sigtnture instead of an ecdsa::SerializedSignature
Patch 2 takes a secp256k1::PublicKey removing the need for an error path (discussed below).

junderw · 2023-09-20T05:38:38Z

bitcoin/src/blockdata/witness.rs

+        if pubkey.compressed {
+            self.push_slice(&pubkey.inner.serialize())
+        } else {
+            self.push_slice(&pubkey.inner.serialize_uncompressed())
+        }


Segwit v0 forbids uncompressed keys for use in signature operations.

This is a big enough foot gun that I think we should somehow prevent it.

Oh boy, my bad, thanks man. I'll fix it up.

I whipped this up in case we want to allow the footgun but at least warn the caller.

This could probably be brushed up and made prettier with a nice error variant name containing the closure with a name that makes it clear why it's a footgun.

But tbh it's probably just better to fail on uncompressed.

pub fn push_p2wpkh<'a>( &'a mut self, signature: &'a ecdsa::SerializedSignature, hash_type: EcdsaSighashType, pubkey: &'a PublicKey, ) -> Result<(), impl FnMut() + 'a> { let mut closure = move || { self.push_bitcoin_signature(signature, hash_type); if pubkey.compressed { self.push_slice(&pubkey.inner.serialize()) } else { self.push_slice(&pubkey.inner.serialize_uncompressed()) } }; if pubkey.compressed { closure(); Ok(()) } else { Err(closure) } }

Or didn't we add a CompressedPublicKey type a while back (I forget)

I added a UncompressPubkeyError type, and the following check

if !pubkey.compressed { return Err(UncompressedPubkeyError); }

I would have thought this sort of check would exist in other places but my quick greping couldn't find one. The error type is currently defined in witness but might be better in crypto::key?

We are trying to "hide" the legacy key stuff but so far haven't come up with an elegant solution.

junderw · 2023-09-20T05:54:47Z

bitcoin/src/blockdata/witness.rs

+    /// Pushes a DER-encoded ECDSA signature with a signature hash type as a new element on the
+    /// witness, followed by the serialized public key.
+    ///
+    /// Also useful for spending a P2SH-P2WPKH output.


Should add Errors section to docs.

Done, thanks!

apoelstra · 2023-09-20T15:43:56Z

Let's just take an ecdsa::Signature. I suspect the old API used SerializedSignature for dumb historical reasons.

apoelstra · 2023-09-20T15:44:03Z

6b8fcdb looks good otherwise.

tcharding · 2023-09-21T03:51:23Z

With fresh eyes it seems much better to just take a secp256k1::PublicKey and remove the need for an error case. This is justified by the fact that users likely just create the sig and have a secp256k1::SecretKey so converting to a secp pubkey is obvious, converting to a bitcoin pubkey is unnecessary and unergonomic.

@stevenroose you're welcome :)

apoelstra · 2023-09-21T14:27:21Z

LGTM, though I'm a little worried that the API might change again if we replace secp256k1::PublicKey with a dedicated bitcoin::PublicKey type (and move the existing bitcoin::PublicKey to LegacyPublicKey or something as suggested in #2013).

Though maybe not, I'm warming up to the idea of just re-exporting the secp pubkey types.. we'll see.

apoelstra

ACK 60882e8

tcharding · 2023-09-21T21:29:28Z

LGTM, though I'm a little worried that the API might change again if we replace secp256k1::PublicKey with a dedicated bitcoin::PublicKey type

Well that change will trigger a bunch of API changes, right? So one more shouldn't hurt folk too much :)

clarkmoody · 2023-09-22T21:30:32Z

bitcoin/src/blockdata/witness.rs

@@ -523,6 +546,21 @@ impl From<Vec<&[u8]>> for Witness {
    fn from(vec: Vec<&[u8]>) -> Self { Witness::from_slice(&vec) }
 }

+/// Tried to use a legacy uncompressed pubkey in a segwit v0 transaction.
+#[derive(Clone, Debug, PartialEq, Eq)]
+pub struct UncompressedPubkeyError;


It looks like this error is not actually used anywhere yet. Am I missing something?

Ouch, you are correct, it was used in an old version. Thanks, will fix.

tcharding · 2023-09-23T05:44:04Z

Removed the stale error type.

apoelstra

ACK 4f0daa1

sanket1729

@tcharding, I believe this simplifies the code significantly. We already have a ecdsa::Signature inside bitcoin crate.

     /// - `hash_type` was used when creating the sighash signed to create `signature`.
     pub fn push_p2wpkh(
         &mut self,
-        signature: &ecdsa::Signature,
-        hash_type: EcdsaSighashType,
+        signature: &crate::ecdsa::Signature,
         pubkey: &secp256k1::PublicKey,
     ) {
-        self.push_bitcoin_signature(signature, hash_type);
+        self.push_bitcoin_signature(signature);
         self.push_slice(&pubkey.serialize());
     }
 
@@ -345,15 +344,9 @@ impl Witness {
     /// this `signature` signed.
     pub fn push_bitcoin_signature(
         &mut self,
-        signature: &ecdsa::Signature,
-        hash_type: EcdsaSighashType,
+        signature: &crate::ecdsa::Signature,
     ) {
-        let ser = signature.serialize_der();
-        // Note that a maximal length ECDSA signature is 72 bytes, plus the sighash type makes 73
-        let mut sig = [0; 73];
-        sig[..ser.len()].copy_from_slice(&ser);
-        sig[ser.len()] = hash_type as u8;
-        self.push_slice(&sig[..ser.len() + 1]);
+        self.push_slice(&signature.serialize());
     }

tcharding · 2023-09-24T08:52:43Z

You are correct, I did know of this type but I had a reason for not using it at the time. I can't remember it right now so this message is just a response to say I'll have to come back and see if I can remember what I was thinking. Thanks for the review!

tcharding · 2023-09-24T20:53:16Z

Using your suggestion I played around with the signing code from the workshop and we get

    // Update the witness stack.
    let signature = bitcoin::ecdsa::Signature { sig, hash_ty: EcdsaSighashType::All };
    let pk = sk.public_key(&secp);
    sighasher.witness_mut(input_index).unwrap().push_p2wpkh(&signature, &pk);

Which seems reasonable, could push_p2wpkh be better named?

Re push_bitcoin_signature: I think this needs renaming now, when exactly is this used? push_ecdsa_signature?

And I think we should add to this PR

/// Pushes a taproot signature, as required to do a p2tr key path spend, onto the witness, requires an allocation.
pub fn push_p2tr_key_path(&mut self, sig: &taproot::Signature)

(better name?)

The `Witness::push_bitcoin_signature` method is old and a bit stale. Bitcoin has taproot signatures now so the name is stale, also we have the `crate::ecdsa::Signature` type that holds the secp sig and the hash type so we can use that instead of having two separate parameters. Add a new, up to date, `Witness::push_ecdsa_signature` function and deprecate the `push_bitcoin_signature` one.

tcharding · 2023-09-24T21:23:06Z

Re-did the PR after @sanket1729's review suggestion.

apoelstra · 2023-09-25T13:23:10Z

Couldi/should we make this a constructor? So you'd write

*sighasher.witness_mut(input_index).unwrap() = Witness::p2wpkh(&signature, &pk);

?

tcharding · 2023-09-25T19:36:13Z

Oh, we could go further than that and have one constructor, used for constructing the witness of a single input spend for each of the output types. That would be super useful for documentation also [0].

Witness::push_empty() would be useful, its not immediately obvious how to do that by looking at witness.rs.

We could push this even further and have methods on ScriptBuf for the scriptSig output types also, if we had

/// The scriptSig for sepnding a p2wpkh output is empty.
pub fn p2wpkh_script_sig() -> ScriptBuf {
    ScriptBuff::EMPTY
}

/// The scriptSig for spending a p2pkh output.
pub fn p2pkh_script_sig(sig: ecdsa::Signature, pk: PublicKey) -> ScriptBuf {
    ScriptBuf(v) // cat them together correctly into a vector
}

Along with ones for all the other output types it would be super user friendly for newer bitcoin devs (and ones that to double check their memories).

[0] Currently I have a table that has each output type and what goes in scriptPubkey, scriptSig, and witness. Having these functions would mean I could stop using that table and just look at the code.

sanket1729

ACK df758fe

apoelstra · 2023-09-25T20:13:09Z

[0] Currently I have a table that has each output type and what goes in scriptPubkey, scriptSig, and witness. Having these functions would mean I could stop using that table and just look at the code.

Making this table implicitly obvious is a great goal but it's not clear how we'd do it and I think it's out of scope for this PR.

For this PR, I simply observe that you never want to push a pubkey/signature onto an existing witness, and if you do, you're not doing a p2wpkh output. For p2wpkh, the pubkey/signature pair is the entire witness, so I think it should be a Witness constructor.

If you disagree, or agree but think it'll be a bigger project, then I'll just ACK this.

tcharding · 2023-09-25T21:58:20Z

[0] Currently I have a table that has each output type and what goes in scriptPubkey, scriptSig, and witness. Having these functions would mean I could stop using that table and just look at the code.

Making this table implicitly obvious is a great goal but it's not clear how we'd do it and I think it's out of scope for this PR.

Definitely out of scope :)

For this PR, I simply observe that you never want to push a pubkey/signature onto an existing witness, and if you do, you're not doing a p2wpkh output. For p2wpkh, the pubkey/signature pair is the entire witness, so I think it should be a Witness constructor.

If you disagree, or agree but think it'll be a bigger project, then I'll just ACK this.

oooo, I had it in mind that the witness was the combination of the witnesses for all the inputs so we would do multiple pushes, but the cache handles the combination for us. My bad.

Will change to a constructor as suggested - legend.

tcharding · 2023-09-25T22:00:53Z

Please note, now the two patches in this PR are totally unrelated and should really be two separate PRs.

apoelstra · 2023-09-26T00:10:42Z

A constructor should not take a &mut self :)

tcharding · 2023-09-26T00:11:36Z

A constructor should not take a &mut self :)

lol, face palm - its my first week of Rust.

In order to create the witness to spend p2wpkh output one must push the signature and the pubkey, we should have a function for this.

tcharding · 2023-09-26T00:17:52Z

I wonder if it would be useful for maintenance if we had the exact same code in the cookbook in bitcoin/examples, would achieve:

Updates would be tested for master
The cookbook could stay on released code
Upgrading the cookbook after release would be super easy because the changes would be in master already

cc @realeinherjar

sanket1729

ACK 5901d35

apoelstra

ACK 5901d35

tcharding changed the title ~~Add push_p2wpkh function on Witness~~ Add push_p2wpkh function on Witness Sep 20, 2023

junderw reviewed Sep 20, 2023

View reviewed changes

tcharding mentioned this pull request Sep 20, 2023

Update docs on witness_mut #2085

Merged

tcharding force-pushed the 09-06-push-p2wpkh branch from 5dac089 to e8786c7 Compare September 20, 2023 05:53

junderw reviewed Sep 20, 2023

View reviewed changes

tcharding force-pushed the 09-06-push-p2wpkh branch from e8786c7 to 6b8fcdb Compare September 20, 2023 05:58

tcharding force-pushed the 09-06-push-p2wpkh branch from 6b8fcdb to 1b416a8 Compare September 21, 2023 03:48

tcharding force-pushed the 09-06-push-p2wpkh branch from 1b416a8 to 60882e8 Compare September 21, 2023 03:53

apoelstra previously approved these changes Sep 21, 2023

View reviewed changes

clarkmoody reviewed Sep 22, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via 4f0daa1 September 23, 2023 05:43

tcharding force-pushed the 09-06-push-p2wpkh branch from 60882e8 to 4f0daa1 Compare September 23, 2023 05:43

apoelstra previously approved these changes Sep 23, 2023

View reviewed changes

sanket1729 reviewed Sep 24, 2023

View reviewed changes

tcharding dismissed apoelstra’s stale review via df758fe September 24, 2023 21:22

tcharding force-pushed the 09-06-push-p2wpkh branch from 4f0daa1 to df758fe Compare September 24, 2023 21:22

sanket1729 previously approved these changes Sep 25, 2023

View reviewed changes

tcharding mentioned this pull request Sep 25, 2023

feat: cookbook v1 rust-bitcoin/www.rust-bitcoin.org#9

Merged

12 tasks

tcharding dismissed sanket1729’s stale review via 764ac44 September 25, 2023 22:00

tcharding force-pushed the 09-06-push-p2wpkh branch from df758fe to 764ac44 Compare September 25, 2023 22:00

tcharding changed the title ~~Add push_p2wpkh function on Witness~~ Add Witness::p2wpkh constructor Sep 25, 2023

Add push_p2wpkh function on Witness

5901d35

In order to create the witness to spend p2wpkh output one must push the signature and the pubkey, we should have a function for this.

tcharding force-pushed the 09-06-push-p2wpkh branch from 764ac44 to 5901d35 Compare September 26, 2023 00:14

sanket1729 approved these changes Sep 26, 2023

View reviewed changes

apoelstra approved these changes Sep 26, 2023

View reviewed changes

apoelstra mentioned this pull request Sep 26, 2023

Create ecdsa::LegacySignature and ecdsa::SegwitV0Signature #1989

Draft

apoelstra merged commit 1c29dd9 into rust-bitcoin:master Sep 26, 2023
29 checks passed

tcharding deleted the 09-06-push-p2wpkh branch September 28, 2023 22:53

This was referenced Sep 30, 2023

bitcoin: Add signing examples #2095

Merged

fix: Cookbook for rust-bitcoin@0.31 rust-bitcoin/rust-bitcoin.github.io#7

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add `Witness::p2wpkh` constructor #2084

Add `Witness::p2wpkh` constructor #2084

tcharding commented Sep 20, 2023 •

edited

junderw Sep 20, 2023

tcharding Sep 20, 2023

junderw Sep 20, 2023

junderw Sep 20, 2023

tcharding Sep 20, 2023

tcharding Sep 20, 2023

junderw Sep 20, 2023

tcharding Sep 20, 2023

apoelstra commented Sep 20, 2023

apoelstra commented Sep 20, 2023

tcharding commented Sep 21, 2023

apoelstra commented Sep 21, 2023

apoelstra left a comment

tcharding commented Sep 21, 2023

clarkmoody Sep 22, 2023

tcharding Sep 23, 2023

tcharding commented Sep 23, 2023

apoelstra left a comment

sanket1729 left a comment

tcharding commented Sep 24, 2023

tcharding commented Sep 24, 2023 •

edited

tcharding commented Sep 24, 2023

apoelstra commented Sep 25, 2023 •

edited

tcharding commented Sep 25, 2023 •

edited

sanket1729 left a comment

apoelstra commented Sep 25, 2023

tcharding commented Sep 25, 2023

tcharding commented Sep 25, 2023

apoelstra commented Sep 26, 2023

tcharding commented Sep 26, 2023

tcharding commented Sep 26, 2023 •

edited

sanket1729 left a comment

apoelstra left a comment

Add Witness::p2wpkh constructor #2084

Add Witness::p2wpkh constructor #2084

Conversation

tcharding commented Sep 20, 2023 • edited

Notes

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

apoelstra commented Sep 20, 2023

apoelstra commented Sep 20, 2023

tcharding commented Sep 21, 2023

apoelstra commented Sep 21, 2023

apoelstra left a comment

Choose a reason for hiding this comment

tcharding commented Sep 21, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tcharding commented Sep 23, 2023

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 left a comment

Choose a reason for hiding this comment

tcharding commented Sep 24, 2023

tcharding commented Sep 24, 2023 • edited

tcharding commented Sep 24, 2023

apoelstra commented Sep 25, 2023 • edited

tcharding commented Sep 25, 2023 • edited

sanket1729 left a comment

Choose a reason for hiding this comment

apoelstra commented Sep 25, 2023

tcharding commented Sep 25, 2023

tcharding commented Sep 25, 2023

apoelstra commented Sep 26, 2023

tcharding commented Sep 26, 2023

tcharding commented Sep 26, 2023 • edited

sanket1729 left a comment

Choose a reason for hiding this comment

apoelstra left a comment

Choose a reason for hiding this comment

Add `Witness::p2wpkh` constructor #2084

Add `Witness::p2wpkh` constructor #2084

tcharding commented Sep 20, 2023 •

edited

tcharding commented Sep 24, 2023 •

edited

apoelstra commented Sep 25, 2023 •

edited

tcharding commented Sep 25, 2023 •

edited

tcharding commented Sep 26, 2023 •

edited