Separate signature hash types by sanket1729 · Pull Request #702 · rust-bitcoin/rust-bitcoin

sanket1729 · 2021-11-16T00:10:10Z

Fixes #670 . Separates SchnorrSigHashType and LegacySigHashType. Also adds the following new structs:

pub struct SchnorrSig {
    /// The underlying schnorr signature
    pub sig: secp256k1::schnorrsig::Signature,
    /// The corresponding hash type
    pub hash_ty: SchnorrSigHashType,
}

pub struct EcdsaSig {
    /// The underlying DER serialized Signature
    pub sig: secp256k1::Signature,
    /// The corresponding hash type
    pub hash_ty: LegacySigHashType,
}

This code is currently minimal to aid reviews. We can at a later point implement (Encodeable, psbt::Serialize, FromHex, ToHex) etc in follow-up PRs.

dr-orlovsky

Left some comments and questions

src/blockdata/transaction.rs

src/util/ecdsa.rs

src/util/schnorr.rs

sanket1729 · 2021-11-19T18:57:01Z

Ready for review again.

thomaseizinger

Few ideas :)

src/util/schnorr.rs

thomaseizinger · 2021-11-20T20:25:16Z

src/util/schnorr.rs

This could be written as an ok_or_else after the split_last. That would get rid of the explicit pattern match.

Also if this is kept, use return Err(...) instead of Err(...)?

thomaseizinger · 2021-11-20T20:26:12Z

src/util/schnorr.rs

Could early return here and get rid of the else block. Would reduce another indentation level of the code below.

Kixunil · 2021-11-21T12:41:16Z

src/util/ecdsa.rs

let (hash_ty, sig) = sl.split_last().ok_or(EcdsaSigError::InvalidEcdsaSig)?;
and get rid of indentation

src/util/schnorr.rs

Kixunil · 2021-11-21T12:45:04Z

src/util/schnorr.rs

I'm not convinced this helps much. Most people would probably blindly try ? and thus start implicitly relying on it.

This is how it is done throughout the library. I never really thought if this was useful or not, but have been mechanically adding it whenever I from From error impl

dr-orlovsky

re-ACK with two small improvements since needs rebase anyway

src/util/ecdsa.rs

src/util/schnorr.rs

sanket1729 · 2021-11-23T19:18:58Z

Rebased

dr-orlovsky

ACK ace924e1f7095ea3d5f8d366e43dbc1dd862519b

I think all of my comments are just nits on coding style. One really non-stylish thing is returning more clear error from a schnorr witness signature parser, but again I would not postpone further taproot progress just because of error handling UX.

src/util/ecdsa.rs

src/util/schnorr.rs

dr-orlovsky · 2021-11-23T23:10:00Z

src/util/sighash.rs

Nit: probably we create it from a byte value, not u8. I.e. from_byte may be more "consensus-style"

dr-orlovsky

utACK 681acd18a2f16eaca731da93e6f68fdeb28ea6fe

dr-orlovsky · 2021-12-02T19:16:17Z

Can pls @Kixunil or @RCasatta review this PR such we can move forward with one of these Taproot release blockers?

RCasatta

ACK 681acd18a2f16eaca731da93e6f68fdeb28ea6fe

left a nit, but it's so small I will merge anyway to avoid turnover time

src/util/ecdsa.rs

RCasatta · 2021-12-10T10:35:56Z

can't merge needs rebase

sanket1729 · 2021-12-12T06:48:51Z

Rebased again

RCasatta

utACK f60b14e50f2afa849c9019580de8721658d5a99d

dr-orlovsky · 2021-12-12T12:01:52Z

utACK f60b14e50f2afa849c9019580de8721658d5a99d

Was unable to test commit-wise since rust stable panics on one of the commits due to internal error.

Command that fails:
cargo +stable test --verbose --features=use-serde

Commit that fails: ed40f3d

RCasatta · 2021-12-12T14:18:33Z

Commit that fails: ed40f3d

I can't reproduce, and is it also out of this PR commit's?

sanket1729 · 2021-12-15T08:48:13Z

Rebased again after #721

RCasatta · 2021-12-15T09:37:42Z

ACK 5769744b74464be67f328e57b3f286a12703f6e6

Kixunil

Would've ACKed but I really don't like throwing away of error information. From my experience, it makes debugging harder.

If there's a rationale why throw it away I'd like to know.

Kixunil · 2021-12-15T10:16:14Z

src/util/ecdsa.rs

Why throw out the errors?

The earlier versions of the PR did capture the underlying errors. I did this in order to

Avoid all but one unreachable error states in secp256k1::Error. In this scenario, I did check that only one variant of error was being returned in the corresponding secp functions.

It would have required to me have another return error type for empty signature. I don't know if the end-user really cares about this error type when the signature is empty or if the signature is malformed. I want to avoid creating unnecessary error enums that are not useful, but it makes matching with them downstream even harder.

I was recently convinced on rust discord by someone else that I should always capture because in case upstream secp changes error reporting, we might want to capture it. Changing it back to the way it was previously

src/util/schnorr.rs

src/util/ecdsa.rs

sanket1729 · 2021-12-15T14:29:41Z

@Kixunil captured the underlying error types.

Export Sigs/Sigerrors

Kixunil

ACK 8361129

Kixunil · 2021-12-15T15:05:12Z

src/util/ecdsa.rs

+        let (hash_ty, sig) = sl.split_last()
+            .ok_or(EcdsaSigError::EmptySignature)?;
+        let hash_ty = EcdsaSigHashType::from_u32_standard(*hash_ty as u32)
+            .map_err(|_| EcdsaSigError::NonStandardSigHashType(*hash_ty))?;


May be nicer to match on NonStandardSigHashType (map_err(|NonStandardSigHashType| EcdsaSigError::NonStandardSigHashType(*hash_ty)) to protect against possible future refactors but not a big deal.

RCasatta

ACK 8361129

3eea63e Re-export SigHashType in lib.rs (sanket1729) Pull request description: Using the latest version of rust-bitcoin master on rust-miniscript errors on bitcoin::SigHashType not found. In the original PR, I only renamed the export to ECDSASigHashType, but original re-export should also be there in lib.rs to avoid breaking changes downstream. Fixup to #702 Before this PR, ``` | 89 | required: bitcoin::SigHashType, | ^^^^^^^^^^^ not found in `bitcoin ``` After this PR, ``` warning: use of deprecated type alias `bitcoin::SigHashType`: Please use [`EcdsaSigHashType`] instead --> src/psbt/mod.rs:89:28 | 89 | required: bitcoin::SigHashType, ``` ACKs for top commit: apoelstra: ACK 3eea63e RCasatta: ACK 3eea63e Tree-SHA512: 9cb8683835828e316ffa782c2a249559b4e534aa251b61667f3512b4e872fd88c726615c626ed9abf2061d31dc815ee77bb3d064962c93d0e0befd722fc2ceeb

sanket1729 force-pushed the add_sig_types branch 2 times, most recently from 304e5ec to 75750a4 Compare November 16, 2021 00:31

sanket1729 mentioned this pull request Nov 16, 2021

Update BitcoinECsig and BitcoinSchnorrSig to rust-bitcoin native types rust-bitcoin/rust-miniscript#281

Closed

dr-orlovsky added this to the 0.28.0 milestone Nov 16, 2021

dr-orlovsky reviewed Nov 16, 2021

View reviewed changes

sanket1729 force-pushed the add_sig_types branch 2 times, most recently from 48f15e4 to d6bd475 Compare November 19, 2021 18:55

thomaseizinger reviewed Nov 20, 2021

View reviewed changes

Kixunil reviewed Nov 21, 2021

View reviewed changes

src/util/schnorr.rs Outdated Show resolved Hide resolved

Kixunil reviewed Nov 21, 2021

View reviewed changes

sanket1729 force-pushed the add_sig_types branch from d6bd475 to 72c3a56 Compare November 21, 2021 20:26

dr-orlovsky suggested changes Nov 23, 2021

View reviewed changes

src/util/ecdsa.rs Show resolved Hide resolved

src/util/schnorr.rs Outdated Show resolved Hide resolved

dr-orlovsky mentioned this pull request Nov 23, 2021

PSBT taproot spending signature type #671

Closed

sanket1729 force-pushed the add_sig_types branch from 72c3a56 to ace924e Compare November 23, 2021 19:16

dr-orlovsky previously approved these changes Nov 23, 2021

View reviewed changes

sanket1729 dismissed dr-orlovsky’s stale review via 681acd1 November 24, 2021 17:22

sanket1729 force-pushed the add_sig_types branch from ace924e to 681acd1 Compare November 24, 2021 17:22

dr-orlovsky previously approved these changes Nov 24, 2021

View reviewed changes

Kixunil added the one ack PRs that have one ACK, so one more can progress them label Dec 2, 2021

RCasatta previously approved these changes Dec 10, 2021

View reviewed changes

src/util/ecdsa.rs Outdated Show resolved Hide resolved

RCasatta added the waiting for author This can only progress if the author responds to a request. label Dec 10, 2021

sanket1729 dismissed stale reviews from RCasatta and dr-orlovsky via 04c0bc6 December 12, 2021 05:48

sanket1729 force-pushed the add_sig_types branch 2 times, most recently from 04c0bc6 to 3e693cd Compare December 12, 2021 05:56

sanket1729 force-pushed the add_sig_types branch from 04c0bc6 to f60b14e Compare December 12, 2021 06:48

RCasatta previously approved these changes Dec 12, 2021

View reviewed changes

sanket1729 dismissed RCasatta’s stale review via 78df9f4 December 15, 2021 08:47

sanket1729 force-pushed the add_sig_types branch from f60b14e to 78df9f4 Compare December 15, 2021 08:47

sanket1729 force-pushed the add_sig_types branch from 78df9f4 to 5769744 Compare December 15, 2021 08:51

sanket1729 requested review from Kixunil, RCasatta and dr-orlovsky December 15, 2021 08:51

Kixunil reviewed Dec 15, 2021

View reviewed changes

sanket1729 force-pushed the add_sig_types branch from 5769744 to dc9c201 Compare December 15, 2021 14:29

sanket1729 added 5 commits December 15, 2021 20:00

Add EcdsaSig

fa112a7

Rename sighash::SigHashType::SigHashType to SchnorrSigHashType

410e8bf

Add SchnorrSigHashType::from_u8

648b397

Rename existing SigHashType to EcdsaSigHashType

94cfe79

Add SchnorrSig type

8361129

Export Sigs/Sigerrors

sanket1729 force-pushed the add_sig_types branch from dc9c201 to 8361129 Compare December 15, 2021 14:31

Kixunil approved these changes Dec 15, 2021

View reviewed changes

RCasatta approved these changes Dec 15, 2021

View reviewed changes

RCasatta merged commit 970f574 into rust-bitcoin:master Dec 15, 2021

RCasatta mentioned this pull request Dec 15, 2021

New Witness struct to improve ser/de perfomance #672

Merged

sanket1729 mentioned this pull request Dec 25, 2021

Re-export SigHashType in lib.rs #749

Merged

sanket1729 mentioned this pull request Dec 28, 2021

Add support for taproot psbt fields BIP 371 #681

Merged

dr-orlovsky mentioned this pull request Jan 15, 2022

Bump version to v0.28 #785

Closed

Conversation

sanket1729 commented Nov 16, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

sanket1729 commented Nov 19, 2021

Uh oh!

thomaseizinger left a comment

Choose a reason for hiding this comment

Uh oh!

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!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sanket1729 Nov 21, 2021 • 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!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sanket1729 commented Nov 23, 2021

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Dec 2, 2021

Uh oh!

RCasatta left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

RCasatta commented Dec 10, 2021

Uh oh!

sanket1729 commented Dec 12, 2021

Uh oh!

RCasatta left a comment

Choose a reason for hiding this comment

Uh oh!

dr-orlovsky commented Dec 12, 2021

Uh oh!

RCasatta commented Dec 12, 2021

Uh oh!

sanket1729 commented Dec 15, 2021

Uh oh!

RCasatta commented Dec 15, 2021

Uh oh!

Kixunil left a comment

Choose a reason for hiding this comment

Uh oh!

sanket1729 commented Nov 16, 2021 •

edited

Loading

sanket1729 Nov 21, 2021 •

edited

Loading