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

[WIP]: Add bn256 #3795

Closed
wants to merge 1 commit into from
Closed

Conversation

0xforever9
Copy link

@0xforever9 0xforever9 commented Jan 11, 2023

@0xforever9 0xforever9 requested a review from tarakby as a code owner January 11, 2023 09:45
@tarakby
Copy link
Contributor

tarakby commented Jan 11, 2023

@4ever9 Thank you for the PR and your contribution!
I had a quick look at the code, but will need to have a deeper look.

Meanwhile, it would be great to add a PR description summarizing the code changes and especially the motivation behind adding BLS on bn256. In particular, I am curious what would be the use case of BLS with this curve, and if this use case isn't impacted by its security level being strictly smaller than 128 bits (more details here : https://moderncrypto.org/mail-archive/curves/2016/000740.html)
You may notice that the two libraries imported in the PR have mentioned it (golang crypto has deprecated the package, cloudflare only mentions the issue)

@0xforever9 0xforever9 closed this Feb 2, 2023
@0xforever9
Copy link
Author

0xforever9 commented Feb 2, 2023

@tarakby Thx for your reply. I am a developer from MAP Protocol. We have received a grant from dapper labs to build an cross-chain infrastructure for Flow.
Our grant info can be found here: onflow/developer-grants#47

Since then We have been working on the plan and have submitted this PR for Flow team to review.

@0xforever9 0xforever9 reopened this Feb 4, 2023
@tarakby
Copy link
Contributor

tarakby commented Feb 6, 2023

Thanks for the info.

  • after looking at the grant proposal, I see your protocol relies on curve bn254 (which is supported by EVM), while your PR implements bn256.
  • bn256 and bn254 both suffer from the security issue I mentioned above. I believe there are EIP discussions to include precompiles of other more secure curves. What does MAP protocol think about this? Are you planning any workaround?

@0xforever9
Copy link
Author

@tarakby
Thanks for your reply.

  1. bn254, bn256, and alt_bn128 are actually the same thing. To avoid misunderstandings, we will unify them as bn254.
  2. Our team is aware of the security issues related to bn254, but for the sake of ecosystem compatibility, we believe it is necessary to continue using it. As bls12-381 has not yet been widely adopted, many other ecosystem projects, including many Ethereum-based zk projects, still rely on bn254. Even when bls12-381 is introduced by Ethereum, it will take a significant amount of time for the broader EVM ecosystem to fully support it. Therefore, when we integrate with these ecosystems, we need to support bn254. In summary, bn254 is still widely used in many ecosystems, and we need to adapt and support it. However, as the ecosystem develops, we will upgrade to the more secure bls12-381.

@0xforever9 0xforever9 mentioned this pull request Mar 7, 2023
@0xforever9 0xforever9 reopened this Mar 13, 2023
@tarakby
Copy link
Contributor

tarakby commented Mar 30, 2023

Hi @4ever9, apologies for the late reply.

  1. Curve to be supported:

bn254, bn256, and alt_bn128 are actually the same thing. To avoid misunderstandings, we will unify them as bn254.

bn254 and bn256 are both belonging to the same BN family, however:

From my understanding, the MAP protocol design requires bn254, while this PR implements bn256.
Do you have references clarifying that bn254 and bn256 are actually the same curve? or that the MAP protocol can work with bn256 (although it is not supported by EVM precompile) ?

  1. Tools required to be exported by Cadence:
    I understand that the objective of this PR is to eventually to export zkSNARKs tools on Cadence (through future PRs). Is that correct?
    If yes, it would be great to list the exact operations we would like Cadence to support (for instance EC arithmetic, pairing..?). I'm asking because I see that the PR adds a support for BLS signature scheme on the new curve, but is that really needed for zkSNARK support?

  2. Security issue of bn254 and bn256

Our team is aware of the security issues related to bn254, but for the sake of ecosystem compatibility, we believe it is necessary to continue using it

Some platforms supported these curves when they were known to be secure enough. Once, the security reduction was discovered, these platforms did not deprecate the curves yet. I can understand that moving away from some tools may take time (even though I think security related topics should be addressed quickly).
However, the case of Flow and Cadence is about supporting a tool that is known to be below the minimum security in a fresh new platform. I think this only makes the overall ecosystem security and rigour below the acceptable level, and makes the ecosystem technical debt bigger (the more projects are built on these tools, the harder to deprecate them and replace them).
This is only my opinion of course and the community needs to decide on this. I'm reaching out to the team to see what's the best way to continue this discussion. I'll give you updates when I have more info.
Meanwhile, we can continue discussing the points (1) and (2) above.

@tarakby
Copy link
Contributor

tarakby commented Apr 27, 2023

Hi @4ever9, I'm getting back to you regarding the point (3) above. The Flow team thinks it makes sense to support the bn254 curve although it does not provide the standard minimum security, in order to remain compatible with the broader ecosystem that still relies on this curve. It makes sense for this to happen with the following conditions:

  • for better transparency with users, the documentation should state clearly that bn254 does not provide the standard minimum security.
  • If Cadence would support zkSNARK with bn254, it should also provide the same tools with a more secure alternative curve. This would allow users to rely on a more secure alternative if they don't have compatibility constraints tying them to bn254. We could factor the engineering effort to support zkSNARK with the 2 curves on Cadence.

Curious to hear your thoughts on this, and also on the points (1) and (2) from my previous message.

@tarakby
Copy link
Contributor

tarakby commented Aug 28, 2024

Closing since the PR is stale.
bn254 will be available on Flow via EVM from the Crescendo upgrade (Sept 4th, 2024), though not directly supported on Cadence.
bn254 is still not recommended to be used for its lower security level. BLS12-381 offers a closer security to 128 bits and should be used instead, the curve will be available soon on EVM.

@tarakby tarakby closed this Aug 28, 2024
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.

2 participants