Change EcdsaSig hash type deser in psbt #776

sanket1729 · 2022-01-13T04:31:13Z

Changes the parsing behavior in PSBT on non-standard sighash types to give an explicit error, rather than silently mangling the parsed value

Kixunil

Looks correct. How hard would it be to introduce a new type? I guess not even urgent for Taproot, right?

Kixunil · 2022-01-13T22:57:05Z

src/blockdata/transaction.rs

+     /// This might cause unexpected behavior because it does not roundtrip. That is,
+     /// `EcdsaSigHashType::from_u32_consensus(n) as u32 != n` for non-standard values of
+     /// `n`. While verifying signatures, the user should retain the `n` and use it compute the
+     /// signature hash message.
     pub fn from_u32_consensus(n: u32) -> EcdsaSigHashType {


Wouldn't it be better to have a separate type allowing this? (One a newtype over the other.)

I think if people are dealing with non-standard stuff, they better know what they are doing. So, I don't think we should concern ourselves by adding more code to safeguard non-standard users.

It is not an effort to introduce a new type, but I think I would be more comfortable with less code bloat about a code that no one is using. Document the non-standard stuff, and if someone wants it they can PR one and raise an issue.

We still need non-standard to validate consensus/process valid block, right? Couldn't it be a problem?

Yes, we need it. Our code for legacy_signature_hash does take input a sighash_u32 that we expect the user to provide correctly!

However, our code for bip143 segwitv0 sighash does not support signature verification of non-standard transactions! This is a bug and should be addressed independently.

src/util/ecdsa.rs

Kixunil · 2022-01-13T23:06:07Z

src/util/psbt/serialize.rs

            }
-            _ => Err(encode::Error::ParseFailed("Invalid Schnorr signature len"))
+            Err(schnorr::SchnorrSigError::InvalidSchnorrSigSize(_)) =>
+                Err(encode::Error::ParseFailed("Invalid Schnorr signature len")),


Nit: I'd prefer not to abbreviate "length" in an error message.

Kixunil · 2022-01-13T23:07:27Z

src/util/psbt/serialize.rs

+                Err(encode::Error::from(psbt::Error::NonStandardSigHashType(flag))),
+            Err(EcdsaSigError::Secp256k1(..)) =>
+                Err(encode::Error::ParseFailed("Invalid Ecdsa signature")),
+            Err(EcdsaSigError::HexEncoding(..)) => unreachable!("Decoding from slice, not hex")


Unrelated but this is why I don't like catch-all error types.

dr-orlovsky

ACK efbeba0

ebdeed0 Cleanup imports (sanket1729) 382c8f9 Introduce PsbtSigHashType (sanket1729) Pull request description: We cannot really use `Psbt` for taproot because the sighash type is currently EcdsaSigHashType. We could introduce an enum with two options but then deser is not really clear, so I chose the approach in the current PR. Feedback or other ways to do this welcome :) This is NOT related to #776 ACKs for top commit: apoelstra: ACK ebdeed0 dr-orlovsky: ACK ebdeed0 Tree-SHA512: f9424cf3db09098d73f0d431a45ff86a47f11f7d40785bf95e58991fd4d16f0db0a9a3a63f898628b29c95bbd2ca901312a6a44ac6d8aec73a6a34710f6354a2

dr-orlovsky · 2022-01-14T09:31:38Z

Needs rebase after #779 merge

dr-orlovsky

ACK abe52f6

dr-orlovsky · 2022-01-15T09:31:01Z

@Kixunil let's get this closed and ship taproot! 🥇

apoelstra

ACK abe52f6

@sanket1729 can you change the description from "borderline trivial" to "Changes the parsing behavior in PSBT on non-standard sighash types to give an explicit error, rather than silently mangling the parsed value"?

My first thought was to NACK this PR because we didn't have agreement on what the correct behavior was, but after studying it, I realized that our current behavior is definitely wrong and that this is at least an improvement.

sanket1729 · 2022-01-15T15:57:17Z

@apoelstra changed the description.

dr-orlovsky · 2022-01-16T10:54:55Z

We already have 2 ACKs, but I assume it would be nice to hear from @Kixunil reviewing this previously that all his comments are addresses before merging this

Kixunil

ACK abe52f6

Go ahead and merge this, we can make a trivial followup issue/PR to rewrite to map_err

Kixunil · 2022-01-17T09:32:17Z

src/util/psbt/serialize.rs

+            Err(schnorr::SchnorrSigError::InvalidSchnorrSigSize(_)) =>
+                Err(encode::Error::ParseFailed("Invalid Schnorr signature length")),
+            Err(schnorr::SchnorrSigError::Secp256k1(..)) =>
+                Err(encode::Error::ParseFailed("Invalid Schnorr signature")),


This could've been map_err which would communicate the intent a bit better. Only realized it now.

Issue: #793

1b2dbd7 0.28.0-rc.1 release (Dr Maxim Orlovsky) Pull request description: We still have quite a few issues and PRs pending to be addressed/merged before full 0.28 release: see https://github.com/rust-bitcoin/rust-bitcoin/milestone/10 Most important, we have to find a way to address #777; additionally it will be desirable to get #587, #786, #776. We also have quite of work to write CHANGELOG in #785 Nevertheless I propose to start with a `beta-1` subrelease to unlock ability for `miniscript` release and downstream testing. The Taproot API looks final, and the pending PRs are addresses internal issues/bugs which is perfectly fine for beta releases. ACKs for top commit: Kixunil: ACK 1b2dbd7 Tree-SHA512: a7bd6dd3e7a489f7fd758fb0d7a60103bfe0cdf88997b2163f163841fa1c12e7b31fa8f03b9f817a671379578dbc10a2ca583f49b64d2d2de4a9ebb57b2b9bd5

sanket1729 added the bug label Jan 13, 2022

sanket1729 added this to the 0.28.0 milestone Jan 13, 2022

sanket1729 force-pushed the psbt_sighash_ty branch from 1efbad5 to 5176bee Compare January 13, 2022 04:38

apoelstra mentioned this pull request Jan 13, 2022

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

Open

This was referenced Jan 13, 2022

Introduce PsbtSigHashType #779

Merged

Tracking issue for Taproot #503

Closed

Kixunil reviewed Jan 13, 2022

View reviewed changes

sanket1729 force-pushed the psbt_sighash_ty branch 2 times, most recently from f3134b8 to 2a91e4a Compare January 14, 2022 00:06

sanket1729 mentioned this pull request Jan 14, 2022

Rust 2018 & MSRV bump 1.41.1 tracking issue #510

Closed

20 tasks

sanket1729 force-pushed the psbt_sighash_ty branch from 2a91e4a to efbeba0 Compare January 14, 2022 00:14

dr-orlovsky added this to In progress in PSBT via automation Jan 14, 2022

dr-orlovsky previously approved these changes Jan 14, 2022

View reviewed changes

sanket1729 dismissed dr-orlovsky’s stale review via a74f088 January 15, 2022 00:39

sanket1729 force-pushed the psbt_sighash_ty branch from efbeba0 to a74f088 Compare January 15, 2022 00:39

PSBT automation moved this from In progress to Review in progress Jan 15, 2022

sanket1729 force-pushed the psbt_sighash_ty branch from a74f088 to 649f6cc Compare January 15, 2022 00:43

sanket1729 added 4 commits January 15, 2022 06:15

Change EcdsaSig hash type deser

0fed04e

Update documentation of from_u32_consensus

85009a7

Update documentation of EcdsaSig::from_slice

fbd86dc

Cleanup/Dedup psbt (De)Serialization code

abe52f6

sanket1729 force-pushed the psbt_sighash_ty branch from 649f6cc to abe52f6 Compare January 15, 2022 00:46

dr-orlovsky approved these changes Jan 15, 2022

View reviewed changes

dr-orlovsky requested a review from Kixunil January 15, 2022 09:30

dr-orlovsky added code quality Makes code easier to understand and less likely to lead to problems documentation one ack PRs that have one ACK, so one more can progress them labels Jan 15, 2022

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

PSBT automation moved this from Review in progress to Ready for merge Jan 15, 2022

apoelstra approved these changes Jan 15, 2022

View reviewed changes

dr-orlovsky added this to Ready for merge in Taproot Jan 15, 2022

dr-orlovsky mentioned this pull request Jan 16, 2022

Release v0.28 RC1 #789

Merged

Kixunil mentioned this pull request Jan 17, 2022

Support signature verification of non-standard transactions #792

Open

Kixunil approved these changes Jan 17, 2022

View reviewed changes

dr-orlovsky merged commit d5686ee into rust-bitcoin:master Jan 17, 2022

Taproot automation moved this from Ready for merge to Done Jan 17, 2022

PSBT automation moved this from Ready for merge to Done Jan 17, 2022

dr-orlovsky mentioned this pull request Jan 17, 2022

Use map_err in PSBT EcdsaSigHash deserializer #793

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Change EcdsaSig hash type deser in psbt #776

Change EcdsaSig hash type deser in psbt #776

sanket1729 commented Jan 13, 2022 •

edited

Kixunil left a comment

Kixunil Jan 13, 2022

sanket1729 Jan 13, 2022

Kixunil Jan 14, 2022

sanket1729 Jan 15, 2022

Kixunil Jan 13, 2022

Kixunil Jan 13, 2022

dr-orlovsky left a comment

dr-orlovsky commented Jan 14, 2022

dr-orlovsky left a comment

dr-orlovsky commented Jan 15, 2022

apoelstra left a comment

sanket1729 commented Jan 15, 2022

dr-orlovsky commented Jan 16, 2022

Kixunil left a comment

Kixunil Jan 17, 2022

dr-orlovsky Jan 17, 2022

Change EcdsaSig hash type deser in psbt #776

Change EcdsaSig hash type deser in psbt #776

Conversation

sanket1729 commented Jan 13, 2022 • edited

Kixunil 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

dr-orlovsky left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Jan 14, 2022

dr-orlovsky left a comment

Choose a reason for hiding this comment

dr-orlovsky commented Jan 15, 2022

apoelstra left a comment

Choose a reason for hiding this comment

sanket1729 commented Jan 15, 2022

dr-orlovsky commented Jan 16, 2022

Kixunil left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

sanket1729 commented Jan 13, 2022 •

edited