feat(precompiles): optimize bn128 precompiles

[AllFi]

What does this PR do?
This PR optimizes BN128Addition, BN128Multiplication, and BN128Pairing precompiled contracts by using arkworks implementation of bn128 operations. This PR uses LibarkworksWrapper introduced in tronprotocol/zksnark-java-sdk#9.

Why are these changes required?

The native implementation of these precompiles is relatively slow. In general, this hinders on-chain zkSNARKs verification and, consequently, makes zk-based apps almost unfeasible (#4311). With this PR (or a similar approach), these applications become viable. This PR is necessary to resolve this issue.

It is important to note that these changes are not proposed to solve a specific problem of ZkBob but rather enable a whole class of applications and protocols based on elliptic curve cryptography and pairings in particular. The possibilities brought by ZKP are not limited to privacy focused protocols , some other examples are zk based auth for AA wallets, light clients, zk based bridges, computational integrity enforcing contracts etc.

This PR has been tested by:

• Unit Tests (test cases were copied from the go-ethereum)
• Manual Testing

Follow up

Extra details
I've added some benchmarks before (https://github.com/zkBob/java-tron/tree/bn128-native-bench) and after modifications (https://github.com/zkBob/java-tron/tree/bn128-arkworks-bench). The results can be found below.

The average time of operations before (Intel(R) Core(TM) i7-10750H CPU, 32 GB RAM):

BN128Addition: 66387 ns
BN128Multiplication: 3553350 ns
BN128Pairing (10 pairs): 101565419 ns

The average time of operations after:

BN128Addition: 11576 ns
BN128Multiplication: 181301 ns
BN128Pairing (10 pairs): 3285601 ns

Closes #5492

[tomatoishealthy]

There are some concerns here.

upgrade

If this optimization is not enabled at the specified block height or through a proposal, the related contract transaction results may behave inconsistently on different nodes due to differences in node upgrade times.

For example, some SRs have adopted the latest version, and other SRs(even fullnodes) have not been upgraded in time. At this time, if a user initiates a transaction involving these precompilations, there is a high probability that the transaction will be successfully executed on the upgraded SRs, but timeout on the non-upgraded SRs. This will cause a network fork.

Therefore, in theory, it should be ensured that this optimization is enabled on all nodes at the same time. There are currently two feasible solutions:

• option1: Enable this optimization at the specified block height
• option2: Enable this optimization through the proposal

Both require a mandatory upgrade, option1 seems simpler, but there is no guarantee that all nodes will run the latest version in advance. option2 is more formal, but requires more development and community coordination. Of course, we can also discuss whether there are other better ways. @lvs007 @Federico2014 @halibobo1205 @ss3344520

Another concern: fee.
Since this PR optimizes the execution time of bn128, do the related precompiles fees need to be adjusted accordingly? @yanghang8612

If all of you feel these concerns are complex and independent, we can also try to open a new issue for discussion.

[r0wdy1]

@tomatoishealthy, thanks for description, these concerns seem reasonable.
Apart from main net is it possible to make estimates about any public testnet deployments?
Thank you.

[halibobo1205]

@tomatoishealthy, thanks for description, these concerns seem reasonable. Apart from main net is it possible to make estimates about any public testnet deployments? Thank you.

@r0wdy1 I agree with the idea.

1. Confirm the final upgrade plan.
2. Private test network deployed and tested
3. Public test network deployed, fully tested and stabilized for a period of time

cc @tomatoishealthy @lvs007 @Federico2014 @ss3344520

[lvs007]

@tomatoishealthy, thanks for description, these concerns seem reasonable. Apart from main net is it possible to make estimates about any public testnet deployments? Thank you.

@r0wdy1 I agree with the idea.

1. Confirm the final upgrade plan.
2. Private test network deployed and tested
3. Public test network deployed, fully tested and stabilized for a period of time

cc @tomatoishealthy @lvs007 @Federico2014 @ss3344520

Yes, a detailed code review is also required

[tomatoishealthy]

Apart from main net is it possible to make estimates about any public testnet deployments?

@r0wdy1 Sorry for not replying to you in time, because there are some details that need to be confirmed

java-tron will release a non-mandatory upgrade version v4.7.3 in October (this issue states that v4.7.3 will be upgraded in Q4, which is actually October). After this version upgrade is completed, I think we can start to promote this optimization into the testing phase.

Although the specific time cannot be determined at present, if we can solve the problems mentioned above, there is a high probability that this optimization can be launched on the nile test network after October.

[tomatoishealthy]

For upgrade compatibility

I prefer option 1. This solution is simpler. It only needs to determine the effective block height and does not require SR voting to reach a consensus. Because this PR itself is a performance optimization, it is not like one that requires consensus to take effect.

In addition, no matter which solution we adopt, we need to retain the previous logic and use a way like if else to cooperate with optimization. The block height may need to be confirmed before upgrading and a new PR issued to update it

In addition, we also need to confirm: Should the code responsible for compatibility be submitted with a separate PR? @AllFi @halibobo1205 @lvs007 @Federico2014 @yanghang8612

Fee

I think the fee should be adjusted appropriately. After all, once optimized, the resource consumption of this instruction will be greatly reduced.
Fee adjustments should be implemented in the form of proposals. In addition, the specific proportion of fee adjustments also needs to be supported by statistical data. This may require opening an independent issue for separate discussion. What do you think? @yanghang8612

In principle: However, the priority of fee adjustment is not high, and it should not affect our overall progress of the upgrades.

[r0wdy1]

In addition, no matter which solution we adopt, we need to retain the previous logic and use a way like if else to cooperate with optimization. The block height may need to be confirmed before upgrading and a new PR issued to update it

The purpose of this is to avoid and necessity of simultaneous upgrade and let anyone to upgrade anytime before certain block height, right?
Do you expect help from our team with this task?

[tomatoishealthy]

Do you expect help from our team with this task?

With my pleasure, after the devs confirm whether to choose option 1 or option 2, I can start the follow-up work.

Now, we need to wait for this PR and the PR for the zksnark-java-sdk to be fully reviewed

[mumianhua]

the patch code coverage is lower than 80%, so the ci task is failed.
please add some tests to cover more added code !
https://app.codecov.io/github/tronprotocol/java-tron/commit/118b0f183b362c66641a9904ecca96675c4f2017

[maikReal]

@tomatoishealthy @halibobo1205 @lvs007 @mumianhua
Hi guys, I'm PM at zkBob! I'm trying to make a post on the Tron forum, but I don't have access to do so, because I'm new on the forum. May I ask you to help me and get access to write posts on the forum?

My nickname is @maikyman and the link to the profile

I want to make a small pre-announce for the Tron audience that we're working together on deploying the privacy ZKP protocol on the Tron network. I think they will be interested to hear about this 😌

[AllFi]

Hello, @mumianhua! I've added test cases with invalid data to cover most of the logic. The only code that is still uncovered is the handling of unsuccessful calls to libarkworksAddG1 and libarkworksMulG1. Since there is preliminary validation of the input by calling libarkworksG1IsValid these methods should not fail with the current implementation. Perhaps, we can consider removing redundant checks but I think there is no harm in keeping them just in case.

The CentOS build has two failed tests but it seems that I didn't change anything related to them. Should I investigate this further, or are they possibly just flaky tests?

[tomatoishealthy]

@tomatoishealthy @halibobo1205 @lvs007 @mumianhua Hi guys, I'm PM at zkBob! I'm trying to make a post on the Tron forum, but I don't have access to do so, because I'm new on the forum. May I ask you to help me and get access to write posts on the forum?

My nickname is @maikyman and the link to the profile

I want to make a small pre-announce for the Tron audience that we're working together on deploying the privacy ZKP protocol on the Tron network. I think they will be interested to hear about this 😌

The relevant person in charge has been notified and will notify you as soon as possible after access is opened. @maikReal

[SeanTD7]

It's a perfect

[r0wdy1]

@mumianhua @tomatoishealthy , is there anything else we can help you with to proceed with this?

[tomatoishealthy]

@mumianhua @tomatoishealthy , is there anything else we can help you with to proceed with this?

Sorry, some colleagues are celebrating the Chinese National Day holiday recently and will start working after October 7th. We can continue to move forward then.

[tomatoishealthy]

@r0wdy1, @AllFi

v4.7.3 is expected to be released at the end of this month. After that, we can immediately start processing the next mandatory upgrade which will include this optimization.

If all goes well, these PRs are expected to be merged next month, and we will immediately start developing the compatibility logic. The development of the compatibility logic will not take long. After both are completed, I think the feature can be merged into the test net and start testing.

Thank you very much for your contribution. Any changes will be notified here in time.

[tomatoishealthy]

The next step #5529

[srbaeza-ai]

Thus one

[srbaeza-ai]

• [ ]

[r0wdy1]

could you please elaborate?

[srbaeza-ai]

• @.

[r0wdy1]

could you please elaborate?

[CarlChaoCarl]

To make this solution feasible, it is essential to ensure the security and stability of third-party packages.

[lxcmyf]

@AllFi
Based on this, I have the following concerns:

1. I noticed that compile 'com.github.zkbob:zksnark-java-sdk:feature~arkworks_Alt_Bn128-SNAPSHOT' The static class LibarkworksJNI in the dependency uses JNI implementation, which may lead to potential risks such as memory management, buffer overflow, abnormal crash, code injection, etc. These risks have not been further verified, and it is not recommended to do so here
2. Some of the performance improvements you mentioned may be implemented in safer and more convenient ways, such as increasing timeout time, etc
3. The third-party dependency referenced by this PR has not been fully verified, and its security and stability are not yet clear

[r0wdy1]

@lxcmyf

I noticed that compile 'com.github.zkbob:zksnark-java-sdk:feature~arkworks_Alt_Bn128-SNAPSHOT' The static class LibarkworksJNI in the dependency uses JNI implementation, which may lead to potential risks such as memory management, buffer overflow, abnormal crash, code injection, etc. These risks have not been further verified, and it is not recommended to do so here

This is exactly the same approach that has already been implemented with shielded transactions more than 3 years ago. You can check out zksnark sdk repo
and see that it has rust imports from ZCash libraries literally for years now. The only problem with it is that it's too high level and ZCash Sapling specific, and thus it doesn't allow to build abstract snark-based protocols

Some of the performance improvements you mentioned may be implemented in safer and more convenient ways, such as increasing timeout time, etc

I'm afraid timeout increase is not a "performance improvement" , it's just a way to lower the bar. As you can see from pairing benchmarks the timeout would have to be increased by a factor of 30 to gain the same result. There are other factors like allowing to build more complex applications and being able to maintain this code further ( see next paragraph)

The third-party dependency referenced by this PR has not been fully verified, and its security and stability are not yet clear

Quite the contrary arkworks is very well-known and widely used library you can see the list projects that use it as dependency https://crates.io/crates/ark-ff/reverse_dependencies
(Solana , Starknet-rs , Penumbra, Sui by Mysten Labs etc)
Speaking about stability and security, current implementation of BN128 comes from a EthereumJ project , you can check it in the Copyright notice:
This project has been deprecated at least from 2020
So the only stability out there lies in inability to use this code: even if there was a bug or a security vulnerability nobody could notice it because those transaction would never be processed successfully.
Getting back to timeout increase I would like to reiterate that instead of taking a highly optimized and battle tested code you suggest to continue to use unoptimized EthereumJ code that was written in 2016 when a lot of today proof system didn't even exist ( Groth16 paper was published in 2016, ZCash launched only in Q4 2016), that has basically zero support and maintenance. When you increase the timeout and transaction can get through that would be the first time when we would be able to see all the edge cases in the wild.
I understand the overall intuition behind not changing what is not broken and "not changing horses in the midstream", but this occasion is slightly different because no one has ever verified that the old one was a horse at all

[r0wdy1]

@lxcmyf @tomatoishealthy @halibobo1205 looking forward for your feedback and updated plan on the issue: we should either proceed with this PR or reopen #4311 and make some plan for timeout increase.
We're very excited about launching our DApp on Tron and this problem is the last roadblock in our way

[tomatoishealthy]

@r0wdy1 Very impressive points.

@lxcmyf suggested increasing the transaction timeout. The focus is more on maintaining the stability of the network. After all, TRON carries huge value, and any small changes may bring huge losses. From this perspective, it may be a quick intermediate solution.

This PR is more of a long-term solution and can bring huge improvements to performance and user experience. From the perspective of long-term development, this plan is better than the above-mentioned plans. However, considering security and stability, this optimization requires comprehensive and sufficient testing.

Currently, v4.7.3 is in the grayscale release stage, and it is expected that it will take a few days to confirm the stability of this version. This window period can give everyone more time to think about these issues. Maybe we can reach a consensus in the near future. @AllFi @r0wdy1 @halibobo1205 @lxcmyf @lvs007 @yuekun0707

In addition, the development to provide compatibility for upgrades is not complicated. Once the final solution is determined, we should be able to immediately promote the subsequent process.