Re-enable signature health checks with a faster library #1820
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Yup! |
|
It seems like these CI failures are from the tests introduced in #1804. I'm not really sure why I'm only seeing it here though. I'm going to try and isolate that problem. |
|
@rehandalal I think this would work now, if you want to review it. |
bors bot
added a commit
that referenced
this pull request
Apr 1, 2019
1820: Re-enable signature health checks with a faster library r=rehandalal a=mythmon As a reminder, we disabled these checks because they were too slow. The [library](https://pypi.org/project/ecdsa/) we were using to validate signatures is a pure Python library, and is not focused on speed. I added [a new library](https://pypi.org/project/fastecdsa/) that is more focused on speed, though it includes C dependencies, which might make our life harder. The new library is significantly less friendly, as well. I had to jump through some more hoops to actually use it, and I actually still use the slow library to decode the signature into the format expected by the new one. I think that's probably fine for now, but I could work on removing it entirely if it's important. To test the speed increase, I created 100 signed recipes, re-enabled the test, and timed the signature verification check before and after this change. Before: ``` Checked signatures for 100 recipes in 0:00:20.166276 (4.96 checks/second) ``` After: ``` Checked signatures for 100 recipes in 0:00:00.913976 (109 checks/second) ``` I think an almost 22x speed up should make it ok to re-enable these checks. :smile: I haven't tested the Docker build yet. Given that the new library has a C component, I suspect it might fail to install on some systems. On my laptop, I had to install `gmp-devel` to build the library. I expect CI will catch this issue though, so I'm not worried. Co-authored-by: Mike Cooper <mythmon@gmail.com> Co-authored-by: Michael Cooper <mythmon@gmail.com>
Build succeeded |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
As a reminder, we disabled these checks because they were too slow. The library we were using to validate signatures is a pure Python library, and is not focused on speed. I added a new library that is more focused on speed, though it includes C dependencies, which might make our life harder.
The new library is significantly less friendly, as well. I had to jump through some more hoops to actually use it, and I actually still use the slow library to decode the signature into the format expected by the new one. I think that's probably fine for now, but I could work on removing it entirely if it's important.
To test the speed increase, I created 100 signed recipes, re-enabled the test, and timed the signature verification check before and after this change.
Before:
After:
I think an almost 22x speed up should make it ok to re-enable these checks. 😄
I haven't tested the Docker build yet. Given that the new library has a C component, I suspect it might fail to install on some systems. On my laptop, I had to install
gmp-develto build the library. I expect CI will catch this issue though, so I'm not worried.