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

Serialized signture improvements #658

Merged

Conversation

Kixunil
Copy link
Collaborator

@Kixunil Kixunil commented Oct 31, 2023

This deprecates methods returning constants and impls a few traits.

These functions always return the same value so are not useful and
actually calling them is a red flag.
These traits were missing and could be useful if e.g. one wants to store
serialized signatures in a set/map and access them using `[u8]`.
Converting signature to serialized signature and back is natural, so the
conversion traits should be implemented.
@Kixunil Kixunil force-pushed the serialized-signture-improvements branch from 4c845c7 to 62c839c Compare October 31, 2023 14:07
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 62c839c

@apoelstra apoelstra merged commit fe2905d into rust-bitcoin:master Oct 31, 2023
20 checks passed
@Kixunil Kixunil deleted the serialized-signture-improvements branch October 31, 2023 16:31
@@ -84,6 +127,7 @@ impl SerializedSignature {
}

/// Get the capacity of the underlying data buffer.
#[deprecated = "This always returns 72"]
Copy link
Member

Choose a reason for hiding this comment

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

If this is true why is the buffer size const called MAX_LEN? Also, if it is always 72 why do we even have the len field?

Copy link
Member

Choose a reason for hiding this comment

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

FTR I didn't look up the rules of sigs right now, and I don't knew them off the top of my head, I'm just looking at the code.

Copy link
Member

Choose a reason for hiding this comment

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

This is related to #659, reviewing that PR made me wonder why we do equality/ord comparison on the bytes not covered by the len field.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Don't confuse self.data.len() (which is MAX_LEN because of array) with self.len :)

why we do equality/ord comparison on the bytes not covered by the len field.

I'm not sure what you mean. The array only contains valid data up to len so we compare only those. The rest may be considered padding. (If we wanted to be crazy efficient we would use MaybeUninit<u8> instead.)

Copy link
Member

Choose a reason for hiding this comment

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

Ignore me, I don't know what code I was looking at yesterday, the **self == **other seemed totally new to me just now, I was baffled for a minute till I saw the Deref impl.

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