PSBT: partial sig data type #669
Conversation
src/util/psbt/mod.rs
Outdated
| type Err = encode::Error; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| let hex = Vec::from_hex(s) |
There was a problem hiding this comment.
nit: this is bytes, not a hex
src/util/psbt/mod.rs
Outdated
| /// [`crate::TxIn`] | ||
| #[derive(Copy, Clone, Eq, PartialEq, Debug)] | ||
| #[cfg_attr(feature = "serde", derive(Serialize, Deserialize))] | ||
| pub struct PartialSignature { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
For taproot we will need a different structure - see #671
It has a different serialization logic.
src/util/psbt/serialize.rs
Outdated
| 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]), |
There was a problem hiding this comment.
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.
src/util/psbt/serialize.rs
Outdated
| 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")) |
There was a problem hiding this comment.
Is there any standardness rule around sig size? In any case, this should allow for non-standard things
|
i think i'm a nack here -- wouldn't this break compat with the PSBT spec? |
|
@JeremyRubin removed the part that may break PSBT compatibility @sanket1729 addressed your suggestions |
|
Hmm isn't the compat issue the partial sig itself? how is it fixed/what did you fix that you thought was the isseu? |
|
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) |
|
concept ACK, but needs rebase |
|
@apoelstra rebased |
|
@dr-orlovsky, I think the new EcsdaSignature type is a good fit here. We should not need a custom type for psbt partial sigs |
|
sorry to just be coming back to this.... BIP-174 describes the partial signature field as: i think this (edit: this PR) precludes the use of a nulldummy, which we should also support, no? |
src/util/psbt/map/input.rs
Outdated
| @@ -266,7 +265,7 @@ impl Map for Input { | |||
| } | |||
|
|
|||
| impl_psbt_get_pair! { | |||
| rv.push(self.partial_sigs as <PSBT_IN_PARTIAL_SIG, PublicKey>|<Vec<u8>>) | |||
| rv.push(self.partial_sigs as <PSBT_IN_PARTIAL_SIG, PublicKey>|<PartialSignature>) | |||
There was a problem hiding this comment.
Why here is PartialSignature instead of PartialSig?
There was a problem hiding this comment.
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
There was a problem hiding this comment.
Created an issue: #754
There was a problem hiding this comment.
Also fixed the same problem for SchnorrSig in the current implementation in 5e4687817fe132db65295b36ff6f67e391b23699
src/util/psbt/mod.rs
Outdated
| return Err(encode::Error::ParseFailed("empty partial signature data in hex representation")) | ||
| } | ||
| let len = bytes.len(); | ||
| let sighash_byte = bytes[len - 1]; |
There was a problem hiding this comment.
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.
src/util/psbt/serialize.rs
Outdated
|
|
||
| impl Deserialize for PartialSig { | ||
| fn deserialize(bytes: &[u8]) -> Result<Self, encode::Error> { | ||
| if bytes.len() <= 1 { |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
|
@sanket1729 I have re-factored this PR on top of your work on @Kixunil addresses the issue we discussed @JeremyRubin not sure I understood your comment... |
src/util/ecdsa.rs
Outdated
| @@ -453,11 +454,33 @@ impl EcdsaSig { | |||
| } | |||
| } | |||
|
|
|||
| impl fmt::Display for EcdsaSig { | |||
| fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result { | |||
| write!(f, "{}:{}", self.sig.serialize_der().to_hex(), self.hash_ty as u8) | |||
There was a problem hiding this comment.
In 70515b322bd9c90e0d8133e6c9c3fe08986f0092:
Can you use hex::format_hex here to avoid the allocation?
|
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. |
|
@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. |
|
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. |
|
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 Furthermore, from BIP 174
I don't know if NULLDUMMY really qualifies as a signature. |
src/util/psbt/serialize.rs
Outdated
| @@ -20,7 +20,7 @@ | |||
|
|
|||
| use prelude::*; | |||
|
|
|||
| use io; | |||
| use ::{EcdsaSig, io}; | |||
There was a problem hiding this comment.
nit: Non-merge blocking.
I don't like merging std imports in the same line as same library imports.
There was a problem hiding this comment.
Oh, this is IDE's work. Will fix if there will be another code update round
|
I think we can do another enum type, like |
|
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). |
| type Err = EcdsaSigError; | ||
|
|
||
| fn from_str(s: &str) -> Result<Self, Self::Err> { | ||
| let bytes = Vec::from_hex(s)?; |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
But if the signature is less than 73 bytes the <[u8; 73]>::from_hex would fail, isn't it?
There was a problem hiding this comment.
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].
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
Ok, will do as a follow-up PR
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
@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
|
Done reviewing 550948e1a632105a9d91a79f4fe415fb2b3c99ee. Just a couple nits. |
|
@apoelstra and @sanket1729 fixed your suggestions - except the allocation one, on which I provided rationale in the comment above. |
| impl Serialize for EcdsaSig { | ||
| fn serialize(&self) -> Vec<u8> { | ||
| let mut buf = Vec::with_capacity(72); | ||
| buf.extend(self.sig.serialize_der().iter()); |
There was a problem hiding this comment.
Note: extend_from_slice not needed because extend is already specialized. Unimportant: could've been & instead of .iter() I think.
There was a problem hiding this comment.
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().
There was a problem hiding this comment.
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...
| // 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) | ||
| }) |
There was a problem hiding this comment.
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 was a problem hiding this comment.
There is no TryFrom in the current MSRV :(
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Oh, that's surprising difference. Why are they different?
There was a problem hiding this comment.
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
|
@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... |
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.