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

feat(signature-validation): add ed25519 signature validation to twilight-util #2205

Open
wants to merge 18 commits into
base: main
Choose a base branch
from

Conversation

Monadic-Cat
Copy link

Implementing #2169. This PR is not complete, but I felt like making what I wrote just now public quickly so people know progress exists.

@github-actions github-actions bot added c-util Affects the util crate t-feature Addition of a new feature labels May 9, 2023
@Erk-
Copy link
Member

Erk- commented May 14, 2023

It would be nice if we could use a digest API for the signature so we don't have to allocate a vec for it. A quick look makes it seem like it is currently not possible but it may be possible in version 2 of ed2519-dalek. Though it may need a pr such as the below one.

dalek-cryptography/ed25519-dalek#196

@Monadic-Cat
Copy link
Author

It would be nice if we could use a digest API for the signature so we don't have to allocate a vec for it. A quick look makes it seem like it is currently not possible but it may be possible in version 2 of ed2519-dalek. Though it may need a pr such as the below one.

dalek-cryptography/ed25519-dalek#196

I agree, it'd be really nice to have that. So, I'm following up over there to see if we can get what we need for that added.

@Monadic-Cat
Copy link
Author

Okay, I've been a little busy for a bit, but I'm going to have a solid few days soon I can dedicate to this.

While it would be nice to have that feature on ed25519-dalek, I believe that this isn't actually blocked on that, since merging such an improvement would be entirely internal here- it wouldn't need to break this API.

So with that in mind, my goal is going to be getting this in a working state and ready to merge here, and later on pushing the stuff over at ed25519-dalek to completion.

@Monadic-Cat
Copy link
Author

Well, I never did get that time I was expecting to for this, but it turns out the remaining work is small. I've added extract_interaction(...), so all that remains should be documentation.

@Monadic-Cat
Copy link
Author

Side note: For avoiding the extra allocation, it seems we are waiting on one of these two PRs to be merged over at curve25519-dalek:

@Erk-
Copy link
Member

Erk- commented Jan 19, 2024

Should we get it merged and then have a issue tracking the version with less allocations in a issue?

@Monadic-Cat
Copy link
Author

Monadic-Cat commented Jan 19, 2024

Should we get it merged and then have a issue tracking the version with less allocations in a issue?

Yeah, that'd be for the best. I'm going to do some housecleaning on this PR today, and then it should be ready for final review and merge.

  • Rebase to fix Cargo.toml conflict with main branch
  • Move to latest release of ed25519-dalek
  • Document anything not yet documented
  • Fix miscellaneous broken stuff, :V

@Monadic-Cat
Copy link
Author

Monadic-Cat commented Jan 19, 2024

It's essentially done at this point, but I'd like to suggest doing additional feature gates to support someone using only check_signature() and not getting an unused serde_json dep pulled in.

@Monadic-Cat Monadic-Cat force-pushed the feat-signature-validation branch 2 times, most recently from 44d726f to ff22ff0 Compare January 19, 2024 20:39
@Monadic-Cat Monadic-Cat force-pushed the feat-signature-validation branch 2 times, most recently from 620cbfd to d3f3f85 Compare April 18, 2024 17:50
@Monadic-Cat
Copy link
Author

Rebased to fix conflict with updates to the main branch (in particular, in the examples directory) that appeared after I made this PR.

@Monadic-Cat Monadic-Cat marked this pull request as ready for review April 18, 2024 18:05
Copy link
Member

@Erk- Erk- left a comment

Choose a reason for hiding this comment

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

I think it looks good, only a few small nits that I would like to have changed.

twilight-util/Cargo.toml Show resolved Hide resolved
twilight-util/Cargo.toml Outdated Show resolved Hide resolved
twilight-util/src/signature_validation.rs Outdated Show resolved Hide resolved
twilight-util/src/signature_validation.rs Outdated Show resolved Hide resolved
@Monadic-Cat
Copy link
Author

Added the last doc comment required by your Clippy configuration. But I have no idea what's making these tests fail.

@Erk-
Copy link
Member

Erk- commented Apr 21, 2024

Added the last doc comment required by your Clippy configuration. But I have no idea what's making these tests fail.

They fail because of the issue solved here: #2326 so you can just look away from those.


/// Extracting the body of an Interaction failed.
#[cfg(feature = "signature-validation-extract-interaction")]
pub enum ExtractFailure {
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this have Debug derived on it? All the other error structs do. Ditto Display and Error.

Copy link
Member

@vilgotf vilgotf left a comment

Choose a reason for hiding this comment

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

There's a few formatting and documentation issues too that I've not commented on that I'll cover in the next round of reviews. Sorry for letting this linger.

twilight-util/src/signature_validation.rs Outdated Show resolved Hide resolved
twilight-util/src/signature_validation.rs Outdated Show resolved Hide resolved
twilight-util/src/signature_validation.rs Outdated Show resolved Hide resolved
twilight-util/src/signature_validation.rs Show resolved Hide resolved
twilight-util/src/signature_validation.rs Outdated Show resolved Hide resolved
@Monadic-Cat
Copy link
Author

There's a few formatting and documentation issues too that I've not commented on that I'll cover in the next round of reviews. Sorry for letting this linger.

...Next round of reviews? I'd rather be able to churn through all your change requests in one session. Mind if I just wait until you're done?

@vilgotf
Copy link
Member

vilgotf commented Jun 2, 2024

...Next round of reviews? I'd rather be able to churn through all your change requests in one session. Mind if I just wait until you're done?

The next round will just be documentation and formatting pointers (see CONTRIBUTING.md), if we stick with my changes. Formatting doesn't matter that much until the API is finalized.

@Monadic-Cat
Copy link
Author

I should state this publicly. I am no longer going to be working on this PR.

The maintainers are invited to edit and bikeshed it to completion. This PR has the "Allow edits by maintainers" option checked.

If no one does anything, I'm going to change gears and build a separate crate.

@Erk- Erk- requested a review from vilgotf June 15, 2024 16:13
@Erk- Erk- force-pushed the feat-signature-validation branch from e51e9bf to 094684e Compare June 15, 2024 16:20
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
c-util Affects the util crate t-feature Addition of a new feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants