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(basic_bitcoin): cache ecdsa #1024

Merged
merged 1 commit into from
Oct 21, 2024
Merged

Conversation

dantol29
Copy link
Contributor

Overview
Why do we need this feature? What are we trying to accomplish?

The current process for retrieving a public key (via schnorr_public_key or ecdsa_public_key) takes approximately 9 seconds each time a user requests an address. By storing public keys in a static variable, I want to improve performance and minimize cycle burn rate in the canister. It can serve as a good example for developers.

Requirements
What requirements are necessary to consider this problem solved?

Public keys should be stored in a static variable, allowing retrieval without calling the key generation functions every time.

Considered Solutions
What solutions were considered?

thread_local storage with static variables that do persist across calls, but not across updates.

Recommended Solution
What solution are you recommending? Why?

Storing public keys in a static variable with HashMap for different derivation_paths

Considerations
What impact will this change have on security, performance, users (e.g. breaking changes) or the team (e.g. maintenance costs)?

This solution will significantly improve performance and lower cycle burn rate

@sa-github-api
Copy link
Collaborator

Dear @dantol29,

In order to potentially merge your code in this open-source repository and therefore proceed with your contribution, we need to have your approval on DFINITY's CLA.

If you decide to agree with it, please visit this issue and read the instructions there. Once you have signed it, re-trigger the workflow on this PR to see if your code can be merged.

— The DFINITY Foundation

@dantol29 dantol29 force-pushed the cache-ecdsa branch 5 times, most recently from bee78b4 to c586ea0 Compare October 16, 2024 14:16
Copy link
Contributor

@altkdf altkdf left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM!

@altkdf
Copy link
Contributor

altkdf commented Oct 21, 2024

@dantol29 I believe that the failing darwin job is something unrelated and has been fixed recently #1025. So the CI should become green if you merge master.

@dantol29 dantol29 requested review from a team as code owners October 21, 2024 14:48
@altkdf
Copy link
Contributor

altkdf commented Oct 21, 2024

github seems confused about the last commits, maybe git checkout cache-ecdsa && git merge master would help?

@dantol29 dantol29 force-pushed the cache-ecdsa branch 3 times, most recently from 19e9b69 to c586ea0 Compare October 21, 2024 15:26
@dantol29
Copy link
Contributor Author

github seems confused about the last commits, maybe git checkout cache-ecdsa && git merge master would help?

I removed garbage from commit history. Now should be fine

@altkdf altkdf merged commit 94b2d62 into dfinity:master Oct 21, 2024
5 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants