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.

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

Remove mentions of 32-byte slice from tweak APIs #453

Merged
merged 1 commit into from Jun 21, 2022

Conversation

Kixunil
Copy link
Collaborator

@Kixunil Kixunil commented Jun 21, 2022

These methods accept &Scalar, not slice and &Scalar already guarantees 32-bytes, so this failure case is impossible.

These methods accept `&Scalar`, not slice and `&Scalar` already guarantees 32-bytes, so this failure case is impossible.
Copy link
Member

@sanket1729 sanket1729 left a comment

Choose a reason for hiding this comment

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

ACK e612458.

This is a strict improvement. But our code documentation can be improved, which is a separate issue.

@@ -301,8 +300,7 @@ impl SecretKey {
///
/// # Errors
///
/// Returns an error if the resulting key would be invalid or if the tweak was not a 32-byte
/// length slice.
/// Returns an error if the resulting key would be invalid.
Copy link
Member

Choose a reason for hiding this comment

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

This can only error when Scalar is zero.

Copy link
Member

Choose a reason for hiding this comment

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

Same comment as above about mutating self.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don't think that's the case. The resulting key would be invalid if it's 0, so scalar being zero can cause error only if the key is already invalid which this library prevents. This should produce error though: priv_key.add_tweak(priv_key.negate().into())

Copy link
Member

Choose a reason for hiding this comment

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

This comment is for mul_tweak. Which happens only when the scalar is zero.

Copy link
Member

Choose a reason for hiding this comment

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

Anycase, these are unrelated to the PR itself. We can tackle this in separate issue

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Ah, annoying GitHub UI...

@@ -270,8 +270,7 @@ impl SecretKey {
///
/// # Errors
///
/// Returns an error if the resulting key would be invalid or if the tweak was not a 32-byte
/// length slice.
/// Returns an error if the resulting key would be invalid.
Copy link
Member

Choose a reason for hiding this comment

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

We can be more specific and say, returns an error only if the resultant key is zero. This only happens when Scalar is the negation of Secret Key.

The upstream documentation also states:

seckey will be set to some unspecified value if this function returns 0.

We should either 1) document this or 2) Make a copy and make sure self is not mutated here.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It's already copied. :)

Copy link
Member

Choose a reason for hiding this comment

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

This comment is for tweak_add. This only happens when Scalar is the negation of Secret Key.

Copy link
Member

Choose a reason for hiding this comment

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

If we let the other parameter be SecretKey, we can safely unwrap

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

If other is SecretKey we can unwrap for tweak_mul but not for tweak_add. To represent it in the type system:

// This should be sealed and associated items hidden, showing unsealed for simplicity
trait IntoScalar: Into<Scalar> {
    type TweakMulError;

    fn invalid_tweak_mul() -> Self::TweakMulError;
}

impl IntoScalar for Scalar {
    type TweakMulError = Error;

    fn invalid_tweak_mul() -> Self::TweakMulError {
        Error::InvalidTweak
    }
}


impl IntoScalar for SecretKey {
    type TweakMulError = core::convert::Infallible;

    fn invalid_tweak_mul() -> Self::TweakMulError {
        unreachable!(); // _unchecked if we're brave :)
    }
}

Then accept IntoScalar in tweak_mul()

@sanket1729
Copy link
Member

Opened #455.

Copy link
Member

@apoelstra apoelstra left a comment

Choose a reason for hiding this comment

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

ACK e612458

@apoelstra apoelstra merged commit 81e5801 into rust-bitcoin:master Jun 21, 2022
@Kixunil Kixunil deleted the patch-1 branch June 21, 2022 22:31
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants