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

perf: allow skipping public key verification #53

Merged
merged 4 commits into from
Jan 18, 2023
Merged

perf: allow skipping public key verification #53

merged 4 commits into from
Jan 18, 2023

Conversation

kewde
Copy link
Contributor

@kewde kewde commented Jan 5, 2023 •
edited
Loading

The public key verification is rather slower in a pure JS mode. This PR introduces an argument skipVerification to fromExtendedKey() to allow skipping the verification.$

builds on #52

I did some quick benchmarks for thisPR.

elliptic with BN.js: 8x performance increase with skipVerification set to true.

HDKey.fromExtendedKey(key) x 4,712 ops/sec ±4.69% (75 runs sampled)
HDKey.fromExtendedKey(key, null, true) x 39,627 ops/sec ±4.94% (71 runs sampled)
Fastest is HDKey.fromExtendedKey(key, null, true)

This allows fromExtendedKey to run just as quick as the native performance:

HDKey.fromExtendedKey(key) x 30,868 ops/sec ±4.71% (77 runs sampled)
HDKey.fromExtendedKey(key, null, true) x 38,105 ops/sec ±5.65% (67 runs sampled)
Fastest is HDKey.fromExtendedKey(key, null, true)

@kewde
Copy link
Contributor Author

kewde commented Jan 6, 2023

@RyanZim

@RyanZim
Copy link
Member

RyanZim commented Jan 6, 2023

#52 is merged; please rebase.

@kewde kewde force-pushed the kewde/allow-unsafe-import branch from b108dab to df1cabb Compare January 6, 2023 21:16
@kewde
Copy link
Contributor Author

kewde commented Jan 6, 2023

@RyanZim rebased

RyanZim
RyanZim reviewed Jan 9, 2023
View reviewed changes
@kewde kewde requested a review from RyanZim January 13, 2023 10:09
RyanZim
RyanZim approved these changes Jan 13, 2023
View reviewed changes
Copy link
Member

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

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

utACK; code looks good. Perhaps we should have a unit test to at least exercise this code path?

@kewde
Copy link
Contributor Author

kewde commented Jan 14, 2023 •
edited
Loading

@RyanZim I think there's already a unit test included in the PR that covers it.

@RyanZim
Copy link
Member

RyanZim commented Jan 14, 2023

🤦‍♂️ I had seen that earlier, but somehow missed it; must have been just reviewing a later commit.

RyanZim
RyanZim approved these changes Jan 14, 2023
View reviewed changes
Copy link
Member

@RyanZim RyanZim left a comment

Choose a reason for hiding this comment

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

utACK

@RyanZim RyanZim merged commit 0eec10d into cryptocoinjs:master Jan 18, 2023
@RyanZim
Copy link
Member

RyanZim commented Jan 18, 2023

Published in v2.1.0 🎉

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RyanZim RyanZim RyanZim approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@kewde @RyanZim