PSBT: partial sig data type by dr-orlovsky · Pull Request #669 · rust-bitcoin/rust-bitcoin

dr-orlovsky · 2021-09-29T00:41:53Z

Previously signatures were kept in PSBT as raw byte vec, without processing. This adds specific data type, capable of holding & serializing/deserializing partial signature with sighash flag information.

sanket1729

Don't have a strong preference about renaming the ECDSA thing in this PR(up to you). Left a few comments, rest looks good

sanket1729 · 2021-09-30T18:38:20Z

src/util/psbt/mod.rs

+    type Err = encode::Error;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let hex = Vec::from_hex(s)


nit: this is bytes, not a hex

sanket1729 · 2021-09-30T18:41:34Z

src/util/psbt/mod.rs

+/// [`crate::TxIn`]
+#[derive(Copy, Clone, Eq, PartialEq, Debug)]
+#[cfg_attr(feature = "serde", derive(Serialize, Deserialize))]
+pub struct PartialSignature {


nit(don't fix unless there is some other thing): in the near future we will have a separate field for Schnorr sigs, so this can be better named as PartialEcdsaSig or PartialEcdsaSignature.

For taproot we will need a different structure - see #671

It has a different serialization logic.

sanket1729 · 2021-09-30T18:48:46Z

src/util/psbt/serialize.rs

+impl Deserialize for PartialSignature {
+    fn deserialize(bytes: &[u8]) -> Result<Self, encode::Error> {
+        let (sig_slice, sighash_byte) = match bytes.len() {
+            70..=73 => (&bytes[..bytes.len() - 1], bytes[bytes.len() - 1]),


Fun fact: It is also possible to have short signatures(< 70 bytes) in bitcoin. See:
bitcoin/bitcoin@ea5a50f/src/secp256k1/src/ecdsa_impl.h#L189-L190

I think with a probability of 1/1024 we can have sigs of 70 bytes. 1/(1024*256) 69 bytes and so on. Russell o'connor from Blockstream has used sigs as short as 300602010102010101 on testnet.

sanket1729 · 2021-09-30T18:49:59Z

src/util/psbt/serialize.rs

+    fn deserialize(bytes: &[u8]) -> Result<Self, encode::Error> {
+        let (sig_slice, sighash_byte) = match bytes.len() {
+            70..=73 => (&bytes[..bytes.len() - 1], bytes[bytes.len() - 1]),
+            _ => return Err(encode::Error::ParseFailed("non-standard partial signature length"))


Is there any standardness rule around sig size? In any case, this should allow for non-standard things

JeremyRubin · 2021-11-10T21:09:52Z

i think i'm a nack here -- wouldn't this break compat with the PSBT spec?

dr-orlovsky · 2021-11-12T21:16:25Z

@JeremyRubin removed the part that may break PSBT compatibility

@sanket1729 addressed your suggestions

JeremyRubin · 2021-11-12T22:21:50Z

Hmm isn't the compat issue the partial sig itself? how is it fixed/what did you fix that you thought was the isseu?

dr-orlovsky · 2021-11-12T23:16:09Z

I think I do not understand your original comment "break compat with the PSBT spec". Which part of the code breaks the spec? (I thought it was additional requirements on signature, which are not a part of the spec and which @sanket1729 was talking about in his review)

apoelstra · 2021-12-20T14:55:44Z

concept ACK, but needs rebase

dr-orlovsky · 2021-12-30T00:04:31Z

@apoelstra rebased

sanket1729 · 2021-12-30T17:43:26Z

@dr-orlovsky, I think the new EcsdaSignature type is a good fit here. We should not need a custom type for psbt partial sigs

JeremyRubin · 2021-12-30T19:17:02Z

sorry to just be coming back to this....

BIP-174 describes the partial signature field as:

The signature as would be pushed to the stack from a scriptSig or witness.

i think this (edit: this PR) precludes the use of a nulldummy, which we should also support, no?

RCasatta · 2021-12-31T09:31:05Z

src/util/psbt/map/input.rs

Why here is PartialSignature instead of PartialSig?

I had spotted this bug(not this specific instance) while porting this to rust-elements. The last part of the macro expression for impl_psbt_get_pair is not being used. I did not consider it a serious problem but if this can cause this confusion, I think we should clean up the macro in a later PR. At least for this instance, this should be PartialSig

Created an issue: #754

Also fixed the same problem for SchnorrSig in the current implementation in 5e4687817fe132db65295b36ff6f67e391b23699

Kixunil

The +-1 issue looks like a bug to me, please explain if it's not.

Kixunil · 2022-01-03T16:02:07Z

src/util/psbt/mod.rs

I think

let (sighash_byte, signature) = bytes.split_last() .ok_or(encode::Error::ParseFailed("empty partial signature data in hex representation"))?;

without explicit is_empty check would be more readable.

Kixunil · 2022-01-03T16:06:28Z

src/util/psbt/serialize.rs

Why is this checking <= 1 ant the one above is_empty? I think they should be the same (presumably is_empty but see above).

If I'm not mistaken from_str could just call deserialize() after hex-decoding which would avoid such discrepancies.

This is not a bug but rather tradeoff on how to report error.

If we have len == 1, we assume the last byte to be a sighash - and signature to be empty. Above, we error on it while parsing signature (empty signature will always error on parsing). Here, we explicitly check and error on empty sigs before parsing.

After consideration I think it is better to use error from signature deserializer and propagate it (i.e. they way you suggested to do above with split_last.

I am refactoring this PR and will use this strategy. Pls let me know if you do not agree.

dr-orlovsky · 2022-01-06T11:11:03Z

@sanket1729 I have re-factored this PR on top of your work on EcdsaSig

@Kixunil addresses the issue we discussed

@JeremyRubin not sure I understood your comment...

apoelstra · 2022-01-06T16:41:29Z

src/util/ecdsa.rs

In 70515b322bd9c90e0d8133e6c9c3fe08986f0092:

Can you use hex::format_hex here to avoid the allocation?

Fixed this too

apoelstra

ACK 5e4687817fe132db65295b36ff6f67e391b23699

except for the hex-encoding thing, which I don't feel too strongly about

JeremyRubin · 2022-01-07T21:16:40Z

I think it boils down to that:

pub partial_sigs: BTreeMap<PublicKey, EcdsaSig>,

must instead be one of:

pub partial_sigs: BTreeMap<PublicKey, Option<EcdsaSig>>,
pub partial_sigs: BTreeMap<PublicKey, Result<EcdsaSig, NullDummy>>,
pub partial_sigs: BTreeMap<PublicKey, Result<EcdsaSig, Vec<u8>>>,

to actually capture what might be encoded in a PSBT. I'm not sure which is the best option.

dr-orlovsky · 2022-01-07T21:46:46Z

@JeremyRubin seems this question is much more broad and not directly related to this PR. Also, it probably should affect other fields as well. Probably opening an issue would be the best way forward to start discussion on the matter.

JeremyRubin · 2022-01-07T22:11:29Z

It is to be discussed here because you are introducing a substantive breaking change by making the partial_sig go from a Vec to an ECDSASig.

sanket1729 · 2022-01-08T03:43:58Z

NULLDUMMY might be useful to dissatisfy a public key. However, I think that such dissatisfaction can be produced by finalizers themselves. For example, a miniscript finalizer would do it if required.

We are already breaking the spec by enforcing extra things using the rust-type system. Bitcoin core/psbt spec does not even check if the public keys are fully valid curve points. Nor, does it check if the schnorr signature is well-formed. By this logic, we would be just left with Map from Vec<u8> -> Vec<u8>. I think we should go ahead with this PR for a more ergonomic API.

Furthermore, from BIP 174

The signature as would be pushed to the stack from a scriptSig or witness.

I don't know if NULLDUMMY really qualifies as a signature.

sanket1729

ACK 550948e1a632105a9d91a79f4fe415fb2b3c99ee. Waiting a day to see what others think about the NULLDUMMY issue.

sanket1729 · 2022-01-08T03:46:14Z

src/util/psbt/serialize.rs

 use prelude::*;

-use io;
+use ::{EcdsaSig, io};


nit: Non-merge blocking.

I don't like merging std imports in the same line as same library imports.

Oh, this is IDE's work. Will fix if there will be another code update round

dr-orlovsky · 2022-01-08T12:27:55Z

I think we can do another enum type, like PsbtEcdsaSig, with a normal sig and NullDummy variants. If others agree on this I can do it as a follow-up PR since this matter is controversial and will take more time to review/discuss comparing to a trivial current PR.

apoelstra · 2022-01-08T15:10:24Z

After discussion with Jeremy and achow101 on IRC, my view is that a NULLDUMMY is not a signature, does not belong in sigdata, and was only accepted because of weak parsing rules (similarly, in Core these are accepted for ECDSA sigs but not schnorr ones, simply because the parser does a length check for Schnorr but can't do a sensible length check on ECDSA sigs).

I am not entirely convinced of the value of letting signers provide a non-binding (and forgeable even!) assertion that they are not intending to sign with a specific key, but regardless, abusing the signature data field to do this is non-standard and confusing.

I also agree with Sanket that under normal circumstances, it is the finalizer's job to decide where to put NULLDUMMY's (and other dissatisfactions, none of which involve signatures and therefore none of which even "belong" to any specific participant).

apoelstra · 2022-01-08T15:20:05Z

src/util/ecdsa.rs

+    type Err = EcdsaSigError;
+
+    fn from_str(s: &str) -> Result<Self, Self::Err> {
+        let bytes = Vec::from_hex(s)?;


In 0af1c3f:

Sorry to nit about performance again, but could you use a [u8; 73] here, or better, lop off the last two characters and then directly call Signature::from_str? Again, won't hold up merging for this.

But if the signature is less than 73 bytes the <[u8; 73]>::from_hex would fail, isn't it?

No -- you can check the implementation of Signature::from_str which does exactly this. (Agtually I think you're right that <[u8;73]>::from_hex won't work, but there is something almost-as-simple involving [u8; 72] which I found by checking the implementation of rust-secp src/ecdsa/mod.rs).

My concern though is that we probably special-cased [u8; 72] as an implementor of FromHex and didn't special-case [u8; 73].

Oh @dr-orlovsky sorry, I just checked again and rust-secp uses a private method from_hex, which despite being suggestively named, is not part of the FromHex trait and is not exported. So you can't do that.

What you can do is split the last two characters off of the string, use Signature::from_str and u8::from_hex, which is a bit uglier especially as Rust doesn't like you splitting strings by bytes like this (and we probably need to check that the string is all-ASCII first to avoid splitting characters).

But again, I won't hold the PR up for this if you feel this is overcomplicated.

Splitting off will require multiple checks on the string length and, more importantly, on the fact that it is a correct UTF-8 string - or will introduce a panic. Arguably this will present more computational load than just a single allocation as now.

If we will stick to using Signature-style of decoding, it will require introducing additional from_hex function, duplicating logic from the same private function in secp256k1. Both seems overkill for such a marginal or arguable performance improve.

Checking a short string for ASCII will definitely be less computational load than an allocation, even in the optimistic case that there is no memory fragmentation. And in terms of code, all you need to do is call str.is_ascii().

Ok, will do as a follow-up PR

Perhaps for the future, there's an even more performant way to implement this. Write fn from_hex_bytes(hex_bytes: &[u8]) -> Result<Self, Error> on Signature which decodes hex without requiring type proof of UTF-8 - all hex chars are ASCII anyway so if there's anything wrong it'll just error. Then call the byte implementation from both places. Then it becomes safe to drop last two bytes and a the optimizer should throw away redundant bounds checks if the function checks constant len at the top.

Thus no redundant checking if it's ASCII nor UTF-8 chars checks.

@apoelstra there is no u8::from_hex impl (as well as <[u8; 1]>::from_hex), so this can't be done the way you suggested.

@Kixunil what you propose requires changes to Signature + new release in rust-secp256k1 lib

src/util/psbt/map/input.rs

apoelstra · 2022-01-08T15:26:33Z

Done reviewing 550948e1a632105a9d91a79f4fe415fb2b3c99ee. Just a couple nits.

dr-orlovsky · 2022-01-08T15:39:04Z

@apoelstra and @sanket1729 fixed your suggestions - except the allocation one, on which I provided rationale in the comment above.

apoelstra

ACK 14ace92

Kixunil

ACK 14ace92

My comments are not too important. We can make them good first issues so newcomers have something to play with. :)

Kixunil · 2022-01-08T17:29:00Z

src/util/psbt/serialize.rs

+impl Serialize for EcdsaSig {
+    fn serialize(&self) -> Vec<u8> {
+        let mut buf = Vec::with_capacity(72);
+        buf.extend(self.sig.serialize_der().iter());


Note: extend_from_slice not needed because extend is already specialized. Unimportant: could've been & instead of .iter() I think.

Not working that way: serialize_der returns SerializedSignature, which is not an iterator - and requires &* prefix to deref first into slice and than get iterator over it. IMO having this prefix is much uglier than calling iter().

Ah, thought it's [u8; N]. Would be nice to impl IntoIterator for SerailizedSignature

Edit: just looked it up and it derefs to &[u8] so it would work. Also looking at the insides of the type this code is wrong - capacity should've been 73. Or rather than hand-computing:

self.sig.serialize_der().iter().copied().chain(core::iter::once(self.hash_ty as u8)).collect()

Why didn't I realize this earlier...

Kixunil · 2022-01-08T17:33:32Z

src/util/psbt/serialize.rs

+            // a scriptSig or witness" we should use a consensus deserialization and do
+            // not error on a non-standard values.
+            hash_ty: EcdsaSigHashType::from_u32_consensus(*sighash_byte as u32)
+        })


This looks nearly identical to the implementation of FromStr. It'd be nicer to put the common part into a separate function and call it from both places. Maybe even impl<'a> TryFrom<&[u8]> for EcdsaSig

There is no TryFrom in the current MSRV :(

And they are not the same: one uses from_u32_consensus and the other from_u32_standard, which is an important difference. Also, errors are different, so moving this code to a shared function will result in having more code and not less.

Oh, that's surprising difference. Why are they different?

Since when we parsing string, we require user to provide us only correct values - and when parsing PSBT, we have to follow consensus serialization rules for signatures

dr-orlovsky · 2022-01-08T19:42:03Z

@Kixunil can you merge this pls? Let's do your suggestions as a follow-up since I do not want to disturb Andrew with re-ACKing over and over again...

dr-orlovsky added the API break This PR requires a version bump for the next release label Sep 29, 2021

dr-orlovsky added this to the 0.28.0 milestone Sep 29, 2021

dr-orlovsky changed the title ~~PSBT: implicit partial sig data type~~ PSBT: partial sig data type Sep 29, 2021

sanket1729 reviewed Sep 30, 2021

View reviewed changes

dr-orlovsky mentioned this pull request Nov 14, 2021

Miniscript tapscript support (Pr 2 of 3) rust-bitcoin/rust-miniscript#275

Merged

RCasatta reviewed Dec 31, 2021

View reviewed changes

Kixunil requested changes Jan 3, 2022

View reviewed changes

dr-orlovsky mentioned this pull request Jan 6, 2022

Remove unused last argument in impl_psbt_get_pair macro #754

Closed

apoelstra reviewed Jan 6, 2022

View reviewed changes

apoelstra previously approved these changes Jan 6, 2022

View reviewed changes

sanket1729 mentioned this pull request Jan 7, 2022

Tracking issue for Taproot #503

Closed

20 tasks

dr-orlovsky mentioned this pull request Jan 7, 2022

Derive Se/Deserialize for EcdsaSig #756

Closed

dr-orlovsky added 2 commits January 7, 2022 21:49

Add EcdsaSig::sighash_all convenience constructor

c36a3da

Improve NonStandardSigHashType

daf0eac

dr-orlovsky dismissed apoelstra’s stale review via 2c7053c January 7, 2022 20:53

dr-orlovsky requested review from Kixunil, RCasatta, apoelstra and sanket1729 January 7, 2022 20:53

dr-orlovsky added 2 commits January 7, 2022 21:57

Add Display and FromStr for EcdsaSig

0af1c3f

PSBT serialize/deserialize impl for EcdsaSig type

c92057d

dr-orlovsky added 2 commits January 7, 2022 21:57

Add serde impl for EcdsaSig

141dbbd

Use EcdsaSig in PSBT partial signatures instead of Vec<u8>

2b53000

sanket1729 previously approved these changes Jan 8, 2022

View reviewed changes

apoelstra reviewed Jan 8, 2022

View reviewed changes

src/util/psbt/map/input.rs Outdated Show resolved Hide resolved

Fix SchnorrSig type references in PSBT serialization macros

14ace92

dr-orlovsky dismissed sanket1729’s stale review via 14ace92 January 8, 2022 15:30

apoelstra approved these changes Jan 8, 2022

View reviewed changes

Kixunil approved these changes Jan 8, 2022

View reviewed changes

apoelstra merged commit 2ec9af3 into rust-bitcoin:master Jan 8, 2022

Kixunil mentioned this pull request Jan 8, 2022

Unify EcdsaSig decoding #759

Closed

JeremyRubin mentioned this pull request Jan 8, 2022

[BIP-174] Clarify that partial_sig should be a valid. bitcoin/bips#1270

Merged

This was referenced Jan 12, 2022

Remove fuzztaget and cleanup psbt finalizer rust-bitcoin/rust-miniscript#290

Merged

non-standard ECDSA sighash types are not preserved, but allowed in PSBT #777

Open

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

Conversation

dr-orlovsky commented Sep 29, 2021

Uh oh!

sanket1729 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!

JeremyRubin commented Nov 10, 2021

Uh oh!

dr-orlovsky commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

JeremyRubin commented Nov 12, 2021

Uh oh!

dr-orlovsky commented Nov 12, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

apoelstra commented Dec 20, 2021

Uh oh!

dr-orlovsky commented Dec 30, 2021

Uh oh!

sanket1729 commented Dec 30, 2021

Uh oh!

JeremyRubin commented Dec 30, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

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!

dr-orlovsky Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Kixunil 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!

dr-orlovsky commented Jan 6, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

apoelstra left a comment

Choose a reason for hiding this comment

Uh oh!

JeremyRubin commented Jan 7, 2022

Uh oh!

dr-orlovsky commented Jan 7, 2022

Uh oh!

JeremyRubin commented Jan 7, 2022

Uh oh!

sanket1729 commented Jan 8, 2022

Uh oh!

sanket1729 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!

dr-orlovsky commented Jan 8, 2022

dr-orlovsky commented Nov 12, 2021 •

edited

Loading

dr-orlovsky commented Nov 12, 2021 •

edited

Loading

JeremyRubin commented Dec 30, 2021 •

edited

Loading

dr-orlovsky Jan 6, 2022 •

edited

Loading

dr-orlovsky commented Jan 6, 2022 •

edited

Loading

apoelstra Jan 8, 2022 •

edited

Loading