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

[solana-program] Move bn254 logic from solana-program into a separate crate #1495

Merged
merged 16 commits into from
Jul 18, 2024

Conversation

samkim-crypto
Copy link

@samkim-crypto samkim-crypto commented May 27, 2024

Problem

The bn128 logic in solana-program is quite independent of the rest of the crate. Currently, the downstream projects that depend on only solana-program, but not bn128, will need to build with unnecessary bn128 logic, which is quite heavy.

Summary of Changes

Move bn128 logic into its separate crate.

Note: I named the crate to be solana-bn254. Bn254 is a more modern and descriptive name for bn128, which does not provide 128 bits of security any more. I will limit this PR to just the restructuring of the crate; I will update the contents inside the crate to use bn254 in a follow-up PR.

This is part of #983.

Fixes #

@samkim-crypto samkim-crypto force-pushed the bn254 branch 6 times, most recently from 007e749 to e8e727a Compare May 29, 2024 07:09
@samkim-crypto samkim-crypto force-pushed the bn254 branch 3 times, most recently from 81bfa4c to dd7d418 Compare June 6, 2024 00:07
@yihau
Copy link
Member

yihau commented Jun 7, 2024

hey, @dmakarov! do you think it is a good idea to bump this number again in this PR?

if ((solana_program_count > 20)); then

we got Regression of build redundancy error again 😢

@dmakarov
Copy link

dmakarov commented Jun 7, 2024

hey, @dmakarov! do you think it is a good idea to bump this number again in this PR?

if ((solana_program_count > 20)); then

we got Regression of build redundancy error again 😢

I think this check should be removed. It’s not useful because we don’t have a mechanism to keep the redundancy to a minimum.

@samkim-crypto
Copy link
Author

Hm, okay, then since PR is a couple weeks old now, let me bump the threshold this time to satisfy the CI 🙏 . Let's discuss whether we want to remove the redundancy build check on a follow-up.

@@ -471,7 +471,6 @@ extern crate self as solana_program;

pub mod account_info;
pub mod address_lookup_table;
pub mod alt_bn128;
Copy link

Choose a reason for hiding this comment

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

Um, what about gentle deprecation?
Maybe an alias to the new crate?

Copy link
Author

Choose a reason for hiding this comment

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

Oh right, good idea 😅

Copy link
Author

Choose a reason for hiding this comment

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

Actually, it seems difficult to import the new crate in solana-program due to circular dependency. It seems best to just leave the original logic in solana-program and add deprecation warnings pointing to the new crate. Wdyt?

Choose a reason for hiding this comment

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

When #1827 lands, the circular dependency can be avoided by just moving the required syscall definitions from solana-program to solana-bn254

Choose a reason for hiding this comment

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

#1827 has landed so you can just move the define_syscall! invocation and re-export the syscall like in #1656

@samkim-crypto samkim-crypto force-pushed the bn254 branch 3 times, most recently from 79c1597 to c90ab5c Compare July 17, 2024 02:41
@samkim-crypto samkim-crypto requested a review from Lichtso July 17, 2024 07:29
Lichtso
Lichtso previously approved these changes Jul 17, 2024
@samkim-crypto samkim-crypto merged commit c54d840 into anza-xyz:master Jul 18, 2024
53 checks passed
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.

5 participants