Change EcdsaSig hash type deser in psbt#776
Conversation
1efbad5 to
5176bee
Compare
Kixunil
left a comment
There was a problem hiding this comment.
Looks correct. How hard would it be to introduce a new type? I guess not even urgent for Taproot, right?
src/blockdata/transaction.rs
Outdated
There was a problem hiding this comment.
Wouldn't it be better to have a separate type allowing this? (One a newtype over the other.)
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
We still need non-standard to validate consensus/process valid block, right? Couldn't it be a problem?
There was a problem hiding this comment.
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/psbt/serialize.rs
Outdated
There was a problem hiding this comment.
Nit: I'd prefer not to abbreviate "length" in an error message.
src/util/psbt/serialize.rs
Outdated
There was a problem hiding this comment.
Unrelated but this is why I don't like catch-all error types.
f3134b8 to
2a91e4a
Compare
2a91e4a to
efbeba0
Compare
dr-orlovsky
left a comment
There was a problem hiding this comment.
ACK efbeba0a56088078e14f89fa26a03884a5460f8a
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
|
Needs rebase after #779 merge |
a74f088 to
649f6cc
Compare
649f6cc to
abe52f6
Compare
|
@Kixunil let's get this closed and ship taproot! 🥇 |
apoelstra
left a comment
There was a problem hiding this comment.
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.
|
@apoelstra changed the description. |
|
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 |
| Err(schnorr::SchnorrSigError::InvalidSchnorrSigSize(_)) => | ||
| Err(encode::Error::ParseFailed("Invalid Schnorr signature length")), | ||
| Err(schnorr::SchnorrSigError::Secp256k1(..)) => | ||
| Err(encode::Error::ParseFailed("Invalid Schnorr signature")), |
There was a problem hiding this comment.
This could've been map_err which would communicate the intent a bit better. Only realized it now.
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
Changes the parsing behavior in PSBT on non-standard sighash types to give an explicit error, rather than silently mangling the parsed value