Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Improve errors #601

Closed
wants to merge 2 commits into from
Closed

Improve errors #601

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
645238f
Remove derives from error types
tcharding Apr 18, 2023
3caefd7
Refactor source method
tcharding Apr 18, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion examples/sign_verify_recovery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -29,6 +29,7 @@ fn sign_recovery<C: Signing>(
Ok(secp.sign_ecdsa_recoverable(&msg, &seckey))
}

#[allow(unused_variables)] // triggered by matches macro.
fn main() {
let secp = Secp256k1::new();

Expand All @@ -47,5 +48,5 @@ fn main() {

let (recovery_id, serialize_sig) = signature.serialize_compact();

assert_eq!(recover(&secp, msg, serialize_sig, recovery_id.to_i32() as u8), Ok(pubkey));
assert!(matches!(recover(&secp, msg, serialize_sig, recovery_id.to_i32() as u8), Ok(pubkey)));
}
4 changes: 2 additions & 2 deletions src/ecdsa/mod.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -375,10 +375,10 @@ impl<C: Verification> Secp256k1<C> {
/// #
/// let message = Message::from_slice(&[0xab; 32]).expect("32 bytes");
/// let sig = secp.sign_ecdsa(&message, &secret_key);
/// assert_eq!(secp.verify_ecdsa(&message, &sig, &public_key), Ok(()));
/// assert!(secp.verify_ecdsa(&message, &sig, &public_key).is_ok());
///
/// let message = Message::from_slice(&[0xcd; 32]).expect("32 bytes");
/// assert_eq!(secp.verify_ecdsa(&message, &sig, &public_key), Err(Error::IncorrectSignature));
/// assert!(matches!(secp.verify_ecdsa(&message, &sig, &public_key), Err(Error::IncorrectSignature)));
/// # }
/// ```
#[inline]
Expand Down
65 changes: 39 additions & 26 deletions src/ecdsa/recovery.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -234,6 +234,7 @@ mod tests {

#[test]
#[cfg(feature = "rand-std")]
#[allow(unused_variables)] // triggered by matches macro.
fn capabilities() {
let sign = Secp256k1::signing_only();
let vrfy = Secp256k1::verification_only();
Expand All @@ -253,8 +254,9 @@ mod tests {
assert!(vrfy.recover_ecdsa(&msg, &sigr).is_ok());
assert!(full.recover_ecdsa(&msg, &sigr).is_ok());

assert_eq!(vrfy.recover_ecdsa(&msg, &sigr), full.recover_ecdsa(&msg, &sigr));
assert_eq!(full.recover_ecdsa(&msg, &sigr), Ok(pk));
let vrfy_res = vrfy.recover_ecdsa(&msg, &sigr);
let full_res = full.recover_ecdsa(&msg, &sigr);
assert!(matches!(vrfy_res, full_res));
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In 645238f:

This is not correct -- you can't use matches! on two variables like this; the result is to bind vrfy_res to full_res and then check that the binding succeeded (which it always will because there are no further restrictions). See https://stackoverflow.com/questions/72537643/apparent-unused-variable-in-match-statement

In general I'm a bit suspicious of these allow(unused_variables) lines because I think they're masking similar mistakes .... and in retrospect we should revisit what's going on in rust-bitcoin with these.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ok, I need to read the matches code then before using it willy-nilly. Just had a thought, perhaps we could do this on error types

#[derive(Debug)]
#[non_exhaustive]
#[allow(missing_copy_implementations)]
#[cfg_attr(test, derive(Copy, PartialEq, Eq, Clone))]

Then all the test changes in this PR are not required and matches! use goes away.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why are we removing PartialEq in secp256k1? The biggest reason to remove it is that we may add std::io::Error inside one day but I don't expect this to happen in this crate. Same with Clone. I'd avoid Copy since adding something that allocates is quite plausible.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, that's a good point -- I had half-forgotten that the reason for "error types don't impl any traits" is that io::Error tends to poison our types. But you're right that we're extremely unlikely to ever have I/O errors from this pure-math crate.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh, I thought that the "great error cleanup", across the org, meant that all errors should only derive Debug? Are you guys saying that for every error type I/we have to think about what traits to derive? That sounds like unnecessary work, can't we just decide on a minimum set and use that (which is what I thought we did and how we got to only deriving Debug)?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just lazy typing by me, I should have written PartialEq/Eq :)

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh I didn't type it, lazy typing by @Kixunil (I assume) - double smiley face.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not necessarily. We may avoid promising Eq to avoid future breakage like what happened with LockTime. But I'm not entirely sure about this one, there's a good chance Eq is just fine.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The LockTime breakage was about Ord. Having a type that is PartialEq but not Eq is a pretty obscure use-case, and I'm doubtful that crypto errors will meet it.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Indeed, I'm very open to having Eq.

}

#[test]
Expand All @@ -267,6 +269,7 @@ mod tests {
#[cfg(not(fuzzing))] // fixed sig vectors can't work with fuzz-sigs
#[cfg(feature = "rand-std")]
#[rustfmt::skip]
#[allow(unused_variables)] // triggered by matches macro.
fn sign() {
let mut s = Secp256k1::new();
s.randomize(&mut rand::thread_rng());
Expand All @@ -276,22 +279,27 @@ mod tests {

let sig = s.sign_ecdsa_recoverable(&msg, &sk);

assert_eq!(Ok(sig), RecoverableSignature::from_compact(&[
0x66, 0x73, 0xff, 0xad, 0x21, 0x47, 0x74, 0x1f,
0x04, 0x77, 0x2b, 0x6f, 0x92, 0x1f, 0x0b, 0xa6,
0xaf, 0x0c, 0x1e, 0x77, 0xfc, 0x43, 0x9e, 0x65,
0xc3, 0x6d, 0xed, 0xf4, 0x09, 0x2e, 0x88, 0x98,
0x4c, 0x1a, 0x97, 0x16, 0x52, 0xe0, 0xad, 0xa8,
0x80, 0x12, 0x0e, 0xf8, 0x02, 0x5e, 0x70, 0x9f,
0xff, 0x20, 0x80, 0xc4, 0xa3, 0x9a, 0xae, 0x06,
0x8d, 0x12, 0xee, 0xd0, 0x09, 0xb6, 0x8c, 0x89],
RecoveryId(1)))
let want = RecoverableSignature::from_compact(
&[
0x66, 0x73, 0xff, 0xad, 0x21, 0x47, 0x74, 0x1f,
0x04, 0x77, 0x2b, 0x6f, 0x92, 0x1f, 0x0b, 0xa6,
0xaf, 0x0c, 0x1e, 0x77, 0xfc, 0x43, 0x9e, 0x65,
0xc3, 0x6d, 0xed, 0xf4, 0x09, 0x2e, 0x88, 0x98,
0x4c, 0x1a, 0x97, 0x16, 0x52, 0xe0, 0xad, 0xa8,
0x80, 0x12, 0x0e, 0xf8, 0x02, 0x5e, 0x70, 0x9f,
0xff, 0x20, 0x80, 0xc4, 0xa3, 0x9a, 0xae, 0x06,
0x8d, 0x12, 0xee, 0xd0, 0x09, 0xb6, 0x8c, 0x89
],
RecoveryId(1)
).unwrap();
assert!(matches!(sig, want));
}

#[test]
#[cfg(not(fuzzing))] // fixed sig vectors can't work with fuzz-sigs
#[cfg(feature = "rand-std")]
#[rustfmt::skip]
#[allow(unused_variables)] // triggered by matches macro.
fn sign_with_noncedata() {
let mut s = Secp256k1::new();
s.randomize(&mut rand::thread_rng());
Expand All @@ -302,16 +310,21 @@ mod tests {

let sig = s.sign_ecdsa_recoverable_with_noncedata(&msg, &sk, &noncedata);

assert_eq!(Ok(sig), RecoverableSignature::from_compact(&[
0xb5, 0x0b, 0xb6, 0x79, 0x5f, 0x31, 0x74, 0x8a,
0x4d, 0x37, 0xc3, 0xa9, 0x7e, 0xbd, 0x06, 0xa2,
0x2e, 0xa3, 0x37, 0x71, 0x04, 0x0f, 0x5c, 0x05,
0xd6, 0xe2, 0xbb, 0x2d, 0x38, 0xc6, 0x22, 0x7c,
0x34, 0x3b, 0x66, 0x59, 0xdb, 0x96, 0x99, 0x59,
0xd9, 0xfd, 0xdb, 0x44, 0xbd, 0x0d, 0xd9, 0xb9,
0xdd, 0x47, 0x66, 0x6a, 0xb5, 0x28, 0x71, 0x90,
0x1d, 0x17, 0x61, 0xeb, 0x82, 0xec, 0x87, 0x22],
RecoveryId(0)))
let want = RecoverableSignature::from_compact(
&[
0xb5, 0x0b, 0xb6, 0x79, 0x5f, 0x31, 0x74, 0x8a,
0x4d, 0x37, 0xc3, 0xa9, 0x7e, 0xbd, 0x06, 0xa2,
0x2e, 0xa3, 0x37, 0x71, 0x04, 0x0f, 0x5c, 0x05,
0xd6, 0xe2, 0xbb, 0x2d, 0x38, 0xc6, 0x22, 0x7c,
0x34, 0x3b, 0x66, 0x59, 0xdb, 0x96, 0x99, 0x59,
0xd9, 0xfd, 0xdb, 0x44, 0xbd, 0x0d, 0xd9, 0xb9,
0xdd, 0x47, 0x66, 0x6a, 0xb5, 0x28, 0x71, 0x90,
0x1d, 0x17, 0x61, 0xeb, 0x82, 0xec, 0x87, 0x22
],
RecoveryId(0)
).unwrap();

assert!(matches!(sig, want));
}

#[test]
Expand All @@ -330,7 +343,7 @@ mod tests {

let msg = crate::random_32_bytes(&mut rand::thread_rng());
let msg = Message::from_slice(&msg).unwrap();
assert_eq!(s.verify_ecdsa(&msg, &sig, &pk), Err(Error::IncorrectSignature));
assert!(matches!(s.verify_ecdsa(&msg, &sig, &pk), Err(Error::IncorrectSignature)));

let recovered_key = s.recover_ecdsa(&msg, &sigr).unwrap();
assert!(recovered_key != pk);
Expand All @@ -349,7 +362,7 @@ mod tests {

let sig = s.sign_ecdsa_recoverable(&msg, &sk);

assert_eq!(s.recover_ecdsa(&msg, &sig), Ok(pk));
assert_eq!(s.recover_ecdsa(&msg, &sig).unwrap(), pk);
}

#[test]
Expand All @@ -367,7 +380,7 @@ mod tests {

let sig = s.sign_ecdsa_recoverable_with_noncedata(&msg, &sk, &noncedata);

assert_eq!(s.recover_ecdsa(&msg, &sig), Ok(pk));
assert_eq!(s.recover_ecdsa(&msg, &sig).unwrap(), pk);
}

#[test]
Expand All @@ -380,7 +393,7 @@ mod tests {

// Zero is not a valid sig
let sig = RecoverableSignature::from_compact(&[0; 64], RecoveryId(0)).unwrap();
assert_eq!(s.recover_ecdsa(&msg, &sig), Err(Error::InvalidSignature));
assert!(matches!(s.recover_ecdsa(&msg, &sig), Err(Error::InvalidSignature)));
// ...but 111..111 is
let sig = RecoverableSignature::from_compact(&[1; 64], RecoveryId(0)).unwrap();
assert!(s.recover_ecdsa(&msg, &sig).is_ok());
Expand Down
90 changes: 47 additions & 43 deletions src/key.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -1425,11 +1425,8 @@ impl BitXor for Parity {
}

/// Error returned when conversion from an integer to `Parity` fails.
//
// Note that we don't allow inspecting the value because we may change the type.
// Yes, this comment is intentionally NOT doc comment.
// Too many derives for compatibility with current Error type.
#[derive(Copy, Clone, Debug, Eq, PartialEq, Hash, Ord, PartialOrd)]
#[derive(Debug)]
#[allow(missing_copy_implementations)]
pub struct InvalidParityValue(i32);

impl fmt::Display for InvalidParityValue {
Expand Down Expand Up @@ -1576,16 +1573,16 @@ mod test {
#[test]
fn skey_from_slice() {
let sk = SecretKey::from_slice(&[1; 31]);
assert_eq!(sk, Err(InvalidSecretKey));
assert!(matches!(sk, Err(InvalidSecretKey)));

let sk = SecretKey::from_slice(&[1; 32]);
assert!(sk.is_ok());
}

#[test]
fn pubkey_from_slice() {
assert_eq!(PublicKey::from_slice(&[]), Err(InvalidPublicKey));
assert_eq!(PublicKey::from_slice(&[1, 2, 3]), Err(InvalidPublicKey));
assert!(matches!(PublicKey::from_slice(&[]), Err(InvalidPublicKey)));
assert!(matches!(PublicKey::from_slice(&[1, 2, 3]), Err(InvalidPublicKey)));

let uncompressed = PublicKey::from_slice(&[
4, 54, 57, 149, 239, 162, 148, 175, 246, 254, 239, 75, 154, 152, 10, 82, 234, 224, 85,
Expand All @@ -1604,13 +1601,14 @@ mod test {

#[test]
#[cfg(feature = "rand-std")]
#[allow(unused_variables)] // triggered by matches macro.
fn keypair_slice_round_trip() {
let s = Secp256k1::new();

let (sk1, pk1) = s.generate_keypair(&mut rand::thread_rng());
assert_eq!(SecretKey::from_slice(&sk1[..]), Ok(sk1));
assert_eq!(PublicKey::from_slice(&pk1.serialize()[..]), Ok(pk1));
assert_eq!(PublicKey::from_slice(&pk1.serialize_uncompressed()[..]), Ok(pk1));
assert!(matches!(SecretKey::from_slice(&sk1[..]), Ok(sk1)));
assert!(matches!(PublicKey::from_slice(&pk1.serialize()[..]), Ok(pk1)));
assert!(matches!(PublicKey::from_slice(&pk1.serialize_uncompressed()[..]), Ok(pk1)));
}

#[test]
Expand All @@ -1626,15 +1624,16 @@ mod test {

#[test]
#[rustfmt::skip]
#[allow(unused_variables)] // triggered by matches macro.
fn invalid_secret_key() {
// Zero
assert_eq!(SecretKey::from_slice(&[0; 32]), Err(InvalidSecretKey));
assert_eq!(
assert!(matches!(SecretKey::from_slice(&[0; 32]), Err(InvalidSecretKey)));
assert!(matches!(
SecretKey::from_str("0000000000000000000000000000000000000000000000000000000000000000"),
Err(InvalidSecretKey)
);
));
// -1
assert_eq!(SecretKey::from_slice(&[0xff; 32]), Err(InvalidSecretKey));
assert!(matches!(SecretKey::from_slice(&[0xff; 32]), Err(InvalidSecretKey)));
// Top of range
assert!(SecretKey::from_slice(&[
0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF, 0xFF,
Expand Down Expand Up @@ -1684,58 +1683,60 @@ mod test {
}

#[test]
#[allow(unused_variables)] // triggered by matches macro.
fn test_pubkey_from_bad_slice() {
// Bad sizes
assert_eq!(
assert!(matches!(
PublicKey::from_slice(&[0; constants::PUBLIC_KEY_SIZE - 1]),
Err(InvalidPublicKey)
);
assert_eq!(
));
assert!(matches!(
PublicKey::from_slice(&[0; constants::PUBLIC_KEY_SIZE + 1]),
Err(InvalidPublicKey)
);
assert_eq!(
));
assert!(matches!(
PublicKey::from_slice(&[0; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE - 1]),
Err(InvalidPublicKey)
);
assert_eq!(
));
assert!(matches!(
PublicKey::from_slice(&[0; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE + 1]),
Err(InvalidPublicKey)
);
));

// Bad parse
assert_eq!(
assert!(matches!(
PublicKey::from_slice(&[0xff; constants::UNCOMPRESSED_PUBLIC_KEY_SIZE]),
Err(InvalidPublicKey)
);
assert_eq!(
));
assert!(matches!(
PublicKey::from_slice(&[0x55; constants::PUBLIC_KEY_SIZE]),
Err(InvalidPublicKey)
);
assert_eq!(PublicKey::from_slice(&[]), Err(InvalidPublicKey));
));
assert!(matches!(PublicKey::from_slice(&[]), Err(InvalidPublicKey)));
}

#[test]
#[allow(unused_variables)] // triggered by matches macro.
fn test_seckey_from_bad_slice() {
// Bad sizes
assert_eq!(
assert!(matches!(
SecretKey::from_slice(&[0; constants::SECRET_KEY_SIZE - 1]),
Err(InvalidSecretKey)
);
assert_eq!(
));
assert!(matches!(
SecretKey::from_slice(&[0; constants::SECRET_KEY_SIZE + 1]),
Err(InvalidSecretKey)
);
));
// Bad parse
assert_eq!(
assert!(matches!(
SecretKey::from_slice(&[0xff; constants::SECRET_KEY_SIZE]),
Err(InvalidSecretKey)
);
assert_eq!(
));
assert!(matches!(
SecretKey::from_slice(&[0x00; constants::SECRET_KEY_SIZE]),
Err(InvalidSecretKey)
);
assert_eq!(SecretKey::from_slice(&[]), Err(InvalidSecretKey));
));
assert!(matches!(SecretKey::from_slice(&[]), Err(InvalidSecretKey)));
}

#[test]
Expand Down Expand Up @@ -1994,6 +1995,7 @@ mod test {

#[test]
#[cfg(not(fuzzing))]
#[allow(unused_variables)] // triggered by matches macro.
fn pubkey_combine() {
let compressed1 = PublicKey::from_slice(&hex!(
"0241cc121c419921942add6db6482fb36243faf83317c866d2a28d8c6d7089f7ba"
Expand All @@ -2012,12 +2014,13 @@ mod test {
assert!(sum1.is_ok());
let sum2 = compressed2.combine(&compressed1);
assert!(sum2.is_ok());
assert_eq!(sum1, sum2);
assert_eq!(sum1.unwrap(), exp_sum);
assert!(matches!(sum1, ref sum2));
assert!(matches!(sum1.unwrap(), exp_sum));
}

#[test]
#[cfg(not(fuzzing))]
#[allow(unused_variables)] // triggered by matches macro.
fn pubkey_combine_keys() {
let compressed1 = PublicKey::from_slice(&hex!(
"0241cc121c419921942add6db6482fb36243faf83317c866d2a28d8c6d7089f7ba"
Expand All @@ -2040,8 +2043,8 @@ mod test {
assert!(sum1.is_ok());
let sum2 = PublicKey::combine_keys(&[&compressed1, &compressed2, &compressed3]);
assert!(sum2.is_ok());
assert_eq!(sum1, sum2);
assert_eq!(sum1.unwrap(), exp_sum);
assert!(matches!(sum1, ref sum2));
assert!(matches!(sum1.unwrap(), exp_sum));
}

#[test]
Expand All @@ -2052,6 +2055,7 @@ mod test {

#[test]
#[cfg(feature = "rand-std")]
#[allow(unused_variables)] // triggered by matches macro.
fn create_pubkey_combine() {
let s = Secp256k1::new();

Expand All @@ -2062,11 +2066,11 @@ mod test {
assert!(sum1.is_ok());
let sum2 = pk2.combine(&pk1);
assert!(sum2.is_ok());
assert_eq!(sum1, sum2);
assert!(matches!(sum2, ref sum1));

let tweaked = sk1.add_tweak(&Scalar::from(sk2)).unwrap();
let sksum = PublicKey::from_secret_key(&s, &tweaked);
assert_eq!(Ok(sksum), sum1);
assert_eq!(sksum, sum1.unwrap());
}

#[cfg(not(fuzzing))]
Expand Down
Loading